On Thu, 21 Jul 2016 12:26:03 +0300
Marcel Apfelbaum <mar...@redhat.com> wrote:
> On 07/21/2016 11:54 AM, Cornelia Huck wrote:
> > On Wed, 20 Jul 2016 18:28:21 +0300
> > Marcel Apfelbaum <mar...@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.
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 <mar...@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.
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 <cornelia.h...@de.ibm.com>
