[libvirt] [PATCHv2 10/11] Introduce QEMU_CAPS_DEVICE_VIRTIO_REVISION

[Ján Tomko]

On Tue, Aug 09, 2016 at 11:08:10PM -0400, Laine Stump wrote:
>On 08/08/2016 12:35 PM, Ján Tomko wrote:
>> Check whether the disable-legacy property is present on the following
>> devices:
>>    virtio-balloon-pci
>>    virtio-blk-pci
>>    virtio-scsi-pci
>>    virtio-net-pci
>>    virtio-gpu-pci
>>
>> Assuming that if QEMU knows other virtio devices where this property
>> is applicable, it will have at least one of these devices.
>
>Pretty cool idea to look for the presence of disable-legacy in several
>virtio devices in case the binary has  been built without one of them.
>When I was doing a similar check, I semi-randomly picked
>"disable-modern", but only checked it on virtio-net. Although in
>practice that will probably work just as well, I like your idea better.
>

I only added it to those devices where we look for other properties.

>I'm curious though - did you have a reason for checking for
>disable-legacy rather than disable-modern? Or was your choice also
>semi-random?
>

I don't remember thinking about it, I just picked the first one.
But chronologically, virtio 0.9 was introduced first.

>
>> Added in QEMU by:
>> commit e266d421490e0ae83044bbebb209b2d3650c0ba6
>>      virtio-pci: add flags to enable/disable legacy/modern
>> ---
>>   src/qemu/qemu_capabilities.c                            | 6 ++++++
>>   src/qemu/qemu_capabilities.h                            | 1 +
>>   tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml        | 1 +
>>   tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml        | 1 +
>>   tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 +
>>   tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 +
>>   tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml       | 1 +
>>   tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml        | 1 +
>>   tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml        | 1 +
>>   9 files changed, 14 insertions(+)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 43e3ea7..340691a 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -340,6 +340,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>                 "display", /* 230 */
>>                 "intel-iommu",
>>                 "smm",
>> +              "virtio-revision",
>
>I had thought about naming my capability flag based on what I used it
>for as well, but decided it would be better to make the name of the
>capability as close to the actual option name as possible, to avoid
>confusion and allow re-use.

Okay, this string needs to be stable across libvirt releases, so
matching it as close as possible makes sense,

>
>Maybe an even better idea would be to make capabilities named
>"virtio-disable-modern" and "virtio-disable-legacy" set directly based
>on the presence of the options, then create a synthetic capability named
>virtio-revision that would be set if both of those others were true (you
>might find that you need to add more checks in the future, e.g. to
>disable it for certain interim versions of qemu that had bugs in virtio-1.0)

but I don't see a point of adding two capabilites for it if we are only
using one. The second one can just be added later if needed.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160810/35b3b095/attachment-0001.sig>