On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:

Currently qemu.conf contains a nvram parameter which lists firmware code files and the corresponding nvram file path. We need to know which architecture and features are associated with each firmware file for future enhancement. This extends the syntax in a backwards compatible manner to record this info.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu.conf | 14 ++++-- src/qemu/test_libvirtd_qemu.aug.in | 6 +-- src/util/virfirmware.c | 94 ++++++++++++++++++++++++++++++++++---- src/util/virfirmware.h | 7 +++ 5 files changed, 105 insertions(+), 17 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 68d0ea9..a2f0b83 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1628,6 +1628,7 @@ virFirewallStartTransaction;

# util/virfirmware.h +virFirmwareFind; virFirmwareFreeList; virFirmwareParse; virFirmwareParseList; diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e4c2aae..56a0e55 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -595,16 +595,22 @@ # using this master file as image. Each UEFI firmware can, # however, have different variables store. Therefore the nvram is # a list of strings when a single item is in form of: -# ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}. +# +# ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}:${ARCH}[:${FEATURE}:...]. +# +# Current valid features include: +# +# 'secboot' - the firmware has secure boot enabled +# # Later, when libvirt creates per domain variable store, this list is # searched for the master image. The UEFI firmware can be called # differently for different guest architectures. For instance, it's OVMF # for x86_64 and i686, but it's AAVMF for aarch64. The libvirt default # follows this scheme. #nvram = [ -# "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd", -# "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd", -# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" +# "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64", +# "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64:secboot", +# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd:aarch64" #]

# The backend to use for handling stdout/stderr output from diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index cd162ae..b78a0b3 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -83,8 +83,8 @@ module Test_libvirtd_qemu = { "migration_port_max" = "49215" } { "log_timestamp" = "0" } { "nvram" - { "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" } - { "2" = "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd" } - { "3" = "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" } + { "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64" } + { "2" = "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64:secboot" } + { "3" = "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd:aarch64" } } { "stdio_handler" = "logd" } diff --git a/src/util/virfirmware.c b/src/util/virfirmware.c index 6b20c06..117e8ef 100644 --- a/src/util/virfirmware.c +++ b/src/util/virfirmware.c @@ -61,30 +61,83 @@ int virFirmwareParse(const char *str, virFirmwarePtr firmware) { int ret = -1; - char **token; + char **token, **tmp; + size_t ntoken = 0; + int arch;

- if (!(token = virStringSplit(str, ":", 0))) + if (!(tmp = token = virStringSplit(str, ":", 0)))

virStringSplitCount would return the number allow the usage of a for loop below rather than counting ntoken...

goto cleanup;

