[libvirt] [PATCH 03/18] qemu: Support disk model=virtio-{non-}transitional

[Andrea Bolognani]

On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
[...]
> @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
>      { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP },
>      { "zpci", QEMU_CAPS_DEVICE_ZPCI },
>      { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD },
> +    {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL},
> +    {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL},

There should be whitespace before and after curly braces, for
consistency with existing entries.

[...]

> +++ b/src/qemu/qemu_capabilities.h
> @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
>      /* 325 */
>      QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */
>      QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */
> +    QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */
> +    QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */

A bit more verbose, but I think we should go for

  QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_{,NON_}TRANSITIONAL

since there are several non-PCI variants of VirtIO devices.

It'd also be nice if adding the capabilities and wiring up the
command line generation bits happened in separate patches.

[...]

> +static int
> +qemuBuildVirtioTransitional(virBufferPtr buf,
> +                            const char *baseName,
> +                            virQEMUCapsPtr qemuCaps,
> +                            virDomainDeviceAddressType type,
> +                            int model,
> +                            virDomainDeviceType devtype)
> +{
> +    int tmodel_cap, ntmodel_cap;
> +    bool has_tmodel, has_ntmodel;
> +
> +    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
> +        return -1;
> +
> +    switch (devtype) {
> +        case VIR_DOMAIN_DEVICE_DISK:
> +            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
> +            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
> +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;
> +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;
> +            break;

I like this approach much better than the one used in the RFC,
eg. passing two booleans to the function. Nice!

What I don't like is that you're building this fairly thin wrapper
around qemuBuildVirtioDevStr() when IMHO you should rather be
agumenting the original function - mostly because the new name is
not nearly as good :) Do you think you could make that happen?

[...]
> +    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +        (has_tmodel || has_ntmodel)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("virtio transitional models are not supported "
> +                         "for address type=%s"),

s/transitional/(non-)transitional/

[...]
> +    if (has_tmodel) {
> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
> +            virBufferAddLit(buf, "-transitional");
> +
> +        /* No error for if -transitional is not supported: our address
> +         * allocation will force the device into plain PCI bus, which
> +         * is functionally identical to standard 'virtio-XXX' behavior
> +         */
> +    } else if (has_ntmodel) {
> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {
> +            virBufferAddLit(buf, "-non-transitional");
> +        } else if (virQEMUCapsGet(qemuCaps,
> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
> +            virBufferAddLit(buf, ",disable-legacy=on");
> +        } else {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtio non-transitional model not supported "
> +                             "for this qemu"));
> +            return -1;
> +        }
> +    }

Would it make sense to be more explicit here? Current versions of
QEMU default to disable-modern=off,disable-legacy=off for virtio-pci
devices plugged into conventional PCI slots, but unless I'm mistaken
that was not always the case, so it would perhaps be preferrable to
not rely on that behavior and always explicitly set both disable-*
options when the new devices are not available; if the options
themselves are not available, then we should error out.

[...]
> @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>      case VIR_DOMAIN_DEVICE_DISK:
>          switch ((virDomainDiskBus) dev->data.disk->bus) {
>          case VIR_DOMAIN_DISK_BUS_VIRTIO:
> +            if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL)
> +                return pciFlags;

Perhaps a short comment about how transitional VirtIO devices can
only be plugged into conventional PCI slots would be appropriate
here, for the benefit of those looking at the code years from now :)

-- 
Andrea Bolognani / Red Hat / Virtualization