How to mark internal properties (was: Re: [PATCH v4 12/27] target/i386/cpu: Remove CPUX86State::enable_cpuid_0xb field)

[Thomas Huth <thuth@redhat.com>]

On 09/05/2025 09.32, Zhao Liu wrote:
> 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.

Thanks for your clarifications, Zhao! But I think this shows again the 
problem that we have hit a couple of times in the past already: Properties 
are currently used for both, config knobs for the users and internal 
switches for configuration of the machine. We lack a proper way to say "this 
property is usable for the user" and "this property is meant for internal 
configuration only".

I wonder whether we could maybe come up with a naming scheme to better 
distinguish the two sets, e.g. by using a prefix similar to the "x-" prefix 
for experimental properties? We could e.g. say that all properties starting 
with a "q-" are meant for QEMU-internal configuration only or something 
similar (and maybe even hide those from the default help output when running 
"-device xyz,help" ?)? Anybody any opinions or better ideas on this?

  Thomas


> 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
> 
>