On 04/18/18 08:02, Gerd Hoffmann wrote:

On Wed, Apr 18, 2018 at 12:40:54AM +0200, Laszlo Ersek wrote:

...

Add a schema that describes the different uses and properties of virtual machine firmware.

Looks good to me overall.

...

+{ 'enum' : 'FirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

openbios missing.

...

+{ 'enum' : 'FirmwareArchitecture', + 'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }

ppc(64) missing (but you have slof above ;) ... s390 too.

I figured those would be contributed by people that actually use them, as separate patches :) In fact I would rather prefer removing "slof" and "uboot" from this initial version, because I have zero clue about them. Of course, if reviewers can tell me what *exact* enum constants I should add, I'll comply.

...

+# @machines: Lists the machine types (known by the emulator that is specified +# through @architecture) that can execute the firmware. Elements of +# @machines are not supposed to be versioned; if a machine type is +# versioned in QEMU (e.g. "pc-i440fx-2.12"), then its unversioned +# variant, which typically refers to the latest version (e.g. "pc"), +# should be listed in @machines. On the QEMU command line, "-machine +# type=..." specifies the requested machine type.

Hmm, I'd tend to ignore the aliases here (pc, q35, virt) and use wildcards instead (pc-i440fx-*, pc-q35-*, virt-*).

I think that will be easier for libvirt to work with because it always resolves aliases to actual machine types when storing them in the domain xml.

This surfaced in the RFCv1 discussion, but Daniel suggested ignoring version numbers: http://mid.mail-archive.com/20180410093412.GI5155@redhat.com On 04/10/18 11:34, Daniel P. Berrangé wrote:

IMHO it would be valid to just keep life simple and only record the base machine type name that can use the firmware ie "pc", "q35", and ignore the fact that in some cases the firmware might require a specific version of the machine type.

Continuing: On 04/18/18 08:02, Gerd Hoffmann wrote:

...

+# @secure-boot: The firmware implements the software interfaces for UEFI Secure +# Boot, as defined in the UEFI specification. Note that without +# @requires-smm, guest code running with kernel privileges can +# undermine the security of Secure Boot. +# +# @secure-boot-enrolled-keys: The variable store (NVRAM) template associated

I think "enrolled-keys" should better be a separate feature.

It's not possible from the edk2 side; without -D SECURE_BOOT_ENABLE, the SB-related variables (SetupMode, PK, KEK, ...) don't work at all. For example, if you try to run "EnrollDefaultKeys.efi" on an OVMF that was built without SB, you get:

FS1:\> EnrollDefaultKeys.efi error: GetVariable("SetupMode", 8BE4DF61-93CA-11D2-AA0D-00E098032B8C): Not Found

In other words, I don't see any use in a @Firmware element where @nvram_template had the SB operational mode enabled and certificates enrolled (hence listing the proposed @enrolled-keys in @features), while @executable lacked the SB feature itself (hence not listing @secure-boot in @features). In the first place, such a varstore file (to be saved as @nvram_template) could never be *established* from within the guest, using the @executable in question -- see the error message above. Thanks! Laszlo

On 04/18/18 11:04, Gerd Hoffmann wrote:

...

This surfaced in the RFCv1 discussion, but Daniel suggested ignoring version numbers:

http://mid.mail-archive.com/20180410093412.GI5155@redhat.com

On 04/10/18 11:34, Daniel P. Berrangé wrote:

...

IMHO it would be valid to just keep life simple and only record the base machine type name that can use the firmware ie "pc", "q35", and ignore the fact that in some cases the firmware might require a specific version of the machine type.

IIRC this bit referes to the fact that SMM requires qemu >= 2.x (don't remember which x) to work. So smm-enabled edk2 would just say "pc-q35-*" instead of trying to specifying a version range somehow.

OK. I'm fine either way; Dan, can you please confirm you are OK with the suggested wildcard format? (I.e., we still shouldn't include actual version numbers in the supported machtypes list, but we should be more specific than just "pc" and "q35" -- if the machine type is versioned, use an asterisk for covering the version number.)

...

Continuing:

On 04/18/18 08:02, Gerd Hoffmann wrote:

...

...

+# @secure-boot: The firmware implements the software interfaces for UEFI Secure +# Boot, as defined in the UEFI specification. Note that without +# @requires-smm, guest code running with kernel privileges can +# undermine the security of Secure Boot. +# +# @secure-boot-enrolled-keys: The variable store (NVRAM) template associated

I think "enrolled-keys" should better be a separate feature.

It's not possible from the edk2 side; without -D SECURE_BOOT_ENABLE, the SB-related variables (SetupMode, PK, KEK, ...) don't work at all.

Sure. The firmware builds will advertise both "secure-boot" and "enrolled-keys" features to specify that.

But I think it should be enough to check for "secure-boot" feature to figure whenever a given firmware build supports secure boot, not both "secure-boot" and "secure-boot-plus-something-else".

Hmmm, I'm not sure I agree. One use case is that you want a domain config in which well-known OS-es, signed by the MS UEFI certs, just boot with SB enabled. (Some of our internal folks really want this.) Another use case is that you want a domain in which SB *can* be enabled, but your installer is signed with a different certificate chain (or it is unsigned), and with *just* the MS certs enrolled, it wouldn't boot at all. So you want the SB *feature*, but definitely not the initial enrollment / SB *operational mode*. For me to understand you better, are you suggesting merely that I: - rename @secure-boot-enrolled-keys to @enrolled-keys, and - drop the reference to @secure-boot from the end of the @enrolled-keys documentation paragraph? (Namely, "@secure-boot-enrolled-keys is only valid if the firmware also supports @secure-boot"). Or are you suggesting something more? Thanks! Laszlo

Hi,

So now the question is should this string be declared to be glob format, or regex format. Regex is more flexible, but regex syntax is ill-defined because every regex engine is slightly different.

I'd go for glob. It's good enough for the version number and most likely we don't need anything else. And the machine types are a list, so there is still the option to just have multiple entries in case it turns out we need something glob can't cover with a single entry. cheers, Gerd

On 04/18/18 14:32, Daniel P. Berrangé wrote:

On Wed, Apr 18, 2018 at 02:30:02PM +0200, Gerd Hoffmann wrote:

...

Hi,

...

So now the question is should this string be declared to be glob format, or regex format. Regex is more flexible, but regex syntax is ill-defined because every regex engine is slightly different.

I'd go for glob. It's good enough for the version number and most likely we don't need anything else.

And the machine types are a list, so there is still the option to just have multiple entries in case it turns out we need something glob can't cover with a single entry.

Ok, works for me.

Me too, thanks. Laszlo

On 04/18/18 14:23, Gerd Hoffmann wrote:

Hi,

...

Hmmm, I'm not sure I agree. One use case is that you want a domain config in which well-known OS-es, signed by the MS UEFI certs, just boot with SB enabled. (Some of our internal folks really want this.)

Another use case is that you want a domain in which SB *can* be enabled, but your installer is signed with a different certificate chain (or it is unsigned), and with *just* the MS certs enrolled, it wouldn't boot at all. So you want the SB *feature*, but definitely not the initial enrollment / SB *operational mode*.

So "secure-boot-enrolled-keys" also has SB turned on.

Yes.

...

For me to understand you better, are you suggesting merely that I:

- rename @secure-boot-enrolled-keys to @enrolled-keys, and

- drop the reference to @secure-boot from the end of the @enrolled-keys documentation paragraph? (Namely, "@secure-boot-enrolled-keys is only valid if the firmware also supports @secure-boot").

Yes. So "secure-boot" specifies "firmware binary supports secure-boot" and "enrolled-keys" specifies "firmware nvram template has keys enrolled (and SB enabled).

OK.

Other question: Do we want allow to specify which certs/keys are enrolled? Which would probably mean to drop "enrolled-keys" from features and make it an optional string instead,

Not an enum? "Microsoft" below should be an enum constant, shouldn't it?

then specify "'enrolled-keys' : 'Microsoft'" in the json file.

If this is really necessary (up to Dan :) ), I'm down with it. Thanks Laszlo

