Re: [PATCH 3/6] target/i386: Add CPUID.24 leaf for AVX10

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

On Mon, Oct 28, 2024 at 11:04:07PM +0800, Xiaoyao Li wrote:
> On 10/28/2024 10:45 AM, Tao Su wrote:
> > +    case 0x24: {
> > +        *eax = 0;
> > +        *ebx = 0;
> > +        *ecx = 0;
> > +        *edx = 0;
> > +        if (!(env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10)) {
> > +            break;
> > +        }
> > +
> > +        if (count == 0) {
> > +            uint8_t v = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24,
> > +                                                     0, R_EBX);
> > +            if (env->avx10_version && env->avx10_version < v) {
> > +                v = env->avx10_version;
> > +            }
> 
> Here, if user specified avx10_version is >= kvm reported value, it uses
> KVM's reported value silently.
> 
> I think it's not good. It'd better to validate if user specified value can
> be satisfied or not, and emit a warning when not. e.g., in
> x86_cpu_filter_features() or in kvm_cpu_realizefn(). Also we can put the
> behavior along with it that "use KVM reported maximum value when
> avx10_version is 0 "
> 
> then, here we can simply do
> 
> 	*ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;

Is it necessary to add enforce_cpuid for avx10_version too?

How about checking this in x86_cpu_realizefn, because I see this may be a
more suitable place to implement check_cpuid and enforce_cpuid.


@@ -7816,6 +7810,29 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)

     x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);

+    if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
+        uint8_t version =
+            kvm_arch_get_supported_cpuid(cs->kvm_state, 0x24, 0, R_EBX);
+
+        if (!env->avx10_version) {
+            env->avx10_version = version;
+        }
+
+        if (version < env->avx10_version) {
+            const char *msg = accel_uses_host_cpuid()
+                              ? "Host doesn't support requested feature"
+                              : "TCG doesn't support requested feature";
+            if (cpu->enforce_cpuid) {
+                error_setg(&local_err, "%s: avx10.%d", msg,
+                           env->avx10_version);
+                goto out;
+            } else if (cpu->check_cpuid) {
+                warn_report("%s: avx10.%d", msg, env->avx10_version);
+            }
+            env->avx10_version = version;
+        }
+    }
+
     if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
         error_setg(&local_err,
                    accel_uses_host_cpuid() ?