vhost-user: add NEED_REPLY flag

[PATCH] vhost-user: add NEED_REPLY flag

Posted by luzhixing12345 3 months, 3 weeks ago

Front-end message requests which need reply should set NEED_REPLY_MASK
in flag, and response from slave need clear NEED_REPLY_MASK flag.

---
 hw/virtio/vhost-user.c                    | 2 +-
 subprojects/libvhost-user/libvhost-user.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 00561daa06..edf2271e0a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1082,7 +1082,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
     int ret;
     VhostUserMsg msg = {
         .hdr.request = request,
-        .hdr.flags = VHOST_USER_VERSION,
+        .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
     };
 
     if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 9c630c2170..40f665bd7f 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -667,6 +667,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
 {
     /* Set the version in the flags when sending the reply */
     vmsg->flags &= ~VHOST_USER_VERSION_MASK;
+    vmsg->flags &= ~VHOST_USER_NEED_REPLY_MASK;
     vmsg->flags |= VHOST_USER_VERSION;
     vmsg->flags |= VHOST_USER_REPLY_MASK;
 
-- 
2.34.1

Re: [PATCH] vhost-user: add NEED_REPLY flag

Posted by Michael S. Tsirkin 2 months, 2 weeks ago

On Sun, Aug 04, 2024 at 11:48:59PM +0800, luzhixing12345 wrote:
> Front-end message requests which need reply should set NEED_REPLY_MASK
> in flag, and response from slave need clear NEED_REPLY_MASK flag.


neither this.

> ---
>  hw/virtio/vhost-user.c                    | 2 +-
>  subprojects/libvhost-user/libvhost-user.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 00561daa06..edf2271e0a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1082,7 +1082,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
>      int ret;
>      VhostUserMsg msg = {
>          .hdr.request = request,
> -        .hdr.flags = VHOST_USER_VERSION,
> +        .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
>      };
>  
>      if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 9c630c2170..40f665bd7f 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -667,6 +667,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>  {
>      /* Set the version in the flags when sending the reply */
>      vmsg->flags &= ~VHOST_USER_VERSION_MASK;
> +    vmsg->flags &= ~VHOST_USER_NEED_REPLY_MASK;
>      vmsg->flags |= VHOST_USER_VERSION;
>      vmsg->flags |= VHOST_USER_REPLY_MASK;
>  
> -- 
> 2.34.1

Re: [PATCH] vhost-user: add NEED_REPLY flag

Posted by 陆知行 3 months, 2 weeks ago

Hi, can someone review this patch?
I find requests which call  vhost_user_get_u64 does not set NEED_REPLY flag

luzhixing12345 <luzhixing12345@gmail.com> 于2024年8月4日周日 23:50写道：

> Front-end message requests which need reply should set NEED_REPLY_MASK
> in flag, and response from slave need clear NEED_REPLY_MASK flag.
>
> ---
>  hw/virtio/vhost-user.c                    | 2 +-
>  subprojects/libvhost-user/libvhost-user.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 00561daa06..edf2271e0a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1082,7 +1082,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev,
> int request, uint64_t *u64)
>      int ret;
>      VhostUserMsg msg = {
>          .hdr.request = request,
> -        .hdr.flags = VHOST_USER_VERSION,
> +        .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
>      };
>
>      if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
> diff --git a/subprojects/libvhost-user/libvhost-user.c
> b/subprojects/libvhost-user/libvhost-user.c
> index 9c630c2170..40f665bd7f 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -667,6 +667,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg
> *vmsg)
>  {
>      /* Set the version in the flags when sending the reply */
>      vmsg->flags &= ~VHOST_USER_VERSION_MASK;
> +    vmsg->flags &= ~VHOST_USER_NEED_REPLY_MASK;
>      vmsg->flags |= VHOST_USER_VERSION;
>      vmsg->flags |= VHOST_USER_REPLY_MASK;
>
> --
> 2.34.1
>
>

Re: [PATCH] vhost-user: add NEED_REPLY flag

Posted by Stefano Garzarella 2 months, 4 weeks ago

On Mon, Aug 12, 2024 at 12:53:19PM GMT, 陆知行 wrote:
>Hi, can someone review this patch?
>I find requests which call  vhost_user_get_u64 does not set NEED_REPLY flag

Can you provide an example to trigger this issue?

Also, with this change all calls to vhost_user_get_u64() will set that 
flag, is that following the vhost-user user specification?

Please use `scripts/checkpatch.pl` before sending patches, this one for 
example is missing SoB.

Thanks,
Stefano

>
>luzhixing12345 <luzhixing12345@gmail.com> 于2024年8月4日周日 23:50写道：
>
>> Front-end message requests which need reply should set NEED_REPLY_MASK
>> in flag, and response from slave need clear NEED_REPLY_MASK flag.
>>
>> ---
>>  hw/virtio/vhost-user.c                    | 2 +-
>>  subprojects/libvhost-user/libvhost-user.c | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 00561daa06..edf2271e0a 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -1082,7 +1082,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev,
>> int request, uint64_t *u64)
>>      int ret;
>>      VhostUserMsg msg = {
>>          .hdr.request = request,
>> -        .hdr.flags = VHOST_USER_VERSION,
>> +        .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
>>      };
>>
>>      if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>> b/subprojects/libvhost-user/libvhost-user.c
>> index 9c630c2170..40f665bd7f 100644
>> --- a/subprojects/libvhost-user/libvhost-user.c
>> +++ b/subprojects/libvhost-user/libvhost-user.c
>> @@ -667,6 +667,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg
>> *vmsg)
>>  {
>>      /* Set the version in the flags when sending the reply */
>>      vmsg->flags &= ~VHOST_USER_VERSION_MASK;
>> +    vmsg->flags &= ~VHOST_USER_NEED_REPLY_MASK;
>>      vmsg->flags |= VHOST_USER_VERSION;
>>      vmsg->flags |= VHOST_USER_REPLY_MASK;
>>
>> --
>> 2.34.1
>>
>>

Re: [PATCH] vhost-user: add NEED_REPLY flag

Posted by luzhixing12345 2 months, 2 weeks ago

Signed-off-by: luzhixing12345 <luzhixing12345@gmail.com>

>On Mon, Aug 12, 2024 at 12:53:19PM GMT, 陆知行 wrote:
>>Hi, can someone review this patch?
>>I find requests which call  vhost_user_get_u64 does not set NEED_REPLY flag
>
>Can you provide an example to trigger this issue?
>
>Also, with this change all calls to vhost_user_get_u64() will set that 
>flag, is that following the vhost-user user specification?

It will not trigger a bug.

For each function that calls vhost_user_get_u64() like vhost_user_get_features/vhost_user_get_status, if you set a breakpoint in gdb at subprojects/libvhost-user/libvhost-user.c/vu_dispatch and you will find that

```
bool
vu_dispatch(VuDev *dev)
{
    // ...
    need_reply = vmsg.flags & VHOST_USER_NEED_REPLY_MASK; // 0

    reply_requested = vu_process_message(dev, &vmsg);     // 1
    // ...
}

vhost-user protocol doc list some requests that need reply like VHOST_USER_GET_FEATURES/VHOST_USER_GET_PROTOCOL_FEATURES, the flag should be set with NEED_REPLY_MASK.

The current code does not raise an error because in libvhost-user(vu_process_message) it will not check this flag and always choose whether or not reply based on the request type.   

So this patch fills the flag and make sure need_reply to 1 for the requests that need reply.

>Please use `scripts/checkpatch.pl` before sending patches, this one for 
>example is missing SoB.
>
>Thanks,
>Stefano
>
>>
>>luzhixing12345 <luzhixing12345@gmail.com> 于2024年8月4日周日 23:50写道:
>>
>>> Front-end message requests which need reply should set NEED_REPLY_MASK
>>> in flag, and response from slave need clear NEED_REPLY_MASK flag.
>>>
>>> ---
>>>  hw/virtio/vhost-user.c                    | 2 +-
>>>  subprojects/libvhost-user/libvhost-user.c | 1 +
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index 00561daa06..edf2271e0a 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -1082,7 +1082,7 @@ static int vhost_user_get_u64(struct vhost_dev *dev,
>>> int request, uint64_t *u64)
>>>      int ret;
>>>      VhostUserMsg msg = {
>>>          .hdr.request = request,
>>> -        .hdr.flags = VHOST_USER_VERSION,
>>> +        .hdr.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
>>>      };
>>>
>>>      if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
>>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>>> b/subprojects/libvhost-user/libvhost-user.c
>>> index 9c630c2170..40f665bd7f 100644
>>> --- a/subprojects/libvhost-user/libvhost-user.c
>>> +++ b/subprojects/libvhost-user/libvhost-user.c
>>> @@ -667,6 +667,7 @@ vu_send_reply(VuDev *dev, int conn_fd, VhostUserMsg
>>> *vmsg)
>>>  {
>>>      /* Set the version in the flags when sending the reply */
>>>      vmsg->flags &= ~VHOST_USER_VERSION_MASK;
>>> +    vmsg->flags &= ~VHOST_USER_NEED_REPLY_MASK;
>>>      vmsg->flags |= VHOST_USER_VERSION;
>>>      vmsg->flags |= VHOST_USER_REPLY_MASK;
>>>
>>> --
>>> 2.34.1
>>>
>>>