[Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature

jiangyiwen posted 1 patch 5 years, 3 months ago
Test asan passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/5C10DAE9.3010605@huawei.com
hw/virtio/vhost-vsock.c                       | 9 +++++++--
include/standard-headers/linux/virtio_vsock.h | 3 +++
2 files changed, 10 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by jiangyiwen 5 years, 3 months ago
Currently vhost-vsock doesn't have any feature bits, so it
don't support parse mergeable rx buffer feature. And the
feature is support in another series of patches named
"VSOCK: support mergeable rx buffer in vhost-vsock".

So we neet to support parse mergeable feature in vhost-vsock
if above patches are merged.

Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
---
 hw/virtio/vhost-vsock.c                       | 9 +++++++--
 include/standard-headers/linux/virtio_vsock.h | 3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index aa5af92..5023c05 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -178,8 +178,13 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
                                          uint64_t requested_features,
                                          Error **errp)
 {
-    /* No feature bits used yet */
-    return requested_features;
+    VHostVSock *vsock = VHOST_VSOCK(vdev);
+    uint64_t features;
+
+    virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_MRG_RXBUF);
+    features = requested_features & vsock->vhost_dev.features;
+
+    return features;
 }

 static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
index be44321..4c583ec 100644
--- a/include/standard-headers/linux/virtio_vsock.h
+++ b/include/standard-headers/linux/virtio_vsock.h
@@ -38,6 +38,9 @@
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"

+/* Virtio-vsock feature */
+#define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
+
 struct virtio_vsock_config {
 	uint64_t guest_cid;
 } QEMU_PACKED;
-- 
1.8.3.1



Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by Michael S. Tsirkin 5 years, 3 months ago
On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
> Currently vhost-vsock doesn't have any feature bits, so it
> don't support parse mergeable rx buffer feature. And the
> feature is support in another series of patches named
> "VSOCK: support mergeable rx buffer in vhost-vsock".
> 
> So we neet to support parse mergeable feature in vhost-vsock
> if above patches are merged.
> 
> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>

OK but what does it do? this just defines the feature bit ...
Also pls copy virtio-dev whenever you change the host/guest
interface.

Thanks!

> ---
>  hw/virtio/vhost-vsock.c                       | 9 +++++++--
>  include/standard-headers/linux/virtio_vsock.h | 3 +++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index aa5af92..5023c05 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -178,8 +178,13 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
>                                           uint64_t requested_features,
>                                           Error **errp)
>  {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
> +    uint64_t features;
> +
> +    virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_MRG_RXBUF);
> +    features = requested_features & vsock->vhost_dev.features;
> +
> +    return features;
>  }
> 
>  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
> index be44321..4c583ec 100644
> --- a/include/standard-headers/linux/virtio_vsock.h
> +++ b/include/standard-headers/linux/virtio_vsock.h
> @@ -38,6 +38,9 @@
>  #include "standard-headers/linux/virtio_ids.h"
>  #include "standard-headers/linux/virtio_config.h"
> 
> +/* Virtio-vsock feature */
> +#define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
> +
>  struct virtio_vsock_config {
>  	uint64_t guest_cid;
>  } QEMU_PACKED;
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by jiangyiwen 5 years, 3 months ago
On 2018/12/12 21:19, Michael S. Tsirkin wrote:
> On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
>> Currently vhost-vsock doesn't have any feature bits, so it
>> don't support parse mergeable rx buffer feature. And the
>> feature is support in another series of patches named
>> "VSOCK: support mergeable rx buffer in vhost-vsock".
>>
>> So we neet to support parse mergeable feature in vhost-vsock
>> if above patches are merged.
>>
>> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> 
> OK but what does it do? this just defines the feature bit ...
> Also pls copy virtio-dev whenever you change the host/guest
> interface.
> 
> Thanks!
> 

Hi Michael,

In my opinion, for vhost device, device feature bits need to be
intersected between vhost and qemu, so I add mergeable rx buffer feature
into vdev->host_features, and then intersected with vhost_dev features.

Later if someone want to add new feature, it can use virtio_add_feature()
in vhost_vsock_get_features().

Thanks,
Yiwen.

