On 9/10/2023 8:52 PM, Jason Wang wrote:

On Sat, Sep 9, 2023 at 9:46 PM Si-Wei Liu [off-list ref] wrote:

quoted

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 
PERSIST_IOTLB? If unconditional, it breaks older host kernel, as the 
older kernel still silently drops all mapping across vDPA reset (VM 
reboot), 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.

Btw, is there a Qemu patch for reference for this new feature?

There's a WIP version, not ready yet for review:

https://github.com/siwliu-kernel/qemu
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.

Thanks,
-Siwei

Thanks

quoted

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 <redacted>
---
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)) &&
                      !vhost_vdpa_can_suspend(v))

@@ -730,6 +739,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
                 if ((features & BIT_ULL(VHOST_BACKEND_F_DESC_ASID)) &&
                      !vhost_vdpa_has_desc_group(v))
                         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;
                 break;

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 */
+#define VHOST_BACKEND_F_IOTLB_PERSIST  0x7

  #endif
--

1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization