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

[Tao Su <tao1.su@linux.intel.com>]

On Wed, Oct 30, 2024 at 04:44:54PM +0800, Zhao Liu wrote:
> On Tue, Oct 29, 2024 at 04:18:54PM +0100, Paolo Bonzini wrote:
> > Date: Tue, 29 Oct 2024 16:18:54 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 4/8] target/i386: add AVX10 feature and AVX10 version
> >  property
> > X-Mailer: git-send-email 2.47.0
> > 
> > From: Tao Su <tao1.su@linux.intel.com>
> > 
> > When AVX10 enable bit is set, the 0x24 leaf will be present as "AVX10
> > Converged Vector ISA leaf" containing fields for the version number and
> > the supported vector bit lengths.
> > 
> > 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 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!

> > +            }
> > +            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().

> +        } 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) {
> +        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.

> +            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…