>> ---
>>  hw/virtio/vhost-vsock.c                       | 9 +++++++--
>>  include/standard-headers/linux/virtio_vsock.h | 3 +++
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index aa5af92..5023c05 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -178,8 +178,13 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
>>                                           uint64_t requested_features,
>>                                           Error **errp)
>>  {
>> -    /* No feature bits used yet */
>> -    return requested_features;
>> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
>> +    uint64_t features;
>> +
>> +    virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_MRG_RXBUF);
>> +    features = requested_features & vsock->vhost_dev.features;
>> +
>> +    return features;
>>  }
>>
>>  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>> diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
>> index be44321..4c583ec 100644
>> --- a/include/standard-headers/linux/virtio_vsock.h
>> +++ b/include/standard-headers/linux/virtio_vsock.h
>> @@ -38,6 +38,9 @@
>>  #include "standard-headers/linux/virtio_ids.h"
>>  #include "standard-headers/linux/virtio_config.h"
>>
>> +/* Virtio-vsock feature */
>> +#define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
>> +
>>  struct virtio_vsock_config {
>>  	uint64_t guest_cid;
>>  } QEMU_PACKED;
>> -- 
>> 1.8.3.1
>>
> 
> .
> 



Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by Michael S. Tsirkin 5 years, 3 months ago
On Thu, Dec 13, 2018 at 11:27:24AM +0800, jiangyiwen wrote:
> On 2018/12/12 21:19, Michael S. Tsirkin wrote:
> > On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
> >> Currently vhost-vsock doesn't have any feature bits, so it
> >> don't support parse mergeable rx buffer feature. And the
> >> feature is support in another series of patches named
> >> "VSOCK: support mergeable rx buffer in vhost-vsock".
> >>
> >> So we neet to support parse mergeable feature in vhost-vsock
> >> if above patches are merged.
> >>
> >> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> > 
> > OK but what does it do? this just defines the feature bit ...
> > Also pls copy virtio-dev whenever you change the host/guest
> > interface.
> > 
> > Thanks!
> > 
> 
> Hi Michael,
> 
> In my opinion, for vhost device, device feature bits need to be
> intersected between vhost and qemu, so I add mergeable rx buffer feature
> into vdev->host_features, and then intersected with vhost_dev features.
> 
> Later if someone want to add new feature, it can use virtio_add_feature()
> in vhost_vsock_get_features().
> 
> Thanks,
> Yiwen.

I understand. Sorry.  It seems that your patchset wasn't threaded
properly, each patch was by its own. Can you pls take a look at fixing
that?

> >> ---
> >>  hw/virtio/vhost-vsock.c                       | 9 +++++++--
> >>  include/standard-headers/linux/virtio_vsock.h | 3 +++
> >>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> >> index aa5af92..5023c05 100644
> >> --- a/hw/virtio/vhost-vsock.c
> >> +++ b/hw/virtio/vhost-vsock.c
> >> @@ -178,8 +178,13 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
> >>                                           uint64_t requested_features,
> >>                                           Error **errp)
> >>  {
> >> -    /* No feature bits used yet */
> >> -    return requested_features;
> >> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
> >> +    uint64_t features;
> >> +
> >> +    virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_MRG_RXBUF);
> >> +    features = requested_features & vsock->vhost_dev.features;
> >> +
> >> +    return features;
> >>  }
> >>
> >>  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >> diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
> >> index be44321..4c583ec 100644
> >> --- a/include/standard-headers/linux/virtio_vsock.h
> >> +++ b/include/standard-headers/linux/virtio_vsock.h
> >> @@ -38,6 +38,9 @@
> >>  #include "standard-headers/linux/virtio_ids.h"
> >>  #include "standard-headers/linux/virtio_config.h"
> >>
> >> +/* Virtio-vsock feature */
> >> +#define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
> >> +
> >>  struct virtio_vsock_config {
> >>  	uint64_t guest_cid;
> >>  } QEMU_PACKED;
> >> -- 
> >> 1.8.3.1
> >>
> > 
> > .
> > 
> 

Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by jiangyiwen 5 years, 3 months ago
On 2018/12/13 22:54, Michael S. Tsirkin wrote:
> On Thu, Dec 13, 2018 at 11:27:24AM +0800, jiangyiwen wrote:
>> On 2018/12/12 21:19, Michael S. Tsirkin wrote:
>>> On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
>>>> Currently vhost-vsock doesn't have any feature bits, so it
>>>> don't support parse mergeable rx buffer feature. And the
>>>> feature is support in another series of patches named
>>>> "VSOCK: support mergeable rx buffer in vhost-vsock".
>>>>
>>>> So we neet to support parse mergeable feature in vhost-vsock
>>>> if above patches are merged.
>>>>
>>>> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
>>>
>>> OK but what does it do? this just defines the feature bit ...
>>> Also pls copy virtio-dev whenever you change the host/guest
>>> interface.
>>>
>>> Thanks!
>>>
>>
>> Hi Michael,
>>
>> In my opinion, for vhost device, device feature bits need to be
>> intersected between vhost and qemu, so I add mergeable rx buffer feature
>> into vdev->host_features, and then intersected with vhost_dev features.
>>
>> Later if someone want to add new feature, it can use virtio_add_feature()
>> in vhost_vsock_get_features().
>>
>> Thanks,
>> Yiwen.
> 
> I understand. Sorry.  It seems that your patchset wasn't threaded
> properly, each patch was by its own. Can you pls take a look at fixing
> that?
> 

