[PATCH v4 0/4] Fix grant/foreign mappings with auto-translated guests

[PATCH v4 0/4] Fix grant/foreign mappings with auto-translated guests

[Roger Pau Monne]

This series adds proper IOMMU entries when mapping foreign memory 
(either grants or foreign pages) inside of auto-translated guests 
when the p2m is not shared between HAP and IOMMUs, and disables p2m 
sharing when running on AMD hardware.

In order for the guest to know if it's safe to use grant mapped pages 
for IO with hardware devices a new xen cpuid feature is introduced in 
the last patch.

Thanks for the review, Roger.

[PATCH v4 1/4] xen: make logdirty and iommu mutually exclusive

[Roger Pau Monne]

Prevent the usage of global logdirty if the domain is using the IOMMU,
and also prevent passthrough of devices if logdirty is enabled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
Changes since v2:
 - Check for global logdirty enable in paging_log_dirty_enable instead
   of doing it in shadow/ept code.
 - Use the newly introduced global_logdirty flag to check if the
   domain has global logdirty enabled.
---
 xen/arch/x86/mm/paging.c      |    9 +++++++++
 xen/drivers/passthrough/pci.c |    3 ++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 9e9a11b..32764ba 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -168,6 +168,15 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
 {
     int ret;
 
+    if ( need_iommu(d) && log_global )
+    {
+        /*
+         * Refuse to turn on global log-dirty mode
+         * if the domain is using the IOMMU.
+         */
+        return -EINVAL;
+    }
+
     if ( paging_mode_log_dirty(d) )
         return -EINVAL;
 
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index b7f6e8a..43c1a81 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1233,7 +1233,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
      * enabled for this domain */
     if ( unlikely(!need_iommu(d) &&
             (d->arch.hvm_domain.mem_sharing_enabled ||
-             d->mem_event->paging.ring_page)) )
+             d->mem_event->paging.ring_page ||
+             p2m_get_hostp2m(d)->global_logdirty)) )
         return -EXDEV;
 
     if ( !spin_trylock(&pcidevs_lock) )
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v4 1/4] xen: make logdirty and iommu mutually exclusive

[Tim Deegan]

At 12:06 +0200 on 04 Jun (1401879978), Roger Pau Monne wrote:
> Prevent the usage of global logdirty if the domain is using the IOMMU,
> and also prevent passthrough of devices if logdirty is enabled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

[PATCH v4 2/4] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Roger Pau Monne]

If the memory map is not shared between HAP and IOMMU we fail to set
correct IOMMU mappings for memory types other than p2m_ram_rw.

This patchs adds IOMMU support for the following memory types:
p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro, p2m_grant_map_ro and
p2m_ram_logdirty.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Tested-by: David Zhuang <david.zhuang@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
 - Move the p2m type switch to IOMMU flags to an inline function that
   is shared between p2m-ept and p2m-pt.
 - Make p2m_set_entry also use p2m_get_iommu_flags.
---
 xen/arch/x86/mm/p2m-ept.c |    7 ++++---
 xen/arch/x86/mm/p2m-pt.c  |   11 +++++------
 xen/include/asm-x86/p2m.h |   27 +++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 9fda28d..15c6e83 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -776,10 +776,11 @@ out:
             iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            unsigned int flags = p2m_get_iommu_flags(p2mt);
+
+            if ( flags != 0 )
                 for ( i = 0; i < (1 << order); i++ )
-                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
-                                   IOMMUF_readable | IOMMUF_writable);
+                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i, flags);
             else
                 for ( i = 0; i < (1 << order); i++ )
                     iommu_unmap_page(d, gfn + i);
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index a1794d0..085ab6f 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -491,9 +491,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
     l2_pgentry_t l2e_content;
     l3_pgentry_t l3e_content;
     int rc;
-    unsigned int iommu_pte_flags = (p2mt == p2m_ram_rw) ?
-                                   IOMMUF_readable|IOMMUF_writable:
-                                   0; 
+    unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
     unsigned long old_mfn = 0;
 
     if ( tb_init_done )
