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>