Message ID | 20211112145749.618157-2-pasic@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | virtio: early detect 'modern' virtio | expand |
On Fri, Nov 12 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). > > 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 <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 | 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.fsf@redhat.com/? I think at least some of it still applies. > 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. > +{ > + 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. 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. 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. > + /* 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
On Fri, 12 Nov 2021 16:37:20 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, Nov 12 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). > > > > 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 <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 | 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.fsf@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 > >
On Mon, Nov 15 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > On Fri, 12 Nov 2021 16:37:20 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Fri, Nov 12 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). >> > >> > 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 <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 | 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.fsf@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. My personal preference is "indicate", I don't like "force". I don't want a semantics discussion; I'll leave the decision to the virtio maintainers. > >> >> > +{ >> > + 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. Why? We just want VERSION_1 so that the checks work, why do we need to fiddle with other settings? We only need to propagate it to e.g. vhost. > >> 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. Well, what about first fixing the endianness bugs, before we potentially open up a can of worms? > > 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. What is wrong with verifying with one single feature?
On Mon, 15 Nov 2021 17:57:28 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Mon, Nov 15 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > > > On Fri, 12 Nov 2021 16:37:20 +0100 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Fri, Nov 12 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). > >> > > >> > 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 <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 | 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.fsf@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. > > My personal preference is "indicate", I don't like "force". I don't want > a semantics discussion; I'll leave the decision to the virtio > maintainers. I can't really follow your train of thought, but I'm OK with the outcome. > > > > >> > >> > +{ > >> > + 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. > > Why? We just want VERSION_1 so that the checks work, why do we need to > fiddle with other settings? We only need to propagate it to e.g. vhost. > Intermediate states are necessary, because transports can not set features as an atomic operation. That is, if both feature bits > 31 and <= 31 need to be set, an intermediate state is necessary. I think virtio_set_features() is what computer science calls a "mutator method" (for reference see: https://en.wikipedia.org/wiki/Mutator_method). Do we agree that virtio_set_features() is a mutator? Not using the mutator method is considered bad practice AFAIK. Just to get this perfectly clear you want me to not use virtio_set_features() here, right? > > > >> 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. > > Well, what about first fixing the endianness bugs, before we potentially > open up a can of worms? That can of worms is already open, because the driver can set features several times. I'm not exactly clear on what exactly is your concern here. So let us have a look at what virtio_set_features() does: (1) Makes sure the device is in a state when features can be set (!VIRTIO_CONFIG_S_FEATURES_OK) (2) Does basic validation and filtering (accepts only features that are indeed offered). (3) Calls the callback of the device class ->set_features() if any. (4) If VIRTIO_RING_F_EVENT_IDX does virtio_init_region_cache() for relevant queues. (5) Manages start_on_kick for legacy. In my opinion points (1) and (2) are desirable and could help catching bugs. Points (4) and (5) are irrelevant but conditional and obviously do not hurt. So I assume your problem is with the call to VirtioDeviceClass.set_features(), right? > > > > > 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. > > What is wrong with verifying with one single feature? > I don't think verifying with one single features is wrong. I don't think I ever said it is wrong. And I'm not sure what you mean by this. What is in my opinion not nice, and arguably an encapsulation and layering violation is tweaking the features without calling VirtioDeviceClass.set_features(). In my opinion, a device implementation can reasonably assume that the features didn't change if the implementation provided a set_features() callback, and this callback was not called. IMHO the callback is there, so that the device implementation can notified about feature changes. Or do you disagree? For something like vhost, when parts of the device don't live in the qemu process and have no access to vdev->guest_features, this is very reasonable: the QEMU external part will "cache" the relevant features, but it ain't prohibited in the general case either. What needs to be done and what may be done by the device on setting the features isnot over-specified. In the sense of virtio 1.0 or higher the changes have to take effect when feature negotiation is completed. That is when FEATURES_OK is set. For pre 1.0 virtio, it is for me hard to tell. I believe "feature negotiation completed" is tightly related to the QEMU concept of "started", although we also have "DRIVER_OK" which also plays a role. The validation of the features also takes place when FEATURES_OK is set and not before. I really can't put my finger on what are you concerned about. In practice guest_features goes from 0 to VIRTIO_F_VERSION_1. So even if the ->set_features callback were to do some funky feature enabling and disabling, it would only enable the functionality it associates with VIRTIO_F_VERSION_1 a.k.a. "modern", which is exactly what we want, and disable nothing, because no bit went from 1 to 0. Right? Yes, adding an unsolicited change to the feature set when transport detects a modern driver that is trying to drive a modern device is something unprecedented. But that is inherent. We want a transitional device to behave according to "features == 0" (legacy) -> "features == VIRTIO_F_VERSION_1" (modern) -> "features negotiated and final" (modern and features negotiated). Michael, can you please help us with this dispute? Regards, Halil
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) +{ + 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); + /* 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 diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 8bab9cfb75..1bb1551865 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -126,6 +126,7 @@ struct VirtioDeviceClass { int (*validate_features)(VirtIODevice *vdev); void (*get_config)(VirtIODevice *vdev, uint8_t *config); void (*set_config)(VirtIODevice *vdev, const uint8_t *config); + void (*force_modern)(VirtIODevice *vdev); void (*reset)(VirtIODevice *vdev); void (*set_status)(VirtIODevice *vdev, uint8_t val); /* For transitional devices, this is a bitmap of features @@ -394,6 +395,7 @@ static inline bool virtio_device_disabled(VirtIODevice *vdev) return unlikely(vdev->disabled || vdev->broken); } +void virtio_force_modern(VirtIODevice *vdev); bool virtio_legacy_allowed(VirtIODevice *vdev); bool virtio_legacy_check_disabled(VirtIODevice *vdev);
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 <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 | 13 +++++++++++++ include/hw/virtio/virtio.h | 2 ++ 2 files changed, 15 insertions(+)