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

[Marcel Apfelbaum]

On 07/21/2016 11:54 AM, Cornelia Huck wrote:
> On Wed, 20 Jul 2016 18:28:21 +0300
> Marcel Apfelbaum <address@hidden> 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 <address@hidden>
>
> (...)
>
> > +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