[PATCH v5 0/4] docs/interop/firmware.json: scripts/qapi-gen.py compatibility

[PATCH v5 0/4] docs/interop/firmware.json: scripts/qapi-gen.py compatibility

[Thomas Weißschuh]

docs/interop/firmware.json is currently not usable with qapi-gen.py due
to various non-functional issues.
Fix those issue to provide compatibility.

Sorry for the quick resend, but the series got broken by the just-merged
pull "pull-qapi-2024-07-17" which made the newly introduced patch 3
necessary.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Changes in v5:
- Add Reviewed-by from Daniel
- Add patch to drop Example section
- Link to v4: https://lore.kernel.org/r/20240718-qapi-firmware-json-v4-0-449ce672db5c@linutronix.de

Changes in v4:
- Update "since" to 9.1
- Add comment to 'member-name-exceptions'
- Document enum members and drop 'documentation-exceptions'
- Link to v3: https://lore.kernel.org/r/20240311-qapi-firmware-json-v3-0-ceea6e35eb4a@linutronix.de

Changes in v3:
- Drop already picked up patches
- Drop include of pragma.json
- Introduce new enums FirmwareFormat and FirmwareArchitecture
- Link to v2: https://lore.kernel.org/r/20240307-qapi-firmware-json-v2-0-3b29eabb9b9a@linutronix.de

Changes in v2:
- Add review tag from Philippe
- Add Fixes tag (Philippe)
- Add testcase (Philippe)
- Link to v1: https://lore.kernel.org/r/20240306-qapi-firmware-json-v1-0-619f7122a249@linutronix.de

---
Thomas Weißschuh (4):
      docs/interop/firmware.json: add new enum FirmwareFormat
      docs/interop/firmware.json: add new enum FirmwareArchitecture
      docs/interop/firmware.json: convert "Example" section
      docs: add test for firmware.json QAPI

 docs/interop/firmware.json | 48 +++++++++++++++++++++++++++++++++++++++++-----
 docs/meson.build           |  5 +++++
 2 files changed, 48 insertions(+), 5 deletions(-)
---
base-commit: d74ec4d7dda6322bcc51d1b13ccbd993d3574795
change-id: 20240306-qapi-firmware-json-6fb1213936dd

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

[PATCH v5 1/4] docs/interop/firmware.json: add new enum FirmwareFormat

[Thomas Weißschuh]

Only a small subset of all blockdev drivers make sense for firmware
images. Introduce and use a new enum to represent this.

This also reduces the dependency on firmware.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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 54a1fc6c1041..a26fe81bf2fe 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -15,7 +15,6 @@
 ##
 
 { 'include' : 'machine.json' }
-{ 'include' : 'block-core.json' }
 
 ##
 # @FirmwareOSInterface:
@@ -200,6 +199,20 @@
              'enrolled-keys', 'requires-smm', 'secure-boot',
              'verbose-dynamic', 'verbose-static' ] }
 
+##
+# @FirmwareFormat:
+#
+# Formats that are supported for firmware images.
+#
+# @raw: Raw disk image format.
+#
+# @qcow2: QEMU image format.
+#
+# Since: 9.1
+##
+{ 'enum': 'FirmwareFormat',
+  'data': [ 'raw', 'qcow2' ] }
+
 ##
 # @FirmwareFlashFile:
 #
@@ -219,7 +232,7 @@
 ##
 { 'struct' : 'FirmwareFlashFile',
   'data'   : { 'filename' : 'str',
-               'format'   : 'BlockdevDriver' } }
+               'format'   : 'FirmwareFormat' } }
 
 
 ##

-- 
2.45.2

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

[Thomas Weißschuh]

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.
+# The values are a subset of the enum SysEmuTarget.
+#
+# @aarch64: 64-bit Arm.
+#
+# @arm: 32-bit Arm.
+#
+# @i386: 32-bit x86.
+#
+# @loongarch64: 64-bit LoongArch.
+#
+# @x86_64: 64-bit x86.
+#
+# Since: 9.1
+##
+{ 'enum' : 'FirmwareArchitecture',
+  'data' : [ 'aarch64', 'arm', 'i386', 'loongarch64', 'x86_64' ] }
+
+
 ##
 # @FirmwareTarget:
 #
