[PATCH 0/6] IOMMU, vtd and iotlb flush rework

[PATCH 0/6] IOMMU, vtd and iotlb flush rework

[Jean Guyader]

In one of my previous email I detailed a bug I was seeing when passing
through a Intel GPU on a guest that has more that 4G or RAM.

Allen suggested that I go for the Plan B but after a discussion with Tim
we agreed that Plan B was way to disruptive in term of code change.

This patch series implements Plan A.

http://xen.1045712.n5.nabble.com/VTD-Intel-iommu-IOTLB-flush-really-slow-td4952866.html

Jean Guyader (6):
      vtd: Refactor iotlb flush code
      iommu: Introduce iommu_flush and iommu_flush_all.
      add_to_physmap: Move the code for XENMEM_add_to_physmap.
      mm: Add new map space for add_to_physmap, XENMAPSPACE_gmfn_range.
      hvmloader: Change memory relocation loop when overlap with PCI hole.
      Introduce domain flag (dont_flush_iotlb) to avoid unnecessary iotlb flush.

 tools/firmware/hvmloader/pci.c      |   20 +++-
 xen/arch/x86/mm.c                   |  197 ++++++++++++++++++++---------------
 xen/drivers/passthrough/iommu.c     |   26 +++++
 xen/drivers/passthrough/vtd/iommu.c |  100 ++++++++++--------
 xen/include/public/memory.h         |    4 +
 xen/include/xen/iommu.h             |    5 +
 xen/include/xen/sched.h             |    1 +
 7 files changed, 222 insertions(+), 131 deletions(-)

[PATCH 1/6] vtd: Refactor iotlb flush code

[Jean Guyader]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]


Factorize the iotlb flush code from map_page and unmap_page into
it's own function.

Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 xen/drivers/passthrough/vtd/iommu.c |   86 +++++++++++++++++-----------------
 1 files changed, 43 insertions(+), 43 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-vtd-Refactor-iotlb-flush-code.patch --]
[-- Type: text/x-patch; name="0001-vtd-Refactor-iotlb-flush-code.patch", Size: 4122 bytes --]

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 7717ab4..5a5b6be 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -578,16 +578,53 @@ static void iommu_flush_all(void)
     }
 }
 
+static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
+        int dma_old_pte_present, unsigned int page_count)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+    struct acpi_drhd_unit *drhd;
+    struct iommu *iommu;
+    int flush_dev_iotlb;
+    int iommu_domid;
+
+    /*
+     * No need pcideves_lock here because we have flush
+     * when assign/deassign device
+     */
+    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 ( page_count > 1 || gfn == -1 )
+        {
+            if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
+                        0, flush_dev_iotlb) )
+                iommu_flush_write_buffer(iommu);
+        }
+        else
+        {
+            if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
+                        (paddr_t)gfn << PAGE_SHIFT_4K, 0,
+                        !dma_old_pte_present, flush_dev_iotlb) )
+                iommu_flush_write_buffer(iommu);
+        }
+    }
+}
+
 /* clear one page's page table */
 static void dma_pte_clear_one(struct domain *domain, u64 addr)
 {
     struct hvm_iommu *hd = domain_hvm_iommu(domain);
-    struct acpi_drhd_unit *drhd;
-    struct iommu *iommu;
     struct dma_pte *page = NULL, *pte = NULL;
     u64 pg_maddr;
-    int flush_dev_iotlb;
-    int iommu_domid;
     struct mapped_rmrr *mrmrr;
 
     spin_lock(&hd->mapping_lock);
@@ -613,21 +650,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
     spin_unlock(&hd->mapping_lock);
     iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
 
-    /* No need pcidevs_lock here since do that on assign/deassign device*/
-    for_each_drhd_unit ( drhd )
-    {
-        iommu = drhd->iommu;
-        if ( test_bit(iommu->index, &hd->iommu_bitmap) )
-        {
-            flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
-            iommu_domid= domain_iommu_domid(domain, iommu);
-            if ( iommu_domid == -1 )
-                continue;
-            if ( iommu_flush_iotlb_psi(iommu, iommu_domid, addr,
-                                       0, 0, flush_dev_iotlb) )
-                iommu_flush_write_buffer(iommu);
-        }
-    }
+    __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K , 0, 1);
 
     unmap_vtd_domain_page(page);
 
