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

[PATCH v2 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 v2 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>
---
 xen/arch/x86/mm/hap/hap.c       |    9 +++++++++
 xen/arch/x86/mm/shadow/common.c |    9 +++++++++
 xen/drivers/passthrough/iommu.c |    3 ++-
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 68a0c25..352edfd 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -175,6 +175,15 @@ out:
  */
 static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
 {
+
+    if ( iommu_enabled && need_iommu(d) && log_global )
+    {
+        /*
+         * Refuse to turn on global log-dirty mode
+         * if the domain is using the IOMMU.
+         */
+        return -EINVAL;
+    }
     /* turn on PG_log_dirty bit in paging mode */
     paging_lock(d);
     d->arch.paging.mode |= PG_log_dirty;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 9258d2a..dd55455 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3422,6 +3422,15 @@ int shadow_enable_log_dirty(struct domain *d, bool_t log_global)
 {
     int ret;
 
+    if ( iommu_enabled && need_iommu(d) && log_global )
+    {
+        /*
+         * Refuse to turn on global log-dirty mode
+         * if the domain is using the IOMMU.
+         */
+        return -EINVAL;
+    }
+
     paging_lock(d);
     if ( shadow_mode_enabled(d) )
     {
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 0cf0748..ef6660f 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -292,7 +292,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 ||
+             paging_mode_log_dirty(d))) )
         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

[PATCH v2 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>
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 7dcfcb6..6e8a1af 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -472,10 +472,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 915f60d..2b36a10 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -291,9 +291,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 )
@@ -456,10 +454,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 47604da..54f5a88 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -670,6 +670,33 @@ void p2m_flush_nestedp2m(struct domain *d);
 void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
     l1_pgentry_t *p, mfn_t table_mfn, 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 v2 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 4686813..a717601 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 v2 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 b371f8f..7589dc1 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 v2 1/4] xen: make logdirty and iommu mutually exclusive

[Jan Beulich]

>>> On 23.04.14 at 10:16, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -175,6 +175,15 @@ out:
>   */
>  static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>  {
> +
> +    if ( iommu_enabled && need_iommu(d) && log_global )

need_iommu() implies iommu_enabled, and hence checking only the
former ought to be sufficient.

Also, why do you do this here and in shadow code rather than in
paging_log_dirty_enable()?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -292,7 +292,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 ||
> +             paging_mode_log_dirty(d))) )

I'm afraid that would return true also for non-global log dirty mode.
This patch series
http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg02750.html
(patch 1) introduces a way to know whether global mode is enabled
- maybe you should base your series on top of that one?

Jan

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

[Jan Beulich]

>>> On 23.04.14 at 10:16, <roger.pau@citrix.com> 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>

> 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 7dcfcb6..6e8a1af 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -472,10 +472,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 915f60d..2b36a10 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -291,9 +291,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 )
> @@ -456,10 +454,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 47604da..54f5a88 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -670,6 +670,33 @@ void p2m_flush_nestedp2m(struct domain *d);
>  void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
>      l1_pgentry_t *p, mfn_t table_mfn, 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 v2 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[Jan Beulich]

>>> On 23.04.14 at 10:16, <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.

Doesn't this need to go before patch 2?

Jan

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

[Jan Beulich]

>>> On 23.04.14 at 10:16, <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.

Considering Boris' pending introduction of a hypervisor capabilities
CPUID leaf, I wonder whether we shouldn't stop surfacing HVM-only
features via XENFEAT_* flags (which afaict originally were meant to
be a PV vehicle only anyway).

Jan

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

[Roger Pau Monné]

On 23/04/14 11:57, Jan Beulich wrote:
>>>> On 23.04.14 at 10:16, <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.
> 
> Doesn't this need to go before patch 2?

It could certainly go before patch 2, but I don't see such a hard
requirement in the ordering (either way AMD hardware needs both patches
to work correctly, and applying one before the other is not going to
break bisection AFAICT).

Roger.

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

[Jan Beulich]

>>> On 23.04.14 at 12:04, <roger.pau@citrix.com> wrote:
> On 23/04/14 11:57, Jan Beulich wrote:
>>>>> On 23.04.14 at 10:16, <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.
>> 
>> Doesn't this need to go before patch 2?
> 
> It could certainly go before patch 2, but I don't see such a hard
> requirement in the ordering (either way AMD hardware needs both patches
> to work correctly, and applying one before the other is not going to
> break bisection AFAICT).

Patch 2 allows the high bits in present IOMMU entries to be non-zero,
thus allowing for IOMMU faults without this patch already in effect.

Jan

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

[Roger Pau Monné]