@@ -80,7 +105,7 @@
 # Since: 3.0
 ##
 { 'struct' : 'FirmwareTarget',
-  'data'   : { 'architecture' : 'SysEmuTarget',
+  'data'   : { 'architecture' : 'FirmwareArchitecture',
                'machines'     : [ 'str' ] } }
 
 ##

-- 
2.45.2

[PATCH v5 3/4] docs/interop/firmware.json: convert "Example" section

[Thomas Weißschuh]

Since commit 3c5f6114d9ff ("qapi: remove "Example" doc section")
the "Example" section is not valid anymore.
It has been replaced by the "qmp-example" role.

This was not detected earlier as firmware.json was not validated.
As this validation is about to be added, adapt firmware.json.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 docs/interop/firmware.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 2eb0be11d595..eea82eef3a07 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -471,7 +471,7 @@
 #
 # Since: 3.0
 #
-# Examples:
+# .. qmp-example::
 #
 #     {
 #         "description": "SeaBIOS",

-- 
2.45.2

[PATCH v5 4/4] docs: add test for firmware.json QAPI

[Thomas Weißschuh]

To make sure that the QAPI description stays valid add a testcase.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Link: https://lore.kernel.org/qemu-devel/d9ce0234-4beb-4b90-b14c-76810d3b81d7@linaro.org/
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 docs/meson.build | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/docs/meson.build b/docs/meson.build
index 9040f860ae1a..bcca45a342a3 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -99,3 +99,8 @@ if build_docs
   alias_target('html', sphinxdocs)
   alias_target('man', sphinxmans)
 endif
+
+test('QAPI firmware.json regression tests', python,
+     args: [qapi_gen.full_path(), '-o', meson.current_build_dir() / 'qapi',
+            meson.current_source_dir() / 'interop/firmware.json'],
+     env: test_env, suite: ['qapi-schema', 'qapi-interop'])

-- 
2.45.2

Re: [PATCH v5 3/4] docs/interop/firmware.json: convert "Example" section

[Daniel P. Berrangé]

On Thu, Jul 18, 2024 at 02:21:47PM +0200, Thomas Weißschuh wrote:
> Since commit 3c5f6114d9ff ("qapi: remove "Example" doc section")
> the "Example" section is not valid anymore.
> It has been replaced by the "qmp-example" role.
> 
> This was not detected earlier as firmware.json was not validated.
> As this validation is about to be added, adapt firmware.json.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  docs/interop/firmware.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Markus Armbruster]

Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:

> Only a small subset of all blockdev drivers make sense for firmware
> images. Introduce and use a new enum to represent this.
>
> This also reduces the dependency on firmware.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 | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 54a1fc6c1041..a26fe81bf2fe 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -15,7 +15,6 @@
>  ##
>  
>  { 'include' : 'machine.json' }
> -{ 'include' : 'block-core.json' }
>  
>  ##
>  # @FirmwareOSInterface:
> @@ -200,6 +199,20 @@
>               'enrolled-keys', 'requires-smm', 'secure-boot',
>               'verbose-dynamic', 'verbose-static' ] }
>  
> +##
> +# @FirmwareFormat:
> +#
> +# Formats that are supported for firmware images.
> +#
> +# @raw: Raw disk image format.
> +#
> +# @qcow2: QEMU image format.

It's not the only QEMU image format...  Maybe "The QCOW2 image format."
Almost tautological, but I don't have better ideas.

> +#
> +# Since: 9.1

The enum type is indeed since 9.1, but its two members are since 3.0,
and that's what matters.

