Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Nov 12 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> Legacy vs modern should be detected via transport specific means. We
> can't wait till feature negotiation is done. Let us introduce
> virtio_force_modern() as a means for the transport code to signal
> that the device should operate in modern mode (because a modern driver
> was detected).
>
> A new callback is added for the situations where the device needs
> to do more than just setting the VIRTIO_F_VERSION_1 feature bit. For
> example, when vhost is involved, we may need to propagate the features
> to the vhost device.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>
> I'm still struggling with how to deal with vhost-user and co. The
> problem is that I'm not very familiar with the life-cycle of, let us
> say, a vhost_user device.
>
> Looks to me like the vhost part might be just an implementation detail,
> and could even become a hot swappable thing.
>
> Another thing is, that vhost processes set_features differently. It
> might or might not be a good idea to change this.
>
> Does anybody know why don't we propagate the features on features_set,
> but under a set of different conditions, one of which is the vhost
> device is started?
> ---
>  hw/virtio/virtio.c         | 13 +++++++++++++
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 15 insertions(+)
>

Did you see my feedback in
https://lore.kernel.org/qemu-devel/87tugzc26y.fsf@redhat.com/? I think
at least some of it still applies.

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3a1f6c520c..26db1b31e6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3281,6 +3281,19 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>      vdev->use_guest_notifier_mask = true;
>  }
>  
> +void  virtio_force_modern(VirtIODevice *vdev)

I'd still prefer to call this virtio_indicate_modern: we don't really
force anything; the driver has simply already decided that it will use
the modern interface and we provide an early indication in the features
so that code looking at them makes the right decisions.

> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +    virtio_add_feature(&vdev->guest_features, VIRTIO_F_VERSION_1);
> +    /* Let the device do it's normal thing. */
> +    virtio_set_features(vdev, vdev->guest_features);

I don't think this is substantially different from setting VERSION_1
only: At least the callers you introduce call this during reset,
i.e. when guest_features is 0 anyway. We still have the whole processing
that is done after feature setting that may have effects different from
what the ultimate feature setting will give us. While I don't think
calling set_features twice is forbidden, that sequence is likely quite
untested, and I'm not sure we can exclude side effects.

> +    /* For example for vhost-user we have to propagate to the vhost dev. */
> +    if (k->force_modern) {
> +        k->force_modern(vdev);
> +    }
> +}
> +
>  /*
>   * Only devices that have already been around prior to defining the virtio
>   * standard support legacy mode; this includes devices not specified in the