Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern()

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Oct 29 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).
>
> 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         | 12 ++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 13 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3a1f6c520c..75aee0e098 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>      vdev->use_guest_notifier_mask = true;
>  }
>  
> +void  virtio_force_modern(VirtIODevice *vdev)

<bikeshed> I'm not sure I like that name. We're not actually forcing the
device to be modern, we just set an early indication in the device
before proper feature negotiation has finished. Maybe
virtio_indicate_modern()? </bikeshed>

> +{
> +    /*
> +     * This takes care of the devices that implement config space access
> +     * in QEMU. For vhost-user and similar we need to make sure the features
> +     * are actually propagated to the device implementing the config space.
> +     *
> +     * A VirtioDeviceClass callback may be a good idea.
> +     */
> +    virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1));

Do we really need/want to do the whole song-and-dance for setting
features, just for setting VERSION_1? Devices may modify some of their
behaviour or features, depending on what features they are called with,
and we will be calling this one again later with what is likely a
different feature set. Also, the return code is not checked.

Maybe introduce a new function that sets guest_features directly and
errors out if the features are not set in host_features? If we try to
set VERSION_1 here despite the device not offering it, we are in a
pickle anyway, as we should not have gotten here if we did not offer it,
and we really should moan and fail in that case.

> +}
> +
>  /*
>   * Only devices that have already been around prior to defining the virtio
>   * standard support legacy mode; this includes devices not specified in the