On 04/18/18 15:06, Gerd Hoffmann wrote:

...

...

Other question: Do we want allow to specify which certs/keys are enrolled? Which would probably mean to drop "enrolled-keys" from features and make it an optional string instead,

Not an enum? "Microsoft" below should be an enum constant, shouldn't it?

I don't think so. If we want allow other certificate providers (not sure it makes sense as all physical hardware actually runs with the microsoft certificates), then we don't want a fixed list here. So any CA can be listed, be it microsoft, redhat, canonical, verisign or kraxel.org ;)

I agree (obviously), but then, at what detail do we stop? Because, if we go for full flexibility, then we should formalize: - the certificate that goes into PK, - the list of certificates that go into KEK, - the list of certificates that go into db, and we should likely formalize "certificate" itself as the following pair: - human-readable description (possibly the Common Name of the Subject), - SHA256 digest (fingerprint) of the certificate. I do think this would totally be overkill, but I don't know where to draw the line - between the currently proposed @enrolled-keys (or similar enums that stand for well-defined "constellations") - and the full details as described above. A simple string like "Microsoft" doesn't seem to me helpful for either the user or management software -- the former won't know what "Microsoft" stands for, and the latter won't want to look for free-form strings. (Same issue as with @tags vs. @features.) Thanks Laszlo

On 04/18/18 15:53, Daniel P. Berrangé wrote:

On Wed, Apr 18, 2018 at 03:30:36PM +0200, Laszlo Ersek wrote:

...

On 04/18/18 15:06, Gerd Hoffmann wrote:

...

...

...

Other question: Do we want allow to specify which certs/keys are enrolled? Which would probably mean to drop "enrolled-keys" from features and make it an optional string instead,

Not an enum? "Microsoft" below should be an enum constant, shouldn't it?

I don't think so. If we want allow other certificate providers (not sure it makes sense as all physical hardware actually runs with the microsoft certificates), then we don't want a fixed list here. So any CA can be listed, be it microsoft, redhat, canonical, verisign or kraxel.org ;)

I agree (obviously), but then, at what detail do we stop?

Because, if we go for full flexibility, then we should formalize: - the certificate that goes into PK, - the list of certificates that go into KEK, - the list of certificates that go into db,

and we should likely formalize "certificate" itself as the following pair: - human-readable description (possibly the Common Name of the Subject), - SHA256 digest (fingerprint) of the certificate.

I do think this would totally be overkill, but I don't know where to draw the line - between the currently proposed @enrolled-keys (or similar enums that stand for well-defined "constellations") - and the full details as described above.

A simple string like "Microsoft" doesn't seem to me helpful for either the user or management software -- the former won't know what "Microsoft" stands for, and the latter won't want to look for free-form strings. (Same issue as with @tags vs. @features.)

Ignoring secboot cert name for a minute, ultimately no matter what we do in terms of metadata there is always going to be the possibility that many firmware images all match the criteria libvirt is searching on.

Apps thus need rules to pick one of the many matches, and users need the ability to override distro defaults. We can achieve that as follows:

Recommend that firmware files are created with a double-digit prefix eg 50-ovmf.json 50-seabios-256k.json, etc, etc, so they can be sorted in predictable order