@@ -662,10 +660,11 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         }
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            unsigned int flags = p2m_get_iommu_flags(p2mt);
+
+            if ( flags != 0 )
                 for ( i = 0; i < (1UL << page_order); i++ )
-                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i,
-                                   IOMMUF_readable|IOMMUF_writable);
+                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
             else
                 for ( int i = 0; i < (1UL << page_order); i++ )
                     iommu_unmap_page(p2m->domain, gfn+i);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d0cfdac..0ddbadb 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -691,6 +691,33 @@ void p2m_flush_nestedp2m(struct domain *d);
 void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
     l1_pgentry_t *p, l1_pgentry_t new, unsigned int level);
 
+/*
+ * p2m type to IOMMU flags
+ */
+static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
+{
+    unsigned int flags;
+
+    switch( p2mt )
+    {
+    case p2m_ram_rw:
+    case p2m_grant_map_rw:
+    case p2m_ram_logdirty:
+    case p2m_map_foreign:
+        flags =  IOMMUF_readable | IOMMUF_writable;
+        break;
+    case p2m_ram_ro:
+    case p2m_grant_map_ro:
+        flags = IOMMUF_readable;
+        break;
+    default:
+        flags = 0;
+        break;
+    }
+
+    return flags;
+}
+
 #endif /* _XEN_P2M_H */
 
 /*
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Roger Pau Monne]

According to the comment in p2m.h, AMD IOMMUs don't work correctly
with page types different than p2m_ram_rw when the p2m is shared
between HAP and IOMMU, so disable this sharing when using AMD IOMMUs.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Xiantao Zhang <xiantao.zhang@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
I have not tested this patch on AMD hardware, so I would like some
confirmation that this actually works.
---
Changes since v1:
 - Add a debug message when disabling iommu_hap_pt_share.
---
 xen/drivers/passthrough/amd/iommu_init.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index b2f74ef..56bda00 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1255,6 +1255,14 @@ int __init amd_iommu_init(void)
     if ( iterate_ivrs_mappings(amd_iommu_setup_device_table) != 0 )
         goto error_out;
 
+    /*
+     * Disable sharing HAP page tables with AMD IOMMU,
+     * since it only supports p2m_ram_rw, and this would
+     * prevent doing IO to/from mapped grant frames.
+     */
+    iommu_hap_pt_share = 0;
+    printk(XENLOG_DEBUG "AMD-Vi: Disabled HAP memory map sharing with IOMMU\n");
+
     /* per iommu initialization  */
     for_each_amd_iommu ( iommu )
         if ( amd_iommu_init_one(iommu) != 0 )
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Jan Beulich]

>>> On 04.06.14 at 12:06, <roger.pau@citrix.com> wrote:
> According to the comment in p2m.h, AMD IOMMUs don't work correctly
> with page types different than p2m_ram_rw when the p2m is shared
> between HAP and IOMMU, so disable this sharing when using AMD IOMMUs.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Xiantao Zhang <xiantao.zhang@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>

Instead of copying Xiantao and me, you should have included
Aravind as the second AMD IOMMU maintainer here, raising
your chances of getting the necessary ack.

Jan

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

Re: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Jan Beulich]

>>> On 04.06.14 at 12:06, <roger.pau@citrix.com> wrote:
> According to the comment in p2m.h, AMD IOMMUs don't work correctly
> with page types different than p2m_ram_rw when the p2m is shared
> between HAP and IOMMU, so disable this sharing when using AMD IOMMUs.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Xiantao Zhang <xiantao.zhang@intel.com>

Suravee, Aravind,

this is still sitting in my to-be-applied queue, awaiting an AMD
maintainer's ack (or other comment). I realize only Suravee was
copied on the original submission...

Thanks, Jan

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

Re: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Suravee Suthikulpanit]

Roger,

I am not quite sure why you would disable "iommu_hap_pt_share" for AMD 
IOMMU. The current implementation assumes that the p2m can be shared.

Also, I feel that simply just set iommu_hap_pt_share = 0 (while still 
having several places in the AMD iommu drivers and p2m-pt.c assuming 
that it can be shared) seems a bit messy.

Thanks,

Suravee

