From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xu Zaibo Subject: Re: [PATCH v2 13/40] vfio: Add support for Shared Virtual Addressing Date: Thu, 24 May 2018 20:35:08 +0800 Message-ID: <5B06B17C.1090809@huawei.com> References: <20180511190641.23008-1-jean-philippe.brucker@arm.com> <20180511190641.23008-14-jean-philippe.brucker@arm.com> <5B0536A3.1000304@huawei.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4375755699768683648==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Jean-Philippe Brucker , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org Cc: bharatku-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, ashok.raj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, ilias.apalodimas-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, okaya-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, rgummal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, rfranz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org, liguozhu , christian.koenig-5C7GfCeVMHo@public.gmane.org List-Id: iommu@lists.linux-foundation.org --===============4375755699768683648== Content-Type: multipart/alternative; boundary="------------070003080700080202060305" --------------070003080700080202060305 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit 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, >>> + struct vfio_group *group, >>> + struct vfio_mm *vfio_mm) >>> +{ >>> + int ret; >>> + bool enabled_sva = false; >>> + struct vfio_iommu_sva_bind_data data = { >>> + .vfio_mm = vfio_mm, >>> + .iommu = iommu, >>> + .count = 0, >>> + }; >>> + >>> + if (!group->sva_enabled) { >>> + ret = iommu_group_for_each_dev(group->iommu_group, NULL, >>> + 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,/ if (features & ~IOMMU_SVA_FEAT_IOPF) return -EINVAL; /+ /* If already exists, do nothing */// //+ mutex_lock(&dev->iommu_param->lock);// //+ if (dev->iommu_param->sva_param) {// //+ mutex_unlock(&dev->iommu_param->lock);// //+ return 0;// //+ }// //+ mutex_unlock(&dev->iommu_param->lock);// //// // if (features & IOMMU_SVA_FEAT_IOPF) {// // ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,///// / // // //@@ -621,6 +646,14 @@ int iommu_sva_device_shutdown(struct device *dev)// // if (!domain)// // return -ENODEV;// //// //+ /* If any other process is working on the device, shut down does nothing. */// //+ mutex_lock(&dev->iommu_param->lock);// //+ if (!list_empty(&dev->iommu_param->sva_param->mm_list)) {// //+ mutex_unlock(&dev->iommu_param->lock);// //+ return 0;// //+ }// //+ mutex_unlock(&dev->iommu_param->lock);// //+// // __iommu_sva_unbind_dev_all(dev);// //// // 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 > > . > --------------070003080700080202060305 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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,
+                 struct vfio_group *group,
+                 struct vfio_mm *vfio_mm)
+{
+    int ret;
+    bool enabled_sva = false;
+    struct vfio_iommu_sva_bind_data data = {
+        .vfio_mm    = vfio_mm,
+        .iommu        = iommu,
+        .count        = 0,
+    };
+
+    if (!group->sva_enabled) {
+        ret = iommu_group_for_each_dev(group->iommu_group, NULL,
+                           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,
      if (features & ~IOMMU_SVA_FEAT_IOPF)
         return -EINVAL;

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


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

.

--------------070003080700080202060305-- --===============4375755699768683648== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============4375755699768683648==--