Re: [PATCH v2 05/15] hw/virtio: add boilerplate for vhost-user-gpio device

From: "Alex Bennée" <alex.bennee@linaro.org>
To: Vincent Whitchurch <vincent.whitchurch@axis.com>
Cc: qemu-devel@nongnu.org, slp@redhat.com, mst@redhat.com,
	marcandre.lureau@redhat.com, stefanha@redhat.com,
	mathieu.poirier@linaro.org, viresh.kumar@linaro.org
Subject: Re: [PATCH v2 05/15] hw/virtio: add boilerplate for vhost-user-gpio device
Date: Wed, 06 Jul 2022 14:37:07 +0100	[thread overview]
Message-ID: <87sfnejr63.fsf@linaro.org> (raw)
In-Reply-To: <20220610083747.GA29012@axis.com>

Vincent Whitchurch <vincent.whitchurch@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.whitchurch@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