[PATCH v2 0/7] target/i386: Fix physical address masking bugs

[PATCH v2 0/7] target/i386: Fix physical address masking bugs

[Paolo Bonzini]

The address translation logic in get_physical_address() will currently
truncate physical addresses to 32 bits unless long mode is enabled.
This is incorrect when using physical address extensions (PAE) outside
of long mode, with the result that a 32-bit operating system using PAE
to access memory above 4G will experience undefined behaviour.
Instead, truncation must be applied to the linear address.  Because
this truncation is diffent between 32- and 64-bit modes, the series
opts to split 32- and 64-bit modes to separate MMU indices, which is
the simplest way to ensure that, for example, a kernel that runs both
32-bit and 64-bit programs looks up the correct address in the
(64-bit) page tables.

Furthermore, when inspecting the code I noticed that the A20 mask is
applied incorrectly when NPT is active.  The mask should not be applied
to the addresses that are looked up in the NPT, only to the physical
addresses.  Obviously no hypervisor is going to leave A20 masking on,
but the code actually becomes simpler so let's do it.

Patches 1 and 2 fix cases in which the addresses must be masked,
or overflow is otherwise invalid, for MMU_PHYS_IDX accesses.

Patches 3 and 4 introduce separate MMU indexes for 32- and 64-bit
accesses.

Patch 5 fixes the bug, by limiting the masking to the 32-bit MMU indexes.

Patches 6 and 7 further clean up the MMU functions to centralize
application of the A20 mask and fix bugs in the process.

Tested with kvm-unit-tests SVM tests and with an old 32-bit Debian image.

Paolo Bonzini (7):
  target/i386: mask high bits of CR3 in 32-bit mode
  target/i386: check validity of VMCB addresses
  target/i386: introduce function to query MMU indices
  target/i386: use separate MMU indexes for 32-bit accesses
  target/i386: Fix physical address truncation
  target/i386: remove unnecessary/wrong application of the A20 mask
  target/i386: leave the A20 bit set in the final NPT walk

 target/i386/cpu.h                    | 46 +++++++++++++++++++-----
 target/i386/cpu.c                    |  9 +++--
 target/i386/tcg/sysemu/excp_helper.c | 52 +++++++++++++---------------
 target/i386/tcg/sysemu/misc_helper.c |  3 ++
 target/i386/tcg/sysemu/svm_helper.c  | 27 +++++++++++----
 5 files changed, 92 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH v2 1/7] target/i386: mask high bits of CR3 in 32-bit mode

[Paolo Bonzini]

CR3 bits 63:32 are ignored in 32-bit mode (either legacy 2-level
paging or PAE paging).  Do this in mmu_translate() to remove
the last case where get_physical_address() meaningfully drops
the high bits of the address.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/excp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 5b86f439add..11126c860d4 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -238,7 +238,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
             /*
              * Page table level 3
              */
-            pte_addr = ((in->cr3 & ~0x1f) + ((addr >> 27) & 0x18)) & a20_mask;
+            pte_addr = ((in->cr3 & 0xffffffe0ULL) + ((addr >> 27) & 0x18)) & a20_mask;
             if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
@@ -306,7 +306,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
         /*
          * Page table level 2
          */
-        pte_addr = ((in->cr3 & ~0xfff) + ((addr >> 20) & 0xffc)) & a20_mask;
+        pte_addr = ((in->cr3 & 0xfffff000ULL) + ((addr >> 20) & 0xffc)) & a20_mask;
         if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
-- 
2.43.0

Re: [PATCH v2 1/7] target/i386: mask high bits of CR3 in 32-bit mode

[Zhao Liu]

On Fri, Feb 23, 2024 at 02:09:42PM +0100, Paolo Bonzini wrote:
> Date: Fri, 23 Feb 2024 14:09:42 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 1/7] target/i386: mask high bits of CR3 in 32-bit mode
> X-Mailer: git-send-email 2.43.0
> 
> CR3 bits 63:32 are ignored in 32-bit mode (either legacy 2-level
> paging or PAE paging).  Do this in mmu_translate() to remove
> the last case where get_physical_address() meaningfully drops
> the high bits of the address.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/tcg/sysemu/excp_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index 5b86f439add..11126c860d4 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -238,7 +238,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>              /*
>               * Page table level 3
>               */
> -            pte_addr = ((in->cr3 & ~0x1f) + ((addr >> 27) & 0x18)) & a20_mask;
> +            pte_addr = ((in->cr3 & 0xffffffe0ULL) + ((addr >> 27) & 0x18)) & a20_mask;
>              if (!ptw_translate(&pte_trans, pte_addr)) {
>                  return false;
>              }
> @@ -306,7 +306,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>          /*
>           * Page table level 2
>           */
> -        pte_addr = ((in->cr3 & ~0xfff) + ((addr >> 20) & 0xffc)) & a20_mask;
> +        pte_addr = ((in->cr3 & 0xfffff000ULL) + ((addr >> 20) & 0xffc)) & a20_mask;
>          if (!ptw_translate(&pte_trans, pte_addr)) {
>              return false;
>          }
> -- 
> 2.43.0
> 
> 

