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

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

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