On 06/04/2014 05:06 AM, Roger Pau Monne wrote:
> This message has been archived. View the original item
> <http://ausev2.amd.com/EnterpriseVault/ViewMessage.asp?VaultId=1EBD12133601C6E47868220A36CFE2B201110000amdvault.amd.com&SavesetId=201407064668470~201406041006380000~Z~B0E2C95E06CD0962996EB51CFC709411>
> According to the comment in p2m.h, AMD IOMMUs don't work correctly
> with page types different than p2m_ram_rw when the p2m is shared
> between HAP and IOMMU, so disable this sharing when using AMD IOMMUs.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Xiantao Zhang <xiantao.zhang@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
> I have not tested this patch on AMD hardware, so I would like some
> confirmation that this actually works.
> ---
> Changes since v1:
>   - Add a debug message when disabling iommu_hap_pt_share.
> ---
>   xen/drivers/passthrough/amd/iommu_init.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
> b/xen/drivers/passthrough/amd/iommu_init.c
> index b2f74ef..56bda00 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1255,6 +1255,14 @@ int __init amd_iommu_init(void)
>       if ( iterate_ivrs_mappings(amd_iommu_setup_device_table) != 0 )
>           goto error_out;
>
> +    /*
> +     * Disable sharing HAP page tables with AMD IOMMU,
> +     * since it only supports p2m_ram_rw, and this would
> +     * prevent doing IO to/from mapped grant frames.
> +     */
> +    iommu_hap_pt_share = 0;
> +    printk(XENLOG_DEBUG "AMD-Vi: Disabled HAP memory map sharing with
> IOMMU");
> +
>       /* per iommu initialization  */
>       for_each_amd_iommu ( iommu )
>           if ( amd_iommu_init_one(iommu) != 0 )
> --
> 1.7.7.5 (Apple Git-26)
>

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

Re: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Roger Pau Monné]

On 22/07/14 19:30, Suravee Suthikulpanit wrote:
> Roger,
> 
> I am not quite sure why you would disable "iommu_hap_pt_share" for AMD
> IOMMU. The current implementation assumes that the p2m can be shared.
> 
> Also, I feel that simply just set iommu_hap_pt_share = 0 (while still
> having several places in the AMD iommu drivers and p2m-pt.c assuming
> that it can be shared) seems a bit messy.

According to the comment in p2m.h, AMD IOMMU only supports bit 52 to bit
58 in the pte to be 0, otherwise the hw generates page faults.

If we want to support doing IO to devices behind an IOMMU from page
types different than p2m_ram_rw the p2m tables cannot be shared, because
the bits from 52 to 58 will indeed be different than 0, and will
generate page faults.

Roger.

Re: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Suravee Suthikulanit]

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

On 7/22/2014 12:39 PM, Roger Pau Monné wrote:
> On 22/07/14 19:30, Suravee Suthikulpanit wrote:
>> Roger,
>>
>> I am not quite sure why you would disable "iommu_hap_pt_share" for AMD
>> IOMMU. The current implementation assumes that the p2m can be shared.
>>
>> Also, I feel that simply just set iommu_hap_pt_share = 0 (while still
>> having several places in the AMD iommu drivers and p2m-pt.c assuming
>> that it can be shared) seems a bit messy.
>
> According to the comment in p2m.h, AMD IOMMU only supports bit 52 to bit
> 58 in the pte to be 0, otherwise the hw generates page faults.
>
> If we want to support doing IO to devices behind an IOMMU from page
> types different than p2m_ram_rw the p2m tables cannot be shared, because
> the bits from 52 to 58 will indeed be different than 0, and will
> generate page faults.
>
> Roger.
>

As you have mentioned, they cannot be shared due to the 52 and 58 bits. 
  However, what I was trying to say is that, besides just simply set the 
flag to 0, we probably should remove existing logic in various places 
that assumes that AMD IOMMU can have share_p2m_table=1.

If you are agree, the attachment is the patch that should do that.

I have tested device-passthrough w/ the amd-iommu: disable 
iommu_hap_pt_share with AMD IOMMUs, and my patch and it is working.

