[libvirt] [PATCH v2 00/15] Firmware auto selection

v2 of: https://www.redhat.com/archives/libvir-list/2019-February/msg01503.html As usual, you can find my patches at my github: https://github.com/zippy2/libvirt/commits/firmware_v2 diff to v1: - Hopefully all Lazlo's comment are worked in (fixing the logic that chooses suitable firmware for secboot/SMM domains, commit message adjustements, sanity check for parsed FW descriptions, etc.) - Added more debug messages around FW selection code, i.e. it'll be visible from the logs why given FS is not suitable (or that it is). Michal Prívozník (15): virmock: Initialize both symbols in VIR_MOCK_REAL_INIT_ALT qemu_domain: Separate NVRAM VAR store file name generation qemu_capabilities: Expose qemu <-> libvirt arch translators virDomainLoaderDefParseXML: Allow loader path to be NULL conf: Introduce VIR_DOMAIN_LOADER_TYPE_NONE conf: Introduce firmware attribute to <os/> qemu: Introduce basic skeleton for parsing firmware description test: Introduce qemufirmwaretest qemu_firmware: Introduce qemuFirmwareFetchConfigs qemufirmwaretest: Test qemuFirmwareFetchConfigs() qemu_firmware: Introduce qemuFirmwareFillDomain() qemu_process: Call qemuFirmwareFillDomain qemuDomainDefValidate: Don't require SMM if automatic firmware selection enabled qemu: Enable firmware autoselection qemuxml2argvtest: Test os.firmware autoselection docs/formatdomain.html.in | 22 +- docs/schemas/domaincommon.rng | 12 +- src/conf/domain_conf.c | 113 +- src/conf/domain_conf.h | 15 +- src/libvirt_private.syms | 2 + src/qemu/Makefile.inc.am | 2 + src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 34 +- src/qemu/qemu_domain.h | 4 + src/qemu/qemu_firmware.c | 1364 +++++++++++++++++ src/qemu/qemu_firmware.h | 50 + src/qemu/qemu_process.c | 5 + tests/Makefile.am | 14 +- tests/domaincapsschemadata/full.xml | 1 + .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + .../etc/qemu/firmware/60-ovmf-sb.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../usr/share/qemu/firmware/40-bios.json | 35 + .../share/qemu/firmware/50-ovmf-sb-keys.json | 36 + .../usr/share/qemu/firmware/60-ovmf-sb.json | 35 + .../usr/share/qemu/firmware/61-ovmf.json | 33 + .../usr/share/qemu/firmware/70-aavmf.json | 35 + tests/qemufirmwaretest.c | 135 ++ ...arch64-os-firmware-efi.aarch64-latest.args | 37 + .../aarch64-os-firmware-efi.xml | 30 + .../os-firmware-bios.x86_64-latest.args | 39 + tests/qemuxml2argvdata/os-firmware-bios.xml | 68 + ...os-firmware-efi-secboot.x86_64-latest.args | 43 + .../os-firmware-efi-secboot.xml | 68 + .../os-firmware-efi.x86_64-latest.args | 42 + tests/qemuxml2argvdata/os-firmware-efi.xml | 68 + tests/qemuxml2argvtest.c | 17 + .../aarch64-os-firmware-efi.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-bios.xml | 1 + .../os-firmware-efi-secboot.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-efi.xml | 1 + tests/qemuxml2xmltest.c | 27 + tests/virmock.h | 5 +- 40 files changed, 2374 insertions(+), 30 deletions(-) create mode 100644 src/qemu/qemu_firmware.c create mode 100644 src/qemu/qemu_firmware.h create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json create mode 100644 tests/qemufirmwaretest.c create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-bios.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi.xml -- 2.19.2

It may happen that both symbols are present. Especially when chaining mocks. For instance if a test is using virpcimock and then both stat and __xstat would be present in the address space as virpcimock implements both. Then, if the test would try to use say virfilewrapper (which again uses VIR_MOCK_REAL_INIT_ALT() to init real_stat and real___xstat) it would find stat() from virpcimock and stop there. The virfilewrapper.c:real___xstat wouldn't be initialized and thus it may result in a segfault. The reason for segfault is that sys/stat.h may redefine stat() to call __xstat(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virmock.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/virmock.h b/tests/virmock.h index 853dbb8be2..9c7ecf60ce 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -292,8 +292,9 @@ # define VIR_MOCK_REAL_INIT_ALT(name1, name2) \ do { \ - if (!(real_ ## name1 = dlsym(RTLD_NEXT, #name1)) && \ - !(real_ ## name2 = dlsym(RTLD_NEXT, #name2))) { \ + real_ ## name1 = dlsym(RTLD_NEXT, #name1); \ + real_ ## name2 = dlsym(RTLD_NEXT, #name2); \ + if (!real_##name1 && !real_##name2) { \ fprintf(stderr, "Cannot find real '%s' or '%s' symbol\n", \ #name1, #name2); \ abort(); \ -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:11AM +0100, Michal Privoznik wrote:
It may happen that both symbols are present. Especially when chaining mocks. For instance if a test is using virpcimock and then both stat and __xstat would be present in the address space as virpcimock implements both. Then, if the test would try to use say virfilewrapper (which again uses VIR_MOCK_REAL_INIT_ALT() to init real_stat and real___xstat) it would find stat() from virpcimock and stop there. The virfilewrapper.c:real___xstat wouldn't be initialized and thus it may result in a segfault.
The reason for segfault is that sys/stat.h may redefine stat() to call __xstat().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virmock.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> This looks independant of the series so can just be pushed 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 :|

Move the code that (possibly) generates filename of NVRAM VAR store into a single function so that it can be re-used later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1487268a89..2b0d05f2db 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3874,14 +3874,8 @@ qemuDomainDefPostParse(virDomainDefPtr def, goto cleanup; } - if (def->os.loader && - def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && - def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON && - !def->os.loader->nvram) { - if (virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd", - cfg->nvramDir, def->name) < 0) - goto cleanup; - } + if (qemuDomainNVRAMPathGenerate(cfg, def) < 0) + goto cleanup; if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0) goto cleanup; @@ -13956,3 +13950,19 @@ qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk) virStorageSourceIsLocalStorage(disk->src) && disk->src->path && !virFileExists(disk->src->path); } + + +int +qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def) +{ + if (def->os.loader && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && + def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON && + !def->os.loader->nvram) { + return virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd", + cfg->nvramDir, def->name); + } + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7c6b50184c..136a7a7c72 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1107,4 +1107,8 @@ qemuDomainIsUsingNoShutdown(qemuDomainObjPrivatePtr priv); bool qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk); +int +qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def); + #endif /* LIBVIRT_QEMU_DOMAIN_H */ -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:12AM +0100, Michal Privoznik wrote:
Move the code that (possibly) generates filename of NVRAM VAR store into a single function so that it can be re-used later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- src/qemu/qemu_domain.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 22 insertions(+), 8 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