- if (token[0]) { - virSkipSpaces((const char **) &token[0]); - if (token[1]) - virSkipSpaces((const char **) &token[1]); + while (tmp && *tmp) { + virSkipSpaces((const char **) &tmp); + if (STREQ(*tmp, "")) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Invalid nvram format for '%s', token " + "must not be the empty string"), + str); + goto cleanup; + } + ntoken++; + tmp++; }

- /* Exactly two tokens are expected */ - if (!token[0] || !token[1] || token[2] || - STREQ(token[0], "") || STREQ(token[1], "")) { + if (ntoken < 2 || ntoken > 4) { virReportError(VIR_ERR_CONF_SYNTAX, - _("Invalid nvram format: '%s'"), + _("Invalid nvram format for '%s', expected " + "CODE-PATH:NVRAM-PATH:[ARCH:[FEATURE,...]]"),

s/:[ARCH:[FEATURE/[:ARCH[:FEATURE The ending colon is not optional and if used had better have ARCH or FEATURE after it

str); goto cleanup; }

+ if (ntoken > 2) { + if ((arch = virArchFromString(token[2])) < 0) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Unknown arch in nvram config '%s'"), + str); + goto cleanup; + } + firmware->arch = arch; + } else { + if (strstr(token[0], "OVMF")) { + firmware->arch = VIR_ARCH_X86_64;

Could we get ourselves in trouble here since OVMF can be used for i686 and x86_64? Maybe we should just document the defaults if not provided above in qemu.conf?

+ } else if (strstr(token[1], "AVMF")) { + firmware->arch = VIR_ARCH_AARCH64; + } else { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Cannot guest arch for nvram config '%s', " + "please specify it explicitly"),

Not sure if that's meant to be "cannot guess arch" or "cannot get guest arch"

+ str); + goto cleanup; + } + } + if (VIR_STRDUP(firmware->name, token[0]) < 0 || VIR_STRDUP(firmware->nvram, token[1]) < 0) goto cleanup;

+ /* Remaining tokens are feature flags */ + if (ntoken > 3) { + tmp = token + 3; + while (*tmp) { + if (STREQ(*tmp, "secboot")) { + firmware->secboot = true; + } else { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Unknown feature flag in nvram config '%s'"), + str);

Could also print the unknown feature

+ goto cleanup; + } + tmp++; + } + } else { + if (strstr(firmware->name, "secboot")) + firmware->secboot = true;

An undocumented default... IOW Should we describe in the .conf file that if not supplied, an assumption will be made if secboot is found in the firmware name? Not really that important

+ } + + VIR_DEBUG("Parsed firmware code='%s' nvram='%s' arch='%s' secboot='%d'", + firmware->name, firmware->nvram, + virArchToString(firmware->arch), firmware->secboot); + ret = 0; cleanup: virStringFreeList(token); @@ -135,3 +188,24 @@ virFirmwareParseList(const char *list, virStringFreeList(token); return ret; } + + +virFirmwarePtr virFirmwareFind(virFirmwarePtr *firmwares,

Usually see two lines: virFirmwarePtr virFirmwareFind(virFirmwarePtr *firmwares, (and adjust following indents) ACK with at least the error messages cleaned up and formatting stuff. Your call on using virStringSplitCount and how/if to document those defaults. John

+ size_t nfirmwares, + virArch arch, + bool secboot) +{ + size_t i; + + for (i = 0; i < nfirmwares; i++) { + if (firmwares[i]->arch == arch && + firmwares[i]->secboot == secboot) { + return firmwares[i]; + } + } + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cannot find a firmware for arch %s with secboot=%d"), + virArchToString(arch), secboot); + return NULL; +} diff --git a/src/util/virfirmware.h b/src/util/virfirmware.h index 682a865..f3ac36a 100644 --- a/src/util/virfirmware.h +++ b/src/util/virfirmware.h @@ -24,11 +24,14 @@ # define __VIR_FIRMWARE_H__

# include "internal.h" +# include "virarch.h"

typedef struct _virFirmware virFirmware; typedef virFirmware *virFirmwarePtr;

struct _virFirmware { + virArch arch; + bool secboot; char *name; char *nvram; }; @@ -47,5 +50,9 @@ virFirmwareParseList(const char *list, size_t *nfirmwares) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);

+virFirmwarePtr virFirmwareFind(virFirmwarePtr *firmwares, + size_t nfirmwares, + virArch arch, + bool secboot);

#endif /* __VIR_FIRMWARE_H__ */

On Wed, Oct 19, 2016 at 01:07:25PM +0100, Richard W.M. Jones wrote:

On Mon, Oct 03, 2016 at 04:49:47PM +0100, Daniel P. Berrange wrote:

...

--- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -595,16 +595,22 @@ # using this master file as image. Each UEFI firmware can, # however, have different variables store. Therefore the nvram is # a list of strings when a single item is in form of: -# ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}. +# +# ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}:${ARCH}[:${FEATURE}:...]. +# +# Current valid features include: +# +# 'secboot' - the firmware has secure boot enabled +# # Later, when libvirt creates per domain variable store, this list is # searched for the master image. The UEFI firmware can be called # differently for different guest architectures. For instance, it's OVMF # for x86_64 and i686, but it's AAVMF for aarch64. The libvirt default # follows this scheme. #nvram = [ -# "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd", -# "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd", -# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" +# "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64", +# "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd:x86_64:secboot", +# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd:aarch64"

This is all good, and could remove a duplicate copy of this database which is currently stored in libguestfs & virt-v2v:

https://github.com/libguestfs/libguestfs/blob/master/generator/uefi.ml#L30

The flags (arch, secboot) even precisely match the ones we currently need to store.

Unfortunately it's a case of so near and yet so far. You're proposing this essentially static and non-secret data be stored in /etc/libvirt/qemu.conf, which is not readable as non-root. virt-v2v (which can run as non-root) would still need to store a duplicate copy of the data.

What does v2v need the mapping data for ? Any use case needs to be addressed via the APIs, not having apps poke at libvirt private config files.

I don't see any need for config files to default to unreadable, it's just security through obscurity (and not even obscure), but assuming that isn't going to change, please put this into a different file which can be read as non-root. There is literally nothing possibly secret about it, it's just the location of some files.

Even if the config files were readable, this data is not going to be present in the config file by default - it'll only be there if the admin has needed to override libvirts builtin default value. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:

Currently if you want to enable UEFI firmware for a guest you need to know about the hypervisor platform specific firmware path. This does not even work for all platforms, as hypervisors like VMWare don't expose UEFI as a path, they just have a direct config option for turning it on or off.

This adds ability to use the much simpler:

<loader firmware="uefi|bios"/>

to choose the different firmware types.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- docs/formatdomain.html.in | 9 +++++- docs/schemas/domaincommon.rng | 12 +++++++- src/conf/domain_conf.c | 70 ++++++++++++++++++++++++++++++++++++++----- src/conf/domain_conf.h | 11 +++++++ 4 files changed, 93 insertions(+), 9 deletions(-)

The xml2xml tests from patch 3 could move here, but at least it exists..

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7008005..b8e9315 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -143,7 +143,14 @@ <code>pflash</code>. Moreover, some firmwares may implement the Secure boot feature. Attribute <code>secure</code> can be used then to control it. - <span class="since">Since 2.1.0</span></dd> + <span class="since">Since 2.1.0</span>. The <code>firmware</code> + attribute can be used to request a specific type of firmware image + based on its common name, accepting the values <code>uefi</code> + and <code>bios</code>. <span class="since">Since 2.4.0</span>. When + <code>firmware</code> is set, it is not neccessary to provide any

necessary

+ path for the loader, nor set the <code>type</code> attribute or + <code>nvram</code> elements, as they will be automatically set + to the hypervisor specific default values.</dd>

So, if the firmware attribute isn't set, then is path is required? It seems to only ever be defaulted in patch 3 in the post parse.

<dt><code>nvram</code></dt> <dd>Some UEFI firmwares may want to use a non-volatile memory to store some variables. In the host, this is represented as a file and the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6eeb4e9..197b542 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,7 +276,17 @@ </choice> </attribute> </optional> - <ref name="absFilePath"/> + <optional> + <attribute name="firmware"> + <choice> + <value>bios</value> + <value>uefi</value> + </choice> + </attribute> + </optional> + <optional> + <ref name="absFilePath"/> + </optional> </element> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7972a4e..054be94 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -835,6 +835,12 @@ VIR_ENUM_IMPL(virDomainLoader, "rom", "pflash")

+VIR_ENUM_IMPL(virDomainLoaderFirmware, + VIR_DOMAIN_LOADER_FIRMWARE_LAST, + "default", + "bios", + "uefi") + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob) @@ -15539,17 +15545,36 @@ virDomainLoaderDefParseXML(xmlNodePtr node, char *readonly_str = NULL; char *secure_str = NULL; char *type_str = NULL; + char *firmware_str = NULL;

readonly_str = virXMLPropString(node, "readonly"); secure_str = virXMLPropString(node, "secure"); type_str = virXMLPropString(node, "type"); + firmware_str = virXMLPropString(node, "firmware"); loader->path = (char *) xmlNodeGetContent(node);

- if (readonly_str && - (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { - virReportError(VIR_ERR_XML_DETAIL, - _("unknown readonly value: %s"), readonly_str); - goto cleanup; + if (loader->path && STREQ(loader->path, "")) + VIR_FREE(loader->path);

STREQ_NULLABLE works too. Is it 'undocumented' feature of having "" for path mean some sort of default? It becomes more important in the next patch. But this does follow the schema w/r/t path being optional I suppose. ACK in principle - your call on the loader->path question. John

+ + if (firmware_str) { + int firmware; + if ((firmware = virDomainLoaderFirmwareTypeFromString(firmware_str)) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown firmware value: %s"), firmware_str); + goto cleanup; + } + loader->firmware = firmware; + } + + if (readonly_str) { + if ((loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown readonly value: %s"), readonly_str); + goto cleanup; + } + } else { + if (loader->firmware == VIR_DOMAIN_LOADER_FIRMWARE_UEFI) + loader->readonly = VIR_TRISTATE_SWITCH_ON; }

if (secure_str && @@ -15567,6 +15592,29 @@ virDomainLoaderDefParseXML(xmlNodePtr node, goto cleanup; } loader->type = type; + + switch (loader->firmware) { + case VIR_DOMAIN_LOADER_FIRMWARE_UEFI: + if (loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("UEFI firmware must use pflash type")); + goto cleanup; + } + break; + case VIR_DOMAIN_LOADER_FIRMWARE_BIOS: + if (loader->type != VIR_DOMAIN_LOADER_TYPE_ROM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("BIOS firmware must use ROM type")); + goto cleanup; + } + break; + case VIR_DOMAIN_LOADER_FIRMWARE_DEFAULT: + case VIR_DOMAIN_LOADER_FIRMWARE_LAST: + break; + } + } else { + if (loader->firmware == VIR_DOMAIN_LOADER_FIRMWARE_UEFI) + loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH; }

ret = 0; @@ -15574,6 +15622,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node, VIR_FREE(readonly_str); VIR_FREE(secure_str); VIR_FREE(type_str); + VIR_FREE(firmware_str); return ret; }

@@ -22872,18 +22921,25 @@ virDomainLoaderDefFormat(virBufferPtr buf, const char *readonly = virTristateBoolTypeToString(loader->readonly); const char *secure = virTristateBoolTypeToString(loader->secure); const char *type = virDomainLoaderTypeToString(loader->type); + const char *firmware = virDomainLoaderFirmwareTypeToString(loader->firmware);

virBufferAddLit(buf, "<loader");

+ if (loader->firmware) + virBufferAsprintf(buf, " firmware='%s'", firmware); + if (loader->readonly) virBufferAsprintf(buf, " readonly='%s'", readonly);

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); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 561a179..204c330 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1724,6 +1724,16 @@ struct _virDomainBIOSDef { };

typedef enum { + VIR_DOMAIN_LOADER_FIRMWARE_DEFAULT = 0, + VIR_DOMAIN_LOADER_FIRMWARE_BIOS, + VIR_DOMAIN_LOADER_FIRMWARE_UEFI, + + VIR_DOMAIN_LOADER_FIRMWARE_LAST +} virDomainLoaderFirmware; + +VIR_ENUM_DECL(virDomainLoaderFirmware) + +typedef enum { VIR_DOMAIN_LOADER_TYPE_ROM = 0, VIR_DOMAIN_LOADER_TYPE_PFLASH,

@@ -1735,6 +1745,7 @@ VIR_ENUM_DECL(virDomainLoader) typedef struct _virDomainLoaderDef virDomainLoaderDef; typedef virDomainLoaderDef *virDomainLoaderDefPtr; struct _virDomainLoaderDef { + virDomainLoaderFirmware firmware; char *path; int readonly; /* enum virTristateBool */ virDomainLoader type;

On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:

This wires up the domain XML post-parse callback so that it can fill in the loader/nvram paths, when seeing the "firmware" attribute set.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 6 ++- src/qemu/qemu_conf.c | 12 ++--- src/qemu/qemu_conf.h | 7 +++ src/qemu/qemu_domain.c | 60 +++++++++++++++++++--- .../qemuxml2argv-bios-firmware.args | 26 ++++++++++ .../qemuxml2argv-bios-firmware.xml | 41 +++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-bios-firmware.xml | 48 +++++++++++++++++ tests/qemuxml2xmltest.c | 1 + tests/testutilsqemu.c | 30 ++++++++++- 10 files changed, 214 insertions(+), 18 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-firmware.xml

The xml2xml tests could go in patch2 I suppose, but they're fine here too - at least they're provided. Could add that empty path for bios/rom type test too if you felt so compelled.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7e52963..a7529bf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8927,8 +8927,10 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,

switch ((virDomainLoader) loader->type) { case VIR_DOMAIN_LOADER_TYPE_ROM: - virCommandAddArg(cmd, "-bios"); - virCommandAddArg(cmd, loader->path); + if (loader->path) { + virCommandAddArg(cmd, "-bios"); + virCommandAddArg(cmd, loader->path);

If !loader->path then -bios has no path? Is that OK from QEMU?

+ } break;

case VIR_DOMAIN_LOADER_TYPE_PFLASH: diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 635fa27..879e2aa 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -124,13 +124,6 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) }

-#define VIR_QEMU_OVMF_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.fd" -#define VIR_QEMU_OVMF_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd" -#define VIR_QEMU_OVMF_SEC_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.secboot.fd" -#define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd" -#define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd" -#define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd" - virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { virQEMUDriverConfigPtr cfg; @@ -334,6 +327,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) VIR_STRDUP(cfg->firmwares[2]->name, VIR_QEMU_OVMF_SEC_LOADER_PATH) < 0 || VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) < 0) goto error; + + cfg->firmwares[0]->arch = VIR_ARCH_AARCH64; + cfg->firmwares[1]->arch = VIR_ARCH_X86_64; + cfg->firmwares[2]->arch = VIR_ARCH_X86_64; + cfg->firmwares[2]->secboot = true; #endif

return cfg; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d32689a..2f0c91f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -66,6 +66,13 @@ typedef virQEMUDriver *virQEMUDriverPtr; typedef struct _virQEMUDriverConfig virQEMUDriverConfig; typedef virQEMUDriverConfig *virQEMUDriverConfigPtr;

+# define VIR_QEMU_OVMF_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.fd" +# define VIR_QEMU_OVMF_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd" +# define VIR_QEMU_OVMF_SEC_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.secboot.fd" +# define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd" +# define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd" +# define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd" + /* Main driver config. The data in these object * instances is immutable, so can be accessed * without locking. Threads must, however, hold diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9b1a32e..3badd38 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2346,13 +2346,59 @@ 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 (def->os.loader) { + switch (def->os.loader->firmware) { + case VIR_DOMAIN_LOADER_FIRMWARE_DEFAULT: + break;

Again, is having !def->os.loader.path OK here? or will it be a problem? It seems from qemu-kvm --help it will be: -bios file set the filename for the BIOS

+ + case VIR_DOMAIN_LOADER_FIRMWARE_BIOS: + if (def->os.arch != VIR_ARCH_I686 && + def->os.arch != VIR_ARCH_X86_64) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("BIOS firmware only supported on i686/x86_64")); + goto cleanup; + } + break; + + case VIR_DOMAIN_LOADER_FIRMWARE_UEFI: + if (def->os.arch != VIR_ARCH_X86_64 && + def->os.arch != VIR_ARCH_AARCH64) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("BIOS firmware only supported on x86_64/i686"));

UEFI on x86_64/aarch Could also use the virArchToString for both failure messages to be precise. ACK w/ fixed error message and handling that empty -bios arg. John

+ goto cleanup; + } + + if (def->os.loader->path == NULL) { + virFirmwarePtr firmware; + + if (!(firmware = virFirmwareFind(cfg->firmwares, + cfg->nfirmwares, + def->os.arch, + def->os.loader->secure == + VIR_TRISTATE_SWITCH_ON))) + goto cleanup; + + if (VIR_STRDUP(def->os.loader->path, + firmware->name) < 0) + goto cleanup; + + if (def->os.loader->templt == NULL && + VIR_STRDUP(def->os.loader->templt, + firmware->nvram) < 0) + goto cleanup; + } + + case VIR_DOMAIN_LOADER_FIRMWARE_LAST: + break; + } + + if (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; + } }

