Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1

[Maxime Coquelin]

On 09/09/2016 01:49 PM, Cornelia Huck wrote:
> On Fri, 9 Sep 2016 13:44:35 +0200
> Maxime Coquelin <address@hidden> wrote:
>
> > On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> > > On Fri, 9 Sep 2016 14:04:55 +0300
> > > Marcel Apfelbaum <address@hidden> wrote:
> > >
> > > > On 09/09/2016 01:40 PM, Cornelia Huck wrote:
> > > > > On Fri,  9 Sep 2016 12:14:32 +0200
> > > > > Maxime Coquelin <address@hidden> wrote:
> > > > >
> > > > > > This patch makes pci devices plugging more robust, by not confusing
> > > > > > guest with modern interface when the backend doesn't support
> > > > > > VIRTIO_F_VERSION_1.
> > > > > >
> > > > > > Cc: Marcel Apfelbaum <address@hidden>
> > > > > > Cc: Michael S. Tsirkin <address@hidden>
> > > > > > Cc: address@hidden
> > > > > > Signed-off-by: Maxime Coquelin <address@hidden>
> > > > > > ---
> > > > > >  hw/virtio/virtio-pci.c | 15 +++++++++++++++
> > > > > >  hw/virtio/virtio-pci.h |  5 +++++
> > > > > >  2 files changed, 20 insertions(+)
> > > > >
> > > > > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
> > > > > ccw") fixes this issue for ccw via the introduction of a
> > > > > ->post_plugged() callback. Unfortunately, we did not find a good way to
> > > > > make it work for pci back then.
> > > >
> > > > It seems that for ccw is enough to rewind dev->rev_max,
> > > > sadly for pci we need to rewind a lot of settings/resources.
> > >
> > > Yes, that what I meant with 'more flexibility for ccw'.
> > Maybe we could replace post_plugged with a pre_plugged approach?
> >
> > In ->pre_plugged(), cww and pci would specify which features it can
> > support using virtio_add_feature().
> > Then we could call get_features() before ->device_plugged().
>
> I think that would work for ccw (haven't looked at pci).
Good, once quick fix accepted, I'll try this solution.

> > Doing this, both ccw and pci would have the needed information without
> > having to rewind any settings.
> >
> > Does that make sense?
> >
> > But for now, I think it would be better to merge something in the spirit
> > of this series (taking into account to remarks).
> > Indeed, I think we want this fixed in stable, but the above proposal
> > would be too huge for stable.
>
> A 'just check for VERSION_1' approach would probably be best for stable.
Ok, thanks.
I will send a v2 replacing the generic function with a VERISON_1
specfic:
bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);

Maxime