| Message ID | 4f8de2059fc963bb67920a1a2f8481f969a35eec.1641912994.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/vhost-user-i2c: Add support for VIRTIO_I2C_F_ZERO_LENGTH_REQUEST | expand |
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 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 */
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.
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 */
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(-)