In some cases, the string representing architecture is different in qemu and libvirt. That is the reason why we have virQEMUCapsArchFromString() and virQEMUCapsArchToString(). So far, we did not need them outside of qemu_capabilities code, but this will change shortly. Expose them then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c9700193fd..4c8229fbda 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -616,7 +616,7 @@ static int virQEMUCapsOnceInit(void) VIR_ONCE_GLOBAL_INIT(virQEMUCaps); -static virArch virQEMUCapsArchFromString(const char *arch) +virArch virQEMUCapsArchFromString(const char *arch) { if (STREQ(arch, "i386")) return VIR_ARCH_I686; @@ -629,7 +629,7 @@ static virArch virQEMUCapsArchFromString(const char *arch) } -static const char *virQEMUCapsArchToString(virArch arch) +const char *virQEMUCapsArchToString(virArch arch) { if (arch == VIR_ARCH_I686) return "i386"; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1d677f6a06..06c7606e2f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -643,4 +643,7 @@ bool virQEMUCapsCPUFilterFeatures(const char *name, virSEVCapabilityPtr virQEMUCapsGetSEVCapabilities(virQEMUCapsPtr qemuCaps); +virArch virQEMUCapsArchFromString(const char *arch); +const char *virQEMUCapsArchToString(virArch arch); + #endif /* LIBVIRT_QEMU_CAPABILITIES_H */ -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:13AM +0100, Michal Privoznik wrote:
In some cases, the string representing architecture is different in qemu and libvirt. That is the reason why we have virQEMUCapsArchFromString() and virQEMUCapsArchToString(). So far, we did not need them outside of qemu_capabilities code, but this will change shortly. Expose them then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Except not really. At least for now. In the future, the firmware will be selected automagically. Therefore, it makes no sense to require the pathname of a specific firmware binary in the domain XML. But since it is not implemented do not really allow the path to be NULL. Only move code around to prepare it for further expansion. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- docs/schemas/domaincommon.rng | 4 +++- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 80f9f84f70..c8d63c4912 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -283,7 +283,9 @@ </choice> </attribute> </optional> - <ref name="absFilePath"/> + <optional> + <ref name="absFilePath"/> + </optional> </element> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eb660f5764..053b2cb210 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6590,6 +6590,22 @@ virDomainDefMemtuneValidate(const virDomainDef *def) } +static int +virDomainDefOSValidate(const virDomainDef *def) +{ + if (!def->os.loader) + return 0; + + if (!def->os.loader->path) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("no loader path specified")); + return -1; + } + + return 0; +} + + static int virDomainDefValidateInternal(const virDomainDef *def) { @@ -6628,6 +6644,9 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefMemtuneValidate(def) < 0) return -1; + if (virDomainDefOSValidate(def) < 0) + return -1; + return 0; } @@ -18246,6 +18265,9 @@ virDomainLoaderDefParseXML(xmlNodePtr node, type_str = virXMLPropString(node, "type"); loader->path = (char *) xmlNodeGetContent(node); + if (STREQ_NULLABLE(loader->path, "")) + VIR_FREE(loader->path); + if (readonly_str && (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { virReportError(VIR_ERR_XML_DETAIL, @@ -26989,9 +27011,12 @@ virDomainLoaderDefFormat(virBufferPtr buf, if (loader->secure) virBufferAsprintf(buf, " secure='%s'", secure); - virBufferAsprintf(buf, " type='%s'>", type); + virBufferAsprintf(buf, " type='%s'", type); - virBufferEscapeString(buf, "%s</loader>\n", loader->path); + if (loader->path) + virBufferEscapeString(buf, ">%s</loader>\n", loader->path); + else + virBufferAddLit(buf, "/>\n"); if (loader->nvram || loader->templt) { virBufferAddLit(buf, "<nvram"); virBufferEscapeString(buf, " template='%s'", loader->templt); -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:14AM +0100, Michal Privoznik wrote:
Except not really. At least for now.
In the future, the firmware will be selected automagically. Therefore, it makes no sense to require the pathname of a specific firmware binary in the domain XML. But since it is not implemented do not really allow the path to be NULL. Only move code around to prepare it for further expansion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- docs/schemas/domaincommon.rng | 4 +++- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 03/07/19 10:29, Michal Privoznik wrote:
Except not really. At least for now.
In the future, the firmware will be selected automagically. Therefore, it makes no sense to require the pathname of a specific firmware binary in the domain XML. But since it is not implemented do not really allow the path to be NULL. Only move code around to prepare it for further expansion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- docs/schemas/domaincommon.rng | 4 +++- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-)
The commit message looks good. Thanks! Laszlo

This is going to extend virDomainLoader enum. The reason is that once loader path is NULL its type makes no sense. However, since value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the following XML would be produced: <os> <loader type='rom'/> ... </os> To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would correspond to value of zero and then use post parse callback to set the default loader type to 'rom' if needed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- src/conf/domain_conf.c | 23 +++++++++++++++++++++-- src/conf/domain_conf.h | 3 ++- src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 1 + tests/domaincapsschemadata/full.xml | 1 + 5 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 053b2cb210..fbfdd57224 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1059,6 +1059,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST, VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, + "none", "rom", "pflash", ); @@ -4305,6 +4306,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def, } +static void +virDomainDefPostParseOs(virDomainDefPtr def) +{ + if (!def->os.loader) + return; + + if (def->os.loader->path && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) { + /* By default, loader is type of 'rom' */ + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM; + } +} + + static void virDomainDefPostParseMemtune(virDomainDefPtr def) { @@ -5480,6 +5495,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefPostParseMemory(def, data->parseFlags) < 0) return -1; + virDomainDefPostParseOs(def); + virDomainDefPostParseMemtune(def); if (virDomainDefRejectDuplicateControllers(def) < 0) @@ -18284,7 +18301,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node, if (type_str) { int type; - if ((type = virDomainLoaderTypeFromString(type_str)) < 0) { + if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) { virReportError(VIR_ERR_XML_DETAIL, _("unknown type value: %s"), type_str); return -1; @@ -27011,12 +27028,14 @@ virDomainLoaderDefFormat(virBufferPtr buf, if (loader->secure) virBufferAsprintf(buf, " secure='%s'", secure); - virBufferAsprintf(buf, " type='%s'", type); + if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE) + virBufferAsprintf(buf, " type='%s'", type); if (loader->path) virBufferEscapeString(buf, ">%s</loader>\n", loader->path); else virBufferAddLit(buf, "/>\n"); + if (loader->nvram || loader->templt) { virBufferAddLit(buf, "<nvram"); virBufferEscapeString(buf, " template='%s'", loader->templt); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c2dcc87ba1..ed5d00c399 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1954,7 +1954,8 @@ struct _virDomainBIOSDef { }; typedef enum { - VIR_DOMAIN_LOADER_TYPE_ROM = 0, + VIR_DOMAIN_LOADER_TYPE_NONE = 0, + VIR_DOMAIN_LOADER_TYPE_ROM, VIR_DOMAIN_LOADER_TYPE_PFLASH, VIR_DOMAIN_LOADER_TYPE_LAST diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a2ec7f26c..5e56447b76 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9967,6 +9967,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, } break; + case VIR_DOMAIN_LOADER_TYPE_NONE: case VIR_DOMAIN_LOADER_TYPE_LAST: /* nada */ break; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2b0d05f2db..e9b2b8453b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12213,6 +12213,7 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, goto cleanup; break; + case VIR_DOMAIN_LOADER_TYPE_NONE: case VIR_DOMAIN_LOADER_TYPE_LAST: break; } diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml index 28263466a4..74bc740e0d 100644 --- a/tests/domaincapsschemadata/full.xml +++ b/tests/domaincapsschemadata/full.xml @@ -10,6 +10,7 @@ <value>/foo/bar</value> <value>/tmp/my_path</value> <enum name='type'> + <value>none</value> <value>rom</value> <value>pflash</value> </enum> -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:15AM +0100, Michal Privoznik wrote:
This is going to extend virDomainLoader enum. The reason is that once loader path is NULL its type makes no sense. However, since value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the following XML would be produced:
<os> <loader type='rom'/> ... </os>
To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would correspond to value of zero and then use post parse callback to set the default loader type to 'rom' if needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> --- src/conf/domain_conf.c | 23 +++++++++++++++++++++-- src/conf/domain_conf.h | 3 ++- src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 1 + tests/domaincapsschemadata/full.xml | 1 + 5 files changed, 26 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

The idea is that using this attribute users enable libvirt to automagically select firmware image for their domain. For instance: <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> </os> <os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> </os> (The automagic of selecting firmware image will be described in later commits.) Accepted values are 'bios' and 'efi' to let libvirt select corresponding type of firmware. I know it is a good sign to introduce xml2xml test case when changing XML config parser but that will have to come later. Firmware auto selection is not enabled for any driver just yet so any xml2xml test would fail right away. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 22 +++++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 12 ++++++ src/libvirt_private.syms | 2 + 5 files changed, 103 insertions(+), 16 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb00c01d96..fedca0eedc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -128,7 +128,7 @@ <pre> ... -<os> +<os firmware='uefi'> <type>hvm</type> <loader readonly='yes' secure='no' type='rom'>/usr/lib/xen/boot/hvmloader</loader> <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> @@ -141,6 +141,26 @@ ...</pre> <dl> + <dt><code>firmware</code></dt> + <dd>The <code>firmware</code> attribute allows management + applications to automatically fill <code><loader/></code> + and <code><nvram/></code> elements and possibly enable + some features required by selected firmware. Accepted values are + <code>bios</code> and <code>efi</code>.<br/> + The selection process scans for files describing installed + firmware images in specified location and uses the most specific + one which fulfils domain requirements. The locations in order of + preference (from generic to most specific one) are: + <ul> + <li><code>/usr/share/qemu/firmware</code></li> + <li><code>/etc/qemu/firmware</code></li> + <li><code>$XDG_CONFIG_HOME/qemu/firmware</code></li> + </ul> + For more information refer to firmware metadata specification as + described in <code>docs/interop/firmware.json</code> in QEMU + repository. Regular users do not need to bother. + <span class="since">Since 5.2.0 (QEMU and KVM only)</span> + </dd> <dt><code>type</code></dt> <dd>The content of the <code>type</code> element specifies the type of operating system to be booted in the virtual machine. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c8d63c4912..87ba9daeda 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -256,6 +256,14 @@ </optional> <element name="os"> <interleave> + <optional> + <attribute name="firmware"> + <choice> + <value>bios</value> + <value>efi</value> + </choice> + </attribute> + </optional> <ref name="ostypehvm"/> <optional> <element name="loader"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbfdd57224..f92ebeb892 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1079,6 +1079,13 @@ VIR_ENUM_IMPL(virDomainHPTResizing, "required", ); +VIR_ENUM_IMPL(virDomainOsDefFirmware, + VIR_DOMAIN_OS_DEF_FIRMWARE_LAST, + "none", + "bios", + "efi", +); + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob); @@ -6608,14 +6615,23 @@ virDomainDefMemtuneValidate(const virDomainDef *def) static int -virDomainDefOSValidate(const virDomainDef *def) +virDomainDefOSValidate(const virDomainDef *def, + virDomainXMLOptionPtr xmlopt) { if (!def->os.loader) return 0; - if (!def->os.loader->path) { + if (def->os.firmware && + !(xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT)) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("no loader path specified")); + _("firmware auto selection not implemented for this driver")); + return -1; + } + + if (!def->os.loader->path && + def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("no loader path specified and firmware auto selection disabled")); return -1; } @@ -6624,7 +6640,8 @@ virDomainDefOSValidate(const virDomainDef *def) static int -virDomainDefValidateInternal(const virDomainDef *def) +virDomainDefValidateInternal(const virDomainDef *def, + virDomainXMLOptionPtr xmlopt) { if (virDomainDefCheckDuplicateDiskInfo(def) < 0) return -1; @@ -6661,7 +6678,7 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefMemtuneValidate(def) < 0) return -1; - if (virDomainDefOSValidate(def) < 0) + if (virDomainDefOSValidate(def, xmlopt) < 0) return -1; return 0; @@ -6712,7 +6729,7 @@ virDomainDefValidate(virDomainDefPtr def, &data) < 0) return -1; - if (virDomainDefValidateInternal(def) < 0) + if (virDomainDefValidateInternal(def, xmlopt) < 0) return -1; return 0; @@ -18271,19 +18288,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) static int virDomainLoaderDefParseXML(xmlNodePtr node, - virDomainLoaderDefPtr loader) + virDomainLoaderDefPtr loader, + bool fwAutoSelect) { VIR_AUTOFREE(char *) readonly_str = NULL; VIR_AUTOFREE(char *) secure_str = NULL; VIR_AUTOFREE(char *) type_str = NULL; - readonly_str = virXMLPropString(node, "readonly"); secure_str = virXMLPropString(node, "secure"); - type_str = virXMLPropString(node, "type"); - loader->path = (char *) xmlNodeGetContent(node); - if (STREQ_NULLABLE(loader->path, "")) - VIR_FREE(loader->path); + if (!fwAutoSelect) { + readonly_str = virXMLPropString(node, "readonly"); + type_str = virXMLPropString(node, "type"); + loader->path = (char *) xmlNodeGetContent(node); + if (STREQ_NULLABLE(loader->path, "")) + VIR_FREE(loader->path); + } if (readonly_str && (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { @@ -18678,6 +18698,7 @@ virDomainDefParseBootOptions(virDomainDefPtr def, def->os.type == VIR_DOMAIN_OSTYPE_XENPVH || def->os.type == VIR_DOMAIN_OSTYPE_HVM || def->os.type == VIR_DOMAIN_OSTYPE_UML) { + VIR_AUTOFREE(char *) firmware = NULL; xmlNodePtr loader_node; def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); @@ -18685,15 +18706,35 @@ virDomainDefParseBootOptions(virDomainDefPtr def, def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt); def->os.dtb = virXPathString("string(./os/dtb[1])", ctxt); def->os.root = virXPathString("string(./os/root[1])", ctxt); + + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && + (firmware = virXPathString("string(./os/@firmware)", ctxt))) { + int fw = virDomainOsDefFirmwareTypeFromString(firmware); + + if (fw <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown firmware value %s"), + firmware); + return -1; + } + + def->os.firmware = fw; + } + if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { + const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + if (VIR_ALLOC(def->os.loader) < 0) return -1; - if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) + if (virDomainLoaderDefParseXML(loader_node, + def->os.loader, + fwAutoSelect) < 0) return -1; def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + if (!fwAutoSelect) + def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); } } @@ -28082,7 +28123,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->os.bootloaderArgs); } - virBufferAddLit(buf, "<os>\n"); + virBufferAddLit(buf, "<os"); + if (def->os.firmware) + virBufferAsprintf(buf, " firmware='%s'", + virDomainOsDefFirmwareTypeToString(def->os.firmware)); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferAddLit(buf, "<type"); if (def->os.arch) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ed5d00c399..fe9d4fb81a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2005,10 +2005,21 @@ struct _virDomainOSEnv { char *value; }; +typedef enum { + VIR_DOMAIN_OS_DEF_FIRMWARE_NONE = 0, + VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS, + VIR_DOMAIN_OS_DEF_FIRMWARE_EFI, + + VIR_DOMAIN_OS_DEF_FIRMWARE_LAST +} virDomainOsDefFirmware; + +VIR_ENUM_DECL(virDomainOsDefFirmware); + typedef struct _virDomainOSDef virDomainOSDef; typedef virDomainOSDef *virDomainOSDefPtr; struct _virDomainOSDef { int type; + virDomainOsDefFirmware firmware; virArch arch; char *machine; size_t nBootDevs; @@ -2729,6 +2740,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4), VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5), VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER = (1 << 6), + VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT = (1 << 7), } virDomainDefFeatures; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d2a240fc7a..e8bb0d8bc4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -509,6 +509,8 @@ virDomainObjTaint; virDomainObjUpdateModificationImpact; virDomainObjWait; virDomainObjWaitUntil; +virDomainOsDefFirmwareTypeFromString; +virDomainOsDefFirmwareTypeToString; virDomainOSTypeFromString; virDomainOSTypeToString; virDomainParseMemory; -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:16AM +0100, Michal Privoznik wrote:
The idea is that using this attribute users enable libvirt to automagically select firmware image for their domain. For instance:
<os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/>
Ah, now I see why you need <loader> without a path specified. We need to be able to set the secure attribute.
</os>
<os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> </os>
(The automagic of selecting firmware image will be described in later commits.)
Accepted values are 'bios' and 'efi' to let libvirt select corresponding type of firmware.
I know it is a good sign to introduce xml2xml test case when changing XML config parser but that will have to come later. Firmware auto selection is not enabled for any driver just yet so any xml2xml test would fail right away.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 22 +++++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 12 ++++++ src/libvirt_private.syms | 2 + 5 files changed, 103 insertions(+), 16 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb00c01d96..fedca0eedc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -128,7 +128,7 @@
<pre> ... -<os> +<os firmware='uefi'> <type>hvm</type> <loader readonly='yes' secure='no' type='rom'>/usr/lib/xen/boot/hvmloader</loader> <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> @@ -141,6 +141,26 @@ ...</pre>
<dl> + <dt><code>firmware</code></dt> + <dd>The <code>firmware</code> attribute allows management + applications to automatically fill <code><loader/></code> + and <code><nvram/></code> elements and possibly enable + some features required by selected firmware. Accepted values are + <code>bios</code> and <code>efi</code>.<br/>
NB, this firmware attribute is something we'll want in the VMWare driver too. So the text from this point downwards should be explicitly noted as being specific to QEMU. Or maybe this text about search locations should just be in the drvqemu.html.in file, and linked from here ?
+ The selection process scans for files describing installed + firmware images in specified location and uses the most specific + one which fulfils domain requirements. The locations in order of + preference (from generic to most specific one) are: + <ul> + <li><code>/usr/share/qemu/firmware</code></li> + <li><code>/etc/qemu/firmware</code></li> + <li><code>$XDG_CONFIG_HOME/qemu/firmware</code></li> + </ul> + For more information refer to firmware metadata specification as + described in <code>docs/interop/firmware.json</code> in QEMU + repository. Regular users do not need to bother. + <span class="since">Since 5.2.0 (QEMU and KVM only)</span> + </dd> <dt><code>type</code></dt> <dd>The content of the <code>type</code> element specifies the type of operating system to be booted in the virtual machine.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 3/11/19 4:04 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:16AM +0100, Michal Privoznik wrote:
The idea is that using this attribute users enable libvirt to automagically select firmware image for their domain. For instance:
<os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/>
Ah, now I see why you need <loader> without a path specified. We need to be able to set the secure attribute.
</os>
<os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> </os>
(The automagic of selecting firmware image will be described in later commits.)
Accepted values are 'bios' and 'efi' to let libvirt select corresponding type of firmware.
I know it is a good sign to introduce xml2xml test case when changing XML config parser but that will have to come later. Firmware auto selection is not enabled for any driver just yet so any xml2xml test would fail right away.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 22 +++++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 12 ++++++ src/libvirt_private.syms | 2 + 5 files changed, 103 insertions(+), 16 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb00c01d96..fedca0eedc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -128,7 +128,7 @@
<pre> ... -<os> +<os firmware='uefi'> <type>hvm</type> <loader readonly='yes' secure='no' type='rom'>/usr/lib/xen/boot/hvmloader</loader> <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> @@ -141,6 +141,26 @@ ...</pre>
<dl> + <dt><code>firmware</code></dt> + <dd>The <code>firmware</code> attribute allows management + applications to automatically fill <code><loader/></code> + and <code><nvram/></code> elements and possibly enable + some features required by selected firmware. Accepted values are + <code>bios</code> and <code>efi</code>.<br/>
NB, this firmware attribute is something we'll want in the VMWare driver too. So the text from this point downwards should be explicitly noted as being specific to QEMU. Or maybe this text about search locations should just be in the drvqemu.html.in file, and linked from here ?
I'll go with the former. When implementing this for VMWare it can be moved to drvqemu. BTW why do we need this for VMWare?
+ The selection process scans for files describing installed + firmware images in specified location and uses the most specific + one which fulfils domain requirements. The locations in order of + preference (from generic to most specific one) are: + <ul> + <li><code>/usr/share/qemu/firmware</code></li> + <li><code>/etc/qemu/firmware</code></li> + <li><code>$XDG_CONFIG_HOME/qemu/firmware</code></li> + </ul> + For more information refer to firmware metadata specification as + described in <code>docs/interop/firmware.json</code> in QEMU + repository. Regular users do not need to bother. + <span class="since">Since 5.2.0 (QEMU and KVM only)</span> + </dd> <dt><code>type</code></dt> <dd>The content of the <code>type</code> element specifies the type of operating system to be booted in the virtual machine.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thanks, Michal

The firmware description is a JSON file which follows specification from qemu.git/docs/interop/firmware.json. The description file basically says: Firmware file X is {bios|uefi}, supports these targets and machine types, requires these features to be enabled on qemu cmd line and this is how you put it onto qemu cmd line. The firmware.json specification covers more (i.e. how to select the right firmware) but that will be covered and implemented in next commits. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/Makefile.inc.am | 2 + src/qemu/qemu_firmware.c | 901 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 40 ++ 3 files changed, 943 insertions(+) create mode 100644 src/qemu/qemu_firmware.c create mode 100644 src/qemu/qemu_firmware.h diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am index 0b2bc074c0..8d10e1085a 100644 --- a/src/qemu/Makefile.inc.am +++ b/src/qemu/Makefile.inc.am @@ -21,6 +21,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_cgroup.h \ qemu/qemu_extdevice.c \ qemu/qemu_extdevice.h \ + qemu/qemu_firmware.c \ + qemu/qemu_firmware.h \ qemu/qemu_hostdev.c \ qemu/qemu_hostdev.h \ qemu/qemu_hotplug.c \ diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c new file mode 100644 index 0000000000..8f718ee2a6 --- /dev/null +++ b/src/qemu/qemu_firmware.c @@ -0,0 +1,901 @@ +/* + * qemu_firmware.c: QEMU firmware + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "qemu_firmware.h" +#include "qemu_capabilities.h" +#include "virarch.h" +#include "virfile.h" +#include "virjson.h" +#include "virlog.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +VIR_LOG_INIT("qemu.qemu_firmware"); + + +typedef enum { + QEMU_FIRMWARE_OS_INTERFACE_NONE = 0, + QEMU_FIRMWARE_OS_INTERFACE_BIOS, + QEMU_FIRMWARE_OS_INTERFACE_OPENFIRMWARE, + QEMU_FIRMWARE_OS_INTERFACE_UBOOT, + QEMU_FIRMWARE_OS_INTERFACE_UEFI, + + QEMU_FIRMWARE_OS_INTERFACE_LAST +} qemuFirmwareOSInterface; + +VIR_ENUM_DECL(qemuFirmwareOSInterface); +VIR_ENUM_IMPL(qemuFirmwareOSInterface, + QEMU_FIRMWARE_OS_INTERFACE_LAST, + "", + "bios", + "openfirmware", + "uboot", + "uefi", +); + + +typedef struct _qemuFirmwareFlashFile qemuFirmwareFlashFile; +typedef qemuFirmwareFlashFile *qemuFirmwareFlashFilePtr; +struct _qemuFirmwareFlashFile { + char *filename; + char *format; +}; + + +typedef struct _qemuFirmwareMappingFlash qemuFirmwareMappingFlash; +typedef qemuFirmwareMappingFlash *qemuFirmwareMappingFlashPtr; +struct _qemuFirmwareMappingFlash { + qemuFirmwareFlashFile executable; + qemuFirmwareFlashFile nvram_template; +}; + + +typedef struct _qemuFirmwareMappingKernel qemuFirmwareMappingKernel; +typedef qemuFirmwareMappingKernel *qemuFirmwareMappingKernelPtr; +struct _qemuFirmwareMappingKernel { + char *filename; +}; + + +typedef struct _qemuFirmwareMappingMemory qemuFirmwareMappingMemory; +typedef qemuFirmwareMappingMemory *qemuFirmwareMappingMemoryPtr; +struct _qemuFirmwareMappingMemory { + char *filename; +}; + + +typedef enum { + QEMU_FIRMWARE_DEVICE_NONE = 0, + QEMU_FIRMWARE_DEVICE_FLASH, + QEMU_FIRMWARE_DEVICE_KERNEL, + QEMU_FIRMWARE_DEVICE_MEMORY, + + QEMU_FIRMWARE_DEVICE_LAST +} qemuFirmwareDevice; + +VIR_ENUM_DECL(qemuFirmwareDevice); +VIR_ENUM_IMPL(qemuFirmwareDevice, + QEMU_FIRMWARE_DEVICE_LAST, + "", + "flash", + "kernel", + "memory", +); + + +typedef struct _qemuFirmwareMapping qemuFirmwareMapping; +typedef qemuFirmwareMapping *qemuFirmwareMappingPtr; +struct _qemuFirmwareMapping { + qemuFirmwareDevice device; + + union { + qemuFirmwareMappingFlash flash; + qemuFirmwareMappingKernel kernel; + qemuFirmwareMappingMemory memory; + } data; +}; + + +typedef struct _qemuFirmwareTarget qemuFirmwareTarget; +typedef qemuFirmwareTarget *qemuFirmwareTargetPtr; +struct _qemuFirmwareTarget { + virArch architecture; + size_t nmachines; + char **machines; +}; + + +typedef enum { + QEMU_FIRMWARE_FEATURE_NONE = 0, + QEMU_FIRMWARE_FEATURE_ACPI_S3, + QEMU_FIRMWARE_FEATURE_ACPI_S4, + QEMU_FIRMWARE_FEATURE_AMD_SEV, + QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS, + QEMU_FIRMWARE_FEATURE_REQUIRES_SMM, + QEMU_FIRMWARE_FEATURE_SECURE_BOOT, + QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC, + QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC, + + QEMU_FIRMWARE_FEATURE_LAST +} qemuFirmwareFeature; + +VIR_ENUM_DECL(qemuFirmwareFeature); +VIR_ENUM_IMPL(qemuFirmwareFeature, + QEMU_FIRMWARE_FEATURE_LAST, + "", + "acpi-s3", + "acpi-s4", + "amd-sev", + "enrolled-keys", + "requires-smm", + "secure-boot", + "verbose-dynamic", + "verbose-static" +); + + +struct _qemuFirmware { + /* Description intentionally not parsed. */ + + size_t ninterfaces; + qemuFirmwareOSInterface *interfaces; + + qemuFirmwareMapping mapping; + + size_t ntargets; + qemuFirmwareTargetPtr *targets; + + size_t nfeatures; + qemuFirmwareFeature *features; + + /* Tags intentionally not parsed. */ +}; + + +static void +qemuFirmwareOSInterfaceFree(qemuFirmwareOSInterface *interfaces) +{ + VIR_FREE(interfaces); +} + + +VIR_DEFINE_AUTOPTR_FUNC(qemuFirmwareOSInterface, qemuFirmwareOSInterfaceFree); + + +static void +qemuFirmwareFlashFileFree(qemuFirmwareFlashFile flash) +{ + VIR_FREE(flash.filename); + VIR_FREE(flash.format); +} + + +static void +qemuFirmwareMappingFlashFree(qemuFirmwareMappingFlash flash) +{ + qemuFirmwareFlashFileFree(flash.executable); + qemuFirmwareFlashFileFree(flash.nvram_template); +} + + +static void +qemuFirmwareMappingKernelFree(qemuFirmwareMappingKernel kernel) +{ + VIR_FREE(kernel.filename); +} + + +static void +qemuFirmwareMappingMemoryFree(qemuFirmwareMappingMemory memory) +{ + VIR_FREE(memory.filename); +} + + +static void +qemuFirmwareMappingFree(qemuFirmwareMapping mapping) +{ + switch (mapping.device) { + case QEMU_FIRMWARE_DEVICE_FLASH: + qemuFirmwareMappingFlashFree(mapping.data.flash); + break; + case QEMU_FIRMWARE_DEVICE_KERNEL: + qemuFirmwareMappingKernelFree(mapping.data.kernel); + break; + case QEMU_FIRMWARE_DEVICE_MEMORY: + qemuFirmwareMappingMemoryFree(mapping.data.memory); + break; + case QEMU_FIRMWARE_DEVICE_NONE: + case QEMU_FIRMWARE_DEVICE_LAST: + break; + } +} + + +static void +qemuFirmwareTargetFree(qemuFirmwareTargetPtr target) +{ + if (!target) + return; + + virStringListFreeCount(target->machines, target->nmachines); + + VIR_FREE(target); +} + + +VIR_DEFINE_AUTOPTR_FUNC(qemuFirmwareTarget, qemuFirmwareTargetFree); + + +static void +qemuFirmwareFeatureFree(qemuFirmwareFeature *features) +{ + VIR_FREE(features); +} + + +VIR_DEFINE_AUTOPTR_FUNC(qemuFirmwareFeature, qemuFirmwareFeatureFree); + + +void +qemuFirmwareFree(qemuFirmwarePtr fw) +{ + size_t i; + + if (!fw) + return; + + qemuFirmwareOSInterfaceFree(fw->interfaces); + qemuFirmwareMappingFree(fw->mapping); + for (i = 0; i < fw->ntargets; i++) + qemuFirmwareTargetFree(fw->targets[i]); + VIR_FREE(fw->targets); + qemuFirmwareFeatureFree(fw->features); + + VIR_FREE(fw); +} + + +static int +qemuFirmwareInterfaceParse(const char *path, + virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + virJSONValuePtr interfacesJSON; + VIR_AUTOPTR(qemuFirmwareOSInterface) interfaces = NULL; + VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; + size_t ninterfaces; + size_t i; + + if (!(interfacesJSON = virJSONValueObjectGetArray(doc, "interface-types"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get interface-types from '%s'"), + path); + return -1; + } + + ninterfaces = virJSONValueArraySize(interfacesJSON); + + if (VIR_ALLOC_N(interfaces, ninterfaces) < 0) + return -1; + + for (i = 0; i < ninterfaces; i++) { + virJSONValuePtr item = virJSONValueArrayGet(interfacesJSON, i); + const char *tmpStr = virJSONValueGetString(item); + int tmp; + + if ((tmp = qemuFirmwareOSInterfaceTypeFromString(tmpStr)) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown interface type: '%s'"), + tmpStr); + return -1; + } + + virBufferAsprintf(&buf, " %s", tmpStr); + interfaces[i] = tmp; + } + + VIR_DEBUG("firmware description path '%s' supported interfaces: %s", + path, NULLSTR_MINUS(virBufferCurrentContent(&buf))); + + VIR_STEAL_PTR(fw->interfaces, interfaces); + fw->ninterfaces = ninterfaces; + return 0; +} + + +static int +qemuFirmwareFlashFileParse(const char *path, + virJSONValuePtr doc, + qemuFirmwareFlashFilePtr flash) +{ + const char *filename; + const char *format; + + if (!(filename = virJSONValueObjectGetString(doc, "filename"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'filename' in '%s'"), + path); + return -1; + } + + if (VIR_STRDUP(flash->filename, filename) < 0) + return -1; + + if (!(format = virJSONValueObjectGetString(doc, "format"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'format' in '%s'"), + path); + return -1; + } + + if (VIR_STRDUP(flash->format, format) < 0) + return -1; + + return 0; +} + + +static int +qemuFirmwareMappingFlashParse(const char *path, + virJSONValuePtr doc, + qemuFirmwareMappingFlashPtr flash) +{ + virJSONValuePtr executable; + virJSONValuePtr nvram_template; + + if (!(executable = virJSONValueObjectGet(doc, "executable"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'executable' in '%s'"), + path); + return -1; + } + + if (qemuFirmwareFlashFileParse(path, executable, &flash->executable) < 0) + return -1; + + if (!(nvram_template = virJSONValueObjectGet(doc, "nvram-template"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'nvram-template' in '%s'"), + path); + return -1; + } + + if (qemuFirmwareFlashFileParse(path, nvram_template, &flash->nvram_template) < 0) + return -1; + + return 0; +} + + +static int +qemuFirmwareMappingKernelParse(const char *path, + virJSONValuePtr doc, + qemuFirmwareMappingKernelPtr kernel) +{ + const char *filename; + + if (!(filename = virJSONValueObjectGetString(doc, "filename"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'filename' in '%s'"), + path); + } + + if (VIR_STRDUP(kernel->filename, filename) < 0) + return -1; + + return 0; +} + + +static int +qemuFirmwareMappingMemoryParse(const char *path, + virJSONValuePtr doc, + qemuFirmwareMappingMemoryPtr memory) +{ + const char *filename; + + if (!(filename = virJSONValueObjectGetString(doc, "filename"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'filename' in '%s'"), + path); + } + + if (VIR_STRDUP(memory->filename, filename) < 0) + return -1; + + return 0; +} + + +static int +qemuFirmwareMappingParse(const char *path, + virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + virJSONValuePtr mapping = virJSONValueObjectGet(doc, "mapping"); + const char *deviceStr; + int tmp; + + if (!(deviceStr = virJSONValueObjectGetString(mapping, "device"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing device type in '%s'"), + path); + return -1; + } + + if ((tmp = qemuFirmwareDeviceTypeFromString(deviceStr)) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown device type in '%s'"), + path); + return -1; + } + + fw->mapping.device = tmp; + + switch (fw->mapping.device) { + case QEMU_FIRMWARE_DEVICE_FLASH: + if (qemuFirmwareMappingFlashParse(path, mapping, &fw->mapping.data.flash) < 0) + return -1; + break; + case QEMU_FIRMWARE_DEVICE_KERNEL: + if (qemuFirmwareMappingKernelParse(path, mapping, &fw->mapping.data.kernel) < 0) + return -1; + break; + case QEMU_FIRMWARE_DEVICE_MEMORY: + if (qemuFirmwareMappingMemoryParse(path, mapping, &fw->mapping.data.memory) < 0) + return -1; + break; + + case QEMU_FIRMWARE_DEVICE_NONE: + case QEMU_FIRMWARE_DEVICE_LAST: + break; + } + + return 0; +} + + +static int +qemuFirmwareTargetParse(const char *path, + virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + virJSONValuePtr targetsJSON; + qemuFirmwareTargetPtr *targets = NULL; + size_t ntargets; + size_t i; + int ret = -1; + + if (!(targetsJSON = virJSONValueObjectGetArray(doc, "targets"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get targets from '%s'"), + path); + return -1; + } + + ntargets = virJSONValueArraySize(targetsJSON); + + if (VIR_ALLOC_N(targets, ntargets) < 0) + return -1; + + for (i = 0; i < ntargets; i++) { + virJSONValuePtr item = virJSONValueArrayGet(targetsJSON, i); + virJSONValuePtr machines; + VIR_AUTOPTR(qemuFirmwareTarget) t = NULL; + const char *architectureStr = NULL; + size_t nmachines; + size_t j; + + if (VIR_ALLOC(t) < 0) + goto cleanup; + + if (!(architectureStr = virJSONValueObjectGetString(item, "architecture"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'architecture' in '%s'"), + path); + goto cleanup; + } + + if ((t->architecture = virQEMUCapsArchFromString(architectureStr)) == VIR_ARCH_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown architecture '%s'"), + architectureStr); + goto cleanup; + } + + if (!(machines = virJSONValueObjectGetArray(item, "machines"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing 'machines' in '%s'"), + path); + goto cleanup; + } + + nmachines = virJSONValueArraySize(machines); + + if (VIR_ALLOC_N(t->machines, nmachines) < 0) + goto cleanup; + + for (j = 0; j < nmachines; j++) { + virJSONValuePtr machine = virJSONValueArrayGet(machines, j); + VIR_AUTOFREE(char *) machineStr = NULL; + + if (VIR_STRDUP(machineStr, virJSONValueGetString(machine)) < 0) + goto cleanup; + + VIR_APPEND_ELEMENT_INPLACE(t->machines, t->nmachines, machineStr); + } + + VIR_STEAL_PTR(targets[i], t); + } + + VIR_STEAL_PTR(fw->targets, targets); + fw->ntargets = ntargets; + ntargets = 0; + ret = 0; + + cleanup: + for (i = 0; i < ntargets; i++) + qemuFirmwareTargetFree(targets[i]); + VIR_FREE(targets); + return ret; +} + + +static int +qemuFirmwareFeatureParse(const char *path, + virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + virJSONValuePtr featuresJSON; + VIR_AUTOPTR(qemuFirmwareFeature) features = NULL; + size_t nfeatures; + size_t i; + + if (!(featuresJSON = virJSONValueObjectGetArray(doc, "features"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get features from '%s'"), + path); + return -1; + } + + nfeatures = virJSONValueArraySize(featuresJSON); + + if (VIR_ALLOC_N(features, nfeatures) < 0) + return -1; + + fw->nfeatures = nfeatures; + + for (i = 0; i < nfeatures; i++) { + virJSONValuePtr item = virJSONValueArrayGet(featuresJSON, i); + const char *tmpStr = virJSONValueGetString(item); + int tmp; + + if ((tmp = qemuFirmwareFeatureTypeFromString(tmpStr)) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown feature %s"), + tmpStr); + return -1; + } + + features[i] = tmp; + } + + VIR_STEAL_PTR(fw->features, features); + fw->nfeatures = nfeatures; + return 0; +} + + +/* 1MiB should be enough for everybody (TM) */ +#define DOCUMENT_SIZE (1024 * 1024) + +qemuFirmwarePtr +qemuFirmwareParse(const char *path) +{ + VIR_AUTOFREE(char *) cont = NULL; + VIR_AUTOPTR(virJSONValue) doc = NULL; + VIR_AUTOPTR(qemuFirmware) fw = NULL; + qemuFirmwarePtr ret = NULL; + + if (virFileReadAll(path, DOCUMENT_SIZE, &cont) < 0) + return NULL; + + if (!(doc = virJSONValueFromString(cont))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to parse json file '%s'"), + path); + return NULL; + } + + if (VIR_ALLOC(fw) < 0) + return NULL; + + if (qemuFirmwareInterfaceParse(path, doc, fw) < 0) + return NULL; + + if (qemuFirmwareMappingParse(path, doc, fw) < 0) + return NULL; + + if (qemuFirmwareTargetParse(path, doc, fw) < 0) + return NULL; + + if (qemuFirmwareFeatureParse(path, doc, fw) < 0) + return NULL; + + VIR_STEAL_PTR(ret, fw); + return ret; +} + + +static int +qemuFirmwareInterfaceFormat(virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + VIR_AUTOPTR(virJSONValue) interfacesJSON = NULL; + size_t i; + + if (!(interfacesJSON = virJSONValueNewArray())) + return -1; + + for (i = 0; i < fw->ninterfaces; i++) { + if (virJSONValueArrayAppendString(interfacesJSON, + qemuFirmwareOSInterfaceTypeToString(fw->interfaces[i])) < 0) + return -1; + } + + if (virJSONValueObjectAppend(doc, + "interface-types", + interfacesJSON) < 0) + return -1; + + interfacesJSON = NULL; + return 0; +} + + +static virJSONValuePtr +qemuFirmwareFlashFileFormat(qemuFirmwareFlashFile flash) +{ + VIR_AUTOPTR(virJSONValue) json = NULL; + virJSONValuePtr ret; + + if (!(json = virJSONValueNewObject())) + return NULL; + + if (virJSONValueObjectAppendString(json, + "filename", + flash.filename) < 0) + return NULL; + + if (virJSONValueObjectAppendString(json, + "format", + flash.format) < 0) + return NULL; + + VIR_STEAL_PTR(ret, json); + return ret; +} + + +static int +qemuFirmwareMappingFlashFormat(virJSONValuePtr mapping, + qemuFirmwareMappingFlashPtr flash) +{ + VIR_AUTOPTR(virJSONValue) executable = NULL; + VIR_AUTOPTR(virJSONValue) nvram_template = NULL; + + if (!(executable = qemuFirmwareFlashFileFormat(flash->executable))) + return -1; + + if (!(nvram_template = qemuFirmwareFlashFileFormat(flash->nvram_template))) + return -1; + + if (virJSONValueObjectAppend(mapping, + "executable", + executable) < 0) + return -1; + + executable = NULL; + + if (virJSONValueObjectAppend(mapping, + "nvram-template", + nvram_template) < 0) + return -1; + + nvram_template = NULL; + + return 0; +} + + +static int +qemuFirmwareMappingKernelFormat(virJSONValuePtr mapping, + qemuFirmwareMappingKernelPtr kernel) +{ + if (virJSONValueObjectAppendString(mapping, + "filename", + kernel->filename) < 0) + return -1; + + return 0; +} + + +static int +qemuFirmwareMappingMemoryFormat(virJSONValuePtr mapping, + qemuFirmwareMappingMemoryPtr memory) +{ + if (virJSONValueObjectAppendString(mapping, + "filename", + memory->filename) < 0) + return -1; + + return 0; +} + + +static int +qemuFirmwareMappingFormat(virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + VIR_AUTOPTR(virJSONValue) mapping = NULL; + + if (!(mapping = virJSONValueNewObject())) + return -1; + + if (virJSONValueObjectAppendString(mapping, + "device", + qemuFirmwareDeviceTypeToString(fw->mapping.device)) < 0) + return -1; + + switch (fw->mapping.device) { + case QEMU_FIRMWARE_DEVICE_FLASH: + if (qemuFirmwareMappingFlashFormat(mapping, &fw->mapping.data.flash) < 0) + return -1; + break; + case QEMU_FIRMWARE_DEVICE_KERNEL: + if (qemuFirmwareMappingKernelFormat(mapping, &fw->mapping.data.kernel) < 0) + return -1; + break; + case QEMU_FIRMWARE_DEVICE_MEMORY: + if (qemuFirmwareMappingMemoryFormat(mapping, &fw->mapping.data.memory) < 0) + return -1; + break; + + case QEMU_FIRMWARE_DEVICE_NONE: + case QEMU_FIRMWARE_DEVICE_LAST: + break; + } + + if (virJSONValueObjectAppend(doc, "mapping", mapping) < 0) + return -1; + + mapping = NULL; + return 0; +} + + +static int +qemuFirmwareTargetFormat(virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + VIR_AUTOPTR(virJSONValue) targetsJSON = NULL; + size_t i; + + if (!(targetsJSON = virJSONValueNewArray())) + return -1; + + for (i = 0; i < fw->ntargets; i++) { + qemuFirmwareTargetPtr t = fw->targets[i]; + VIR_AUTOPTR(virJSONValue) target = NULL; + VIR_AUTOPTR(virJSONValue) machines = NULL; + size_t j; + + if (!(target = virJSONValueNewObject())) + return -1; + + if (virJSONValueObjectAppendString(target, + "architecture", + virQEMUCapsArchToString(t->architecture)) < 0) + return -1; + + if (!(machines = virJSONValueNewArray())) + return -1; + + for (j = 0; j < t->nmachines; j++) { + if (virJSONValueArrayAppendString(machines, + t->machines[j]) < 0) + return -1; + } + + if (virJSONValueObjectAppend(target, "machines", machines) < 0) + return -1; + + machines = NULL; + + if (virJSONValueArrayAppend(targetsJSON, target) < 0) + return -1; + + target = NULL; + } + + if (virJSONValueObjectAppend(doc, "targets", targetsJSON) < 0) + return -1; + + targetsJSON = NULL; + return 0; +} + + +static int +qemuFirmwareFeatureFormat(virJSONValuePtr doc, + qemuFirmwarePtr fw) +{ + VIR_AUTOPTR(virJSONValue) featuresJSON = NULL; + size_t i; + + if (!(featuresJSON = virJSONValueNewArray())) + return -1; + + for (i = 0; i < fw->nfeatures; i++) { + if (virJSONValueArrayAppendString(featuresJSON, + qemuFirmwareFeatureTypeToString(fw->features[i])) < 0) + return -1; + } + + if (virJSONValueObjectAppend(doc, + "features", + featuresJSON) < 0) + return -1; + + featuresJSON = NULL; + return 0; +} + + +char * +qemuFirmwareFormat(qemuFirmwarePtr fw) +{ + VIR_AUTOPTR(virJSONValue) doc = NULL; + + if (!fw) + return NULL; + + if (!(doc = virJSONValueNewObject())) + return NULL; + + if (qemuFirmwareInterfaceFormat(doc, fw) < 0) + return NULL; + + if (qemuFirmwareMappingFormat(doc, fw) < 0) + return NULL; + + if (qemuFirmwareTargetFormat(doc, fw) < 0) + return NULL; + + if (qemuFirmwareFeatureFormat(doc, fw) < 0) + return NULL; + + return virJSONValueToString(doc, true); +} diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h new file mode 100644 index 0000000000..952615d42b --- /dev/null +++ b/src/qemu/qemu_firmware.h @@ -0,0 +1,40 @@ +/* + * qemu_firmware.h: QEMU firmware + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef LIBVIRT_QEMU_FIRMWARE_H +# define LIBVIRT_QEMU_FIRMWARE_H + +# include "viralloc.h" + +typedef struct _qemuFirmware qemuFirmware; +typedef qemuFirmware *qemuFirmwarePtr; + +void +qemuFirmwareFree(qemuFirmwarePtr fw); + +VIR_DEFINE_AUTOPTR_FUNC(qemuFirmware, qemuFirmwareFree); + +qemuFirmwarePtr +qemuFirmwareParse(const char *path); + +char * +qemuFirmwareFormat(qemuFirmwarePtr fw); + +#endif /* LIBVIRT_QEMU_FIRMWARE_H */ -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:17AM +0100, Michal Privoznik wrote:
The firmware description is a JSON file which follows specification from qemu.git/docs/interop/firmware.json. The description file basically says: Firmware file X is {bios|uefi}, supports these targets and machine types, requires these features to be enabled on qemu cmd line and this is how you put it onto qemu cmd line.
The firmware.json specification covers more (i.e. how to select the right firmware) but that will be covered and implemented in next commits.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/Makefile.inc.am | 2 + src/qemu/qemu_firmware.c | 901 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 40 ++ 3 files changed, 943 insertions(+) create mode 100644 src/qemu/qemu_firmware.c create mode 100644 src/qemu/qemu_firmware.h
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Test firmware description parsing so far. The test files come from three locations: 1) ovmf-sb-keys.json and ovmf-sb.json come from OVMF package from RHEL-7 (with slight name change to reflect their features in filename too), 2) bios.json and aavmf.json come form comments from firmware.json from qemu's git (3a0adfc9bf), 3) ovmf.json is then copied from ovmf-sb.json and stripped of SECURE_BOOT and REQUIRES_SMM flags (plus OVMF path change). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 +++ tests/qemufirmwaredata/aavmf.json | 35 +++++++++++ tests/qemufirmwaredata/bios.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf-sb-keys.json | 36 ++++++++++++ tests/qemufirmwaredata/ovmf-sb.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf.json | 33 +++++++++++ tests/qemufirmwaretest.c | 75 ++++++++++++++++++++++++ 7 files changed, 258 insertions(+) create mode 100644 tests/qemufirmwaredata/aavmf.json create mode 100644 tests/qemufirmwaredata/bios.json create mode 100644 tests/qemufirmwaredata/ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/ovmf-sb.json create mode 100644 tests/qemufirmwaredata/ovmf.json create mode 100644 tests/qemufirmwaretest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 72f0420bab..b3449fa96b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -132,6 +132,7 @@ EXTRA_DIST = \ qemuxml2xmloutdata \ qemustatusxml2xmldata \ qemumemlockdata \ + qemufirmwaredata \ secretxml2xmlin \ securityselinuxhelperdata \ securityselinuxlabeldata \ @@ -292,6 +293,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \ qemublocktest \ qemumigparamstest \ qemusecuritytest \ + qemufirmwaretest \ $(NULL) test_helpers += qemucapsprobe test_libraries += libqemumonitortestutils.la \ @@ -700,6 +702,12 @@ qemusecuritytest_SOURCES = \ testutilsqemu.h testutilsqemu.c qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) +qemufirmwaretest_SOURCES = \ + qemufirmwaretest.c \ + testutils.h testutils.c \ + $(NULL) +qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS) + else ! WITH_QEMU EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ domainsnapshotxml2xmltest.c \ @@ -713,6 +721,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ qemumigparamstest.c \ qemusecuritytest.c qemusecuritytest.h \ qemusecuritymock.c \ + qemufirmwaretest.c \ $(QEMUMONITORTESTUTILS_SOURCES) endif ! WITH_QEMU diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/aavmf.json new file mode 100644 index 0000000000..114d1475a2 --- /dev/null +++ b/tests/qemufirmwaredata/aavmf.json @@ -0,0 +1,35 @@ +{ + "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" + } + }, + "targets": [ + { + "architecture": "aarch64", + "machines": [ + "virt-*" + ] + } + ], + "features": [ + + ], + "tags": [ + "-a AARCH64", + "-p ArmVirtPkg/ArmVirtQemu.dsc", + "-t GCC48", + "-b DEBUG", + "-D DEBUG_PRINT_ERROR_LEVEL=0x80000000" + ] +} diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/bios.json new file mode 100644 index 0000000000..137ff70779 --- /dev/null +++ b/tests/qemufirmwaredata/bios.json @@ -0,0 +1,35 @@ +{ + "description": "SeaBIOS", + "interface-types": [ + "bios" + ], + "mapping": { + "device": "memory", + "filename": "/usr/share/seabios/bios-256k.bin" + }, + "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" + ] +} diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/ovmf-sb-keys.json new file mode 100644 index 0000000000..c804ac1038 --- /dev/null +++ b/tests/qemufirmwaredata/ovmf-sb-keys.json @@ -0,0 +1,36 @@ +{ + "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" + } + }, + "targets": [ + { + "architecture": "x86_64", + "machines": [ + "pc-q35-*" + ] + } + ], + "features": [ + "acpi-s3", + "amd-sev", + "enrolled-keys", + "requires-smm", + "secure-boot", + "verbose-dynamic" + ], + "tags": [ + + ] +} diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/ovmf-sb.json new file mode 100644 index 0000000000..5e8a94ae78 --- /dev/null +++ b/tests/qemufirmwaredata/ovmf-sb.json @@ -0,0 +1,35 @@ +{ + "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" + } + }, + "targets": [ + { + "architecture": "x86_64", + "machines": [ + "pc-q35-*" + ] + } + ], + "features": [ + "acpi-s3", + "amd-sev", + "requires-smm", + "secure-boot", + "verbose-dynamic" + ], + "tags": [ + + ] +} diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/ovmf.json new file mode 100644 index 0000000000..9d53094778 --- /dev/null +++ b/tests/qemufirmwaredata/ovmf.json @@ -0,0 +1,33 @@ +{ + "description": "OVMF with SB+SMM, empty varstore", + "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" + } + }, + "targets": [ + { + "architecture": "x86_64", + "machines": [ + "pc-q35-*" + ] + } + ], + "features": [ + "acpi-s3", + "amd-sev", + "verbose-dynamic" + ], + "tags": [ + + ] +} diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c new file mode 100644 index 0000000000..176cf0920d --- /dev/null +++ b/tests/qemufirmwaretest.c @@ -0,0 +1,75 @@ +#include <config.h> + +#include "testutils.h" +#include "qemu/qemu_firmware.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +/* A very basic test. Parse given JSON firmware description into + * an internal structure, format it back and compare with the + * contents of the file (minus some keys that are not parsed). + */ +static int +testParseFormatFW(const void *opaque) +{ + const char *filename = opaque; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOPTR(qemuFirmware) fw = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOPTR(virJSONValue) json = NULL; + VIR_AUTOFREE(char *) expected = NULL; + VIR_AUTOFREE(char *) actual = NULL; + + if (virAsprintf(&path, "%s/qemufirmwaredata/%s", + abs_srcdir, filename) < 0) + return -1; + + if (!(fw = qemuFirmwareParse(path))) + return -1; + + if (virFileReadAll(path, + 1024 * 1024, /* 1MiB */ + &buf) < 0) + return -1; + + if (!(json = virJSONValueFromString(buf))) + return -1; + + /* Description and tags are not parsed. */ + if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 || + virJSONValueObjectRemoveKey(json, "tags", NULL) < 0) + return -1; + + if (!(expected = virJSONValueToString(json, true))) + return -1; + + if (!(actual = qemuFirmwareFormat(fw))) + return -1; + + return virTestCompareToString(expected, actual); +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_PARSE_TEST(filename) \ + do { \ + if (virTestRun("QEMU FW " filename, \ + testParseFormatFW, filename) < 0) \ + ret = -1; \ + } while (0) + + DO_PARSE_TEST("bios.json"); + DO_PARSE_TEST("ovmf-sb-keys.json"); + DO_PARSE_TEST("ovmf-sb.json"); + DO_PARSE_TEST("ovmf.json"); + DO_PARSE_TEST("aavmf.json"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + + +VIR_TEST_MAIN(mymain); -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:18AM +0100, Michal Privoznik wrote:
Test firmware description parsing so far.
The test files come from three locations: 1) ovmf-sb-keys.json and ovmf-sb.json come from OVMF package from RHEL-7 (with slight name change to reflect their features in filename too),
2) bios.json and aavmf.json come form comments from
s/form/from/
firmware.json from qemu's git (3a0adfc9bf),
3) ovmf.json is then copied from ovmf-sb.json and stripped of SECURE_BOOT and REQUIRES_SMM flags (plus OVMF path change).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 +++ tests/qemufirmwaredata/aavmf.json | 35 +++++++++++ tests/qemufirmwaredata/bios.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf-sb-keys.json | 36 ++++++++++++ tests/qemufirmwaredata/ovmf-sb.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf.json | 33 +++++++++++ tests/qemufirmwaretest.c | 75 ++++++++++++++++++++++++ 7 files changed, 258 insertions(+) create mode 100644 tests/qemufirmwaredata/aavmf.json create mode 100644 tests/qemufirmwaredata/bios.json create mode 100644 tests/qemufirmwaredata/ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/ovmf-sb.json create mode 100644 tests/qemufirmwaredata/ovmf.json create mode 100644 tests/qemufirmwaretest.c
[snip]
+static int +testParseFormatFW(const void *opaque) +{ + const char *filename = opaque; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOPTR(qemuFirmware) fw = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOPTR(virJSONValue) json = NULL; + VIR_AUTOFREE(char *) expected = NULL; + VIR_AUTOFREE(char *) actual = NULL; + + if (virAsprintf(&path, "%s/qemufirmwaredata/%s", + abs_srcdir, filename) < 0) + return -1; + + if (!(fw = qemuFirmwareParse(path))) + return -1; + + if (virFileReadAll(path, + 1024 * 1024, /* 1MiB */ + &buf) < 0) + return -1; + + if (!(json = virJSONValueFromString(buf))) + return -1; + + /* Description and tags are not parsed. */ + if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 || + virJSONValueObjectRemoveKey(json, "tags", NULL) < 0)
Any reason why you didn't parse them. Feels like it would have been easy enough to parse those 2 fields & avoid the special case ?
+ return -1; + + if (!(expected = virJSONValueToString(json, true))) + return -1; + + if (!(actual = qemuFirmwareFormat(fw))) + return -1; + + return virTestCompareToString(expected, actual); +}
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 3/11/19 4:08 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:18AM +0100, Michal Privoznik wrote:
Test firmware description parsing so far.
The test files come from three locations: 1) ovmf-sb-keys.json and ovmf-sb.json come from OVMF package from RHEL-7 (with slight name change to reflect their features in filename too),
2) bios.json and aavmf.json come form comments from
s/form/from/
firmware.json from qemu's git (3a0adfc9bf),
3) ovmf.json is then copied from ovmf-sb.json and stripped of SECURE_BOOT and REQUIRES_SMM flags (plus OVMF path change).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 +++ tests/qemufirmwaredata/aavmf.json | 35 +++++++++++ tests/qemufirmwaredata/bios.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf-sb-keys.json | 36 ++++++++++++ tests/qemufirmwaredata/ovmf-sb.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf.json | 33 +++++++++++ tests/qemufirmwaretest.c | 75 ++++++++++++++++++++++++ 7 files changed, 258 insertions(+) create mode 100644 tests/qemufirmwaredata/aavmf.json create mode 100644 tests/qemufirmwaredata/bios.json create mode 100644 tests/qemufirmwaredata/ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/ovmf-sb.json create mode 100644 tests/qemufirmwaredata/ovmf.json create mode 100644 tests/qemufirmwaretest.c
[snip]
+static int +testParseFormatFW(const void *opaque) +{ + const char *filename = opaque; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOPTR(qemuFirmware) fw = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOPTR(virJSONValue) json = NULL; + VIR_AUTOFREE(char *) expected = NULL; + VIR_AUTOFREE(char *) actual = NULL; + + if (virAsprintf(&path, "%s/qemufirmwaredata/%s", + abs_srcdir, filename) < 0) + return -1; + + if (!(fw = qemuFirmwareParse(path))) + return -1; + + if (virFileReadAll(path, + 1024 * 1024, /* 1MiB */ + &buf) < 0) + return -1; + + if (!(json = virJSONValueFromString(buf))) + return -1; + + /* Description and tags are not parsed. */ + if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 || + virJSONValueObjectRemoveKey(json, "tags", NULL) < 0)
Any reason why you didn't parse them. Feels like it would have been easy enough to parse those 2 fields & avoid the special case ?
We don't need them for anything and firmware.json says that: Management software may or may not display @description. @tags serves development and debugging purposes only, and management software shall explicitly ignore it. But I can save that for a follow up patch, if we want to parse them.
+ return -1; + + if (!(expected = virJSONValueToString(json, true))) + return -1; + + if (!(actual = qemuFirmwareFormat(fw))) + return -1; + + return virTestCompareToString(expected, actual); +}
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thanks, Michal

