Re: [PATCH v5 1/4] target/i386: add missing vmx features for several CPU models

[Xiaoyao Li]

On 7/13/2020 10:44 PM, Eduardo Habkost wrote:
> On Mon, Jul 13, 2020 at 03:45:55PM +0800, Xiaoyao Li wrote:
> > On 7/13/2020 3:23 PM, Chenyi Qiang wrote:
> > > On 7/11/2020 12:48 AM, Eduardo Habkost wrote:
> > > > On Fri, Jul 10, 2020 at 09:45:49AM +0800, Chenyi Qiang wrote:
> > > > > On 7/10/2020 6:12 AM, Eduardo Habkost wrote:
> > > > > > I'm very sorry for taking so long to review this.  Question
> > > > > > below:
> > > > > >
> > > > > > On Fri, Jun 19, 2020 at 03:31:11PM +0800, Chenyi Qiang wrote:
> > > > > > > Add some missing VMX features in Skylake-Server,
> > > > > > > Cascadelake-Server and
> > > > > > > Icelake-Server CPU models based on the output of Paolo's script.
> > > > > > >
> > > > > > > Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> > > > > >
> > > > > > Why are you changing the v1 definition instead adding those new
> > > > > > features in a new version of the CPU model, just like you did in
> > > > > > patch 3/4?
> > > > >
> > > > > I suppose these missing vmx features are not quite necessary for
> > > > > customers.
> > > > > Just post it here to see if they are worth being added.
> > > > > Adding a new version is reasonable. Is it appropriate to put all
> > > > > the missing
> > > > > features in patch 1/4, 3/4, 4/4 in a same version?
> > > >
> > > > Yes, it would be OK to add only one new version with all the new
> > > > features.
> > >
> > > During the coding, I prefer to split the missing vmx features into a new
> > > version of CPU model, because the vmx features depends on CPUID_EXT_VMX.
> > > I think It would be better to distinguish it instead of enabling the vmx
> > > transparently. i.e.
> > > {
> > >   Â Â Â Â .version = 4,
> > >   Â Â Â Â .props = (PropValue[]) {
> > >   Â Â Â Â Â Â Â  { "sha-ni", "on" },
> > >   Â Â Â Â Â Â Â  ... ...
> > >   Â Â Â Â Â Â Â  { "model", "106" },
> > >   Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  { /* end of list */ }
> > >   Â Â Â Â },
> > > },
> > > {
> > >   Â Â Â Â .version = 5,
> > >   Â Â Â Â .props = (PropValue[]) {
> > >   Â Â Â Â Â Â Â  { "vmx", "on" }
> >
> > Chenyi,
> >
> > This is not we have discussed. I prefer to changing the logic of versioned
> > CPU model to not add the features in versioned CPU model to
> > env->user_features[]. They're not supposed to be added to
> > env->user_features[] since they're not set by user through -feature/+feature
> >
> > Eduardo,
> >
> > What do you think?
>
> If features added by the CPU model versions appear in
> user_features, that's a bug.  What's the user-visible symptom you
> are seeing because of it?

It's the vmx features that the PATCH 1 wants to add. They require VMX to 
be there because feature_dependencies[] checking in 
x86_cpu_expand_features().

Paolo didn't met this issue because he added VMX features to named CPU 
models directly without adding new versions. Chenyi have to deal with it 
since you require them to be added in a new version. He wants to add 
{vmx, on} in the new version to avoid the warning. But I don't think 
that's a good idea since other CPU models don't have VMX.