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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

Paolo