[RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[Halil Pasic]

This is an early RFC for a transport specific early detecton of
modern virtio, which is most relevant for transitional devices on big
endian platforms, when drivers access the config space before
FEATURES_OK is set.

The most important part that is missing here is fixing all the problems
that arise in the situation described in the previous paragraph, when
the config is managed by a vhost device (and thus outside QEMU. This
series tackles this problem only for virtio_net+vhost as an example. If
this approach is deemed good, we need to do something very similar for
every single affected device.

This series was only lightly tested. The vhost stuff is entirely
untested, unfortunately I don't have a working setup where this
handling would be needed (because the config space is handled in the
device). DPDK is not supported on s390x so at the moment I can't test
DPDK based setups. 

v1 -> v2:

* add callback
* tweak feature manipulation
* add generic handling for vhost that needs to be called by devices
* add handling for virtio

Halil Pasic (5):
  virtio: introduce virtio_force_modern()
  virtio-ccw: use virtio_force_modern()
  virtio-pci: use virtio_force_modern()
  vhost: push features to backend on force_modern
  virtio-net: handle force_modern for vhost

 hw/net/virtio-net.c        | 20 ++++++++++++++++++++
 hw/s390x/virtio-ccw.c      |  3 +++
 hw/virtio/vhost.c          | 17 +++++++++++++++++
 hw/virtio/virtio-pci.c     |  1 +
 hw/virtio/virtio.c         | 13 +++++++++++++
 include/hw/virtio/vhost.h  |  2 ++
 include/hw/virtio/virtio.h |  2 ++
 7 files changed, 58 insertions(+)


base-commit: 2c3e83f92d93fbab071b8a96b8ab769b01902475
-- 
2.25.1

[RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Halil Pasic]

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(+)

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);
 
-- 
2.25.1

[RFC PATCH v2 2/5] virtio-ccw: use virtio_force_modern()

[Halil Pasic]

The fact that revision > 0 was negotiated implies that VIRTIO_VERSION_1
aka modern must be used. This negotiation is done before the obligatory
reset. Let us call virtio_force_modern() after the reset if revision > 0
was negotiated, so that the VIRTIO_VERSION_1 feature can be set, and
endianness starts working as it should for devices that comply to the
virtio spec.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/s390x/virtio-ccw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 6a2df1c1e9..88fbe87942 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -266,6 +266,9 @@ static void virtio_ccw_reset_virtio(VirtioCcwDevice *dev, VirtIODevice *vdev)
         dev->summary_indicator = NULL;
     }
     ccw_dev->sch->thinint_active = false;
+    if (dev->revision > 0) {
+        virtio_force_modern(vdev);
+    }
 }
 
 static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
-- 
2.25.1

[RFC PATCH v2 3/5] virtio-pci: use virtio_force_modern()

[Halil Pasic]

Let us detect usage via the modern interface by tapping into the place
that implements the 'modern' reset.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/virtio/virtio-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 6e16e2705c..8dd862da21 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1297,6 +1297,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
 
         if (vdev->status == 0) {
             virtio_pci_reset(DEVICE(proxy));
+            virtio_force_modern(virtio_bus_get_device(&proxy->bus));
         }
 
         break;
-- 
2.25.1

[RFC PATCH v2 4/5] vhost: push features to backend on force_modern

[Halil Pasic]

In vhost we don't push the features to the vhost device when the
features are set, but when the vhost device is started. This can
lead to problems when config space is implemented in the vhost
device, and the driver does some early config space reading (early in a
sense that it precedes setting FEATURES_OK).

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/virtio/vhost.c         | 17 +++++++++++++++++
 include/hw/virtio/vhost.h |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index b4b29413e6..5764970298 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1628,6 +1628,23 @@ void vhost_dev_free_inflight(struct vhost_inflight *inflight)
     }
 }
 