[PATCH v2 2/7] target/i386: check validity of VMCB addresses

[Paolo Bonzini]

MSR_VM_HSAVE_PA bits 0-11 are reserved, as are the bits above the
maximum physical address width of the processor.  Setting them to
1 causes a #GP (see "15.30.4 VM_HSAVE_PA MSR" in the AMD manual).

The same is true of VMCB addresses passed to VMRUN/VMLOAD/VMSAVE,
even though the manual is not clear on that.

Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/misc_helper.c |  3 +++
 target/i386/tcg/sysemu/svm_helper.c  | 27 +++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index 7de0a6e866d..edb7c3d8940 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -212,6 +212,9 @@ void helper_wrmsr(CPUX86State *env)
         tlb_flush(cs);
         break;
     case MSR_VM_HSAVE_PA:
+        if (val & (0xfff | ((~0ULL) << env_archcpu(env)->phys_bits))) {
+            goto error;
+        }
         env->vm_hsave = val;
         break;
 #ifdef TARGET_X86_64
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 32ff0dbb13c..5d6de2294fa 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -164,14 +164,19 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     uint64_t new_cr3;
     uint64_t new_cr4;
 
-    cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
-
     if (aflag == 2) {
         addr = env->regs[R_EAX];
     } else {
         addr = (uint32_t)env->regs[R_EAX];
     }
 
+    /* Exceptions are checked before the intercept.  */
+    if (addr & (0xfff | ((~0ULL) << env_archcpu(env)->phys_bits))) {
+        raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
+    }
+
+    cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
+
     qemu_log_mask(CPU_LOG_TB_IN_ASM, "vmrun! " TARGET_FMT_lx "\n", addr);
 
     env->vm_vmcb = addr;
@@ -463,14 +468,19 @@ void helper_vmload(CPUX86State *env, int aflag)
     int mmu_idx = MMU_PHYS_IDX;
     target_ulong addr;
 
-    cpu_svm_check_intercept_param(env, SVM_EXIT_VMLOAD, 0, GETPC());
-
     if (aflag == 2) {
         addr = env->regs[R_EAX];
     } else {
         addr = (uint32_t)env->regs[R_EAX];
     }
 
+    /* Exceptions are checked before the intercept.  */
+    if (addr & (0xfff | ((~0ULL) << env_archcpu(env)->phys_bits))) {
+        raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
+    }
+
+    cpu_svm_check_intercept_param(env, SVM_EXIT_VMLOAD, 0, GETPC());
+
     if (virtual_vm_load_save_enabled(env, SVM_EXIT_VMLOAD, GETPC())) {
         mmu_idx = MMU_NESTED_IDX;
     }
@@ -519,14 +529,19 @@ void helper_vmsave(CPUX86State *env, int aflag)
     int mmu_idx = MMU_PHYS_IDX;
     target_ulong addr;
 
-    cpu_svm_check_intercept_param(env, SVM_EXIT_VMSAVE, 0, GETPC());
-
     if (aflag == 2) {
         addr = env->regs[R_EAX];
     } else {
         addr = (uint32_t)env->regs[R_EAX];
     }
 
+    /* Exceptions are checked before the intercept.  */
+    if (addr & (0xfff | ((~0ULL) << env_archcpu(env)->phys_bits))) {
+        raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
+    }
+
+    cpu_svm_check_intercept_param(env, SVM_EXIT_VMSAVE, 0, GETPC());
+
     if (virtual_vm_load_save_enabled(env, SVM_EXIT_VMSAVE, GETPC())) {
         mmu_idx = MMU_NESTED_IDX;
     }
-- 
2.43.0

[PATCH v2 3/7] target/i386: introduce function to query MMU indices

[Paolo Bonzini]

Remove knowledge of specific MMU indexes (other than MMU_NESTED_IDX and
MMU_PHYS_IDX) from mmu_translate().  This will make it possible to split
32-bit and 64-bit MMU indexes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h                    | 10 ++++++++++
 target/i386/tcg/sysemu/excp_helper.c |  4 ++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index dfe43b82042..8c271ca62e5 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2305,6 +2305,16 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define MMU_NESTED_IDX  3
 #define MMU_PHYS_IDX    4
 