@@ -1677,12 +1700,8 @@ static int intel_iommu_map_page(
     unsigned int flags)
 {
     struct hvm_iommu *hd = domain_hvm_iommu(d);
-    struct acpi_drhd_unit *drhd;
-    struct iommu *iommu;
     struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 };
     u64 pg_maddr;
-    int flush_dev_iotlb;
-    int iommu_domid;
 
     /* Do nothing if VT-d shares EPT page table */
     if ( iommu_use_hap_pt(d) )
@@ -1724,26 +1743,7 @@ static int intel_iommu_map_page(
     spin_unlock(&hd->mapping_lock);
     unmap_vtd_domain_page(page);
 
-    /*
-     * No need pcideves_lock here because we have flush
-     * when assign/deassign device
-     */
-    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, 0,
-                                   !dma_pte_present(old), flush_dev_iotlb) )
-            iommu_flush_write_buffer(iommu);
-    }
+    __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1);
 
     return 0;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.

[Jean Guyader]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]


Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 xen/drivers/passthrough/iommu.c     |   20 ++++++++++++++++++++
 xen/drivers/passthrough/vtd/iommu.c |   12 ++++++++++++
 xen/include/xen/iommu.h             |    5 +++++
 3 files changed, 37 insertions(+), 0 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-iommu-Introduce-iommu_flush-and-iommu_flush_all.patch --]
[-- Type: text/x-patch; name="0002-iommu-Introduce-iommu_flush-and-iommu_flush_all.patch", Size: 2742 bytes --]

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index cd6174d..9c62861 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -301,6 +301,26 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn)
     return hd->platform_ops->unmap_page(d, gfn);
 }
 
+void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+
+    if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
+        return;
+
+    hd->platform_ops->iotlb_flush(d, gfn, page_count);
+}
+
+void iommu_iotlb_flush_all(struct domain *d)
+{
+    struct hvm_iommu *hd = domain_hvm_iommu(d);
+
+    if ( !iommu_enabled || !hd->platform_ops )
+        return;
+
+    hd->platform_ops->iotlb_flush_all(d);
+}
+
 /* caller should hold the pcidevs_lock */
 int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
 {
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5a5b6be..7ec9541 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -619,6 +619,16 @@ static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
     }
 }
 
+static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
+{
+    __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
+}
+
+static void intel_iommu_iotlb_flush_all(struct domain *d)
+{
+    __intel_iommu_iotlb_flush(d, 0, 0, 0);
+}
+
 /* clear one page's page table */
 static void dma_pte_clear_one(struct domain *domain, u64 addr)
 {
@@ -2329,6 +2339,8 @@ const struct iommu_ops intel_iommu_ops = {
     .resume = vtd_resume,
     .share_p2m = iommu_set_pgd,
     .crash_shutdown = vtd_crash_shutdown,
+    .iotlb_flush = intel_iommu_iotlb_flush,
+    .iotlb_flush_all = intel_iommu_iotlb_flush_all,
 };
 
 /*
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 837e60d..a1034df 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -139,6 +139,8 @@ struct iommu_ops {
     void (*resume)(void);
     void (*share_p2m)(struct domain *d);
     void (*crash_shutdown)(void);
+    void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
+    void (*iotlb_flush_all)(struct domain *d);
 };
 
 void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
@@ -155,4 +157,7 @@ void iommu_share_p2m_table(struct domain *d);
 
 int iommu_do_domctl(struct xen_domctl *, XEN_GUEST_HANDLE(xen_domctl_t));
 
+void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count);
+void iommu_iotlb_flush_all(struct domain *d);
+
 #endif /* _IOMMU_H_ */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 3/6] add_to_physmap: Move the code for XENMEM_add_to_physmap.

[Jean Guyader]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]


