target/i386: Add more features enumerated by CPUID.7.2.EDX

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

Posted by Chao Gao 1 month ago

Following 5 bits in CPUID.7.2.EDX are supported by KVM. Add their
supports in QEMU. Each of them indicates certain bits of IA32_SPEC_CTRL
are supported. Those bits can control CPU speculation behavior which can
be used to defend against side-channel attacks.

bit0: intel-psfd
  if 1, indicates bit 7 of the IA32_SPEC_CTRL MSR is supported. Bit 7 of
  this MSR disables Fast Store Forwarding Predictor without disabling
  Speculative Store Bypass

bit1: ipred-ctrl
  If 1, indicates bits 3 and 4 of the IA32_SPEC_CTRL MSR are supported.
  Bit 3 of this MSR enables IPRED_DIS control for CPL3. Bit 4 of this
  MSR enables IPRED_DIS control for CPL0/1/2

bit2: rrsba-ctrl
  If 1, indicates bits 5 and 6 of the IA32_SPEC_CTRL MSR are supported.
  Bit 5 of this MSR disables RRSBA behavior for CPL3. Bit 6 of this MSR
  disables RRSBA behavior for CPL0/1/2

bit3: ddpd-u
  If 1, indicates bit 8 of the IA32_SPEC_CTRL MSR is supported. Bit 8 of
  this MSR disables Data Dependent Prefetcher.

bit4: bhi-ctrl
  if 1, indicates bit 10 of the IA32_SPEC_CTRL MSR is supported. Bit 10
  of this MSR enables BHI_DIS_S behavior.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 target/i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
-- 
2.46.1

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

Posted by Paolo Bonzini 1 week, 5 days ago

Queued, thanks.

Paolo

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

Posted by Zhao Liu 1 week, 6 days ago

On Thu, Sep 19, 2024 at 01:10:11PM +0800, Chao Gao wrote:
> Date: Thu, 19 Sep 2024 13:10:11 +0800
> From: Chao Gao <chao.gao@intel.com>
> Subject: [PATCH] target/i386: Add more features enumerated by CPUID.7.2.EDX
> X-Mailer: git-send-email 2.46.1
> 
> Following 5 bits in CPUID.7.2.EDX are supported by KVM. Add their
> supports in QEMU. Each of them indicates certain bits of IA32_SPEC_CTRL
> are supported. Those bits can control CPU speculation behavior which can
> be used to defend against side-channel attacks.
> 
> bit0: intel-psfd
>   if 1, indicates bit 7 of the IA32_SPEC_CTRL MSR is supported. Bit 7 of
>   this MSR disables Fast Store Forwarding Predictor without disabling
>   Speculative Store Bypass
> 
> bit1: ipred-ctrl
>   If 1, indicates bits 3 and 4 of the IA32_SPEC_CTRL MSR are supported.
>   Bit 3 of this MSR enables IPRED_DIS control for CPL3. Bit 4 of this
>   MSR enables IPRED_DIS control for CPL0/1/2
> 
> bit2: rrsba-ctrl
>   If 1, indicates bits 5 and 6 of the IA32_SPEC_CTRL MSR are supported.
>   Bit 5 of this MSR disables RRSBA behavior for CPL3. Bit 6 of this MSR
>   disables RRSBA behavior for CPL0/1/2
> 
> bit3: ddpd-u
>   If 1, indicates bit 8 of the IA32_SPEC_CTRL MSR is supported. Bit 8 of
>   this MSR disables Data Dependent Prefetcher.
> 
> bit4: bhi-ctrl
>   if 1, indicates bit 10 of the IA32_SPEC_CTRL MSR is supported. Bit 10
>   of this MSR enables BHI_DIS_S behavior.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  target/i386/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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[].

-Zhao

>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
> -- 
> 2.46.1
> 
>

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

Posted by Chao Gao 1 week, 6 days ago

>> 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?

>
>-Zhao
>
>>              NULL, NULL, NULL, NULL,
>>              NULL, NULL, NULL, NULL,
>>              NULL, NULL, NULL, NULL,
>> -- 
>> 2.46.1
>> 
>>

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

Posted by Paolo Bonzini 1 week, 5 days ago

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

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

Posted by Zhao Liu 1 week, 4 days ago

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