On Fri, 12 Nov 2021 16:37:20 +0100 Cornelia Huck <coh...@redhat.com> wrote:
> On Fri, Nov 12 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). > > > > 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 <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 | 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....@redhat.com/? I think > at least some of it still applies. > Sure. My idea was to send out a v2 first which helps us think about the bigger picture, and then answer that mail. Now I realize I should have sent the response first, and then the v2 immediately afterwards. > > 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. I tried to explain my dislike for virtio_indicate_modern in my response to that email. In somewhat different words: IMHO indication is about an external observer and has a symbolic dimension to it. Please see https://www.oxfordlearnersdictionaries.com/definition/english/indicate This function is about changing the behavior of the device. Its post-condition is: the device acts compliant to virtio 1.0 or higher. > > > +{ > > + 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. I agree. Just wanted to be conservative, and preserve whatever is there. > 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. Yes, this is an intermediate state. As I pointed out, intermediate states are necessary. > 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. I can't disagree with that. But IMHO we can just say: such problems, if any, are bugs that need to be fixed. I think not doing the whole song-and-dance is conceptually more problematic because it is more likely to lead to inconsistent internal state. For example check out: vhost acked_features <-> guest_features. Regards, Halil > > > + /* 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 > >