Re: [PATCH] target/i386: Add more features enumerated by CPUID.7.2.EDX

[Zhao Liu <zhao1.liu@intel.com>]

On Thu, Oct 10, 2024 at 03:17:16PM +0200, Paolo Bonzini wrote:
> Date: Thu, 10 Oct 2024 15:17:16 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH] target/i386: Add more features enumerated by
>  CPUID.7.2.EDX
> 
> On 10/9/24 10:12, Chao Gao wrote:
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index 85ef7452c0..18ba958f46 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -1148,8 +1148,8 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > > >       [FEAT_7_2_EDX] = {
> > > >           .type = CPUID_FEATURE_WORD,
> > > >           .feat_names = {
> > > > -            NULL, NULL, NULL, NULL,
> > > > -            NULL, "mcdt-no", NULL, NULL,
> > > > +            "intel-psfd", "ipred-ctrl", "rrsba-ctrl", "ddpd-u",
> > > > +            "bhi-ctrl", "mcdt-no", NULL, NULL,
> > > 
> > > IIUC, these bits depend on "spec-ctrl", which indicates the presence of
> > > IA32_SPEC_CTRL.
> > > 
> > > Then I think we'd better add dependencies in feature_dependencies[].
> > 
> > (+ kvm mailing list)
> > 
> > Thanks for pointing that out. It seems that any of these bits imply the
> > presence of IA32_SPEC_CTRL. According to SDM vol4, chapter 2, table 2.2,
> > the 'Comment' column for the IA32_SPEC_CTRL MSR states:
> > 
> >    If any one of the enumeration conditions for defined bit field positions holds.
> > 
> > So, it might be more appropriate to fix KVM's handling of the
> > IA32_SPEC_CTRL MSR (i.e., guest_has_spec_ctrl_msr()).
> > 
> > what do you think?
> 
> You're right, the spec-ctrl CPUID feature covers the IBRS bit of
> MSR_IA32_SPEC_CTRL and also the IBPB feature of MSR_IA32_PRED_CMD.  It does
> not specify the existence of MSR_IA32_SPEC_CTRL.
> 
> In practice it's probably not a good idea to omit spec-ctrl when passing
> other features to the guest that cover that MSR; but the specification says
> it's fine.

I think these features are also worth updating in the CPU models, as
well as in this document: 'cpu-models-x86.rst.inc' - section 'Important
CPU features for Intel x86 hosts' (maybe in the followup patches :))

Thanks,
Zhao