On Sat, 10 Sep 2016 10:23:37 +0200
Maxime Coquelin <address@hidden> 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 <address@hidden>
Cc: Marcel Apfelbaum <address@hidden>
Cc: address@hidden
Acked-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Maxime Coquelin <address@hidden>
---
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...