[PATCH RFC 0/2] Fix grant map/unmap with auto-translated guests

[PATCH RFC 0/2] Fix grant map/unmap with auto-translated guests

[Roger Pau Monne]

This series adds proper IOMMU entries when performing grant 
mapping/unmapping inside of auto-translated guests.

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.

[PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on
[PATCH RFC 2/2] xen: expose that grant table mappings update the

[PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Roger Pau Monne]

When using grant mappings with auto-translated guests (PVH, HVM), Xen
was not updating the IOMMU entries for the domain. This patch adds the
necessary bits in order to properly update the IOMMU when doing grant
maps/unmap on auto-translated guests.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/common/grant_table.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 107b000..665f34b 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
             goto undo_out;
         }
     }
+    else if ( need_iommu(ld) )
+    {
+        int err;
+
+        BUG_ON(!paging_mode_translate(ld));
+        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
+                             op->flags & GNTMAP_readonly ?
+                                IOMMUF_readable :
+                                IOMMUF_readable|IOMMUF_writable);
+        if ( err )
+        {
+            double_gt_unlock(lgt, rgt);
+            rc = GNTST_general_error;
+            goto undo_out;
+        }
+    }
 
     TRACE_1D(TRC_MEM_PAGE_GRANT_MAP, op->dom);
 
@@ -947,6 +963,18 @@ __gnttab_unmap_common(
             goto unmap_out;
         }
     }
+    else if ( need_iommu(ld) )
+    {
+        int err;
+
+        BUG_ON(!paging_mode_translate(ld));
+        err = iommu_unmap_page(ld, op->host_addr >> PAGE_SHIFT);
+        if ( err )
+        {
+            rc = GNTST_general_error;
+            goto unmap_out;
+        }
+    }
 
     /* If just unmapped a writable mapping, mark as dirtied */
     if ( !(op->flags & GNTMAP_readonly) )
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH RFC 2/2] 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: Mukesh Rathor <mukesh.rathor@oracle.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 RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Jan Beulich]

>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>              goto undo_out;
>          }
>      }
> +    else if ( need_iommu(ld) )
> +    {
> +        int err;
> +
> +        BUG_ON(!paging_mode_translate(ld));
> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
> +                             op->flags & GNTMAP_readonly ?
> +                                IOMMUF_readable :
> +                                IOMMUF_readable|IOMMUF_writable);
> +        if ( err )
> +        {
> +            double_gt_unlock(lgt, rgt);
> +            rc = GNTST_general_error;
> +            goto undo_out;
> +        }
> +    }

As much of this as possible should be folded with the if() branch.
And looking at the PV code, I think it makes no sense for the
conditions whether to map the page r/o or r/w to be different
between PV and non-PV.

Plus - wouldn't it be better to have this taken care of via
create_grant_host_mapping(), by not blindly calling
iommu_unmap_page() on anything other than p2m_ram_rw in
ept_set_entry() and p2m_set_entry(), the more that this should
already be taken care of in the iommu_hap_pt_share case.

> @@ -947,6 +963,18 @@ __gnttab_unmap_common(
>              goto unmap_out;
>          }
>      }
> +    else if ( need_iommu(ld) )
> +    {
> +        int err;
> +
> +        BUG_ON(!paging_mode_translate(ld));
> +        err = iommu_unmap_page(ld, op->host_addr >> PAGE_SHIFT);
> +        if ( err )
> +        {
> +            rc = GNTST_general_error;
> +            goto unmap_out;
> +        }
> +    }

Same thing here obviously.

Jan

Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Roger Pau Monné]

On 04/04/14 17:35, Jan Beulich wrote:
>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>              goto undo_out;
>>          }
>>      }
>> +    else if ( need_iommu(ld) )
>> +    {
>> +        int err;
>> +
>> +        BUG_ON(!paging_mode_translate(ld));
>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>> +                             op->flags & GNTMAP_readonly ?
>> +                                IOMMUF_readable :
>> +                                IOMMUF_readable|IOMMUF_writable);
>> +        if ( err )
>> +        {
>> +            double_gt_unlock(lgt, rgt);
>> +            rc = GNTST_general_error;
>> +            goto undo_out;
>> +        }
>> +    }
> 
> As much of this as possible should be folded with the if() branch.
> And looking at the PV code, I think it makes no sense for the
> conditions whether to map the page r/o or r/w to be different
> between PV and non-PV.
> 
> Plus - wouldn't it be better to have this taken care of via
> create_grant_host_mapping(), by not blindly calling
> iommu_unmap_page() on anything other than p2m_ram_rw in
> ept_set_entry() and p2m_set_entry(), the more that this should
> already be taken care of in the iommu_hap_pt_share case.