+static inline bool is_mmu_index_smap(int mmu_index)
+{
+    return mmu_index == MMU_KSMAP_IDX;
+}
+
+static inline bool is_mmu_index_user(int mmu_index)
+{
+    return mmu_index == MMU_USER_IDX;
+}
+
 static inline int cpu_mmu_index_kernel(CPUX86State *env)
 {
     return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 11126c860d4..a0d5ce39300 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -137,7 +137,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
     const int32_t a20_mask = x86_get_a20_mask(env);
     const target_ulong addr = in->addr;
     const int pg_mode = in->pg_mode;
-    const bool is_user = (in->mmu_idx == MMU_USER_IDX);
+    const bool is_user = is_mmu_index_user(in->mmu_idx);
     const MMUAccessType access_type = in->access_type;
     uint64_t ptep, pte, rsvd_mask;
     PTETranslate pte_trans = {
@@ -363,7 +363,7 @@ do_check_protect_pse36:
     }
 
     int prot = 0;
-    if (in->mmu_idx != MMU_KSMAP_IDX || !(ptep & PG_USER_MASK)) {
+    if (!is_mmu_index_smap(in->mmu_idx) || !(ptep & PG_USER_MASK)) {
         prot |= PAGE_READ;
         if ((ptep & PG_RW_MASK) || !(is_user || (pg_mode & PG_MODE_WP))) {
             prot |= PAGE_WRITE;
-- 
2.43.0

Re: [PATCH v2 3/7] target/i386: introduce function to query MMU indices

[Zhao Liu]

On Fri, Feb 23, 2024 at 02:09:44PM +0100, Paolo Bonzini wrote:
> Date: Fri, 23 Feb 2024 14:09:44 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 3/7] target/i386: introduce function to query MMU indices
> X-Mailer: git-send-email 2.43.0
> 
> Remove knowledge of specific MMU indexes (other than MMU_NESTED_IDX and
> MMU_PHYS_IDX) from mmu_translate().  This will make it possible to split
> 32-bit and 64-bit MMU indexes.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.h                    | 10 ++++++++++
>  target/i386/tcg/sysemu/excp_helper.c |  4 ++--
>  2 files changed, 12 insertions(+), 2 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index dfe43b82042..8c271ca62e5 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2305,6 +2305,16 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  #define MMU_NESTED_IDX  3
>  #define MMU_PHYS_IDX    4
>  
> +static inline bool is_mmu_index_smap(int mmu_index)
> +{
> +    return mmu_index == MMU_KSMAP_IDX;
> +}
> +
> +static inline bool is_mmu_index_user(int mmu_index)
> +{
> +    return mmu_index == MMU_USER_IDX;
> +}
> +
>  static inline int cpu_mmu_index_kernel(CPUX86State *env)
>  {
>      return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index 11126c860d4..a0d5ce39300 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -137,7 +137,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
>      const int32_t a20_mask = x86_get_a20_mask(env);
>      const target_ulong addr = in->addr;
>      const int pg_mode = in->pg_mode;
> -    const bool is_user = (in->mmu_idx == MMU_USER_IDX);
> +    const bool is_user = is_mmu_index_user(in->mmu_idx);
>      const MMUAccessType access_type = in->access_type;
>      uint64_t ptep, pte, rsvd_mask;
>      PTETranslate pte_trans = {
> @@ -363,7 +363,7 @@ do_check_protect_pse36:
>      }
>  
>      int prot = 0;
> -    if (in->mmu_idx != MMU_KSMAP_IDX || !(ptep & PG_USER_MASK)) {
> +    if (!is_mmu_index_smap(in->mmu_idx) || !(ptep & PG_USER_MASK)) {
>          prot |= PAGE_READ;
>          if ((ptep & PG_RW_MASK) || !(is_user || (pg_mode & PG_MODE_WP))) {
>              prot |= PAGE_WRITE;
> -- 
> 2.43.0
> 
> 

[PATCH v2 4/7] target/i386: use separate MMU indexes for 32-bit accesses

[Paolo Bonzini]

Accesses from a 32-bit environment (32-bit code segment for instruction
accesses, EFER.LMA==0 for processor accesses) have to mask away the
upper 32 bits of the address.  While a bit wasteful, the easiest way
to do so is to use separate MMU indexes.  These days, QEMU anyway is
compiled with a fixed value for NB_MMU_MODES.  Split MMU_USER_IDX,
MMU_KSMAP_IDX and MMU_KNOSMAP_IDX in two.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h                    | 34 ++++++++++++++++++++--------
 target/i386/cpu.c                    | 11 +++++----
 target/i386/tcg/sysemu/excp_helper.c |  3 ++-
 3 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 8c271ca62e5..ee4ad372021 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2299,27 +2299,41 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define cpu_list x86_cpu_list
 
 /* MMU modes definitions */
-#define MMU_KSMAP_IDX   0
-#define MMU_USER_IDX    1
-#define MMU_KNOSMAP_IDX 2
-#define MMU_NESTED_IDX  3
-#define MMU_PHYS_IDX    4
+#define MMU_KSMAP64_IDX    0
+#define MMU_KSMAP32_IDX    1
+#define MMU_USER64_IDX     2
+#define MMU_USER32_IDX     3
+#define MMU_KNOSMAP64_IDX  4
+#define MMU_KNOSMAP32_IDX  5
+#define MMU_PHYS_IDX       6
+#define MMU_NESTED_IDX     7
+
+#ifdef CONFIG_USER_ONLY
+#ifdef TARGET_X86_64
+#define MMU_USER_IDX MMU_USER64_IDX
+#else
+#define MMU_USER_IDX MMU_USER32_IDX
+#endif
+#endif
 
 static inline bool is_mmu_index_smap(int mmu_index)
 {
-    return mmu_index == MMU_KSMAP_IDX;
+    return (mmu_index & ~1) == MMU_KSMAP64_IDX;
 }
 
 static inline bool is_mmu_index_user(int mmu_index)
 {
-    return mmu_index == MMU_USER_IDX;
+    return (mmu_index & ~1) == MMU_USER64_IDX;
 }
 
 static inline int cpu_mmu_index_kernel(CPUX86State *env)
 {
-    return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
-        ((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK))
-        ? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
+    int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
+    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;
+
+    return mmu_index_base + mmu_index_32;
 }
 
 #define CC_DST  (env->cc_dst)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7f908236767..647371198c7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7732,13 +7732,16 @@ static bool x86_cpu_has_work(CPUState *cs)
     return x86_cpu_pending_interrupt(cs, cs->interrupt_request) != 0;
 }
 
-static int x86_cpu_mmu_index(CPUState *cs, bool ifetch)
+static int x86_cpu_mmu_index(CPUState *env, bool ifetch)
 {
     CPUX86State *env = cpu_env(cs);
+    int mmu_index_32 = (env->hflags & HF_CS64_MASK) ? 1 : 0;
+    int mmu_index_base =
+        (env->hflags & HF_CPL_MASK) == 3 ? MMU_USER64_IDX :
+        !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP64_IDX :
+        (env->eflags & AC_MASK) ? MMU_KNOSMAP64_IDX : MMU_KSMAP64_IDX;
 
-    return (env->hflags & HF_CPL_MASK) == 3 ? MMU_USER_IDX :
-        (!(env->hflags & HF_SMAP_MASK) || (env->eflags & AC_MASK))
-        ? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
+    return mmu_index_base + mmu_index_32;
 }
 
 static void x86_disas_set_info(CPUState *cs, disassemble_info *info)
diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index a0d5ce39300..b2c525e1a92 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -545,7 +545,8 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
         if (likely(use_stage2)) {
             in.cr3 = env->nested_cr3;
             in.pg_mode = env->nested_pg_mode;
-            in.mmu_idx = MMU_USER_IDX;
+            in.mmu_idx =
+                env->nested_pg_mode & PG_MODE_LMA ? MMU_USER64_IDX : MMU_USER32_IDX;
             in.ptw_idx = MMU_PHYS_IDX;
 
             if (!mmu_translate(env, &in, out, err)) {
-- 
2.43.0

Re: [PATCH v2 4/7] target/i386: use separate MMU indexes for 32-bit accesses

[Zhao Liu]

On Fri, Feb 23, 2024 at 02:09:45PM +0100, Paolo Bonzini wrote:
> Date: Fri, 23 Feb 2024 14:09:45 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 4/7] target/i386: use separate MMU indexes for 32-bit
>  accesses
> X-Mailer: git-send-email 2.43.0
> 
> Accesses from a 32-bit environment (32-bit code segment for instruction
> accesses, EFER.LMA==0 for processor accesses) have to mask away the
> upper 32 bits of the address.  While a bit wasteful, the easiest way
> to do so is to use separate MMU indexes.  These days, QEMU anyway is
> compiled with a fixed value for NB_MMU_MODES.  Split MMU_USER_IDX,
> MMU_KSMAP_IDX and MMU_KNOSMAP_IDX in two.

Maybe s/in/into/ ?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.h                    | 34 ++++++++++++++++++++--------
>  target/i386/cpu.c                    | 11 +++++----
>  target/i386/tcg/sysemu/excp_helper.c |  3 ++-
>  3 files changed, 33 insertions(+), 15 deletions(-)
> 

[snip]

>  
>  static inline int cpu_mmu_index_kernel(CPUX86State *env)
>  {
> -    return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
> -        ((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK))
> -        ? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
> +    int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
> +    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;

Change the line?

Re: [PATCH v2 4/7] target/i386: use separate MMU indexes for 32-bit accesses

[Paolo Bonzini]

On Mon, Feb 26, 2024 at 9:22 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> On Fri, Feb 23, 2024 at 02:09:45PM +0100, Paolo Bonzini wrote:
> > Accesses from a 32-bit environment (32-bit code segment for instruction
> > accesses, EFER.LMA==0 for processor accesses) have to mask away the
> > upper 32 bits of the address.  While a bit wasteful, the easiest way
> > to do so is to use separate MMU indexes.  These days, QEMU anyway is
> > compiled with a fixed value for NB_MMU_MODES.  Split MMU_USER_IDX,
> > MMU_KSMAP_IDX and MMU_KNOSMAP_IDX in two.
>
> Maybe s/in/into/ ?

Both are acceptable grammar.

> >  static inline int cpu_mmu_index_kernel(CPUX86State *env)
> >  {
> > -    return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
> > -        ((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK))
> > -        ? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
> > +    int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
> > +    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;

> Change the line?

It's reformatted but the logic is the same.

- if !SMAP -> MMU_KNOSMAP_IDX

- if CPL < 3 && EFLAGS.AC - MMU_KNOSMAP_IDX

- else MMU_KSMAP_IDX

The only change is adding the "64" suffix, which is then changed to
32-bit if needed via mmu_index_32.

Paolo

Re: [PATCH v2 4/7] target/i386: use separate MMU indexes for 32-bit accesses

[Zhao Liu]

Hi Paolo,

On Mon, Feb 26, 2024 at 10:55:14AM +0100, Paolo Bonzini wrote:
> Date: Mon, 26 Feb 2024 10:55:14 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v2 4/7] target/i386: use separate MMU indexes for
>  32-bit accesses
> 
> On Mon, Feb 26, 2024 at 9:22 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > On Fri, Feb 23, 2024 at 02:09:45PM +0100, Paolo Bonzini wrote:
> > > Accesses from a 32-bit environment (32-bit code segment for instruction
> > > accesses, EFER.LMA==0 for processor accesses) have to mask away the
> > > upper 32 bits of the address.  While a bit wasteful, the easiest way
> > > to do so is to use separate MMU indexes.  These days, QEMU anyway is
> > > compiled with a fixed value for NB_MMU_MODES.  Split MMU_USER_IDX,
> > > MMU_KSMAP_IDX and MMU_KNOSMAP_IDX in two.
> >
> > Maybe s/in/into/ ?
> 
> Both are acceptable grammar.
> 
> > >  static inline int cpu_mmu_index_kernel(CPUX86State *env)
> > >  {
> > > -    return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
> > > -        ((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK))
> > > -        ? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
> > > +    int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
> > > +    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;
> 
> > Change the line?
> 
> It's reformatted but the logic is the same.
> 
> - if !SMAP -> MMU_KNOSMAP_IDX
> 
> - if CPL < 3 && EFLAGS.AC - MMU_KNOSMAP_IDX
> 
> - else MMU_KSMAP_IDX
> 
> The only change is adding the "64" suffix, which is then changed to
> 32-bit if needed via mmu_index_32.
> 

Thanks for the explanation, I get your point.
Similarly, I also understand your change in x86_cpu_mmu_index().

LGTM, please allow me to add my review tag:

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

[PATCH v2 5/7] target/i386: Fix physical address truncation

[Paolo Bonzini]

The address translation logic in get_physical_address() will currently
truncate physical addresses to 32 bits unless long mode is enabled.
This is incorrect when using physical address extensions (PAE) outside
of long mode, with the result that a 32-bit operating system using PAE
to access memory above 4G will experience undefined behaviour.

The truncation code was originally introduced in commit 33dfdb5 ("x86:
only allow real mode to access 32bit without LMA"), where it applied
only to translations performed while paging is disabled (and so cannot
affect guests using PAE).

Commit 9828198 ("target/i386: Add MMU_PHYS_IDX and MMU_NESTED_IDX")
rearranged the code such that the truncation also applied to the use
of MMU_PHYS_IDX and MMU_NESTED_IDX.  Commit 4a1e9d4 ("target/i386: Use
atomic operations for pte updates") brought this truncation into scope
for page table entry accesses, and is the first commit for which a
Windows 10 32-bit guest will reliably fail to boot if memory above 4G
is present.

The truncation code however is not completely redundant.  Even though the
maximum address size for any executed instruction is 32 bits, helpers for
operations such as BOUND, FSAVE or XSAVE may ask get_physical_address()
to translate an address outside of the 32-bit range, if invoked with an
argument that is close to the 4G boundary.  Likewise for processor
accesses, for example TSS or IDT accesses, when EFER.LMA==0.

So, move the address truncation in get_physical_address() so that it
applies to 32-bit MMU indexes, but not to MMU_PHYS_IDX and MMU_NESTED_IDX.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2040
Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
Co-developed-by: Michael Brown <mcb30@ipxe.org>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h                    |  6 ++++++
 target/i386/cpu.c                    |  2 +-
 target/i386/tcg/sysemu/excp_helper.c | 12 +++++-------
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ee4ad372021..8a165889de6 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2326,6 +2326,12 @@ static inline bool is_mmu_index_user(int mmu_index)
     return (mmu_index & ~1) == MMU_USER64_IDX;
 }
 
+static inline bool is_mmu_index_32(int mmu_index)
+{
+    assert(mmu_index < MMU_PHYS_IDX);
+    return mmu_index & 1;
+}
+
 static inline int cpu_mmu_index_kernel(CPUX86State *env)
 {
     int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 647371198c7..ba6d7b80a7f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7732,7 +7732,7 @@ static bool x86_cpu_has_work(CPUState *cs)
     return x86_cpu_pending_interrupt(cs, cs->interrupt_request) != 0;
 }
 
-static int x86_cpu_mmu_index(CPUState *env, bool ifetch)
+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;
diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index b2c525e1a92..8bcdd2906d5 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -558,6 +558,10 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
         break;
 
     default:
+        if (is_mmu_index_32(mmu_idx)) {
+            addr = (uint32_t)addr;
+        }
+
         if (likely(env->cr[0] & CR0_PG_MASK)) {
             in.cr3 = env->cr[3];
             in.mmu_idx = mmu_idx;
@@ -581,14 +585,8 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
         break;
     }
 
-    /* Translation disabled. */
+    /* No translation needed. */
     out->paddr = addr & x86_get_a20_mask(env);
-#ifdef TARGET_X86_64
-    if (!(env->hflags & HF_LMA_MASK)) {
-        /* Without long mode we can only address 32bits in real mode */
-        out->paddr = (uint32_t)out->paddr;
-    }
-#endif
     out->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
     out->page_size = TARGET_PAGE_SIZE;
     return true;
-- 
2.43.0

Re: [PATCH v2 5/7] target/i386: Fix physical address truncation

[Zhao Liu]

On Fri, Feb 23, 2024 at 02:09:46PM +0100, Paolo Bonzini wrote:
> Date: Fri, 23 Feb 2024 14:09:46 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 5/7] target/i386: Fix physical address truncation
> X-Mailer: git-send-email 2.43.0
> 
> The address translation logic in get_physical_address() will currently
> truncate physical addresses to 32 bits unless long mode is enabled.
> This is incorrect when using physical address extensions (PAE) outside
> of long mode, with the result that a 32-bit operating system using PAE
> to access memory above 4G will experience undefined behaviour.
> 
> The truncation code was originally introduced in commit 33dfdb5 ("x86:
> only allow real mode to access 32bit without LMA"), where it applied
> only to translations performed while paging is disabled (and so cannot
> affect guests using PAE).
> 
> Commit 9828198 ("target/i386: Add MMU_PHYS_IDX and MMU_NESTED_IDX")
> rearranged the code such that the truncation also applied to the use
> of MMU_PHYS_IDX and MMU_NESTED_IDX.  Commit 4a1e9d4 ("target/i386: Use
> atomic operations for pte updates") brought this truncation into scope
> for page table entry accesses, and is the first commit for which a
> Windows 10 32-bit guest will reliably fail to boot if memory above 4G
> is present.
> 
> The truncation code however is not completely redundant.  Even though the
> maximum address size for any executed instruction is 32 bits, helpers for
> operations such as BOUND, FSAVE or XSAVE may ask get_physical_address()
> to translate an address outside of the 32-bit range, if invoked with an
> argument that is close to the 4G boundary.  Likewise for processor
> accesses, for example TSS or IDT accesses, when EFER.LMA==0.
> 
> So, move the address truncation in get_physical_address() so that it
> applies to 32-bit MMU indexes, but not to MMU_PHYS_IDX and MMU_NESTED_IDX.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2040
> Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
> Co-developed-by: Michael Brown <mcb30@ipxe.org>
> Signed-off-by: Michael Brown <mcb30@ipxe.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.h                    |  6 ++++++
>  target/i386/cpu.c                    |  2 +-
>  target/i386/tcg/sysemu/excp_helper.c | 12 +++++-------
>  3 files changed, 12 insertions(+), 8 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index ee4ad372021..8a165889de6 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2326,6 +2326,12 @@ static inline bool is_mmu_index_user(int mmu_index)
>      return (mmu_index & ~1) == MMU_USER64_IDX;
>  }
>  
> +static inline bool is_mmu_index_32(int mmu_index)
> +{
> +    assert(mmu_index < MMU_PHYS_IDX);
> +    return mmu_index & 1;
> +}
> +
>  static inline int cpu_mmu_index_kernel(CPUX86State *env)
>  {
>      int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 647371198c7..ba6d7b80a7f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7732,7 +7732,7 @@ static bool x86_cpu_has_work(CPUState *cs)
>      return x86_cpu_pending_interrupt(cs, cs->interrupt_request) != 0;
>  }
>  
> -static int x86_cpu_mmu_index(CPUState *env, bool ifetch)
> +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;
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index b2c525e1a92..8bcdd2906d5 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -558,6 +558,10 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
>          break;
>  
>      default:
> +        if (is_mmu_index_32(mmu_idx)) {
> +            addr = (uint32_t)addr;
> +        }
> +
>          if (likely(env->cr[0] & CR0_PG_MASK)) {
>              in.cr3 = env->cr[3];
>              in.mmu_idx = mmu_idx;
> @@ -581,14 +585,8 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
>          break;
>      }
>  
> -    /* Translation disabled. */
> +    /* No translation needed. */
>      out->paddr = addr & x86_get_a20_mask(env);
> -#ifdef TARGET_X86_64
> -    if (!(env->hflags & HF_LMA_MASK)) {
> -        /* Without long mode we can only address 32bits in real mode */
> -        out->paddr = (uint32_t)out->paddr;
> -    }
> -#endif
>      out->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>      out->page_size = TARGET_PAGE_SIZE;
>      return true;
> -- 
> 2.43.0
> 
> 

[PATCH v2 6/7] target/i386: remove unnecessary/wrong application of the A20 mask

[Paolo Bonzini]

If ptw_translate() does a MMU_PHYS_IDX access, the A20 mask is already
applied in get_physical_address(), which is called via probe_access_full()
and x86_cpu_tlb_fill().

If ptw_translate() on the other hand does a MMU_NESTED_IDX access,
the A20 mask must not be applied to the address that is looked up in
the nested page tables; it must be applied only to the addresses that
hold the NPT entries (which is achieved via MMU_PHYS_IDX, per the
previous paragraph).

Therefore, we can remove A20 masking from the computation of the page
table entry's address, and let get_physical_address() or mmu_translate()
apply it when they know they are returning a host-physical address.

Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/excp_helper.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 8bcdd2906d5..2ddc08b4bb6 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -164,8 +164,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
                 /*
                  * Page table level 5
                  */
-                pte_addr = ((in->cr3 & ~0xfff) +
-                            (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
+                pte_addr = (in->cr3 & ~0xfff) + (((addr >> 48) & 0x1ff) << 3);
                 if (!ptw_translate(&pte_trans, pte_addr)) {
                     return false;
                 }
@@ -189,8 +188,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
             /*
              * Page table level 4
              */
-            pte_addr = ((pte & PG_ADDRESS_MASK) +
-                        (((addr >> 39) & 0x1ff) << 3)) & a20_mask;
+            pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 39) & 0x1ff) << 3);
             if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
@@ -210,8 +208,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
             /*
              * Page table level 3
              */
-            pte_addr = ((pte & PG_ADDRESS_MASK) +
-                        (((addr >> 30) & 0x1ff) << 3)) & a20_mask;
+            pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 30) & 0x1ff) << 3);
             if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
@@ -238,7 +235,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
             /*
              * Page table level 3
              */
-            pte_addr = ((in->cr3 & 0xffffffe0ULL) + ((addr >> 27) & 0x18)) & a20_mask;
+            pte_addr = (in->cr3 & 0xffffffe0ULL) + ((addr >> 27) & 0x18);
             if (!ptw_translate(&pte_trans, pte_addr)) {
                 return false;
             }
@@ -260,8 +257,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
         /*
          * Page table level 2
          */
-        pte_addr = ((pte & PG_ADDRESS_MASK) +
-                    (((addr >> 21) & 0x1ff) << 3)) & a20_mask;
+        pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 21) & 0x1ff) << 3);
         if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
@@ -287,8 +283,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
         /*
          * Page table level 1
          */
-        pte_addr = ((pte & PG_ADDRESS_MASK) +
-                    (((addr >> 12) & 0x1ff) << 3)) & a20_mask;
+        pte_addr = (pte & PG_ADDRESS_MASK) + (((addr >> 12) & 0x1ff) << 3);
         if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
@@ -306,7 +301,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
         /*
          * Page table level 2
          */
-        pte_addr = ((in->cr3 & 0xfffff000ULL) + ((addr >> 20) & 0xffc)) & a20_mask;
+        pte_addr = (in->cr3 & 0xfffff000ULL) + ((addr >> 20) & 0xffc);
         if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
@@ -335,7 +330,7 @@ static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
         /*
          * Page table level 1
          */
-        pte_addr = ((pte & ~0xfffu) + ((addr >> 10) & 0xffc)) & a20_mask;
+        pte_addr = (pte & ~0xfffu) + ((addr >> 10) & 0xffc);
         if (!ptw_translate(&pte_trans, pte_addr)) {
             return false;
         }
-- 
2.43.0

[PATCH v2 7/7] target/i386: leave the A20 bit set in the final NPT walk

[Paolo Bonzini]

The A20 mask is only applied to the final memory access.  Nested
page tables are always walked with the raw guest-physical address.

Unlike the previous patch, in this one the masking must be kept, but
it was done too early.

Fixes: 4a1e9d4d11c ("target/i386: Use atomic operations for pte updates", 2022-10-18)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/excp_helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 2ddc08b4bb6..8f7011d9663 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -134,7 +134,6 @@ static inline bool ptw_setl(const PTETranslate *in, uint32_t old, uint32_t set)
 static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
                           TranslateResult *out, TranslateFault *err)
 {
-    const int32_t a20_mask = x86_get_a20_mask(env);
     const target_ulong addr = in->addr;
     const int pg_mode = in->pg_mode;
     const bool is_user = is_mmu_index_user(in->mmu_idx);
@@ -417,10 +416,13 @@ do_check_protect_pse36:
         }
     }
 
-    /* align to page_size */
-    paddr = (pte & a20_mask & PG_ADDRESS_MASK & ~(page_size - 1))
-          | (addr & (page_size - 1));
+    /* merge offset within page */
+    paddr = (pte & PG_ADDRESS_MASK & ~(page_size - 1)) | (addr & (page_size - 1));
 
+    /*
+     * Note that NPT is walked (for both paging structures and final guest
+     * addresses) using the address with the A20 bit set.
+     */
     if (in->ptw_idx == MMU_NESTED_IDX) {
         CPUTLBEntryFull *full;
         int flags, nested_page_size;
@@ -459,7 +461,7 @@ do_check_protect_pse36:
         }
     }
 
