Re: [PATCH] target/i386: fix direction of "32-bit MMU" test

[Michael Tokarev]

11.03.2024 10:58, Paolo Bonzini wrote:
> The low bit of MMU indices for x86 TCG indicates whether the processor is
> in 32-bit mode and therefore linear addresses have to be masked to 32 bits.
> However, the index was computed incorrectly, leading to possible conflicts
> in the TLB for any address above 4G.
>
> Analyzed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: b1661801c18 ("target/i386: Fix physical address truncation", 2024-02-28)
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2206
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Anyone can guess why this rather trivial and obviously correct patch causes 
segfaults
in a few tests in staging-7.2 - when run in tcg mode, namely:

  pxe-test
  migration-test
  boot-serial-test
  bios-tables-test
  vmgenid-test
  cdrom-test

When reverting this single commit from staging-7.2, it all works fine again.

This area in 7.2 is a bit twisty since I picked up previous commit (for which
this one is a fix), b1661801c18 "target/i386: Fix physical address truncation",
before a few others in the same place, especially before 90f641531c782c8
"target/i386: use separate MMU indexes for 32-bit accesses", so that picking
up the other commits has become more problematic due to differences in context.
Also, 7.2 lacks ace0c5fe59 "target/i386: Populate CPUClass.mmu_index" which 
moves
one of the places this commit touches to another file.  Hopefully I haven't
screwed up this place entirely and the prob is just some other missing commit.

I'm re-verifying my cherry-picks once again. Meanwhile, help from someone who
is more familiar with this place is definitely welcome.

BTW, Paolo, it looks like this patch is missing two tags - Reviewed-by Richard
and Tested-by Mark.  It's obviously too late to change that now, but it's
something to check for in the future.

Thanks,

/mjt

> ---
>   target/i386/cpu.h | 2 +-
>   target/i386/cpu.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 952174bb6f5..6b057380791 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2334,7 +2334,7 @@ static inline bool is_mmu_index_32(int mmu_index)
>   
>   static inline int cpu_mmu_index_kernel(CPUX86State *env)
>   {
> -    int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
> +    int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 0 : 1;
>       int mmu_index_base =
>           !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP64_IDX :
>           ((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK)) ? 
> MMU_KNOSMAP64_IDX : MMU_KSMAP64_IDX;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 2666ef38089..78524bc6073 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7735,7 +7735,7 @@ static bool x86_cpu_has_work(CPUState *cs)
>   static int x86_cpu_mmu_index(CPUState *cs, bool ifetch)
>   {
>       CPUX86State *env = cpu_env(cs);
> -    int mmu_index_32 = (env->hflags & HF_CS64_MASK) ? 1 : 0;
> +    int mmu_index_32 = (env->hflags & HF_CS64_MASK) ? 0 : 1;
>       int mmu_index_base =
>           (env->hflags & HF_CPL_MASK) == 3 ? MMU_USER64_IDX :
>           !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP64_IDX :