Thanks for the comment, it indeed makes much more sense to fix this in 
ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
all page types except p2m_mmio_direct (when the hap memory map is not 
shared with IOMMU):

---
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index beb7288..611c8e2 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -475,18 +475,22 @@ out:
             iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present);
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
             {
+                unsigned int flags;
+
+                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == p2m_ram_ro) ) ?
+                        IOMMUF_readable : IOMMUF_readable | IOMMUF_writable;
                 if ( order > 0 )
                 {
                     for ( i = 0; i < (1 << order); i++ )
                         iommu_map_page(
                             p2m->domain, gfn - offset + i, mfn_x(mfn) - offset + i,
-                            IOMMUF_readable | IOMMUF_writable);
+                            flags);
                 }
                 else if ( !order )
                     iommu_map_page(
-                        p2m->domain, gfn, mfn_x(mfn), IOMMUF_readable | IOMMUF_writable);
+                        p2m->domain, gfn, mfn_x(mfn), flags);
             }
             else
             {
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 766fd67..c564cb6 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -450,11 +450,15 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         }
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
+            {
+                unsigned int flags;
+
+                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == p2m_ram_ro) ) ?
+                        IOMMUF_readable : IOMMUF_readable | IOMMUF_writable;
                 for ( i = 0; i < (1UL << page_order); i++ )
-                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i,
-                                   IOMMUF_readable|IOMMUF_writable);
-            else
+                    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);
         }

Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Jan Beulich]

>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote:
> On 04/04/14 17:35, Jan Beulich wrote:
>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>>              goto undo_out;
>>>          }
>>>      }
>>> +    else if ( need_iommu(ld) )
>>> +    {
>>> +        int err;
>>> +
>>> +        BUG_ON(!paging_mode_translate(ld));
>>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>>> +                             op->flags & GNTMAP_readonly ?
>>> +                                IOMMUF_readable :
>>> +                                IOMMUF_readable|IOMMUF_writable);
>>> +        if ( err )
>>> +        {
>>> +            double_gt_unlock(lgt, rgt);
>>> +            rc = GNTST_general_error;
>>> +            goto undo_out;
>>> +        }
>>> +    }
>> 
>> As much of this as possible should be folded with the if() branch.
>> And looking at the PV code, I think it makes no sense for the
>> conditions whether to map the page r/o or r/w to be different
>> between PV and non-PV.
>> 
>> Plus - wouldn't it be better to have this taken care of via
>> create_grant_host_mapping(), by not blindly calling
>> iommu_unmap_page() on anything other than p2m_ram_rw in
>> ept_set_entry() and p2m_set_entry(), the more that this should
>> already be taken care of in the iommu_hap_pt_share case.
> 
> Thanks for the comment, it indeed makes much more sense to fix this in 
> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
> all page types except p2m_mmio_direct (when the hap memory map is not 
> shared with IOMMU):

This seems to be going a little too far - I'm not sure we want to include
all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged,
and p2m_ram_broken all might require different treatment. Please do
this via some sort of switch() setting the permissions instead, calling
iommu_unmap_page() when the permissions remain zero.

