Re: How to mark internal properties

[Markus Armbruster <armbru@redhat.com>]

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>