On Wed, 23 Apr 2025 19:46:58 +0800
Zhao Liu <zhao1....@intel.com> wrote:
> From: Xiaoyao Li <xiaoyao...@intel.com>
>
> Currently, QEMU exposes CPUID 0x1f to guest only when necessary, i.e.,
> when topology level that cannot be enumerated by leaf 0xB, e.g., die or
> module level, are configured for the guest, e.g., -smp xx,dies=2.
>
> However, TDX architecture forces to require CPUID 0x1f to configure CPU
> topology.
>
> Introduce a bool flag, enable_cpuid_0x1f, in CPU for the case that
> requires CPUID leaf 0x1f to be exposed to guest.
>
> Introduce a new function x86_has_cpuid_0x1f(), which is the warpper of
> cpu->enable_cpuid_0x1f and x86_has_extended_topo() to check if it needs
> to enable cpuid leaf 0x1f for the guest.
that reminds me about recent attempt to remove enable_cpuid_0xb,
So is enable_cpuid_0x1f inteded to be used by external users or
it's internal only knob for TDX sake?
I'd push for it being marked as internal|unstable with the means
we currently have (i.e. adding 'x-' prefix)
and also enable_ is not right here, the leaf is enabled when
topology requires it.
perhaps s/enable_/force_/
>
> Signed-off-by: Xiaoyao Li <xiaoyao...@intel.com>
> ---
> target/i386/cpu.c | 4 ++--
> target/i386/cpu.h | 9 +++++++++
> target/i386/kvm/kvm.c | 2 +-
> 3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d90e048d48f2..e0716dbe5934 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7292,7 +7292,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> break;
> case 0x1F:
> /* V2 Extended Topology Enumeration Leaf */
> - if (!x86_has_extended_topo(env->avail_cpu_topo)) {
> + if (!x86_has_cpuid_0x1f(cpu)) {
> *eax = *ebx = *ecx = *edx = 0;
> break;
> }
> @@ -8178,7 +8178,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> * cpu->vendor_cpuid_only has been unset for compatibility with older
> * machine types.
> */
> - if (x86_has_extended_topo(env->avail_cpu_topo) &&
> + if (x86_has_cpuid_0x1f(cpu) &&
> (IS_INTEL_CPU(env) || !cpu->vendor_cpuid_only)) {
> x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
> }
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 76f24446a55d..3910b488f775 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2251,6 +2251,9 @@ struct ArchCPU {
> /* Compatibility bits for old machine types: */
> bool enable_cpuid_0xb;
>
> + /* Force to enable cpuid 0x1f */
> + bool enable_cpuid_0x1f;
> +
> /* Enable auto level-increase for all CPUID leaves */
> bool full_cpuid_auto_level;
>
> @@ -2513,6 +2516,12 @@ void host_cpuid(uint32_t function, uint32_t count,
> uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
> bool cpu_has_x2apic_feature(CPUX86State *env);
>
> +static inline bool x86_has_cpuid_0x1f(X86CPU *cpu)
> +{
> + return cpu->enable_cpuid_0x1f ||
> + x86_has_extended_topo(cpu->env.avail_cpu_topo);
> +}
> +
> /* helper.c */
> void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
> void cpu_sync_avx_hflag(CPUX86State *env);
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 6c749d4ee812..23b8de308525 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1863,7 +1863,7 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
> break;
> }
> case 0x1f:
> - if (!x86_has_extended_topo(env->avail_cpu_topo)) {
> + if (!x86_has_cpuid_0x1f(env_archcpu(env))) {
> cpuid_i--;
> break;
> }
