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

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

On Tue, Oct 29, 2024 at 09:25:53AM +0100, Paolo Bonzini wrote:
> On 10/28/24 03:45, Tao Su wrote:
> > @@ -6835,6 +6850,26 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >           }
> >           break;
> >       }
> > +    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;
> > +            }
> > +
> > +            *ebx = env->features[FEAT_24_0_EBX] | v;
> > +        }
> > +        break;
> > +    }
> >       case 0x40000000:
> >           /*
> >            * CPUID code in kvm_arch_init_vcpu() ignores stuff
> 
> This check should be done elsewhere (called by x86_cpu_realizefn());
> cpu_x86_cpuid() should only report the value:
> 
>          if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && count == 0) {
>              *ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
>          }
> 
> Also, the check should use x86_cpu_get_supported_cpuid() because KVM is not the
> only accelerator.
> 

Yes, I see, I add check here:

@@ -7679,6 +7719,27 @@ 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) {
+        uint32_t eax, ebx, ecx, edx;
+        x86_cpu_get_supported_cpuid(0x24, 0, &eax, &ebx, &ecx, &edx);
+
+        ebx &= 0xff;
+
+        if (ebx < 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 = ebx;
+        }
+    }
+
     if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
         error_setg(&local_err,
                    accel_uses_host_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;
> > +        }
> > +
> 
> This should not be done here, but in max_x86_cpu_realize().  It should also
> use x86_cpu_get_supported_cpuid().
> 

Yes, I try to add here:

@@ -5327,6 +5365,12 @@ static void max_x86_cpu_realize(DeviceState *dev, Error **errp)
         }
     }

+    if (!object_property_get_uint(obj, "avx10-version", &error_abort)) {
+        uint32_t eax, ebx, ecx, edx;
+        x86_cpu_get_supported_cpuid(0x24, 0, &eax, &ebx, &ecx, &edx);
+        object_property_set_uint(obj, "avx10-version", ebx & 0xff, &error_abort);
+    }
+
     x86_cpu_realizefn(dev, errp);
 }


> For Granite Rapids you're only setting the AVX10 version in v2 and therefore
> you don't need it, but there should also be (for the future) an avx10_version
> field in X86CPUDefinition, which is set into the avx10-version property at
> x86_cpu_load_model().
> 

Yes, for new CPU model, we should do that.

> > index d845384dcd..5566a13f4f 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -662,6 +662,7 @@ typedef enum FeatureWord {
> >       FEAT_XSAVE_XSS_HI,     /* CPUID[EAX=0xd,ECX=1].EDX */
> >       FEAT_7_1_EDX,       /* CPUID[EAX=7,ECX=1].EDX */
> >       FEAT_7_2_EDX,       /* CPUID[EAX=7,ECX=2].EDX */
> > +    FEAT_24_0_EBX,      /* CPUID[EAX=0x24,ECX=0].EBX */
> 
> Adding FEAT_24_0_EBX should be a separate patch.
> 

Yes, all you said above are excellent suggestions and I have tested on
my platform. Should I submit a v2 with these changes or wait for you to
send v2 directly? :-)