+int vhost_dev_force_modern(struct vhost_dev *hdev)
+{
+    uint64_t features;
+    int r;
+
+    assert(hdev->vhost_ops);
+
+    hdev->acked_features |= (0x1ULL << VIRTIO_F_VERSION_1);
+    features = hdev->acked_features;
+    r = hdev->vhost_ops->vhost_set_features(hdev, features);
+    if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_features failed");
+        return -errno;
+    }
+    return 0;
+}
+
 static int vhost_dev_resize_inflight(struct vhost_inflight *inflight,
                                      uint64_t new_size)
 {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 1a9fc65089..9ef784e2e9 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -138,6 +138,8 @@ int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
                          uint32_t config_len, Error **errp);
 int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data,
                          uint32_t offset, uint32_t size, uint32_t flags);
+
+int vhost_dev_force_modern(struct vhost_dev *vdev);
 /* notifier callback in case vhost device config space changed
  */
 void vhost_dev_set_config_notifier(struct vhost_dev *dev,
-- 
2.25.1

[RFC PATCH v2 5/5] virtio-net: handle force_modern for vhost

[Halil Pasic]

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---

Inspired by virtio_net_set_features() which I don't quite understand.
Why do we have to do vhost_net_ack_features() for each possible queue?
---
 hw/net/virtio-net.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..43ed9ef3ba 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -766,6 +766,25 @@ static uint64_t virtio_net_bad_features(VirtIODevice *vdev)
     return features;
 }
 
+static void  virtio_net_force_modern(VirtIODevice *vdev)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    int i;
+
+    /*
+     * Why do we have to loop over all queues? Are not features a
+     * per-device thing?
+     */
+    for (i = 0;  i < n->max_queues; i++) {
+        NetClientState *nc = qemu_get_subqueue(n->nic, i);
+
+        if (!get_vhost_net(nc->peer)) {
+            continue;
+        }
+        vhost_dev_force_modern(&get_vhost_net(nc->peer)->dev);
+    }
+}
+
 static void virtio_net_apply_guest_offloads(VirtIONet *n)
 {
     qemu_set_offload(qemu_get_queue(n->nic)->peer,
@@ -3668,6 +3687,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->get_features = virtio_net_get_features;
     vdc->set_features = virtio_net_set_features;
     vdc->bad_features = virtio_net_bad_features;
+    vdc->force_modern = virtio_net_force_modern;
     vdc->reset = virtio_net_reset;
     vdc->set_status = virtio_net_set_status;
     vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
-- 
2.25.1

Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Cornelia Huck]

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

Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Halil Pasic]

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

Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Cornelia Huck]

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?

Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Halil Pasic]

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

