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

[Xiaoyao Li]

On 1/3/2024 5:25 PM, Binbin Wu wrote:
> 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.

Frankly, I don't know. As you explained in commit message, missing the 
check doesn't cause any functional issue because the function is only 
called for tcg code and LAM is not enabled for tcg.

But personally, I think adding the check does no harm and the logic is 
straightforward, while not adding the check looks not intuitive and begs 
a comment to explain.

So my preference is to add the check.