Jan

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -475,18 +475,22 @@ out:
>              iommu_pte_flush(d, gfn, (u64*)ept_entry, order, 
> vtd_pte_present);
>          else
>          {
> -            if ( p2mt == p2m_ram_rw )
> +            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
>              {
> +                unsigned int flags;
> +
> +                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == 
> p2m_ram_ro) ) ?
> +                        IOMMUF_readable : IOMMUF_readable | 
> IOMMUF_writable;
>                  if ( order > 0 )
>                  {
>                      for ( i = 0; i < (1 << order); i++ )
>                          iommu_map_page(
>                              p2m->domain, gfn - offset + i, mfn_x(mfn) - offset 
> + i,
> -                            IOMMUF_readable | IOMMUF_writable);
> +                            flags);
>                  }
>                  else if ( !order )
>                      iommu_map_page(
> -                        p2m->domain, gfn, mfn_x(mfn), IOMMUF_readable | 
> IOMMUF_writable);
> +                        p2m->domain, gfn, mfn_x(mfn), flags);
>              }
>              else
>              {
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 766fd67..c564cb6 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -450,11 +450,15 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long 
> gfn, mfn_t mfn,
>          }
>          else
>          {
> -            if ( p2mt == p2m_ram_rw )
> +            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
> +            {
> +                unsigned int flags;
> +
> +                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == 
> p2m_ram_ro) ) ?
> +                        IOMMUF_readable : IOMMUF_readable | 
> IOMMUF_writable;
>                  for ( i = 0; i < (1UL << page_order); i++ )
> -                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i,
> -                                   IOMMUF_readable|IOMMUF_writable);
> -            else
> +                    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);
>          }

Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Roger Pau Monné]

On 07/04/14 14:04, Jan Beulich wrote:
>>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote:
>> On 04/04/14 17:35, Jan Beulich wrote:
>>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>>>              goto undo_out;
>>>>          }
>>>>      }
>>>> +    else if ( need_iommu(ld) )
>>>> +    {
>>>> +        int err;
>>>> +
>>>> +        BUG_ON(!paging_mode_translate(ld));
>>>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>>>> +                             op->flags & GNTMAP_readonly ?
>>>> +                                IOMMUF_readable :
>>>> +                                IOMMUF_readable|IOMMUF_writable);
>>>> +        if ( err )
>>>> +        {
>>>> +            double_gt_unlock(lgt, rgt);
>>>> +            rc = GNTST_general_error;
>>>> +            goto undo_out;
>>>> +        }
>>>> +    }
>>>
>>> As much of this as possible should be folded with the if() branch.
>>> And looking at the PV code, I think it makes no sense for the
>>> conditions whether to map the page r/o or r/w to be different
>>> between PV and non-PV.
>>>
>>> Plus - wouldn't it be better to have this taken care of via
>>> create_grant_host_mapping(), by not blindly calling
>>> iommu_unmap_page() on anything other than p2m_ram_rw in
>>> ept_set_entry() and p2m_set_entry(), the more that this should
>>> already be taken care of in the iommu_hap_pt_share case.
>>
>> Thanks for the comment, it indeed makes much more sense to fix this in 
>> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
>> all page types except p2m_mmio_direct (when the hap memory map is not 
>> shared with IOMMU):
> 
> This seems to be going a little too far - I'm not sure we want to include
> all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged,
> and p2m_ram_broken all might require different treatment. Please do
> this via some sort of switch() setting the permissions instead, calling
> iommu_unmap_page() when the permissions remain zero.

Right, I was looking at the wrong p2m.h header (the ARM one), which has 
a more limited set of p2m types.

I'm not sure if the type p2m_ram_shared should be given an IOMMU 
read-only entry, that's what I've done in the patch below.

Also, there's a comment on the top of p2m.h that worries me:

/*
 * AMD IOMMU: When we share p2m table with iommu, bit 52 -bit 58 in pte 
 * cannot be non-zero, otherwise, hardware generates io page faults when 
 * device access those pages. Therefore, p2m_ram_rw has to be defined as 0.
 */

Does this mean that on AMD if the map is shared between HAP and IOMMU, 
the only page type accessible from the IOMMU would be p2m_ram_rw? If 
so, that should be fixed also (probably by setting iommu_hap_pt_share = 
0 on AMD hardware).