Acked-by Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,

Suravee

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: iommu-removal-of-share_p2m_table-from-AMD-IOMMU.patch --]
[-- Type: text/x-patch; name="iommu-removal-of-share_p2m_table-from-AMD-IOMMU.patch", Size: 5501 bytes --]

>From f28838679867fbbc3be6286556eed7f908eea559 Mon Sep 17 00:00:00 2001
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Date: Fri, 8 Aug 2014 07:26:15 -0500
Subject: [PATCH] iommu: Removal of share_p2m_table from AMD IOMMU

This patch removes existing logics which assumes iommu_hap_pt_share
is enabled for AMD IOMMU.

Signed-off-by  Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cd: Roger Pau Monné <roger.pau@citrix.com>
Cc: Xiantao Zhang <xiantao.zhang@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
NOTES: This patch depends on the "amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs"
patch from Roger Pau Monne <roger.pau@citrix.com>.

 xen/arch/x86/mm/p2m-pt.c                    | 23 ++++++--------------
 xen/drivers/passthrough/amd/iommu_map.c     | 33 -----------------------------
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ----
 xen/drivers/passthrough/iommu.c             |  2 +-
 4 files changed, 8 insertions(+), 54 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 085ab6f..6bec0e9 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -36,7 +36,6 @@
 #include <xen/event.h>
 #include <xen/trace.h>
 #include <asm/hvm/nestedhvm.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
 
 #include "mm-locks.h"
 
@@ -653,22 +652,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
 
     if ( iommu_enabled && need_iommu(p2m->domain) )
     {
-        if ( iommu_hap_pt_share )
-        {
-            if ( old_mfn && (old_mfn != mfn_x(mfn)) )
-                amd_iommu_flush_pages(p2m->domain, gfn, page_order);
-        }
+        unsigned int flags = p2m_get_iommu_flags(p2mt);
+
+        if ( flags != 0 )
+            for ( i = 0; i < (1UL << page_order); i++ )
+                iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
         else
-        {
-            unsigned int flags = p2m_get_iommu_flags(p2mt);
-
-            if ( flags != 0 )
-                for ( i = 0; i < (1UL << page_order); i++ )
-                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
-            else
-                for ( int i = 0; i < (1UL << page_order); i++ )
-                    iommu_unmap_page(p2m->domain, gfn+i);
-        }
+            for ( int i = 0; i < (1UL << page_order); i++ )
+                iommu_unmap_page(p2m->domain, gfn+i);
     }
 
  out:
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index a8c60ec..2808c31 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -640,9 +640,6 @@ int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
 
     BUG_ON( !hd->arch.root_table );
 
-    if ( iommu_use_hap_pt(d) )
-        return 0;
-
     memset(pt_mfn, 0, sizeof(pt_mfn));
 
     spin_lock(&hd->arch.mapping_lock);
@@ -718,9 +715,6 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
 
     BUG_ON( !hd->arch.root_table );
 
-    if ( iommu_use_hap_pt(d) )
-        return 0;
-
     memset(pt_mfn, 0, sizeof(pt_mfn));
 
     spin_lock(&hd->arch.mapping_lock);
@@ -777,30 +771,3 @@ int amd_iommu_reserve_domain_unity_map(struct domain *domain,
     }
     return 0;
 }