On 03/07/19 10:29, Michal Privoznik wrote:
Test firmware description parsing so far.
The test files come from three locations: 1) ovmf-sb-keys.json and ovmf-sb.json come from OVMF package from RHEL-7 (with slight name change to reflect their features in filename too),
2) bios.json and aavmf.json come form comments from
(a) s/form/from/ (b) s/comments/example JSON documents/ -- I'm emphasizing this because QEMU itself is pretty strict about calling those comment sections "Examples".
firmware.json from qemu's git (3a0adfc9bf),
3) ovmf.json is then copied from ovmf-sb.json and stripped of SECURE_BOOT and REQUIRES_SMM flags (plus OVMF path change).
The way you derived "ovmf.json" from "ovmf-sb.json" is not bad, but it should be improved. I'll make comments on the file itself, below. (c) Please don't forget to update this section of the commit message in sync.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 +++ tests/qemufirmwaredata/aavmf.json | 35 +++++++++++ tests/qemufirmwaredata/bios.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf-sb-keys.json | 36 ++++++++++++ tests/qemufirmwaredata/ovmf-sb.json | 35 +++++++++++ tests/qemufirmwaredata/ovmf.json | 33 +++++++++++ tests/qemufirmwaretest.c | 75 ++++++++++++++++++++++++ 7 files changed, 258 insertions(+) create mode 100644 tests/qemufirmwaredata/aavmf.json create mode 100644 tests/qemufirmwaredata/bios.json create mode 100644 tests/qemufirmwaredata/ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/ovmf-sb.json create mode 100644 tests/qemufirmwaredata/ovmf.json create mode 100644 tests/qemufirmwaretest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index 72f0420bab..b3449fa96b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -132,6 +132,7 @@ EXTRA_DIST = \ qemuxml2xmloutdata \ qemustatusxml2xmldata \ qemumemlockdata \ + qemufirmwaredata \ secretxml2xmlin \ securityselinuxhelperdata \ securityselinuxlabeldata \ @@ -292,6 +293,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \ qemublocktest \ qemumigparamstest \ qemusecuritytest \ + qemufirmwaretest \ $(NULL) test_helpers += qemucapsprobe test_libraries += libqemumonitortestutils.la \ @@ -700,6 +702,12 @@ qemusecuritytest_SOURCES = \ testutilsqemu.h testutilsqemu.c qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS)
+qemufirmwaretest_SOURCES = \ + qemufirmwaretest.c \ + testutils.h testutils.c \ + $(NULL) +qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS) + else ! WITH_QEMU EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ domainsnapshotxml2xmltest.c \ @@ -713,6 +721,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ qemumigparamstest.c \ qemusecuritytest.c qemusecuritytest.h \ qemusecuritymock.c \ + qemufirmwaretest.c \ $(QEMUMONITORTESTUTILS_SOURCES) endif ! WITH_QEMU
diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/aavmf.json new file mode 100644 index 0000000000..114d1475a2 --- /dev/null +++ b/tests/qemufirmwaredata/aavmf.json @@ -0,0 +1,35 @@ +{ + "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" + } + }, + "targets": [ + { + "architecture": "aarch64", + "machines": [ + "virt-*" + ] + } + ], + "features": [ + + ], + "tags": [ + "-a AARCH64", + "-p ArmVirtPkg/ArmVirtQemu.dsc", + "-t GCC48", + "-b DEBUG", + "-D DEBUG_PRINT_ERROR_LEVEL=0x80000000" + ] +} diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/bios.json new file mode 100644 index 0000000000..137ff70779 --- /dev/null +++ b/tests/qemufirmwaredata/bios.json @@ -0,0 +1,35 @@ +{ + "description": "SeaBIOS", + "interface-types": [ + "bios" + ], + "mapping": { + "device": "memory", + "filename": "/usr/share/seabios/bios-256k.bin" + }, + "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" + ] +} diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/ovmf-sb-keys.json new file mode 100644 index 0000000000..c804ac1038 --- /dev/null +++ b/tests/qemufirmwaredata/ovmf-sb-keys.json @@ -0,0 +1,36 @@ +{ + "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" + } + }, + "targets": [ + { + "architecture": "x86_64", + "machines": [ + "pc-q35-*" + ] + } + ], + "features": [ + "acpi-s3", + "amd-sev", + "enrolled-keys", + "requires-smm", + "secure-boot", + "verbose-dynamic" + ], + "tags": [ + + ] +} diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/ovmf-sb.json new file mode 100644 index 0000000000..5e8a94ae78 --- /dev/null +++ b/tests/qemufirmwaredata/ovmf-sb.json @@ -0,0 +1,35 @@ +{ + "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" + } + }, + "targets": [ + { + "architecture": "x86_64", + "machines": [ + "pc-q35-*" + ] + } + ], + "features": [ + "acpi-s3", + "amd-sev", + "requires-smm", + "secure-boot", + "verbose-dynamic" + ], + "tags": [ + + ] +} diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/ovmf.json new file mode 100644 index 0000000000..9d53094778 --- /dev/null +++ b/tests/qemufirmwaredata/ovmf.json @@ -0,0 +1,33 @@ +{ + "description": "OVMF with SB+SMM, empty varstore",
"SB", "SMM", and "empty varstore" are each either wrong or useless to mention here. (d) So I suggest retrofitting the AAVMF description instead -- just say "UEFI firmware for x86_64 virtual machines".
+ "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" + } + }, + "targets": [ + { + "architecture": "x86_64", + "machines": [ + "pc-q35-*"
(e) Because this build does not include the SMM driver stack, we should permit "pc-i440fx-*", in addition to "pc-q35-*". (Points (d) and (e) are not really relevant for the self-test in libvirt, it's just that the examples should be as "meaningful" as possible.) Other than that, thanks for addressing my comments. With the above updates: Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
+ ] + } + ], + "features": [ + "acpi-s3", + "amd-sev", + "verbose-dynamic" + ], + "tags": [ + + ] +} diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c new file mode 100644 index 0000000000..176cf0920d --- /dev/null +++ b/tests/qemufirmwaretest.c @@ -0,0 +1,75 @@ +#include <config.h> + +#include "testutils.h" +#include "qemu/qemu_firmware.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +/* A very basic test. Parse given JSON firmware description into + * an internal structure, format it back and compare with the + * contents of the file (minus some keys that are not parsed). + */ +static int +testParseFormatFW(const void *opaque) +{ + const char *filename = opaque; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOPTR(qemuFirmware) fw = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOPTR(virJSONValue) json = NULL; + VIR_AUTOFREE(char *) expected = NULL; + VIR_AUTOFREE(char *) actual = NULL; + + if (virAsprintf(&path, "%s/qemufirmwaredata/%s", + abs_srcdir, filename) < 0) + return -1; + + if (!(fw = qemuFirmwareParse(path))) + return -1; + + if (virFileReadAll(path, + 1024 * 1024, /* 1MiB */ + &buf) < 0) + return -1; + + if (!(json = virJSONValueFromString(buf))) + return -1; + + /* Description and tags are not parsed. */ + if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 || + virJSONValueObjectRemoveKey(json, "tags", NULL) < 0) + return -1; + + if (!(expected = virJSONValueToString(json, true))) + return -1; + + if (!(actual = qemuFirmwareFormat(fw))) + return -1; + + return virTestCompareToString(expected, actual); +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_PARSE_TEST(filename) \ + do { \ + if (virTestRun("QEMU FW " filename, \ + testParseFormatFW, filename) < 0) \ + ret = -1; \ + } while (0) + + DO_PARSE_TEST("bios.json"); + DO_PARSE_TEST("ovmf-sb-keys.json"); + DO_PARSE_TEST("ovmf-sb.json"); + DO_PARSE_TEST("ovmf.json"); + DO_PARSE_TEST("aavmf.json"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + + +VIR_TEST_MAIN(mymain);

