| Message ID | 20250508133550.81391-13-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386/pc: Remove deprecated 2.6 and 2.7 PC machines | expand |
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. > DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true), > DEFINE_PROP_BOOL("x-amd-topoext-features-only", X86CPU, amd_topoext_features_only, true), > DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
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
On Fri, May 09, 2025 at 12:04:19PM +0200, Thomas Huth wrote: > Date: Fri, 9 May 2025 12:04:19 +0200 > From: Thomas Huth <thuth@redhat.com> > Subject: How to mark internal properties (was: Re: [PATCH v4 12/27] > target/i386/cpu: Remove CPUX86State::enable_cpuid_0xb field) > > 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". Hi Thomas, thank you. AFAIK, there are two ways to configure whether an object/device is allowed to be created by user or not: * TYPE_USER_CREATABLE * DeviceClass: user_creatable So, it looks like it would be tricky to change the infrastructure around object_property_add because it's not easy to be compatible with both of the above user creation ways. > 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? Therefore, I think the “q-” prefix might be a good way, simple and effective. Let's see if any other maintainers have a better idea. Regards, Zhao
Thomas Huth <thuth@redhat.com> writes: > 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". Correct. Exposing properties meant for internal use at the external interface inevitably leads to (uncertainty about) external use. > 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? This papers over our inability / unwillingness to isolate the external interface from internal detail. The proper solution is to make the internal properties inaccessible at the external interface. This requires declaring properties' intent. Which strikes me as a very good idea. A naming convention is a simple, stupid way to do that. There are drawbacks, as experience with the "x-" prefix has shown: * Flipping a flag bit involves changing the name. Tolerable when all uses are internal, compatibility break when not. Not a problem when the bit governs external access, of course. * Name capture: consider InputBarrier properties x-origin, y-origin. Oops. * If we have multiple flag bits, their prefixes can accumulate. This gets ugly and confusing real quick. Not an issue when at most one of the flags can be set, as is the case for "unstable" and "internal use". * QAPI reserves "q_" for the generator's use. Since "q-" would get mapped to "q_" in C, we risk name clashes. For what it's worth, QAPI abandoned the "x-" naming convention (commit a3c45b3e629 (qapi: New special feature flag "unstable"), commit message appended for your convenience). Developers are free to use "x-" to help guide human users, but the feature flag is the sole source of thruth. [...] commit a3c45b3e62962f99338716b1347cfb0d427cea44 Author: Markus Armbruster <armbru@redhat.com> Date: Thu Oct 28 12:25:12 2021 +0200 qapi: New special feature flag "unstable" By convention, names starting with "x-" are experimental. The parts of external interfaces so named may be withdrawn or changed incompatibly in future releases. The naming convention makes unstable interfaces easy to recognize. Promoting something from experimental to stable involves a name change. Client code needs to be updated. Occasionally bothersome. Worse, the convention is not universally observed: * QOM type "input-barrier" has properties "x-origin", "y-origin". Looks accidental, but it's ABI since 4.2. * QOM types "memory-backend-file", "memory-backend-memfd", "memory-backend-ram", and "memory-backend-epc" have a property "x-use-canonical-path-for-ramblock-id" that is documented to be stable despite its name. We could document these exceptions, but documentation helps only humans. We want to recognize "unstable" in code, like "deprecated". So support recognizing it the same way: introduce new special feature flag "unstable". It will be treated specially by the QAPI generator, like the existing feature flag "deprecated", and unlike regular feature flags. This commit updates documentation and prepares tests. The next commit updates the QAPI schema. The remaining patches update the QAPI generator and wire up -compat policy checking. Management applications can then use query-qmp-schema and -compat to manage or guard against use of unstable interfaces the same way as for deprecated interfaces. docs/devel/qapi-code-gen.txt no longer mandates the naming convention. Using it anyway might help writers of programs that aren't full-fledged management applications. Not using it can save us bothersome renames. We'll see how that shakes out. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20211028102520.747396-2-armbru@redhat.com>
On Fri, 9 May 2025 at 11:04, Thomas Huth <thuth@redhat.com> wrote: > 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? I think a q-prefix is potentially a bit clunky unless we also have infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) and have it auto-add the prefix, and to have the C APIs for setting properties search for both "foo" and "q-foo" so you don't have to write qdev_prop_set_bit(dev, "q-foo", ...). thanks -- PMM
On Mon, May 12, 2025 at 09:46:30AM +0100, Peter Maydell wrote: > On Fri, 9 May 2025 at 11:04, Thomas Huth <thuth@redhat.com> wrote: > > 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? > > I think a q-prefix is potentially a bit clunky unless we also have > infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) > and have it auto-add the prefix, and to have the C APIs for > setting properties search for both "foo" and "q-foo" so you > don't have to write qdev_prop_set_bit(dev, "q-foo", ...). I think it is also not obvious enough that a 'q-' prefix means private. Perhaps borrow from the C world and declare that a leading underscore indicates a private property. People are more likely to understand and remember that, than 'q-'. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, May 12, 2025 at 09:46:30AM +0100, Peter Maydell wrote: >> On Fri, 9 May 2025 at 11:04, Thomas Huth <thuth@redhat.com> wrote: >> > 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? >> >> I think a q-prefix is potentially a bit clunky unless we also have >> infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) >> and have it auto-add the prefix, and to have the C APIs for >> setting properties search for both "foo" and "q-foo" so you >> don't have to write qdev_prop_set_bit(dev, "q-foo", ...). If we make intent explicit with DEFINE_INTERNAL_PROP_FOO(), is repeating intent in the name useful? > I think it is also not obvious enough that a 'q-' prefix means private. Concur. > Perhaps borrow from the C world and declare that a leading underscore > indicates a private property. People are more likely to understand and > remember that, than 'q-'. This is fine for device properties now. It's not fine for properties of user-creatable objects, because these are defined in QAPI, and QAPI prohibits names starting with a single underscore. I append relevant parts of docs/devel/qapi-code-gen.rst for your convenience. Why does QAPI prohibit leading underscores? Chiefly because such names are reserved identifiers in C. Instead of complicating the mapping from QAPI name to C identifier, we restrict QAPI names and call it a day. The mapping between device property name and C identifiers is entirely manual. When a property is backed by a member of the device state struct, naming the member exactly like the property makes sense. Having to mentally strip / insert a leading underscore would hardly be terrible, just a bit of friction. I'd prefer not to. Naming rules and reserved names ------------------------------- All names must begin with a letter, and contain only ASCII letters, digits, hyphen, and underscore. There are two exceptions: enum values may start with a digit, and names that are downstream extensions (see section `Downstream extensions`_) start with underscore. Names beginning with ``q_`` are reserved for the generator, which uses them for munging QMP names that resemble C keywords or other problematic strings. For example, a member named ``default`` in qapi becomes ``q_default`` in the generated C code. [...] Downstream extensions --------------------- QAPI schema names that are externally visible, say in the Client JSON Protocol, need to be managed with care. Names starting with a downstream prefix of the form __RFQDN_ are reserved for the downstream who controls the valid, reverse fully qualified domain name RFQDN. RFQDN may only contain ASCII letters, digits, hyphen and period. Example: Red Hat, Inc. controls redhat.com, and may therefore add a downstream command ``__com.redhat_drive-mirror``.
On 5/12/2025 6:54 PM, Markus Armbruster wrote: > Daniel P. Berrangé<berrange@redhat.com> writes: > >> On Mon, May 12, 2025 at 09:46:30AM +0100, Peter Maydell wrote: >>> On Fri, 9 May 2025 at 11:04, Thomas Huth<thuth@redhat.com> wrote: >>>> 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? >>> I think a q-prefix is potentially a bit clunky unless we also have >>> infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) >>> and have it auto-add the prefix, and to have the C APIs for >>> setting properties search for both "foo" and "q-foo" so you >>> don't have to write qdev_prop_set_bit(dev, "q-foo", ...). > If we make intent explicit with DEFINE_INTERNAL_PROP_FOO(), is repeating > intent in the name useful? +1 for DEFINE_INTERNAL_PROP_FOO(). I have the same thought. We need something in code to restrict the *internal* property really internal, i.e., not user settable. What the name of the property is doesn't matter.
On Mon, 12 May 2025, Xiaoyao Li wrote: > On 5/12/2025 6:54 PM, Markus Armbruster wrote: >> Daniel P. Berrangé<berrange@redhat.com> writes: >>> On Mon, May 12, 2025 at 09:46:30AM +0100, Peter Maydell wrote: >>>> On Fri, 9 May 2025 at 11:04, Thomas Huth<thuth@redhat.com> wrote: >>>>> 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? >>>> I think a q-prefix is potentially a bit clunky unless we also have >>>> infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) >>>> and have it auto-add the prefix, and to have the C APIs for >>>> setting properties search for both "foo" and "q-foo" so you >>>> don't have to write qdev_prop_set_bit(dev, "q-foo", ...). > >> If we make intent explicit with DEFINE_INTERNAL_PROP_FOO(), is repeating >> intent in the name useful? > > +1 for DEFINE_INTERNAL_PROP_FOO(). I have the same thought. > > We need something in code to restrict the *internal* property really > internal, i.e., not user settable. What the name of the property is doesn't > matter. What's an internal property? Properties are there to make some field of an object introspectable and settable from command line and QEMU monitor or other external interfaces. If that's not needed for something why is it defined as a property in the first place and not just e.g. C accessor functions as part of the device's interface instead? I think this may be overusing QOM for things that may not need it and adding complexity where not needed. It reminds me of patches that wanted to export via-ide IRQs or ISA IRQs just to be able to connect them to other parts _of the same chip_ becuase this chip is modeled as multiple QOM objects for reusing code from those. But in reality the chip does not have such pins and these are internal connections so I think it would be better to model these as functions and not QOM constructs that the user can change. In general, if the device or object has an external connection or a knob that the user may need to change or connect to another device (like building a board from parts you can wire pins together) then those need properties or qemu_irqs but other "internal properties" may need some other way to access and often simple accessor functions are enough for this as these internal properties are only accessed form the code. That way we would not need even more complexity to hide these from the user, instead of that just don't expose them but use something else where a property is not needed. A property is just like an accessor function with additional complexity to expose it to other interfaces so it's externally settable and introspectable but we don't need those for internal properties so we can drop that complexity and get back to the accessor function at the bottom of it. Regards, BALATON Zoltan
On 12/05/2025 11:54, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Mon, May 12, 2025 at 09:46:30AM +0100, Peter Maydell wrote: >>> On Fri, 9 May 2025 at 11:04, Thomas Huth <thuth@redhat.com> wrote: >>>> 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? >>> >>> I think a q-prefix is potentially a bit clunky unless we also have >>> infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) >>> and have it auto-add the prefix, and to have the C APIs for >>> setting properties search for both "foo" and "q-foo" so you >>> don't have to write qdev_prop_set_bit(dev, "q-foo", ...). > > If we make intent explicit with DEFINE_INTERNAL_PROP_FOO(), is repeating > intent in the name useful? > >> I think it is also not obvious enough that a 'q-' prefix means private. > > Concur. > >> Perhaps borrow from the C world and declare that a leading underscore >> indicates a private property. People are more likely to understand and >> remember that, than 'q-'. > > This is fine for device properties now. It's not fine for properties of > user-creatable objects, because these are defined in QAPI, and QAPI > prohibits names starting with a single underscore. I append relevant > parts of docs/devel/qapi-code-gen.rst for your convenience. On a related note this also brings us back to the discussion as to the relationship between qdev and QOM: at one point I was under the impression that qdev properties were simply QOM properties that were exposed externally, i.e on the commmand line for use with -device. Can you provide an update on what the current thinking is in this area, in particular re: scoping of qdev vs QOM properties? ATB, Mark.
On Fri, 9 May 2025 15:32:30 +0800 Zhao Liu <zhao1.liu@intel.com> 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> ... > > > @@ -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), ... > @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. +1, we should not delete this without due process (aka deprecation). So perhaps deprecate now and remove in couple of releases
On Mon, 12 May 2025 12:54:26 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Mon, May 12, 2025 at 09:46:30AM +0100, Peter Maydell wrote: > >> On Fri, 9 May 2025 at 11:04, Thomas Huth <thuth@redhat.com> wrote: > >> > 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? > >> > >> I think a q-prefix is potentially a bit clunky unless we also have > >> infrastructure to say eg DEFINE_INTERNAL_PROP_BOOL("foo", ...) > >> and have it auto-add the prefix, and to have the C APIs for > >> setting properties search for both "foo" and "q-foo" so you > >> don't have to write qdev_prop_set_bit(dev, "q-foo", ...). > > If we make intent explicit with DEFINE_INTERNAL_PROP_FOO(), is repeating > intent in the name useful? While we are inventing a new API, I'd say that _INTERNAL_ is not the only thing on my wish-list wrt properties. It would be also nice to know when a property is set by internal or external user or if it still has default value. Basically we are looking at different flags for properties and INERNAL being one of them. Maybe instead of specialized macro, we should have a more generic DEFINE_PROP_WITH_FLAGS_FOO(...,flags) So we won't have to rewrite it again when we think of another flag to turn on/off. From previous uses of x- flag, some of such properties are created as temporary | developer-only and occasionally as a crutch (still no intended for end user). But then sometimes such properties get promoted to ABI with fat warnings not to touch them. Having stable|unstable flag could help here without need to rename property (and prevent breaking users who (ab)used it if we care).
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), DEFINE_PROP_BOOL("x-vendor-cpuid-only", X86CPU, vendor_cpuid_only, true), DEFINE_PROP_BOOL("x-amd-topoext-features-only", X86CPU, amd_topoext_features_only, true), DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
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(-)