Re: [PATCH RFC v3 4/4] vhost-vdpa: introduce IOTLB_PERSIST backend feature bit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On 9/12/2023 12:01 AM, Jason Wang wrote:
On Tue, Sep 12, 2023 at 8:28 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:

On 9/10/2023 8:52 PM, Jason Wang wrote:
On Sat, Sep 9, 2023 at 9:46 PM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
Userspace needs this feature flag to distinguish if vhost-vdpa
iotlb in the kernel supports persistent IOTLB mapping across
device reset.
As discussed, the IOTLB persists for devices with platform IOMMU at
least. You've mentioned that this behaviour is covered by Qemu since
it reset IOTLB on reset. I wonder what happens if we simply decouple
the IOTLB reset from vDPA reset in Qemu. Could we meet bugs there?
Not sure I understand. Simple decouple meaning to remove memory_listener
registration/unregistration calls *unconditionally* from the vDPA dev
start/stop path in QEMU, or make it conditional around the existence of
If my memory is correct, currently the listeners were
registered/unregistered during start/stop. I mean what if we
register/unregister during init/free?
Yes, the move to init/cleanup was assumed in below response.

If unconditional, it breaks older host kernel, as the
older kernel still silently drops all mapping across vDPA reset (VM
Ok, right.

ending up with network loss afterwards; if make the QEMU change
conditional on PERSIST_IOTLB without the .reset_map API, it can't cover
the virtio-vdpa 1:1 identity mapping case.
Ok, I see. Let's add the above and explain why it can't cover the 1:1
mapping somewhere (probably the commit log) in the next version.
OK. Will do.

So I think we can probably introduce reset_map() but not say it's for
on-chip IOMMU. We can probably say, it's for resetting the vendor
specific mapping into initialization state?
For sure. That's exactly the intent, though I didn't specifically tie on-chip to two-dimension or entity mapping. Yes I can reword to "vendor specific" in the next rev to avoid confusions and ambiguity.


Btw, is there a Qemu patch for reference for this new feature?
There's a WIP version, not ready yet for review:
branch: vdpa-svq-asid-poc

Will need to clean up code and split to smaller patches before I can
post it, if the kernel part can be settled.




There are two cases that backend may claim
this feature bit on:

- parent device that has to work with platform IOMMU
- parent device with on-chip IOMMU that has the expected
    .reset_map support in driver

Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
RFC v2 -> v3:
    - fix missing return due to merge error

   drivers/vhost/vdpa.c             | 16 +++++++++++++++-
   include/uapi/linux/vhost_types.h |  2 ++
   2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 71fbd559..b404504 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -414,6 +414,14 @@ static bool vhost_vdpa_has_desc_group(const struct vhost_vdpa *v)
          return ops->get_vq_desc_group;

+static bool vhost_vdpa_has_persistent_map(const struct vhost_vdpa *v)
+       struct vdpa_device *vdpa = v->vdpa;
+       const struct vdpa_config_ops *ops = vdpa->config;
+       return (!ops->set_map && !ops->dma_map) || ops->reset_map;
   static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
          struct vdpa_device *vdpa = v->vdpa;
@@ -716,7 +724,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
                  if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
                                   BIT_ULL(VHOST_BACKEND_F_DESC_ASID) |
                                   BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
-                                BIT_ULL(VHOST_BACKEND_F_RESUME)))
+                                BIT_ULL(VHOST_BACKEND_F_RESUME) |
+                                BIT_ULL(VHOST_BACKEND_F_IOTLB_PERSIST)))
                          return -EOPNOTSUPP;
                  if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
@@ -730,6 +739,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
                  if ((features & BIT_ULL(VHOST_BACKEND_F_DESC_ASID)) &&
                          return -EOPNOTSUPP;
+               if ((features & BIT_ULL(VHOST_BACKEND_F_IOTLB_PERSIST)) &&
+                    !vhost_vdpa_has_persistent_map(v))
+                       return -EOPNOTSUPP;
                  vhost_set_backend_features(&v->vdev, features);
                  return 0;
@@ -785,6 +797,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
                          features |= BIT_ULL(VHOST_BACKEND_F_RESUME);
                  if (vhost_vdpa_has_desc_group(v))
                          features |= BIT_ULL(VHOST_BACKEND_F_DESC_ASID);
+               if (vhost_vdpa_has_persistent_map(v))
+                       features |= BIT_ULL(VHOST_BACKEND_F_IOTLB_PERSIST);
                  if (copy_to_user(featurep, &features, sizeof(features)))
                          r = -EFAULT;
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index 6acc604..0fdb6f0 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -186,5 +186,7 @@ struct vhost_vdpa_iova_range {
    * buffers may reside. Requires VHOST_BACKEND_F_IOTLB_ASID.
   #define VHOST_BACKEND_F_DESC_ASID    0x6
+/* IOTLB don't flush memory mapping across device reset */


Virtualization mailing list

[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux