On Sat, 10 Sep 2016 10:23:37 +0200
Maxime Coquelin <maxime.coque...@redhat.com> wrote:
> Currently, devices are plugged before features are negotiated.
> If the backend doesn't support VIRTIO_F_VERSION_1, the transport
> need to rewind some settings.
>
> This is the case for CCW, for which a post_plugged callback had
> been introduced, where max_rev field is just updated if
> VIRTIO_F_VERSION_1 is not supported by the backend.
> For PCI, implementing the post_plugged would be much more
s/the//
> complicated, so it needs to know whether the backend supports
> VIRTIO_F_VERSION_1 at plug time.
>
> Currently, nothing is done for PCI. Modern capabilitities get
> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported
> by the backend, which confuses the guest.
>
> This patch proposes to replace existing post_plugged solution
Nit: The patch does not propose anything, it just does it :)
> with an approach that fits with both transports.
> Features negociation is performed before ->device_plugged() call.
> A pre_plugged callback is introduced so that the transports can
> set their supported features.
With all those callbacks and so on, I think we should add some kind of
diagram/description under doc/ that describes the order in which they
are invoked and what elements of the virtio device the code can expect
to be in a reasonable state already. Nothing that needs to go with this
patch, but this is getting rather complex.
>
> Cc: Cornelia Huck <cornelia.h...@de.ibm.com>
> Cc: Marcel Apfelbaum <mar...@redhat.com>
> Cc: qemu-sta...@nongnu.org
> Acked-by: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
> ---
> Hi,
>
> This patch replaces series "virtio-pci: Improve device plugging
> whith legacy backends", as fixes the problem in a cleaner and
> more generic way. Goal is to have it also in stable tree.
I think this is fine for stable, with one comment below.
>
> Michael, I added your ack, as the changes compared to the RFC
> are minor:
> - Rebased on top of your pci branch
> - Improve error hanling when modern no supported and legacy
> disabled by the user
>
> I ran check-qtest, and tested PCI with vhost-user-bridge with
> and without VERSION_1 enabled.
>
> What is missing is testing CCW, Cornelia, can you handle that?
I can confirm that it continues to work with revision set to 1 or 0. I
still need to test what happens with an old host kernel (anyone knows
where vhost gained virtio-1 support? If not, I'll manage to find out.)
>
> Thanks,
> Maxime
> ---
> hw/s390x/virtio-ccw.c | 30 +++++++++++++++---------------
> hw/virtio/virtio-bus.c | 9 +++++----
> hw/virtio/virtio-pci.c | 36 ++++++++++++++++++++++++++++++++----
> hw/virtio/virtio-pci.h | 5 +++++
> include/hw/virtio/virtio-bus.h | 10 +++++-----
> 5 files changed, 62 insertions(+), 28 deletions(-)
(...)
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index f3e5ef3..24caa0a 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -54,16 +54,16 @@ typedef struct VirtioBusClass {
> int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
> void (*vmstate_change)(DeviceState *d, bool running);
> /*
> + * Expose the features the transport layer supports before
> + * the negotiation takes place.
> + */
> + void (*pre_plugged)(DeviceState *d, Error **errp);
> + /*
> * transport independent init function.
> * This is called by virtio-bus just after the device is plugged.
> */
> void (*device_plugged)(DeviceState *d, Error **errp);
> /*
> - * Re-evaluate setup after feature bits have been validated
> - * by the device backend.
> - */
> - void (*post_plugged)(DeviceState *d, Error **errp);
> - /*
> * transport independent exit function.
> * This is called by virtio-bus just before the device is unplugged.
> */
I'm not sure we want to rip out an interface in stable. I think the
interface may have value in itself - but OTOH, its only user is now
gone...
