[libvirt] [PATCH 3/3] qemu: format intel-iommu on the command line

[John Ferlan]

On 06/23/2016 06:47 AM, Ján Tomko wrote:
> <devices>
>   <iommu model='intel-iommu'/>
> </devices>
> 
> results in:
> 
> -device intel-iommu

Might be nice to add a bit more "meat" to the commit message.

> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1235580
> ---
>  src/qemu/qemu_command.c                            | 30 ++++++++++++++++++++++
>  .../qemuxml2argvdata/qemuxml2argv-intel-iommu.args | 22 ++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  2 ++
>  3 files changed, 54 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args
> 

One other thing that wasn't 100% from the qemu patches - is there a
specific need to ensure the "-device intel-iommu" is placed on the
command line prior to any PCI device being placed there as well?

It's that same snippet I pulled from review of patch 1:

"The device is part of the machine properties because we wanted
to ensure is created before any other PCI device."

But qemu patch 5/5 removes iommu from the machine command line, so I'm
left wondering if there isn't some assumption that the -device option
will need to be generated before any other PCI device.

I only mention this because if that's the case, then I think we need to
add some comments prior to calling qemuBuildIOMMUCommandLine to indicate
that placement for the "-device intel-iommu" must be done before other
PCI devices. I realize there's a general feeling against too many
comments; however, it might avoid having some bug creep in because
someone didn't know or follow a bz link or a qemu doc link from a commit
message...

Alternatively if the XML implementation becomes a hypervisor feature,
then qemuBuildBootCommandLine could add the option.

That also provides a way to have "either" 'iommu=on' or 'device
intel-iommu' be generated with "-machine" or after machine is created to
help hide or ensure future generations don't erroneously move things.

John


> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6944129..9f1d447 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6171,6 +6171,33 @@ qemuBuildBootCommandLine(virCommandPtr cmd,
>  
>  
>  static int
> +qemuBuildIOMMUCommandLine(virCommandPtr cmd,
> +                          const virDomainDef *def,
> +                          virQEMUCapsPtr qemuCaps)
> +{
> +    if (!def->iommu)
> +        return 0;
> +
> +    switch (def->iommu->model) {
> +    case VIR_DOMAIN_IOMMU_MODEL_INTEL:
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_INTEL_IOMMU)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("IOMMU device: '%s' is not supported with "
> +                             "this QEMU binary"),
> +                           virDomainIOMMUModelTypeToString(def->iommu->model));
> +            return -1;
> +        }
> +        virCommandAddArgList(cmd, "-device",
> +                             virDomainIOMMUModelTypeToString(def->iommu->model),
> +                             NULL);

Obviously this would need adjustment if you changed the model...

> +    case VIR_DOMAIN_IOMMU_MODEL_LAST:
> +        break;
> +    }
> +    return 0;
> +}
> +
> +
> +static int
>  qemuBuildGlobalControllerCommandLine(virCommandPtr cmd,
>                                       const virDomainDef *def,
>                                       virQEMUCapsPtr qemuCaps)
> @@ -9254,6 +9281,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>      if (qemuBuildBootCommandLine(cmd, def, qemuCaps, &emitBootindex) < 0)
>          goto error;
>  
> +    if (qemuBuildIOMMUCommandLine(cmd, def, qemuCaps) < 0)
> +        goto error;
> +
>      if (qemuBuildGlobalControllerCommandLine(cmd, def, qemuCaps) < 0)
>          goto error;
>  
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args
> new file mode 100644
> index 0000000..69e4490
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args
> @@ -0,0 +1,22 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest1 \
> +-S \
> +-M q35 \
> +-m 214 \
> +-smp 1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-device intel-iommu \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index a4b8bf4..59fa420 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2023,6 +2023,8 @@ mymain(void)
>                                QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_USB_HUB);
>  
>      DO_TEST("acpi-table", NONE);
> +    DO_TEST("intel-iommu", QEMU_CAPS_DEVICE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_INTEL_IOMMU);
>  
>      qemuTestDriverFree(&driver);
>  
>