Re: [Qemu-devel] [PATCH V3] hw/virtio-pci: fix virtio behaviour

[Marcel Apfelbaum <marcel@redhat.com>]

On 07/21/2016 11:54 AM, Cornelia Huck wrote:
> On Wed, 20 Jul 2016 18:28:21 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> Enable transitional virtio devices by default.
>> Enable virtio-1.0 for devices plugged into
>> PCIe ports (Root ports or Downstream ports).
>

Hi Cornelia,
Thank you for the review.

> Add "by default", as this can still be overridden?
>

Yes, using -device virtio*,disable-modern=x,disable-legacy=y
are respected as before.


>>
>> Using the virtio-1 mode will remove the limitation
>
> s/Using the virtio-1 mode/Disabling the legacy mode/
>
> ?
>

Well, the way I see it virtio-1 'pure' is not using the IO BAR.
This is why virtio-1 == disable-modern=off && disable-legacy=on IMHO.

If you or Michael see this differently I have nothing against re-wording it.

>> of the number of devices that can be attached to a machine
>> by removing the need for the IO BAR.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> (...)
>
>> +static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
>> +{
>> +    return !proxy->disable_modern;
>> +}
>> +
>> +static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy)
>> +{
>> +    return proxy->disable_legacy == ON_OFF_AUTO_OFF;
>> +}
>> +
>> +static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
>
> One thing I still find a bit confusing is that you refer to 'modern'
> above, but force to 'virtio_1' here... but that's a minor thing.
>

I went for 'virtio-1' because of the existing comments (force virtio-1)
and also because 'modern' does not imply "no legacy" - those are independent flags.

BTW, instead of the 'disable*' properties (which I find hard to follow) I would go for one
property : "mode" with "legacy"/"transitional"/"virtio-1"/"auto" values.
But is too late for that (at least for 2.7).

>> +{
>> +    proxy->disable_modern = false;
>> +    proxy->disable_legacy = ON_OFF_AUTO_ON;
>> +}
>>
>>   /*
>>    * virtio-scsi-pci: This extends VirtioPCIProxy.
>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>> index 9914e7a..1531399 100644
>> --- a/include/hw/compat.h
>> +++ b/include/hw/compat.h
>> @@ -6,6 +6,14 @@
>>           .driver   = "virtio-mmio",\
>>           .property = "format_transport_address",\
>>           .value    = "off",\
>> +    },{\
>> +        .driver   = "virtio-pci",\
>> +        .property = "disable-modern",\
>> +        .value    = "on",\
>> +    },{\
>> +        .driver   = "virtio-pci",\
>> +        .property = "disable-legacy",\
>> +        .value    = "off",\
>
> After looking at the code, I think this will work - did you test this
> with a compat machine, though?
>

Yes, I tested it with pc/q35 2.5 and 2.6 machines. The previous
behavior remains the same.

>>       },
>>
>>   #define HW_COMPAT_2_5 \
>
> But generally, looks good and I think this is also an improvement in
> readability.
>

Thanks!
Marcel