Move the code for the XENMEM_add_to_physmap case into it's own
function (xenmem_add_to_physmap).

Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 xen/arch/x86/mm.c |  188 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 99 insertions(+), 89 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-add_to_physmap-Move-the-code-for-XENMEM_add_to_physm.patch --]
[-- Type: text/x-patch; name="0003-add_to_physmap-Move-the-code-for-XENMEM_add_to_physm.patch", Size: 7065 bytes --]

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index acc1f34..f75011e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4592,119 +4592,129 @@ static int handle_iomem_range(unsigned long s, unsigned long e, void *p)
     return 0;
 }
 
-long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
+static int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap xatp)
 {
     struct page_info *page = NULL;
+    unsigned long prev_mfn, mfn = 0, gpfn;
     int rc;
 
-    switch ( op )
-    {
-    case XENMEM_add_to_physmap:
+    switch ( xatp.space )
     {
-        struct xen_add_to_physmap xatp;
-        unsigned long prev_mfn, mfn = 0, gpfn;
-        struct domain *d;
-
-        if ( copy_from_guest(&xatp, arg, 1) )
-            return -EFAULT;
+    case XENMAPSPACE_shared_info:
+        if ( xatp.idx == 0 )
+            mfn = virt_to_mfn(d->shared_info);
+        break;
+    case XENMAPSPACE_grant_table:
+        spin_lock(&d->grant_table->lock);
 
-        rc = rcu_lock_target_domain_by_id(xatp.domid, &d);
-        if ( rc != 0 )
-            return rc;
+        if ( d->grant_table->gt_version == 0 )
+            d->grant_table->gt_version = 1;
 
-        if ( xsm_add_to_physmap(current->domain, d) )
+        if ( d->grant_table->gt_version == 2 &&
+             (xatp.idx & XENMAPIDX_grant_table_status) )
         {
-            rcu_unlock_domain(d);
-            return -EPERM;
+            xatp.idx &= ~XENMAPIDX_grant_table_status;
+            if ( xatp.idx < nr_status_frames(d->grant_table) )
+                mfn = virt_to_mfn(d->grant_table->status[xatp.idx]);
+        }
+        else
+        {
+            if ( (xatp.idx >= nr_grant_frames(d->grant_table)) &&
+                 (xatp.idx < max_nr_grant_frames) )
+                gnttab_grow_table(d, xatp.idx + 1);
+
+            if ( xatp.idx < nr_grant_frames(d->grant_table) )
+                mfn = virt_to_mfn(d->grant_table->shared_raw[xatp.idx]);
         }
 
-        switch ( xatp.space )
+        spin_unlock(&d->grant_table->lock);
+        break;
+    case XENMAPSPACE_gmfn:
+    {
+        p2m_type_t p2mt;
+
+        xatp.idx = mfn_x(gfn_to_mfn_unshare(d, xatp.idx, &p2mt));
+        /* If the page is still shared, exit early */
+        if ( p2m_is_shared(p2mt) )
         {
-        case XENMAPSPACE_shared_info:
-            if ( xatp.idx == 0 )
-                mfn = virt_to_mfn(d->shared_info);
+            rcu_unlock_domain(d);
+            return -ENOMEM;
+        }
+        if ( !get_page_from_pagenr(xatp.idx, d) )
             break;
-        case XENMAPSPACE_grant_table:
-            spin_lock(&d->grant_table->lock);
+        mfn = xatp.idx;
+        page = mfn_to_page(mfn);
+        break;
+    }
+    default:
+        break;
+    }
 
-            if ( d->grant_table->gt_version == 0 )
-                d->grant_table->gt_version = 1;
+    if ( !paging_mode_translate(d) || (mfn == 0) )
+    {
+        if ( page )
+            put_page(page);
+        rcu_unlock_domain(d);
+        return -EINVAL;
+    }
 
-            if ( d->grant_table->gt_version == 2 &&
-                 (xatp.idx & XENMAPIDX_grant_table_status) )
-            {
-                xatp.idx &= ~XENMAPIDX_grant_table_status;
-                if ( xatp.idx < nr_status_frames(d->grant_table) )
-                    mfn = virt_to_mfn(d->grant_table->status[xatp.idx]);
-            }
-            else
-            {
-                if ( (xatp.idx >= nr_grant_frames(d->grant_table)) &&
-                     (xatp.idx < max_nr_grant_frames) )
-                    gnttab_grow_table(d, xatp.idx + 1);
+    domain_lock(d);
 
-                if ( xatp.idx < nr_grant_frames(d->grant_table) )
-                    mfn = virt_to_mfn(d->grant_table->shared_raw[xatp.idx]);
-            }
+    if ( page )
+        put_page(page);
 
-            spin_unlock(&d->grant_table->lock);
-            break;
-        case XENMAPSPACE_gmfn:
-        {
-            p2m_type_t p2mt;
+    /* Remove previously mapped page if it was present. */
+    prev_mfn = gmfn_to_mfn(d, xatp.gpfn);
+    if ( mfn_valid(prev_mfn) )
+    {
+        if ( is_xen_heap_mfn(prev_mfn) )
+            /* Xen heap frames are simply unhooked from this phys slot. */
+            guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
+        else
+            /* Normal domain memory is freed, to avoid leaking memory. */
+            guest_remove_page(d, xatp.gpfn);
+    }
 
-            xatp.idx = mfn_x(gfn_to_mfn_unshare(d, xatp.idx, &p2mt));
-            /* If the page is still shared, exit early */
-            if ( p2m_is_shared(p2mt) )
-            {
-                rcu_unlock_domain(d);
-                return -ENOMEM;
-            }
-            if ( !get_page_from_pagenr(xatp.idx, d) )
-                break;
-            mfn = xatp.idx;
-            page = mfn_to_page(mfn);
-            break;
-        }
-        default:
-            break;
-        }
+    /* Unmap from old location, if any. */
+    gpfn = get_gpfn_from_mfn(mfn);
+    ASSERT( gpfn != SHARED_M2P_ENTRY );
+    if ( gpfn != INVALID_M2P_ENTRY )
+        guest_physmap_remove_page(d, gpfn, mfn, 0);
 
-        if ( !paging_mode_translate(d) || (mfn == 0) )
-        {
-            if ( page )
-                put_page(page);
-            rcu_unlock_domain(d);
-            return -EINVAL;
-        }
+    /* Map at new location. */
+    rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
 
-        domain_lock(d);
+    domain_unlock(d);
 
-        if ( page )
-            put_page(page);
+    return rc;
+}
 
