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

Re: [Xen-devel] [RFC for-4.5 08/12] xen/passthrough: iommu: Split generic IOMMU code



Hello Jan,

Thanks for the review.

On 10/02/14 08:22, Jan Beulich wrote:
On 07.02.14 at 18:43, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
The generic IOMMU framework code (xen/drivers/passthrough/iommu.c) contains
functions specific to x86 and PCI.

Split the framework in 3 distincts files:
     - iommu.c: contains generic functions shared between x86 and ARM
                (when it will be supported)
     - iommu_pci.c: contains specific functions for PCI passthrough
     - iommu_x86.c: contains specific functions for x86

iommu_pci.c will be only compiled when PCI is supported by the architecture
(eg. HAS_PCI is defined).

This patch is mostly code movement in new files.

Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
Cc: Xiantao Zhang <xiantao.zhang@xxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
---
  xen/drivers/passthrough/Makefile    |    6 +-
  xen/drivers/passthrough/iommu.c     |  473 +----------------------------------
  xen/drivers/passthrough/iommu_pci.c |  468 ++++++++++++++++++++++++++++++++++

There's xen/drivers/passthrough/pci.c already - any reason not to
move the code there?

I didn't specific about moving the code directly in passthrough/pci.c. I will do on the next version.


  xen/drivers/passthrough/iommu_x86.c |   65 +++++

Same here for xen/drivers/passthrough/x86/.

@@ -696,125 +344,6 @@ void iommu_crash_shutdown(void)
      iommu_enabled = iommu_intremap = 0;
  }

-int iommu_do_domctl(
-    struct xen_domctl *domctl, struct domain *d,
-    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)

The function itself should probably not be moved out. Either the
PCI-specific pieces of it should be made conditional, or a
descendant function be created. Since (afaict) you'll need all of
the domctl-s (with different arguments) too for pass-through on
ARM - what's your plan for them?

PCI passthrough will be supported on ARM in the future. But we will also have to support passthrough (via the device tree). I think, we will add new DOMCTL for that. Didn't really think about that know.

I will add a descendant function to handle PCI DOMCTL.


--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1784,31 +1784,31 @@ static int intel_iommu_unmap_page(struct domain *d,
unsigned long gfn)

  void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
                       int order, int present)
-{
-    struct acpi_drhd_unit *drhd;
-    struct iommu *iommu = NULL;
-    struct hvm_iommu *hd = domain_hvm_iommu(d);
-    int flush_dev_iotlb;
-    int iommu_domid;
+    {
+        struct acpi_drhd_unit *drhd;
+        struct iommu *iommu = NULL;
+        struct hvm_iommu *hd = domain_hvm_iommu(d);
+        int flush_dev_iotlb;
+        int iommu_domid;

-    iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
+        iommu_flush_cache_entry(pte, sizeof(struct dma_pte));

-    for_each_drhd_unit ( drhd )
-    {
-        iommu = drhd->iommu;
-        if ( !test_bit(iommu->index, &hd->iommu_bitmap) )
-            continue;
+        for_each_drhd_unit ( drhd )
+        {
+            iommu = drhd->iommu;
+            if ( !test_bit(iommu->index, &hd->iommu_bitmap) )
+                continue;

-        flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
-        iommu_domid= domain_iommu_domid(d, iommu);
-        if ( iommu_domid == -1 )
-            continue;
-        if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
-                                   (paddr_t)gfn << PAGE_SHIFT_4K,
-                                   order, !present, flush_dev_iotlb) )
-            iommu_flush_write_buffer(iommu);
+            flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
+            iommu_domid= domain_iommu_domid(d, iommu);
+            if ( iommu_domid == -1 )
+                continue;
+            if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
+                                       (paddr_t)gfn << PAGE_SHIFT_4K,
+                                       order, !present, flush_dev_iotlb) )
+                iommu_flush_write_buffer(iommu);
+        }
      }
-}

What are these changes to indentation about? Are you
deliberately breaking common rules here, or is this some sort of
unintentional leftover?

It's an error when I have created this patch. I will remove it.

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.