Re: [PATCH v3 2/2] target/i386: add control bits support for LAM

[Binbin Wu]

On 12/28/2023 4:51 PM, Xiaoyao Li wrote:
> On 7/21/2023 4:08 PM, Binbin Wu wrote:
> > LAM uses CR3[61] and CR3[62] to configure/enable LAM on user pointers.
> > LAM uses CR4[28] to configure/enable LAM on supervisor pointers.
> >
> > For CR3 LAM bits, no additional handling needed:
> > - TCG
> >    LAM is not supported for TCG of target-i386. helper_write_crN() 
> > and helper_vmrun()
> >    check max physical address bits before calling 
> > cpu_x86_update_cr3(), no change needed,
> >    i.e. CR3 LAM bits are not allowed to be set in TCG.
> > - gdbstub
> >    x86_cpu_gdb_write_register() will call cpu_x86_update_cr3() to 
> > update cr3. Allow gdb
> >    to set the LAM bit(s) to CR3, if vcpu doesn't support LAM, 
> > KVM_SET_SREGS will fail as
> >    other CR3 reserved bits.
> >
> > For CR4 LAM bit, its reservation depends on vcpu supporting LAM 
> > feature or not.
> > - TCG
> >    LAM is not supported for TCG of target-i386. helper_write_crN() 
> > and helper_vmrun()
> >    check CR4 reserved bit before calling cpu_x86_update_cr4(), i.e. 
> > CR4 LAM bit is not
> >    allowed to be set in TCG.
> > - gdbstub
> >    x86_cpu_gdb_write_register() will call cpu_x86_update_cr4() to 
> > update cr4. Allow gdb
> >    to set the LAM bit to CR4, if vcpu doesn't support LAM, 
> > KVM_SET_SREGS will fail.
>
> I would go follow the current code, to mask out LAM bit if no CPUID.

I can do it in the next version.

But I am curious what's the rule of masking out a CR4 bit if no CPUID
in cpu_x86_update_cr4()?
e.g. current code checks SMAP but not SMEP.


> > - x86_cpu_reset_hold() doesn't need special handling.
> >
> > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> > ---
> >   target/i386/cpu.h | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 4db97899fe..710fadf550 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -261,6 +261,7 @@ typedef enum X86Seg {
> >   #define CR4_SMAP_MASK   (1U << 21)
> >   #define CR4_PKE_MASK   (1U << 22)
> >   #define CR4_PKS_MASK   (1U << 24)
> > +#define CR4_LAM_SUP_MASK (1U << 28)
> >     #define CR4_RESERVED_MASK \
> >   (~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
> > @@ -269,7 +270,8 @@ typedef enum X86Seg {
> >                   | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK | 
> > CR4_UMIP_MASK \
> >                   | CR4_LA57_MASK \
> >                   | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | 
> > CR4_OSXSAVE_MASK \
> > -                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | 
> > CR4_PKS_MASK))
> > +                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | 
> > CR4_PKS_MASK \
> > +                | CR4_LAM_SUP_MASK))
> >     #define DR6_BD          (1 << 13)
> >   #define DR6_BS          (1 << 14)
> > @@ -2478,6 +2480,9 @@ static inline uint64_t 
> > cr4_reserved_bits(CPUX86State *env)
> >       if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS)) {
> >           reserved_bits |= CR4_PKS_MASK;
> >       }
> > +    if (!(env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_LAM)) {
> > +        reserved_bits |= CR4_LAM_SUP_MASK;
> > +    }
> >       return reserved_bits;
> >   }