Re: [PATCH 7/7] vhost-vdpa: backend feature should set only once

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 3/31/2022 7:39 PM, Jason Wang wrote:
> On Thu, Mar 31, 2022 at 5:20 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
>> On Thu, Mar 31, 2022 at 10:54 AM Jason Wang <jasowang@redhat.com> wrote:
>>>
>>> 在 2022/3/31 下午4:02, Eugenio Perez Martin 写道:
>>>> On Thu, Mar 31, 2022 at 1:03 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>>
>>>>> On 3/30/2022 12:01 PM, Eugenio Perez Martin wrote:
>>>>>> On Wed, Mar 30, 2022 at 8:33 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>>>> The vhost_vdpa_one_time_request() branch in
>>>>>>> vhost_vdpa_set_backend_cap() incorrectly sends down
>>>>>>> iotls on vhost_dev with non-zero index. This may
>>>>>>> end up with multiple VHOST_SET_BACKEND_FEATURES
>>>>>>> ioctl calls sent down on the vhost-vdpa fd that is
>>>>>>> shared between all these vhost_dev's.
>>>>>>>
>>>>>> Not only that. This means that qemu thinks the device supports iotlb
>>>>>> batching as long as the device does not have cvq. If vdpa does not
>>>>>> support batching, it will return an error later with no possibility of
>>>>>> doing it ok.
>>>>> I think the implicit assumption here is that the caller should back off
>>>>> to where it was if it comes to error i.e. once the first
>>>>> vhost_dev_set_features call gets an error, vhost_dev_start() will fail
>>>>> straight.
>>>> Sorry, I don't follow you here, and maybe my message was not clear enough.
>>>>
>>>> What I meant is that your patch fixes another problem not stated in
>>>> the message: it is not possible to initialize a net vdpa device that
>>>> does not have cvq and does not support iotlb batches without it. Qemu
>>>> will assume that the device supports batching, so the write of
>>>> VHOST_IOTLB_BATCH_BEGIN will fail. I didn't test what happens next but
>>>> it probably cannot continue.
>>>
>>> So you mean we actually didn't call VHOST_SET_BACKEND_CAP in this case.
>>> Fortunately, kernel didn't check the backend cap when accepting batching
>>> hints.
>>>
>>> We are probably fine?
>>>
>> We're fine as long as the vdpa driver in the kernel effectively
>> supports batching. If not, qemu will try to batch, and it will fail.
>>
>> It was introduced in v5.9, so qemu has not supported kernel <5.9 since
>> we introduced multiqueue support (I didn't test). Unless we apply this
>> patch. That's the reason it should be marked as fixed and backported
>> to stable IMO.
> Ok, so it looks to me we have more issues.
>
> In vhost_vdpa_set_backend_cap() we fail when
> VHOST_VDPA_GET_BACKEND_FEATURES fails. This breaks the older kernel
> since that ioctl is introduced in
>
> 653055b9acd4 ("vhost-vdpa: support get/set backend features")
Yep, the GET/SET_BACKEND ioctl pair got introduced together in this 
exact commit.
>
> We should:
>
> 1) make it work by not failing the vhost_vdpa_set_backend_cap() and
> assuming MSG_V2.
This issue is orthogonal with my fix, which was pre-existing before the 
multiqueue support. I believe there should be another separate patch to 
fix QEMU for pre-GET/SET_BACKEND kernel.

> 2) check the batching support in vhost_vdpa_listener_begin_batch()
> instead of trying to set VHOST_IOTLB_BATCH_BEGIN uncondtionally
This is non-issue since VHOST_BACKEND_F_IOTLB_BATCH is already validated 
in the caller vhost_vdpa_iotlb_batch_begin_once().

-Siwei
>
> Thanks
>
>> Thanks!
>>
>>> Thanks
>>>
>>>
>>>> In that regard, this commit needs to be marked as "Fixes: ...", either
>>>> ("a5bd058 vhost-vdpa: batch updating IOTLB mappings") or maybe better
>>>> ("4d191cf vhost-vdpa: classify one time request"). We have a
>>>> regression if we introduce both, or the second one and the support of
>>>> any other backend feature.
>>>>
>>>>> Noted that the VHOST_SET_BACKEND_FEATURES ioctl is not per-vq
>>>>> and it doesn't even need to. There seems to me no possibility for it to
>>>>> fail in a way as thought here. The capture is that IOTLB batching is at
>>>>> least a vdpa device level backend feature, if not per-kernel. Same as
>>>>> IOTLB_MSG_V2.
>>>>>
>>>> At this moment it is per-kernel, yes. With your patch there is no need
>>>> to fail because of the lack of _F_IOTLB_BATCH, the code should handle
>>>> this case ok.
>>>>
>>>> But if VHOST_GET_BACKEND_FEATURES returns no support for
>>>> VHOST_BACKEND_F_IOTLB_MSG_V2, the qemu code will happily send v2
>>>> messages anyway. This has nothing to do with the patch, I'm just
>>>> noting it here.
>>>>
>>>> In that case, maybe it is better to return something like -ENOTSUP?
>>>>
>>>> Thanks!
>>>>
>>>>> -Siwei
>>>>>
>>>>>>     Some open questions:
>>>>>>
>>>>>> Should we make the vdpa driver return error as long as a feature is
>>>>>> used but not set by qemu, or let it as undefined? I guess we have to
>>>>>> keep the batching at least without checking so the kernel supports old
>>>>>> versions of qemu.
>>>>>>
>>>>>> On the other hand, should we return an error if IOTLB_MSG_V2 is not
>>>>>> supported here? We're basically assuming it in other functions.
>>>>>>
>>>>>>> To fix it, send down ioctl only once via the first
>>>>>>> vhost_dev with index 0. Toggle the polarity of the
>>>>>>> vhost_vdpa_one_time_request() test would do the trick.
>>>>>>>
>>>>>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>>> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>
>>>>>>> ---
>>>>>>>     hw/virtio/vhost-vdpa.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>>>>> index c5ed7a3..27ea706 100644
>>>>>>> --- a/hw/virtio/vhost-vdpa.c
>>>>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>>>>> @@ -665,7 +665,7 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
>>>>>>>
>>>>>>>         features &= f;
>>>>>>>
>>>>>>> -    if (vhost_vdpa_one_time_request(dev)) {
>>>>>>> +    if (!vhost_vdpa_one_time_request(dev)) {
>>>>>>>             r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
>>>>>>>             if (r) {
>>>>>>>                 return -EFAULT;
>>>>>>> --
>>>>>>> 1.8.3.1
>>>>>>>