-        /* Remove previously mapped page if it was present. */
-        prev_mfn = gmfn_to_mfn(d, xatp.gpfn);
-        if ( mfn_valid(prev_mfn) )
-        {
-            if ( is_xen_heap_mfn(prev_mfn) )
-                /* Xen heap frames are simply unhooked from this phys slot. */
-                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
-            else
-                /* Normal domain memory is freed, to avoid leaking memory. */
-                guest_remove_page(d, xatp.gpfn);
-        }
 
-        /* Unmap from old location, if any. */
-        gpfn = get_gpfn_from_mfn(mfn);
-        ASSERT( gpfn != SHARED_M2P_ENTRY );
-        if ( gpfn != INVALID_M2P_ENTRY )
-            guest_physmap_remove_page(d, gpfn, mfn, 0);
+long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
+{
+    int rc;
+
+    switch ( op )
+    {
+    case XENMEM_add_to_physmap:
+    {
+        struct xen_add_to_physmap xatp;
+        struct domain *d;
 
-        /* Map at new location. */
-        rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
+        if ( copy_from_guest(&xatp, arg, 1) )
+            return -EFAULT;
+
+        rc = rcu_lock_target_domain_by_id(xatp.domid, &d);
+        if ( rc != 0 )
+            return rc;
+
+        if ( xsm_add_to_physmap(current->domain, d) )
+        {
+            rcu_unlock_domain(d);
+            return -EPERM;
+        }
 
-        domain_unlock(d);
+        xenmem_add_to_physmap(d, xatp);
 
         rcu_unlock_domain(d);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 4/6] mm: Add new map space for add_to_physmap, XENMAPSPACE_gmfn_range.

[Jean Guyader]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]


XENMAPSPACE_gmfn_range is like XENMAPSPACE_gmfn but with a size which
is the number of pages on which xen will iterate.

Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 xen/arch/x86/mm.c           |   15 ++++++++++++++-
 xen/include/public/memory.h |    4 ++++
 2 files changed, 18 insertions(+), 1 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0004-mm-Add-new-map-space-for-add_to_physmap-XENMAPSPACE_.patch --]
