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@xxxxxxxxxx>
> ---
> 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__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list