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

[PATCH 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.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Thomas Weißschuh (4):
      docs/interop/firmware.json: Align examples
      docs/interop/firmware.json: Fix doc for FirmwareFlashMode
      docs/interop/firmware.json: Use full include paths
      docs/interop/firmware.json: Include pragma.json

 docs/interop/firmware.json | 392 +++++++++++++++++++++++----------------------
 1 file changed, 197 insertions(+), 195 deletions(-)
---
base-commit: db596ae19040574e41d086e78469014191d7d7fc
change-id: 20240306-qapi-firmware-json-6fb1213936dd

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

[PATCH 1/4] docs/interop/firmware.json: Align examples

[Thomas Weißschuh]

The QAPI generator now validates the alignment and rejects this file.

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

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index cc8f86918681..a024f1b9bf3f 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -435,203 +435,203 @@
 #
 # Examples:
 #
-# {
-#     "description": "SeaBIOS",
-#     "interface-types": [
-#         "bios"
-#     ],
-#     "mapping": {
-#         "device": "memory",
-#         "filename": "/usr/share/seabios/bios-256k.bin"
-#     },
-#     "targets": [
-#         {
-#             "architecture": "i386",
-#             "machines": [
-#                 "pc-i440fx-*",
-#                 "pc-q35-*"
-#             ]
+#     {
+#         "description": "SeaBIOS",
+#         "interface-types": [
+#             "bios"
+#         ],
+#         "mapping": {
+#             "device": "memory",
+#             "filename": "/usr/share/seabios/bios-256k.bin"
 #         },
-#         {
-#             "architecture": "x86_64",
-#             "machines": [
-#                 "pc-i440fx-*",
-#                 "pc-q35-*"
-#             ]
-#         }
-#     ],
-#     "features": [
-#         "acpi-s3",
-#         "acpi-s4"
-#     ],
-#     "tags": [
-#         "CONFIG_BOOTSPLASH=n",
-#         "CONFIG_ROM_SIZE=256",
-#         "CONFIG_USE_SMM=n"
-#     ]
-# }
-#
-# {
-#     "description": "OVMF with SB+SMM, empty varstore",
-#     "interface-types": [
-#         "uefi"
-#     ],
-#     "mapping": {
-#         "device": "flash",
-#         "executable": {
-#             "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
-#             "format": "raw"
+#         "targets": [
+#             {
+#                 "architecture": "i386",
+#                 "machines": [
+#                     "pc-i440fx-*",
+#                     "pc-q35-*"
+#                 ]
+#             },
+#             {
+#                 "architecture": "x86_64",
+#                 "machines": [
+#                     "pc-i440fx-*",
+#                     "pc-q35-*"
+#                 ]
+#             }
+#         ],
+#         "features": [
+#             "acpi-s3",
+#             "acpi-s4"
+#         ],
+#         "tags": [
+#             "CONFIG_BOOTSPLASH=n",
+#             "CONFIG_ROM_SIZE=256",
+#             "CONFIG_USE_SMM=n"
+#         ]
+#     }
+#
+#     {
+#         "description": "OVMF with SB+SMM, empty varstore",
+#         "interface-types": [
+#             "uefi"
+#         ],
+#         "mapping": {
+#             "device": "flash",
+#             "executable": {
+#                 "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
+#                 "format": "raw"
+#             },
+#             "nvram-template": {
+#                 "filename": "/usr/share/OVMF/OVMF_VARS.fd",
+#                 "format": "raw"
+#             }
 #         },
-#         "nvram-template": {
-#             "filename": "/usr/share/OVMF/OVMF_VARS.fd",
-#             "format": "raw"
-#         }
-#     },
-#     "targets": [
-#         {
-#             "architecture": "x86_64",
-#             "machines": [
-#                 "pc-q35-*"
-#             ]
-#         }
-#     ],
-#     "features": [
-#         "acpi-s3",
-#         "amd-sev",
-#         "requires-smm",
-#         "secure-boot",
-#         "verbose-dynamic"
-#     ],
-#     "tags": [
-#         "-a IA32",
-#         "-a X64",
-#         "-p OvmfPkg/OvmfPkgIa32X64.dsc",
-#         "-t GCC48",
-#         "-b DEBUG",
-#         "-D SMM_REQUIRE",
-#         "-D SECURE_BOOT_ENABLE",
-#         "-D FD_SIZE_4MB"
-#     ]
-# }
-#
-# {
-#     "description": "OVMF with SB+SMM, SB enabled, MS certs enrolled",
-#     "interface-types": [
-#         "uefi"
-#     ],
-#     "mapping": {
-#         "device": "flash",
-#         "executable": {
-#             "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
-#             "format": "raw"
+#         "targets": [
+#             {
+#                 "architecture": "x86_64",
+#                 "machines": [
+#                     "pc-q35-*"
+#                 ]
+#             }
+#         ],
+#         "features": [
+#             "acpi-s3",
+#             "amd-sev",
+#             "requires-smm",
+#             "secure-boot",
+#             "verbose-dynamic"
+#         ],
+#         "tags": [
+#             "-a IA32",
+#             "-a X64",
+#             "-p OvmfPkg/OvmfPkgIa32X64.dsc",
+#             "-t GCC48",
+#             "-b DEBUG",
+#             "-D SMM_REQUIRE",
+#             "-D SECURE_BOOT_ENABLE",
+#             "-D FD_SIZE_4MB"
+#         ]
+#     }
+#
+#     {
+#         "description": "OVMF with SB+SMM, SB enabled, MS certs enrolled",
+#         "interface-types": [
+#             "uefi"
+#         ],
+#         "mapping": {
+#             "device": "flash",
+#             "executable": {
+#                 "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
+#                 "format": "raw"
+#             },
+#             "nvram-template": {
+#                 "filename": "/usr/share/OVMF/OVMF_VARS.secboot.fd",
+#                 "format": "raw"
+#             }
 #         },
-#         "nvram-template": {
-#             "filename": "/usr/share/OVMF/OVMF_VARS.secboot.fd",
-#             "format": "raw"
-#         }
-#     },
-#     "targets": [
-#         {
-#             "architecture": "x86_64",
-#             "machines": [
-#                 "pc-q35-*"
-#             ]
-#         }
-#     ],
-#     "features": [
-#         "acpi-s3",
-#         "amd-sev",
-#         "enrolled-keys",
-#         "requires-smm",
-#         "secure-boot",
-#         "verbose-dynamic"
-#     ],
-#     "tags": [
-#         "-a IA32",
-#         "-a X64",
-#         "-p OvmfPkg/OvmfPkgIa32X64.dsc",
-#         "-t GCC48",
-#         "-b DEBUG",
-#         "-D SMM_REQUIRE",
-#         "-D SECURE_BOOT_ENABLE",
-#         "-D FD_SIZE_4MB"
-#     ]
-# }
-#
-# {
-#     "description": "OVMF with SEV-ES support",
-#     "interface-types": [
-#         "uefi"
-#     ],
-#     "mapping": {
-#         "device": "flash",
-#         "executable": {
-#             "filename": "/usr/share/OVMF/OVMF_CODE.fd",
-#             "format": "raw"
+#         "targets": [
+#             {
+#                 "architecture": "x86_64",
+#                 "machines": [
+#                     "pc-q35-*"
+#                 ]
+#             }
+#         ],
+#         "features": [
+#             "acpi-s3",
+#             "amd-sev",
+#             "enrolled-keys",
+#             "requires-smm",
+#             "secure-boot",
+#             "verbose-dynamic"
+#         ],
+#         "tags": [
+#             "-a IA32",
+#             "-a X64",
+#             "-p OvmfPkg/OvmfPkgIa32X64.dsc",
+#             "-t GCC48",
+#             "-b DEBUG",
+#             "-D SMM_REQUIRE",
+#             "-D SECURE_BOOT_ENABLE",
+#             "-D FD_SIZE_4MB"
+#         ]
+#     }
+#
+#     {
+#         "description": "OVMF with SEV-ES support",
+#         "interface-types": [
+#             "uefi"
+#         ],
+#         "mapping": {
+#             "device": "flash",
+#             "executable": {
+#                 "filename": "/usr/share/OVMF/OVMF_CODE.fd",
+#                 "format": "raw"
+#             },
+#             "nvram-template": {
+#                 "filename": "/usr/share/OVMF/OVMF_VARS.fd",
+#                 "format": "raw"
+#             }
 #         },
-#         "nvram-template": {
-#             "filename": "/usr/share/OVMF/OVMF_VARS.fd",
-#             "format": "raw"
-#         }
-#     },
-#     "targets": [
-#         {
-#             "architecture": "x86_64",
-#             "machines": [
-#                 "pc-q35-*"
-#             ]
-#         }
-#     ],
-#     "features": [
-#         "acpi-s3",
-#         "amd-sev",
-#         "amd-sev-es",
-#         "verbose-dynamic"
-#     ],
-#     "tags": [
-#         "-a X64",
-#         "-p OvmfPkg/OvmfPkgX64.dsc",
-#         "-t GCC48",
-#         "-b DEBUG",
-#         "-D FD_SIZE_4MB"
-#     ]
-# }
-#
-# {
-#     "description": "UEFI firmware for ARM64 virtual machines",
-#     "interface-types": [
-#         "uefi"
-#     ],
-#     "mapping": {
-#         "device": "flash",
-#         "executable": {
-#             "filename": "/usr/share/AAVMF/AAVMF_CODE.fd",
-#             "format": "raw"
+#         "targets": [
+#             {
+#                 "architecture": "x86_64",
+#                 "machines": [
+#                     "pc-q35-*"
+#                 ]
+#             }
+#         ],
+#         "features": [
+#             "acpi-s3",
+#             "amd-sev",
+#             "amd-sev-es",
+#             "verbose-dynamic"
+#         ],
+#         "tags": [
+#             "-a X64",
+#             "-p OvmfPkg/OvmfPkgX64.dsc",
+#             "-t GCC48",
+#             "-b DEBUG",
+#             "-D FD_SIZE_4MB"
+#         ]
+#     }
+#
+#     {
+#         "description": "UEFI firmware for ARM64 virtual machines",
+#         "interface-types": [
+#             "uefi"
+#         ],
+#         "mapping": {
+#             "device": "flash",
+#             "executable": {
+#                 "filename": "/usr/share/AAVMF/AAVMF_CODE.fd",
+#                 "format": "raw"
+#             },
+#             "nvram-template": {
+#                 "filename": "/usr/share/AAVMF/AAVMF_VARS.fd",
+#                 "format": "raw"
+#             }
 #         },
-#         "nvram-template": {
-#             "filename": "/usr/share/AAVMF/AAVMF_VARS.fd",
-#             "format": "raw"
-#         }
-#     },
-#     "targets": [
-#         {
-#             "architecture": "aarch64",
-#             "machines": [
-#                 "virt-*"
-#             ]
-#         }
-#     ],
-#     "features": [
-#
-#     ],
-#     "tags": [
-#         "-a AARCH64",
-#         "-p ArmVirtPkg/ArmVirtQemu.dsc",
-#         "-t GCC48",
-#         "-b DEBUG",
-#         "-D DEBUG_PRINT_ERROR_LEVEL=0x80000000"
-#     ]
-# }
+#         "targets": [
+#             {
+#                 "architecture": "aarch64",
+#                 "machines": [
+#                     "virt-*"
+#                 ]
+#             }
+#         ],
+#         "features": [
+#
+#         ],
+#         "tags": [
+#             "-a AARCH64",
+#             "-p ArmVirtPkg/ArmVirtQemu.dsc",
+#             "-t GCC48",
+#             "-b DEBUG",
+#             "-D DEBUG_PRINT_ERROR_LEVEL=0x80000000"
+#         ]
+#     }
 ##
 { 'struct' : 'Firmware',
   'data'   : { 'description'     : 'str',

-- 
2.44.0

[PATCH 2/4] docs/interop/firmware.json: Fix doc for FirmwareFlashMode

[Thomas Weißschuh]

The doc title did not match the actual definition.

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 a024f1b9bf3f..54a1fc6c1041 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -223,7 +223,7 @@
 
 
 ##
-# @FirmwareFlashType:
+# @FirmwareFlashMode:
 #
 # Describes how the firmware build handles code versus variable
 # persistence.

-- 
2.44.0

[PATCH 3/4] docs/interop/firmware.json: Use full include paths

[Thomas Weißschuh]

The included files are part of the toplevel QAPI directory and need to
be included from there.

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

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 54a1fc6c1041..4ac840e2b413 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -14,8 +14,8 @@
 # = Firmware
 ##
 
-{ 'include' : 'machine.json' }
-{ 'include' : 'block-core.json' }
+{ 'include' : '../../qapi/machine.json' }
+{ 'include' : '../../qapi/block-core.json' }
 
 ##
 # @FirmwareOSInterface:

-- 
2.44.0

[PATCH 4/4] docs/interop/firmware.json: Include pragma.json

[Thomas Weißschuh]

The files included by firmware.json use names that do not satisfy the
generators requirements.
By including pragma.json these errors are suppressed.

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

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 4ac840e2b413..a7e9a22a0046 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -10,6 +10,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later. See the COPYING file in the top-level directory.
 
+{ 'include' : '../../qapi/pragma.json' }
+
 ##
 # = Firmware
 ##

-- 
2.44.0

Re: [PATCH 2/4] docs/interop/firmware.json: Fix doc for FirmwareFlashMode

[Philippe Mathieu-Daudé]

On 6/3/24 11:31, Thomas Weißschuh wrote:
> The doc title did not match the actual definition.
> 

Fixes: 2720ceda05 ("docs: expand firmware descriptor to allow flash 
without NVRAM")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 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 a024f1b9bf3f..54a1fc6c1041 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -223,7 +223,7 @@
>   
>   
>   ##
> -# @FirmwareFlashType:
> +# @FirmwareFlashMode:
>   #
>   # Describes how the firmware build handles code versus variable
>   # persistence.
> 

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

[Philippe Mathieu-Daudé]

On 6/3/24 11:31, Thomas Weißschuh wrote:
> docs/interop/firmware.json is currently not usable with qapi-gen.py due
> to various non-functional issues.
> Fix those issue to provide compatibility.

Could we add some lines in docs/meson.build to cover this files
during our CI tests?

> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> Thomas Weißschuh (4):
>        docs/interop/firmware.json: Align examples
>        docs/interop/firmware.json: Fix doc for FirmwareFlashMode
>        docs/interop/firmware.json: Use full include paths
>        docs/interop/firmware.json: Include pragma.json
> 
>   docs/interop/firmware.json | 392 +++++++++++++++++++++++----------------------
>   1 file changed, 197 insertions(+), 195 deletions(-)
> ---
> base-commit: db596ae19040574e41d086e78469014191d7d7fc
> change-id: 20240306-qapi-firmware-json-6fb1213936dd
> 
> Best regards,

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

[Thomas Weißschuh]

On Wed, Mar 06, 2024 at 05:15:34PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/3/24 11:31, Thomas Weißschuh wrote:
> > docs/interop/firmware.json is currently not usable with qapi-gen.py due
> > to various non-functional issues.
> > Fix those issue to provide compatibility.
> 
> Could we add some lines in docs/meson.build to cover this files
> during our CI tests?

Sounds good.

I am also developing a new tool which would use the generated QAPI
files which I plan to submit soonish.

> > 
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> > Thomas Weißschuh (4):
> >        docs/interop/firmware.json: Align examples
> >        docs/interop/firmware.json: Fix doc for FirmwareFlashMode
> >        docs/interop/firmware.json: Use full include paths
> >        docs/interop/firmware.json: Include pragma.json
> > 
> >   docs/interop/firmware.json | 392 +++++++++++++++++++++++----------------------
> >   1 file changed, 197 insertions(+), 195 deletions(-)
> > ---
> > base-commit: db596ae19040574e41d086e78469014191d7d7fc
> > change-id: 20240306-qapi-firmware-json-6fb1213936dd
> > 
> > Best regards,

Re: [PATCH 1/4] docs/interop/firmware.json: Align examples

[Markus Armbruster]

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

> The QAPI generator now validates the alignment and rejects this file.

Perhaps

  Adjust indentation for commit d23055b8db8 (qapi: Require descriptions
  and tagged sections to be indented).

>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

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

Re: [PATCH 3/4] docs/interop/firmware.json: Use full include paths

[Markus Armbruster]

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

> The included files are part of the toplevel QAPI directory and need to
> be included from there.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  docs/interop/firmware.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 54a1fc6c1041..4ac840e2b413 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -14,8 +14,8 @@
>  # = Firmware
>  ##
>  
> -{ 'include' : 'machine.json' }
> -{ 'include' : 'block-core.json' }
> +{ 'include' : '../../qapi/machine.json' }
> +{ 'include' : '../../qapi/block-core.json' }
>  
>  ##
>  # @FirmwareOSInterface:

The coupling with the main QAPI schema is unfortunate.

The purpose of docs/interop/firmware.json is to serve as schema for
firmware descriptions: a firmware description is a JSON object that
conforms to this schema's struct Firmware.

Such a description should be installed along with each firmware binary.

QAPI tooling can be used to check conformance: parse the firmware
description JSON object, feed it to the generated visit_type_Firmware().
Success implies conformance.

If you find more uses for the C struct Firmware created by
visit_type_Firmware(), more power to you.

firmware.json needs machine.json for SysEmuTarget, and block-core.json
for BlockdevDriver.  The largest and the third-largest QAPI module just
for two enums.  Almost a quarter Mebibyte of code.

qapi-gen.py generates more than 12kSLOC.  Without the include (and with
the enums dumbed down to 'str' to enable that), it generates less than
800.

We could use Sphinx to generate a manual from firmware.json's document.
Except that manual would be useless, because of its 11,000 lines of
HTML, less than 800 are for firmware.json.

Options:

* Live with the mess.

* Refactor QAPI modules so firmware.json can include just the enums.

  Drawback: we spread the mess into qapi/.  Ugh.

* Copy the enums to firmware.json.

  Drawback: we risk them going stale.

* Dumb down to 'str'.

  Drawback: the conformance check no longer enforces the value of
  FirmwareTarget member @architecture and FirmwareFlashFile member
  @format is valid.

Thoughts?

Re: [PATCH 3/4] docs/interop/firmware.json: Use full include paths

[Thomas Weißschuh]

On Fri, Mar 08, 2024 at 04:19:42PM +0100, Markus Armbruster wrote:
> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
> 
> > The included files are part of the toplevel QAPI directory and need to
> > be included from there.
> >
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> >  docs/interop/firmware.json | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> > index 54a1fc6c1041..4ac840e2b413 100644
> > --- a/docs/interop/firmware.json
> > +++ b/docs/interop/firmware.json
> > @@ -14,8 +14,8 @@
> >  # = Firmware
> >  ##
> >  
> > -{ 'include' : 'machine.json' }
> > -{ 'include' : 'block-core.json' }
> > +{ 'include' : '../../qapi/machine.json' }
> > +{ 'include' : '../../qapi/block-core.json' }
> >  
> >  ##
> >  # @FirmwareOSInterface:
> 
> The coupling with the main QAPI schema is unfortunate.
> 
> The purpose of docs/interop/firmware.json is to serve as schema for
> firmware descriptions: a firmware description is a JSON object that
> conforms to this schema's struct Firmware.
> 
> Such a description should be installed along with each firmware binary.
> 
> QAPI tooling can be used to check conformance: parse the firmware
> description JSON object, feed it to the generated visit_type_Firmware().
> Success implies conformance.
> 
> If you find more uses for the C struct Firmware created by
> visit_type_Firmware(), more power to you.

I am writing a tool "qemu-firmware" that can be used to query and
introspect those installed JSON description files.
This is where my changes are coming from.

That tool is meant to be pushed to qemu upstream but it's not ready yet.

> firmware.json needs machine.json for SysEmuTarget, and block-core.json
> for BlockdevDriver.  The largest and the third-largest QAPI module just
> for two enums.  Almost a quarter Mebibyte of code.
> 
> qapi-gen.py generates more than 12kSLOC.  Without the include (and with
> the enums dumbed down to 'str' to enable that), it generates less than
> 800.

The generated code also doesn't link properly because it has
dependencies on various parts of qemu internals.

In my "qemu-firmware" branch I have two more commits that split
SysEmuTarget and BlockdevDriver into their own JSON files.
Then everything works nicely.
These commits were not submitted yet as they felt specific to my
usecase.

> We could use Sphinx to generate a manual from firmware.json's document.
> Except that manual would be useless, because of its 11,000 lines of
> HTML, less than 800 are for firmware.json.
> 
> Options:
> 
> * Live with the mess.
> 
> * Refactor QAPI modules so firmware.json can include just the enums.
> 
>   Drawback: we spread the mess into qapi/.  Ugh.

As mentioned above, this is what I ended up doing and which I prefer for
my usecase.

> * Copy the enums to firmware.json.
> 
>   Drawback: we risk them going stale.
> 
> * Dumb down to 'str'.
> 
>   Drawback: the conformance check no longer enforces the value of
>   FirmwareTarget member @architecture and FirmwareFlashFile member
>   @format is valid.
> 
> Thoughts?

Re: [PATCH 3/4] docs/interop/firmware.json: Use full include paths

[Daniel P. Berrangé]

On Fri, Mar 08, 2024 at 04:19:42PM +0100, Markus Armbruster wrote:
> The coupling with the main QAPI schema is unfortunate.
> 
> The purpose of docs/interop/firmware.json is to serve as schema for
> firmware descriptions: a firmware description is a JSON object that
> conforms to this schema's struct Firmware.
> 
> Such a description should be installed along with each firmware binary.
> 
> QAPI tooling can be used to check conformance: parse the firmware
> description JSON object, feed it to the generated visit_type_Firmware().
> Success implies conformance.
> 
> If you find more uses for the C struct Firmware created by
> visit_type_Firmware(), more power to you.
> 
> firmware.json needs machine.json for SysEmuTarget, and block-core.json
> for BlockdevDriver.  The largest and the third-largest QAPI module just
> for two enums.  Almost a quarter Mebibyte of code.

firmware.json can use BlockdevDriver, but we could question
whether it /should/ use BlockdevDriver. Is there really a
compelling reason to support every possible block driver for
readonly firmware and tiny nvram file ? I thin kit would be
totally reasonable to define a "FirmwareFormat" enum which
only permitted 'raw' and 'qcow2'.   If someone wants to
justify why we need another format, I'm all ears...

For SysEmuTarget its a little more useful, as in theory the
firmware could be extended to any QEMU target. In practice
thus far we've only used it todescribe EFI based firmware,
which is relevant for a subset of targets. It doesn't seem
to be a huge downside to define a FirmwareTarget enum with
only the arches we've actually got a use for so far. When
someone comes along with a need for non-EFI we can extend
it, and we'll need to extend libvirt at the same time anyway

> qapi-gen.py generates more than 12kSLOC.  Without the include (and with
> the enums dumbed down to 'str' to enable that), it generates less than
> 800.
> 
> We could use Sphinx to generate a manual from firmware.json's document.
> Except that manual would be useless, because of its 11,000 lines of
> HTML, less than 800 are for firmware.json.
> 
> Options:
> 
> * Live with the mess.
> 
> * Refactor QAPI modules so firmware.json can include just the enums.
> 
>   Drawback: we spread the mess into qapi/.  Ugh.
> 
> * Copy the enums to firmware.json.
> 
>   Drawback: we risk them going stale.

IMHO copy the enum. While the risk exists, I don't think it is a
risk worth worrying about in reality. If someone points out a gap
that's important is a matter of minutes to patch it.

> * Dumb down to 'str'.
> 
>   Drawback: the conformance check no longer enforces the value of
>   FirmwareTarget member @architecture and FirmwareFlashFile member
>   @format is valid.




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 3/4] docs/interop/firmware.json: Use full include paths

[Philippe Mathieu-Daudé]

On 8/3/24 17:09, Daniel P. Berrangé wrote:
> On Fri, Mar 08, 2024 at 04:19:42PM +0100, Markus Armbruster wrote:
>> The coupling with the main QAPI schema is unfortunate.
>>
>> The purpose of docs/interop/firmware.json is to serve as schema for
>> firmware descriptions: a firmware description is a JSON object that
>> conforms to this schema's struct Firmware.
>>
>> Such a description should be installed along with each firmware binary.
>>
>> QAPI tooling can be used to check conformance: parse the firmware
>> description JSON object, feed it to the generated visit_type_Firmware().
>> Success implies conformance.
>>
>> If you find more uses for the C struct Firmware created by
>> visit_type_Firmware(), more power to you.
>>
>> firmware.json needs machine.json for SysEmuTarget, and block-core.json
>> for BlockdevDriver.  The largest and the third-largest QAPI module just
>> for two enums.  Almost a quarter Mebibyte of code.
> 
> firmware.json can use BlockdevDriver, but we could question
> whether it /should/ use BlockdevDriver. Is there really a
> compelling reason to support every possible block driver for
> readonly firmware and tiny nvram file ? I thin kit would be
> totally reasonable to define a "FirmwareFormat" enum which
> only permitted 'raw' and 'qcow2'.   If someone wants to
> justify why we need another format, I'm all ears...
> 
> For SysEmuTarget its a little more useful, as in theory the
> firmware could be extended to any QEMU target. In practice
> thus far we've only used it todescribe EFI based firmware,
> which is relevant for a subset of targets. It doesn't seem
> to be a huge downside to define a FirmwareTarget enum with
> only the arches we've actually got a use for so far. When
> someone comes along with a need for non-EFI we can extend
> it, and we'll need to extend libvirt at the same time anyway
> 
>> qapi-gen.py generates more than 12kSLOC.  Without the include (and with
>> the enums dumbed down to 'str' to enable that), it generates less than
>> 800.
>>
>> We could use Sphinx to generate a manual from firmware.json's document.
>> Except that manual would be useless, because of its 11,000 lines of
>> HTML, less than 800 are for firmware.json.

=)

>>
>> Options:
>>
>> * Live with the mess.
>>
>> * Refactor QAPI modules so firmware.json can include just the enums.
>>
>>    Drawback: we spread the mess into qapi/.  Ugh.
>>
>> * Copy the enums to firmware.json.
>>
>>    Drawback: we risk them going stale.
> 
> IMHO copy the enum. While the risk exists, I don't think it is a
> risk worth worrying about in reality. If someone points out a gap
> that's important is a matter of minutes to patch it.

Since I concur with Daniel PoV, I'm queuing patches 1 (with
Markus reword) and 2 so far, so less to carry for v3.

Regards,

Phil.