[PATCH] hw/vhost-user-i2c: Add support for VIRTIO_I2C_F_ZERO_LENGTH_REQUEST

[PATCH] hw/vhost-user-i2c: Add support for VIRTIO_I2C_F_ZERO_LENGTH_REQUEST

[Viresh Kumar]

VIRTIO_I2C_F_ZERO_LENGTH_REQUEST is a mandatory feature, that must be
implemented by everyone. Add its support.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 hw/virtio/vhost-user-i2c.c         | 10 ++++++++--
 include/hw/virtio/vhost-user-i2c.h |  3 +++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index d172632bb0cd..6323a7203ae4 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -19,6 +19,10 @@
 #define VIRTIO_ID_I2C_ADAPTER                34
 #endif
 
+static const int feature_bits[] = {
+    VIRTIO_I2C_F_ZERO_LENGTH_REQUEST
+};
+
 static void vu_i2c_start(VirtIODevice *vdev)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
@@ -113,8 +117,10 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
 static uint64_t vu_i2c_get_features(VirtIODevice *vdev,
                                     uint64_t requested_features, Error **errp)
 {
-    /* No feature bits used yet */
-    return requested_features;
+    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
+
+    virtio_add_feature(&requested_features, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST);
+    return vhost_get_features(&i2c->vhost_dev, feature_bits, requested_features);
 }
 
 static void vu_i2c_handle_output(VirtIODevice *vdev, VirtQueue *vq)
diff --git a/include/hw/virtio/vhost-user-i2c.h b/include/hw/virtio/vhost-user-i2c.h
index deae47a76d55..d8372f3b43ea 100644
--- a/include/hw/virtio/vhost-user-i2c.h
+++ b/include/hw/virtio/vhost-user-i2c.h
@@ -25,4 +25,7 @@ struct VHostUserI2C {
     bool connected;
 };
 
+/* Virtio Feature bits */
+#define VIRTIO_I2C_F_ZERO_LENGTH_REQUEST		0
+
 #endif /* _QEMU_VHOST_USER_I2C_H */
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH] hw/vhost-user-i2c: Add support for VIRTIO_I2C_F_ZERO_LENGTH_REQUEST

[Viresh Kumar]

On 11-01-22, 20:28, Viresh Kumar wrote:
> VIRTIO_I2C_F_ZERO_LENGTH_REQUEST is a mandatory feature, that must be
> implemented by everyone. Add its support.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  hw/virtio/vhost-user-i2c.c         | 10 ++++++++--
>  include/hw/virtio/vhost-user-i2c.h |  3 +++
>  2 files changed, 11 insertions(+), 2 deletions(-)

Ping.

-- 
viresh

Re: [PATCH] hw/vhost-user-i2c: Add support for VIRTIO_I2C_F_ZERO_LENGTH_REQUEST

[Alex Bennée]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> VIRTIO_I2C_F_ZERO_LENGTH_REQUEST is a mandatory feature, that must be
> implemented by everyone. Add its support.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

but...

<snip>
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> @@ -113,8 +117,10 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>  static uint64_t vu_i2c_get_features(VirtIODevice *vdev,
>                                      uint64_t requested_features, Error **errp)
>  {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> +
> +    virtio_add_feature(&requested_features, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST);
> +    return vhost_get_features(&i2c->vhost_dev, feature_bits, requested_features);
>  }

It's a bit weird we set it and then pass it to the vhost-user backend.
It does raise the question of why the stub actually cares about feature
bits at all when really it's a negotiation with the backend.

IOW what would happen if we just called:

    return vhost_get_features(&i2c->vhost_dev, feature_bits, -1);

>  
>  static void vu_i2c_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/include/hw/virtio/vhost-user-i2c.h b/include/hw/virtio/vhost-user-i2c.h
> index deae47a76d55..d8372f3b43ea 100644
> --- a/include/hw/virtio/vhost-user-i2c.h
> +++ b/include/hw/virtio/vhost-user-i2c.h
> @@ -25,4 +25,7 @@ struct VHostUserI2C {
>      bool connected;
>  };
>  
> +/* Virtio Feature bits */
> +#define VIRTIO_I2C_F_ZERO_LENGTH_REQUEST		0
> +
>  #endif /* _QEMU_VHOST_USER_I2C_H */


-- 
Alex Bennée

Re: [PATCH] hw/vhost-user-i2c: Add support for VIRTIO_I2C_F_ZERO_LENGTH_REQUEST

[Viresh Kumar]

On 10-02-22, 08:29, Alex Bennée wrote:
> 
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> > @@ -113,8 +117,10 @@ static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> >  static uint64_t vu_i2c_get_features(VirtIODevice *vdev,
> >                                      uint64_t requested_features, Error **errp)
> >  {
> > -    /* No feature bits used yet */
> > -    return requested_features;
> > +    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> > +
> > +    virtio_add_feature(&requested_features, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST);
> > +    return vhost_get_features(&i2c->vhost_dev, feature_bits, requested_features);
> >  }
> 
> It's a bit weird we set it and then pass it to the vhost-user backend.
> It does raise the question of why the stub actually cares about feature
> bits at all when really it's a negotiation with the backend.
> 
> IOW what would happen if we just called:
> 
>     return vhost_get_features(&i2c->vhost_dev, feature_bits, -1);

That works as well.

Also I noticed just now that I haven't added VHOST_INVALID_FEATURE_BIT
at the end of the feature_bits[]. Will fix that.

-- 
viresh