Vincent Whitchurch <vincent.whitchu...@axis.com> writes:
> On Tue, May 24, 2022 at 04:40:46PM +0100, Alex Bennée wrote:
>> +static int vu_gpio_start(VirtIODevice *vdev)
>> +{
>> + BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>> + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> + VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
>> + int ret, i;
>> +
>> + if (!k->set_guest_notifiers) {
>> + error_report("binding does not support guest notifiers");
>> + return -ENOSYS;
>> + }
>> +
>> + ret = vhost_dev_enable_notifiers(&gpio->vhost_dev, vdev);
>> + if (ret < 0) {
>> + error_report("Error enabling host notifiers: %d", ret);
>> + return ret;
>> + }
>> +
>> + ret = k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, true);
>> + if (ret < 0) {
>> + error_report("Error binding guest notifier: %d", ret);
>> + goto err_host_notifiers;
>> + }
>> +
>> + /*
>> + * Before we start up we need to ensure we have the final feature
>> + * set needed for the vhost configuration.
>> + */
>> + vhost_ack_features(&gpio->vhost_dev, feature_bits,
>> vdev->backend_features);
>
> This is doing the feature handling differently from the other
> vhost-user-* implementations, and it doesn't seem to work for me.
> Negotiated features (I noticed it with VIRTIO_RING_F_EVENT_IDX) never
> make it to VHOST_USER_SET_FEATURES.
>
> If I change this code to match vhost-user-i2c and the other
> implementations like in the patch below, it works.
Unfortunately the virtio-i2c backend doesn't need
VHOST_USER_F_PROTOCOL_FEATURES which gets squashed with the bellow
changes which is the cause of the eventual failure in the qos-test:
# Start of read-guest-mem tests
vu_net_set_features: 0
**
ERROR:../../tests/qtest/vhost-user-test.c:1031:vu_net_set_features: assertion
failed: (msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
which adds to my confusion about the exact route the negotiation of
vhost-user feature bits is meant to make through the code.
>
> The guest is Linux v5.18. The backend uses libvhost-user and is the one
> posted here (with a few fixes):
>
>
> https://lore.kernel.org/lkml/20220311162445.346685-3-vincent.whitchu...@axis.com/
>
> 8<-------
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 87e3976880..1dc7af6b03 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -73,7 +73,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
> * Before we start up we need to ensure we have the final feature
> * set needed for the vhost configuration.
> */
> - vhost_ack_features(&gpio->vhost_dev, feature_bits,
> vdev->backend_features);
> + gpio->vhost_dev.acked_features = vdev->guest_features;
>
> ret = vhost_dev_start(&gpio->vhost_dev, vdev);
> if (ret < 0) {
> @@ -156,9 +156,7 @@ static uint64_t vu_gpio_get_features(VirtIODevice *vdev,
> uint64_t features,
> virtio_add_feature(&features, VIRTIO_GPIO_F_IRQ);
> virtio_add_feature(&features, VIRTIO_F_VERSION_1);
>
> - vdev->backend_features = vhost_get_features(&gpio->vhost_dev,
> feature_bits,
> - features);
> - return vdev->backend_features;
> + return vhost_get_features(&gpio->vhost_dev, feature_bits, features);
> }
>
> static void vu_gpio_handle_output(VirtIODevice *vdev, VirtQueue *vq)
--
Alex Bennée
