1. Patchew
  2. QEMU
  3. [PATCH v1 0/3] target/i386: Add nested FRED support
  4. View patch

[PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

Xin Li (Intel) posted 3 patches 3 months, 2 weeks ago
[PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support
Posted by Xin Li (Intel) 3 months, 2 weeks ago 
Add definitions of
  1) VM-exit activate secondary controls bit
  2) VM-entry load FRED bit
which are required to enable nested FRED.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.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..31f287cae0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "vmx-exit-save-efer", "vmx-exit-load-efer",
                 "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
             NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
-            NULL, "vmx-exit-load-pkrs", NULL, NULL,
+            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",
         },
         .msr = {
             .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
@@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, "vmx-entry-ia32e-mode", NULL, NULL,
             NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
             "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
-            NULL, NULL, "vmx-entry-load-pkrs", NULL,
+            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-- 
2.45.2
Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support
Posted by Zhao Liu 3 months, 2 weeks ago 
Hi Xin,

On Wed, Aug 07, 2024 at 01:18:11AM -0700, Xin Li (Intel) wrote:
> Date: Wed,  7 Aug 2024 01:18:11 -0700
> From: "Xin Li (Intel)" <xin@zytor.com>
> Subject: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED
>  support
> X-Mailer: git-send-email 2.45.2
> 
> Add definitions of
>   1) VM-exit activate secondary controls bit
>   2) VM-entry load FRED bit
> which are required to enable nested FRED.
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.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..31f287cae0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "vmx-exit-save-efer", "vmx-exit-load-efer",
>                  "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
>              NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
> -            NULL, "vmx-exit-load-pkrs", NULL, NULL,
> +            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",

Oh, the order of my reviews is mixed up.
It's better to move VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS into this patch.

>          },
>          .msr = {
>              .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
> @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              NULL, "vmx-entry-ia32e-mode", NULL, NULL,
>              NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
>              "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
> -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
> +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",

Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
if they are not used.

Regards,
Zhao

>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
>          },
> -- 
> 2.45.2
>
Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support
Posted by Xin Li 3 months, 2 weeks ago 
On 8/7/2024 8:58 AM, Zhao Liu wrote:
> On Wed, Aug 07, 2024 at 01:18:11AM -0700, Xin Li (Intel) wrote:
>> @@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>               "vmx-exit-save-efer", "vmx-exit-load-efer",
>>                   "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
>>               NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
>> -            NULL, "vmx-exit-load-pkrs", NULL, NULL,
>> +            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",
> 
> Oh, the order of my reviews is mixed up.
> It's better to move VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS into this patch.

Usually a simple definition is added in a patch where it is used, not in
qemu?

>>           },
>>           .msr = {
>>               .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
>> @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>               NULL, "vmx-entry-ia32e-mode", NULL, NULL,
>>               NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
>>               "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
>> -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
>> +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
> 
> Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
> and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
> if they are not used.

I'm not sure, but why add something that is not being used (thus not
tested)?
Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support
Posted by Zhao Liu 3 months, 2 weeks ago 
Hi Xin,

On Thu, Aug 08, 2024 at 12:04:42AM -0700, Xin Li wrote:
> Date: Thu, 8 Aug 2024 00:04:42 -0700
> From: Xin Li <xin@zytor.com>
> Subject: Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested
>  FRED support
> 
> On 8/7/2024 8:58 AM, Zhao Liu wrote:
> > On Wed, Aug 07, 2024 at 01:18:11AM -0700, Xin Li (Intel) wrote:
> > > @@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > >               "vmx-exit-save-efer", "vmx-exit-load-efer",
> > >                   "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
> > >               NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
> > > -            NULL, "vmx-exit-load-pkrs", NULL, NULL,
> > > +            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",
> > 
> > Oh, the order of my reviews is mixed up.
> > It's better to move VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS into this patch.
> 
> Usually a simple definition is added in a patch where it is used, not in
> qemu?
> 
> > >           },
> > >           .msr = {
> > >               .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
> > > @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > >               NULL, "vmx-entry-ia32e-mode", NULL, NULL,
> > >               NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
> > >               "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
> > > -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
> > > +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
> > 
> > Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
> > and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
> > if they are not used.
> 
> I'm not sure, but why add something that is not being used (thus not
> tested)?

Yes, the use of macros is a factor. My another consideration is the
integrity of the feature definitions. When the such feature definitions
were first introduced in commit 704798add83b (”target/i386: add VMX
definitions”), I understand thay were mainly used to enumerate and
reflect hardware support and not all defs are used directly.

