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...@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é <phi...@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....@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