-    out->paddr = paddr;
+    out->paddr = paddr & x86_get_a20_mask(env);
     out->prot = prot;
     out->page_size = page_size;
     return true;
-- 
2.43.0

Re: [PATCH v2 0/7] target/i386: Fix physical address masking bugs

[Michael Brown]

On 23/02/2024 13:09, Paolo Bonzini wrote:
> The address translation logic in get_physical_address() will currently
> truncate physical addresses to 32 bits unless long mode is enabled.
> This is incorrect when using physical address extensions (PAE) outside
> of long mode, with the result that a 32-bit operating system using PAE
> to access memory above 4G will experience undefined behaviour.
> Instead, truncation must be applied to the linear address.  Because
> this truncation is diffent between 32- and 64-bit modes, the series
> opts to split 32- and 64-bit modes to separate MMU indices, which is
> the simplest way to ensure that, for example, a kernel that runs both
> 32-bit and 64-bit programs looks up the correct address in the
> (64-bit) page tables.
> 
> Furthermore, when inspecting the code I noticed that the A20 mask is
> applied incorrectly when NPT is active.  The mask should not be applied
> to the addresses that are looked up in the NPT, only to the physical
> addresses.  Obviously no hypervisor is going to leave A20 masking on,
> but the code actually becomes simpler so let's do it.
> 
> Patches 1 and 2 fix cases in which the addresses must be masked,
> or overflow is otherwise invalid, for MMU_PHYS_IDX accesses.
> 
> Patches 3 and 4 introduce separate MMU indexes for 32- and 64-bit
> accesses.
> 
> Patch 5 fixes the bug, by limiting the masking to the 32-bit MMU indexes.
> 
> Patches 6 and 7 further clean up the MMU functions to centralize
> application of the A20 mask and fix bugs in the process.
> 
> Tested with kvm-unit-tests SVM tests and with an old 32-bit Debian image.
> 
> Paolo Bonzini (7):
>    target/i386: mask high bits of CR3 in 32-bit mode
>    target/i386: check validity of VMCB addresses
>    target/i386: introduce function to query MMU indices
>    target/i386: use separate MMU indexes for 32-bit accesses
>    target/i386: Fix physical address truncation
>    target/i386: remove unnecessary/wrong application of the A20 mask
>    target/i386: leave the A20 bit set in the final NPT walk
> 
>   target/i386/cpu.h                    | 46 +++++++++++++++++++-----
>   target/i386/cpu.c                    |  9 +++--
>   target/i386/tcg/sysemu/excp_helper.c | 52 +++++++++++++---------------
>   target/i386/tcg/sysemu/misc_helper.c |  3 ++
>   target/i386/tcg/sysemu/svm_helper.c  | 27 +++++++++++----
>   5 files changed, 92 insertions(+), 45 deletions(-)

Thank you for putting in the work to fix this all up!

I confirm that this patch series resolves the issue that I originally 
observed in https://gitlab.com/qemu-project/qemu/-/issues/2040 and that 
Windows 10 32-bit is able to boot with PAE enabled and memory over 4G.

Tested-by: Michael Brown <mcb30@ipxe.org>

Thanks,

Michael