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

[Cole Robinson]

On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
> 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.
> 

Indeed, I'll fix that in all the patches

> [...]
> 
>> +++ 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.
> 

Sure sounds good

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

OK

>> +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?
> 

Hmm, seems weird to make call sites that will never support the
transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into
this function, passing DEVICE_TYPE etc. I can split the transitional
bits into their own function and not have it wrap BuildVirtioDevStr but
be a follow on, so for example:

if (qemuBuildVirtioDevStr(...) < 0)
    goto error;
if (qemuBuildVirtioTransitionalStr(...) < )
    goto error;

Does that work?

> [...]
>> +    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.
> 

I'll respond separately

> [...]
>> @@ -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 :)
> 

This same pattern is duplicated in the rest of the series, seems weird
to comment one site, but commenting all of them is overkill. I guess
commenting one site is the middle ground unless you have a better idea
of where to put the comment

Thanks for the review
- Cole