Implementation for yet another part of firmware description specification. This one covers selecting which files to parse. There are three locations from which description files can be loaded. In order of preference, from most generic to most specific these are: /usr/share/qemu/firmware /etc/qemu/firmware $XDG_CONFIG_HOME/qemu/firmware If a file is found in two or more locations then the most specific one is used. Moreover, if file is empty then it means it is overriding some generic description and disabling it. Again, this is described in more details and with nice examples in firmware.json specification (qemu commit 3a0adfc9bf). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 134 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 137 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 8f718ee2a6..a818f60c91 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -21,9 +21,11 @@ #include <config.h> #include "qemu_firmware.h" +#include "configmake.h" #include "qemu_capabilities.h" #include "virarch.h" #include "virfile.h" +#include "virhash.h" #include "virjson.h" #include "virlog.h" #include "virstring.h" @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw) return virJSONValueToString(doc, true); } + + +static int +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir) +{ + DIR *dirp; + struct dirent *ent = NULL; + int rc; + int ret = -1; + + if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) + return -1; + + if (rc == 0) + return 0; + + while ((rc = virDirRead(dirp, &ent, dir)) > 0) { + VIR_AUTOFREE(char *) filename = NULL; + VIR_AUTOFREE(char *) path = NULL; + + if (ent->d_type != DT_REG && ent->d_type != DT_LNK) + continue; + + if (STRPREFIX(ent->d_name, ".")) + continue; + + if (VIR_STRDUP(filename, ent->d_name) < 0) + goto cleanup; + + if (virAsprintf(&path, "%s/%s", dir, filename) < 0) + goto cleanup; + + if (virHashUpdateEntry(files, filename, path) < 0) + goto cleanup; + + path = NULL; + } + + if (rc < 0) + goto cleanup; + + ret = 0; + cleanup: + virDirClose(&dirp); + return ret; +} + +static int +qemuFirmwareFilesSorter(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + return strcmp(a->key, b->key); +} + +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware" +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware" + +int +qemuFirmwareFetchConfigs(char ***firmwares) +{ + VIR_AUTOPTR(virHashTable) files = NULL; + VIR_AUTOFREE(char *) homeConfig = NULL; + VIR_AUTOFREE(char *) xdgConfig = NULL; + VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL; + virHashKeyValuePairPtr tmp = NULL; + + *firmwares = NULL; + + if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0) + return -1; + + if (!xdgConfig) { + VIR_AUTOFREE(char *) home = virGetUserDirectory(); + + if (!home) + return -1; + + if (virAsprintf(&xdgConfig, "%s/.config", home) < 0) + return -1; + } + + if (virAsprintf(&homeConfig, "%s/qemu/firmware", xdgConfig) < 0) + return -1; + + if (!(files = virHashCreate(10, virHashValueFree))) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, homeConfig) < 0) + return -1; + + /* At this point, the @files hash table contains unique set of filenames + * where each filename (as key) has the highest priority full pathname + * associated with it. */ + + if (virHashSize(files) == 0) + return 0; + + if (!(pairs = virHashGetItems(files, qemuFirmwareFilesSorter))) + return -1; + + for (tmp = pairs; tmp->key; tmp++) { + const char *path = tmp->value; + off_t len; + + if ((len = virFileLength(path, -1)) < 0) { + virReportSystemError(errno, + _("unable to get size of '%s'"), + path); + return -1; + } + + VIR_DEBUG("firmware description path '%s' len=%jd", + path, (intmax_t) len); + + if (len == 0) { + /* Empty files are used to mask less specific instances + * of the same file. */ + continue; + } + + if (virStringListAdd(firmwares, path) < 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h index 952615d42b..321169f56c 100644 --- a/src/qemu/qemu_firmware.h +++ b/src/qemu/qemu_firmware.h @@ -37,4 +37,7 @@ qemuFirmwareParse(const char *path); char * qemuFirmwareFormat(qemuFirmwarePtr fw); +int +qemuFirmwareFetchConfigs(char ***firmwares); + #endif /* LIBVIRT_QEMU_FIRMWARE_H */ -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:19AM +0100, Michal Privoznik wrote:
Implementation for yet another part of firmware description specification. This one covers selecting which files to parse.
There are three locations from which description files can be loaded. In order of preference, from most generic to most specific these are:
/usr/share/qemu/firmware /etc/qemu/firmware $XDG_CONFIG_HOME/qemu/firmware
If a file is found in two or more locations then the most specific one is used. Moreover, if file is empty then it means it is overriding some generic description and disabling it.
Again, this is described in more details and with nice examples in firmware.json specification (qemu commit 3a0adfc9bf).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 134 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 137 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 8f718ee2a6..a818f60c91 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -21,9 +21,11 @@ #include <config.h>
#include "qemu_firmware.h" +#include "configmake.h" #include "qemu_capabilities.h" #include "virarch.h" #include "virfile.h" +#include "virhash.h" #include "virjson.h" #include "virlog.h" #include "virstring.h" @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
return virJSONValueToString(doc, true); } + + +static int +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir) +{ + DIR *dirp; + struct dirent *ent = NULL; + int rc; + int ret = -1; + + if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) + return -1; + + if (rc == 0) + return 0; + + while ((rc = virDirRead(dirp, &ent, dir)) > 0) { + VIR_AUTOFREE(char *) filename = NULL; + VIR_AUTOFREE(char *) path = NULL; + + if (ent->d_type != DT_REG && ent->d_type != DT_LNK) + continue; + + if (STRPREFIX(ent->d_name, ".")) + continue; + + if (VIR_STRDUP(filename, ent->d_name) < 0) + goto cleanup; + + if (virAsprintf(&path, "%s/%s", dir, filename) < 0) + goto cleanup; + + if (virHashUpdateEntry(files, filename, path) < 0) + goto cleanup; + + path = NULL; + } + + if (rc < 0) + goto cleanup; + + ret = 0; + cleanup: + virDirClose(&dirp); + return ret; +} + +static int +qemuFirmwareFilesSorter(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + return strcmp(a->key, b->key); +} + +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware" +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware" + +int +qemuFirmwareFetchConfigs(char ***firmwares) +{ + VIR_AUTOPTR(virHashTable) files = NULL; + VIR_AUTOFREE(char *) homeConfig = NULL; + VIR_AUTOFREE(char *) xdgConfig = NULL; + VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL; + virHashKeyValuePairPtr tmp = NULL; + + *firmwares = NULL; + + if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0) + return -1; + + if (!xdgConfig) { + VIR_AUTOFREE(char *) home = virGetUserDirectory(); + + if (!home) + return -1; + + if (virAsprintf(&xdgConfig, "%s/.config", home) < 0) + return -1; + } + + if (virAsprintf(&homeConfig, "%s/qemu/firmware", xdgConfig) < 0) + return -1; + + if (!(files = virHashCreate(10, virHashValueFree))) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, homeConfig) < 0) + return -1;
I wonder if it really makes sense to read /root/ for this. Normally we would only look at the home config for unprivileged daemon, eg we don't look in /root for finding PKI x509 certs IIRC. 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 3/7/19 12:55 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:19AM +0100, Michal Privoznik wrote:
Implementation for yet another part of firmware description specification. This one covers selecting which files to parse.
There are three locations from which description files can be loaded. In order of preference, from most generic to most specific these are:
/usr/share/qemu/firmware /etc/qemu/firmware $XDG_CONFIG_HOME/qemu/firmware
If a file is found in two or more locations then the most specific one is used. Moreover, if file is empty then it means it is overriding some generic description and disabling it.
Again, this is described in more details and with nice examples in firmware.json specification (qemu commit 3a0adfc9bf).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 134 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 137 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 8f718ee2a6..a818f60c91 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -21,9 +21,11 @@ #include <config.h>
#include "qemu_firmware.h" +#include "configmake.h" #include "qemu_capabilities.h" #include "virarch.h" #include "virfile.h" +#include "virhash.h" #include "virjson.h" #include "virlog.h" #include "virstring.h" @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
return virJSONValueToString(doc, true); } + + +static int +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir) +{ + DIR *dirp; + struct dirent *ent = NULL; + int rc; + int ret = -1; + + if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) + return -1; + + if (rc == 0) + return 0; + + while ((rc = virDirRead(dirp, &ent, dir)) > 0) { + VIR_AUTOFREE(char *) filename = NULL; + VIR_AUTOFREE(char *) path = NULL; + + if (ent->d_type != DT_REG && ent->d_type != DT_LNK) + continue; + + if (STRPREFIX(ent->d_name, ".")) + continue; + + if (VIR_STRDUP(filename, ent->d_name) < 0) + goto cleanup; + + if (virAsprintf(&path, "%s/%s", dir, filename) < 0) + goto cleanup; + + if (virHashUpdateEntry(files, filename, path) < 0) + goto cleanup; + + path = NULL; + } + + if (rc < 0) + goto cleanup; + + ret = 0; + cleanup: + virDirClose(&dirp); + return ret; +} + +static int +qemuFirmwareFilesSorter(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + return strcmp(a->key, b->key); +} + +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware" +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware" + +int +qemuFirmwareFetchConfigs(char ***firmwares) +{ + VIR_AUTOPTR(virHashTable) files = NULL; + VIR_AUTOFREE(char *) homeConfig = NULL; + VIR_AUTOFREE(char *) xdgConfig = NULL; + VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL; + virHashKeyValuePairPtr tmp = NULL; + + *firmwares = NULL; + + if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0) + return -1; + + if (!xdgConfig) { + VIR_AUTOFREE(char *) home = virGetUserDirectory(); + + if (!home) + return -1; + + if (virAsprintf(&xdgConfig, "%s/.config", home) < 0) + return -1; + } + + if (virAsprintf(&homeConfig, "%s/qemu/firmware", xdgConfig) < 0) + return -1; + + if (!(files = virHashCreate(10, virHashValueFree))) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, homeConfig) < 0) + return -1;
I wonder if it really makes sense to read /root/ for this. Normally we would only look at the home config for unprivileged daemon, eg we don't look in /root for finding PKI x509 certs IIRC.
Fair enough. Root is able to put configs into /etc/qemu/firmware anyways. Will fix this locally for now. Michal