/* check for emulator and create a default one if needed */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.args b/tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.args new file mode 100644 index 0000000..e10f4ec --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name test-bios \ +-S \ +-M pc \ +-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,\ +readonly=on \ +-drive file=/tmp/nvram/test-bios_VARS.fd,if=pflash,format=raw,unit=1 \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-test-bios/monitor.sock,server,nowait \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-serial pty \ +-device usb-tablet,id=input0,bus=usb.0,port=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.xml b/tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.xml new file mode 100644 index 0000000..cea1efc --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-firmware.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader firmware='uefi'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 903276d..5a45f17 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -727,6 +727,7 @@ mymain(void) QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_SMM_OPT, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("bios-firmware", NONE); DO_TEST("clock-utc", QEMU_CAPS_NODEFCONFIG); DO_TEST("clock-localtime", NONE); DO_TEST("clock-localtime-basis-localtime", QEMU_CAPS_RTC); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-firmware.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-firmware.xml new file mode 100644 index 0000000..624c60f --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-firmware.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader firmware='uefi' readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/tmp/nvram/test-bios_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index fb05c85..5f94c26 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -808,6 +808,7 @@ mymain(void)

DO_TEST("bios-nvram", NONE); DO_TEST("bios-nvram-os-interleave", NONE); + DO_TEST("bios-firmware", NONE);

