On Thu, Mar 31, 2022 at 11:15 PM Si-Wei Liu <si-wei....@oracle.com> wrote:
>
>
>
> On 3/31/2022 1:02 AM, Eugenio Perez Martin wrote:
> > On Thu, Mar 31, 2022 at 1:03 AM Si-Wei Liu <si-wei....@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....@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.
> This is not what I see from the code? For e.g.
> vhost_vdpa_iotlb_batch_begin_once() has the following:
>
> 140 if (v->dev->backend_cap & (0x1ULL <<
> VHOST_BACKEND_F_IOTLB_BATCH) &&
> 141 !v->iotlb_batch_begin_sent) {
> 142 vhost_vdpa_listener_begin_batch(v);
> 143 }
>
> If backend_cap doesn't contain the VHOST_BACKEND_F_IOTLB_BATCH bit, QEMU
> shouldn't send down VHOST_IOTLB_BATCH_BEGIN...
>
> Noted in vhost_vdpa_set_backend_cap(), VHOST_GET_BACKEND_FEATURES was
> supposed to get the backend capability from the kernel ahead of the
> VHOST_SET_BACKEND_FEATURES call. In which case of your concern, at least
> feature VHOST_BACKEND_F_IOTLB_MSG_V2 should be successfully returned and
> stored in the backend_cap, even if the VHOST_SET_BACKEND_FEATURES ioctl
> was missed in between. Hence the resulting backend_cap shouldn't have
> the VHOST_BACKEND_F_IOTLB_BATCH bit set. What am I missing here?
>
You're right, I missed that the GET is not skipped, thanks!
>
> > 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.
> Sure, it's not that I am unwilling to add the "Fixes" tag, though I'd
> like to make sure if the worry is real upfront. Thanks for pointing it
> out anyway.
>
> Thanks,
> -Siwei
>
> >
> >> 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....@oracle.com>
> >>> Acked-by: Eugenio Pérez <epere...@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
> >>>>
>