-
-/* Share p2m table with iommu. */
-void amd_iommu_share_p2m(struct domain *d)
-{
-    struct hvm_iommu *hd  = domain_hvm_iommu(d);
-    struct page_info *p2m_table;
-    mfn_t pgd_mfn;
-
-    ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
-
-    if ( !iommu_use_hap_pt(d) )
-        return;
-
-    pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
-    p2m_table = mfn_to_page(mfn_x(pgd_mfn));
-
-    if ( hd->arch.root_table != p2m_table )
-    {
-        free_amd_iommu_pgtable(hd->arch.root_table);
-        hd->arch.root_table = p2m_table;
-
-        /* When sharing p2m with iommu, paging mode = 4 */
-        hd->arch.paging_mode = IOMMU_PAGING_MODE_LEVEL_4;
-        AMD_IOMMU_DEBUG("Share p2m table with iommu: p2m table = %#lx\n",
-                        mfn_x(pgd_mfn));
-    }
-}
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 0b301b3..c893dea 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -453,9 +453,6 @@ static void deallocate_iommu_page_tables(struct domain *d)
 {
     struct hvm_iommu *hd  = domain_hvm_iommu(d);
 
-    if ( iommu_use_hap_pt(d) )
-        return;
-
     spin_lock(&hd->arch.mapping_lock);
     if ( hd->arch.root_table )
     {
@@ -619,7 +616,6 @@ const struct iommu_ops amd_iommu_ops = {
     .setup_hpet_msi = amd_setup_hpet_msi,
     .suspend = amd_iommu_suspend,
     .resume = amd_iommu_resume,
-    .share_p2m = amd_iommu_share_p2m,
     .crash_shutdown = amd_iommu_suspend,
     .dump_p2m_table = amd_dump_p2m_table,
 };
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index cc12735..5d3299a 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -332,7 +332,7 @@ void iommu_share_p2m_table(struct domain* d)
 {
     const struct iommu_ops *ops = iommu_get_ops();
 
-    if ( iommu_enabled && is_hvm_domain(d) )
+    if ( iommu_enabled && is_hvm_domain(d) && ops->share_p2m)
         ops->share_p2m(d);
 }
 
-- 
1.9.0


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

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

Re: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Roger Pau Monné]

On 08/08/14 17:36, Suravee Suthikulanit wrote:
> On 7/22/2014 12:39 PM, Roger Pau Monné wrote:
>> On 22/07/14 19:30, Suravee Suthikulpanit wrote:
>>> Roger,
>>>
>>> I am not quite sure why you would disable "iommu_hap_pt_share" for AMD
>>> IOMMU. The current implementation assumes that the p2m can be shared.
>>>
>>> Also, I feel that simply just set iommu_hap_pt_share = 0 (while still
>>> having several places in the AMD iommu drivers and p2m-pt.c assuming
>>> that it can be shared) seems a bit messy.
>>
>> According to the comment in p2m.h, AMD IOMMU only supports bit 52 to bit
>> 58 in the pte to be 0, otherwise the hw generates page faults.
>>
>> If we want to support doing IO to devices behind an IOMMU from page
>> types different than p2m_ram_rw the p2m tables cannot be shared, because
>> the bits from 52 to 58 will indeed be different than 0, and will
>> generate page faults.
>>
>> Roger.
>>
> 
> As you have mentioned, they cannot be shared due to the 52 and 58 bits.
>  However, what I was trying to say is that, besides just simply set the
> flag to 0, we probably should remove existing logic in various places
> that assumes that AMD IOMMU can have share_p2m_table=1.
> 
> If you are agree, the attachment is the patch that should do that.
> 
> I have tested device-passthrough w/ the amd-iommu: disable
> iommu_hap_pt_share with AMD IOMMUs, and my patch and it is working.
> 
> Acked-by Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Thanks,
> 
> Suravee
> 
> iommu-removal-of-share_p2m_table-from-AMD-IOMMU.patch
> 
> 
> From f28838679867fbbc3be6286556eed7f908eea559 Mon Sep 17 00:00:00 2001
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> Date: Fri, 8 Aug 2014 07:26:15 -0500
> Subject: [PATCH] iommu: Removal of share_p2m_table from AMD IOMMU
> 
> This patch removes existing logics which assumes iommu_hap_pt_share
> is enabled for AMD IOMMU.
> 
> Signed-off-by  Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cd: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Xiantao Zhang <xiantao.zhang@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
> NOTES: This patch depends on the "amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs"
> patch from Roger Pau Monne <roger.pau@citrix.com>.
> 
>  xen/arch/x86/mm/p2m-pt.c                    | 23 ++++++--------------
>  xen/drivers/passthrough/amd/iommu_map.c     | 33 -----------------------------
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ----
>  xen/drivers/passthrough/iommu.c             |  2 +-
>  4 files changed, 8 insertions(+), 54 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 085ab6f..6bec0e9 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -36,7 +36,6 @@
>  #include <xen/event.h>
>  #include <xen/trace.h>
>  #include <asm/hvm/nestedhvm.h>
> -#include <asm/hvm/svm/amd-iommu-proto.h>
>  
>  #include "mm-locks.h"
>  
> @@ -653,22 +652,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>  
>      if ( iommu_enabled && need_iommu(p2m->domain) )
>      {
> -        if ( iommu_hap_pt_share )
> -        {
> -            if ( old_mfn && (old_mfn != mfn_x(mfn)) )
> -                amd_iommu_flush_pages(p2m->domain, gfn, page_order);
> -        }
> +        unsigned int flags = p2m_get_iommu_flags(p2mt);
> +
> +        if ( flags != 0 )
> +            for ( i = 0; i < (1UL << page_order); i++ )
> +                iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
>          else
> -        {
> -            unsigned int flags = p2m_get_iommu_flags(p2mt);
> -
> -            if ( flags != 0 )
> -                for ( i = 0; i < (1UL << page_order); i++ )
> -                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
> -            else
> -                for ( int i = 0; i < (1UL << page_order); i++ )
> -                    iommu_unmap_page(p2m->domain, gfn+i);
> -        }
> +            for ( int i = 0; i < (1UL << page_order); i++ )
> +                iommu_unmap_page(p2m->domain, gfn+i);
>      }
>  
>   out:
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index a8c60ec..2808c31 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -640,9 +640,6 @@ int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
>  
>      BUG_ON( !hd->arch.root_table );
>  
> -    if ( iommu_use_hap_pt(d) )
> -        return 0;
> -
>      memset(pt_mfn, 0, sizeof(pt_mfn));
>  
>      spin_lock(&hd->arch.mapping_lock);
> @@ -718,9 +715,6 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>  
>      BUG_ON( !hd->arch.root_table );
>  
> -    if ( iommu_use_hap_pt(d) )
> -        return 0;
> -
>      memset(pt_mfn, 0, sizeof(pt_mfn));
>  
>      spin_lock(&hd->arch.mapping_lock);
> @@ -777,30 +771,3 @@ int amd_iommu_reserve_domain_unity_map(struct domain *domain,
>      }
>      return 0;
>  }
> -
> -/* Share p2m table with iommu. */
> -void amd_iommu_share_p2m(struct domain *d)
> -{
> -    struct hvm_iommu *hd  = domain_hvm_iommu(d);
> -    struct page_info *p2m_table;
> -    mfn_t pgd_mfn;
> -
> -    ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
> -
> -    if ( !iommu_use_hap_pt(d) )
> -        return;
> -
> -    pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
> -    p2m_table = mfn_to_page(mfn_x(pgd_mfn));
> -
> -    if ( hd->arch.root_table != p2m_table )
> -    {
> -        free_amd_iommu_pgtable(hd->arch.root_table);
> -        hd->arch.root_table = p2m_table;
> -
> -        /* When sharing p2m with iommu, paging mode = 4 */
> -        hd->arch.paging_mode = IOMMU_PAGING_MODE_LEVEL_4;
> -        AMD_IOMMU_DEBUG("Share p2m table with iommu: p2m table = %#lx\n",
> -                        mfn_x(pgd_mfn));
> -    }
> -}
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 0b301b3..c893dea 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -453,9 +453,6 @@ static void deallocate_iommu_page_tables(struct domain *d)
>  {
>      struct hvm_iommu *hd  = domain_hvm_iommu(d);
>  
> -    if ( iommu_use_hap_pt(d) )
> -        return;
> -
>      spin_lock(&hd->arch.mapping_lock);
>      if ( hd->arch.root_table )
>      {
> @@ -619,7 +616,6 @@ const struct iommu_ops amd_iommu_ops = {
>      .setup_hpet_msi = amd_setup_hpet_msi,
>      .suspend = amd_iommu_suspend,
>      .resume = amd_iommu_resume,
> -    .share_p2m = amd_iommu_share_p2m,
>      .crash_shutdown = amd_iommu_suspend,
>      .dump_p2m_table = amd_dump_p2m_table,
>  };
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index cc12735..5d3299a 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -332,7 +332,7 @@ void iommu_share_p2m_table(struct domain* d)
>  {
>      const struct iommu_ops *ops = iommu_get_ops();
>  
> -    if ( iommu_enabled && is_hvm_domain(d) )
> +    if ( iommu_enabled && is_hvm_domain(d) && ops->share_p2m)
>          ops->share_p2m(d);

Looks fine to me, however I'm not sure if it would be clearer to use
iommu_use_hap_pt(d) instead of checking ops->share_p2m directly.

Roger.


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

Ping: [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Jan Beulich]

>>> On 04.06.14 at 12:06, <roger.pau@citrix.com> wrote:
> According to the comment in p2m.h, AMD IOMMUs don't work correctly
> with page types different than p2m_ram_rw when the p2m is shared
> between HAP and IOMMU, so disable this sharing when using AMD IOMMUs.

Guys,

over 2 months have passed since this was sent, and we still didn't
get an ack from either of you. (Yes, I realize that only Suravee was
copied originally.)

Thanks, Jan

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Xiantao Zhang <xiantao.zhang@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
> I have not tested this patch on AMD hardware, so I would like some
> confirmation that this actually works.
> ---
> Changes since v1:
>  - Add a debug message when disabling iommu_hap_pt_share.
> ---
>  xen/drivers/passthrough/amd/iommu_init.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
> b/xen/drivers/passthrough/amd/iommu_init.c
> index b2f74ef..56bda00 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1255,6 +1255,14 @@ int __init amd_iommu_init(void)
>      if ( iterate_ivrs_mappings(amd_iommu_setup_device_table) != 0 )
>          goto error_out;
>  
> +    /*
> +     * Disable sharing HAP page tables with AMD IOMMU,
> +     * since it only supports p2m_ram_rw, and this would
> +     * prevent doing IO to/from mapped grant frames.
> +     */
> +    iommu_hap_pt_share = 0;
> +    printk(XENLOG_DEBUG "AMD-Vi: Disabled HAP memory map sharing with IOMMU\n");
> +
>      /* per iommu initialization  */
>      for_each_amd_iommu ( iommu )
>          if ( amd_iommu_init_one(iommu) != 0 )
> -- 
> 1.7.7.5 (Apple Git-26)



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

[PATCH v4 4/4] xen: expose that grant table mappings update the IOMMU

[Roger Pau Monne]

Add a new CPUID flag for leaf 4 that indicates that grant/foreign
mappings update the IOMMU accordingly.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/hvm.c              |   10 ++++++++++
 xen/include/public/arch-x86/cpuid.h |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4f993f4..f4de00d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -67,6 +67,7 @@
 #include <asm/mem_access.h>
 #include <public/mem_event.h>
 #include <xen/rangeset.h>
+#include <public/arch-x86/cpuid.h>
 
 bool_t __read_mostly hvm_enabled;
 
@@ -4074,6 +4075,15 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
     *eax = *ebx = *ecx = *edx = 0;
     if ( hvm_funcs.hypervisor_cpuid_leaf )
         hvm_funcs.hypervisor_cpuid_leaf(sub_idx, eax, ebx, ecx, edx);
+
+    if ( sub_idx == 0 )
+    {
+        /*
+         * Indicate that memory mapped from other domains (either grants or
+         * foreign pages) has valid IOMMU entries.
+         */
+        *eax |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
+    }
 }
 
 void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index 7135d54..e57a457 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -73,6 +73,8 @@
 /* EAX Features */
 #define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC registers */
 #define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC accesses */
+/* Memory mapped from other domains has valid IOMMU entries */
+#define XEN_HVM_CPUID_IOMMU_MAPPINGS   (1u << 2)
 
 #define XEN_CPUID_MAX_NUM_LEAVES 4
 
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v4 4/4] xen: expose that grant table mappings update the IOMMU

[Jan Beulich]

>>> On 04.06.14 at 12:06, <roger.pau@citrix.com> wrote:
> Add a new CPUID flag for leaf 4 that indicates that grant/foreign
> mappings update the IOMMU accordingly.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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