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

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

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