Re: [PATCH v5 2/4] docs/interop/firmware.json: add new enum FirmwareArchitecture

[Markus Armbruster <armbru@redhat.com>]

Thomas Weißschuh <t-8ch@linutronix.de> writes:

> On Thu, Jul 18, 2024 at 03:18:07PM GMT, Markus Armbruster wrote:
>> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
>> 
>> > Only a small subset of all architectures supported by qemu make use of
>> > firmware files. Introduce and use a new enum to represent this.
>> >
>> > This also removes the dependency to machine.json from the global qapi
>> > definitions.
>> >
>> > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> > ---
>> >  docs/interop/firmware.json | 29 +++++++++++++++++++++++++++--
>> >  1 file changed, 27 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> > index a26fe81bf2fe..2eb0be11d595 100644
>> > --- a/docs/interop/firmware.json
>> > +++ b/docs/interop/firmware.json
>> > @@ -14,7 +14,10 @@
>> >  # = Firmware
>> >  ##
>> >  
>> > -{ 'include' : 'machine.json' }
>> > +{ 'pragma': {
>> > +    'member-name-exceptions': [
>> > +        'FirmwareArchitecture' # x86_64
>> > +    ] } }
>> >  
>> >  ##
>> >  # @FirmwareOSInterface:
>> > @@ -59,6 +62,28 @@
>> >  { 'enum' : 'FirmwareDevice',
>> >    'data' : [ 'flash', 'kernel', 'memory' ] }
>> >  
>> > +##
>> > +# @FirmwareArchitecture:
>> > +#
>> > +# Enumerations of architectures for which Qemu uses additional firmware files.
>> 
>> docs/devel/qapi-code-gen.rst section "Documentation markup":
>> 
>>     For legibility, wrap text paragraphs so every line is at most 70
>>     characters long.
>> 
>> > +# The values are a subset of the enum SysEmuTarget.
>
> Ack.
>
>> Will consumers of firmware.json care for this?
>
> Most probably not.
>
>> Or is it just a reminder for developers to keep the two enums in sync?
>
> I guess so.
> Should I drop it?

You can drop it.

If you think it's useful for developers, you can instead make it a
non-doc comment, like ...

>> 
>> > +#
>> > +# @aarch64: 64-bit Arm.
>> > +#
>> > +# @arm: 32-bit Arm.
>> > +#
>> > +# @i386: 32-bit x86.
>> > +#
>> > +# @loongarch64: 64-bit LoongArch.
>> > +#
>> > +# @x86_64: 64-bit x86.
>> > +#
>> > +# Since: 9.1
>> 
>> The enum type is indeed since 9.1, but its members are since 3.0, and
>> that's what matters.  Except for @loongarch, which is since 7.1.0 (not
>> documented in qapi/machine.json; I'll fix that).
>> 
>> > +##
>> > +{ 'enum' : 'FirmwareArchitecture',
>> > +  'data' : [ 'aarch64', 'arm', 'i386', 'loongarch64', 'x86_64' ] }

... here:

      # The values are a subset of the enum SysEmuTarget defined in
      # qapi/machine.json.

If you do, consider adding a similar note to FirmwareFormat in the
previous patch.

>> > +
>> > +
>> 
>> Drop one blank line, please.
>
> Ack.
>
>> 
>> >  ##
>> >  # @FirmwareTarget:
>> >  #
>> > @@ -80,7 +105,7 @@
>> >  # Since: 3.0
>> >  ##
>> >  { 'struct' : 'FirmwareTarget',
>> > -  'data'   : { 'architecture' : 'SysEmuTarget',
>> > +  'data'   : { 'architecture' : 'FirmwareArchitecture',
>> >                 'machines'     : [ 'str' ] } }
>> >  
>> >  ##
>>