On 23/04/14 12:51, Jan Beulich wrote:
>>>> On 23.04.14 at 12:04, <roger.pau@citrix.com> wrote:
>> On 23/04/14 11:57, Jan Beulich wrote:
>>>>>> On 23.04.14 at 10:16, <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.
>>>
>>> Doesn't this need to go before patch 2?
>>
>> It could certainly go before patch 2, but I don't see such a hard
>> requirement in the ordering (either way AMD hardware needs both patches
>> to work correctly, and applying one before the other is not going to
>> break bisection AFAICT).
> 
> Patch 2 allows the high bits in present IOMMU entries to be non-zero,
> thus allowing for IOMMU faults without this patch already in effect.

Since on AMD hardware iommu_hap_pt_share is always true (unless the user
forces it to false), the previous patch has no effect without this one
(because the previous patch only deals with iommu_hap_pt_share ==
false). I don't mind moving it anyway, and will do it in the next version.

Roger.

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

[Jan Beulich]

>>> On 23.04.14 at 16:31, <roger.pau@citrix.com> wrote:
> On 23/04/14 12:51, Jan Beulich wrote:
>>>>> On 23.04.14 at 12:04, <roger.pau@citrix.com> wrote:
>>> On 23/04/14 11:57, Jan Beulich wrote:
>>>>>>> On 23.04.14 at 10:16, <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.
>>>>
>>>> Doesn't this need to go before patch 2?
>>>
>>> It could certainly go before patch 2, but I don't see such a hard
>>> requirement in the ordering (either way AMD hardware needs both patches
>>> to work correctly, and applying one before the other is not going to
>>> break bisection AFAICT).
>> 
>> Patch 2 allows the high bits in present IOMMU entries to be non-zero,
>> thus allowing for IOMMU faults without this patch already in effect.
> 
> Since on AMD hardware iommu_hap_pt_share is always true (unless the user
> forces it to false), the previous patch has no effect without this one
> (because the previous patch only deals with iommu_hap_pt_share ==
> false). I don't mind moving it anyway, and will do it in the next version.

And right you are - feel free to keep the order then as it was.

Jan

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

[Roger Pau Monné]

On 23/04/14 11:54, Jan Beulich wrote:
>>>> On 23.04.14 at 10:16, <roger.pau@citrix.com> wrote:
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -175,6 +175,15 @@ out:
>>   */
>>  static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>>  {
>> +
>> +    if ( iommu_enabled && need_iommu(d) && log_global )
> 
> need_iommu() implies iommu_enabled, and hence checking only the
> former ought to be sufficient.
> 
> Also, why do you do this here and in shadow code rather than in
> paging_log_dirty_enable()?

Sight... yes, much better place. Will change it in next version.

>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -292,7 +292,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 ||
>> +             paging_mode_log_dirty(d))) )
> 
> I'm afraid that would return true also for non-global log dirty mode.
> This patch series
> http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg02750.html
> (patch 1) introduces a way to know whether global mode is enabled
> - maybe you should base your series on top of that one?

Sure, this series are already on top of Mukesh p2m cleanup patches, I
don't mind rebasing it on top of your series also.

Roger.

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

[Roger Pau Monné]

On 23/04/14 12:00, Jan Beulich wrote:
>>>> On 23.04.14 at 10:16, <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.
> 
> Considering Boris' pending introduction of a hypervisor capabilities
> CPUID leaf, I wonder whether we shouldn't stop surfacing HVM-only
> features via XENFEAT_* flags (which afaict originally were meant to
> be a PV vehicle only anyway).

I don't have a strong opinion on the best way to publish this feature,
so if you prefer to do it using the new CPUID leaf, I will rework this
patch (although I have to say using XENFEAT_* seems easier).

Roger.

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

[Jan Beulich]

>>> On 24.04.14 at 09:43, <roger.pau@citrix.com> wrote:
> On 23/04/14 12:00, Jan Beulich wrote:
>>>>> On 23.04.14 at 10:16, <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.
>> 
>> Considering Boris' pending introduction of a hypervisor capabilities
>> CPUID leaf, I wonder whether we shouldn't stop surfacing HVM-only
>> features via XENFEAT_* flags (which afaict originally were meant to
>> be a PV vehicle only anyway).
> 
> I don't have a strong opinion on the best way to publish this feature,
> so if you prefer to do it using the new CPUID leaf, I will rework this
> patch (although I have to say using XENFEAT_* seems easier).

Let's explicitly ask a few others who may have an opinion either way
(by way of adding them to the To: list).

Jan

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

[Tim Deegan]

Hi, 

At 10:16 +0200 on 23 Apr (1398244581), 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>