Hi Michael,

Thanks your suggestions, I want to how to send patch when
patch involving multiple communities, like qemu
and linux kernel. In this case, how should we send them?

Thanks,
Yiwen.

>>>> ---
>>>>  hw/virtio/vhost-vsock.c                       | 9 +++++++--
>>>>  include/standard-headers/linux/virtio_vsock.h | 3 +++
>>>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>>>> index aa5af92..5023c05 100644
>>>> --- a/hw/virtio/vhost-vsock.c
>>>> +++ b/hw/virtio/vhost-vsock.c
>>>> @@ -178,8 +178,13 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
>>>>                                           uint64_t requested_features,
>>>>                                           Error **errp)
>>>>  {
>>>> -    /* No feature bits used yet */
>>>> -    return requested_features;
>>>> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
>>>> +    uint64_t features;
>>>> +
>>>> +    virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_MRG_RXBUF);
>>>> +    features = requested_features & vsock->vhost_dev.features;
>>>> +
>>>> +    return features;
>>>>  }
>>>>
>>>>  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>>> diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
>>>> index be44321..4c583ec 100644
>>>> --- a/include/standard-headers/linux/virtio_vsock.h
>>>> +++ b/include/standard-headers/linux/virtio_vsock.h
>>>> @@ -38,6 +38,9 @@
>>>>  #include "standard-headers/linux/virtio_ids.h"
>>>>  #include "standard-headers/linux/virtio_config.h"
>>>>
>>>> +/* Virtio-vsock feature */
>>>> +#define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
>>>> +
>>>>  struct virtio_vsock_config {
>>>>  	uint64_t guest_cid;
>>>>  } QEMU_PACKED;
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 



Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by Michael S. Tsirkin 5 years, 3 months ago
On Fri, Dec 14, 2018 at 05:43:28PM +0800, jiangyiwen wrote:
> On 2018/12/13 22:54, Michael S. Tsirkin wrote:
> > On Thu, Dec 13, 2018 at 11:27:24AM +0800, jiangyiwen wrote:
> >> On 2018/12/12 21:19, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
> >>>> Currently vhost-vsock doesn't have any feature bits, so it
> >>>> don't support parse mergeable rx buffer feature. And the
> >>>> feature is support in another series of patches named
> >>>> "VSOCK: support mergeable rx buffer in vhost-vsock".
> >>>>
> >>>> So we neet to support parse mergeable feature in vhost-vsock
> >>>> if above patches are merged.
> >>>>
> >>>> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> >>>
> >>> OK but what does it do? this just defines the feature bit ...
> >>> Also pls copy virtio-dev whenever you change the host/guest
> >>> interface.
> >>>
> >>> Thanks!
> >>>
> >>
> >> Hi Michael,
> >>
> >> In my opinion, for vhost device, device feature bits need to be
> >> intersected between vhost and qemu, so I add mergeable rx buffer feature
> >> into vdev->host_features, and then intersected with vhost_dev features.
> >>
> >> Later if someone want to add new feature, it can use virtio_add_feature()
> >> in vhost_vsock_get_features().
> >>
> >> Thanks,
> >> Yiwen.
> > 
> > I understand. Sorry.  It seems that your patchset wasn't threaded
> > properly, each patch was by its own. Can you pls take a look at fixing
> > that?
> > 
> 
> Hi Michael,
> 
> Thanks your suggestions, I want to how to send patch when
> patch involving multiple communities, like qemu
> and linux kernel. In this case, how should we send them?
> 
> Thanks,
> Yiwen.


Both use the same threading so just copy both.