> +##
> +{ 'enum': 'FirmwareFormat',
> +  'data': [ 'raw', 'qcow2' ] }
> +
>  ##
>  # @FirmwareFlashFile:
>  #
> @@ -219,7 +232,7 @@
>  ##
>  { 'struct' : 'FirmwareFlashFile',
>    'data'   : { 'filename' : 'str',
> -               'format'   : 'BlockdevDriver' } }
> +               'format'   : 'FirmwareFormat' } }
>  
>  
>  ##

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

[Markus Armbruster]

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.

Will consumers of firmware.json care for this?  Or is it just a reminder
for developers to keep the two enums in sync?

> +#
> +# @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' ] }
> +
> +

Drop one blank line, please.

>  ##
>  # @FirmwareTarget:
>  #
> @@ -80,7 +105,7 @@
>  # Since: 3.0
>  ##
>  { 'struct' : 'FirmwareTarget',
> -  'data'   : { 'architecture' : 'SysEmuTarget',
> +  'data'   : { 'architecture' : 'FirmwareArchitecture',
>                 'machines'     : [ 'str' ] } }
>  
>  ##

Re: [PATCH v5 3/4] docs/interop/firmware.json: convert "Example" section

[Markus Armbruster]

Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:

> Since commit 3c5f6114d9ff ("qapi: remove "Example" doc section")
> the "Example" section is not valid anymore.
> It has been replaced by the "qmp-example" role.

Make that "directive" instead of role.

> This was not detected earlier as firmware.json was not validated.

Sorry about that!

> As this validation is about to be added, adapt firmware.json.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  docs/interop/firmware.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 2eb0be11d595..eea82eef3a07 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -471,7 +471,7 @@
>  #
>  # Since: 3.0
>  #
> -# Examples:
> +# .. qmp-example::
>  #
>  #     {
>  #         "description": "SeaBIOS",

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Thomas Weißschuh]

On Thu, Jul 18, 2024 at 03:09:37PM GMT, Markus Armbruster wrote:
> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
> 
> > Only a small subset of all blockdev drivers make sense for firmware
> > images. Introduce and use a new enum to represent this.
> >
> > This also reduces the dependency on firmware.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 | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> > index 54a1fc6c1041..a26fe81bf2fe 100644
> > --- a/docs/interop/firmware.json
> > +++ b/docs/interop/firmware.json
> > @@ -15,7 +15,6 @@
> >  ##
> >  
> >  { 'include' : 'machine.json' }
> > -{ 'include' : 'block-core.json' }
> >  
> >  ##
> >  # @FirmwareOSInterface:
> > @@ -200,6 +199,20 @@
> >               'enrolled-keys', 'requires-smm', 'secure-boot',
> >               'verbose-dynamic', 'verbose-static' ] }
> >  
> > +##
> > +# @FirmwareFormat:
> > +#
> > +# Formats that are supported for firmware images.
> > +#
> > +# @raw: Raw disk image format.
> > +#
> > +# @qcow2: QEMU image format.
> 
> It's not the only QEMU image format...  Maybe "The QCOW2 image format."
> Almost tautological, but I don't have better ideas.

I used the wording from qemu-img(1).
If you prefer "The QCOW2 image format", I'll switch to that.

> > +#
> > +# Since: 9.1
> 
> The enum type is indeed since 9.1, but its two members are since 3.0,
> and that's what matters.

Ack.

So I change the Since: of the whole enum?
And not on the individual members?

> > +##
> > +{ 'enum': 'FirmwareFormat',
> > +  'data': [ 'raw', 'qcow2' ] }
> > +
> >  ##
> >  # @FirmwareFlashFile:
> >  #
> > @@ -219,7 +232,7 @@
> >  ##
> >  { 'struct' : 'FirmwareFlashFile',
> >    'data'   : { 'filename' : 'str',
> > -               'format'   : 'BlockdevDriver' } }
> > +               'format'   : 'FirmwareFormat' } }
> >  
> >  
> >  ##
> 

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

[Thomas Weißschuh]

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?

