Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()
On Mon, 15 Nov 2021 17:57:28 +0100
Cornelia Huck wrote:
> On Mon, Nov 15 2021, Halil Pasic wrote:
>
> > On Fri, 12 Nov 2021 16:37:20 +0100
> > Cornelia Huck wrote:
> >
> >> On Fri, Nov 12 2021, Halil Pasic 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
> >> > ---
> >> >
> >> > 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.
>
> 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(>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?
> >
> >>
Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()
On Mon, Nov 15 2021, Halil Pasic wrote:
> On Fri, 12 Nov 2021 16:37:20 +0100
> Cornelia Huck wrote:
>
>> On Fri, Nov 12 2021, Halil Pasic 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
>> > ---
>> >
>> > 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.
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(>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?
Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()
On Fri, 12 Nov 2021 16:37:20 +0100
Cornelia Huck wrote:
> On Fri, Nov 12 2021, Halil Pasic 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
> > ---
> >
> > 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(>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
>
>
Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()
On Fri, Nov 12 2021, Halil Pasic 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
> ---
>
> 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.
> 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(>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
[RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()
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
---
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(+)
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(>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);
--
2.25.1
