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

[Cornelia Huck]

On Thu, 21 Jul 2016 12:26:03 +0300
Marcel Apfelbaum <address@hidden> wrote:

> 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.

IMV: transitional = legacy + modern and therefore
modern = transitional - legacy (with modern and virtio-1 the same
for pci). I think taking legacy away from transitional is clearer.

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

Let's see what Michael prefers.

> 
> >> 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.

Hm, for my view see above.

> 
> 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).

It would need some compat handling as well.

If you really want to go with such a 'mode' property, I'd prefer
'modern' over 'virtio-1': 'modern' is a virtio-pci concept, while
'virtio-1' is transport-agnostic.

But it's all just words in the end :)

Regardless what you end up with, feel free to add my

Reviewed-by: Cornelia Huck <address@hidden>