Re: [PATCH] target/ppc: Fix regression in Radix MMU

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 10/28/22 15:36, Leandro Lupori wrote:
> Commit 47e83d9107 ended up unintentionally changing the control flow
> of ppc_radix64_process_scoped_xlate(). When guest_visible is false,
> it must not raise an exception, even if the radix configuration is
> not valid.
> 
> This regression prevented Linux boot in a nested environment with
> L1 using TCG and emulating KVM (cap-nested-hv=on) and L2 using
> KVM. L2 would hang on Linux's futex_init(), when it tested how a
> futex_atomic_cmpxchg_inatomic() handled a fault, because L1 would
> start a loop of trying to perform partition scoped translations
> and raising exceptions.
> 
> Fixes: 47e83d9107 ("target/ppc: Improve Radix xlate level validation")
> Reported-by: Victor Colombo <victor.colombo@eldorado.org.br>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
>   target/ppc/mmu-radix64.c | 28 ++++++++++++++++++++--------
>   1 file changed, 20 insertions(+), 8 deletions(-)

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

I'll queue this up in the pending pull request.


Thanks,

Daniel

> 
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 00f2e9fa2e..171379db69 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -238,6 +238,8 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType access_type,
>   
>   static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
>   {
> +    bool ret;
> +
>       /*
>        * Check if this is a valid level, according to POWER9 and POWER10
>        * Processor User's Manuals, sections 4.10.4.1 and 5.10.6.1, respectively:
> @@ -249,16 +251,24 @@ static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
>        */
>       switch (level) {
>       case 0:     /* Root Page Dir */
> -        return psize == 52 && nls == 13;
> +        ret = psize == 52 && nls == 13;
> +        break;
>       case 1:
>       case 2:
> -        return nls == 9;
> +        ret = nls == 9;
> +        break;
>       case 3:
> -        return nls == 9 || nls == 5;
> +        ret = nls == 9 || nls == 5;
> +        break;
>       default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "invalid radix level: %d\n", level);
> -        return false;
> +        ret = false;
> +    }
> +
> +    if (unlikely(!ret)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "invalid radix configuration: "
> +                      "level %d size %d nls %ld\n", level, psize, nls);
>       }
> +    return ret;
>   }
>   
>   static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
> @@ -519,11 +529,13 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
>   
>               if (!ppc_radix64_is_valid_level(level++, *g_page_size, nls)) {
>                   fault_cause |= DSISR_R_BADCONFIG;
> -                return 1;
> +                ret = 1;
> +            } else {
> +                ret = ppc_radix64_next_level(cs->as, eaddr & R_EADDR_MASK,
> +                                             &h_raddr, &nls, g_page_size,
> +                                             &pte, &fault_cause);
>               }
>   
> -            ret = ppc_radix64_next_level(cs->as, eaddr & R_EADDR_MASK, &h_raddr,
> -                                         &nls, g_page_size, &pte, &fault_cause);
>               if (ret) {
>                   /* No valid pte */
>                   if (guest_visible) {