---
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index beb7288..993d11a 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -475,18 +475,37 @@ out:
             iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present);
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            unsigned int flags;
+
+            switch( p2mt )
+            {
+            case p2m_ram_rw:
+            case p2m_grant_map_rw:
+            case p2m_map_foreign:
+                flags =  IOMMUF_readable | IOMMUF_writable;
+                break;
+            case p2m_ram_ro:
+            case p2m_grant_map_ro:
+            case p2m_ram_shared:
+                flags = IOMMUF_readable;
+                break;
+            default:
+                flags = 0;
+                break;
+            }
+
+            if ( flags != 0 )
             {
                 if ( order > 0 )
                 {
                     for ( i = 0; i < (1 << order); i++ )
                         iommu_map_page(
                             p2m->domain, gfn - offset + i, mfn_x(mfn) - offset + i,
-                            IOMMUF_readable | IOMMUF_writable);
+                            flags);
                 }
                 else if ( !order )
                     iommu_map_page(
-                        p2m->domain, gfn, mfn_x(mfn), IOMMUF_readable | IOMMUF_writable);
+                        p2m->domain, gfn, mfn_x(mfn), flags);
             }
             else
             {
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 766fd67..33addf9 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -450,10 +450,28 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         }
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            unsigned int flags;
+
+            switch( p2mt )
+            {
+            case p2m_ram_rw:
+            case p2m_grant_map_rw:
+            case p2m_map_foreign:
+                flags =  IOMMUF_readable | IOMMUF_writable;
+                break;
+            case p2m_ram_ro:
+            case p2m_grant_map_ro:
+            case p2m_ram_shared:
+                flags = IOMMUF_readable;
+                break;
+            default:
+                flags = 0;
+                break;
+            }
+
+            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);

Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Jan Beulich]

>>> On 07.04.14 at 16:40, <roger.pau@citrix.com> wrote:
> On 07/04/14 14:04, Jan Beulich wrote:
>>>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote:
>>> On 04/04/14 17:35, Jan Beulich wrote:
>>>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>>>>              goto undo_out;
>>>>>          }
>>>>>      }
>>>>> +    else if ( need_iommu(ld) )
>>>>> +    {
>>>>> +        int err;
>>>>> +
>>>>> +        BUG_ON(!paging_mode_translate(ld));
>>>>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>>>>> +                             op->flags & GNTMAP_readonly ?
>>>>> +                                IOMMUF_readable :
>>>>> +                                IOMMUF_readable|IOMMUF_writable);
>>>>> +        if ( err )
>>>>> +        {
>>>>> +            double_gt_unlock(lgt, rgt);
>>>>> +            rc = GNTST_general_error;
>>>>> +            goto undo_out;
>>>>> +        }
>>>>> +    }
>>>>
>>>> As much of this as possible should be folded with the if() branch.
>>>> And looking at the PV code, I think it makes no sense for the
>>>> conditions whether to map the page r/o or r/w to be different
>>>> between PV and non-PV.
>>>>
>>>> Plus - wouldn't it be better to have this taken care of via
>>>> create_grant_host_mapping(), by not blindly calling
>>>> iommu_unmap_page() on anything other than p2m_ram_rw in
>>>> ept_set_entry() and p2m_set_entry(), the more that this should
>>>> already be taken care of in the iommu_hap_pt_share case.
>>>
>>> Thanks for the comment, it indeed makes much more sense to fix this in 
>>> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
>>> all page types except p2m_mmio_direct (when the hap memory map is not 
>>> shared with IOMMU):
>> 
>> This seems to be going a little too far - I'm not sure we want to include
>> all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged,
>> and p2m_ram_broken all might require different treatment. Please do
>> this via some sort of switch() setting the permissions instead, calling
>> iommu_unmap_page() when the permissions remain zero.
> 
> Right, I was looking at the wrong p2m.h header (the ARM one), which has 
> a more limited set of p2m types.
> 
> I'm not sure if the type p2m_ram_shared should be given an IOMMU 
> read-only entry, that's what I've done in the patch below.

I'm not sure about the right behavior there too, hence copying Tim
and Andres. However, I'm told page sharing doesn't work when an
IOMMU is in use by a domain anyway, so not handling this case
properly wouldn't have any bad consequences for now.

> Also, there's a comment on the top of p2m.h that worries me:
> 
> /*
>  * AMD IOMMU: When we share p2m table with iommu, bit 52 -bit 58 in pte 
>  * cannot be non-zero, otherwise, hardware generates io page faults when 
>  * device access those pages. Therefore, p2m_ram_rw has to be defined as 0.
>  */
> 
> Does this mean that on AMD if the map is shared between HAP and IOMMU, 
> the only page type accessible from the IOMMU would be p2m_ram_rw? If 

Yes, that does mean exactly that.

> so, that should be fixed also (probably by setting iommu_hap_pt_share = 
> 0 on AMD hardware).

