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

2022-07-28 Thread Alex Bennée 


Jason Wang  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 
>> ---
>>   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

2022-07-28 Thread Jason Wang 



在 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 
---
  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

2022-07-27 Thread Kevin Wolf 
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 

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

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

2022-07-26 Thread 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.

[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 
---
 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