DO_TEST("tap-vhost", NONE); DO_TEST("tap-vhost-incorrect", NONE); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index e66903a..3492f28 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -552,21 +552,24 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary, int qemuTestDriverInit(virQEMUDriver *driver) { virSecurityManagerPtr mgr = NULL; + virQEMUDriverConfigPtr cfg;

memset(driver, 0, sizeof(*driver));

if (virMutexInit(&driver->lock) < 0) return -1;

- driver->config = virQEMUDriverConfigNew(false); + cfg = driver->config = virQEMUDriverConfigNew(false); if (!driver->config) goto error;

/* Overwrite some default paths so it's consistent for tests. */ VIR_FREE(driver->config->libDir); VIR_FREE(driver->config->channelTargetDir); + VIR_FREE(driver->config->nvramDir); if (VIR_STRDUP(driver->config->libDir, "/tmp/lib") < 0 || - VIR_STRDUP(driver->config->channelTargetDir, "/tmp/channel") < 0) + VIR_STRDUP(driver->config->channelTargetDir, "/tmp/channel") < 0 || + VIR_STRDUP(driver->config->nvramDir, "/tmp/nvram") < 0) goto error;

driver->caps = testQemuCapsInit(); @@ -592,6 +595,29 @@ int qemuTestDriverInit(virQEMUDriver *driver) if (!(driver->securityManager = virSecurityManagerNewStack(mgr))) goto error;

+ /* The default firmware list may be changed by configure arg, + * so we must clear it and setup a fixed firmware list for testing */ + virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); + if (VIR_ALLOC_N(cfg->firmwares, 3) < 0) + goto error; + cfg->nfirmwares = 3; + if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 0 || + VIR_ALLOC(cfg->firmwares[2]) < 0) + goto error; + + if (VIR_STRDUP(cfg->firmwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[0]->nvram, VIR_QEMU_AAVMF_NVRAM_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[2]->name, VIR_QEMU_OVMF_SEC_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) < 0) + goto error; + + cfg->firmwares[0]->arch = VIR_ARCH_AARCH64; + cfg->firmwares[1]->arch = VIR_ARCH_X86_64; + cfg->firmwares[2]->arch = VIR_ARCH_X86_64; + cfg->firmwares[2]->secboot = true; + return 0;

error: