[libvirt] [PATCH] qemu: Relax os.loader->type check when validating domain

When validating a domain among all the checks there are two that concern VIR_DOMAIN_LOADER_TYPE_PFLASH specifically. The first check ensures that on x86 ACPI is enabled when UEFI is requested, the second ensures that UEFI is used when ACPI is requested on aarch64. However, check for UEFI is done by plain comparison of def->os.loader->type which is insufficient because we have def->os.firmware too. NB, this wouldn't be a problem for active domain, because on startup process def->os.loader->type gets filled by qemuFirmwareEnableFeatures(), but that's not the case for inactive domains. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1729604 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0f1fda2384..ed33e31699 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4221,8 +4221,9 @@ qemuDomainDefValidate(const virDomainDef *def, } /* On x86, UEFI requires ACPI */ - if (def->os.loader && - def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && + if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI || + (def->os.loader && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH)) && ARCH_IS_X86(def->os.arch) && def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -4233,8 +4234,9 @@ qemuDomainDefValidate(const virDomainDef *def, /* On aarch64, ACPI requires UEFI */ if (def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_TRISTATE_SWITCH_ON && def->os.arch == VIR_ARCH_AARCH64 && - (!def->os.loader || - def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH)) { + (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_EFI && + (!def->os.loader || + def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("ACPI requires UEFI on this architecture")); goto cleanup; -- 2.21.0

On Sat, 2019-07-13 at 09:29 +0200, Michal Privoznik wrote:
When validating a domain among all the checks there are two that concern VIR_DOMAIN_LOADER_TYPE_PFLASH specifically. The first check ensures that on x86 ACPI is enabled when UEFI is requested, the second ensures that UEFI is used when ACPI is requested on aarch64. However, check for UEFI is done by plain comparison of def->os.loader->type which is insufficient because we have def->os.firmware too.
NB, this wouldn't be a problem for active domain, because on startup process def->os.loader->type gets filled by qemuFirmwareEnableFeatures(), but that's not the case for inactive domains.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1729604
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
The changes look good, but can you please at the same time add the <acpi/> element under <features/> in tests/qemuxml2argvdata/aarch64-os-firmware-efi.xml so that we gain coverage for it? With the above taken care of, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani
-
Michal Privoznik