Re: [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Laszlo Ersek <lersek@redhat.com>]

On 04/18/18 10:47, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:

>> +##
>> +# @FirmwareArchitecture:
>> +#
>> +# Defines the target architectures (emulator binaries) that firmware may
>> +# execute on.
>> +#
>> +# @aarch64: The firmware can be executed by the qemu-system-aarch64 emulator.
>> +#
>> +# @arm: The firmware can be executed by the qemu-system-arm emulator.
>> +#
>> +# @i386: The firmware can be executed by the qemu-system-i386 emulator.
>> +#
>> +# @x86_64: The firmware can be executed by the qemu-system-x86_64 emulator.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum' : 'FirmwareArchitecture',
>> +  'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }
> 
> Partial dupe of CpuInfoArch from misc.json.  Neither covers all our
> target architectures.  Should we have one that does in common.json
> instead?

If we had one there, I'd use it here :)

For collecting the arch identifiers, is it OK to run "./configure
--help", and look for the "*-softmmu" options under
"--target-list=LIST"? Because that's what I need here; the qemu-system-*
emulators.

>> +##
>> +# @FirmwareFeature:
>> +#
>> +# Defines the features that firmware may support, and the platform requirements
>> +# that firmware may present.
>> +#
>> +# @acpi-s3: The firmware supports S3 sleep (suspend to RAM), as defined in the
>> +#           ACPI specification. On the "pc" machine type of the @i386 and
>> +#           @x86_64 emulation targets, S3 can be enabled with "-global
>> +#           PIIX4_PM.disable_s3=0" and disabled with "-global
>> +#           PIIX4_PM.disable_s3=1". On the "q35" machine type of the @i386 and
>> +#           @x86_64 emulation targets, S3 can be enabled with "-global
>> +#           ICH9-LPC.disable_s3=0" and disabled with "-global
>> +#           ICH9-LPC.disable_s3=1".
>> +#
>> +# @acpi-s4: The firmware supports S4 hibernation (suspend to disk), as defined
>> +#           in the ACPI specification. On the "pc" machine type of the @i386
>> +#           and @x86_64 emulation targets, S4 can be enabled with "-global
>> +#           PIIX4_PM.disable_s4=0" and disabled with "-global
>> +#           PIIX4_PM.disable_s4=1". On the "q35" machine type of the @i386 and
>> +#           @x86_64 emulation targets, S4 can be enabled with "-global
>> +#           ICH9-LPC.disable_s4=0" and disabled with "-global
>> +#           ICH9-LPC.disable_s4=1".
> 
> Would you mind breaking documentation lines a bit ealier?

Not at all; I aimed at 79 characters (like I do for the QEMU C source
code as well), but perhaps that's too wide for schema docs. What width
do you prefer?

While at it: is it possible to break the documentation for a single
@entry to multiple paragraphs? I tried it (simply by inserting an empty
line, without starting another @entry), and the generator didn't like it.

>> +##
>> +# @FirmwareFlashFile:
>> +#
>> +# Defines common properties that are necessary for loading a firmware file into
>> +# a pflash chip. The corresponding QEMU command line option is "-drive
>> +# file=@pathname,format=@format". Note however that the option-argument shown
>> +# here is incomplete; it is completed under @FirmwareMappingFlash.
>> +#
>> +# @pathname: Specifies the pathname on the host filesystem where the firmware
>> +#            file can be found.
> 
> We use @filename elsewhere in the QAPI schema.  Let's stick to that.
> More of the same below.

Will do.


>> +#
>> +# @format: Specifies the block format of the file pointed-to by @pathname, such
>> +#          as @raw or @qcow2.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct' : 'FirmwareFlashFile',
>> +  'data'   : { 'pathname' : 'str',
>> +               'format'   : 'BlockdevDriver' } }
>> +
>> +##
>> +# @FirmwareMappingFlash:
>> +#
>> +# Describes loading and mapping properties for the firmware executable and its
>> +# accompanying NVRAM file, when @FirmwareDevice is @flash.
>> +#
>> +# @executable: Identifies the firmware executable. The firmware executable may
>> +#              be shared by multiple virtual machine definitions. The
>> +#              corresponding QEMU command line option is "-drive
>> +#              if=pflash,unit=0,readonly=on,file=@executable.@pathname,format=@executable.@format".
> 
> I guess @executable.@pathname means member @pathname of @executable.  I
> read it as two distinct parameters first, then wondered where parameter
> @pathname is.  Perhaps @executable.pathname would be clearer.

I can try that, yes -- in the HTML documentation, will the monospace
style apply to the full field specification?

> 
>> +#
>> +# @nvram_template: Identifies the NVRAM template compatible with @executable.
>> +#                  Management software instantiates an individual copy -- a
>> +#                  specific NVRAM file -- from @nvram_template.@pathname for
>> +#                  each new virtual machine definition created.
>> +#                  @nvram_template.@pathname itself is never mapped into
>> +#                  virtual machines, only individual copies of it are. An NVRAM
>> +#                  file is typically used for persistently storing the
>> +#                  non-volatile UEFI variables of a virtual machine definition.
>> +#                  The corresponding QEMU command line option is "-drive
>> +#                  if=pflash,unit=1,readonly=off,file=PATHNAME_OF_PRIVATE_NVRAM_FILE,format=@nvram_template.@format".
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct' : 'FirmwareMappingFlash',
>> +  'data'   : { 'executable'     : 'FirmwareFlashFile',
>> +               'nvram_template' : 'FirmwareFlashFile' } }

