[v3] virtio-gpio and various virtio cleanups

[PATCH v3 05/21] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES

Posted by Alex Bennée 3 years, 6 months ago

This bit is unused in actual VirtIO feature negotiation and should
only appear in the vhost-user messages between master and slave.

[AJB: experiment, this doesn't break the tests but I'm not super
confident of the range of tests]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 block/export/vhost-user-blk-server.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 3409d9e02e..d31436006d 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
                1ull << VIRTIO_BLK_F_MQ |
                1ull << VIRTIO_F_VERSION_1 |
                1ull << VIRTIO_RING_F_INDIRECT_DESC |
-               1ull << VIRTIO_RING_F_EVENT_IDX |
-               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
+               1ull << VIRTIO_RING_F_EVENT_IDX ;
 
     if (!vexp->handler.writable) {
         features |= 1ull << VIRTIO_BLK_F_RO;
-- 
2.30.2

Re: [PATCH v3 05/21] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES

Posted by Jason Wang 3 years, 6 months ago

在 2022/7/27 03:21, Alex Bennée 写道:
> This bit is unused in actual VirtIO feature negotiation and should
> only appear in the vhost-user messages between master and slave.


I might be wrong, but this is actually used between master and slave not 
the device and driver?

Thanks


>
> [AJB: experiment, this doesn't break the tests but I'm not super
> confident of the range of tests]
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   block/export/vhost-user-blk-server.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> index 3409d9e02e..d31436006d 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
>                  1ull << VIRTIO_BLK_F_MQ |
>                  1ull << VIRTIO_F_VERSION_1 |
>                  1ull << VIRTIO_RING_F_INDIRECT_DESC |
> -               1ull << VIRTIO_RING_F_EVENT_IDX |
> -               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
> +               1ull << VIRTIO_RING_F_EVENT_IDX ;
>   
>       if (!vexp->handler.writable) {
>           features |= 1ull << VIRTIO_BLK_F_RO;

Re: [PATCH v3 05/21] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES

Posted by Alex Bennée 3 years, 6 months ago

Jason Wang <jasowang@redhat.com> writes:

> 在 2022/7/27 03:21, Alex Bennée 写道:
>> This bit is unused in actual VirtIO feature negotiation and should
>> only appear in the vhost-user messages between master and slave.
>
>
> I might be wrong, but this is actually used between master and slave
> not the device and driver?

Argh you may be right. However got confused with grepping:

  static const VuDevIface vu_blk_iface = {
      .get_features          = vu_blk_get_features,
      .queue_set_started     = vu_blk_queue_set_started,
      .get_protocol_features = vu_blk_get_protocol_features,
      .get_config            = vu_blk_get_config,
      .set_config            = vu_blk_set_config,
      .process_msg           = vu_blk_process_msg,
  };

and this is all part of libvhostuser but vhost-user-blk-server is in the
main tree. I guess it isn't moved into tools/ because it also re-uses
bits of the block layer?

Anyway I shall drop this patch.

>
> Thanks
>
>
>>
>> [AJB: experiment, this doesn't break the tests but I'm not super
>> confident of the range of tests]
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   block/export/vhost-user-blk-server.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
>> index 3409d9e02e..d31436006d 100644
>> --- a/block/export/vhost-user-blk-server.c
>> +++ b/block/export/vhost-user-blk-server.c
>> @@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
>>                  1ull << VIRTIO_BLK_F_MQ |
>>                  1ull << VIRTIO_F_VERSION_1 |
>>                  1ull << VIRTIO_RING_F_INDIRECT_DESC |
>> -               1ull << VIRTIO_RING_F_EVENT_IDX |
>> -               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
>> +               1ull << VIRTIO_RING_F_EVENT_IDX ;
>>         if (!vexp->handler.writable) {
>>           features |= 1ull << VIRTIO_BLK_F_RO;


-- 
Alex Bennée

Re: [PATCH v3 05/21] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES

Posted by Kevin Wolf 3 years, 6 months ago

Am 26.07.2022 um 21:21 hat Alex Bennée geschrieben:
> This bit is unused in actual VirtIO feature negotiation and should
> only appear in the vhost-user messages between master and slave.
> 
> [AJB: experiment, this doesn't break the tests but I'm not super
> confident of the range of tests]
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

I guess the rationale is that this doesn't make a difference because
vu_get_features_exec(), which is the only caller of .get_features(),
adds the VHOST_USER_F_PROTOCOL_FEATURES bit back before sending a
response.

Can you please add this to the commit message?

>  block/export/vhost-user-blk-server.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> index 3409d9e02e..d31436006d 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
>                 1ull << VIRTIO_BLK_F_MQ |
>                 1ull << VIRTIO_F_VERSION_1 |
>                 1ull << VIRTIO_RING_F_INDIRECT_DESC |
> -               1ull << VIRTIO_RING_F_EVENT_IDX |
> -               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
> +               1ull << VIRTIO_RING_F_EVENT_IDX ;

This has an extra space before the semicolon.

Kevin