On 03/07/19 13:46, Michal Privoznik wrote:
On 3/7/19 12:55 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:19AM +0100, Michal Privoznik wrote:
Implementation for yet another part of firmware description specification. This one covers selecting which files to parse.
There are three locations from which description files can be loaded. In order of preference, from most generic to most specific these are:
/usr/share/qemu/firmware /etc/qemu/firmware $XDG_CONFIG_HOME/qemu/firmware
If a file is found in two or more locations then the most specific one is used. Moreover, if file is empty then it means it is overriding some generic description and disabling it.
Again, this is described in more details and with nice examples in firmware.json specification (qemu commit 3a0adfc9bf).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 134 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 137 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 8f718ee2a6..a818f60c91 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -21,9 +21,11 @@ #include <config.h> #include "qemu_firmware.h" +#include "configmake.h" #include "qemu_capabilities.h" #include "virarch.h" #include "virfile.h" +#include "virhash.h" #include "virjson.h" #include "virlog.h" #include "virstring.h" @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw) return virJSONValueToString(doc, true); } + + +static int +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir) +{ + DIR *dirp; + struct dirent *ent = NULL; + int rc; + int ret = -1; + + if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) + return -1; + + if (rc == 0) + return 0; + + while ((rc = virDirRead(dirp, &ent, dir)) > 0) { + VIR_AUTOFREE(char *) filename = NULL; + VIR_AUTOFREE(char *) path = NULL; + + if (ent->d_type != DT_REG && ent->d_type != DT_LNK) + continue; + + if (STRPREFIX(ent->d_name, ".")) + continue; + + if (VIR_STRDUP(filename, ent->d_name) < 0) + goto cleanup; + + if (virAsprintf(&path, "%s/%s", dir, filename) < 0) + goto cleanup; + + if (virHashUpdateEntry(files, filename, path) < 0) + goto cleanup; + + path = NULL; + } + + if (rc < 0) + goto cleanup; + + ret = 0; + cleanup: + virDirClose(&dirp); + return ret; +} + +static int +qemuFirmwareFilesSorter(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + return strcmp(a->key, b->key); +} + +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware" +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware" + +int +qemuFirmwareFetchConfigs(char ***firmwares) +{ + VIR_AUTOPTR(virHashTable) files = NULL; + VIR_AUTOFREE(char *) homeConfig = NULL; + VIR_AUTOFREE(char *) xdgConfig = NULL; + VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL; + virHashKeyValuePairPtr tmp = NULL; + + *firmwares = NULL; + + if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0) + return -1; + + if (!xdgConfig) { + VIR_AUTOFREE(char *) home = virGetUserDirectory(); + + if (!home) + return -1; + + if (virAsprintf(&xdgConfig, "%s/.config", home) < 0) + return -1; + } + + if (virAsprintf(&homeConfig, "%s/qemu/firmware", xdgConfig) < 0) + return -1; + + if (!(files = virHashCreate(10, virHashValueFree))) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, homeConfig) < 0) + return -1;
I wonder if it really makes sense to read /root/ for this. Normally we would only look at the home config for unprivileged daemon, eg we don't look in /root for finding PKI x509 certs IIRC.
Fair enough. Root is able to put configs into /etc/qemu/firmware anyways. Will fix this locally for now.
Please carry forward my R-b, given up-thread, when you implement the tweak suggested by Dan. Thanks Laszlo

On 03/07/19 10:29, Michal Privoznik wrote:
Implementation for yet another part of firmware description specification. This one covers selecting which files to parse.
There are three locations from which description files can be loaded. In order of preference, from most generic to most specific these are:
/usr/share/qemu/firmware /etc/qemu/firmware $XDG_CONFIG_HOME/qemu/firmware
If a file is found in two or more locations then the most specific one is used. Moreover, if file is empty then it means it is overriding some generic description and disabling it.
Again, this is described in more details and with nice examples in firmware.json specification (qemu commit 3a0adfc9bf).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 134 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 137 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 8f718ee2a6..a818f60c91 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -21,9 +21,11 @@ #include <config.h>
#include "qemu_firmware.h" +#include "configmake.h" #include "qemu_capabilities.h" #include "virarch.h" #include "virfile.h" +#include "virhash.h" #include "virjson.h" #include "virlog.h" #include "virstring.h" @@ -899,3 +901,135 @@ qemuFirmwareFormat(qemuFirmwarePtr fw)
return virJSONValueToString(doc, true); } + + +static int +qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir) +{ + DIR *dirp; + struct dirent *ent = NULL; + int rc; + int ret = -1; + + if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) + return -1; + + if (rc == 0) + return 0; + + while ((rc = virDirRead(dirp, &ent, dir)) > 0) { + VIR_AUTOFREE(char *) filename = NULL; + VIR_AUTOFREE(char *) path = NULL; + + if (ent->d_type != DT_REG && ent->d_type != DT_LNK) + continue; + + if (STRPREFIX(ent->d_name, ".")) + continue; + + if (VIR_STRDUP(filename, ent->d_name) < 0) + goto cleanup; + + if (virAsprintf(&path, "%s/%s", dir, filename) < 0) + goto cleanup; + + if (virHashUpdateEntry(files, filename, path) < 0) + goto cleanup; + + path = NULL; + } + + if (rc < 0) + goto cleanup; + + ret = 0; + cleanup: + virDirClose(&dirp); + return ret; +} + +static int +qemuFirmwareFilesSorter(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + return strcmp(a->key, b->key); +} + +#define QEMU_FIRMWARE_SYSTEM_LOCATION PREFIX "/share/qemu/firmware" +#define QEMU_FIRMWARE_ETC_LOCATION SYSCONFDIR "/qemu/firmware" + +int +qemuFirmwareFetchConfigs(char ***firmwares) +{ + VIR_AUTOPTR(virHashTable) files = NULL; + VIR_AUTOFREE(char *) homeConfig = NULL; + VIR_AUTOFREE(char *) xdgConfig = NULL; + VIR_AUTOFREE(virHashKeyValuePairPtr) pairs = NULL; + virHashKeyValuePairPtr tmp = NULL; + + *firmwares = NULL; + + if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0) + return -1; + + if (!xdgConfig) { + VIR_AUTOFREE(char *) home = virGetUserDirectory(); + + if (!home) + return -1; + + if (virAsprintf(&xdgConfig, "%s/.config", home) < 0) + return -1; + } + + if (virAsprintf(&homeConfig, "%s/qemu/firmware", xdgConfig) < 0) + return -1; + + if (!(files = virHashCreate(10, virHashValueFree))) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_SYSTEM_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, QEMU_FIRMWARE_ETC_LOCATION) < 0) + return -1; + + if (qemuFirmwareBuildFileList(files, homeConfig) < 0) + return -1; + + /* At this point, the @files hash table contains unique set of filenames + * where each filename (as key) has the highest priority full pathname + * associated with it. */ + + if (virHashSize(files) == 0) + return 0; + + if (!(pairs = virHashGetItems(files, qemuFirmwareFilesSorter))) + return -1; + + for (tmp = pairs; tmp->key; tmp++) { + const char *path = tmp->value; + off_t len; + + if ((len = virFileLength(path, -1)) < 0) { + virReportSystemError(errno, + _("unable to get size of '%s'"), + path); + return -1; + } + + VIR_DEBUG("firmware description path '%s' len=%jd", + path, (intmax_t) len); + + if (len == 0) { + /* Empty files are used to mask less specific instances + * of the same file. */ + continue; + } + + if (virStringListAdd(firmwares, path) < 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h index 952615d42b..321169f56c 100644 --- a/src/qemu/qemu_firmware.h +++ b/src/qemu/qemu_firmware.h @@ -37,4 +37,7 @@ qemuFirmwareParse(const char *path); char * qemuFirmwareFormat(qemuFirmwarePtr fw);
+int +qemuFirmwareFetchConfigs(char ***firmwares); + #endif /* LIBVIRT_QEMU_FIRMWARE_H */
Thanks for addressing my comments! Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + .../etc/qemu/firmware/60-ovmf-sb.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware/40-bios.json} | 0 .../share/qemu/firmware/50-ovmf-sb-keys.json} | 0 .../share/qemu/firmware/60-ovmf-sb.json} | 0 .../share/qemu/firmware/61-ovmf.json} | 0 .../share/qemu/firmware/70-aavmf.json} | 0 tests/qemufirmwaretest.c | 72 +++++++++++++++++-- 10 files changed, 68 insertions(+), 6 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{bios.json => usr/share/qemu/firmware/40-bios.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb-keys.json => usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb.json => usr/share/qemu/firmware/60-ovmf-sb.json} (100%) rename tests/qemufirmwaredata/{ovmf.json => usr/share/qemu/firmware/61-ovmf.json} (100%) rename tests/qemufirmwaredata/{aavmf.json => usr/share/qemu/firmware/70-aavmf.json} (100%) diff --git a/tests/Makefile.am b/tests/Makefile.am index b3449fa96b..b18b9e67ae 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) qemufirmwaretest_SOURCES = \ qemufirmwaretest.c \ testutils.h testutils.c \ + virfilewrapper.c virfilewrapper.h \ $(NULL) qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS) diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json new file mode 120000 index 0000000000..68e8cbbc2a --- /dev/null +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json @@ -0,0 +1 @@ +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json \ No newline at end of file diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json similarity index 100% rename from tests/qemufirmwaredata/bios.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb-keys.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json similarity index 100% rename from tests/qemufirmwaredata/ovmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json similarity index 100% rename from tests/qemufirmwaredata/aavmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index 176cf0920d..cbf92f2689 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -1,7 +1,9 @@ #include <config.h> #include "testutils.h" +#include "virfilewrapper.h" #include "qemu/qemu_firmware.h" +#include "configmake.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -50,11 +52,66 @@ testParseFormatFW(const void *opaque) } +static int +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED) +{ + VIR_AUTOFREE(char *) fakehome = NULL; + VIR_AUTOSTRINGLIST fwList = NULL; + size_t nfwList; + size_t i; + const char *expected[] = { + PREFIX "/share/qemu/firmware/40-bios.json", + SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/61-ovmf.json", + PREFIX "/share/qemu/firmware/70-aavmf.json", + }; + + if (VIR_STRDUP(fakehome, abs_srcdir "/qemufirmwaredata/home/user/.config") < 0) + return -1; + + setenv("XDG_CONFIG_HOME", fakehome, 1); + + if (qemuFirmwareFetchConfigs(&fwList) < 0) + return -1; + + if (!fwList) { + fprintf(stderr, "Expected result, got nothing\n"); + return -1; + } + + nfwList = virStringListLength((const char **)fwList); + if (nfwList != ARRAY_CARDINALITY(expected)) { + fprintf(stderr, "Expected %zu paths, got %zu\n", + ARRAY_CARDINALITY(expected), nfwList); + return -1; + } + + for (i = 0; i < ARRAY_CARDINALITY(expected); i++) { + if (STRNEQ_NULLABLE(expected[i], fwList[i])) { + fprintf(stderr, + "Unexpected path (i=%zu). Expected %s got %s \n", + i, expected[i], NULLSTR(fwList[i])); + return -1; + } + } + + return 0; +} + + static int mymain(void) { int ret = 0; + virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", + abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); + virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", + abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware"); + virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", + abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware"); + #define DO_PARSE_TEST(filename) \ do { \ if (virTestRun("QEMU FW " filename, \ @@ -62,14 +119,17 @@ mymain(void) ret = -1; \ } while (0) - DO_PARSE_TEST("bios.json"); - DO_PARSE_TEST("ovmf-sb-keys.json"); - DO_PARSE_TEST("ovmf-sb.json"); - DO_PARSE_TEST("ovmf.json"); - DO_PARSE_TEST("aavmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/40-bios.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/50-ovmf-sb-keys.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/60-ovmf-sb.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/61-ovmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/70-aavmf.json"); + + if (virTestRun("QEMU FW precedence test", testFWPrecedence, NULL) < 0) + ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN(mymain) -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:20AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + .../etc/qemu/firmware/60-ovmf-sb.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware/40-bios.json} | 0 .../share/qemu/firmware/50-ovmf-sb-keys.json} | 0 .../share/qemu/firmware/60-ovmf-sb.json} | 0 .../share/qemu/firmware/61-ovmf.json} | 0 .../share/qemu/firmware/70-aavmf.json} | 0 tests/qemufirmwaretest.c | 72 +++++++++++++++++-- 10 files changed, 68 insertions(+), 6 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{bios.json => usr/share/qemu/firmware/40-bios.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb-keys.json => usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb.json => usr/share/qemu/firmware/60-ovmf-sb.json} (100%) rename tests/qemufirmwaredata/{ovmf.json => usr/share/qemu/firmware/61-ovmf.json} (100%) rename tests/qemufirmwaredata/{aavmf.json => usr/share/qemu/firmware/70-aavmf.json} (100%)
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }
-VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN(mymain)
Either this is an accidental change, or it should have been in the previous patch. With that fixed Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

