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

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

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