[-- Type: text/x-patch; name="0004-mm-Add-new-map-space-for-add_to_physmap-XENMAPSPACE_.patch", Size: 1956 bytes --]

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f75011e..77ae77f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4629,6 +4629,7 @@ static int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap xat
 
         spin_unlock(&d->grant_table->lock);
         break;
+    case XENMAPSPACE_gmfn_range:
     case XENMAPSPACE_gmfn:
     {
         p2m_type_t p2mt;
@@ -4700,6 +4701,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
     {
         struct xen_add_to_physmap xatp;
         struct domain *d;
+        unsigned int size, i;
 
         if ( copy_from_guest(&xatp, arg, 1) )
             return -EFAULT;
@@ -4714,7 +4716,18 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
             return -EPERM;
         }
 
-        xenmem_add_to_physmap(d, xatp);
+        size = 1;
+        if ( xatp.space == XENMAPSPACE_gmfn_range )
+        {
+            size = xatp.size;
+        }
+
+        for ( i = 0; i < size; i++ )
+        {
+            xenmem_add_to_physmap(d, xatp);
+            xatp.idx++;
+            xatp.gpfn++;
+        }
 
         rcu_unlock_domain(d);
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 08355e3..860b8c0 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -212,6 +212,7 @@ struct xen_add_to_physmap {
 #define XENMAPSPACE_shared_info 0 /* shared info page */
 #define XENMAPSPACE_grant_table 1 /* grant table page */
 #define XENMAPSPACE_gmfn        2 /* GMFN */
+#define XENMAPSPACE_gmfn_range  3 /* GMFN range */
     unsigned int space;
 
 #define XENMAPIDX_grant_table_status 0x80000000
@@ -221,6 +222,9 @@ struct xen_add_to_physmap {
 
     /* GPFN where the source mapping page should appear. */
     xen_pfn_t     gpfn;
+
+    /* Number of page, used for GMFN range */
+    unsigned int size;
 };
 typedef struct xen_add_to_physmap xen_add_to_physmap_t;
 DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_t);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 5/6] hvmloader: Change memory relocation loop when overlap with PCI hole.

[Jean Guyader]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]


Change the way we relocate the memory page if they overlap with pci hole.
Use new map space (XENMAPSPACE_gmfn_range) to move the loop into xen.

This code usually get triggered when a device is pass through to a guest
and the PCI hole has to be extended to have enough room to map the device BARs.
The PCI hole will starts lower and it might overlap with some RAM that has been
alocated for the guest. That usually happen if the guest has more than 4G of RAM.
We have to relocate those pages in high mem otherwise they won't be accessible.

Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 tools/firmware/hvmloader/pci.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0005-hvmloader-Change-memory-relocation-loop-when-overlap.patch --]
[-- Type: text/x-patch; name="0005-hvmloader-Change-memory-relocation-loop-when-overlap.patch", Size: 1576 bytes --]

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 29ec011..fb451ef 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -185,17 +185,25 @@ void pci_setup(void)
             ((pci_mem_start << 1) != 0) )
         pci_mem_start <<= 1;
 
-    while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
+    if ((pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend)
     {
         struct xen_add_to_physmap xatp;
-        if ( hvm_info->high_mem_pgend == 0 )
-            hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
+        unsigned int size = hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT);
         xatp.domid = DOMID_SELF;
-        xatp.space = XENMAPSPACE_gmfn;
-        xatp.idx   = --hvm_info->low_mem_pgend;
-        xatp.gpfn  = hvm_info->high_mem_pgend++;
+        xatp.space = XENMAPSPACE_gmfn_range;
+        xatp.size = size;
+        xatp.idx = (pci_mem_start >> PAGE_SHIFT);
+        xatp.gpfn = hvm_info->high_mem_pgend;
+
+        printf("Relocate page for pci hole:\n");
         if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
             BUG();
+        printf("  - low_mem_pgend: %x -> %lx\n"
+                "  - high_mem_pgend: %x -> %x\n",
+                hvm_info->low_mem_pgend, (pci_mem_start >> PAGE_SHIFT),
+                hvm_info->high_mem_pgend, hvm_info->high_mem_pgend + size);
+        hvm_info->low_mem_pgend = pci_mem_start >> PAGE_SHIFT;
+        hvm_info->high_mem_pgend += size;
     }
 
     mem_resource.base = pci_mem_start;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 6/6] Introduce domain flag (dont_flush_iotlb) to avoid unnecessary iotlb flush.

