Re: [PATCH v2] i386/cpu: Remove FEAT_24_0_EBX for AVX10

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

Hi Xiaoyao,

Rethinking this patch, I would not remove these feature bits with the
comments inline:

> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0d35e95430fe..1b50fd4e397d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -912,7 +912,6 @@ 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 | \
> @@ -1208,20 +1207,6 @@ 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,
> -    },

"Reserved at 1" means always enabling, not being deprecated.

So if someone has already cofigured "+avx10-128,+avx10-256,+avx10-512"
for his cpu, this patch will break his use.

Although explicitly setting AVX10 vector length feature bits appears
uncommon, but explicitly masking off these feature bits is even more
unusual.

Removing these bits has little benefit but brings the risk of breaking
the API.

In the future, QEMU may need the AVX10 model to handle different
versions. A complete feature list is necessary. This is another reason.

In addition, I also agree with KVM's point to keep these feature names:

https://lore.kernel.org/kvm/Zkz5Ak0PQlAN8DxK@google.com/

...

> @@ -7720,7 +7686,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          *ecx = 0;
>          *edx = 0;
>          if ((env->features[FEAT_7_1_EDX] & CPUID_7_1_EDX_AVX10) && count == 0) {
> -            *ebx = env->features[FEAT_24_0_EBX] | env->avx10_version;
> +            /* bit 16-18 are reserved at 1 */
> +            *ebx = (0x7U << 16) | env->avx10_version;

Hardcoding is not friendly for maintenance, and they are essentially
feature bits.

>          }
>          break;
>      }

Thanks,
Zhao