(adding Dan) On 03/07/19 10:29, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + .../etc/qemu/firmware/60-ovmf-sb.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware/40-bios.json} | 0 .../share/qemu/firmware/50-ovmf-sb-keys.json} | 0 .../share/qemu/firmware/60-ovmf-sb.json} | 0 .../share/qemu/firmware/61-ovmf.json} | 0 .../share/qemu/firmware/70-aavmf.json} | 0 tests/qemufirmwaretest.c | 72 +++++++++++++++++-- 10 files changed, 68 insertions(+), 6 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{bios.json => usr/share/qemu/firmware/40-bios.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb-keys.json => usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb.json => usr/share/qemu/firmware/60-ovmf-sb.json} (100%) rename tests/qemufirmwaredata/{ovmf.json => usr/share/qemu/firmware/61-ovmf.json} (100%) rename tests/qemufirmwaredata/{aavmf.json => usr/share/qemu/firmware/70-aavmf.json} (100%)
diff --git a/tests/Makefile.am b/tests/Makefile.am index b3449fa96b..b18b9e67ae 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) qemufirmwaretest_SOURCES = \ qemufirmwaretest.c \ testutils.h testutils.c \ + virfilewrapper.c virfilewrapper.h \ $(NULL) qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json new file mode 120000 index 0000000000..68e8cbbc2a --- /dev/null +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json @@ -0,0 +1 @@ +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json \ No newline at end of file diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json similarity index 100% rename from tests/qemufirmwaredata/bios.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb-keys.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json similarity index 100% rename from tests/qemufirmwaredata/ovmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json similarity index 100% rename from tests/qemufirmwaredata/aavmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index 176cf0920d..cbf92f2689 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -1,7 +1,9 @@ #include <config.h>
#include "testutils.h" +#include "virfilewrapper.h" #include "qemu/qemu_firmware.h" +#include "configmake.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -50,11 +52,66 @@ testParseFormatFW(const void *opaque) }
+static int +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED) +{ + VIR_AUTOFREE(char *) fakehome = NULL; + VIR_AUTOSTRINGLIST fwList = NULL; + size_t nfwList; + size_t i; + const char *expected[] = { + PREFIX "/share/qemu/firmware/40-bios.json", + SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/61-ovmf.json", + PREFIX "/share/qemu/firmware/70-aavmf.json", + }; + + if (VIR_STRDUP(fakehome, abs_srcdir "/qemufirmwaredata/home/user/.config") < 0) + return -1; + + setenv("XDG_CONFIG_HOME", fakehome, 1); + + if (qemuFirmwareFetchConfigs(&fwList) < 0) + return -1; + + if (!fwList) { + fprintf(stderr, "Expected result, got nothing\n"); + return -1; + }
This error message is extremely confusing. You intend to say "I expected a non-NULL result, but got a NULL result". But the way I read it was: "oh nice, I got the expected result!, which is nothing". Please clean this up.
+ + nfwList = virStringListLength((const char **)fwList); + if (nfwList != ARRAY_CARDINALITY(expected)) { + fprintf(stderr, "Expected %zu paths, got %zu\n", + ARRAY_CARDINALITY(expected), nfwList); + return -1; + } + + for (i = 0; i < ARRAY_CARDINALITY(expected); i++) { + if (STRNEQ_NULLABLE(expected[i], fwList[i])) { + fprintf(stderr, + "Unexpected path (i=%zu). Expected %s got %s \n", + i, expected[i], NULLSTR(fwList[i])); + return -1; + } + } + + return 0; +} + + static int mymain(void) { int ret = 0;
+ virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", + abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); + virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", + abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware"); + virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", + abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware"); +
I don't understand what virFileWrapperAddPrefix() does. I checked the header and the C file, but I'm none the wiser. (There are no comments.) See below why this might matter.
#define DO_PARSE_TEST(filename) \ do { \ if (virTestRun("QEMU FW " filename, \ @@ -62,14 +119,17 @@ mymain(void) ret = -1; \ } while (0)
- DO_PARSE_TEST("bios.json"); - DO_PARSE_TEST("ovmf-sb-keys.json"); - DO_PARSE_TEST("ovmf-sb.json"); - DO_PARSE_TEST("ovmf.json"); - DO_PARSE_TEST("aavmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/40-bios.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/50-ovmf-sb-keys.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/60-ovmf-sb.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/61-ovmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/70-aavmf.json"); + + if (virTestRun("QEMU FW precedence test", testFWPrecedence, NULL) < 0) + ret = -1;
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }
-VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN(mymain)
After this patch, we have the following json files / symlinks, under "tests/qemufirmwaredata": etc/qemu/firmware/40-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json usr/share/qemu/firmware/50-ovmf-sb-keys.json usr/share/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json Are we establishing the precedence between *all* of them? (Again, I don't understand what virFileWrapperAddPrefix() does.) "firmware.json" says, # Management software should build a list of files from all three # locations, then sort the list by filename (i.e., last pathname # component). Management software should choose the first JSON file on # the sorted list that matches the search criteria. If a more specific # directory has a file with same name as a less specific directory, then # the file in the more specific directory takes effect. If the more # specific file is zero length, it hides the less specific one. Assuming we intend to cover all of these files, the list sorted by filename (= last pathname component) is: home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json etc/qemu/firmware/40-ovmf-sb-keys.json usr/share/qemu/firmware/50-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json In any given set, defined by last pathname component, "home" takes precedence over "etc" takes precedence over "usr". There is only one set with more than one elements -- the set represented by "60-ovmf-sb.json". In that set, we have etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/60-ovmf-sb.json and there "etc" takes precedence. So the final list should be home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json etc/qemu/firmware/40-ovmf-sb-keys.json usr/share/qemu/firmware/50-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json This is not what the test case contains however -- "expected" contains 5 elements only --, so I'm thinking that the input dataset is not what I think it is. Which, again, could be because I have no clue what virFileWrapperAddPrefix() does. Thanks Laszlo

On Tue, Mar 12, 2019 at 12:13:46PM +0100, Laszlo Ersek wrote:
(adding Dan)
On 03/07/19 10:29, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + .../etc/qemu/firmware/60-ovmf-sb.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware/40-bios.json} | 0 .../share/qemu/firmware/50-ovmf-sb-keys.json} | 0 .../share/qemu/firmware/60-ovmf-sb.json} | 0 .../share/qemu/firmware/61-ovmf.json} | 0 .../share/qemu/firmware/70-aavmf.json} | 0 tests/qemufirmwaretest.c | 72 +++++++++++++++++-- 10 files changed, 68 insertions(+), 6 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{bios.json => usr/share/qemu/firmware/40-bios.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb-keys.json => usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb.json => usr/share/qemu/firmware/60-ovmf-sb.json} (100%) rename tests/qemufirmwaredata/{ovmf.json => usr/share/qemu/firmware/61-ovmf.json} (100%) rename tests/qemufirmwaredata/{aavmf.json => usr/share/qemu/firmware/70-aavmf.json} (100%)
diff --git a/tests/Makefile.am b/tests/Makefile.am index b3449fa96b..b18b9e67ae 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) qemufirmwaretest_SOURCES = \ qemufirmwaretest.c \ testutils.h testutils.c \ + virfilewrapper.c virfilewrapper.h \ $(NULL) qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json new file mode 120000 index 0000000000..68e8cbbc2a --- /dev/null +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json @@ -0,0 +1 @@ +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json \ No newline at end of file diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json similarity index 100% rename from tests/qemufirmwaredata/bios.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb-keys.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json similarity index 100% rename from tests/qemufirmwaredata/ovmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json similarity index 100% rename from tests/qemufirmwaredata/aavmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index 176cf0920d..cbf92f2689 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -1,7 +1,9 @@ #include <config.h>
#include "testutils.h" +#include "virfilewrapper.h" #include "qemu/qemu_firmware.h" +#include "configmake.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -50,11 +52,66 @@ testParseFormatFW(const void *opaque) }
+static int +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED) +{ + VIR_AUTOFREE(char *) fakehome = NULL; + VIR_AUTOSTRINGLIST fwList = NULL; + size_t nfwList; + size_t i; + const char *expected[] = { + PREFIX "/share/qemu/firmware/40-bios.json", + SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/61-ovmf.json", + PREFIX "/share/qemu/firmware/70-aavmf.json", + }; + + if (VIR_STRDUP(fakehome, abs_srcdir "/qemufirmwaredata/home/user/.config") < 0) + return -1; + + setenv("XDG_CONFIG_HOME", fakehome, 1); + + if (qemuFirmwareFetchConfigs(&fwList) < 0) + return -1; + + if (!fwList) { + fprintf(stderr, "Expected result, got nothing\n"); + return -1; + }
This error message is extremely confusing. You intend to say "I expected a non-NULL result, but got a NULL result".
But the way I read it was: "oh nice, I got the expected result!, which is nothing".
Please clean this up.
+ + nfwList = virStringListLength((const char **)fwList); + if (nfwList != ARRAY_CARDINALITY(expected)) { + fprintf(stderr, "Expected %zu paths, got %zu\n", + ARRAY_CARDINALITY(expected), nfwList); + return -1; + } + + for (i = 0; i < ARRAY_CARDINALITY(expected); i++) { + if (STRNEQ_NULLABLE(expected[i], fwList[i])) { + fprintf(stderr, + "Unexpected path (i=%zu). Expected %s got %s \n", + i, expected[i], NULLSTR(fwList[i])); + return -1; + } + } + + return 0; +} + + static int mymain(void) { int ret = 0;
+ virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", + abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); + virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", + abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware"); + virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", + abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware"); +
I don't understand what virFileWrapperAddPrefix() does. I checked the header and the C file, but I'm none the wiser. (There are no comments.) See below why this might matter.
#define DO_PARSE_TEST(filename) \ do { \ if (virTestRun("QEMU FW " filename, \ @@ -62,14 +119,17 @@ mymain(void) ret = -1; \ } while (0)
- DO_PARSE_TEST("bios.json"); - DO_PARSE_TEST("ovmf-sb-keys.json"); - DO_PARSE_TEST("ovmf-sb.json"); - DO_PARSE_TEST("ovmf.json"); - DO_PARSE_TEST("aavmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/40-bios.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/50-ovmf-sb-keys.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/60-ovmf-sb.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/61-ovmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/70-aavmf.json"); + + if (virTestRun("QEMU FW precedence test", testFWPrecedence, NULL) < 0) + ret = -1;
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }
-VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN(mymain)
After this patch, we have the following json files / symlinks, under "tests/qemufirmwaredata":
etc/qemu/firmware/40-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json usr/share/qemu/firmware/50-ovmf-sb-keys.json usr/share/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json
Are we establishing the precedence between *all* of them? (Again, I don't understand what virFileWrapperAddPrefix() does.)
"firmware.json" says,
# Management software should build a list of files from all three # locations, then sort the list by filename (i.e., last pathname # component). Management software should choose the first JSON file on # the sorted list that matches the search criteria. If a more specific # directory has a file with same name as a less specific directory, then # the file in the more specific directory takes effect. If the more # specific file is zero length, it hides the less specific one.
Assuming we intend to cover all of these files, the list sorted by filename (= last pathname component) is:
home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json etc/qemu/firmware/40-ovmf-sb-keys.json usr/share/qemu/firmware/50-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json
In any given set, defined by last pathname component, "home" takes precedence over "etc" takes precedence over "usr".
There is only one set with more than one elements -- the set represented by "60-ovmf-sb.json". In that set, we have
etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/60-ovmf-sb.json
and there "etc" takes precedence. So the final list should be
home/user/.config/qemu/firmware/10-bios.json
This is a zero length file, so it is a blackout
usr/share/qemu/firmware/40-bios.json etc/qemu/firmware/40-ovmf-sb-keys.json usr/share/qemu/firmware/50-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json
So is this one.
usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json
This is not what the test case contains however -- "expected" contains 5 elements only --, so I'm thinking that the input dataset is not what I think it is.
You just missed that two of the files are zero-length.
Which, again, could be because I have no clue what virFileWrapperAddPrefix() does.
That is a mock override to setup a virtual root directory, so when the code tries to load from "/etc/qemu" it gets redirected to instead load files from $GIT/tests/.... 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 03/12/19 12:20, Daniel P. Berrangé wrote:
On Tue, Mar 12, 2019 at 12:13:46PM +0100, Laszlo Ersek wrote:
(adding Dan)
On 03/07/19 10:29, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + .../etc/qemu/firmware/60-ovmf-sb.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware/40-bios.json} | 0 .../share/qemu/firmware/50-ovmf-sb-keys.json} | 0 .../share/qemu/firmware/60-ovmf-sb.json} | 0 .../share/qemu/firmware/61-ovmf.json} | 0 .../share/qemu/firmware/70-aavmf.json} | 0 tests/qemufirmwaretest.c | 72 +++++++++++++++++-- 10 files changed, 68 insertions(+), 6 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{bios.json => usr/share/qemu/firmware/40-bios.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb-keys.json => usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%) rename tests/qemufirmwaredata/{ovmf-sb.json => usr/share/qemu/firmware/60-ovmf-sb.json} (100%) rename tests/qemufirmwaredata/{ovmf.json => usr/share/qemu/firmware/61-ovmf.json} (100%) rename tests/qemufirmwaredata/{aavmf.json => usr/share/qemu/firmware/70-aavmf.json} (100%)
diff --git a/tests/Makefile.am b/tests/Makefile.am index b3449fa96b..b18b9e67ae 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) qemufirmwaretest_SOURCES = \ qemufirmwaretest.c \ testutils.h testutils.c \ + virfilewrapper.c virfilewrapper.h \ $(NULL) qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json new file mode 120000 index 0000000000..68e8cbbc2a --- /dev/null +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json @@ -0,0 +1 @@ +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json \ No newline at end of file diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json similarity index 100% rename from tests/qemufirmwaredata/bios.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb-keys.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json similarity index 100% rename from tests/qemufirmwaredata/ovmf-sb.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json similarity index 100% rename from tests/qemufirmwaredata/ovmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json similarity index 100% rename from tests/qemufirmwaredata/aavmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index 176cf0920d..cbf92f2689 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -1,7 +1,9 @@ #include <config.h>
#include "testutils.h" +#include "virfilewrapper.h" #include "qemu/qemu_firmware.h" +#include "configmake.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -50,11 +52,66 @@ testParseFormatFW(const void *opaque) }
+static int +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED) +{ + VIR_AUTOFREE(char *) fakehome = NULL; + VIR_AUTOSTRINGLIST fwList = NULL; + size_t nfwList; + size_t i; + const char *expected[] = { + PREFIX "/share/qemu/firmware/40-bios.json", + SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", + PREFIX "/share/qemu/firmware/61-ovmf.json", + PREFIX "/share/qemu/firmware/70-aavmf.json", + }; + + if (VIR_STRDUP(fakehome, abs_srcdir "/qemufirmwaredata/home/user/.config") < 0) + return -1; + + setenv("XDG_CONFIG_HOME", fakehome, 1); + + if (qemuFirmwareFetchConfigs(&fwList) < 0) + return -1; + + if (!fwList) { + fprintf(stderr, "Expected result, got nothing\n"); + return -1; + }
This error message is extremely confusing. You intend to say "I expected a non-NULL result, but got a NULL result".
But the way I read it was: "oh nice, I got the expected result!, which is nothing".
Please clean this up.
+ + nfwList = virStringListLength((const char **)fwList); + if (nfwList != ARRAY_CARDINALITY(expected)) { + fprintf(stderr, "Expected %zu paths, got %zu\n", + ARRAY_CARDINALITY(expected), nfwList); + return -1; + } + + for (i = 0; i < ARRAY_CARDINALITY(expected); i++) { + if (STRNEQ_NULLABLE(expected[i], fwList[i])) { + fprintf(stderr, + "Unexpected path (i=%zu). Expected %s got %s \n", + i, expected[i], NULLSTR(fwList[i])); + return -1; + } + } + + return 0; +} + + static int mymain(void) { int ret = 0;
+ virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", + abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); + virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", + abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware"); + virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", + abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware"); +
I don't understand what virFileWrapperAddPrefix() does. I checked the header and the C file, but I'm none the wiser. (There are no comments.) See below why this might matter.
#define DO_PARSE_TEST(filename) \ do { \ if (virTestRun("QEMU FW " filename, \ @@ -62,14 +119,17 @@ mymain(void) ret = -1; \ } while (0)
- DO_PARSE_TEST("bios.json"); - DO_PARSE_TEST("ovmf-sb-keys.json"); - DO_PARSE_TEST("ovmf-sb.json"); - DO_PARSE_TEST("ovmf.json"); - DO_PARSE_TEST("aavmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/40-bios.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/50-ovmf-sb-keys.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/60-ovmf-sb.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/61-ovmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/70-aavmf.json"); + + if (virTestRun("QEMU FW precedence test", testFWPrecedence, NULL) < 0) + ret = -1;
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }
-VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN(mymain)
After this patch, we have the following json files / symlinks, under "tests/qemufirmwaredata":
etc/qemu/firmware/40-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json usr/share/qemu/firmware/50-ovmf-sb-keys.json usr/share/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json
Are we establishing the precedence between *all* of them? (Again, I don't understand what virFileWrapperAddPrefix() does.)
"firmware.json" says,
# Management software should build a list of files from all three # locations, then sort the list by filename (i.e., last pathname # component). Management software should choose the first JSON file on # the sorted list that matches the search criteria. If a more specific # directory has a file with same name as a less specific directory, then # the file in the more specific directory takes effect. If the more # specific file is zero length, it hides the less specific one.
Assuming we intend to cover all of these files, the list sorted by filename (= last pathname component) is:
home/user/.config/qemu/firmware/10-bios.json usr/share/qemu/firmware/40-bios.json etc/qemu/firmware/40-ovmf-sb-keys.json usr/share/qemu/firmware/50-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json
In any given set, defined by last pathname component, "home" takes precedence over "etc" takes precedence over "usr".
There is only one set with more than one elements -- the set represented by "60-ovmf-sb.json". In that set, we have
etc/qemu/firmware/60-ovmf-sb.json usr/share/qemu/firmware/60-ovmf-sb.json
and there "etc" takes precedence. So the final list should be
home/user/.config/qemu/firmware/10-bios.json
This is a zero length file, so it is a blackout
usr/share/qemu/firmware/40-bios.json etc/qemu/firmware/40-ovmf-sb-keys.json usr/share/qemu/firmware/50-ovmf-sb-keys.json etc/qemu/firmware/60-ovmf-sb.json
So is this one.
usr/share/qemu/firmware/61-ovmf.json usr/share/qemu/firmware/70-aavmf.json
This is not what the test case contains however -- "expected" contains 5 elements only --, so I'm thinking that the input dataset is not what I think it is.
You just missed that two of the files are zero-length.
Ahh, indeed. :)
Which, again, could be because I have no clue what virFileWrapperAddPrefix() does.
That is a mock override to setup a virtual root directory, so when the code tries to load from "/etc/qemu" it gets redirected to instead load files from $GIT/tests/....
Thanks for the explanation! Acked-by: Laszlo Ersek <lersek@redhat.com> Laszlo

And finally the last missing piece. This is what puts it all together. At the beginning, qemuFirmwareFillDomain() loads all possible firmware description files based on algorithm described earlier. Then it tries to find description which matches given domain. The criteria are: - firmware is the right type (e.g. it's bios when bios was requested in domain XML) - firmware is suitable for guest architecture/machine type - firmware allows desired guest features to stay enabled (e.g. if s3/s4 is enabled for guest then firmware has to support it too) Once the desired description has been found it is then used to set various bits of virDomainDef so that proper qemu cmd line is constructed as demanded by the description file. For instance, secure boot enabled firmware might request SMM -> it will be enabled if needed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 329 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 + 2 files changed, 336 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index a818f60c91..c8b337cf2a 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -23,6 +23,8 @@ #include "qemu_firmware.h" #include "configmake.h" #include "qemu_capabilities.h" +#include "qemu_domain.h" +#include "qemu_process.h" #include "virarch.h" #include "virfile.h" #include "virhash.h" @@ -1033,3 +1035,330 @@ qemuFirmwareFetchConfigs(char ***firmwares) return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw, + const char *path) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool requiresSMM = false; + bool supportsSEV = false; + + for (i = 0; i < fw->ninterfaces; i++) { + if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS && + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) || + (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI && + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI)) + break; + } + + if (i == fw->ninterfaces) { + VIR_DEBUG("No matching interface in '%s'", path); + return false; + } + + for (i = 0; i < fw->ntargets; i++) { + size_t j; + + if (def->os.arch != fw->targets[i]->architecture) + continue; + + for (j = 0; j < fw->targets[i]->nmachines; j++) { + if (virStringMatch(def->os.machine, fw->targets[i]->machines[j])) + break; + } + + if (j == fw->targets[i]->nmachines) + continue; + + break; + } + + if (i == fw->ntargets) { + VIR_DEBUG("No matching machine type in '%s'", path); + return false; + } + + for (i = 0; i < fw->nfeatures; i++) { + switch (fw->features[i]) { + case QEMU_FIRMWARE_FEATURE_ACPI_S3: + supportsS3 = true; + break; + case QEMU_FIRMWARE_FEATURE_ACPI_S4: + supportsS4 = true; + break; + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + supportsSEV = true; + break; + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + requiresSMM = true; + break; + + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: + case QEMU_FIRMWARE_FEATURE_NONE: + case QEMU_FIRMWARE_FEATURE_LAST: + break; + } + } + + if (def->pm.s3 == VIR_TRISTATE_BOOL_YES && + !supportsS3) { + VIR_DEBUG("Domain requires S3, firmware '%s' doesn't support it", path); + return false; + } + + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && + !supportsS4) { + VIR_DEBUG("Domain requires S3, firmware '%s' doesn't support it", path); + return false; + } + + if (def->os.loader && + ((def->os.loader->secure == VIR_TRISTATE_BOOL_YES) != requiresSMM)) { + VIR_DEBUG("Not matching secure boot/SMM in '%s'", path); + return false; + } + + if (def->sev && + def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && + !supportsSEV) { + VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", path); + return false; + } + + VIR_DEBUG("Firmware '%s' matches domain requirements", path); + return true; +} + + +static int +qemuFirmwareEnableFeatures(virQEMUDriverPtr driver, + virDomainDefPtr def, + const qemuFirmware *fw) +{ + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); + const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash; + const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel; + const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory; + size_t i; + + switch (fw->mapping.device) { + case QEMU_FIRMWARE_DEVICE_FLASH: + if (!def->os.loader && + VIR_ALLOC(def->os.loader) < 0) + return -1; + + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH; + def->os.loader->readonly = VIR_TRISTATE_BOOL_YES; + + if (STRNEQ(flash->executable.format, "raw")) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported flash format '%s'"), + flash->executable.format); + return -1; + } + + VIR_FREE(def->os.loader->path); + if (VIR_STRDUP(def->os.loader->path, + flash->executable.filename) < 0) + return -1; + + if (STRNEQ(flash->nvram_template.format, "raw")) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported nvram template format '%s'"), + flash->nvram_template.format); + return -1; + } + + VIR_FREE(def->os.loader->templt); + if (VIR_STRDUP(def->os.loader->templt, + flash->nvram_template.filename) < 0) + return -1; + + if (qemuDomainNVRAMPathGenerate(cfg, def) < 0) + return -1; + + VIR_DEBUG("decided on firmware '%s' varstore template '%s'", + def->os.loader->path, + def->os.loader->templt); + break; + + case QEMU_FIRMWARE_DEVICE_KERNEL: + VIR_FREE(def->os.kernel); + if (VIR_STRDUP(def->os.kernel, kernel->filename) < 0) + return -1; + + VIR_DEBUG("decided on kernel '%s'", + def->os.kernel); + break; + + case QEMU_FIRMWARE_DEVICE_MEMORY: + if (!def->os.loader && + VIR_ALLOC(def->os.loader) < 0) + return -1; + + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM; + if (VIR_STRDUP(def->os.loader->path, memory->filename) < 0) + return -1; + + VIR_DEBUG("decided on loader '%s'", + def->os.loader->path); + break; + + case QEMU_FIRMWARE_DEVICE_NONE: + case QEMU_FIRMWARE_DEVICE_LAST: + break; + } + + for (i = 0; i < fw->nfeatures; i++) { + switch (fw->features[i]) { + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + switch (def->features[VIR_DOMAIN_FEATURE_SMM]) { + case VIR_TRISTATE_SWITCH_OFF: + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain has SMM turned off " + "but chosen firmware requires it")); + return -1; + break; + case VIR_TRISTATE_SWITCH_ABSENT: + VIR_DEBUG("Enabling SMM feature"); + def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON; + break; + + case VIR_TRISTATE_SWITCH_ON: + case VIR_TRISTATE_SWITCH_LAST: + break; + } + break; + + case QEMU_FIRMWARE_FEATURE_NONE: + case QEMU_FIRMWARE_FEATURE_ACPI_S3: + case QEMU_FIRMWARE_FEATURE_ACPI_S4: + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: + case QEMU_FIRMWARE_FEATURE_LAST: + break; + } + } + + return 0; +} + + +static void +qemuFirmwareSanityCheck(const qemuFirmware *fw, + const char *filename) +{ + size_t i; + bool requiresSMM = false; + bool supportsSecureBoot = false; + + for (i = 0; i < fw->nfeatures; i++) { + switch (fw->features[i]) { + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + requiresSMM = true; + break; + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_NONE: + case QEMU_FIRMWARE_FEATURE_ACPI_S3: + case QEMU_FIRMWARE_FEATURE_ACPI_S4: + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: + case QEMU_FIRMWARE_FEATURE_LAST: + break; + } + } + + if (supportsSecureBoot != requiresSMM) { + VIR_WARN("Firmware description '%s' has invalid set of features: " + "%s = %d, %s = %d", + filename, + qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_REQUIRES_SMM), + requiresSMM, + qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_SECURE_BOOT), + supportsSecureBoot); + } +} + + +int +qemuFirmwareFillDomain(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + VIR_AUTOSTRINGLIST paths = NULL; + size_t npaths = 0; + qemuFirmwarePtr *firmwares = NULL; + size_t nfirmwares = 0; + const qemuFirmware *theone = NULL; + size_t i; + int ret = -1; + + if (!(flags & VIR_QEMU_PROCESS_START_NEW)) + return 0; + + if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) + return 0; + + if (qemuFirmwareFetchConfigs(&paths) < 0) + return -1; + + npaths = virStringListLength((const char **)paths); + + if (VIR_ALLOC_N(firmwares, npaths) < 0) + return -1; + + nfirmwares = npaths; + + for (i = 0; i < nfirmwares; i++) { + if (!(firmwares[i] = qemuFirmwareParse(paths[i]))) + goto cleanup; + } + + for (i = 0; i < nfirmwares; i++) { + if (qemuFirmwareMatchDomain(vm->def, firmwares[i], paths[i])) { + theone = firmwares[i]; + VIR_DEBUG("Found matching firmware (description path '%s')", + paths[i]); + break; + } + } + + if (!theone) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unable to find any firmware to satisfy '%s'"), + virDomainOsDefFirmwareTypeToString(vm->def->os.firmware)); + goto cleanup; + } + + /* Firstly, let's do some sanity checks. If either of these + * fail we can still start the domain successfully, but it's + * likely that admin/FW manufacturer messed up. */ + qemuFirmwareSanityCheck(theone, paths[i]); + + if (qemuFirmwareEnableFeatures(driver, vm->def, theone) < 0) + goto cleanup; + + vm->def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + + ret = 0; + cleanup: + for (i = 0; i < nfirmwares; i++) + qemuFirmwareFree(firmwares[i]); + VIR_FREE(firmwares); + return ret; +} diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h index 321169f56c..5d42b8d172 100644 --- a/src/qemu/qemu_firmware.h +++ b/src/qemu/qemu_firmware.h @@ -21,7 +21,9 @@ #ifndef LIBVIRT_QEMU_FIRMWARE_H # define LIBVIRT_QEMU_FIRMWARE_H +# include "domain_conf.h" # include "viralloc.h" +# include "qemu_conf.h" typedef struct _qemuFirmware qemuFirmware; typedef qemuFirmware *qemuFirmwarePtr; @@ -40,4 +42,9 @@ qemuFirmwareFormat(qemuFirmwarePtr fw); int qemuFirmwareFetchConfigs(char ***firmwares); +int +qemuFirmwareFillDomain(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags); + #endif /* LIBVIRT_QEMU_FIRMWARE_H */ -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:21AM +0100, Michal Privoznik wrote:
And finally the last missing piece. This is what puts it all together.
At the beginning, qemuFirmwareFillDomain() loads all possible firmware description files based on algorithm described earlier. Then it tries to find description which matches given domain. The criteria are:
- firmware is the right type (e.g. it's bios when bios was requested in domain XML) - firmware is suitable for guest architecture/machine type - firmware allows desired guest features to stay enabled (e.g. if s3/s4 is enabled for guest then firmware has to support it too)
Once the desired description has been found it is then used to set various bits of virDomainDef so that proper qemu cmd line is constructed as demanded by the description file. For instance, secure boot enabled firmware might request SMM -> it will be enabled if needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 329 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 + 2 files changed, 336 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index a818f60c91..c8b337cf2a 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -23,6 +23,8 @@ #include "qemu_firmware.h" #include "configmake.h" #include "qemu_capabilities.h" +#include "qemu_domain.h" +#include "qemu_process.h" #include "virarch.h" #include "virfile.h" #include "virhash.h" @@ -1033,3 +1035,330 @@ qemuFirmwareFetchConfigs(char ***firmwares)
return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw, + const char *path) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool requiresSMM = false; + bool supportsSEV = false; + + for (i = 0; i < fw->ninterfaces; i++) { + if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS && + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) || + (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI && + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI)) + break; + } + + if (i == fw->ninterfaces) { + VIR_DEBUG("No matching interface in '%s'", path); + return false; + } + + for (i = 0; i < fw->ntargets; i++) { + size_t j; + + if (def->os.arch != fw->targets[i]->architecture) + continue; + + for (j = 0; j < fw->targets[i]->nmachines; j++) { + if (virStringMatch(def->os.machine, fw->targets[i]->machines[j]))
Hmm, virStringMatch does a regex match, but AFAIK, the firmware files are using shell style globs. IOW, I think we need fnmatch instead. I think you got lucky during testing as the glob string pc-q35-* is still going to match when interpreted as a regex. ie string prefix "pc-q35" followed by zero or many "-", followed by arbitrary text since it is not anchored 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 :|

On 03/07/19 10:29, Michal Privoznik wrote:
And finally the last missing piece. This is what puts it all together.
At the beginning, qemuFirmwareFillDomain() loads all possible firmware description files based on algorithm described earlier. Then it tries to find description which matches given domain. The criteria are:
- firmware is the right type (e.g. it's bios when bios was requested in domain XML) - firmware is suitable for guest architecture/machine type - firmware allows desired guest features to stay enabled (e.g. if s3/s4 is enabled for guest then firmware has to support it too)
Once the desired description has been found it is then used to set various bits of virDomainDef so that proper qemu cmd line is constructed as demanded by the description file. For instance, secure boot enabled firmware might request SMM -> it will be enabled if needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 329 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 + 2 files changed, 336 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index a818f60c91..c8b337cf2a 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -23,6 +23,8 @@ #include "qemu_firmware.h" #include "configmake.h" #include "qemu_capabilities.h" +#include "qemu_domain.h" +#include "qemu_process.h" #include "virarch.h" #include "virfile.h" #include "virhash.h" @@ -1033,3 +1035,330 @@ qemuFirmwareFetchConfigs(char ***firmwares)
return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw, + const char *path) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool requiresSMM = false; + bool supportsSEV = false; + + for (i = 0; i < fw->ninterfaces; i++) { + if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS && + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) || + (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI && + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI)) + break; + } + + if (i == fw->ninterfaces) { + VIR_DEBUG("No matching interface in '%s'", path); + return false; + } + + for (i = 0; i < fw->ntargets; i++) { + size_t j; + + if (def->os.arch != fw->targets[i]->architecture) + continue; + + for (j = 0; j < fw->targets[i]->nmachines; j++) { + if (virStringMatch(def->os.machine, fw->targets[i]->machines[j])) + break; + } + + if (j == fw->targets[i]->nmachines) + continue; + + break; + } + + if (i == fw->ntargets) { + VIR_DEBUG("No matching machine type in '%s'", path); + return false; + } + + for (i = 0; i < fw->nfeatures; i++) { + switch (fw->features[i]) { + case QEMU_FIRMWARE_FEATURE_ACPI_S3: + supportsS3 = true; + break; + case QEMU_FIRMWARE_FEATURE_ACPI_S4: + supportsS4 = true; + break; + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + supportsSEV = true; + break; + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + requiresSMM = true; + break; + + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: + case QEMU_FIRMWARE_FEATURE_NONE: + case QEMU_FIRMWARE_FEATURE_LAST: + break; + } + } + + if (def->pm.s3 == VIR_TRISTATE_BOOL_YES && + !supportsS3) { + VIR_DEBUG("Domain requires S3, firmware '%s' doesn't support it", path); + return false; + } + + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && + !supportsS4) { + VIR_DEBUG("Domain requires S3, firmware '%s' doesn't support it", path);
(1) This debug message should refer to "S4", not "S3".
+ return false; + } + + if (def->os.loader && + ((def->os.loader->secure == VIR_TRISTATE_BOOL_YES) != requiresSMM)) { + VIR_DEBUG("Not matching secure boot/SMM in '%s'", path); + return false; + }
This check is too strict. Please refer to point (1) in: http://mid.mail-archive.com/9835d837-cfa4-d708-2f41-3d29e2254de4@redhat.com There I wrote:
(1) if REQUIRES_SMM is absent from the firmware descriptor, then @secure must be "no" in the domain config. Equivalently, if @secure is "yes", then the firmware descriptor must come with REQUIRES_SMM.
This means that "@secure *implies* REQUIRES_SMM": @secure --> REQUIRES_SMM and that an equivalent form to write the exact same logical implication is: !REQUIRES_SMM --> !@secure But the C condition above is stricter than this. It will reject (!@secure && REQUIRES_SMM). That's wrong. @secure=no *should* accept a firmware both with and without REQUIRES_SMM. @secure=yes should only accept a firmware with REQUIRES_SMM. The way to write implications in the C language is to first transform the logical predicate from the "implication operator" to the logical negation operator and the disjunction ("or") operator. In general: A --> B is equivalent to !A or B (because: false implies anything; otherwise, if "A" is true, then "B" must be true as well.) Note that, from this spelling of the "implication operator", it is evident that A-->B is equivalent to (!B)-->(!A): !(!B) or (!A) B or !A !A or B (ignoring the short-circuit behavior of the || operator in the C language, for this discussion). This is why I wrote that the following two forms were equivalent: @secure --> REQUIRES_SMM !REQUIRES_SMM --> !@secure ... Anyway, so we know how to write "@secure --> REQUIRES_SMM" with logical negation and logical-or. In the "if" condition however, we want to catch the negation of that condition. Let's calculate that: !(A --> B) // substitute formula with "or" !(!A or B) // enter De Morgan, for pushing down the negation A and !B (2) Therefore, in the C source code, we should write: if (def->os.loader && def->os.loader->secure == VIR_TRISTATE_BOOL_YES && !requiresSMM) { VIR_DEBUG("Domain restricts pflash programming to SMM, " "but firmware '%s' doesn't support SMM", path); return false; } ... Unfortunately, the term "requires SMM" is quite confusing. It is a single term that expresses: - *both* that the firmware *supports* SMM (therefore it makes sense for the user to restrict pflash r/w access to code that runs in SMM, via @secure=yes), - *and* that the firmware *requires* SMM emulation from the underlying QEMU board (hence the requirement on <smm state='on'/>, later). Back to the patch: On 03/07/19 10:29, Michal Privoznik wrote:
+ + if (def->sev && + def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && + !supportsSEV) { + VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", path); + return false; + } + + VIR_DEBUG("Firmware '%s' matches domain requirements", path); + return true; +} + + +static int +qemuFirmwareEnableFeatures(virQEMUDriverPtr driver, + virDomainDefPtr def, + const qemuFirmware *fw) +{ + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); + const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash; + const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel; + const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory; + size_t i; + + switch (fw->mapping.device) { + case QEMU_FIRMWARE_DEVICE_FLASH: + if (!def->os.loader && + VIR_ALLOC(def->os.loader) < 0) + return -1; + + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH; + def->os.loader->readonly = VIR_TRISTATE_BOOL_YES; + + if (STRNEQ(flash->executable.format, "raw")) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported flash format '%s'"), + flash->executable.format); + return -1; + } + + VIR_FREE(def->os.loader->path); + if (VIR_STRDUP(def->os.loader->path, + flash->executable.filename) < 0) + return -1; + + if (STRNEQ(flash->nvram_template.format, "raw")) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported nvram template format '%s'"), + flash->nvram_template.format); + return -1; + } + + VIR_FREE(def->os.loader->templt); + if (VIR_STRDUP(def->os.loader->templt, + flash->nvram_template.filename) < 0) + return -1; + + if (qemuDomainNVRAMPathGenerate(cfg, def) < 0) + return -1; + + VIR_DEBUG("decided on firmware '%s' varstore template '%s'", + def->os.loader->path, + def->os.loader->templt); + break; + + case QEMU_FIRMWARE_DEVICE_KERNEL: + VIR_FREE(def->os.kernel); + if (VIR_STRDUP(def->os.kernel, kernel->filename) < 0) + return -1; + + VIR_DEBUG("decided on kernel '%s'", + def->os.kernel); + break; + + case QEMU_FIRMWARE_DEVICE_MEMORY: + if (!def->os.loader && + VIR_ALLOC(def->os.loader) < 0) + return -1; + + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM; + if (VIR_STRDUP(def->os.loader->path, memory->filename) < 0) + return -1; + + VIR_DEBUG("decided on loader '%s'", + def->os.loader->path); + break; + + case QEMU_FIRMWARE_DEVICE_NONE: + case QEMU_FIRMWARE_DEVICE_LAST: + break; + } + + for (i = 0; i < fw->nfeatures; i++) { + switch (fw->features[i]) { + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + switch (def->features[VIR_DOMAIN_FEATURE_SMM]) { + case VIR_TRISTATE_SWITCH_OFF: + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain has SMM turned off " + "but chosen firmware requires it")); + return -1; + break; + case VIR_TRISTATE_SWITCH_ABSENT: + VIR_DEBUG("Enabling SMM feature"); + def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON; + break; + + case VIR_TRISTATE_SWITCH_ON: + case VIR_TRISTATE_SWITCH_LAST: + break; + } + break;
Right, this is good!
+ + case QEMU_FIRMWARE_FEATURE_NONE: + case QEMU_FIRMWARE_FEATURE_ACPI_S3: + case QEMU_FIRMWARE_FEATURE_ACPI_S4: + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: + case QEMU_FIRMWARE_FEATURE_LAST: + break; + } + } + + return 0; +} + + +static void +qemuFirmwareSanityCheck(const qemuFirmware *fw, + const char *filename) +{ + size_t i; + bool requiresSMM = false; + bool supportsSecureBoot = false; + + for (i = 0; i < fw->nfeatures; i++) { + switch (fw->features[i]) { + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + requiresSMM = true; + break; + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_NONE: + case QEMU_FIRMWARE_FEATURE_ACPI_S3: + case QEMU_FIRMWARE_FEATURE_ACPI_S4: + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: + case QEMU_FIRMWARE_FEATURE_LAST: + break; + } + } + + if (supportsSecureBoot != requiresSMM) { + VIR_WARN("Firmware description '%s' has invalid set of features: " + "%s = %d, %s = %d", + filename, + qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_REQUIRES_SMM), + requiresSMM, + qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_SECURE_BOOT), + supportsSecureBoot); + }
This is also good. I feel tempted to suggest replacing "invalid" with "dubious" or "questionable", but in fact your wording is clearer and better. Outside of development, such configs can be considered invalid. In summary: - please fix the typo in (1), - please fix the condition, and the debug message too, in (2), then you can add: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thank you! Laszlo
+} + + +int +qemuFirmwareFillDomain(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + VIR_AUTOSTRINGLIST paths = NULL; + size_t npaths = 0; + qemuFirmwarePtr *firmwares = NULL; + size_t nfirmwares = 0; + const qemuFirmware *theone = NULL; + size_t i; + int ret = -1; + + if (!(flags & VIR_QEMU_PROCESS_START_NEW)) + return 0; + + if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) + return 0; + + if (qemuFirmwareFetchConfigs(&paths) < 0) + return -1; + + npaths = virStringListLength((const char **)paths); + + if (VIR_ALLOC_N(firmwares, npaths) < 0) + return -1; + + nfirmwares = npaths; + + for (i = 0; i < nfirmwares; i++) { + if (!(firmwares[i] = qemuFirmwareParse(paths[i]))) + goto cleanup; + } + + for (i = 0; i < nfirmwares; i++) { + if (qemuFirmwareMatchDomain(vm->def, firmwares[i], paths[i])) { + theone = firmwares[i]; + VIR_DEBUG("Found matching firmware (description path '%s')", + paths[i]); + break; + } + } + + if (!theone) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unable to find any firmware to satisfy '%s'"), + virDomainOsDefFirmwareTypeToString(vm->def->os.firmware)); + goto cleanup; + } + + /* Firstly, let's do some sanity checks. If either of these + * fail we can still start the domain successfully, but it's + * likely that admin/FW manufacturer messed up. */ + qemuFirmwareSanityCheck(theone, paths[i]); + + if (qemuFirmwareEnableFeatures(driver, vm->def, theone) < 0) + goto cleanup; + + vm->def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + + ret = 0; + cleanup: + for (i = 0; i < nfirmwares; i++) + qemuFirmwareFree(firmwares[i]); + VIR_FREE(firmwares); + return ret; +} diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h index 321169f56c..5d42b8d172 100644 --- a/src/qemu/qemu_firmware.h +++ b/src/qemu/qemu_firmware.h @@ -21,7 +21,9 @@ #ifndef LIBVIRT_QEMU_FIRMWARE_H # define LIBVIRT_QEMU_FIRMWARE_H
+# include "domain_conf.h" # include "viralloc.h" +# include "qemu_conf.h"
typedef struct _qemuFirmware qemuFirmware; typedef qemuFirmware *qemuFirmwarePtr; @@ -40,4 +42,9 @@ qemuFirmwareFormat(qemuFirmwarePtr fw); int qemuFirmwareFetchConfigs(char ***firmwares);
+int +qemuFirmwareFillDomain(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags); + #endif /* LIBVIRT_QEMU_FIRMWARE_H */

