Re: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version property

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

> > > Introduce avx10-version property so that avx10 version can be controlled
> > > by user and cpu model. Per spec, avx10 version can never be 0, the default
> > > value of avx10-version is set to 0 to determine whether it is specified by
> > > user.
> > 
> > The default value of 0 does not reflect whether the user has set it to 0.
> > According to the description here, the spec clearly prohibits 0, so
> > should we report an error when the user sets it to 0?
> > 
> > If so, it might be better to change the default value to -1 and adjust
> > based on the host's support.
> > 
> 
> If user sets version to 0, it will directly use reported version, this
> should be a more neat and intuitive way?

The code implementation is actually similar for different initial
values. And about this:

> If user sets version to 0, it will directly use reported version", 

It's defining a special behavior for the API, which is based on the
special 0 value, and there needs to be documentation to let the user
know that 0 will be considered legal as well as that it will be quietly
overridden... But AFAIK there doesn't seem to be any place to add
documentation for the property ...

There may be similar problems with -1, e.g. if the user writes -1, there
is no way to report an error for the user's behavior. But it's better
than 0. After all, no one would think that a version of -1 is correct.
Topology IDs have been initialized to -1 to include the user's 0 value
in the check.

> > > The default can come from the device model or, for the max model,
> > > from KVM's reported value.
> > > 
> > > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > > Link: https://lore.kernel.org/r/20241028024512.156724-3-tao1.su@linux.intel.com
> > > Link: https://lore.kernel.org/r/20241028024512.156724-4-tao1.su@linux.intel.com
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  target/i386/cpu.h     |  4 +++
> > >  target/i386/cpu.c     | 64 ++++++++++++++++++++++++++++++++++++++-----
> > >  target/i386/kvm/kvm.c |  3 +-
> > >  3 files changed, 63 insertions(+), 8 deletions(-)
> > 
> > [snip]
> > 
> > > @@ -7611,7 +7644,23 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> > >          }
> > >      }
> > >  
> > > -    return x86_cpu_have_filtered_features(cpu);
> > > +    have_filtered_features = x86_cpu_have_filtered_features(cpu);
> > > +
> > > +    if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> > > +        x86_cpu_get_supported_cpuid(0x24, 0,
> > > +                                    &eax_0, &ebx_0, &ecx_0, &edx_0);
> > > +        uint8_t version = ebx_0 & 0xff;
> > > +
> > > +        if (version < env->avx10_version) {
> > > +            if (prefix) {
> > > +                warn_report("%s: avx10.%d", prefix, env->avx10_version);
> > 
> > Perhaps also tell user about revised version?
> > 
> > warn_report("%s: avx10.%d. Adjust to avx10.%d",
> >             prefix, env->avx10_version, version);
> > 
> 
> I see, thanks!

Welcome!

> > > +            }
> > > +            env->avx10_version = version;
> > > +            have_filtered_features = true;
> > > +        }
> > > +    }
> > 
> > 
> > Per Tao's comment, perhaps we can check AVX10 and version here (default
> > version is 0):
> > 
> > @@ -7674,13 +7682,21 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> >                                      &eax_0, &ebx_0, &ecx_0, &edx_0);
> >          uint8_t version = ebx_0 & 0xff;
> > 
> > -        if (version < env->avx10_version) {
> > +        if (!env->avx10_version) {
> > +            env->avx10_version = version;
> 
> x86_cpu_filter_features() is not a good place to assign avx10_version, I
> still tend to set it in max_x86_cpu_realize().

It's not proper to get the host's version when AVX10 cannot be enabled,
even maybe host doesn't support AVX10.

As you found out earlier, max_x86_cpu_realize doesn't know if AVX10 can
be enabled or not.

> > +        } else (version < env->avx10_version) {
> >              if (prefix) {
> > -                warn_report("%s: avx10.%d", prefix, env->avx10_version);
> > +                warn_report("%s: avx10.%d. Adjust to avx10.%d",
> > +                            prefix, env->avx10_version, version);
> >              }
> >              env->avx10_version = version;
> >              have_filtered_features = true;
> >          }
> > +    } else if (env->avx10_version && prefix) {

Oops, here we only need to check env->avx10_version...

> > +        if (prefix) {
> 
> I think it is reasonable, especially when we don't check AVX10 enable bit
> in max_x86_cpu_realize(). But checking prefix here again seems not
> necessary.

Thanks! We only need the second check since have_filtered_features
should change whether prefix exists or not.

> > +            warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> > +        }
> > +        have_filtered_features = true;
> >      }
> > 
> >      return have_filtered_features;
> > 
> > > +    return have_filtered_features;
> > >  }
> > >  
> > >  static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > > @@ -8395,6 +8444,7 @@ static Property x86_cpu_properties[] = {
> > >      DEFINE_PROP_UINT32("min-level", X86CPU, env.cpuid_min_level, 0),
> > >      DEFINE_PROP_UINT32("min-xlevel", X86CPU, env.cpuid_min_xlevel, 0),
> > >      DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
> > > +    DEFINE_PROP_UINT8("avx10-version", X86CPU, env.avx10_version, 0),
> > 
> > As my first comment, we could consider changing the default value to -1.
> > 
> 
> I still think 0 is better…
>