[libvirt] [PATCH v2 4/5] qemu: Enforce ACPI, UEFI requirements

[John Ferlan]

On 03/29/2017 10:08 AM, Andrea Bolognani wrote:
> Depending on the architecture, requirements for ACPI and UEFI can
> be different; more specifically, while on x86 UEFI requires ACPI,
> on aarch64 it's the other way around.
> 
> Enforce these requirements when validating the domain, and make
> the error message more accurate by mentioning that they're not
> necessarily applicable to all architectures.
> 
> Several aarch64 test cases had to be tweaked because they would
> have failed the validation step otherwise.
> ---
>  src/qemu/qemu_command.c                              | 20 ++++----------------
>  src/qemu/qemu_domain.c                               | 20 ++++++++++++++++++++
>  .../qemuxml2argv-aarch64-aavmf-virtio-mmio.args      |  1 +
>  .../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml       |  1 -
>  .../qemuxml2argv-aarch64-cpu-passthrough.args        |  1 +
>  .../qemuxml2argv-aarch64-cpu-passthrough.xml         |  1 -
>  .../qemuxml2argv-aarch64-video-virtio-gpu-pci.args   |  1 +
>  .../qemuxml2argv-aarch64-video-virtio-gpu-pci.xml    |  3 ---
>  ...xml2argv-aarch64-virt-2.6-virtio-pci-default.args |  1 +
>  ...uxml2argv-aarch64-virt-2.6-virtio-pci-default.xml |  1 -
>  .../qemuxml2argv-aarch64-virt-default-nic.args       |  1 +
>  .../qemuxml2argv-aarch64-virt-default-nic.xml        |  3 ---
>  .../qemuxml2argv-aarch64-virt-virtio.args            |  1 +
>  .../qemuxml2argv-aarch64-virt-virtio.xml             |  1 -
>  .../qemuxml2argv-aarch64-virtio-pci-default.args     |  1 +
>  .../qemuxml2argv-aarch64-virtio-pci-default.xml      |  1 -
>  ...xml2argv-aarch64-virtio-pci-manual-addresses.args |  1 +
>  ...uxml2argv-aarch64-virtio-pci-manual-addresses.xml |  1 -
>  .../qemuxml2argv-balloon-mmio-deflate.args           |  1 +
>  .../qemuxml2argv-balloon-mmio-deflate.xml            |  1 -
>  .../qemuxml2xmlout-aarch64-aavmf-virtio-mmio.xml     |  1 -
>  .../qemuxml2xmlout-aarch64-video-virtio-gpu-pci.xml  |  1 -
>  .../qemuxml2xmlout-aarch64-virtio-pci-default.xml    |  1 -
>  ...ml2xmlout-aarch64-virtio-pci-manual-addresses.xml |  1 -
>  24 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 64d2d71..5cf383a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9324,18 +9324,16 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager,
>  }
>  
>  
> -static int
> +static void
>  qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
> -                                 virDomainDefPtr def,
> -                                 virQEMUCapsPtr qemuCaps)
> +                                 virDomainDefPtr def)
>  {
> -    int ret = -1;
>      virDomainLoaderDefPtr loader = def->os.loader;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      int unit = 0;
>  
>      if (!loader)
> -        return 0;
> +        return;
>  
>      switch ((virDomainLoader) loader->type) {
>      case VIR_DOMAIN_LOADER_TYPE_ROM:
> @@ -9344,12 +9342,6 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>          break;
>  
>      case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI) &&
> -            def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("ACPI must be enabled in order to use UEFI"));
> -            goto cleanup;
> -        }
>  
>          if (loader->secure == VIR_TRISTATE_BOOL_YES) {
>              virCommandAddArgList(cmd,
> @@ -9387,10 +9379,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>          break;
>      }
>  
> -    ret = 0;
> - cleanup:
>      virBufferFreeAndReset(&buf);
> -    return ret;
>  }
>  
>  
> @@ -9827,8 +9816,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>      if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
>          goto error;
>  
> -    if (qemuBuildDomainLoaderCommandLine(cmd, def, qemuCaps) < 0)
> -        goto error;
> +    qemuBuildDomainLoaderCommandLine(cmd, def);
>  
>      if (!migrateURI && !snapshot && qemuDomainAlignMemorySizes(def) < 0)
>          goto error;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 458bb5f..e41e8e4 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2884,6 +2884,26 @@ qemuDomainDefValidate(const virDomainDef *def,
>          goto cleanup;
>      }
>  
> +    /* On x86, UEFI requires ACPI */
> +    if (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",
> +                       _("UEFI requires ACPI on this architecture"));
> +        goto cleanup;
> +    }
> +
> +    /* 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)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("ACPI requires UEFI on this architecture"));
> +        return -1;

goto cleanup;

> +    }
> +

So as I said in my last response to v1 - are you sure there couldn't be
a guest running that now disappears because of this check?

IOW should this move to post parse processing...

ACK - if you're positive this won't affect the running guest causing it
to disappear.

John


>      if (def->os.loader &&
>          def->os.loader->secure == VIR_TRISTATE_BOOL_YES) {
>          /* These are the QEMU implementation limitations. But we