[Jean Guyader]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]


Add a domain level flag that will be check by the iommu low level code
to skip iotlb flush. iommu_iotlb_flush has to be called explicitly.

Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 xen/arch/x86/mm.c                   |   12 +++++++++++-
 xen/drivers/passthrough/iommu.c     |    6 ++++++
 xen/drivers/passthrough/vtd/iommu.c |    6 ++++--
 xen/include/xen/sched.h             |    1 +
 4 files changed, 22 insertions(+), 3 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0006-Introduce-domain-flag-dont_flush_iotlb-to-avoid-unne.patch --]
[-- Type: text/x-patch; name="0006-Introduce-domain-flag-dont_flush_iotlb-to-avoid-unne.patch", Size: 3793 bytes --]

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 77ae77f..4e23875 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4699,7 +4699,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
     {
     case XENMEM_add_to_physmap:
     {
-        struct xen_add_to_physmap xatp;
+        struct xen_add_to_physmap xatp, start_xatp;
         struct domain *d;
         unsigned int size, i;
 
@@ -4720,8 +4720,11 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
         if ( xatp.space == XENMAPSPACE_gmfn_range )
         {
             size = xatp.size;
+            if ( need_iommu(d) )
+                d->dont_flush_iotlb = 1;
         }
 
+        start_xatp = xatp;
         for ( i = 0; i < size; i++ )
         {
             xenmem_add_to_physmap(d, xatp);
@@ -4729,6 +4732,13 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
             xatp.gpfn++;
         }
 
+        if ( xatp.space == XENMAPSPACE_gmfn_range && need_iommu(d) )
+        {
+            d->dont_flush_iotlb = 0;
+            iommu_iotlb_flush(d, start_xatp.idx, size);
+            iommu_iotlb_flush(d, start_xatp.gpfn, size);
+        }
+
         rcu_unlock_domain(d);
 
         return rc;
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9c62861..7f87aed 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -113,10 +113,13 @@ void __init iommu_dom0_init(struct domain *d)
         return;
 
     d->need_iommu = !!iommu_dom0_strict;
+    d->dont_flush_iotlb = 0;
     if ( need_iommu(d) )
     {
         struct page_info *page;
         unsigned int i = 0;
+
+        d->dont_flush_iotlb = 1;
         page_list_for_each ( page, &d->page_list )
         {
             unsigned long mfn = page_to_mfn(page);
@@ -129,6 +132,8 @@ void __init iommu_dom0_init(struct domain *d)
             if ( !(i++ & 0xfffff) )
                 process_pending_softirqs();
         }
+        d->dont_flush_iotlb = 0;
+        iommu_iotlb_flush_all(d);
     }
 
     return hd->platform_ops->dom0_init(d);
@@ -210,6 +215,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
     if ( has_arch_pdevs(d) && !need_iommu(d) )
     {
         d->need_iommu = 1;
+        d->dont_flush_iotlb = 0;
         if ( !iommu_use_hap_pt(d) )
             rc = iommu_populate_page_table(d);
         goto done;
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 7ec9541..336232e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -660,7 +660,8 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
     spin_unlock(&hd->mapping_lock);
     iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
 
-    __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K , 0, 1);
+    if ( !domain->dont_flush_iotlb )
+        __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K , 0, 1);
 
     unmap_vtd_domain_page(page);
 
@@ -1753,7 +1754,8 @@ static int intel_iommu_map_page(
     spin_unlock(&hd->mapping_lock);
     unmap_vtd_domain_page(page);
 
-    __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1);
+    if ( !d->dont_flush_iotlb )
+        __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1);
 
     return 0;
 }
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 3ba5495..922f068 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -244,6 +244,7 @@ struct domain
     bool_t           is_hvm;
     /* Does this guest need iommu mappings? */
     bool_t           need_iommu;