Second, we should define three well known directory locations

- /usr/share/qemu/firmware (used by distros packages) - /etc/qemu/firmware (exclusively for sysadmin local additions) - $HOME/.config/qemu/firmware (exclusively for per-user local additions)

Apps like libvirt should build list of files from all three locations, then sort by filename. If a local directory has a file with same name as the distro directory, then it should replace the distro provided file. A zero length file should be simply hide the distro provided file

So for example, distro ships

/usr/share/qemu/firmware/50-ovmf.json /usr/share/qemu/firmware/50-seabios-256k.json

The sysadmin can now prevent the default ovmf being used at all with

$ touch /etc/qemu/firmware/50-ovmf.json

The sysadmin can replace/alter the distro default ovmf with

$ vim /etc/qemu/firmware/50-ovmf.json

Or they can provide a parallel ovmf with higher priority

$ vim /etc/qemu/firmware/10-ovmf.json

Or they can provide a parallel ovmf with lower priority

$ vim /etc/qemu/firmware/99-ovmf.json

$HOME/.config/qemu/firmware would take prior over /etc/qemu and /usr/share/qemu.

Getting back to the question of many ovmf images with various different secboot keys. The distro can now provide many ovmf images each with different keys, if desired and determine which one is picked by default.

The end user can provide their over ovmf with personal keys that replaces the distro microsoft enrolled one entirely, etc.

IOW, don't think we need to record which certs are use for secboot in the JSON metadata. Just lets overrides & ordering take care of it.

OK, thank you. Three more questions: - Would you like me to capture the directory paths in the firmware.json file, or in the commit message for the patch? - Should we keep @secure-boot-enrolled-keys (or, as Gerd suggests, @enrolled-keys) in the schema, as a feature enum constant, but remove the documentation of the actual certificates? (I.e., say "an unspecified set of certificates has been enrolled and SB mode has been enabled".) - Or else, should we drop the feature flag that stands for enrollment completely? Thanks, Laszlo

On 04/18/18 16:06, Daniel P. Berrangé wrote:

On Wed, Apr 18, 2018 at 04:03:03PM +0200, Laszlo Ersek wrote:

...

- Would you like me to capture the directory paths in the firmware.json file, or in the commit message for the patch?

Should be in whatever file ends up in the docs/specs directory eventually.

...

- Should we keep @secure-boot-enrolled-keys (or, as Gerd suggests, @enrolled-keys) in the schema, as a feature enum constant, but remove the documentation of the actual certificates? (I.e., say "an unspecified set of certificates has been enrolled and SB mode has been enabled".)

I think it is worth keeping the feature flag - we simply don't need to say /what/ keys.

All clear, thanks. Laszlo

On 04/19/18 02:09, David Gibson wrote:

On Wed, 18 Apr 2018 10:32:06 +0200 Laszlo Ersek <lersek@redhat.com> wrote:

...

On 04/18/18 08:02, Gerd Hoffmann wrote: [...] [...]

...

Looks good to me overall.

...

