[libvirt] [PATCH v1 00/15] Firmware auto selection
Libvirt allows specifying firmware for domains for quite some time now. However, problem for mgmt applications is that they do not know which firmware to chose as all they see are their paths and from that it's impossible to tell whether one of them supports say secure boot. This problem was addressed by qemu where Lazslo and Daniel created a document, specification which describes metadata for each individual firmware image. In the description (which itself is a JSON file for easy machine parsing) then it's specified whether the firmware it's describing supports secureboot, s3/s4 states, it it's bios or efi, and so on. These patches take advantage of that, and even though the description files are not picked up by that many distributions yet, it allows users to not care about putting specific firmware path into their domain XML. It's as easy as: <os firmware='efi'> <loader secure='yes'/> </os> to have libvirt pick up OVMF image with secure enabled boot (and enabled System Management Mode at the same time). The metadata specification lives under qemu.git/docs/interop/firmware.json and I highly recommend you go and read it before reviewing (unless you're Laszlo or Daniel in which case you already know what the document says). As usual, you can find my patches at my github: https://github.com/zippy2/libvirt/commits/firmware_v1 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 | 1285 +++++++++++++++++ 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.json | 1 + .../etc/qemu/firmware/60-ovmf.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../usr/share/qemu/firmware/40-bios.json | 35 + .../usr/share/qemu/firmware/50-ovmf-sb.json | 36 + .../usr/share/qemu/firmware/60-ovmf.json | 35 + .../usr/share/qemu/firmware/70-aavmf.json | 35 + tests/qemufirmwaretest.c | 129 ++ ...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 | 42 + .../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 +- 39 files changed, 2255 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.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf.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.json create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/60-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
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> --- 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 59fe1eb401..cf7e650b34 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3876,14 +3876,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; @@ -13944,3 +13938,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 02/27/19 11:04, 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> --- 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 59fe1eb401..cf7e650b34 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3876,14 +3876,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; @@ -13944,3 +13938,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 */
I'm not familiar with restrictions on helper functions (e.g. if they are supposed to sanity check input params for null pointers etc), but as a normal code extraction patch, this looks OK to me. Also presumably the other call will arrive from a different C file, hence the external linkage and header file change. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
On 2/28/19 10:11 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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> --- 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 59fe1eb401..cf7e650b34 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3876,14 +3876,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; @@ -13944,3 +13938,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 */
I'm not familiar with restrictions on helper functions (e.g. if they are supposed to sanity check input params for null pointers etc), but as a normal code extraction patch, this looks OK to me.
There are no rules in libvirt for that. Usually, we take the path of least resistance. So when some piece of code needs to be reused, we just move it into a function and expose it. Without us adding special checks (e.g. argument sanitization). Those are added if the function might be called from another place where the assumption that arguments are sane can't be made. But it's not the case for this function.
Also presumably the other call will arrive from a different C file, hence the external linkage and header file change.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks, Michal
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> --- 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 b48bcbebee..32e7a975a2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -615,7 +615,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; @@ -628,7 +628,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 ba84052bca..eb0fa5f3c0 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -642,4 +642,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 02/27/19 11:04, 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> --- 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 b48bcbebee..32e7a975a2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -615,7 +615,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; @@ -628,7 +628,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 ba84052bca..eb0fa5f3c0 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -642,4 +642,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 */
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Except not really. At least for now. In the future, the firmware will be selected automagically. Therefore, it makes no sense to require path to one 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> --- 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 5345e54342..bebe39de76 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 477deb777e..f622a4dddf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6564,6 +6564,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) { @@ -6602,6 +6618,9 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefMemtuneValidate(def) < 0) return -1; + if (virDomainDefOSValidate(def) < 0) + return -1; + return 0; } @@ -18668,6 +18687,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, @@ -27589,9 +27611,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 02/27/19 11:04, 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 path to one in the domain
I suggest replacing "path to one" with "the pathname of a specific firmware binary". "One" in the above context is meaningful, but it takes some mental gymnastics to resolve.
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> --- 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 5345e54342..bebe39de76 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 477deb777e..f622a4dddf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6564,6 +6564,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) { @@ -6602,6 +6618,9 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefMemtuneValidate(def) < 0) return -1;
+ if (virDomainDefOSValidate(def) < 0) + return -1; + return 0; }
@@ -18668,6 +18687,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, @@ -27589,9 +27611,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);
I think this patch does what it says on the tin, but I'm not sure myself if "what it says on the tin" is the right thing to do. (In other words, whether the syntax proposed in the blurb is indeed our end-goal.) I don't doubt it, I just don't have an opinion. So I'll defer to Dan on this patch. Technically, it looks OK to me. With the commit message update: Reviewed-by: Laszlo Ersek <lersek@redhat.com> 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> --- 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 f622a4dddf..b436b91c66 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST, VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, + "none", "rom", "pflash", ); @@ -4286,6 +4287,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) { @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefPostParseMemory(def, data->parseFlags) < 0) return -1; + virDomainDefPostParseOs(def); + virDomainDefPostParseMemtune(def); if (virDomainDefRejectDuplicateControllers(def) < 0) @@ -18706,7 +18723,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); goto cleanup; @@ -27611,12 +27628,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 1f8454b38c..4e8c02b5e3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1899,7 +1899,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 74f34af292..92729485ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9846,6 +9846,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 cf7e650b34..cc3a01397c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12201,6 +12201,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 eafba1ae5b..0a46e6bb78 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 02/27/19 11:04, 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> --- 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(-)
Sounds pretty complex, but I guess it makes sense. If both @type and the pathname content are missing, then @type will default to NONE. (This used not to be possible, given that pathname used to be required.) If @type is absent but the pathname is present, then we flip the type manually to ROM.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f622a4dddf..b436b91c66 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, + "none", "rom", "pflash", ); @@ -4286,6 +4287,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) { @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefPostParseMemory(def, data->parseFlags) < 0) return -1;
+ virDomainDefPostParseOs(def); + virDomainDefPostParseMemtune(def);
if (virDomainDefRejectDuplicateControllers(def) < 0) @@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
if (type_str) { int type; - if ((type = virDomainLoaderTypeFromString(type_str)) < 0) { + if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {
Why is this change necessary? Hm... I assume, due to the enum auto-generation in libvirt, the introduction of "none" will automatically cause virDomainLoaderTypeFromString() to recognize "none" as value 0. But we want to act like "none" isn't a valid value (it's internal use only). Hence the same error message as before. I reckon this is OK. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
virReportError(VIR_ERR_XML_DETAIL, _("unknown type value: %s"), type_str); goto cleanup; @@ -27611,12 +27628,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 1f8454b38c..4e8c02b5e3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1899,7 +1899,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 74f34af292..92729485ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9846,6 +9846,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 cf7e650b34..cc3a01397c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12201,6 +12201,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 eafba1ae5b..0a46e6bb78 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>
On 2/28/19 10:27 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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> --- 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(-)
Sounds pretty complex, but I guess it makes sense. If both @type and the pathname content are missing, then @type will default to NONE. (This used not to be possible, given that pathname used to be required.) If @type is absent but the pathname is present, then we flip the type manually to ROM.
Right. The whole idea is that after all these patches the following domain XML should be valid: <os firmware='efi'> <loader secure='yes'/> </os> This is for inactive domain. While starting the domain libvirt fills in path and its type. But for inactive XML tehre is no path. Therefore, there should be no loader type associated with it.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f622a4dddf..b436b91c66 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
VIR_ENUM_IMPL(virDomainLoader, VIR_DOMAIN_LOADER_TYPE_LAST, + "none", "rom", "pflash", ); @@ -4286,6 +4287,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) { @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefPostParseMemory(def, data->parseFlags) < 0) return -1;
+ virDomainDefPostParseOs(def); + virDomainDefPostParseMemtune(def);
if (virDomainDefRejectDuplicateControllers(def) < 0) @@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
if (type_str) { int type; - if ((type = virDomainLoaderTypeFromString(type_str)) < 0) { + if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {
Why is this change necessary? Hm... I assume, due to the enum auto-generation in libvirt, the introduction of "none" will automatically cause virDomainLoaderTypeFromString() to recognize "none" as value 0. But we want to act like "none" isn't a valid value (it's internal use only). Hence the same error message as before.
Exactly. virDomainLoaderTypeFromString() will take whatever string user provided and return matching enum value. For instance, for "rom" it returns VIR_DOMAIN_LOADER_TYPE_ROM, "pflash" is then mapped on VIR_DOMAIN_LOADER_TYPE_PFLASH. And with this patch "none" would be mapped to VIR_DOMAIN_LOADER_TYPE_NONE which has the value of 0. And we don't want to accept "none" in the domain XML, do we? Hence the change.
I reckon this is OK.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks, Michal
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 b848e535e6..45f7af0e8b 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 bebe39de76..7b858f2685 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 b436b91c66..0ca923b505 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1033,6 +1033,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); @@ -6582,14 +6589,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; } @@ -6598,7 +6614,8 @@ virDomainDefOSValidate(const virDomainDef *def) static int -virDomainDefValidateInternal(const virDomainDef *def) +virDomainDefValidateInternal(const virDomainDef *def, + virDomainXMLOptionPtr xmlopt) { if (virDomainDefCheckDuplicateDiskInfo(def) < 0) return -1; @@ -6635,7 +6652,7 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefMemtuneValidate(def) < 0) return -1; - if (virDomainDefOSValidate(def) < 0) + if (virDomainDefOSValidate(def, xmlopt) < 0) return -1; return 0; @@ -6686,7 +6703,7 @@ virDomainDefValidate(virDomainDefPtr def, &data) < 0) return -1; - if (virDomainDefValidateInternal(def) < 0) + if (virDomainDefValidateInternal(def, xmlopt) < 0) return -1; return 0; @@ -18692,20 +18709,23 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) static int virDomainLoaderDefParseXML(xmlNodePtr node, - virDomainLoaderDefPtr loader) + virDomainLoaderDefPtr loader, + bool fwAutoSelect) { int ret = -1; char *readonly_str = NULL; char *secure_str = NULL; 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) { @@ -19120,6 +19140,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); @@ -19127,15 +19148,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); + goto error; + } + + 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) goto error; - if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) + if (virDomainLoaderDefParseXML(loader_node, + def->os.loader, + fwAutoSelect) < 0) goto error; 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); } } @@ -28459,7 +28500,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 4e8c02b5e3..c1e7845cf8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1950,10 +1950,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; @@ -2670,6 +2681,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 038a744981..fdd67a7fde 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -504,6 +504,8 @@ virDomainObjTaint; virDomainObjUpdateModificationImpact; virDomainObjWait; virDomainObjWaitUntil; +virDomainOsDefFirmwareTypeFromString; +virDomainOsDefFirmwareTypeToString; virDomainOSTypeFromString; virDomainOSTypeToString; virDomainParseMemory; -- 2.19.2
On 02/27/19 11:04, 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'/> </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(-)
I prefer to leave the review of this patch to others; again because I'm unsure if this is the desired syntax in the domain XML. Thanks Laszlo
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..e709a5f608 --- /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 02/27/19 11:04, 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
Wow. I assumed the parser for the schema could be auto-generated from the schema JSON. Isn't that the case with the QMP commands and events? Thanks Laszlo
On 2/28/19 10:31 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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
Wow. I assumed the parser for the schema could be auto-generated from the schema JSON. Isn't that the case with the QMP commands and events?
Not really. We don't have any script to generate parser from qapi. We did not need it so far too as we have a lot of logic in our QMP command generation code. Michal
On 03/05/19 15:38, Michal Privoznik wrote:
On 2/28/19 10:31 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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
Wow. I assumed the parser for the schema could be auto-generated from the schema JSON. Isn't that the case with the QMP commands and events?
Not really. We don't have any script to generate parser from qapi. We did not need it so far too as we have a lot of logic in our QMP command generation code.
Hmmm okay. If you generally interleave business logic and sanity checks with QMP parsing (such as when processing events and command responses), then I see the point. In the present case, the pattern is different (first some larger-scale parsing, second heavy verification etc), but I get the point of not adding a parser generator just for this. Well, I'll let others review this patch then. :) Given the schema definition, I think the patch can be verified regardless of the semantics. Thanks Laszlo
Test firmware description parsing so far. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/qemufirmwaredata/40-bios.json | 35 +++++++++++++ tests/qemufirmwaredata/50-ovmf-sb.json | 36 +++++++++++++ tests/qemufirmwaredata/60-ovmf.json | 35 +++++++++++++ tests/qemufirmwaredata/70-aavmf.json | 35 +++++++++++++ tests/qemufirmwaretest.c | 70 ++++++++++++++++++++++++++ 6 files changed, 220 insertions(+) create mode 100644 tests/qemufirmwaredata/40-bios.json create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/60-ovmf.json create mode 100644 tests/qemufirmwaredata/70-aavmf.json create mode 100644 tests/qemufirmwaretest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index c3f633cee0..d23a8c2812 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -132,6 +132,7 @@ EXTRA_DIST = \ qemuxml2xmloutdata \ qemustatusxml2xmldata \ qemumemlockdata \ + qemufirmwaredata \ secretxml2xmlin \ securityselinuxhelperdata \ securityselinuxlabeldata \ @@ -291,6 +292,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \ qemublocktest \ qemumigparamstest \ qemusecuritytest \ + qemufirmwaretest \ $(NULL) test_helpers += qemucapsprobe test_libraries += libqemumonitortestutils.la \ @@ -698,6 +700,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 \ @@ -711,6 +719,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/40-bios.json b/tests/qemufirmwaredata/40-bios.json new file mode 100644 index 0000000000..137ff70779 --- /dev/null +++ b/tests/qemufirmwaredata/40-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/50-ovmf-sb.json b/tests/qemufirmwaredata/50-ovmf-sb.json new file mode 100644 index 0000000000..c804ac1038 --- /dev/null +++ b/tests/qemufirmwaredata/50-ovmf-sb.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/60-ovmf.json b/tests/qemufirmwaredata/60-ovmf.json new file mode 100644 index 0000000000..5e8a94ae78 --- /dev/null +++ b/tests/qemufirmwaredata/60-ovmf.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/70-aavmf.json b/tests/qemufirmwaredata/70-aavmf.json new file mode 100644 index 0000000000..114d1475a2 --- /dev/null +++ b/tests/qemufirmwaredata/70-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/qemufirmwaretest.c b/tests/qemufirmwaretest.c new file mode 100644 index 0000000000..0c5fb1e55a --- /dev/null +++ b/tests/qemufirmwaretest.c @@ -0,0 +1,70 @@ +#include <config.h> + +#include "testutils.h" +#include "qemu/qemu_firmware.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +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("40-bios.json"); + DO_PARSE_TEST("50-ovmf-sb.json"); + DO_PARSE_TEST("60-ovmf.json"); + DO_PARSE_TEST("70-aavmf.json"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + + +VIR_TEST_MAIN(mymain); -- 2.19.2
On 02/27/19 11:04, Michal Privoznik wrote:
Test firmware description parsing so far.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/qemufirmwaredata/40-bios.json | 35 +++++++++++++ tests/qemufirmwaredata/50-ovmf-sb.json | 36 +++++++++++++ tests/qemufirmwaredata/60-ovmf.json | 35 +++++++++++++ tests/qemufirmwaredata/70-aavmf.json | 35 +++++++++++++ tests/qemufirmwaretest.c | 70 ++++++++++++++++++++++++++ 6 files changed, 220 insertions(+) create mode 100644 tests/qemufirmwaredata/40-bios.json create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/60-ovmf.json create mode 100644 tests/qemufirmwaredata/70-aavmf.json create mode 100644 tests/qemufirmwaretest.c
If the test data files added in this patch are from the examples in QEMU's "docs/interop/firmware.json", I suggest stating so in the commit message, also identifying the QEMU commit that added those examples. In addition, I wonder if the filenames should carry the double-digit priority prefixes at once in this patch. Even if they do, I'm thinking that aavmf shouldn't be ordered (by priority prefix) behind ovmf, given that the qemu targets / machine types they are suitable for are mutually exclusive. Other than that, I have two superficial comments (questions) below, because my knowledge of the libvirt test infrastructure is nonexistent to minimal:
diff --git a/tests/Makefile.am b/tests/Makefile.am index c3f633cee0..d23a8c2812 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -132,6 +132,7 @@ EXTRA_DIST = \ qemuxml2xmloutdata \ qemustatusxml2xmldata \ qemumemlockdata \ + qemufirmwaredata \ secretxml2xmlin \ securityselinuxhelperdata \ securityselinuxlabeldata \ @@ -291,6 +292,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \ qemublocktest \ qemumigparamstest \ qemusecuritytest \ + qemufirmwaretest \ $(NULL) test_helpers += qemucapsprobe test_libraries += libqemumonitortestutils.la \ @@ -698,6 +700,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 \ @@ -711,6 +719,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/40-bios.json b/tests/qemufirmwaredata/40-bios.json new file mode 100644 index 0000000000..137ff70779 --- /dev/null +++ b/tests/qemufirmwaredata/40-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/50-ovmf-sb.json b/tests/qemufirmwaredata/50-ovmf-sb.json new file mode 100644 index 0000000000..c804ac1038 --- /dev/null +++ b/tests/qemufirmwaredata/50-ovmf-sb.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/60-ovmf.json b/tests/qemufirmwaredata/60-ovmf.json new file mode 100644 index 0000000000..5e8a94ae78 --- /dev/null +++ b/tests/qemufirmwaredata/60-ovmf.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/70-aavmf.json b/tests/qemufirmwaredata/70-aavmf.json new file mode 100644 index 0000000000..114d1475a2 --- /dev/null +++ b/tests/qemufirmwaredata/70-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/qemufirmwaretest.c b/tests/qemufirmwaretest.c new file mode 100644 index 0000000000..0c5fb1e55a --- /dev/null +++ b/tests/qemufirmwaretest.c @@ -0,0 +1,70 @@ +#include <config.h> + +#include "testutils.h" +#include "qemu/qemu_firmware.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +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); +}
Can you add a comment to the function about the general idea? Or is this approach common for libvirt tests? I mean, to make heads or tails of this function, I have to decipher what each step does, and what the final comparison compares. It would be easier to read if you explained the steps in a comment, and then we'd only have to verify the steps against that documentation. My other question: when does virJSONValueObjectRemoveKey() fail? I assume it doesn't fail when the key isn't present. Thanks, Laszlo
+ + +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("40-bios.json"); + DO_PARSE_TEST("50-ovmf-sb.json"); + DO_PARSE_TEST("60-ovmf.json"); + DO_PARSE_TEST("70-aavmf.json"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + + +VIR_TEST_MAIN(mymain);
On 2/28/19 10:42 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, Michal Privoznik wrote:
Test firmware description parsing so far.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/qemufirmwaredata/40-bios.json | 35 +++++++++++++ tests/qemufirmwaredata/50-ovmf-sb.json | 36 +++++++++++++ tests/qemufirmwaredata/60-ovmf.json | 35 +++++++++++++ tests/qemufirmwaredata/70-aavmf.json | 35 +++++++++++++ tests/qemufirmwaretest.c | 70 ++++++++++++++++++++++++++ 6 files changed, 220 insertions(+) create mode 100644 tests/qemufirmwaredata/40-bios.json create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/60-ovmf.json create mode 100644 tests/qemufirmwaredata/70-aavmf.json create mode 100644 tests/qemufirmwaretest.c
If the test data files added in this patch are from the examples in QEMU's "docs/interop/firmware.json", I suggest stating so in the commit message, also identifying the QEMU commit that added those examples.
It's a mixture. Some files (50-ovmf-sb.json and 60-ovmf.json) were taken from OVMF package from RHEL-7 and some were taken from firmware.json which has some examples in the comments. I'll put that into the commit message.
In addition, I wonder if the filenames should carry the double-digit priority prefixes at once in this patch.
Even if they do, I'm thinking that aavmf shouldn't be ordered (by priority prefix) behind ovmf, given that the qemu targets / machine types they are suitable for are mutually exclusive.
At this point, the code doesn't care about priority just yet. It is implemented in next patches. But I'm not sure I understand what you mean. You mean that 70-aavmf.json should be renamed to just 'aavmf.json'?
Other than that, I have two superficial comments (questions) below, because my knowledge of the libvirt test infrastructure is nonexistent to minimal:
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c new file mode 100644 index 0000000000..0c5fb1e55a --- /dev/null +++ b/tests/qemufirmwaretest.c @@ -0,0 +1,70 @@ +#include <config.h> + +#include "testutils.h" +#include "qemu/qemu_firmware.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +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); +}
Can you add a comment to the function about the general idea? Or is this approach common for libvirt tests? I mean, to make heads or tails of this function, I have to decipher what each step does, and what the final comparison compares. It would be easier to read if you explained the steps in a comment, and then we'd only have to verify the steps against that documentation.
Okay. Long story short, this test tries to ensure that firmware parsing code does parse given files correctly. qemuFirmwareParse() parses JSON into some internal structre and then qemuFirmwareFormat() formats the structure back into a JSON string. This output should be identical to taking the JSON file and removing "description" and "tags" which are not meant to be parsed.
My other question: when does virJSONValueObjectRemoveKey() fail? I assume it doesn't fail when the key isn't present.
The function removes 'key:pair' from a JSON object. But it has to really be object, not anything else. With our internal JSON APIs it is possible to get poitner to virtually anything within a JSON object. For instance, if you'd have the following document: { "arr": [1,2,3] } it is possible to obtain a pointer to the array [1,2,3]. Obviously, RemoveKey() should fail if such pointer would be pased. But if *p is poitner to the whole JSON document then virJSONValueObjectRemoveKey(p, "arr", NULL) should not fail and leave us with empty document. No, the function doesn't fail if key is not found. Michal
On 03/05/19 15:38, Michal Privoznik wrote:
On 2/28/19 10:42 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, Michal Privoznik wrote:
Test firmware description parsing so far.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/qemufirmwaredata/40-bios.json | 35 +++++++++++++ tests/qemufirmwaredata/50-ovmf-sb.json | 36 +++++++++++++ tests/qemufirmwaredata/60-ovmf.json | 35 +++++++++++++ tests/qemufirmwaredata/70-aavmf.json | 35 +++++++++++++ tests/qemufirmwaretest.c | 70 ++++++++++++++++++++++++++ 6 files changed, 220 insertions(+) create mode 100644 tests/qemufirmwaredata/40-bios.json create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/60-ovmf.json create mode 100644 tests/qemufirmwaredata/70-aavmf.json create mode 100644 tests/qemufirmwaretest.c
If the test data files added in this patch are from the examples in QEMU's "docs/interop/firmware.json", I suggest stating so in the commit message, also identifying the QEMU commit that added those examples.
It's a mixture. Some files (50-ovmf-sb.json and 60-ovmf.json) were taken from OVMF package from RHEL-7 and some were taken from firmware.json which has some examples in the comments. I'll put that into the commit message.
In addition, I wonder if the filenames should carry the double-digit priority prefixes at once in this patch.
Even if they do, I'm thinking that aavmf shouldn't be ordered (by priority prefix) behind ovmf, given that the qemu targets / machine types they are suitable for are mutually exclusive.
At this point, the code doesn't care about priority just yet. It is implemented in next patches. But I'm not sure I understand what you mean. You mean that 70-aavmf.json should be renamed to just 'aavmf.json'?
My first suggestion is to drop the prio prefixes (in this patch only). If you prefer to keep them, then my second suggestion is to give the same prio to the sole aavmf descriptor as to the main (highest prio) ovmf descriptor.
Other than that, I have two superficial comments (questions) below, because my knowledge of the libvirt test infrastructure is nonexistent to minimal:
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c new file mode 100644 index 0000000000..0c5fb1e55a --- /dev/null +++ b/tests/qemufirmwaretest.c @@ -0,0 +1,70 @@ +#include <config.h> + +#include "testutils.h" +#include "qemu/qemu_firmware.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +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); +}
Can you add a comment to the function about the general idea? Or is this approach common for libvirt tests? I mean, to make heads or tails of this function, I have to decipher what each step does, and what the final comparison compares. It would be easier to read if you explained the steps in a comment, and then we'd only have to verify the steps against that documentation.
Okay. Long story short, this test tries to ensure that firmware parsing code does parse given files correctly. qemuFirmwareParse() parses JSON into some internal structre and then qemuFirmwareFormat() formats the structure back into a JSON string. This output should be identical to taking the JSON file and removing "description" and "tags" which are not meant to be parsed.
Ah, that's what I missed, i.e. how virJSONValueObjectRemoveKey() worked.
My other question: when does virJSONValueObjectRemoveKey() fail? I assume it doesn't fail when the key isn't present.
The function removes 'key:pair' from a JSON object. But it has to really be object, not anything else. With our internal JSON APIs it is possible to get poitner to virtually anything within a JSON object. For instance, if you'd have the following document:
{ "arr": [1,2,3] }
it is possible to obtain a pointer to the array [1,2,3]. Obviously, RemoveKey() should fail if such pointer would be pased. But if *p is poitner to the whole JSON document then virJSONValueObjectRemoveKey(p, "arr", NULL) should not fail and leave us with empty document.
No, the function doesn't fail if key is not found.
Thanks for explaining. Laszlo
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 find 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. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 127 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 130 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index e709a5f608..509927b154 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,128 @@ 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; + + 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=%zd", + path, (ssize_t) len); + + if (len == 0) + 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 02/27/19 11:04, 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 find in two or more locations then the most specific
s/find/found/
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.
Please add a QEMU commit reference here as well.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 127 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 130 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index e709a5f608..509927b154 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,128 @@ 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;
(VIR_AUTOFREE() is very unusual to me, so I could be missing lifecycle bugs.)
+ + 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"
You could factor out "qemu/firmware" to a separate macro; you use it three times in total.
+ +int +qemuFirmwareFetchConfigs(char ***firmwares)
Apparently, (char **) is a well known type in libvirt (a "virStringList"). Shouldn't we have an actual typedef for that? It is surprising not to have one, given the other verbose typedefs for example, which basically just say "this is a pointer". I'm also missing some invariants on this type. For example, an empty string list could be represented by a 1-element array that is immediately terminated with a NULL. But on failure this function doesn't produce that, instead it sets *firmwares to NULL (no array at all). Not saying this is wrong, just hard to understand, without docs / typedef.
+{ + 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;
OK, so at this point we have a unique set of filenames (one component only) where each filename (as key) has the highest priority full pathname associated with it. I'd add a comment about this.
+ + 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=%zd", + path, (ssize_t) len);
Sorry about the bike-shedding: - I suggest sticking with one of "'%s'" and "%s", between the error report and the debug message (i.e. be consistent in the use of the apostrophes) - generally speaking, off_t is not safe to cast to ssize_t; e.g. in an ILP32_OFFBIG environment, ssize_t could be 32-bit. The only portable way to print off_t (to my knowledge) is to cast it to intmax_t, and print it with %jd. You can see an example for this in the storageBackendVolZeroSparseFileLocal() function. It seems to expect that the off_t value in question is nonnegative (which is an expectation that I cannot comment upon), but then it correctly casts the value to uintmax_t and prints it with %ju.
+ + if (len == 0) + continue;
Maybe a comment should be added here that the zero size file was (presumably) used to mask the less specific instances of the same file.
+ + 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 */
Looks good to me otherwise. Thanks Laszlo
On 2/28/19 11:21 AM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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 find in two or more locations then the most specific
s/find/found/
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.
Please add a QEMU commit reference here as well.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 127 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 3 + 2 files changed, 130 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index e709a5f608..509927b154 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,128 @@ 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;
(VIR_AUTOFREE() is very unusual to me, so I could be missing lifecycle bugs.)
VIR_AUTOFREE() is just a wrapper over __attribute__((cleanup(free)) which is a way to automagically free the memory once corresponding variable goes out of scope (well, attribute cleanup is more powerful than that as it can call just any function, but we use it only for freeing memory in libvirt).
+ + 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"
You could factor out "qemu/firmware" to a separate macro; you use it three times in total.
+ +int +qemuFirmwareFetchConfigs(char ***firmwares)
Apparently, (char **) is a well known type in libvirt (a "virStringList"). Shouldn't we have an actual typedef for that? It is surprising not to have one, given the other verbose typedefs for example, which basically just say "this is a pointer".
Yeah, we could have something like that. But (my favourite excuse) it is orthogonal to this patch ;-) There is a lot of code that would need fixing after virStringList typedef is introduced. I mean fixing as in replacing char ** with virStringList.
I'm also missing some invariants on this type. For example, an empty string list could be represented by a 1-element array that is immediately terminated with a NULL. But on failure this function doesn't produce that, instead it sets *firmwares to NULL (no array at all).
These two are equivallent from virStringList*() APIs POV. All of our virStringList*() APIs check if passed string list is NULL as the very first step they do.
Not saying this is wrong, just hard to understand, without docs / typedef.
Yep, I understand that.
+{ + 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;
OK, so at this point we have a unique set of filenames (one component only) where each filename (as key) has the highest priority full pathname associated with it. I'd add a comment about this.
Okay.
+ + 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=%zd", + path, (ssize_t) len);
Sorry about the bike-shedding:
- I suggest sticking with one of "'%s'" and "%s", between the error report and the debug message (i.e. be consistent in the use of the apostrophes)
- generally speaking, off_t is not safe to cast to ssize_t; e.g. in an ILP32_OFFBIG environment, ssize_t could be 32-bit. The only portable way to print off_t (to my knowledge) is to cast it to intmax_t, and print it with %jd.
Ah, right. So on 32bits ssize_t can actually be smaller than off_t, since ssize_t is guaranteed to be big enough to address individual bytes in memory (4GiB in this case) and off_t can address just any byt in a file (which can be larger than 4GiB so sizeof(off_t) > sizeof(ssize_t)). Indeed, on my RPi if I print sizeof() with -D_FILE_OFFSET_BITS=64 I get: ssize_t 4 signed off_t 8 signed
You can see an example for this in the storageBackendVolZeroSparseFileLocal() function. It seems to expect that the off_t value in question is nonnegative (which is an expectation that I cannot comment upon), but then it correctly casts the value to uintmax_t and prints it with %ju.
Ah, didn't know that. Thank you for pointing this out.
+ + if (len == 0) + continue;
Maybe a comment should be added here that the zero size file was (presumably) used to mask the less specific instances of the same file.
+ + 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 */
Looks good to me otherwise.
Thanks, Michal
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb.json | 1 + .../etc/qemu/firmware/60-ovmf.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware}/40-bios.json | 0 .../share/qemu/firmware}/50-ovmf-sb.json | 0 .../share/qemu/firmware}/60-ovmf.json | 0 .../share/qemu/firmware}/70-aavmf.json | 0 tests/qemufirmwaretest.c | 69 +++++++++++++++++-- 9 files changed, 66 insertions(+), 5 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/40-bios.json (100%) rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/50-ovmf-sb.json (100%) rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/60-ovmf.json (100%) rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/70-aavmf.json (100%) diff --git a/tests/Makefile.am b/tests/Makefile.am index d23a8c2812..aee078c41c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -703,6 +703,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.json b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb.json new file mode 120000 index 0000000000..30f65574f3 --- /dev/null +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb.json @@ -0,0 +1 @@ +../../../usr/share/qemu/firmware/50-ovmf-sb.json \ No newline at end of file diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf.json b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf.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/40-bios.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json similarity index 100% rename from tests/qemufirmwaredata/40-bios.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json diff --git a/tests/qemufirmwaredata/50-ovmf-sb.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb.json similarity index 100% rename from tests/qemufirmwaredata/50-ovmf-sb.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb.json diff --git a/tests/qemufirmwaredata/60-ovmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf.json similarity index 100% rename from tests/qemufirmwaredata/60-ovmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf.json diff --git a/tests/qemufirmwaredata/70-aavmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json similarity index 100% rename from tests/qemufirmwaredata/70-aavmf.json rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index 0c5fb1e55a..a6c2577709 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 @@ -46,11 +48,65 @@ testParseFormatFW(const void *opaque) } +static int +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED) +{ + VIR_AUTOFREE(char *) fakehome = NULL; + VIR_AUTOPTR(virString) fwList = NULL; + size_t nfwList; + size_t i; + const char *expected[] = { + PREFIX "/share/qemu/firmware/40-bios.json", + SYSCONFDIR "/qemu/firmware/40-ovmf-sb.json", + PREFIX "/share/qemu/firmware/50-ovmf-sb.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, \ @@ -58,13 +114,16 @@ mymain(void) ret = -1; \ } while (0) - DO_PARSE_TEST("40-bios.json"); - DO_PARSE_TEST("50-ovmf-sb.json"); - DO_PARSE_TEST("60-ovmf.json"); - DO_PARSE_TEST("70-aavmf.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/40-bios.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/50-ovmf-sb.json"); + DO_PARSE_TEST("usr/share/qemu/firmware/60-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 02/27/19 11:04, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + .../etc/qemu/firmware/40-ovmf-sb.json | 1 + .../etc/qemu/firmware/60-ovmf.json | 0 .../user/.config/qemu/firmware/10-bios.json | 0 .../share/qemu/firmware}/40-bios.json | 0 .../share/qemu/firmware}/50-ovmf-sb.json | 0 .../share/qemu/firmware}/60-ovmf.json | 0 .../share/qemu/firmware}/70-aavmf.json | 0 tests/qemufirmwaretest.c | 69 +++++++++++++++++-- 9 files changed, 66 insertions(+), 5 deletions(-) create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb.json create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf.json create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/40-bios.json (100%) rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/50-ovmf-sb.json (100%) rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/60-ovmf.json (100%) rename tests/qemufirmwaredata/{ => usr/share/qemu/firmware}/70-aavmf.json (100%)
Right, so I think it makes sense to introduce the double-digit priority prefixes first in this patch, as part of the rename. They matter here, but they don't matter in the previous test case. No other comments for now regarding this patch; I hope that's not a problem. Thanks 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 | 257 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 ++ 2 files changed, 264 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 509927b154..90c611db2d 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" @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares) return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool supportsSecureBoot = 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) + 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) + 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_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + supportsSEV = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + 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) + return false; + + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && + !supportsS4) + return false; + + if (def->os.loader && + def->os.loader->secure == VIR_TRISTATE_BOOL_YES && + !supportsSecureBoot) + return false; + + if (def->sev && + def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && + !supportsSEV) + return false; + + 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 '%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: + VIR_DEBUG("Enabling SMM feature"); + def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON; + 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; +} + + +int +qemuFirmwareFillDomain(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + VIR_AUTOPTR(virString) 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])) { + 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; + } + + + 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 02/27/19 11:04, 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 | 257 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 ++ 2 files changed, 264 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 509927b154..90c611db2d 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" @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares)
return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool supportsSecureBoot = 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) + 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) + 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_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + supportsSEV = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + 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) + return false; + + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && + !supportsS4) + return false; + + if (def->os.loader && + def->os.loader->secure == VIR_TRISTATE_BOOL_YES && + !supportsSecureBoot) + return false;
This check doesn't seem correct. (Also the fact that QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.) The @secure attribute controls whether libvirtd generates the "-global driver=cfi.pflash01,property=secure,value=on" cmdline option. See qemuBuildDomainLoaderCommandLine(). That means that read/write accesses ("programming mode") to the pflash chips will be restricted to guest code that runs in (guest) SMM. In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT. Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM, and irrelevant in itself for the QEMU command line. SECURE_BOOT is only relevant to what firmware interfaces are exposed within the guest. Now, security-wise, there *is* a connection between SECURE_BOOT and REQUIRES_SMM. Namely, it is bad practice (for production uses) for firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See the explanation in the schema JSON. So basically, here's what I suggest: (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. (2) if REQUIRES_SMM is present (regardless of SECURE_BOOT) in the firmware descriptor, then <smm state='on'/> is required under <features>, in the domain config. (3) if SECURE_BOOT is present, but REQUIRES_SMM is absent, in the firmware descriptor, log a big fat warning somewhere, but don't prevent firmware selection or domain startup. There may be valid use cases for this, so we shouldn't block that. No need to log the warning just upon seeing such a firmware descriptor, but do log the warning if the firmware is ultimately selected for a domain. (4) if REQUIRES_SMM is present, but SECURE_BOOT is absent, and the firmware is ultimately selected, log another warning. This is a totally valid (and safe) firmware build, but not overly useful to end-users, so it may not give users what they want.
+ + if (def->sev && + def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && + !supportsSEV) + return false; + + 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 '%s'",
Please log the string "varstore template" instead.
+ 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: + VIR_DEBUG("Enabling SMM feature"); + def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON; + 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; +} + + +int +qemuFirmwareFillDomain(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + VIR_AUTOPTR(virString) 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])) { + 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; + } + + + 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 */
Thanks Laszlo
On 2/28/19 12:15 PM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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 | 257 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 ++ 2 files changed, 264 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 509927b154..90c611db2d 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" @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares)
return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool supportsSecureBoot = 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) + 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) + 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_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + supportsSEV = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + 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) + return false; + + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && + !supportsS4) + return false; + + if (def->os.loader && + def->os.loader->secure == VIR_TRISTATE_BOOL_YES && + !supportsSecureBoot) + return false;
This check doesn't seem correct. (Also the fact that QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.)
[This is exactly why I wanted you to take a look at these patches, because I was almost certain I would get this wrong. Thanks!]
The @secure attribute controls whether libvirtd generates the "-global driver=cfi.pflash01,property=secure,value=on" cmdline option. See qemuBuildDomainLoaderCommandLine(). That means that read/write accesses ("programming mode") to the pflash chips will be restricted to guest code that runs in (guest) SMM.
In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT.
Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM, and irrelevant in itself for the QEMU command line. SECURE_BOOT is only relevant to what firmware interfaces are exposed within the guest.
Now, security-wise, there *is* a connection between SECURE_BOOT and REQUIRES_SMM. Namely, it is bad practice (for production uses) for firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See the explanation in the schema JSON.
So basically, here's what I suggest:
(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.
Ah okay. So @secure should be tied to REQUIRES_SMM. But that leaves SECURE_BOOT unchecked (i.e. unused for finding matching FW description).
(2) if REQUIRES_SMM is present (regardless of SECURE_BOOT) in the firmware descriptor, then <smm state='on'/> is required under <features>, in the domain config.
This is already done in qemuFirmwareEnableFeatures() (towards the end).
(3) if SECURE_BOOT is present, but REQUIRES_SMM is absent, in the firmware descriptor, log a big fat warning somewhere, but don't prevent firmware selection or domain startup. There may be valid use cases for this, so we shouldn't block that. No need to log the warning just upon seeing such a firmware descriptor, but do log the warning if the firmware is ultimately selected for a domain.
(4) if REQUIRES_SMM is present, but SECURE_BOOT is absent, and the firmware is ultimately selected, log another warning. This is a totally valid (and safe) firmware build, but not overly useful to end-users, so it may not give users what they want.
Well, these can be merged into one warning like: REQUIRES_SMM and SECURE_BOOT flags mismatch in $filename Also, I'll have to come up with yet another FW description for my tests that doesn't have REQUIRES_SMM nor SECURE_BOOT to test: <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> </os> But that should be trivial. Michal
On 03/05/19 15:38, Michal Privoznik wrote:
On 2/28/19 12:15 PM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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 | 257 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_firmware.h | 7 ++ 2 files changed, 264 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 509927b154..90c611db2d 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" @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares) return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, + const qemuFirmware *fw) +{ + size_t i; + bool supportsS3 = false; + bool supportsS4 = false; + bool supportsSecureBoot = 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) + 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) + 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_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_AMD_SEV: + supportsSEV = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: + 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) + return false; + + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && + !supportsS4) + return false; + + if (def->os.loader && + def->os.loader->secure == VIR_TRISTATE_BOOL_YES && + !supportsSecureBoot) + return false;
This check doesn't seem correct. (Also the fact that QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.)
[This is exactly why I wanted you to take a look at these patches, because I was almost certain I would get this wrong. Thanks!]
The @secure attribute controls whether libvirtd generates the "-global driver=cfi.pflash01,property=secure,value=on" cmdline option. See qemuBuildDomainLoaderCommandLine(). That means that read/write accesses ("programming mode") to the pflash chips will be restricted to guest code that runs in (guest) SMM.
In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT.
Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM, and irrelevant in itself for the QEMU command line. SECURE_BOOT is only relevant to what firmware interfaces are exposed within the guest.
Now, security-wise, there *is* a connection between SECURE_BOOT and REQUIRES_SMM. Namely, it is bad practice (for production uses) for firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See the explanation in the schema JSON.
So basically, here's what I suggest:
(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.
Ah okay. So @secure should be tied to REQUIRES_SMM. But that leaves SECURE_BOOT unchecked (i.e. unused for finding matching FW description).
That's right. As an alternative, you could change the code so that @secure=='yes' be satisfied by (REQUIRES_SMM && SECURE_BOOT) only. Likely much simpler for the end-user. (Perhaps a bit restrictive for my taste.) I think this mapping/interpretation should be decided by libvirt owners and higher level mgmt app owners. Thanks Laszlo
(2) if REQUIRES_SMM is present (regardless of SECURE_BOOT) in the firmware descriptor, then <smm state='on'/> is required under <features>, in the domain config.
This is already done in qemuFirmwareEnableFeatures() (towards the end).
(3) if SECURE_BOOT is present, but REQUIRES_SMM is absent, in the firmware descriptor, log a big fat warning somewhere, but don't prevent firmware selection or domain startup. There may be valid use cases for this, so we shouldn't block that. No need to log the warning just upon seeing such a firmware descriptor, but do log the warning if the firmware is ultimately selected for a domain.
(4) if REQUIRES_SMM is present, but SECURE_BOOT is absent, and the firmware is ultimately selected, log another warning. This is a totally valid (and safe) firmware build, but not overly useful to end-users, so it may not give users what they want.
Well, these can be merged into one warning like:
REQUIRES_SMM and SECURE_BOOT flags mismatch in $filename
Also, I'll have to come up with yet another FW description for my tests that doesn't have REQUIRES_SMM nor SECURE_BOOT to test:
<os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> </os>
But that should be trivial.
Michal
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
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 cc3a01397c..b25f26f8b0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4157,7 +4157,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 02/27/19 11:04, 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 cc3a01397c..b25f26f8b0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4157,7 +4157,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;
I'm not familiar with the ordering between the code in the previous patch and the code in this patch. What is the order? Do we first validate and then prepare/fill, or vice versa? Also, what happens if the domain config explicitly disables SMM emulation in <features>, but the firmware requires it? See my point (2) in message <https://www.redhat.com/archives/libvir-list/2019-February/msg01669.html>. Will qemuFirmwareEnableFeatures() simply overwrite the setting? I think we should reject the firmware descriptor instead, in qemuFirmwareMatchDomain(). Thanks Laszlo
On 2/28/19 12:40 PM, Laszlo Ersek wrote:
On 02/27/19 11:04, 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 cc3a01397c..b25f26f8b0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4157,7 +4157,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;
I'm not familiar with the ordering between the code in the previous patch and the code in this patch. What is the order? Do we first validate and then prepare/fill, or vice versa?
Yeah, I can probably put this one before 11/15 which enables SMM if needed.
Also, what happens if the domain config explicitly disables SMM emulation in <features>, but the firmware requires it? See my point (2) in message <https://www.redhat.com/archives/libvir-list/2019-February/msg01669.html>. Will qemuFirmwareEnableFeatures() simply overwrite the setting? I think we should reject the firmware descriptor instead, in qemuFirmwareMatchDomain().
Good point. I'll make qemuFirmwareEnableFeatures() fail in that case and thus starting the domain will fail. After all, in point (1) you suggest that for matching FW @secure == REQUIRES_SMM, therefore transitively if @secure='yes' then def->features[SMM] = VIR_TRISTATE_SWITCH_ON. Hence, @secure='yes' and SMM='off' is nonsensical combination. Michal
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 b25f26f8b0..39b606f185 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6905,7 +6905,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
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 | 42 ++++++++++++ .../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, 445 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 aee078c41c..32e318d1b8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -583,7 +583,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..c0925f1e3b --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-secboot.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,smm=on,dump-guest-core=off \ +-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..46a7b1b780 --- /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='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.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-efi.x86_64-latest.args new file mode 100644 index 0000000000..c0925f1e3b --- /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,smm=on,dump-guest-core=off \ +-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.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 dd4f73a5fb..4395c77d69 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 @@ -3004,6 +3016,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 b38cbd6994..d9005f194e 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 Wed, Feb 27, 2019 at 11:04:32AM +0100, Michal Privoznik wrote:
Libvirt allows specifying firmware for domains for quite some time now. However, problem for mgmt applications is that they do not know which firmware to chose as all they see are their paths and from that it's impossible to tell whether one of them supports say secure boot.
This problem was addressed by qemu where Lazslo and Daniel created a document, specification which describes metadata for each individual firmware image. In the description (which itself is a JSON file for easy machine parsing) then it's specified whether the firmware it's describing supports secureboot, s3/s4 states, it it's bios or efi, and so on.
These patches take advantage of that, and even though the description files are not picked up by that many distributions yet, it allows users to not care about putting specific firmware path into their domain XML. It's as easy as:
<os firmware='efi'> <loader secure='yes'/> </os>
Nice, _much_ better than how certain management tools hard-code the path to firmware binaries :-) Thanks for working on this patch series. I'll give them a spin sometime this or early next week.
to have libvirt pick up OVMF image with secure enabled boot (and enabled System Management Mode at the same time).
The metadata specification lives under qemu.git/docs/interop/firmware.json and I highly recommend you go and read it before reviewing (unless you're Laszlo or Daniel in which case you already know what the document says).
As usual, you can find my patches at my github:
[...] -- /kashyap
participants (3)
-
Kashyap Chamarthy -
Laszlo Ersek -
Michal Privoznik