[libvirt] [PATCH v2 12/25] qemu: Support disk model=virtio-{non-}transitional

[Cole Robinson]

On 1/29/19 5:25 AM, Andrea Bolognani wrote:
> On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote:
> [...]
>> +    switch (devtype) {
>> +        case VIR_DOMAIN_DEVICE_DISK:
>> +            has_tmodel = device.data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
>> +            has_ntmodel = device.data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
>> +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_TRANSITIONAL;
>> +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_NON_TRANSITIONAL;
>> +            break;
> 
> I wonder if this would look slightly nicer as
> 
>    case VIR_DOMAIN_DEVICE_DISK: {
>      virDomainDiskDefPtr disk = (virDomainDiskDefPtr) devdata;
> 
>      has_tmodel = disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
>      has_ntmodel = disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
>      tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_TRANSITIONAL;
>      ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_NON_TRANSITIONAL;
> 
>      break;
>    }
> 
> but up to you, really.
>

Makes for shorter lines which is nice but kind of offsets the benefit of 
converting to virDomainDeviceSetData in the first place...


> [...]
>> +    if (has_tmodel) {
>> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
>> +            virBufferAddLit(buf, "-transitional");
> 
> You're definitely using qemuCaps now, so you should remove
> ATTRIBUTE_UNUSED from the parameter in the function signature.
> 
>> +        /* 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
>> +         */
> 
> While this is absolutely correct and we should definitely not error
> out if the transitional device is not available, perhaps for the
> benefit of someone looking at the generated QEMU command line for
> debugging purposes we could do the same as below and also print
> 
>    disable-legacy=off,disable-modern=off
> 
> if the corresponding options are available - we would still not
> error out if they aren't, of course. Sounds reasonable?
>

Good point, i'll change it

> [...]
>> +    } 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,disable-modern=off");
> 
> Maybe add something like
> 
>    /* Even if the QEMU binary doesn't support the non-transitional
>     * device, we can still make it work by manually disabling legacy
>     * VirtIO and enabling modern VirtIO */
> 
> here.
> 
> [...]
>>   }
>>   
>> +
>>   static int
>>   qemuBuildVirtioOptionsStr(virBufferPtr buf,
>>                             virDomainVirtioOptionsPtr virtio,
> 
> Extraneous whitespace change.
> 
> [...]
>> @@ -720,6 +720,9 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>>       case VIR_DOMAIN_DEVICE_DISK:
>>           switch ((virDomainDiskBus) dev->data.disk->bus) {
>>           case VIR_DOMAIN_DISK_BUS_VIRTIO:
>> +            /* Transitional devices only work in conventional PCI slots */
>> +            if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL)
>> +                return pciFlags;
>>               return virtioFlags; /* only virtio disks use PCI */
> 
> Can please you use the same kind of switch statement you later use
> for eg. input devices here?
> 

ACK to all these changes too

Thanks,
Cole