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

[Jason Wang <jasowang@redhat.com>]

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

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