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

[Jason Wang]

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

We should:

1) make it work by not failing the vhost_vdpa_set_backend_cap() and
assuming MSG_V2.
2) check the batching support in vhost_vdpa_listener_begin_batch()
instead of trying to set VHOST_IOTLB_BATCH_BEGIN uncondtionally

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