Here's one thing I'll comment on myself: "nvram_template" should have
been spelled "nvram-template" :)

>> +
>> +##
>> +# @FirmwareMappingKernel:
>> +#
>> +# Describes loading and mapping properties for the firmware executable, when
>> +# @FirmwareDevice is @kernel.
>> +#
>> +# @pathname: Identifies the firmware executable. The firmware executable may be
>> +#            shared by multiple virtual machine definitions. The corresponding
>> +#            QEMU command line option is "-kernel @pathname".
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct' : 'FirmwareMappingKernel',
>> +  'data'   : { 'pathname' : 'str' } }
>> +
>> +##
>> +# @FirmwareMappingMemory:
>> +#
>> +# Describes loading and mapping properties for the firmware executable, when
>> +# @FirmwareDevice is @memory.
>> +#
>> +# @pathname: Identifies the firmware executable. The firmware executable may be
>> +#            shared by multiple virtual machine definitions. The corresponding
>> +#            QEMU command line option is "-bios @pathname".
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct' : 'FirmwareMappingMemory',
>> +  'data'   : { 'pathname' : 'str' } }
>> +
>> +##
>> +# @FirmwareMapping:
>> +#
>> +# Provides a discriminated structure for firmware to describe its loading /
>> +# mapping properties.
>> +#
>> +# @device: Selects the device type that the firmware must be mapped into.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'union'         : 'FirmwareMapping',
>> +  'base'          : { 'device' : 'FirmwareDevice' },
>> +  'discriminator' : 'device',
>> +  'data'          : { 'flash'  : 'FirmwareMappingFlash',
>> +                      'kernel' : 'FirmwareMappingKernel',
>> +                      'memory' : 'FirmwareMappingMemory' } }
> 
> The FirmwareMapping* all have a member @pathname.  It could be moved to
> the base.  Makes sense if we think future FirmwareMappingFOOs will also
> have it.  Your choice.

I could do that, but there would be consequences:

- FirmwareMappingKernel and FirmwareMappingMemory would become empty
  structures,

- in the documentation of @FirmwareMapping, I couldn't document @flash /
@kernel / @memory individually (I tried -- it doesn't work; the
generator wants to generate the documentation automatically).

The issue is that I would like to keep the QEMU cmdline options
"-kernel" and "-bios" *close* to @FirmwareMappingKernel and
@FirmwareMappingMemory. Now, if I make those structs empty, but keep the
-kernel / -bios cmdline options right under them, then I cannot refer to
@pathname (well, @filename) in those options -- because @filename will
no longer be defined under @FirmwareMappingKernel / @FirmwareMappingMemory.

And if I move the documentation of -kernel / -bios under
@FirmwareMapping, then it's awkward again, because then I have to dump
both -kernel and -bios into the documentation of @filename, and explain
which belongs to which union member / discriminator value.

So, if it's not a problem, I'd like to stick with this variant, for the
sake of documenting -kernel and -bios more clearly.

>> +##
>> +# @Firmware:
>> +#
>> +# Describes a firmware (or a firmware use case) to management software.
>> +#
>> +# @description: Provides a human-readable description of the firmware.
>> +#               Management software may or may not display @description.
>> +#
>> +# @type: Exposes the type of the firmware. @type is generally the primary key
>> +#        for which management software looks when picking a firmware for a new
>> +#        virtual machine definition.
>> +#
>> +# @mapping: Describes the loading / mapping properties of the firmware.
>> +#
>> +# @targets: Collects the target architectures (QEMU system emulators) and their
>> +#           machine types that may execute the firmware.
>> +#
>> +# @features: Lists the features that the firmware supports, and the platform
>> +#            requirements it presents.
>> +#
>> +# @tags: A list of auxiliary strings associated with the firmware for which
>> +#        @description is not approprite, due to the latter's possible exposure
> 
> s/approprite/appropriate/
> 
> Feed the new comments to a spell checker?

Right, I got "aspell" installed, and sometimes run it on my status
reports :) I was too tired to do it for the schema. Good catch, thanks.

> Introspection has a similar need for validating data against the schema,
> but it solves it with a test case without adding a QMP command.  Commit
> 39a18158165:
> 
>     A new test-qmp-input-visitor test case feeds its result for both
>     tests/qapi-schema/qapi-schema-test.json and qapi-schema.json to a
>     QmpInputVisitor to verify it actually conforms to the schema.
> 
> The test case has since moved to tests/test-qobject-input-visitor.c,
> function test_visitor_in_qmp_introspect().  Please check it out to see
> whether you can do without a QMP command, too.

Paolo suggested earlier that no C code should be added ultimately; the
schema should be moved under "docs/interop/". I was OK with that, just
wanted to keep the examples verifiable against the schema until the
patch got committed:

http://mid.mail-archive.com/9d6b3e23-ed02-0079-50d0-15de8b11810f@redhat.com

So in the non-RFC version, the QMP command shouldn't exist at all.

Thanks,
Laszlo