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

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

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.

> 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);

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

Thanks,
Zhao