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