On 04/18/18 10:47, Markus Armbruster wrote:
> Laszlo Ersek <lersek@xxxxxxxxxx> 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@xxxxxxxxxx
So in the non-RFC version, the QMP command shouldn't exist at all.
Thanks,
Laszlo
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
