[libvirt] [PATCH v3] qemu: Allow UEFI paths to be specified at compile time
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 | 32 +++++++++++++++++ src/qemu/qemu.conf | 12 +++++-- src/qemu/qemu_conf.c | 41 ++++++++++++++++++---- src/qemu/test_libvirtd_qemu.aug.in | 1 + .../domaincaps-qemu_1.6.50-1.xml | 1 + tests/domaincapstest.c | 15 ++++---- 6 files changed, 85 insertions(+), 17 deletions(-) diff --git a/configure.ac b/configure.ac index 9d12079..164cfad 100644 --- a/configure.ac +++ b/configure.ac @@ -2786,6 +2786,38 @@ 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@:>@])], + [with_loader_nvram=${withval}], + [with_loader_nvram=no]) + +if test "$with_loader_nvram" != "no"; then + IFS=':' read -a loader_arr <<< "${with_loader_nvram}" + + loader_str_c="{" + nvram_str_c="{" + for ((indx=0; indx<${#loader_arr[@]}; indx+=2)); do + loader=${loader_arr[[indx]]} + nvram=${loader_arr[[indx+1]]} + + if test -z "$loader" || test -z "$nvram"; then + AC_MSG_ERROR([Malformed loader-nvram list]) + fi + + loader_str_c="$loader_str_c \"$loader\"," + nvram_str_c="$nvram_str_c \"$nvram\"," + done + + loader_str_c="$loader_str_c }" + nvram_str_c="$nvram_str_c }" + + AC_DEFINE_UNQUOTED([DEFAULT_LOADER], [$loader_str_c], [Array of loader paths]) + AC_DEFINE_UNQUOTED([DEFAULT_NVRAM], [$nvram_str_c], [Array of nvram paths]) +fi + # 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 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 9539231..728d01e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,13 +107,21 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); } -#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) { virQEMUDriverConfigPtr cfg; +#if defined(DEFAULT_LOADER) && defined(DEFAULT_NVRAM) + const char *loader[] = DEFAULT_LOADER; + const char *nvram[] = DEFAULT_NVRAM; + size_t i; +#endif + if (virQEMUConfigInitialize() < 0) return NULL; @@ -258,14 +266,33 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->logTimestamp = true; - if (VIR_ALLOC_N(cfg->loader, 1) < 0 || - VIR_ALLOC_N(cfg->nvram, 1) < 0) +#if defined(DEFAULT_LOADER) && defined(DEFAULT_NVRAM) + verify(ARRAY_CARDINALITY(loader) == ARRAY_CARDINALITY(nvram)); + + if (VIR_ALLOC_N(cfg->loader, ARRAY_CARDINALITY(loader)) < 0 || + VIR_ALLOC_N(cfg->nvram, ARRAY_CARDINALITY(loader)) < 0) + goto error; + cfg->nloader = ARRAY_CARDINALITY(loader); + + for (i = 0; i < ARRAY_CARDINALITY(loader); i++) { + if (VIR_STRDUP(cfg->loader[i], loader[i]) < 0 || + VIR_STRDUP(cfg->nvram[i], nvram[i]) < 0) + goto error; + } + +#else /* !defined(DEFAULT_LOADER) || !defined(DEFAULT_NVRAM) */ + + 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 /* !defined(DEFAULT_LOADER) || !defined(DEFAULT_NVRAM) */ return cfg; 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 70d2ef3..5ca8928 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/AAVMF/AAVMF_CODE.fd", + "/usr/share/OVMF/OVMF_CODE.fd", + NULL) < 0) + return -1; return 0; } #endif /* WITH_QEMU */ -- 2.0.5
On Wed, Jan 14, 2015 at 04:28:33PM +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 | 32 +++++++++++++++++ src/qemu/qemu.conf | 12 +++++-- src/qemu/qemu_conf.c | 41 ++++++++++++++++++---- src/qemu/test_libvirtd_qemu.aug.in | 1 + .../domaincaps-qemu_1.6.50-1.xml | 1 + tests/domaincapstest.c | 15 ++++---- 6 files changed, 85 insertions(+), 17 deletions(-)
diff --git a/configure.ac b/configure.ac index 9d12079..164cfad 100644 --- a/configure.ac +++ b/configure.ac @@ -2786,6 +2786,38 @@ 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@:>@])], + [with_loader_nvram=${withval}], + [with_loader_nvram=no]) + +if test "$with_loader_nvram" != "no"; then + IFS=':' read -a loader_arr <<< "${with_loader_nvram}" +
This will fail miserably on freebsd and basically everything out there that's not bash, I guess. At least for csh and dash this is an invalid line.
+ loader_str_c="{" + nvram_str_c="{" + for ((indx=0; indx<${#loader_arr[@]}; indx+=2)); do
Same here. It's basically not a very good idea to use shell scripts here, mainly for such operations.
+ loader=${loader_arr[[indx]]} + nvram=${loader_arr[[indx+1]]} + + if test -z "$loader" || test -z "$nvram"; then + AC_MSG_ERROR([Malformed loader-nvram list]) + fi + + loader_str_c="$loader_str_c \"$loader\"," + nvram_str_c="$nvram_str_c \"$nvram\"," + done + + loader_str_c="$loader_str_c }" + nvram_str_c="$nvram_str_c }" + + AC_DEFINE_UNQUOTED([DEFAULT_LOADER], [$loader_str_c], [Array of loader paths]) + AC_DEFINE_UNQUOTED([DEFAULT_NVRAM], [$nvram_str_c], [Array of nvram paths]) +fi + # 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 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 9539231..728d01e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,13 +107,21 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); }
-#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) { virQEMUDriverConfigPtr cfg;
+#if defined(DEFAULT_LOADER) && defined(DEFAULT_NVRAM) + const char *loader[] = DEFAULT_LOADER; + const char *nvram[] = DEFAULT_NVRAM; + size_t i; +#endif + if (virQEMUConfigInitialize() < 0) return NULL;
@@ -258,14 +266,33 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->logTimestamp = true;
- if (VIR_ALLOC_N(cfg->loader, 1) < 0 || - VIR_ALLOC_N(cfg->nvram, 1) < 0) +#if defined(DEFAULT_LOADER) && defined(DEFAULT_NVRAM) + verify(ARRAY_CARDINALITY(loader) == ARRAY_CARDINALITY(nvram)); + + if (VIR_ALLOC_N(cfg->loader, ARRAY_CARDINALITY(loader)) < 0 || + VIR_ALLOC_N(cfg->nvram, ARRAY_CARDINALITY(loader)) < 0) + goto error; + cfg->nloader = ARRAY_CARDINALITY(loader); + + for (i = 0; i < ARRAY_CARDINALITY(loader); i++) { + if (VIR_STRDUP(cfg->loader[i], loader[i]) < 0 || + VIR_STRDUP(cfg->nvram[i], nvram[i]) < 0) + goto error; + } + +#else /* !defined(DEFAULT_LOADER) || !defined(DEFAULT_NVRAM) */ + + 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 /* !defined(DEFAULT_LOADER) || !defined(DEFAULT_NVRAM) */
return cfg;
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>
Unrelated to this patch, but under what conditions *should* these values be formatted?
<enum name='type'> <value>rom</value> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 70d2ef3..5ca8928 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]);
And my head just went blank in this hunk. So I'll start from scratch... You are filling in the qemu caps for the test so they are consistent when we are running tests. What you are changing is that before this patch the value got filled in only if there was none, but now you are clearing the whole array. Since the second approach seems to be the right one, does that mean that we had an error there before? Not counting the shell script part it looks fine.
- if (fillStringValues(&loader->values, - "/usr/share/OVMF/OVMF_CODE.fd", - NULL) < 0) - return -1; - } + if (fillStringValues(&loader->values, + "/usr/share/AAVMF/AAVMF_CODE.fd", + "/usr/share/OVMF/OVMF_CODE.fd", + NULL) < 0) + return -1; return 0; } #endif /* WITH_QEMU */ -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 20.01.2015 18:20, Martin Kletzander wrote:
On Wed, Jan 14, 2015 at 04:28:33PM +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 | 32 +++++++++++++++++ src/qemu/qemu.conf | 12 +++++-- src/qemu/qemu_conf.c | 41 ++++++++++++++++++---- src/qemu/test_libvirtd_qemu.aug.in | 1 + .../domaincaps-qemu_1.6.50-1.xml | 1 + tests/domaincapstest.c | 15 ++++---- 6 files changed, 85 insertions(+), 17 deletions(-)
diff --git a/configure.ac b/configure.ac index 9d12079..164cfad 100644 --- a/configure.ac +++ b/configure.ac @@ -2786,6 +2786,38 @@ 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@:>@])], + [with_loader_nvram=${withval}], + [with_loader_nvram=no]) + +if test "$with_loader_nvram" != "no"; then + IFS=':' read -a loader_arr <<< "${with_loader_nvram}" +
This will fail miserably on freebsd and basically everything out there that's not bash, I guess. At least for csh and dash this is an invalid line.
+ loader_str_c="{" + nvram_str_c="{" + for ((indx=0; indx<${#loader_arr[@]}; indx+=2)); do
Same here. It's basically not a very good idea to use shell scripts here, mainly for such operations.
I wanted to check the list at build time. But okay, I can do that at runtime as well. Even thought it has a disadvantage that if passed list is malformed nobody will notice until the first start of the daemon.
+ loader=${loader_arr[[indx]]} + nvram=${loader_arr[[indx+1]]} + + if test -z "$loader" || test -z "$nvram"; then + AC_MSG_ERROR([Malformed loader-nvram list]) + fi + + loader_str_c="$loader_str_c \"$loader\"," + nvram_str_c="$nvram_str_c \"$nvram\"," + done + + loader_str_c="$loader_str_c }" + nvram_str_c="$nvram_str_c }" + + AC_DEFINE_UNQUOTED([DEFAULT_LOADER], [$loader_str_c], [Array of loader paths]) + AC_DEFINE_UNQUOTED([DEFAULT_NVRAM], [$nvram_str_c], [Array of nvram paths]) +fi + # 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 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 9539231..728d01e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,13 +107,21 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); }
-#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) { virQEMUDriverConfigPtr cfg;
+#if defined(DEFAULT_LOADER) && defined(DEFAULT_NVRAM) + const char *loader[] = DEFAULT_LOADER; + const char *nvram[] = DEFAULT_NVRAM; + size_t i; +#endif + if (virQEMUConfigInitialize() < 0) return NULL;
@@ -258,14 +266,33 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->logTimestamp = true;
- if (VIR_ALLOC_N(cfg->loader, 1) < 0 || - VIR_ALLOC_N(cfg->nvram, 1) < 0) +#if defined(DEFAULT_LOADER) && defined(DEFAULT_NVRAM) + verify(ARRAY_CARDINALITY(loader) == ARRAY_CARDINALITY(nvram)); + + if (VIR_ALLOC_N(cfg->loader, ARRAY_CARDINALITY(loader)) < 0 || + VIR_ALLOC_N(cfg->nvram, ARRAY_CARDINALITY(loader)) < 0) + goto error; + cfg->nloader = ARRAY_CARDINALITY(loader); + + for (i = 0; i < ARRAY_CARDINALITY(loader); i++) { + if (VIR_STRDUP(cfg->loader[i], loader[i]) < 0 || + VIR_STRDUP(cfg->nvram[i], nvram[i]) < 0) + goto error; + } + +#else /* !defined(DEFAULT_LOADER) || !defined(DEFAULT_NVRAM) */ + + 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 /* !defined(DEFAULT_LOADER) || !defined(DEFAULT_NVRAM) */
return cfg;
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>
Unrelated to this patch, but under what conditions *should* these values be formatted?
Basically it's builtin default...
<enum name='type'> <value>rom</value> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 70d2ef3..5ca8928 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]);
.. which explains this hunk. The problem is, if a list of uefi:nvram pairs is provided to configure, it becomes builtin default and hence the formatted XML is different to the expected one (in general).
And my head just went blank in this hunk. So I'll start from scratch...
You are filling in the qemu caps for the test so they are consistent when we are running tests. What you are changing is that before this patch the value got filled in only if there was none, but now you are clearing the whole array. Since the second approach seems to be the right one, does that mean that we had an error there before?
Yes and no. Previously there was no way to override the builtin default. And since previously there was exactly one default, the code as is now makes perfect sense. However, since the default can be overridden at build time, we want predictable output here.
Not counting the shell script part it looks fine.
Michal
participants (2)
-
Martin Kletzander -
Michal Privoznik