> >>>> ---
> >>>>  hw/virtio/vhost-vsock.c                       | 9 +++++++--
> >>>>  include/standard-headers/linux/virtio_vsock.h | 3 +++
> >>>>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> >>>> index aa5af92..5023c05 100644
> >>>> --- a/hw/virtio/vhost-vsock.c
> >>>> +++ b/hw/virtio/vhost-vsock.c
> >>>> @@ -178,8 +178,13 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,
> >>>>                                           uint64_t requested_features,
> >>>>                                           Error **errp)
> >>>>  {
> >>>> -    /* No feature bits used yet */
> >>>> -    return requested_features;
> >>>> +    VHostVSock *vsock = VHOST_VSOCK(vdev);
> >>>> +    uint64_t features;
> >>>> +
> >>>> +    virtio_add_feature(&requested_features, VIRTIO_VSOCK_F_MRG_RXBUF);
> >>>> +    features = requested_features & vsock->vhost_dev.features;
> >>>> +
> >>>> +    return features;
> >>>>  }
> >>>>
> >>>>  static void vhost_vsock_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >>>> diff --git a/include/standard-headers/linux/virtio_vsock.h b/include/standard-headers/linux/virtio_vsock.h
> >>>> index be44321..4c583ec 100644
> >>>> --- a/include/standard-headers/linux/virtio_vsock.h
> >>>> +++ b/include/standard-headers/linux/virtio_vsock.h
> >>>> @@ -38,6 +38,9 @@
> >>>>  #include "standard-headers/linux/virtio_ids.h"
> >>>>  #include "standard-headers/linux/virtio_config.h"
> >>>>
> >>>> +/* Virtio-vsock feature */
> >>>> +#define VIRTIO_VSOCK_F_MRG_RXBUF 0 /* Host can merge receive buffers. */
> >>>> +
> >>>>  struct virtio_vsock_config {
> >>>>  	uint64_t guest_cid;
> >>>>  } QEMU_PACKED;
> >>>> -- 
> >>>> 1.8.3.1
> >>>>
> >>>
> >>> .
> >>>
> >>
> > 
> > .
> > 
> 

Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by Stefan Hajnoczi 5 years, 3 months ago
On Thu, Dec 13, 2018 at 11:27:24AM +0800, jiangyiwen wrote:
> On 2018/12/12 21:19, Michael S. Tsirkin wrote:
> > On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
> >> Currently vhost-vsock doesn't have any feature bits, so it
> >> don't support parse mergeable rx buffer feature. And the
> >> feature is support in another series of patches named
> >> "VSOCK: support mergeable rx buffer in vhost-vsock".
> >>
> >> So we neet to support parse mergeable feature in vhost-vsock
> >> if above patches are merged.
> >>
> >> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> > 
> > OK but what does it do? this just defines the feature bit ...
> > Also pls copy virtio-dev whenever you change the host/guest
> > interface.
> > 
> > Thanks!
> > 
> 
> Hi Michael,
> 
> In my opinion, for vhost device, device feature bits need to be
> intersected between vhost and qemu, so I add mergeable rx buffer feature
> into vdev->host_features, and then intersected with vhost_dev features.
> 
> Later if someone want to add new feature, it can use virtio_add_feature()
> in vhost_vsock_get_features().

Hi Yiwen,
I think Michael's comment is about documenting the behavior of the new
feature bit.

Each change to a VIRTIO device host<->guest interface needs to be
discussed in the VIRTIO community <virtio-dev@lists.oasis-open.org> and
described in a patch to the VIRTIO specification
(https://github.com/oasis-tcs/virtio-spec/).

Stefan
Re: [Qemu-devel] [PATCH] vhost-vsock: support parse mergeable feature
Posted by jiangyiwen 5 years, 3 months ago
On 2018/12/13 23:34, Stefan Hajnoczi wrote:
> On Thu, Dec 13, 2018 at 11:27:24AM +0800, jiangyiwen wrote:
>> On 2018/12/12 21:19, Michael S. Tsirkin wrote:
>>> On Wed, Dec 12, 2018 at 05:54:49PM +0800, jiangyiwen wrote:
>>>> Currently vhost-vsock doesn't have any feature bits, so it
>>>> don't support parse mergeable rx buffer feature. And the
>>>> feature is support in another series of patches named
>>>> "VSOCK: support mergeable rx buffer in vhost-vsock".
>>>>
>>>> So we neet to support parse mergeable feature in vhost-vsock
>>>> if above patches are merged.
>>>>
>>>> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
>>>
>>> OK but what does it do? this just defines the feature bit ...
>>> Also pls copy virtio-dev whenever you change the host/guest
>>> interface.
>>>
>>> Thanks!
>>>
>>
>> Hi Michael,
>>
>> In my opinion, for vhost device, device feature bits need to be
>> intersected between vhost and qemu, so I add mergeable rx buffer feature
>> into vdev->host_features, and then intersected with vhost_dev features.
>>
>> Later if someone want to add new feature, it can use virtio_add_feature()
>> in vhost_vsock_get_features().
> 
> Hi Yiwen,
> I think Michael's comment is about documenting the behavior of the new
> feature bit.
> 
> Each change to a VIRTIO device host<->guest interface needs to be
> discussed in the VIRTIO community <virtio-dev@lists.oasis-open.org> and
> described in a patch to the VIRTIO specification
> (https://github.com/oasis-tcs/virtio-spec/).
> 
> Stefan
> 

Hi Stefan,

Ok, I will fix it in the later version, I have not sent
a series of patches before, thank you for your help.

Thanks again,
Yiwen.