On 2018/5/24 19:44, Jean-Philippe Brucker wrote:

Hi,

On 23/05/18 10:38, Xu Zaibo wrote:

+static int vfio_iommu_bind_group(struct vfio_iommu *iommu,
+A A A A A A A A A A A A A A A A  struct vfio_group *group,
+A A A A A A A A A A A A A A A A  struct vfio_mm *vfio_mm)
+{
+A A A  int ret;
+A A A  bool enabled_sva = false;
+A A A  struct vfio_iommu_sva_bind_data data = {
+A A A A A A A  .vfio_mmA A A  = vfio_mm,
+A A A A A A A  .iommuA A A A A A A  = iommu,
+A A A A A A A  .countA A A A A A A  = 0,
+A A A  };
+
+A A A  if (!group->sva_enabled) {
+A A A A A A A  ret = iommu_group_for_each_dev(group->iommu_group, NULL,
+A A A A A A A A A A A A A A A A A A A A A A A A A A  vfio_iommu_sva_init);

Do we need to do *sva_init here or do anything to avoid repeated
initiation?
while another process already did initiation at this device, I think
that current process will get an EEXIST.

Right, sva_init() must be called once for any device that intends to use
bind(). For the second process though, group->sva_enabled will be true
so we won't call sva_init() again, only bind().

Well, while I create mediated devices based on one parent device to support multiple
processes(a new process will create a new 'vfio_group' for the corresponding mediated device,
and 'sva_enabled' cannot work any more), in fact, *sva_init and *sva_shutdown are basically
working on parent device, so, as a result, I just only need sva initiation and shutdown on the
parent device only once. So I change the two as following:

@@ -551,8 +565,18 @@ int iommu_sva_device_init(struct device *dev, unsigned long features,
A A A A A if (features & ~IOMMU_SVA_FEAT_IOPF)
A A A A A A A return -EINVAL;

+A A A /* If already exists, do nothingA */
+A A A mutex_lock(&dev->iommu_param->lock);
+A A A if (dev->iommu_param->sva_param) {
+A A A A A A mutex_unlock(&dev->iommu_param->lock);
+A A A A A A return 0;
+A A A }
+A A A mutex_unlock(&dev->iommu_param->lock);
A
A A A A if (features & IOMMU_SVA_FEAT_IOPF) {
A A A A A A A ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,

@@ -621,6 +646,14 @@ int iommu_sva_device_shutdown(struct device *dev)
A A A A if (!domain)
A A A A A A A return -ENODEV;
A
+A A A /* If any other process is working on the device, shut down does nothing. */
+A A A mutex_lock(&dev->iommu_param->lock);
+A A A if (!list_empty(&dev->iommu_param->sva_param->mm_list)) {
+A A A A A A mutex_unlock(&dev->iommu_param->lock);
+A A A A A A return 0;
+A A A }
+A A A mutex_unlock(&dev->iommu_param->lock);
+
A A A A __iommu_sva_unbind_dev_all(dev);
A
A A A A mutex_lock(&dev->iommu_param->lock);

I add the above two checkings in both *sva_init and *sva_shutdown, it is working now,
but i don't know if it will cause any new problems. What's more, i doubt if it is
reasonable to check this to avoid repeating operation in VFIO, maybe it is better to check
in IOMMU. :)

Thanks
Zaibo