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

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 10/28/2024 10:45 AM, Tao Su wrote:
> 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.
> 
> Tested-by: Xuelian Guo <xuelian.guo@intel.com>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> ---
>   target/i386/cpu.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>   target/i386/cpu.h     |  8 ++++++++
>   target/i386/kvm/kvm.c |  3 ++-
>   3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5b434a107a..91fae0dcb7 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -898,6 +898,7 @@ void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>   #define TCG_SGX_12_0_EAX_FEATURES 0
>   #define TCG_SGX_12_0_EBX_FEATURES 0
>   #define TCG_SGX_12_1_EAX_FEATURES 0
> +#define TCG_24_0_EBX_FEATURES 0
>   
>   #if defined CONFIG_USER_ONLY
>   #define CPUID_8000_0008_EBX_KERNEL_FEATURES (CPUID_8000_0008_EBX_IBPB | \
> @@ -1163,6 +1164,20 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>           },
>           .tcg_features = TCG_7_2_EDX_FEATURES,
>       },
> +    [FEAT_24_0_EBX] = {
> +        .type = CPUID_FEATURE_WORD,
> +        .feat_names = {
> +            [16] = "avx10-128",
> +            [17] = "avx10-256",
> +            [18] = "avx10-512",
> +        },
> +        .cpuid = {
> +            .eax = 0x24,
> +            .needs_ecx = true, .ecx = 0,
> +            .reg = R_EBX,
> +        },
> +        .tcg_features = TCG_24_0_EBX_FEATURES,
> +    },
>       [FEAT_8000_0007_EDX] = {
>           .type = CPUID_FEATURE_WORD,
>           .feat_names = {
> @@ -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;
> +            }

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;

> +            *ebx = env->features[FEAT_24_0_EBX] | v;
> +        }
> +        break;
> +    }
>       case 0x40000000:
>           /*
>            * CPUID code in kvm_arch_init_vcpu() ignores stuff
> @@ -7483,6 +7518,11 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>               x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>           }
>   
> +        /* Advanced Vector Extensions 10 (AVX10) requires CPUID[0x24] */
> +        if (env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) {
> +            x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x24);
> +        }
> +
>           /* SVM requires CPUID[0x8000000A] */
>           if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
>               x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A);
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> 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 */
>       FEATURE_WORDS,
>   } FeatureWord;
>   
> @@ -990,6 +991,13 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
>   /* Packets which contain IP payload have LIP values */
>   #define CPUID_14_0_ECX_LIP              (1U << 31)
>   
> +/* AVX10 128-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_128        (1U << 16)
> +/* AVX10 256-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_256        (1U << 17)
> +/* AVX10 512-bit vector support is present */
> +#define CPUID_24_0_EBX_AVX10_512        (1U << 18)
> +
>   /* RAS Features */
>   #define CPUID_8000_0007_EBX_OVERFLOW_RECOV    (1U << 0)
>   #define CPUID_8000_0007_EBX_SUCCOR      (1U << 1)
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index fd9f198892..8e17942c3b 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1923,7 +1923,8 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
>           case 0x7:
>           case 0x14:
>           case 0x1d:
> -        case 0x1e: {
> +        case 0x1e:
> +        case 0x24: {
>               uint32_t times;
>   
>               c->function = i;