| Message ID | 1648621997-22416-8-git-send-email-si-wei.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-vdpa multiqueue fixes | expand |
On Wed, Mar 30, 2022 at 2:33 PM 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. > > 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: Jason Wang <jasowang@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 >
On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu 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 Little typo s/iotls/ioctls >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. > >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> >--- > 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 > > With that: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Unrelated to this patch, but the name vhost_vdpa_one_time_request() is confusing IMHO. Not that I'm good with names, but how about we change it to vhost_vdpa_skip_one_time_request()? Thanks, Stefano
On 3/30/2022 9:24 AM, Stefano Garzarella wrote: > On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu 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 > > Little typo s/iotls/ioctls Thanks! Will correct it in v2. > >> 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. >> >> 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> >> --- >> 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 >> >> > > With that: > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > > > Unrelated to this patch, but the name vhost_vdpa_one_time_request() is > confusing IMHO. Coincidentally I got the same feeling and wanted to rename it to something else, too. > > Not that I'm good with names, but how about we change it to > vhost_vdpa_skip_one_time_request()? How about vhost_vdpa_request_already_applied()? seems to be more readable in the context. -Siwei > > Thanks, > Stefano >
On Wed, Mar 30, 2022 at 10:12:42AM -0700, Si-Wei Liu wrote: > > >On 3/30/2022 9:24 AM, Stefano Garzarella wrote: >>On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu 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 >> >>Little typo s/iotls/ioctls >Thanks! Will correct it in v2. > >> >>>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. >>> >>>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> >>>--- >>>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 >>> >>> >> >>With that: >> >>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >> >> >> >>Unrelated to this patch, but the name vhost_vdpa_one_time_request() >>is confusing IMHO. >Coincidentally I got the same feeling and wanted to rename it to >something else, too. > >> >>Not that I'm good with names, but how about we change it to >>vhost_vdpa_skip_one_time_request()? >How about vhost_vdpa_request_already_applied()? seems to be more >readable in the context. That's fine too, except you can't discern that it's a single request per device, so maybe I'd add "dev," but I don't know if it gets too long: vhost_vdpa_dev_request_already_applied() And I would also add a comment to the function to explain that we use that function for requests that only need to be applied once, regardless of the number of queues. Thanks, Stefano
On Wed, Mar 30, 2022 at 7:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Wed, Mar 30, 2022 at 10:12:42AM -0700, Si-Wei Liu wrote: > > > > > >On 3/30/2022 9:24 AM, Stefano Garzarella wrote: > >>On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu 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 > >> > >>Little typo s/iotls/ioctls > >Thanks! Will correct it in v2. > > > >> > >>>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. > >>> > >>>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> > >>>--- > >>>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 > >>> > >>> > >> > >>With that: > >> > >>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >> > >> > >> > >>Unrelated to this patch, but the name vhost_vdpa_one_time_request() > >>is confusing IMHO. > >Coincidentally I got the same feeling and wanted to rename it to > >something else, too. > > > >> > >>Not that I'm good with names, but how about we change it to > >>vhost_vdpa_skip_one_time_request()? > >How about vhost_vdpa_request_already_applied()? seems to be more > >readable in the context. > > That's fine too, except you can't discern that it's a single request per > device, so maybe I'd add "dev," but I don't know if it gets too long: > > vhost_vdpa_dev_request_already_applied() > > And I would also add a comment to the function to explain that we use > that function for requests that only need to be applied once, regardless > of the number of queues. > In my opinion it should express what it checks: vhost_vdpa_first, or vhost_vdpa_first_dev, vhost_vdpa_first_queue... and to add a comment like the one you propose. I think the context of the caller gives enough information. I would add that the use is for requests that only need / must be applied once *and before setting up queues*, *at the beginning of operation*, or similar, because we do a similar check with dev->vq_index_end and these are not exchangeable. Thanks! > Thanks, > Stefano >
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. 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 >
On 3/30/2022 11:27 AM, Eugenio Perez Martin wrote: > On Wed, Mar 30, 2022 at 7:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> On Wed, Mar 30, 2022 at 10:12:42AM -0700, Si-Wei Liu wrote: >>> >>> On 3/30/2022 9:24 AM, Stefano Garzarella wrote: >>>> On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu 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 >>>> Little typo s/iotls/ioctls >>> Thanks! Will correct it in v2. >>> >>>>> 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. >>>>> >>>>> 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> >>>>> --- >>>>> 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 >>>>> >>>>> >>>> With that: >>>> >>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >>>> >>>> >>>> >>>> Unrelated to this patch, but the name vhost_vdpa_one_time_request() >>>> is confusing IMHO. >>> Coincidentally I got the same feeling and wanted to rename it to >>> something else, too. >>> >>>> Not that I'm good with names, but how about we change it to >>>> vhost_vdpa_skip_one_time_request()? >>> How about vhost_vdpa_request_already_applied()? seems to be more >>> readable in the context. >> That's fine too, except you can't discern that it's a single request per >> device, so maybe I'd add "dev," but I don't know if it gets too long: >> >> vhost_vdpa_dev_request_already_applied() >> >> And I would also add a comment to the function to explain that we use >> that function for requests that only need to be applied once, regardless >> of the number of queues. >> > In my opinion it should express what it checks: vhost_vdpa_first, or > vhost_vdpa_first_dev, vhost_vdpa_first_queue... Indeed, the same name ever came to my mind that is to reflect what it actually checks. It is just that I have to flip the polarity of the vhost_vdpa_one_time_request() function that made me adopt another name. What matches best for the current code would be something similar to vhost_vdpa_dev_other_than_the_first(), though why bother using another function rather than write the check as-is in place is another question. > and to add a comment > like the one you propose. Will do. > I think the context of the caller gives > enough information. > > I would add that the use is for requests that only need / must be > applied once *and before setting up queues*, *at the beginning of > operation*, or similar, because we do a similar check with > dev->vq_index_end and these are not exchangeable. Nods. Exactly. -Siwei > > Thanks! > >> Thanks, >> Stefano >>
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. 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. -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 >>
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 > >> >
在 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 >>>>
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. 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 > >>>> >
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.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. 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? > 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.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 >>>>
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 > > >>>> > > >
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 >>>>>>>
On Thu, Mar 31, 2022 at 11:15 PM Si-Wei Liu <si-wei.liu@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.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. > 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.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 > >>>> >
On Fri, Apr 1, 2022 at 12:18 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > > > 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. Right. > > > 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(). Yes, I miss that optimization. Thanks > > -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 > >>>>>>> >
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;
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. 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> --- hw/virtio/vhost-vdpa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)