+{ 'enum' : 'FirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

openbios missing.

...

+{ 'enum' : 'FirmwareArchitecture', + 'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }

ppc(64) missing (but you have slof above ;) ... s390 too.

I figured those would be contributed by people that actually use them, as separate patches :) In fact I would rather prefer removing "slof" and "uboot" from this initial version, because I have zero clue about them.

I've only been able to skim this discussion, so apologies if I've missed things. I'm pretty unclear on the overall purpose of this, but in particular this FirmwareType field seems pretty weird.

Specifically the things in the list don't really seem comparable to each other: UEFI is a specified interface, BIOS is a de-facto interface. So far so good. But SLOF is a specific implementation of Open Firmware (of which we have a couple of other partial implementations used for other qemu platforms).

Thank you -- I will replace SLOF with "openfirmware". This also implies I shouldn't add "openbios" separately, which was suggested earlier by Gerd -- according to <https://en.wikipedia.org/wiki/OpenBIOS>, OpenBIOS is another implementation of OFW.

U-Boot is somewhere in between the two, a specific implementation that defines a fair bunch of its own interfaces.

Right, this is about interfaces, so I'll keep "uboot". Thank you! Laszlo

On 20/04/2018 03:03, David Gibson wrote:

...

This also implies I shouldn't add "openbios" separately, which was suggested earlier by Gerd -- according to <https://en.wikipedia.org/wiki/OpenBIOS>, OpenBIOS is another implementation of OFW.

Right. Although I think OpenBIOS and SLOF support a disjoint set of machines. Openhackware which is (was?) used on some machines is yet another (very partial) openfirmware implementation.

We can: 1a) replace the architecture field with an ABI field (seems wrong to me) 1b) add an "ABI" field (containing e.g. prep, spapr, etc.) to complement the architecture field 2) we include directly a glob pattern for the QEMU machine types (also seems wrong). 3) just like we effectively moved the guest ABI to the features field, we split the features into host-features and guest-features, and the host ABI (prep, spapr, etc.; but also OVMF's SMM requirement fits here for example) moves into host-features. Again, just like 1a/1b, this can be replace or complement the architecture field, and I think I'd prefer the latter Paolo

On 18.04.2018 08:02, Gerd Hoffmann wrote:

On Wed, Apr 18, 2018 at 12:40:54AM +0200, Laszlo Ersek wrote:

...

Add a schema that describes the different uses and properties of virtual machine firmware.

Looks good to me overall.

...

+{ 'enum' : 'FirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

openbios missing.

As mentioned somewhere else in this thread, I think it's best to just use "openfirmware" here instead of "slof" and "openbios"

...

+{ 'enum' : 'FirmwareArchitecture', + 'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }

ppc(64) missing (but you have slof above ;) ...

Since you distinguish between 32-bit and 64-bit, we need both "ppc" (32-bit) and "ppc64" (64-bit) here.

s390 too.

Please name that "s390x". "s390" without x means the old 31-bit architecture which is hardly used anymore today. "s390x" is the modern 64-bit architecture. Thomas

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

On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:

Laszlo Ersek <lersek@redhat.com> writes:

...

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.

configure gets its list like this:

default_target_list=""

mak_wilds=""

if [ "$softmmu" = "yes" ]; then mak_wilds="${mak_wilds} $source_path/default-configs/*-softmmu.mak" fi if [ "$linux_user" = "yes" ]; then mak_wilds="${mak_wilds} $source_path/default-configs/*-linux-user.mak" fi if [ "$bsd_user" = "yes" ]; then mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak" fi

for config in $mak_wilds; do default_target_list="${default_target_list} $(basename "$config" .mak)" done

Since we use QMP only in system emulation, a QAPI enum limited to the system emulation targets makes sense.

Replacing CpuInfoArch by such an enum will change the discriminator value from "other" to the real architecture, with the obvious compatibility concerns. But we've accepted similar changes twice already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.

"other" was a bad idea. Hindsight 20/20.

Getting rid of it in one go rather than piecemeal seems like the least bad way out. Too late for 2.12, though. Eric, what do you think?

Given the context in which this "other" value is used, I think it is reasonable to kill it and put a full arch list in there. No app is likely to be accessing the struct under "other" because it is just an empty placeholder. 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 :|

On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote:

On 04/19/18 09:56, Daniel P. Berrangé wrote:

...

On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:

...

Laszlo Ersek <lersek@redhat.com> writes:

...

On 04/18/18 10:47, Markus Armbruster wrote:

...

Laszlo Ersek <lersek@redhat.com> writes: Replacing CpuInfoArch by such an enum will change the discriminator value from "other" to the real architecture, with the obvious compatibility concerns. But we've accepted similar changes twice already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.

"other" was a bad idea. Hindsight 20/20.

Getting rid of it in one go rather than piecemeal seems like the least bad way out. Too late for 2.12, though. Eric, what do you think?

Given the context in which this "other" value is used, I think it is reasonable to kill it and put a full arch list in there.

No app is likely to be accessing the struct under "other" because it is just an empty placeholder.

Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the potential to confuse QMP clients that didn't expect "s390", but otherwise it didn't mess with preexistent enum values / structures.

NB, qemu-system-s390x would previously have returned "other" in this field, and now it returns "s390". So while it didn't remove "other" from the list of things that could potentially exist, it did change what the s390x binary will actually report.

The same applies to commit 25fa194b7b1, just with "riscv" / "CpuInfoRISCV" substituted.

Removing "other" might confuse QMP clients that expect it, except (according to Daniel) no such client exists, probably.

When I say removing "other", I imply that we add an explicit arch for all those which we currently are missing. IOW, all qemu-system-XXX binaries which currently report "other" would change to report their respective "XXX" values. So in this way, it is exactly the same as what we did when we introduced "s390" as an option. The only difference is that once we have every binary reporting the correct arch, we can now also remove "other" from the schema itself as it will then be unused. 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 :|

On Fri, Apr 20, 2018 at 10:11:08AM +0200, Laszlo Ersek wrote:

On 04/19/18 11:12, Daniel P. Berrangé wrote:

...

On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote:

...

On 04/19/18 09:56, Daniel P. Berrangé wrote:

...

On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:

...

Laszlo Ersek <lersek@redhat.com> writes:

...

On 04/18/18 10:47, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: Replacing CpuInfoArch by such an enum will change the discriminator value from "other" to the real architecture, with the obvious compatibility concerns. But we've accepted similar changes twice already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.

"other" was a bad idea. Hindsight 20/20.

Getting rid of it in one go rather than piecemeal seems like the least bad way out. Too late for 2.12, though. Eric, what do you think?

Given the context in which this "other" value is used, I think it is reasonable to kill it and put a full arch list in there.

No app is likely to be accessing the struct under "other" because it is just an empty placeholder.

Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the potential to confuse QMP clients that didn't expect "s390", but otherwise it didn't mess with preexistent enum values / structures.

NB, qemu-system-s390x would previously have returned "other" in this field, and now it returns "s390". So while it didn't remove "other" from the list of things that could potentially exist, it did change what the s390x binary will actually report.

...

The same applies to commit 25fa194b7b1, just with "riscv" / "CpuInfoRISCV" substituted.

Removing "other" might confuse QMP clients that expect it, except (according to Daniel) no such client exists, probably.

When I say removing "other", I imply that we add an explicit arch for all those which we currently are missing. IOW, all qemu-system-XXX binaries which currently report "other" would change to report their respective "XXX" values.

So in this way, it is exactly the same as what we did when we introduced "s390" as an option.

The only difference is that once we have every binary reporting the correct arch, we can now also remove "other" from the schema itself as it will then be unused.

Can we please translate this into more actionable items for me, because I'm getting confused :)

First, if I add "i386" and "x86_64" to the enum list, we'll have all three of "i386", "x86_64" and "x86". Is that useful? How will that work?

Hmm, yes, on closer look this is a big mess as it is. We've been using generic terms for covering multiple architectures :-( 'x86' for both i386 and x86_64, 'sparc' for sparc and sparc64, etc. If we try to fix that we'll be entering a world of backcompat hurt :-( Since your schema is likely to end up just being a file in docs/specs, rather than directly part of our existnig qapi schema, I suggest we just ignore whats there. Just define an arch enum in your spec which is right, and let someone else worry about fixing the mess

Second, assuming I add constants for the ~10 (?) softmmu arches, can I still use @CpuInfoOther as the type for the corresponding new members in @CpuInfo? What C code changes will be necessary?

Yes, we could still use the CpuInfoOther struct, since struct names are invisible to consumers, but as above, lets ignore the mess 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 :|

On Fri, Apr 20, 2018 at 11:46:24AM +0200, Gerd Hoffmann wrote:

Hi,

...

Since your schema is likely to end up just being a file in docs/specs, rather than directly part of our existnig qapi schema, I suggest we just ignore whats there. Just define an arch enum in your spec which is right, and let someone else worry about fixing the mess

I think it would be useful to have this as part of the schema. Should be easy then to write up a small utility then which takes a json file as input and translates that into qemu command line options.

Currently we have two distinct QAPI schemas, one covering the system emulator for QMP and subset of CLI args, and one covering the guest agent. This new schema isn't mapping to anything in QMP / CLI right now though. It can be used to decide how to generate CLI args, but it isn't describing those CLI args. So to me this is a 3rd distinct schema. 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 :|

On 04/20/18 12:41, Gerd Hoffmann wrote:

...

...

...

Since your schema is likely to end up just being a file in docs/specs,

...

...

I think it would be useful to have this as part of the schema. Should be easy then to write up a small utility then which takes a json file as input and translates that into qemu command line options.

Currently we have two distinct QAPI schemas, one covering the system emulator for QMP and subset of CLI args, and one covering the guest agent.

Yep, we can make it a third one, fine with me. But it likewise wouldn't end up in docs/ then.

Let's please focus on the schema itself for now, with the stupid little test command included. Once the schema enjoys universal popularity, I'll be glad to pass it on to QMP wizards -- because, if we ultimately wanted to keep the schema code-enabled, then Markus's earlier suggestions apply too, regarding QMP test cases. I'll admit I haven't dared look at those yet (in particular because Paolo originally suggested to move this under docs/interop). Looking at my todo list, I'd really like to focus on the schema itself. Thanks Laszlo

On Fri, Apr 20, 2018 at 10:34:57AM +0100, Daniel P. Berrangé wrote:

On Fri, Apr 20, 2018 at 10:11:08AM +0200, Laszlo Ersek wrote:

...

On 04/19/18 11:12, Daniel P. Berrangé wrote:

...

On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote:

...

On 04/19/18 09:56, Daniel P. Berrangé wrote:

...

On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:

...

Laszlo Ersek <lersek@redhat.com> writes:

> On 04/18/18 10:47, Markus Armbruster wrote: >> Laszlo Ersek <lersek@redhat.com> writes: Replacing CpuInfoArch by such an enum will change the discriminator value from "other" to the real architecture, with the obvious compatibility concerns. But we've accepted similar changes twice already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.

"other" was a bad idea. Hindsight 20/20.

Getting rid of it in one go rather than piecemeal seems like the least bad way out. Too late for 2.12, though. Eric, what do you think?

Given the context in which this "other" value is used, I think it is reasonable to kill it and put a full arch list in there.

No app is likely to be accessing the struct under "other" because it is just an empty placeholder.

Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the potential to confuse QMP clients that didn't expect "s390", but otherwise it didn't mess with preexistent enum values / structures.

NB, qemu-system-s390x would previously have returned "other" in this field, and now it returns "s390". So while it didn't remove "other" from the list of things that could potentially exist, it did change what the s390x binary will actually report.

...

The same applies to commit 25fa194b7b1, just with "riscv" / "CpuInfoRISCV" substituted.

Removing "other" might confuse QMP clients that expect it, except (according to Daniel) no such client exists, probably.

When I say removing "other", I imply that we add an explicit arch for all those which we currently are missing. IOW, all qemu-system-XXX binaries which currently report "other" would change to report their respective "XXX" values.

So in this way, it is exactly the same as what we did when we introduced "s390" as an option.

The only difference is that once we have every binary reporting the correct arch, we can now also remove "other" from the schema itself as it will then be unused.

Can we please translate this into more actionable items for me, because I'm getting confused :)

First, if I add "i386" and "x86_64" to the enum list, we'll have all three of "i386", "x86_64" and "x86". Is that useful? How will that work?

Hmm, yes, on closer look this is a big mess as it is. We've been using generic terms for covering multiple architectures :-( 'x86' for both i386 and x86_64, 'sparc' for sparc and sparc64, etc. If we try to fix that we'll be entering a world of backcompat hurt :-(

Since your schema is likely to end up just being a file in docs/specs, rather than directly part of our existnig qapi schema, I suggest we just ignore whats there. Just define an arch enum in your spec which is right, and let someone else worry about fixing the mess

Trouble is, for these "biarch" cases, I'm not sure it's always clear what the right value for a firmware is. While whether a userspace binary is i386 or x86_64 is clear and well-defined, a firmware could well be responsible for switching the CPU from its reset mode into the more modern 64-bit mode, and would therefore have at least some code in both archs.

...

Second, assuming I add constants for the ~10 (?) softmmu arches, can I still use @CpuInfoOther as the type for the corresponding new members in @CpuInfo? What C code changes will be necessary?

Yes, we could still use the CpuInfoOther struct, since struct names are invisible to consumers, but as above, lets ignore the mess

Regards, Daniel

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laszlo Ersek <lersek@redhat.com> writes:

On 04/19/18 09:56, Daniel P. Berrangé wrote:

...

On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:

...

Laszlo Ersek <lersek@redhat.com> writes:

...

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.

configure gets its list like this:

default_target_list=""

mak_wilds=""

if [ "$softmmu" = "yes" ]; then mak_wilds="${mak_wilds} $source_path/default-configs/*-softmmu.mak" fi if [ "$linux_user" = "yes" ]; then mak_wilds="${mak_wilds} $source_path/default-configs/*-linux-user.mak" fi if [ "$bsd_user" = "yes" ]; then mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak" fi

for config in $mak_wilds; do default_target_list="${default_target_list} $(basename "$config" .mak)" done

Since we use QMP only in system emulation, a QAPI enum limited to the system emulation targets makes sense.

Replacing CpuInfoArch by such an enum will change the discriminator value from "other" to the real architecture, with the obvious compatibility concerns. But we've accepted similar changes twice already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.

"other" was a bad idea. Hindsight 20/20.

Getting rid of it in one go rather than piecemeal seems like the least bad way out. Too late for 2.12, though. Eric, what do you think?

Given the context in which this "other" value is used, I think it is reasonable to kill it and put a full arch list in there.

No app is likely to be accessing the struct under "other" because it is just an empty placeholder.

Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the potential to confuse QMP clients that didn't expect "s390", but otherwise it didn't mess with preexistent enum values / structures.

The same applies to commit 25fa194b7b1, just with "riscv" / "CpuInfoRISCV" substituted.

Removing "other" might confuse QMP clients that expect it, except (according to Daniel) no such client exists, probably.

It's a done deal anyway; we're not going to hold 2.12 to avoid this QMP compatibility break.

However, replacing the current list of CpuInfoArch constants with the system emulation target list would be more intrusive than all three of the above. In particular there is no "x86" target; only i386 and x86_64 targets exist. For the firmware schema, this distinction is important, but it would break QMP clients that expect "x86" (and such clients must certainly exist).

You're right. Another nice mess.

The targets with softmmu are: aarch64, alpha, arm, cris, hppa, i386, lm32, m68k, microblaze, microblazeel, mips, mips64, mips64el, mipsel, moxie, nios2, or1k, ppc, ppc64, ppcemb, riscv32, riscv64, s390x, sh4, sh4eb, sparc, sparc64, tricore, unicore32, x86_64, xtensa, xtensaeb.

That is, at least the following constants in CpuInfoArch have no corresponding (identical) mapping -- I'll state to the right of the arrow the emulation targets which I know or presume to be associated with the CpuInfoArch constant: - x86 -> i386, x86_64 - sparc -> sparc, sparc64 - ppc -> ppc, ppc64, ppcemb - mips -> mips, mips64, mips64el, mipsel - s390 -> s390x - riscv -> riscv32, riscv64

The only constant that seems to have a 1:1 mapping is "tricore", but I could perfectly well be thinking even that just because I have no clue about "tricore".

So, I don't think CpuInfoArch can be updated so that it both remains compatible with current QMP clients, and serves "firmware.json". In the firmware schema we don't just need CPU architecture, but actual emulator names (and board / machine types).

The completely orthodox fix for CpuInfo would be: * Keep @arch unchanged. In particular, keep "other" for all targets other than 'x86', 'sparc', 'ppc', 'mips', 'tricore'. * Add a new member @target of new enum type Target, whose values match configures idea of targets exactly. * Make the new member the union tag. It's too late for complete orthodoxy; we already changed @arch. A common enum type Target makes sense anyway, I think. Using it in CpuInfo as described above may make sense, too. Could be left to a follow-up patch.

I grepped 'qapi/*json' for the whole word "x86_64", and the only thing that remotely matches is the @TargetInfo structure, in which the @arch field is a string, coming with the example "x86_64". The example also names "i386" separately.

Well spotted. TargetInfo member @arch is set to TARGET_NAME, which matches configure's idea of the target. If we add enum Target, we should change @arch's type from str to Target.

So what might make sense is to introduce a separate enum in "qapi/misc.json" with all the softmmu targets I listed above, and change the type of @TargetInfo.@arch to that enum.

I arrived at this conclusion, too.

In parallel, qmp_query_target() would have to be updated to look up the enum value matching TARGET_NAME. (I do think we can ignore linux-user etc emulators for collecting the relevant arches here: @TargetInfo is only used in the "query-target" QMP command, and Markus said above that QMP is only used with system emulation.)

Should I do this?

Yes, please.

Laszlo Ersek <lersek@redhat.com> writes:

On 04/20/18 14:53, Markus Armbruster wrote:

...

Laszlo Ersek <lersek@redhat.com> writes:

[snip]

...

...

The targets with softmmu are: aarch64, alpha, arm, cris, hppa, i386, lm32, m68k, microblaze, microblazeel, mips, mips64, mips64el, mipsel, moxie, nios2, or1k, ppc, ppc64, ppcemb, riscv32, riscv64, s390x, sh4, sh4eb, sparc, sparc64, tricore, unicore32, x86_64, xtensa, xtensaeb.

That is, at least the following constants in CpuInfoArch have no corresponding (identical) mapping -- I'll state to the right of the arrow the emulation targets which I know or presume to be associated with the CpuInfoArch constant: - x86 -> i386, x86_64 - sparc -> sparc, sparc64 - ppc -> ppc, ppc64, ppcemb - mips -> mips, mips64, mips64el, mipsel - s390 -> s390x - riscv -> riscv32, riscv64

The only constant that seems to have a 1:1 mapping is "tricore", but I could perfectly well be thinking even that just because I have no clue about "tricore".

So, I don't think CpuInfoArch can be updated so that it both remains compatible with current QMP clients, and serves "firmware.json". In the firmware schema we don't just need CPU architecture, but actual emulator names (and board / machine types).

The completely orthodox fix for CpuInfo would be:

* Keep @arch unchanged. In particular, keep "other" for all targets other than 'x86', 'sparc', 'ppc', 'mips', 'tricore'.

* Add a new member @target of new enum type Target, whose values match configures idea of targets exactly.

* Make the new member the union tag.

It's too late for complete orthodoxy; we already changed @arch.

A common enum type Target makes sense anyway, I think.

Using it in CpuInfo as described above may make sense, too. Could be left to a follow-up patch.

...

I grepped 'qapi/*json' for the whole word "x86_64", and the only thing that remotely matches is the @TargetInfo structure, in which the @arch field is a string, coming with the example "x86_64". The example also names "i386" separately.

Well spotted.

TargetInfo member @arch is set to TARGET_NAME, which matches configure's idea of the target. If we add enum Target, we should change @arch's type from str to Target.

...

So what might make sense is to introduce a separate enum in "qapi/misc.json" with all the softmmu targets I listed above, and change the type of @TargetInfo.@arch to that enum.

I arrived at this conclusion, too.

...

In parallel, qmp_query_target() would have to be updated to look up the enum value matching TARGET_NAME. (I do think we can ignore linux-user etc emulators for collecting the relevant arches here: @TargetInfo is only used in the "query-target" QMP command, and Markus said above that QMP is only used with system emulation.)

Should I do this?

Yes, please.

OK, so here's my understanding:

(1) I'll introduce a new @Target enum in misc.json. I'll inherit / include it in firmware.json. This is compatible with what Dan said.

(2) I'll change @TargetInfo.@arch to the new type. I believe this will break the compilation of qmp_query_target(); I'll hack on that until it builds again, with the lookup. :)

(3) Regarding the addition of @target to CpuInfo (accompanying @arch) doesn't look hard; what *does* look hard is changing the union discriminator from @arch to @target. @target has many more values, and I would have to map all of them to the (fewer) @arch values that currently do *not* select @CpuInfoOther. Here's an example:

- Both @i386 and @x86_64 from @target mean @x86 in @arch, - because @x86 currently selects @CpuInfoX86, not @CpuInfoOther, both @i386 and @x86_64 must be assigned @CpuInfoX86.

This depends on the knowledge that "x86" actually *means* "i386 plus x86_64", and I totally don't have that knowledge for the rest of the arches / targets.

Start with qmp_query_cpus()'s code to initialize @arch: #if defined(TARGET_I386) info->value->arch = CPU_INFO_ARCH_X86; info->value->u.x86.pc = env->eip + env->segs[R_CS].base; #elif defined(TARGET_PPC) info->value->arch = CPU_INFO_ARCH_PPC; info->value->u.ppc.nip = env->nip; #elif defined(TARGET_SPARC) info->value->arch = CPU_INFO_ARCH_SPARC; info->value->u.q_sparc.pc = env->pc; info->value->u.q_sparc.npc = env->npc; #elif defined(TARGET_MIPS) info->value->arch = CPU_INFO_ARCH_MIPS; info->value->u.q_mips.PC = env->active_tc.PC; #elif defined(TARGET_TRICORE) info->value->arch = CPU_INFO_ARCH_TRICORE; info->value->u.tricore.PC = env->PC; #elif defined(TARGET_S390X) info->value->arch = CPU_INFO_ARCH_S390; info->value->u.s390.cpu_state = env->cpu_state; #elif defined(TARGET_RISCV) info->value->arch = CPU_INFO_ARCH_RISCV; info->value->u.riscv.pc = env->pc; #else info->value->arch = CPU_INFO_ARCH_OTHER; #endif The TARGET_FOO come from config-target.h, generated by scripts/create_config from config-target.mak. It derives FOO is from config-target.mak's TARGET_BASE_ARCH. config-target.mak is in turn generated by configure. It computes TARGET_BASE_ARCH from target_name. And that's your mapping.

So, the modification of @CpuInfo I would indeed like to pass off to someone else. (Well, if all the architecture maintainers follow up and tell me what emulators exactly fall under the umbrella of each individual @arch value, I can post the patch.) BTW... I wonder how compatibility would be affected if we just added @target to @CpuInfo, even without making it the new discriminator.

Doesn't work for "completely orthodox", because then @arch "other" comprises totally different targets. But we're not doing that. We might have to switch the union tag eventually, but if there's no hard reason to switch it now, feel free to leave it for later. A comment explaining this mess would be nice, though.

... Anyway, I think I've gotten a huge amount of useful and actionable feedback; thanks everyone, let me work on RFCv3 and post it soon. :)

I'm not demanding you do this work. If you can do it, wonderful! But if all you can do is introduce the new enum Target and leave the CpuInfo mess alone, that's okay. Just add a suitable TODO then.

On Wed, Apr 18, 2018 at 01:52:19PM +0200, Laszlo Ersek wrote:

On 04/18/18 11:43, Paolo Bonzini wrote:

...

On 18/04/2018 00:40, Laszlo Ersek wrote:

...

+# +# Lists firmware types commonly used with QEMU virtual machines. +# +# @bios: The firmware was built from the SeaBIOS project. +# +# @slof: The firmware was built from the Slimline Open Firmware project. +# +# @uboot: The firmware was built from the U-Boot project. +# +# @uefi: The firmware was built from the edk2 (EFI Development Kit II) project. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

A very basic question (so not likely a suggestion for change). Why wouldn't these be features too? For example a UEFI with CSM could expose both uefi and bios, a u-boot with UEFI support could expose both uboot and uefi, etc.

Good point. I considered "type" to be a given, from the initial brainstorming, but if Dan is OK with your suggestion, I can turn these into features as well.

...

Perhaps there are two dimensions, the FirmwareType tells you how to configure it and the FirmwareFeature tells you the APIs it exposes to the guest?

I don't know enough firmware types to answer this :) I believe I agree about the FirmwareFeature statement (if we also include "platform requirements" there), but I have no clue about any generalizations for firmware configuration.

IIUC Paolo is basically suggesting { "description": "OVMF firmware" "type": "uefi", "features": [ "uefi", "bios" ], } where 'bios' is only listed if CSM is enabled in the OVMF build. I tend to think that is redundant and we could just do { "description": "OVMF firmware" "features": [ "uefi", "bios" ], } And declare the order of "features" list is significant. ie "features": [ "uefi", "bios" ], means a UEFI firmware which has back compat for BIOS (ie OVMF with CSM) while "features": [ "bios" "uefi", ], means a traditional BIOS firmware with compat for UEFI (thinking uboot being able to include uefi support in this case) 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 :|

On 04/18/18 14:03, Daniel P. Berrangé wrote:

On Wed, Apr 18, 2018 at 01:52:19PM +0200, Laszlo Ersek wrote:

...

On 04/18/18 11:43, Paolo Bonzini wrote:

...

On 18/04/2018 00:40, Laszlo Ersek wrote:

...

+# +# Lists firmware types commonly used with QEMU virtual machines. +# +# @bios: The firmware was built from the SeaBIOS project. +# +# @slof: The firmware was built from the Slimline Open Firmware project. +# +# @uboot: The firmware was built from the U-Boot project. +# +# @uefi: The firmware was built from the edk2 (EFI Development Kit II) project. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

A very basic question (so not likely a suggestion for change). Why wouldn't these be features too? For example a UEFI with CSM could expose both uefi and bios, a u-boot with UEFI support could expose both uboot and uefi, etc.

Good point. I considered "type" to be a given, from the initial brainstorming, but if Dan is OK with your suggestion, I can turn these into features as well.

...

Perhaps there are two dimensions, the FirmwareType tells you how to configure it and the FirmwareFeature tells you the APIs it exposes to the guest?

I don't know enough firmware types to answer this :) I believe I agree about the FirmwareFeature statement (if we also include "platform requirements" there), but I have no clue about any generalizations for firmware configuration.

IIUC Paolo is basically suggesting

{ "description": "OVMF firmware" "type": "uefi", "features": [ "uefi", "bios" ], }

where 'bios' is only listed if CSM is enabled in the OVMF build. I tend to think that is redundant and we could just do

{ "description": "OVMF firmware" "features": [ "uefi", "bios" ], }

Actually, this is how I interpreted Paolo's idea at once. I agree the "type" member can be dropped.

And declare the order of "features" list is significant. ie

"features": [ "uefi", "bios" ],

means a UEFI firmware which has back compat for BIOS (ie OVMF with CSM) while

"features": [ "bios" "uefi", ],

means a traditional BIOS firmware with compat for UEFI (thinking uboot being able to include uefi support in this case)

Is it guaranteed that lists in JSON keep the order of the elements? Because, dictionaries definitely don't promise any ordering between the keys. Thanks, Laszlo

On 04/18/18 17:17, Eric Blake wrote:

On 04/18/2018 07:40 AM, Laszlo Ersek wrote:

...

Is it guaranteed that lists in JSON keep the order of the elements? Because, dictionaries definitely don't promise any ordering between the keys.

Yes, JSON lists preserve order (and we have to explicitly document cases where order within a list is not significant).

Thanks. After digesting the comments on @type for a while longer, from Dan, Gerd and Paolo, I think we *should* keep @type, but rather than having it be a simple enum, let's make it a list of enums, where order matters. Keep @features separate, and document that order does not matter there. Here's why: my brain is crashing trying to come up with a human-parseable explanation why for *some* entries in @features, their relative order is important, and why for some others, it isn't. Discerning the subset for which order matters, from any specific grab-bag of @features, will be no fun for the human user. Given that the "ordered features" idea was brought to life solely because a firmware can provide multiple interfaces (with a strong preference order between them), I guess we might as well be honest about that, and update @type accordingly. Thoughts? :) Thanks, Laszlo

On 04/18/18 17:27, Laszlo Ersek wrote:

On 04/18/18 17:17, Eric Blake wrote:

...

On 04/18/2018 07:40 AM, Laszlo Ersek wrote:

...

Is it guaranteed that lists in JSON keep the order of the elements? Because, dictionaries definitely don't promise any ordering between the keys.

Yes, JSON lists preserve order (and we have to explicitly document cases where order within a list is not significant).

Thanks.

After digesting the comments on @type for a while longer, from Dan, Gerd and Paolo, I think we *should* keep @type, but rather than having it be a simple enum, let's make it a list of enums, where order matters. Keep @features separate, and document that order does not matter there.

Here's why: my brain is crashing trying to come up with a human-parseable explanation why for *some* entries in @features, their relative order is important, and why for some others, it isn't. Discerning the subset for which order matters, from any specific grab-bag of @features, will be no fun for the human user.

Given that the "ordered features" idea was brought to life solely because a firmware can provide multiple interfaces (with a strong preference order between them), I guess we might as well be honest about that, and update @type accordingly.

Thoughts? :)

I mean I realize I just reinvented Paolo's point from <http://mid.mail-archive.com/c5e3f942-2c0e-9809-32ae-df97eaf2b7cd@redhat.com> :) I don't intend to claim the idea, I just needed some time to understand it. Laszlo