On Fri, Oct 29 2021, Halil Pasic <pa...@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 <pa...@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
