Re: [PATCH v4 12/27] target/i386/cpu: Remove CPUX86State::enable_cpuid_0xb field

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

On Fri, May 09, 2025 at 02:49:27PM +0800, Xiaoyao Li wrote:
> Date: Fri, 9 May 2025 14:49:27 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v4 12/27] target/i386/cpu: Remove
>  CPUX86State::enable_cpuid_0xb field
> 
> On 5/8/2025 9:35 PM, Philippe Mathieu-Daudé wrote:
> > The CPUX86State::enable_cpuid_0xb boolean was only disabled
> > for the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
> > removed. Being now always %true, we can remove it and simplify
> > cpu_x86_cpuid().
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   target/i386/cpu.h | 3 ---
> >   target/i386/cpu.c | 6 ------
> >   2 files changed, 9 deletions(-)
> > 
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 0db70a70439..06817a31cf9 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -2241,9 +2241,6 @@ struct ArchCPU {
> >        */
> >       bool legacy_multi_node;
> > -    /* Compatibility bits for old machine types: */
> > -    bool enable_cpuid_0xb;
> > -
> >       /* Enable auto level-increase for all CPUID leaves */
> >       bool full_cpuid_auto_level;
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 49179f35812..6fe37f71b1e 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6982,11 +6982,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >           break;
> >       case 0xB:
> >           /* Extended Topology Enumeration Leaf */
> > -        if (!cpu->enable_cpuid_0xb) {
> > -                *eax = *ebx = *ecx = *edx = 0;
> > -                break;
> > -        }
> > -
> >           *ecx = count & 0xff;
> >           *edx = cpu->apic_id;
> > @@ -8828,7 +8823,6 @@ static const Property x86_cpu_properties[] = {
> >       DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
> >       DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
> >       DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
> > -    DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
> 
> It's deprecating the "cpuid-0xb" property.
> 
> I think we need go with the standard process to deprecate it.

Thanks! I got your point.

Though this property is introduced for compatibility, as its comment
said "Compatibility bits for old machine types", it is also useful for 
somer users.

Fo example, in the early development stages of TDX, when there was no
full support for CPU topology, Intel had disable this property for
testing and found this bug:

https://lore.kernel.org/qemu-devel/20250227062523.124601-3-zhao1.liu@intel.com/

So, I think there may be other similar use cases as well.

And, if someone wants to emulate ancient x86 CPUs (though I can't
currently confirm from which generation of CPUs 0xb support started), he
may want to consider disable this property as well.

The main problem here is that the "property" mechanism doesn't
distinguish between internal use/public use, and although it was originally
intended for internal QEMU use, it also leaks to the user, creating some
external use cases.

@Philippe, thank you for cleaning up this case! I think we can keep this
property, and if you don't mind, I can modify its comment later to
indicate that it's used to adjust the topology support for the CPU.

Thanks,
Zhao