When preparing domain call qemuFirmwareFillDomain() to fill in desired firmware. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 68c670d3f2..d878079eab 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -56,6 +56,7 @@ #include "qemu_interface.h" #include "qemu_security.h" #include "qemu_extdevice.h" +#include "qemu_firmware.h" #include "cpu/cpu.h" #include "datatypes.h" @@ -6082,6 +6083,10 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, if (qemuDomainSecretPrepare(driver, vm) < 0) goto cleanup; + VIR_DEBUG("Prepare bios/uefi paths"); + if (qemuFirmwareFillDomain(driver, vm, flags) < 0) + goto cleanup; + for (i = 0; i < vm->def->nchannels; i++) { if (qemuDomainPrepareChannel(vm->def->channels[i], priv->channelTargetDir) < 0) -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:22AM +0100, Michal Privoznik wrote:
When preparing domain call qemuFirmwareFillDomain() to fill in desired firmware.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 68c670d3f2..d878079eab 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -56,6 +56,7 @@ #include "qemu_interface.h" #include "qemu_security.h" #include "qemu_extdevice.h" +#include "qemu_firmware.h"
#include "cpu/cpu.h" #include "datatypes.h" @@ -6082,6 +6083,10 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, if (qemuDomainSecretPrepare(driver, vm) < 0) goto cleanup;
+ VIR_DEBUG("Prepare bios/uefi paths"); + if (qemuFirmwareFillDomain(driver, vm, flags) < 0) + goto cleanup; + for (i = 0; i < vm->def->nchannels; i++) { if (qemuDomainPrepareChannel(vm->def->channels[i], priv->channelTargetDir) < 0)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

The firmware selection code will enable the feature if needed. There's no need to require SMM to be enabled in that case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e9b2b8453b..32025ea010 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4155,7 +4155,9 @@ qemuDomainDefValidate(const virDomainDef *def, goto cleanup; } - if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) { + /* SMM will be enabled by qemuFirmwareFillDomain() if needed. */ + if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE && + def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Secure boot requires SMM feature enabled")); goto cleanup; -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:23AM +0100, Michal Privoznik wrote:
The firmware selection code will enable the feature if needed. There's no need to require SMM to be enabled in that case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 03/07/19 10:29, Michal Privoznik wrote:
The firmware selection code will enable the feature if needed. There's no need to require SMM to be enabled in that case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e9b2b8453b..32025ea010 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4155,7 +4155,9 @@ qemuDomainDefValidate(const virDomainDef *def, goto cleanup; }
- if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) { + /* SMM will be enabled by qemuFirmwareFillDomain() if needed. */ + if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE && + def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Secure boot requires SMM feature enabled")); goto cleanup;
OK. This makes sense. It restricts the check to the case when the new feature is not active. And the new feature does take care of it, in qemuFirmwareFillDomain() -> qemuFirmwareEnableFeatures(). Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo

https://bugzilla.redhat.com/show_bug.cgi?id=1564270 Now that everything is prepared for qemu driver we can enable parser feature to allow users define such domains. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 32025ea010..dd458f8a52 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6917,7 +6917,8 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG | VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN | VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS | - VIR_DOMAIN_DEF_FEATURE_USER_ALIAS, + VIR_DOMAIN_DEF_FEATURE_USER_ALIAS | + VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT, }; -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:24AM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1564270
Now that everything is prepared for qemu driver we can enable parser feature to allow users define such domains.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 4 +- ...arch64-os-firmware-efi.aarch64-latest.args | 37 ++++++++++ .../aarch64-os-firmware-efi.xml | 30 ++++++++ .../os-firmware-bios.x86_64-latest.args | 39 +++++++++++ tests/qemuxml2argvdata/os-firmware-bios.xml | 68 +++++++++++++++++++ ...os-firmware-efi-secboot.x86_64-latest.args | 43 ++++++++++++ .../os-firmware-efi-secboot.xml | 68 +++++++++++++++++++ .../os-firmware-efi.x86_64-latest.args | 42 ++++++++++++ tests/qemuxml2argvdata/os-firmware-efi.xml | 68 +++++++++++++++++++ tests/qemuxml2argvtest.c | 17 +++++ .../aarch64-os-firmware-efi.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-bios.xml | 1 + .../os-firmware-efi-secboot.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-efi.xml | 1 + tests/qemuxml2xmltest.c | 27 ++++++++ 15 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-bios.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi.xml diff --git a/tests/Makefile.am b/tests/Makefile.am index b18b9e67ae..6c366ecab4 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -585,7 +585,9 @@ qemucpumock_la_LIBADD = $(MOCKLIBS_LIBS) qemuxml2argvtest_SOURCES = \ qemuxml2argvtest.c testutilsqemu.c testutilsqemu.h \ - testutils.c testutils.h + testutils.c testutils.h \ + virfilewrapper.c virfilewrapper.h \ + $(NULL) qemuxml2argvtest_LDADD = libqemutestdriver.la \ $(LDADDS) $(LIBXML_LIBS) diff --git a/tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args b/tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args new file mode 100644 index 0000000000..fe10806ec8 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args @@ -0,0 +1,37 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 \ +-name guest=aarch64test,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-aarch64test/master-key.aes \ +-machine virt,accel=tcg,usb=off,dump-guest-core=off,gic-version=2 \ +-cpu cortex-a53 \ +-drive file=/usr/share/AAVMF/AAVMF_CODE.fd,if=pflash,format=raw,unit=0,\ +readonly=on \ +-drive file=/var/lib/libvirt/qemu/nvram/aarch64test_VARS.fd,if=pflash,\ +format=raw,unit=1 \ +-m 1024 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-kernel /aarch64.kernel \ +-initrd /aarch64.initrd \ +-append 'earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait' \ +-dtb /aarch64.dtb \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml b/tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml new file mode 100644 index 0000000000..d41355b687 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>aarch64test</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='aarch64' machine='virt'>hvm</type> + <kernel>/aarch64.kernel</kernel> + <initrd>/aarch64.initrd</initrd> + <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline> + <dtb>/aarch64.dtb</dtb> + <boot dev='hd'/> + </os> + <features> + <apic/> + <pae/> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='allow'>cortex-a53</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args new file mode 100644 index 0000000000..ffda40a711 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args @@ -0,0 +1,39 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=fedora,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-fedora/master-key.aes \ +-machine pc-q35-4.0,accel=kvm,usb=off,dump-guest-core=off \ +-bios /usr/share/seabios/bios-256k.bin \ +-m 8 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-global ICH9-LPC.disable_s3=0 \ +-global ICH9-LPC.disable_s4=1 \ +-boot menu=on,strict=on \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x8,chassis=3,id=pci.3,bus=pcie.0,addr=0x1 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/os-firmware-bios.xml b/tests/qemuxml2argvdata/os-firmware-bios.xml new file mode 100644 index 0000000000..63886666dd --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-bios.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='bios'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args new file mode 100644 index 0000000000..e26729ecc1 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args @@ -0,0 +1,43 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=fedora,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-fedora/master-key.aes \ +-machine pc-q35-4.0,accel=kvm,usb=off,smm=on,dump-guest-core=off \ +-global driver=cfi.pflash01,property=secure,value=on \ +-drive file=/usr/share/OVMF/OVMF_CODE.secboot.fd,if=pflash,format=raw,unit=0,\ +readonly=on \ +-drive file=/var/lib/libvirt/qemu/nvram/fedora_VARS.fd,if=pflash,format=raw,\ +unit=1 \ +-m 8 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-global ICH9-LPC.disable_s3=0 \ +-global ICH9-LPC.disable_s4=1 \ +-boot menu=on,strict=on \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x8,chassis=3,id=pci.3,bus=pcie.0,addr=0x1 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml b/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml new file mode 100644 index 0000000000..a285e06334 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='yes'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args new file mode 100644 index 0000000000..2080d49e5a --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args @@ -0,0 +1,42 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=fedora,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-fedora/master-key.aes \ +-machine pc-q35-4.0,accel=kvm,usb=off,dump-guest-core=off \ +-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,\ +readonly=on \ +-drive file=/var/lib/libvirt/qemu/nvram/fedora_VARS.fd,if=pflash,format=raw,\ +unit=1 \ +-m 8 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-global ICH9-LPC.disable_s3=0 \ +-global ICH9-LPC.disable_s4=1 \ +-boot menu=on,strict=on \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x8,chassis=3,id=pci.3,bus=pcie.0,addr=0x1 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/os-firmware-efi.xml b/tests/qemuxml2argvdata/os-firmware-efi.xml new file mode 100644 index 0000000000..46a7b1b780 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index de9eb6abdb..652f6f73c4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -23,6 +23,8 @@ # include "virstring.h" # include "storage/storage_driver.h" # include "virmock.h" +# include "virfilewrapper.h" +# include "configmake.h" # define LIBVIRT_QEMU_CAPSPRIV_H_ALLOW # include "qemu/qemu_capspriv.h" @@ -705,6 +707,9 @@ mymain(void) VIR_FREE(driver.config->memoryBackingDir); if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0) return EXIT_FAILURE; + VIR_FREE(driver.config->nvramDir); + if (VIR_STRDUP(driver.config->nvramDir, "/var/lib/libvirt/qemu/nvram") < 0) + return EXIT_FAILURE; capslatest = virHashCreate(4, virHashValueFree); if (!capslatest) @@ -724,6 +729,13 @@ mymain(void) VIR_TEST_VERBOSE("\n"); + virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", + abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); + virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", + abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware"); + virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", + abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware"); + /** * The following set of macros allows testing of XML -> argv conversion with a * real set of capabilities gathered from a real qemu copy. It is desired to use @@ -3010,6 +3022,11 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST("x86_64-pc-headless", "x86_64"); DO_TEST_CAPS_ARCH_LATEST("x86_64-q35-headless", "x86_64"); + DO_TEST_CAPS_LATEST("os-firmware-bios"); + DO_TEST_CAPS_LATEST("os-firmware-efi"); + DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); + DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml new file mode 120000 index 0000000000..beea6b2955 --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/aarch64-os-firmware-efi.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.xml new file mode 120000 index 0000000000..3d36d5df68 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-bios.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/os-firmware-bios.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml new file mode 120000 index 0000000000..93e184e2d2 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/os-firmware-efi-secboot.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.xml new file mode 120000 index 0000000000..15cfad1ea0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/os-firmware-efi.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e08d30c676..b547cfbac2 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1029,9 +1029,36 @@ mymain(void) DO_TEST("smbios", NONE); DO_TEST("smbios-multiple-type2", NONE); + DO_TEST("os-firmware-bios", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_QXL); + DO_TEST("os-firmware-efi", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_QXL); + DO_TEST("os-firmware-efi-secboot", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_DEVICE_QXL); + DO_TEST("aarch64-aavmf-virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO, QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); + DO_TEST("aarch64-os-firmware-efi", + QEMU_CAPS_DEVICE_VIRTIO_MMIO); DO_TEST("aarch64-virtio-pci-default", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, QEMU_CAPS_DEVICE_VIRTIO_MMIO, -- 2.19.2

On Thu, Mar 07, 2019 at 10:29:25AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 4 +- ...arch64-os-firmware-efi.aarch64-latest.args | 37 ++++++++++ .../aarch64-os-firmware-efi.xml | 30 ++++++++ .../os-firmware-bios.x86_64-latest.args | 39 +++++++++++ tests/qemuxml2argvdata/os-firmware-bios.xml | 68 +++++++++++++++++++ ...os-firmware-efi-secboot.x86_64-latest.args | 43 ++++++++++++ .../os-firmware-efi-secboot.xml | 68 +++++++++++++++++++ .../os-firmware-efi.x86_64-latest.args | 42 ++++++++++++ tests/qemuxml2argvdata/os-firmware-efi.xml | 68 +++++++++++++++++++ tests/qemuxml2argvtest.c | 17 +++++ .../aarch64-os-firmware-efi.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-bios.xml | 1 + .../os-firmware-efi-secboot.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-efi.xml | 1 + tests/qemuxml2xmltest.c | 27 ++++++++ 15 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-bios.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi.xml
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Though I would have merged this with the previous patch to prove that enabling the code was functionally correct. 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 3/11/19 4:27 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:25AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 4 +- ...arch64-os-firmware-efi.aarch64-latest.args | 37 ++++++++++ .../aarch64-os-firmware-efi.xml | 30 ++++++++ .../os-firmware-bios.x86_64-latest.args | 39 +++++++++++ tests/qemuxml2argvdata/os-firmware-bios.xml | 68 +++++++++++++++++++ ...os-firmware-efi-secboot.x86_64-latest.args | 43 ++++++++++++ .../os-firmware-efi-secboot.xml | 68 +++++++++++++++++++ .../os-firmware-efi.x86_64-latest.args | 42 ++++++++++++ tests/qemuxml2argvdata/os-firmware-efi.xml | 68 +++++++++++++++++++ tests/qemuxml2argvtest.c | 17 +++++ .../aarch64-os-firmware-efi.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-bios.xml | 1 + .../os-firmware-efi-secboot.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-efi.xml | 1 + tests/qemuxml2xmltest.c | 27 ++++++++ 15 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-bios.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi.xml
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thank you guys both. I've merged these. I'm pleasantly surprised it took only two iterations :-) Michal

