qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Si-Wei Liu
Subject: Re: [PATCH 7/7] vhost-vdpa: backend feature should set only once
Date: Thu, 31 Mar 2022 21:18:13 -0700
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 


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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]