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

[Halil Pasic]

On Fri, 29 Oct 2021 16:53:25 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

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


I don't like virtio_indicate_modern(dev) form object orientation
perspective. In an OO language one would write it like
dev.virtio_indicate_modern()
which would read like the device should indicate modern to somebody.

In my opinion what happens is that we want to disable the legacy
interface if it is exposed by the device, or in other words instruct the
device that should act (precisely and exclusively) according to the
interface specification of the modern interface.

Maybe we can find a better name than force_modern, but I don't think
indicate_modern is a better name.

> 
> > +{
> > +    /*
> > +     * 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? 

When doing the whole song-and-dance the chance is higher that the
information will propagate to every place it needs to reach. For
example to the acked_features of vhost_dev. I've just posted a v2 RFC.
It should not be hard to see what I mean after examining that RFC.

> Devices may modify some of their
> behaviour or features, depending on what features they are called with,

I believe, if this is the case, we want the behavior that corresponds to
VERSION_1 set, i.e. 'modern'. So in my understanding this is rather good
than bad.

> and we will be calling this one again later with what is likely a
> different feature set. 

That is true, but the driver is allowed to set the features multiple
times, and since transports only support piecemeal access to the
features (32 bits at a time), I guess this is biz as usual.

>Also, the return code is not checked.
> 

That is true! It might be a good idea to log an error. Unfortunately I
don't think there is anything else we can sanely do.

> Maybe introduce a new function that sets guest_features directly and
> errors out if the features are not set in host_features? 

See above.

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

I agree about the moan part. I'm not sure what is the best way to
'fail'. Maybe we should continue this discussion in the v2 thread.


Thanks for your feedback! Sorry I didn't answer before sending out a v2.

Regards,
Halil

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