[v4,04/19] vhost: fix payload size of reply
Checks
Commit Message
QEMU doesn't expect any payload for the reply of
VHOST_USER_SET_LOG_BASE request, so don't send any.
Note that the Vhost-user specification isn't clear about
it and would need to be fixed.
Fixes: 54f9e32305d4 ("vhost: handle dirty pages logging request")
Cc: stable@dpdk.org
Reported-by: Ilya Maximets <i.maximets@samsung.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Acked-by: Ilya Maximets <i.maximets@samsung.com>
---
lib/librte_vhost/vhost_user.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Comments
On Mon, Oct 08, 2018 at 05:25:42PM +0200, Maxime Coquelin wrote:
> QEMU doesn't expect any payload for the reply of
> VHOST_USER_SET_LOG_BASE request, so don't send any.
> Note that the Vhost-user specification isn't clear about
> it and would need to be fixed.
>
> Fixes: 54f9e32305d4 ("vhost: handle dirty pages logging request")
> Cc: stable@dpdk.org
>
> Reported-by: Ilya Maximets <i.maximets@samsung.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Acked-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> lib/librte_vhost/vhost_user.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 7f3e86778..71a0e7dd7 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1296,7 +1296,11 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg)
> dev->log_base = dev->log_addr + off;
> dev->log_size = size;
>
> - msg->size = sizeof(msg->payload.u64);
> + /*
> + * The spec is not clear about it (yet), but QEMU doesn't expect
> + * any payload in the reply.
> + */
> + msg->size = 0;
I think the spec is clear about it that no payload is
expected in the reply:
https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L496
But below line in the spec needs to be fixed:
https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L495
>
> return VH_RESULT_REPLY;
> }
> --
> 2.17.1
>
On 10/09/2018 12:21 PM, Tiwei Bie wrote:
> On Mon, Oct 08, 2018 at 05:25:42PM +0200, Maxime Coquelin wrote:
>> QEMU doesn't expect any payload for the reply of
>> VHOST_USER_SET_LOG_BASE request, so don't send any.
>> Note that the Vhost-user specification isn't clear about
>> it and would need to be fixed.
>>
>> Fixes: 54f9e32305d4 ("vhost: handle dirty pages logging request")
>> Cc: stable@dpdk.org
>>
>> Reported-by: Ilya Maximets <i.maximets@samsung.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Acked-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>> lib/librte_vhost/vhost_user.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 7f3e86778..71a0e7dd7 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1296,7 +1296,11 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg)
>> dev->log_base = dev->log_addr + off;
>> dev->log_size = size;
>>
>> - msg->size = sizeof(msg->payload.u64);
>> + /*
>> + * The spec is not clear about it (yet), but QEMU doesn't expect
>> + * any payload in the reply.
>> + */
>> + msg->size = 0;
>
> I think the spec is clear about it that no payload is
> expected in the reply:
>
> https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L496
It isn't really clear because there are two cases[0]:
1. VHOST_USER_PROTOCOL_F_LOG_SHMFD not negotiated: In this case no reply
has to be sent.
2. VHOST_USER_PROTOCOL_F_LOG_SHMFD negotiated: In this case reply with
empty payload hsa to be sent.
[0]:
https://github.com/qemu/qemu/blob/7c69b7c849641a39ba3defa40d384a2ba24cd7a2/docs/interop/vhost-user.txt#L177
> But below line in the spec needs to be fixed:
>
> https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L495
Indeed.
>
>>
>> return VH_RESULT_REPLY;
>> }
>> --
>> 2.17.1
>>
On 10/09/2018 12:30 PM, Maxime Coquelin wrote:
>
>
> On 10/09/2018 12:21 PM, Tiwei Bie wrote:
>> On Mon, Oct 08, 2018 at 05:25:42PM +0200, Maxime Coquelin wrote:
>>> QEMU doesn't expect any payload for the reply of
>>> VHOST_USER_SET_LOG_BASE request, so don't send any.
>>> Note that the Vhost-user specification isn't clear about
>>> it and would need to be fixed.
>>>
>>> Fixes: 54f9e32305d4 ("vhost: handle dirty pages logging request")
>>> Cc: stable@dpdk.org
>>>
>>> Reported-by: Ilya Maximets <i.maximets@samsung.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Acked-by: Ilya Maximets <i.maximets@samsung.com>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c
>>> b/lib/librte_vhost/vhost_user.c
>>> index 7f3e86778..71a0e7dd7 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1296,7 +1296,11 @@ vhost_user_set_log_base(struct virtio_net
>>> **pdev, struct VhostUserMsg *msg)
>>> dev->log_base = dev->log_addr + off;
>>> dev->log_size = size;
>>> - msg->size = sizeof(msg->payload.u64);
>>> + /*
>>> + * The spec is not clear about it (yet), but QEMU doesn't expect
>>> + * any payload in the reply.
>>> + */
>>> + msg->size = 0;
>>
>> I think the spec is clear about it that no payload is
>> expected in the reply:
>>
>> https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L496
>>
> It isn't really clear because there are two cases[0]:
> 1. VHOST_USER_PROTOCOL_F_LOG_SHMFD not negotiated: In this case no reply
> has to be sent.
> 2. VHOST_USER_PROTOCOL_F_LOG_SHMFD negotiated: In this case reply with
> empty payload hsa to be sent.
>
> [0]:
> https://github.com/qemu/qemu/blob/7c69b7c849641a39ba3defa40d384a2ba24cd7a2/docs/interop/vhost-user.txt#L177
>
And I would add that N/A isn't clear, because SET_VRING_ADDR specifies
N/A as slave payload, whereas the implementation does not implement a
reply.
>> But below line in the spec needs to be fixed:
>>
>> https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L495
>>
>
> Indeed.
>
>>
>>> return VH_RESULT_REPLY;
>>> }
>>> --
>>> 2.17.1
>>>
On Tue, Oct 09, 2018 at 12:34:28PM +0200, Maxime Coquelin wrote:
> On 10/09/2018 12:30 PM, Maxime Coquelin wrote:
> > On 10/09/2018 12:21 PM, Tiwei Bie wrote:
> > > On Mon, Oct 08, 2018 at 05:25:42PM +0200, Maxime Coquelin wrote:
> > > > QEMU doesn't expect any payload for the reply of
> > > > VHOST_USER_SET_LOG_BASE request, so don't send any.
> > > > Note that the Vhost-user specification isn't clear about
> > > > it and would need to be fixed.
> > > >
> > > > Fixes: 54f9e32305d4 ("vhost: handle dirty pages logging request")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Reported-by: Ilya Maximets <i.maximets@samsung.com>
> > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > > Acked-by: Ilya Maximets <i.maximets@samsung.com>
> > > > ---
> > > > lib/librte_vhost/vhost_user.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/librte_vhost/vhost_user.c
> > > > b/lib/librte_vhost/vhost_user.c
> > > > index 7f3e86778..71a0e7dd7 100644
> > > > --- a/lib/librte_vhost/vhost_user.c
> > > > +++ b/lib/librte_vhost/vhost_user.c
> > > > @@ -1296,7 +1296,11 @@ vhost_user_set_log_base(struct virtio_net
> > > > **pdev, struct VhostUserMsg *msg)
> > > > dev->log_base = dev->log_addr + off;
> > > > dev->log_size = size;
> > > > - msg->size = sizeof(msg->payload.u64);
> > > > + /*
> > > > + * The spec is not clear about it (yet), but QEMU doesn't expect
> > > > + * any payload in the reply.
> > > > + */
> > > > + msg->size = 0;
> > >
> > > I think the spec is clear about it that no payload is
> > > expected in the reply:
> > >
> > > https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L496
> > >
> > It isn't really clear because there are two cases[0]:
> > 1. VHOST_USER_PROTOCOL_F_LOG_SHMFD not negotiated: In this case no reply
> > has to be sent.
> > 2. VHOST_USER_PROTOCOL_F_LOG_SHMFD negotiated: In this case reply with
> > empty payload hsa to be sent.
> >
> > [0]: https://github.com/qemu/qemu/blob/7c69b7c849641a39ba3defa40d384a2ba24cd7a2/docs/interop/vhost-user.txt#L177
> >
>
> And I would add that N/A isn't clear, because SET_VRING_ADDR specifies
> N/A as slave payload, whereas the implementation does not implement a
> reply.
Good point.
>
> > > But below line in the spec needs to be fixed:
> > >
> > > https://github.com/qemu/qemu/blob/7c69b7c84964/docs/interop/vhost-user.txt#L495
> > >
> >
> > Indeed.
> >
> > >
> > > > return VH_RESULT_REPLY;
> > > > }
> > > > --
> > > > 2.17.1
> > > >
@@ -1296,7 +1296,11 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg)
dev->log_base = dev->log_addr + off;
dev->log_size = size;
- msg->size = sizeof(msg->payload.u64);
+ /*
+ * The spec is not clear about it (yet), but QEMU doesn't expect
+ * any payload in the reply.
+ */
+ msg->size = 0;
return VH_RESULT_REPLY;
}