Yes, this is the sort of thing I had in mind (thuogh I agree with
Jan's review comments).

Cheers,

Tim.

> ---
>  xen/arch/x86/mm/hap/hap.c       |    9 +++++++++
>  xen/arch/x86/mm/shadow/common.c |    9 +++++++++
>  xen/drivers/passthrough/iommu.c |    3 ++-
>  3 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 68a0c25..352edfd 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -175,6 +175,15 @@ out:
>   */
>  static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>  {
> +
> +    if ( iommu_enabled && need_iommu(d) && log_global )
> +    {
> +        /*
> +         * Refuse to turn on global log-dirty mode
> +         * if the domain is using the IOMMU.
> +         */
> +        return -EINVAL;
> +    }
>      /* turn on PG_log_dirty bit in paging mode */
>      paging_lock(d);
>      d->arch.paging.mode |= PG_log_dirty;
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 9258d2a..dd55455 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -3422,6 +3422,15 @@ int shadow_enable_log_dirty(struct domain *d, bool_t log_global)
>  {
>      int ret;
>  
> +    if ( iommu_enabled && need_iommu(d) && log_global )
> +    {
> +        /*
> +         * Refuse to turn on global log-dirty mode
> +         * if the domain is using the IOMMU.
> +         */
> +        return -EINVAL;
> +    }
> +
>      paging_lock(d);
>      if ( shadow_mode_enabled(d) )
>      {
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 0cf0748..ef6660f 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -292,7 +292,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 ||
> +             paging_mode_log_dirty(d))) )
>          return -EXDEV;
>  
>      if ( !spin_trylock(&pcidevs_lock) )
> -- 
> 1.7.7.5 (Apple Git-26)
> 

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

[Tim Deegan]

At 10:16 +0200 on 23 Apr (1398244582), 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: Tim Deegan <tim@xen.org>

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

[Roger Pau Monné]

On 24/04/14 09:36, Roger Pau Monné wrote:
> On 23/04/14 11:54, Jan Beulich wrote:
>>>>> On 23.04.14 at 10:16, <roger.pau@citrix.com> wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -292,7 +292,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 ||
>>> +             paging_mode_log_dirty(d))) )
>>
>> I'm afraid that would return true also for non-global log dirty mode.
>> This patch series
>> http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg02750.html
>> (patch 1) introduces a way to know whether global mode is enabled
>> - maybe you should base your series on top of that one?
> 
> Sure, this series are already on top of Mukesh p2m cleanup patches, I
> don't mind rebasing it on top of your series also.

I'm afraid I will have to wait for either you or Mukesh to rebase on top
of each other, since your patches don't apply cleanly on top of Mukesh's
p2m changes.

Roger.

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

[Konrad Rzeszutek Wilk]

On Wed, Apr 23, 2014 at 10:16:22AM +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>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>

Tested-by: David Zhuang <david.zhuang@oracle.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 7dcfcb6..6e8a1af 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -472,10 +472,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 915f60d..2b36a10 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -291,9 +291,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 )
> @@ -456,10 +454,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 47604da..54f5a88 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -670,6 +670,33 @@ void p2m_flush_nestedp2m(struct domain *d);
>  void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
>      l1_pgentry_t *p, mfn_t table_mfn, 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 v2 4/4] xen: expose that grant table mappings update the IOMMU

[Konrad Rzeszutek Wilk]

On Thu, Apr 24, 2014 at 09:23:15AM +0100, Jan Beulich wrote:
> >>> On 24.04.14 at 09:43, <roger.pau@citrix.com> wrote:
> > On 23/04/14 12:00, Jan Beulich wrote:
> >>>>> On 23.04.14 at 10:16, <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.
> >> 
> >> Considering Boris' pending introduction of a hypervisor capabilities
> >> CPUID leaf, I wonder whether we shouldn't stop surfacing HVM-only
> >> features via XENFEAT_* flags (which afaict originally were meant to
> >> be a PV vehicle only anyway).
> > 
> > I don't have a strong opinion on the best way to publish this feature,
> > so if you prefer to do it using the new CPUID leaf, I will rework this
> > patch (although I have to say using XENFEAT_* seems easier).
> 
> Let's explicitly ask a few others who may have an opinion either way
> (by way of adding them to the To: list).

Silence ..  :-)

How about just the easier path for now (XENFEAT_*)?
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 08.05.14 at 20:39, <konrad@darnok.org> wrote:
> On Thu, Apr 24, 2014 at 09:23:15AM +0100, Jan Beulich wrote:
>> >>> On 24.04.14 at 09:43, <roger.pau@citrix.com> wrote:
>> > On 23/04/14 12:00, Jan Beulich wrote:
>> >>>>> On 23.04.14 at 10:16, <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.
>> >> 
>> >> Considering Boris' pending introduction of a hypervisor capabilities
>> >> CPUID leaf, I wonder whether we shouldn't stop surfacing HVM-only
>> >> features via XENFEAT_* flags (which afaict originally were meant to
>> >> be a PV vehicle only anyway).
>> > 
>> > I don't have a strong opinion on the best way to publish this feature,
>> > so if you prefer to do it using the new CPUID leaf, I will rework this
>> > patch (although I have to say using XENFEAT_* seems easier).
>> 
>> Let's explicitly ask a few others who may have an opinion either way
>> (by way of adding them to the To: list).
> 
> Silence ..  :-)
> 
> How about just the easier path for now (XENFEAT_*)?

While continuing to think it being the wrong direction, I also don't want
to stand in the way of this making progress...

Jan