diff mbox series

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

Message ID 20240807081813.735158-3-xin@zytor.com
State New
Headers show
Series target/i386: Add nested FRED support | expand

Commit Message

Xin Li Aug. 7, 2024, 8:18 a.m. UTC 
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(-)

Comments

Zhao Liu Aug. 7, 2024, 3:58 p.m. UTC | #1 
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
>
Xin Li Aug. 8, 2024, 7:04 a.m. UTC | #2 
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)?
Zhao Liu Aug. 8, 2024, 9:40 a.m. UTC | #3 
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
Xin Li Aug. 9, 2024, 6:38 a.m. UTC | #4 
>>>> @@ -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
> 
>
Zhao Liu Aug. 9, 2024, 9:02 a.m. UTC | #5 
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
diff mbox series

Patch

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,
         },