We're considering to drop the sharing altogether anyway, and are
anxiously awaiting Intel to fix their VT-d code for this to not have an
otherwise expected performance impact.

Jan

Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

[Andres Lagar-Cavilla]

On Apr 7, 2014, at 11:18 AM, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 07.04.14 at 16:40, <roger.pau@citrix.com> wrote:
>> On 07/04/14 14:04, Jan Beulich wrote:
>>>>>> On 07.04.14 at 13:51, <roger.pau@citrix.com> wrote:
>>>> On 04/04/14 17:35, Jan Beulich wrote:
>>>>>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
>>>>>> --- a/xen/common/grant_table.c
>>>>>> +++ b/xen/common/grant_table.c
>>>>>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>>>>>             goto undo_out;
>>>>>>         }
>>>>>>     }
>>>>>> +    else if ( need_iommu(ld) )
>>>>>> +    {
>>>>>> +        int err;
>>>>>> +
>>>>>> +        BUG_ON(!paging_mode_translate(ld));
>>>>>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>>>>>> +                             op->flags & GNTMAP_readonly ?
>>>>>> +                                IOMMUF_readable :
>>>>>> +                                IOMMUF_readable|IOMMUF_writable);
>>>>>> +        if ( err )
>>>>>> +        {
>>>>>> +            double_gt_unlock(lgt, rgt);
>>>>>> +            rc = GNTST_general_error;
>>>>>> +            goto undo_out;
>>>>>> +        }
>>>>>> +    }
>>>>> 
>>>>> As much of this as possible should be folded with the if() branch.
>>>>> And looking at the PV code, I think it makes no sense for the
>>>>> conditions whether to map the page r/o or r/w to be different
>>>>> between PV and non-PV.
>>>>> 
>>>>> Plus - wouldn't it be better to have this taken care of via
>>>>> create_grant_host_mapping(), by not blindly calling
>>>>> iommu_unmap_page() on anything other than p2m_ram_rw in
>>>>> ept_set_entry() and p2m_set_entry(), the more that this should
>>>>> already be taken care of in the iommu_hap_pt_share case.
>>>> 
>>>> Thanks for the comment, it indeed makes much more sense to fix this in 
>>>> ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
>>>> all page types except p2m_mmio_direct (when the hap memory map is not 
>>>> shared with IOMMU):
>>> 
>>> This seems to be going a little too far - I'm not sure we want to include
>>> all types here: p2m_mmio_dm, p2m_ram_paging_*, p2m_ram_paged,
>>> and p2m_ram_broken all might require different treatment. Please do
>>> this via some sort of switch() setting the permissions instead, calling
>>> iommu_unmap_page() when the permissions remain zero.
>> 
>> Right, I was looking at the wrong p2m.h header (the ARM one), which has 
>> a more limited set of p2m types.
>> 
>> I'm not sure if the type p2m_ram_shared should be given an IOMMU 
>> read-only entry, that's what I've done in the patch below.
> 
> I'm not sure about the right behavior there too, hence copying Tim
> and Andres. However, I'm told page sharing doesn't work when an
> IOMMU is in use by a domain anyway, so not handling this case
> properly wouldn't have any bad consequences for now.
IIRC we put a big interlock XORing sharing/paging/pod and IOMMU. Once the IOMMU thinks it can do DMA you can't mutate that p2m region willy-nilly. So easy path for now is to impose mutual exclusion between those features.

Andres
> 
>> Also, there's a comment on the top of p2m.h that worries me:
>> 
>> /*
>> * AMD IOMMU: When we share p2m table with iommu, bit 52 -bit 58 in pte 
>> * cannot be non-zero, otherwise, hardware generates io page faults when 
>> * device access those pages. Therefore, p2m_ram_rw has to be defined as 0.
>> */
>> 
>> Does this mean that on AMD if the map is shared between HAP and IOMMU, 
>> the only page type accessible from the IOMMU would be p2m_ram_rw? If 
> 
> Yes, that does mean exactly that.
> 
>> so, that should be fixed also (probably by setting iommu_hap_pt_share = 
>> 0 on AMD hardware).
> 
> We're considering to drop the sharing altogether anyway, and are
> anxiously awaiting Intel to fix their VT-d code for this to not have an
> otherwise expected performance impact.
> 
> Jan
>