On Fri, Nov 12 2021, Halil Pasic <pa...@linux.ibm.com> wrote: > On Fri, 29 Oct 2021 16:53:25 +0200 > Cornelia Huck <coh...@redhat.com> wrote: > >> 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> > > > 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.
I think that is actually what happens: we indicate that it is a modern device to the code making the endianness decisions. > > 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. I don't see us disabling anything; the driver has already chosen what they want, and we simply need to make sure that all code honours that decision. > > 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 see my comment in the v2: I'm not sure how well tested that actually is. > >>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. Yeah, let's continue there, since that code is a bit different.