[libvirt] [PATCH v4 0/2] Allow UEFI paths to be specified at compile time
diff to v3: - basically the patch is split into two patches - dropped shell script in to configure.ac Michal Privoznik (2): qemu: Allow UEFI paths to be specified at compile time qemu: Add AAVMF to the list of known UEFIs configure.ac | 12 ++++ src/qemu/qemu.conf | 12 +++- src/qemu/qemu_conf.c | 68 +++++++++++++++++++--- src/qemu/test_libvirtd_qemu.aug.in | 1 + .../domaincaps-qemu_1.6.50-1.xml | 1 + tests/domaincapstest.c | 16 ++--- 6 files changed, 93 insertions(+), 17 deletions(-) -- 2.0.5
Up until now there are just two ways how to specify UEFI paths to libvirt. The first one is editing qemu.conf, the other is editing qemu_conf.c and recompile which is not that fancy. So, new configure option is introduced: --with-loader-nvram which takes a list of pairs of UEFI firmware and NVRAM store. This way, the compiled in defaults can be passed during compile time without need to change the code itself. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- configure.ac | 12 ++++++++++++ src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/domaincapstest.c | 15 ++++++++------- 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/configure.ac b/configure.ac index f370475..7153fa5 100644 --- a/configure.ac +++ b/configure.ac @@ -2789,6 +2789,18 @@ AC_ARG_WITH([default-editor], [DEFAULT_EDITOR=vi]) AC_DEFINE_UNQUOTED([DEFAULT_EDITOR], ["$DEFAULT_EDITOR"], [Default editor to use]) +AC_ARG_WITH([loader-nvram], + [AS_HELP_STRING([--with-loader-nvram], + [Pass list of pairs of <loader>:<nvram> paths. Both + pairs and list items are separated by a colon. + @<:default=paths to OVMF and its clones@:>@])], + [if test "$withval" = "no"; then + withval="" + fi + AC_DEFINE_UNQUOTED([DEFAULT_LOADER_NVRAM], + ["$withval"], + [List of laoder:nvram pairs])]) + # Some GNULIB base64 symbols clash with a kerberos library AC_DEFINE_UNQUOTED([isbase64],[libvirt_gl_isbase64],[Hack to avoid symbol clash]) AC_DEFINE_UNQUOTED([base64_encode],[libvirt_gl_base64_encode],[Hack to avoid symbol clash]) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a24c5c5..3e51b49 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,6 +107,49 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); } + +static int ATTRIBUTE_UNUSED +virQEMUDriverConfigLoaderNVRAMParse(virQEMUDriverConfigPtr cfg, + const char *list) +{ + int ret = -1; + char **token; + size_t i; + + if (!(token = virStringSplit(list, ":", 0))) + goto cleanup; + + for (i = 0; token[i]; i += 2) { + if (!token[i] || !token[i + 1] || + STREQ(token[i], "") || STREQ(token[i + 1], "")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid --with-loader-nvram list: %s"), + list); + goto cleanup; + } + } + + if (i) { + if (VIR_ALLOC_N(cfg->loader, (i + 1) / 2) < 0 || + VIR_ALLOC_N(cfg->nvram, (i + 1) / 2) < 0) + goto cleanup; + cfg->nloader = (i + 1) / 2; + + while (i) { + if (VIR_STRDUP(cfg->loader[(i - 1) / 2 ], token[i - 2]) < 0 || + VIR_STRDUP(cfg->nvram[(i - 1) / 2], token[i - 1]) < 0) + goto cleanup; + i -= 2; + } + } + + ret = 0; + cleanup: + virStringFreeList(token); + return ret; +} + + #define VIR_QEMU_LOADER_FILE_PATH "/usr/share/OVMF/OVMF_CODE.fd" #define VIR_QEMU_NVRAM_FILE_PATH "/usr/share/OVMF/OVMF_VARS.fd" @@ -258,6 +301,12 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->logTimestamp = true; +#ifdef DEFAULT_LOADER_NVRAM + if (virQEMUDriverConfigLoaderNVRAMParse(cfg, DEFAULT_LOADER_NVRAM) < 0) + goto error; + +#else + if (VIR_ALLOC_N(cfg->loader, 1) < 0 || VIR_ALLOC_N(cfg->nvram, 1) < 0) goto error; @@ -266,6 +315,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (VIR_STRDUP(cfg->loader[0], VIR_QEMU_LOADER_FILE_PATH) < 0 || VIR_STRDUP(cfg->nvram[0], VIR_QEMU_NVRAM_FILE_PATH) < 0) goto error; +#endif return cfg; diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 70d2ef3..5ec03a4 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -105,6 +105,7 @@ fillQemuCaps(virDomainCapsPtr domCaps, struct fillQemuCapsData *data = (struct fillQemuCapsData *) opaque; virQEMUCapsPtr qemuCaps = data->qemuCaps; virQEMUDriverConfigPtr cfg = data->cfg; + virDomainCapsLoaderPtr loader = &domCaps->os.loader; if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps, cfg->loader, cfg->nloader) < 0) @@ -122,14 +123,14 @@ fillQemuCaps(virDomainCapsPtr domCaps, /* Moreover, as of f05b6a918e28 we are expecting to see * OVMF_CODE.fd file which may not exists everywhere. */ - if (!domCaps->os.loader.values.nvalues) { - virDomainCapsLoaderPtr loader = &domCaps->os.loader; + while (loader->values.nvalues) + VIR_FREE(loader->values.values[--loader->values.nvalues]); + + if (fillStringValues(&loader->values, + "/usr/share/OVMF/OVMF_CODE.fd", + NULL) < 0) + return -1; - if (fillStringValues(&loader->values, - "/usr/share/OVMF/OVMF_CODE.fd", - NULL) < 0) - return -1; - } return 0; } #endif /* WITH_QEMU */ -- 2.0.5
On Wed, Jan 21, 2015 at 08:21:28PM +0100, Michal Privoznik wrote:
Up until now there are just two ways how to specify UEFI paths to libvirt. The first one is editing qemu.conf, the other is editing qemu_conf.c and recompile which is not that fancy. So, new configure option is introduced: --with-loader-nvram which takes a list of pairs of UEFI firmware and NVRAM store. This way, the compiled in defaults can be passed during compile time without need to change the code itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- configure.ac | 12 ++++++++++++ src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/domaincapstest.c | 15 ++++++++------- 3 files changed, 70 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac index f370475..7153fa5 100644 --- a/configure.ac +++ b/configure.ac @@ -2789,6 +2789,18 @@ AC_ARG_WITH([default-editor], [DEFAULT_EDITOR=vi]) AC_DEFINE_UNQUOTED([DEFAULT_EDITOR], ["$DEFAULT_EDITOR"], [Default editor to use])
+AC_ARG_WITH([loader-nvram], + [AS_HELP_STRING([--with-loader-nvram], + [Pass list of pairs of <loader>:<nvram> paths. Both + pairs and list items are separated by a colon. + @<:default=paths to OVMF and its clones@:>@])], + [if test "$withval" = "no"; then + withval="" + fi + AC_DEFINE_UNQUOTED([DEFAULT_LOADER_NVRAM], + ["$withval"], + [List of laoder:nvram pairs])]) + # Some GNULIB base64 symbols clash with a kerberos library AC_DEFINE_UNQUOTED([isbase64],[libvirt_gl_isbase64],[Hack to avoid symbol clash]) AC_DEFINE_UNQUOTED([base64_encode],[libvirt_gl_base64_encode],[Hack to avoid symbol clash]) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a24c5c5..3e51b49 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,6 +107,49 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); }
+ +static int ATTRIBUTE_UNUSED +virQEMUDriverConfigLoaderNVRAMParse(virQEMUDriverConfigPtr cfg, + const char *list) +{ + int ret = -1; + char **token; + size_t i; + + if (!(token = virStringSplit(list, ":", 0))) + goto cleanup; + + for (i = 0; token[i]; i += 2) { + if (!token[i] || !token[i + 1] ||
Double check for "token[i]"; it'd be easier to do: for (i = 0; token[i] && token[i + 1]; i += 2) { ... }
+ STREQ(token[i], "") || STREQ(token[i + 1], "")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid --with-loader-nvram list: %s"), + list); + goto cleanup;
But erroring out here is maybe even more ugly than that bash parsing in configure.ac :( There really is no other option to check this built-time? Can we at least do something like: l="$(echo "$with_loader_nvram" | tr ':' '\n' | wc -l)" test $((l % 2)) -eq 0 || echo ERROR this only checks that there's even number of parameters when split by ':', but it works in dash (and csh should be fine too, but I haven't tested that). And the only thing you were checking was the number of parameters, so this has the same meaning and it's done compile-time.
+ } + } + + if (i) { + if (VIR_ALLOC_N(cfg->loader, (i + 1) / 2) < 0 || + VIR_ALLOC_N(cfg->nvram, (i + 1) / 2) < 0) + goto cleanup; + cfg->nloader = (i + 1) / 2; + + while (i) { + if (VIR_STRDUP(cfg->loader[(i - 1) / 2 ], token[i - 2]) < 0 || + VIR_STRDUP(cfg->nvram[(i - 1) / 2], token[i - 1]) < 0)
You are not checking whether token[i] is '\0', but I don't know whether that breaks later in the code or not. Anyway, if something like this happens, I think working around that and just printing a warning would be better. You can behave like if the file does not exist -- we don't quit the daemon/driver in that case, do we?
On 26.01.2015 10:34, Martin Kletzander wrote:
On Wed, Jan 21, 2015 at 08:21:28PM +0100, Michal Privoznik wrote:
Up until now there are just two ways how to specify UEFI paths to libvirt. The first one is editing qemu.conf, the other is editing qemu_conf.c and recompile which is not that fancy. So, new configure option is introduced: --with-loader-nvram which takes a list of pairs of UEFI firmware and NVRAM store. This way, the compiled in defaults can be passed during compile time without need to change the code itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- configure.ac | 12 ++++++++++++ src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/domaincapstest.c | 15 ++++++++------- 3 files changed, 70 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac index f370475..7153fa5 100644 --- a/configure.ac +++ b/configure.ac @@ -2789,6 +2789,18 @@ AC_ARG_WITH([default-editor], [DEFAULT_EDITOR=vi]) AC_DEFINE_UNQUOTED([DEFAULT_EDITOR], ["$DEFAULT_EDITOR"], [Default editor to use])
+AC_ARG_WITH([loader-nvram], + [AS_HELP_STRING([--with-loader-nvram], + [Pass list of pairs of <loader>:<nvram> paths. Both + pairs and list items are separated by a colon. + @<:default=paths to OVMF and its clones@:>@])], + [if test "$withval" = "no"; then + withval="" + fi + AC_DEFINE_UNQUOTED([DEFAULT_LOADER_NVRAM], + ["$withval"], + [List of laoder:nvram pairs])]) + # Some GNULIB base64 symbols clash with a kerberos library AC_DEFINE_UNQUOTED([isbase64],[libvirt_gl_isbase64],[Hack to avoid symbol clash]) AC_DEFINE_UNQUOTED([base64_encode],[libvirt_gl_base64_encode],[Hack to avoid symbol clash]) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a24c5c5..3e51b49 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,6 +107,49 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); }
+ +static int ATTRIBUTE_UNUSED +virQEMUDriverConfigLoaderNVRAMParse(virQEMUDriverConfigPtr cfg, + const char *list) +{ + int ret = -1; + char **token; + size_t i; + + if (!(token = virStringSplit(list, ":", 0))) + goto cleanup; + + for (i = 0; token[i]; i += 2) { + if (!token[i] || !token[i + 1] ||
Double check for "token[i]"; it'd be easier to do:
for (i = 0; token[i] && token[i + 1]; i += 2) { ... }
Well, I put the check there to mark it explicitly that both values are required. I hoped it will be optimized out, but whatever. Your code won't work in case user had supplied an odd number of tokens. Which btw is what the loop body tries to find out too. Therefore I'd rather keep the code as is.
+ STREQ(token[i], "") || STREQ(token[i + 1], "")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid --with-loader-nvram list: %s"), + list); + goto cleanup;
But erroring out here is maybe even more ugly than that bash parsing in configure.ac :( There really is no other option to check this built-time? Can we at least do something like:
l="$(echo "$with_loader_nvram" | tr ':' '\n' | wc -l)" test $((l % 2)) -eq 0 || echo ERROR
this only checks that there's even number of parameters when split by ':', but it works in dash (and csh should be fine too, but I haven't tested that). And the only thing you were checking was the number of parameters, so this has the same meaning and it's done compile-time.
Whatever, I don't care anymore.
+ } + } + + if (i) { + if (VIR_ALLOC_N(cfg->loader, (i + 1) / 2) < 0 || + VIR_ALLOC_N(cfg->nvram, (i + 1) / 2) < 0) + goto cleanup; + cfg->nloader = (i + 1) / 2; + + while (i) { + if (VIR_STRDUP(cfg->loader[(i - 1) / 2 ], token[i - 2]) < 0 || + VIR_STRDUP(cfg->nvram[(i - 1) / 2], token[i - 1]) < 0)
You are not checking whether token[i] is '\0', but I don't know whether that breaks later in the code or not.
I don't see what good will the double check bring. I mean, tokens are checked for empty strings just in the previous 'for' loop that you've pointed out.
Anyway, if something like this happens, I think working around that and just printing a warning would be better. You can behave like if the file does not exist -- we don't quit the daemon/driver in that case, do we?
No. This is different. If either loader or nvram file does not exists, it's okay and code just handles that perfectly. However, if a token is an empty string it means that the list of defaults was mangled and I think that's something that deserves an explicit error message. Michal
Well, even though users can pass the list of UEFI:NVRAM pairs at the configure time, we may maintain the list of widely available UEFI ourselves too. And as arm64 begin to rises, OVMF was ported there too. With a slight name change - it's called AAVMF, with AAVMF_CODE.fd being the UEFI firmware and AAVMF_VARS.fd being the NVRAM store file. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu.conf | 12 +++++++++--- src/qemu/qemu_conf.c | 18 +++++++++++------- src/qemu/test_libvirtd_qemu.aug.in | 1 + .../domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml | 1 + tests/domaincapstest.c | 1 + 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index c6db568..1c589a2 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -506,6 +506,12 @@ # 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}. -# Later, when libvirt creates per domain variable store, this -# list is searched for the master image. -#nvram = [ "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" ] +# 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/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" +#] diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 3e51b49..9a15208 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -150,8 +150,10 @@ virQEMUDriverConfigLoaderNVRAMParse(virQEMUDriverConfigPtr cfg, } -#define VIR_QEMU_LOADER_FILE_PATH "/usr/share/OVMF/OVMF_CODE.fd" -#define VIR_QEMU_NVRAM_FILE_PATH "/usr/share/OVMF/OVMF_VARS.fd" +#define VIR_QEMU_OVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd" +#define VIR_QEMU_OVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd" +#define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/OVMF/OVMF_CODE.fd" +#define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd" virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { @@ -307,13 +309,15 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) #else - if (VIR_ALLOC_N(cfg->loader, 1) < 0 || - VIR_ALLOC_N(cfg->nvram, 1) < 0) + if (VIR_ALLOC_N(cfg->loader, 2) < 0 || + VIR_ALLOC_N(cfg->nvram, 2) < 0) goto error; - cfg->nloader = 1; + cfg->nloader = 2; - if (VIR_STRDUP(cfg->loader[0], VIR_QEMU_LOADER_FILE_PATH) < 0 || - VIR_STRDUP(cfg->nvram[0], VIR_QEMU_NVRAM_FILE_PATH) < 0) + if (VIR_STRDUP(cfg->loader[0], VIR_QEMU_OVMF_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->nvram[0], VIR_QEMU_OVMF_NVRAM_PATH) < 0 || + VIR_STRDUP(cfg->loader[1], VIR_QEMU_AAVMF_LOADER_PATH) < 0 || + VIR_STRDUP(cfg->nvram[1], VIR_QEMU_AAVMF_NVRAM_PATH) < 0) goto error; #endif diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 30fd27e..fc4935b 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -76,4 +76,5 @@ module Test_libvirtd_qemu = { "log_timestamp" = "0" } { "nvram" { "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" } + { "2" = "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" } } diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml index 346ef65..37d2102 100644 --- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml +++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml @@ -5,6 +5,7 @@ <arch>x86_64</arch> <os supported='yes'> <loader supported='yes'> + <value>/usr/share/AAVMF/AAVMF_CODE.fd</value> <value>/usr/share/OVMF/OVMF_CODE.fd</value> <enum name='type'> <value>rom</value> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 5ec03a4..de417b9 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -127,6 +127,7 @@ fillQemuCaps(virDomainCapsPtr domCaps, VIR_FREE(loader->values.values[--loader->values.nvalues]); if (fillStringValues(&loader->values, + "/usr/share/AAVMF/AAVMF_CODE.fd", "/usr/share/OVMF/OVMF_CODE.fd", NULL) < 0) return -1; -- 2.0.5
On Wed, Jan 21, 2015 at 08:21:29PM +0100, Michal Privoznik wrote:
Well, even though users can pass the list of UEFI:NVRAM pairs at the configure time, we may maintain the list of widely available UEFI ourselves too. And as arm64 begin to rises, OVMF was ported there too. With a slight name change - it's called AAVMF, with AAVMF_CODE.fd being the UEFI firmware and AAVMF_VARS.fd being the NVRAM store file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu.conf | 12 +++++++++--- src/qemu/qemu_conf.c | 18 +++++++++++------- src/qemu/test_libvirtd_qemu.aug.in | 1 + .../domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml | 1 + tests/domaincapstest.c | 1 + 5 files changed, 23 insertions(+), 10 deletions(-)
ACK; after the freeze. Martin
participants (2)
-
Martin Kletzander -
Michal Privoznik