Re: [RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[Halil Pasic]

On Fri, 12 Nov 2021 15:57:44 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> This is an early RFC for a transport specific early detecton of
> modern virtio, which is most relevant for transitional devices on big
> endian platforms, when drivers access the config space before
> FEATURES_OK is set.

[..]

Ping!

@Michael: Can you have a look at this, please?

Re: [RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[Halil Pasic]

On Tue, 23 Nov 2021 14:13:40 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 12 Nov 2021 15:57:44 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > This is an early RFC for a transport specific early detecton of
> > modern virtio, which is most relevant for transitional devices on big
> > endian platforms, when drivers access the config space before
> > FEATURES_OK is set.  
> 
> [..]
> 
> Ping!
> 
> @Michael: Can you have a look at this, please?

Ping^2

Re: [RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[Michael S. Tsirkin]

On Fri, Nov 12, 2021 at 03:57:44PM +0100, Halil Pasic wrote:
> This is an early RFC for a transport specific early detecton of
> modern virtio, which is most relevant for transitional devices on big
> endian platforms, when drivers access the config space before
> FEATURES_OK is set.
> 
> The most important part that is missing here is fixing all the problems
> that arise in the situation described in the previous paragraph, when
> the config is managed by a vhost device (and thus outside QEMU. This
> series tackles this problem only for virtio_net+vhost as an example. If
> this approach is deemed good, we need to do something very similar for
> every single affected device.
> 
> This series was only lightly tested. The vhost stuff is entirely
> untested, unfortunately I don't have a working setup where this
> handling would be needed (because the config space is handled in the
> device). DPDK is not supported on s390x so at the moment I can't test
> DPDK based setups. 

So this looks sane to me. Cornelia requested some name tweaks and we
need to add vhost-user things and more devices, but otherwise we are
good.

> v1 -> v2:
> 
> * add callback
> * tweak feature manipulation
> * add generic handling for vhost that needs to be called by devices
> * add handling for virtio
> 
> Halil Pasic (5):
>   virtio: introduce virtio_force_modern()
>   virtio-ccw: use virtio_force_modern()
>   virtio-pci: use virtio_force_modern()
>   vhost: push features to backend on force_modern
>   virtio-net: handle force_modern for vhost
> 
>  hw/net/virtio-net.c        | 20 ++++++++++++++++++++
>  hw/s390x/virtio-ccw.c      |  3 +++
>  hw/virtio/vhost.c          | 17 +++++++++++++++++
>  hw/virtio/virtio-pci.c     |  1 +
>  hw/virtio/virtio.c         | 13 +++++++++++++
>  include/hw/virtio/vhost.h  |  2 ++
>  include/hw/virtio/virtio.h |  2 ++
>  7 files changed, 58 insertions(+)
> 
> 
> base-commit: 2c3e83f92d93fbab071b8a96b8ab769b01902475
> -- 
> 2.25.1

Re: [RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[Halil Pasic]

On Wed, 8 Dec 2021 13:56:19 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Nov 12, 2021 at 03:57:44PM +0100, Halil Pasic wrote:
> > This is an early RFC for a transport specific early detecton of
> > modern virtio, which is most relevant for transitional devices on big
> > endian platforms, when drivers access the config space before
> > FEATURES_OK is set.
> > 
> > The most important part that is missing here is fixing all the problems
> > that arise in the situation described in the previous paragraph, when
> > the config is managed by a vhost device (and thus outside QEMU. This
> > series tackles this problem only for virtio_net+vhost as an example. If
> > this approach is deemed good, we need to do something very similar for
> > every single affected device.
> > 
> > This series was only lightly tested. The vhost stuff is entirely
> > untested, unfortunately I don't have a working setup where this
> > handling would be needed (because the config space is handled in the
> > device). DPDK is not supported on s390x so at the moment I can't test
> > DPDK based setups.   
> 
> So this looks sane to me. Cornelia requested some name tweaks and we
> need to add vhost-user things and more devices, but otherwise we are
> good.

Thanks for your feedback! There were several points where I could
not reach agreement with Cornelia. From your response I recon that:

1) I should rename virtio_force_modern() to virtio_indicate_modern()
(per maintainer request).
2) Keep the call to virtio_set_features()?

Is that right?

Regards,
Halil

Re: [RFC PATCH v2 0/5] virtio: early detect 'modern' virtio

[Michael S. Tsirkin]

On Thu, Dec 09, 2021 at 02:29:25PM +0100, Halil Pasic wrote:
> On Wed, 8 Dec 2021 13:56:19 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Nov 12, 2021 at 03:57:44PM +0100, Halil Pasic wrote:
> > > This is an early RFC for a transport specific early detecton of
> > > modern virtio, which is most relevant for transitional devices on big
> > > endian platforms, when drivers access the config space before
> > > FEATURES_OK is set.
> > > 
> > > The most important part that is missing here is fixing all the problems
> > > that arise in the situation described in the previous paragraph, when
> > > the config is managed by a vhost device (and thus outside QEMU. This
> > > series tackles this problem only for virtio_net+vhost as an example. If
> > > this approach is deemed good, we need to do something very similar for
> > > every single affected device.
> > > 
> > > This series was only lightly tested. The vhost stuff is entirely
> > > untested, unfortunately I don't have a working setup where this
> > > handling would be needed (because the config space is handled in the
> > > device). DPDK is not supported on s390x so at the moment I can't test
> > > DPDK based setups.   
> > 
> > So this looks sane to me. Cornelia requested some name tweaks and we
> > need to add vhost-user things and more devices, but otherwise we are
> > good.
> 
> Thanks for your feedback! There were several points where I could
> not reach agreement with Cornelia. From your response I recon that:
> 
> 1) I should rename virtio_force_modern() to virtio_indicate_modern()
> (per maintainer request).
> 2) Keep the call to virtio_set_features()?
> 
> Is that right?
> 
> Regards,
> Halil

that's my take, yes.

-- 
MST