Re: [Qemu-devel] [PATCH] virtio-bus: Plug devices after features are negotiated

[Cornelia Huck]

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