The feat word name and the feature definition should essentially be
bound, and it might be possible to generate the feature definition
from the feat word via some script without having to add it manually,
but right now there is no work on this, and no additional constraints,
so we have to manually add and manually check it to make sure that the
two correspond to each other. When a feature word is added, it means
that Host supports the corresponding feature, and from an integrity
perspective, so it is natural to continue adding definition (just like
the commit 52a44ad2b92b ("target/i386: Expose VMX entry/exit load pkrs
control bits")), right?

Though I found that there are still some mismatches between the feature
word and the corresponding definition, but ideally they should coexist.

About the test, if it's just enumerated and not added to a specific CPU
model or involved by other logic, it's harmless?

Thanks,
Zhao
Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support
Posted by Xin Li 3 months, 2 weeks ago 
>>>> @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>>>                NULL, "vmx-entry-ia32e-mode", NULL, NULL,
>>>>                NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
>>>>                "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
>>>> -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
>>>> +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
>>>
>>> Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
>>> and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
>>> if they are not used.
>>
>> I'm not sure, but why add something that is not being used (thus not
>> tested)?
> 
> Yes, the use of macros is a factor. My another consideration is the
> integrity of the feature definitions. When the such feature definitions
> were first introduced in commit 704798add83b (”target/i386: add VMX
> definitions”), I understand thay were mainly used to enumerate and
> reflect hardware support and not all defs are used directly.
> 
> The feat word name and the feature definition should essentially be
> bound, and it might be possible to generate the feature definition
> from the feat word via some script without having to add it manually,
> but right now there is no work on this, and no additional constraints,
> so we have to manually add and manually check it to make sure that the
> two correspond to each other. When a feature word is added, it means
> that Host supports the corresponding feature, and from an integrity
> perspective, so it is natural to continue adding definition (just like
> the commit 52a44ad2b92b ("target/i386: Expose VMX entry/exit load pkrs
> control bits")), right?
> 
> Though I found that there are still some mismatches between the feature
> word and the corresponding definition, but ideally they should coexist.
> 
> About the test, if it's just enumerated and not added to a specific CPU
> model or involved by other logic, it's harmless?

Unless tests are ready, such code are literally dead code, and could get
broken w/o being noticed for a long time.

I think we should add it only when tests are also added.  Otherwise we 
added burden to maintainers, hoping test will be added soon, which often
never happen.

> 
> Thanks,
> Zhao
> 
>
Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support
Posted by Zhao Liu 3 months, 2 weeks ago 
On Thu, Aug 08, 2024 at 11:38:11PM -0700, Xin Li wrote:
> Date: Thu, 8 Aug 2024 23:38:11 -0700
> From: Xin Li <xin@zytor.com>
> Subject: Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested
>  FRED support
> 
> > > > > @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > > > >                NULL, "vmx-entry-ia32e-mode", NULL, NULL,
> > > > >                NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
> > > > >                "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
> > > > > -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
> > > > > +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
> > > > 
> > > > Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
> > > > and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
> > > > if they are not used.
> > > 
> > > I'm not sure, but why add something that is not being used (thus not
> > > tested)?
> > 
> > Yes, the use of macros is a factor. My another consideration is the
> > integrity of the feature definitions. When the such feature definitions
> > were first introduced in commit 704798add83b (”target/i386: add VMX
> > definitions”), I understand thay were mainly used to enumerate and
> > reflect hardware support and not all defs are used directly.
> > 
> > The feat word name and the feature definition should essentially be
> > bound, and it might be possible to generate the feature definition
> > from the feat word via some script without having to add it manually,
> > but right now there is no work on this, and no additional constraints,
> > so we have to manually add and manually check it to make sure that the
> > two correspond to each other. When a feature word is added, it means
> > that Host supports the corresponding feature, and from an integrity
> > perspective, so it is natural to continue adding definition (just like
> > the commit 52a44ad2b92b ("target/i386: Expose VMX entry/exit load pkrs
> > control bits")), right?
> > 
> > Though I found that there are still some mismatches between the feature
> > word and the corresponding definition, but ideally they should coexist.
> > 
> > About the test, if it's just enumerated and not added to a specific CPU
> > model or involved by other logic, it's harmless?
> 
> Unless tests are ready, such code are literally dead code, and could get
> broken w/o being noticed for a long time.
> 
> I think we should add it only when tests are also added.  Otherwise we added
> burden to maintainers, hoping test will be added soon, which often
> never happen.

It makes sense and can reduce the burden on maintainers. Now I totally
agree with you.

Thanks,
Zhao

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.