On 03/12/19 16:15, Michal Privoznik wrote:
On 3/11/19 4:27 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:25AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 4 +- ...arch64-os-firmware-efi.aarch64-latest.args | 37 ++++++++++ .../aarch64-os-firmware-efi.xml | 30 ++++++++ .../os-firmware-bios.x86_64-latest.args | 39 +++++++++++ tests/qemuxml2argvdata/os-firmware-bios.xml | 68 +++++++++++++++++++ ...os-firmware-efi-secboot.x86_64-latest.args | 43 ++++++++++++ .../os-firmware-efi-secboot.xml | 68 +++++++++++++++++++ .../os-firmware-efi.x86_64-latest.args | 42 ++++++++++++ tests/qemuxml2argvdata/os-firmware-efi.xml | 68 +++++++++++++++++++ tests/qemuxml2argvtest.c | 17 +++++ .../aarch64-os-firmware-efi.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-bios.xml | 1 + .../os-firmware-efi-secboot.xml | 1 + tests/qemuxml2xmloutdata/os-firmware-efi.xml | 1 + tests/qemuxml2xmltest.c | 27 ++++++++ 15 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-bios.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-secboot.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-bios.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml create mode 120000 tests/qemuxml2xmloutdata/os-firmware-efi.xml
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thank you guys both. I've merged these. I'm pleasantly surprised it took only two iterations :-)
Thank *you*! Laszlo

On Thu, Mar 07, 2019 at 10:29:10AM +0100, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2019-February/msg01503.html
As usual, you can find my patches at my github:
https://github.com/zippy2/libvirt/commits/firmware_v2
diff to v1: - Hopefully all Lazlo's comment are worked in (fixing the logic that chooses suitable firmware for secboot/SMM domains, commit message adjustements, sanity check for parsed FW descriptions, etc.)
- Added more debug messages around FW selection code, i.e. it'll be visible from the logs why given FS is not suitable (or that it is).
I've done a very quick review of the patches and the design and overall implementation strategy looks good to me. A few misc points - We ought to expose the list of firmware types supported in the domain capabilities, so mgmt apps can probe if uefi is available - What should we do about the 'nvram' config from /etc/libvirt/qemu.conf We can't get rid of it for a while, since the new firmware configs won't be widely supported by distros in forseeable future. In the places which currently use the 'nvram' config though, should we make sure we consult the firmware configs in prefereence to resolve the var store ? Maybe your code is already doing this and I missed the logic from the diffs. - We support 'bios' and 'efi' right now, but QEMU also reoprts 'openfirmware' and 'uboot' as supported types. In practice no config files probably exist for those yet, but if it is easy to make the code support them we should try todo it to get parity with the QEMU spec I don't think any of these are blockers though - all would work as followup patches I expect. 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 3/7/19 1:04 PM, Daniel P. Berrangé wrote:
On Thu, Mar 07, 2019 at 10:29:10AM +0100, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2019-February/msg01503.html
As usual, you can find my patches at my github:
https://github.com/zippy2/libvirt/commits/firmware_v2
diff to v1: - Hopefully all Lazlo's comment are worked in (fixing the logic that chooses suitable firmware for secboot/SMM domains, commit message adjustements, sanity check for parsed FW descriptions, etc.)
- Added more debug messages around FW selection code, i.e. it'll be visible from the logs why given FS is not suitable (or that it is).
I've done a very quick review of the patches and the design and overall implementation strategy looks good to me. A few misc points
- We ought to expose the list of firmware types supported in the domain capabilities, so mgmt apps can probe if uefi is available
Makes sense, I'll start working on this after these are merged.
- What should we do about the 'nvram' config from /etc/libvirt/qemu.conf We can't get rid of it for a while, since the new firmware configs won't be widely supported by distros in forseeable future. In the places which currently use the 'nvram' config though, should we make sure we consult the firmware configs in prefereence to resolve the var store ? Maybe your code is already doing this and I missed the logic from the diffs.
I think we can ignore 'nvram' from qemu.conf safely. The whole point of nvram variable is to define which varstore corresponds to which fw image. These pairs are consulted if and only if no template was specified in domain XML. With firmware autoselect I don't think we are going to fulfil the condition as my code takes whatever FW config said the path to NVRAM is and uses that as template for the domain. And even if FW config did not provide any NVRAM path (which can be viewed as bug IMO) then we would consult qemu.conf (or some hardwired defaults).
- We support 'bios' and 'efi' right now, but QEMU also reoprts 'openfirmware' and 'uboot' as supported types. In practice no config files probably exist for those yet, but if it is easy to make the code support them we should try todo it to get parity with the QEMU spec
Sure. It's just that I don't think there is a way to even configure thos via our domain XML, is there?
I don't think any of these are blockers though - all would work as followup patches I expect.
Yes, agreed. Michal
participants (3)
-
Daniel P. Berrangé
-
Laszlo Ersek
-
Michal Privoznik