+    bool_t           dont_flush_iotlb;
     /* Is this guest fully privileged (aka dom0)? */
     bool_t           is_privileged;
     /* Which guest this guest has privileges on */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 6/6] Introduce domain flag (dont_flush_iotlb) to avoid unnecessary iotlb flush.

[Jan Beulich]

>>> On 04.11.11 at 11:38, Jean Guyader <jean.guyader@eu.citrix.com> wrote:

> Add a domain level flag that will be check by the iommu low level code
> to skip iotlb flush. iommu_iotlb_flush has to be called explicitly.
> 
> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
> ---
>  xen/arch/x86/mm.c                   |   12 +++++++++++-
>  xen/drivers/passthrough/iommu.c     |    6 ++++++
>  xen/drivers/passthrough/vtd/iommu.c |    6 ++++--
>  xen/include/xen/sched.h             |    1 +
>  4 files changed, 22 insertions(+), 3 deletions(-)

Neither the patch nor its description make clear what lock is used
to protect this flag from having an effect on other but the current
vCPU of the guest in question - I doubt that all other relevant code
paths indeed use domain_lock().

Jan

Re: [PATCH 5/6] hvmloader: Change memory relocation loop when overlap with PCI hole.

[Keir Fraser]

On 04/11/2011 10:38, "Jean Guyader" <jean.guyader@eu.citrix.com> wrote:

> 
> Change the way we relocate the memory page if they overlap with pci hole.
> Use new map space (XENMAPSPACE_gmfn_range) to move the loop into xen.
> 
> This code usually get triggered when a device is pass through to a guest
> and the PCI hole has to be extended to have enough room to map the device
> BARs.
> The PCI hole will starts lower and it might overlap with some RAM that has
> been
> alocated for the guest. That usually happen if the guest has more than 4G of
> RAM.
> We have to relocate those pages in high mem otherwise they won't be
> accessible.

If the size field in the add_to_physmap structure is reduced to 16 bits, the
new if() stmt will probably have to become a while() again.

The new printf's are just excess verbiage. We dump the memory map as we exit
hvmloader anyway.

 -- Keir

> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
> ---
>  tools/firmware/hvmloader/pci.c |   20 ++++++++++++++------
>  1 files changed, 14 insertions(+), 6 deletions(-)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 4/6] mm: Add new map space for add_to_physmap, XENMAPSPACE_gmfn_range.

[Jan Beulich]

>>> On 04.11.11 at 11:38, Jean Guyader <jean.guyader@eu.citrix.com> wrote:
> XENMAPSPACE_gmfn_range is like XENMAPSPACE_gmfn but with a size which
> is the number of pages on which xen will iterate.
> 
> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
> ---
>  xen/arch/x86/mm.c           |   15 ++++++++++++++-
>  xen/include/public/memory.h |    4 ++++
>  2 files changed, 18 insertions(+), 1 deletions(-)

You can't add a member to a structure that's part of the public
interface. You'll need to create a new structure.

Jan

Re: [PATCH 4/6] mm: Add new map space for add_to_physmap, XENMAPSPACE_gmfn_range.

[Keir Fraser]

On 04/11/2011 10:38, "Jean Guyader" <jean.guyader@eu.citrix.com> wrote:

> 
> XENMAPSPACE_gmfn_range is like XENMAPSPACE_gmfn but with a size which
> is the number of pages on which xen will iterate.

You can't really extend the size of an existing ABI structure, and always
copy that extended size. Older guests won't know to guarantee that the
extended space is accessible. I suggest you make your new field a uint16_t,
placed directly after the domid field. Then you are making use of existing
pad space. 64k pages = 256MB at a time should be plenty of amortisation.

And, even with the reduced field width, I could imagine 64k remappings
taking a good while. The remapping loop should regularly (even every
iteration) check hypercall_preempt_check(), then
hypercall_create_continuation() and exit if the hypercall is being requested
to voluntarily yield.

 -- Keir

> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
> ---
>  xen/arch/x86/mm.c           |   15 ++++++++++++++-
>  xen/include/public/memory.h |    4 ++++
>  2 files changed, 18 insertions(+), 1 deletions(-)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel