Re: [PATCH v1 1/9] target/i386: Sync AMD CPUID aliases for Hygon

> -GlobalProperty pc_compat_11_0[] = {};
> +GlobalProperty pc_compat_11_0[] = {
> +    { TYPE_X86_CPU, "x-hygon-vendor-abi-fixes", "false" },
> +};

This needs rebase on v11.1 :)

>  const size_t pc_compat_11_0_len = G_N_ELEMENTS(pc_compat_11_0);
>  
>  GlobalProperty pc_compat_10_2[] = {};
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5805d33ab9..32b8bc9ebd 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -8608,6 +8608,20 @@ uint32_t cpu_x86_virtual_addr_width(CPUX86State *env)
>      }
>  }
>  
> +/*
> + * AMD CPUs define CPUID[0x80000001].EDX aliases for selected CPUID[1].EDX
> + * feature bits.  Hygon Dhyana follows the same extended feature alias rules.
> + * Enable the corrected Hygon behavior only for machine types where Hygon
> + * vendor ABI fixes are on.
> + */
> +static bool x86_cpu_has_amd_cpuid_aliases(const X86CPU *cpu)
> +{
> +    const CPUX86State *env = &cpu->env;
> +
> +    return IS_AMD_CPU(env) ||
> +           (cpu->hygon_vendor_abi_fixes && IS_HYGON_CPU(env));
> +}
> +
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                     uint32_t *eax, uint32_t *ebx,
>                     uint32_t *ecx, uint32_t *edx)
> @@ -10147,10 +10161,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> -     * CPUID[1].EDX.
> +    /*
> +     * CPUs that use AMD-compatible extended CPUID aliases must keep selected
> +     * CPUID[0x80000001].EDX bits synchronized with CPUID[1].EDX.
>       */
> -    if (IS_AMD_CPU(env)) {
> +    if (x86_cpu_has_amd_cpuid_aliases(cpu)) {

Or we can omit this helper ("x86_cpu_has_amd_cpuid_aliases(cpu)") and
instead do the check directly inline?

I think we should have fewer helpers like this, unless the conditional
checks are really lengthy... Since the helpers added for each ABI is
similar, but we cant consolidate them into a single generic helper
(because while the AMD and Hygon ABIs are basically similar, there are
still differences), having too many similar helpers makes the code seem
fragmented.

> @@ -2461,6 +2467,12 @@ struct ArchCPU {
>       */
>      bool vendor_cpuid_only_v2;
>  
> +    /*
> +     * Enable Hygon vendor ABI fixes for new machine types.  Old machine
> +     * types disable this to preserve guest-visible CPU ABI.
> +     */
> +    bool hygon_vendor_abi_fixes;
> +

Maybe the following comment to emphasize this is a compat option?

/*
 * Compatibility bits for old machine types: if true, apply Hygon
 * vendor-specific ABI fixes.  Old machine types disable this to
 * preserve the guest-visible CPU ABI.
 */

>      /* Only advertise TOPOEXT features that AMD defines */
>      bool amd_topoext_features_only;
>  
> diff --git a/tests/qtest/test-x86-cpuid-compat.c b/tests/qtest/test-x86-cpuid-compat.c
> index 17c0965827..b7f8834052 100644
> --- a/tests/qtest/test-x86-cpuid-compat.c
> +++ b/tests/qtest/test-x86-cpuid-compat.c
> @@ -113,6 +113,21 @@ typedef struct FeatureTestArgs {
>      bool expected_value;
>  } FeatureTestArgs;
>  
> +typedef struct BoolPropTestArgs {
> +    /* Test name */
> +    const char *name;
> +    /* CPU type */
> +    const char *cpu;
> +    /* CPU features (may be NULL) */
> +    const char *cpufeat;
> +    /* machine type (may be NULL to use default machine) */
> +    const char *machine;
> +    /* CPU property to read */
> +    const char *property;
> +    /* expected value of the property */
> +    bool expected_value;
> +} BoolPropTestArgs;

Good test!

Regards,
Zhao