> 
> > +#
> > +# @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' ] }
> > +
> > +
> 
> Drop one blank line, please.

Ack.

> 
> >  ##
> >  # @FirmwareTarget:
> >  #
> > @@ -80,7 +105,7 @@
> >  # Since: 3.0
> >  ##
> >  { 'struct' : 'FirmwareTarget',
> > -  'data'   : { 'architecture' : 'SysEmuTarget',
> > +  'data'   : { 'architecture' : 'FirmwareArchitecture',
> >                 'machines'     : [ 'str' ] } }
> >  
> >  ##
> 

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

[Markus Armbruster]

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

> On Thu, Jul 18, 2024 at 03:09:37PM GMT, Markus Armbruster wrote:
>> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
>> 
>> > Only a small subset of all blockdev drivers make sense for firmware
>> > images. Introduce and use a new enum to represent this.
>> >
>> > This also reduces the dependency on firmware.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 | 17 +++++++++++++++--
>> >  1 file changed, 15 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
>> > index 54a1fc6c1041..a26fe81bf2fe 100644
>> > --- a/docs/interop/firmware.json
>> > +++ b/docs/interop/firmware.json
>> > @@ -15,7 +15,6 @@
>> >  ##
>> >  
>> >  { 'include' : 'machine.json' }
>> > -{ 'include' : 'block-core.json' }
>> >  
>> >  ##
>> >  # @FirmwareOSInterface:
>> > @@ -200,6 +199,20 @@
>> >               'enrolled-keys', 'requires-smm', 'secure-boot',
>> >               'verbose-dynamic', 'verbose-static' ] }
>> >  
>> > +##
>> > +# @FirmwareFormat:
>> > +#
>> > +# Formats that are supported for firmware images.
>> > +#
>> > +# @raw: Raw disk image format.
>> > +#
>> > +# @qcow2: QEMU image format.
>> 
>> It's not the only QEMU image format...  Maybe "The QCOW2 image format."
>> Almost tautological, but I don't have better ideas.
>
> I used the wording from qemu-img(1).

Interesting, wasn't aware.

> If you prefer "The QCOW2 image format", I'll switch to that.

Up to you, unless Kevin or Hanna have a preference.

>> > +#
>> > +# Since: 9.1
>> 
>> The enum type is indeed since 9.1, but its two members are since 3.0,
>> and that's what matters.
>
> Ack.
>
> So I change the Since: of the whole enum?
> And not on the individual members?

Whole enum is simpler, so that's what I'd do.  Perhaps with a brief
explanation in the commit message.  Here's my try:

    Claim "Since: 3.0" for the new enum, because that's correct for its
    members, and the members are what matters in the interface.

>> > +##
>> > +{ 'enum': 'FirmwareFormat',
>> > +  'data': [ 'raw', 'qcow2' ] }
>> > +
>> >  ##
>> >  # @FirmwareFlashFile:
>> >  #
>> > @@ -219,7 +232,7 @@
>> >  ##
>> >  { 'struct' : 'FirmwareFlashFile',
>> >    'data'   : { 'filename' : 'str',
>> > -               'format'   : 'BlockdevDriver' } }
>> > +               'format'   : 'FirmwareFormat' } }
>> >  
>> >  
>> >  ##
>> 

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

[Markus Armbruster]

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' ] } }
>> >  
>> >  ##
>> 

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

[Kashyap Chamarthy]

On Thu, Jul 18, 2024 at 03:09:37PM +0200, Markus Armbruster wrote:
> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:

[...]

> > +#
> > +# @raw: Raw disk image format.
> > +#
> > +# @qcow2: QEMU image format.
> 
> It's not the only QEMU image format...  Maybe "The QCOW2 image format."
> Almost tautological, but I don't have better ideas.

For slightly less tautology, not sure these are any better:

    @raw: Disk image format that's plain and unstructured.

    @qcow2: Disk image format with support for copy-on-write and more.

[...]

-- 
/kashyap