[PATCH v3 0/4] Fix grant map/unmap with auto-translated guests

[PATCH v3 0/4] Fix grant map/unmap 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 feature is introduced in the 
last patch.

Thanks for the review, Roger.

[PATCH v3 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>
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 v3 1/4] xen: make logdirty and iommu mutually exclusive

[Jan Beulich]

>>> On 23.05.14 at 18:33, <roger.pau@citrix.com> 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>

For the pass-through side:
Acked-by: Jan Beulich <jbeulich@suse.com>

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

[PATCH v3 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>
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 12547a3..28ed184 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -777,10 +777,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

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

[Konrad Rzeszutek Wilk]

On Fri, May 23, 2014 at 06:33:06PM +0200, Roger Pau Monne wrote:
> 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>

Seems that this has slipped:

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 12547a3..28ed184 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -777,10 +777,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 v3 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 ee8851a..34f67ec 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 v3 4/4] xen: expose that grant table mappings update the IOMMU

[Roger Pau Monne]

Add a new XENFEAT_hvm_gntmap_supports_iommu that is used to check
whether the hypervisor properly updates the IOMMU on auto-translated
guests when doing a grant table map/unmap operation.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/common/kernel.c           |    2 ++
 xen/include/public/features.h |    6 ++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 7e83353..d920543 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -316,11 +316,13 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             case guest_type_pvh:
                 fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
                              (1U << XENFEAT_supervisor_mode_kernel) |
+                             (1U << XENFEAT_hvm_gntmap_supports_iommu) |
                              (1U << XENFEAT_hvm_callback_vector);
                 break;
             case guest_type_hvm:
                 fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
                              (1U << XENFEAT_hvm_callback_vector) |
+                             (1U << XENFEAT_hvm_gntmap_supports_iommu) |
                              (1U << XENFEAT_hvm_pirqs);
                 break;
             }
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index a149aa6..eaa0187 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -94,6 +94,12 @@
 /* operation as Dom0 is supported */
 #define XENFEAT_dom0                      11
 
+/*
+ * If set, GNTTABOP_map_grant_ref sets the proper IOMMU mappings so the
+ * resulting mapped page can be used for IO with hardware devices.
+ */
+#define XENFEAT_hvm_gntmap_supports_iommu 12
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
1.7.7.5 (Apple Git-26)


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

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

[Jan Beulich]

>>> On 23.05.14 at 18:33, <roger.pau@citrix.com> wrote:
> Add a new XENFEAT_hvm_gntmap_supports_iommu that is used to check
> whether the hypervisor properly updates the IOMMU on auto-translated
> guests when doing a grant table map/unmap operation.

As said before, if this is indeed HVM-only (I assume PVH is being
implied here), this should be exposed via the new CPUID leaf
rather than via the feature map.

Jan

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

[Roger Pau Monné]

On 26/05/14 11:49, Jan Beulich wrote:
>>>> On 23.05.14 at 18:33, <roger.pau@citrix.com> wrote:
>> Add a new XENFEAT_hvm_gntmap_supports_iommu that is used to check
>> whether the hypervisor properly updates the IOMMU on auto-translated
>> guests when doing a grant table map/unmap operation.
> 
> As said before, if this is indeed HVM-only (I assume PVH is being
> implied here), this should be exposed via the new CPUID leaf
> rather than via the feature map.

OK, I've been looking into this, but it seems that for PVH cpuid is
limited to 2 leafs only, so hvm_hypervisor_cpuid_leaf is never called.
Will look into expanding this, but then I should probably add some
checks to prevent PVH guests from reporting either
XEN_HVM_CPUID_APIC_ACCESS_VIRT or XEN_HVM_CPUID_X2APIC_VIRT for the time
being.

Roger.