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

[PATCH 0/3] 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 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.

[PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share
[PATCH 2/3] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs
[PATCH 3/3] xen: expose that grant table mappings update the IOMMU

[PATCH 1/3] 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 fails to set
correct IOMMU mappings for memory types different than p2m_ram_rw.

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

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/p2m-ept.c |   24 +++++++++++++++++++++---
 xen/arch/x86/mm/p2m-pt.c  |   23 ++++++++++++++++++++---
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index beb7288..cf11a34 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -475,18 +475,36 @@ 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:
+                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..749f7eb 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -450,10 +450,27 @@ 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:
+                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);
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 2/3] 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.
---
 xen/drivers/passthrough/amd/iommu_init.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 4686813..7f75b3e 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1255,6 +1255,13 @@ 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;
+
     /* 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 3/3] 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 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Konrad Rzeszutek Wilk]

On Mon, Apr 07, 2014 at 06:02:02PM +0200, Roger Pau Monne wrote:
> If the memory map is not shared between HAP and IOMMU we fails to set

we fail
> correct IOMMU mappings for memory types different than p2m_ram_rw.
s/different/other/

> 
> This patchs adds IOMMU support for the following memory types:
> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
> 
> 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/p2m-ept.c |   24 +++++++++++++++++++++---
>  xen/arch/x86/mm/p2m-pt.c  |   23 ++++++++++++++++++++---
>  2 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index beb7288..cf11a34 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -475,18 +475,36 @@ 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:
> +                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..749f7eb 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -450,10 +450,27 @@ 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:
> +                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);
> -- 
> 1.7.7.5 (Apple Git-26)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

On Mon, Apr 07, 2014 at 06:02:03PM +0200, Roger Pau Monne 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

s/between/amongst/

And you are missing an '.' at the off the sentence.

> 
> 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.
> ---
>  xen/drivers/passthrough/amd/iommu_init.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index 4686813..7f75b3e 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1255,6 +1255,13 @@ 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;
> +
>      /* 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 2/3] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs

[George Dunlap]

On Mon, Apr 7, 2014 at 5:51 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Apr 07, 2014 at 06:02:03PM +0200, Roger Pau Monne 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
>
> s/between/amongst/

I would say "between".  "Amongst", besides sounding a bit archaic,
gives me the idea of something being divided up (the bread was shared
amongst them), as opposed to a single thing used by both (the car was
shared between them).

 -George

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

[Roger Pau Monné]

On 07/04/14 18:54, George Dunlap wrote:
> On Mon, Apr 7, 2014 at 5:51 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
>> On Mon, Apr 07, 2014 at 06:02:03PM +0200, Roger Pau Monne 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
>>
>> s/between/amongst/
> 
> I would say "between".  "Amongst", besides sounding a bit archaic,
> gives me the idea of something being divided up (the bread was shared
> amongst them), as opposed to a single thing used by both (the car was
> shared between them).

Thanks to both (George and Konrad) for spotting those mistakes, will fix
in the next revision.

Roger.

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Jan Beulich]

>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
> If the memory map is not shared between HAP and IOMMU we fails to set
> correct IOMMU mappings for memory types different than p2m_ram_rw.
> 
> This patchs adds IOMMU support for the following memory types:
> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.

I'm curious about the justification for p2m_map_foreign; the others
I agree with.

I also wonder whether p2m_ram_logdirty shouldn't be treated
equally to p2m_ram_rw: It's clearly better to have some video
corruption than to kill the guest due to excessive IOMMU faults,
should its kernel choose to DMA into the frame buffer. But at the
very minimum this needs to be included alongside p2m_ram_ro.

Jan

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

[Ian Campbell]

On Mon, 2014-04-07 at 18:02 +0200, Roger Pau Monne 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.

Is it the case on x86 that all devices are behind the IOMMU?

On ARM with have the issue that only a subset of peripherals are behind
the IOMMU, for the rest we need other tricks like 1:1 mappings and
swiotlb bouncing (for dom0 only, we just don't support passing these
devices thru to domU).

Just want to make sure you aren't going to trip over a similar issue on
x86 after this ABI has been set...

Ian.

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

[Jan Beulich]

>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1255,6 +1255,13 @@ 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;
> +

I guess at the very least when the option was specified on the
command line you should issue a warning, or perhaps even stay
away from enforcing this.

I also think we ought to consider alternatives before taking a harsh
step like this (and that's independent of my earlier indication that we
may want to switch away from sharing these tables): It looks like the
global bit in the host page tables is unused and ignored, and could
therefore be used to indicate grant entries. The IW bit could be
used to distinguish p2m_{ram,grant}_{ro,rw} respectively.

Curious what Tim's thoughts here are...

Jan

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

[Jan Beulich]

>>> On 08.04.14 at 10:34, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-04-07 at 18:02 +0200, Roger Pau Monne 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.
> 
> Is it the case on x86 that all devices are behind the IOMMU?

All PCI ones are. If someone passes through a device through
raw MMIO/PIO/PIRQ ranges, then the IOMMU may or may not
be involved. But I don't think we formally consider this model
valid/supported/secure for HVM guests (and for PV guests it's
insecure anyway, due to not requiring an IOMMU in the first
place).

Jan

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

[Ian Campbell]

On Tue, 2014-04-08 at 09:56 +0100, Jan Beulich wrote:
> >>> On 08.04.14 at 10:34, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-04-07 at 18:02 +0200, Roger Pau Monne 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.
> > 
> > Is it the case on x86 that all devices are behind the IOMMU?

I suppose I should have said "all DMA capable devices" or some such.

> All PCI ones are. If someone passes through a device through
> raw MMIO/PIO/PIRQ ranges, then the IOMMU may or may not
> be involved. But I don't think we formally consider this model
> valid/supported/secure for HVM guests (and for PV guests it's
> insecure anyway, due to not requiring an IOMMU in the first
> place).

I was thinking of PVH dom0 here, which is the closest analogue to the
ARM model.

Sounds like it might suffer from the same shortcomings as ARM has to
deal with.

Ian.

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Roger Pau Monné]

On 08/04/14 10:30, Jan Beulich wrote:
>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>> If the memory map is not shared between HAP and IOMMU we fails to set
>> correct IOMMU mappings for memory types different than p2m_ram_rw.
>>
>> This patchs adds IOMMU support for the following memory types:
>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
> 
> I'm curious about the justification for p2m_map_foreign; the others
> I agree with.

Thanks for the review, I didn't see a reason to not allow DMA transfers
to/from foreign pages, although there's no user of foreign pages that
use DMA.

> 
> I also wonder whether p2m_ram_logdirty shouldn't be treated
> equally to p2m_ram_rw: It's clearly better to have some video
> corruption than to kill the guest due to excessive IOMMU faults,
> should its kernel choose to DMA into the frame buffer. But at the
> very minimum this needs to be included alongside p2m_ram_ro.

Ack. I wasn't sure about p2m_ram_logdirty, so I just decided to not
include it. I will add it to my next revision as a RW zone if that's fine.

Roger.

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

[Jan Beulich]

>>> On 08.04.14 at 10:58, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-04-08 at 09:56 +0100, Jan Beulich wrote:
>> >>> On 08.04.14 at 10:34, <Ian.Campbell@citrix.com> wrote:
>> > On Mon, 2014-04-07 at 18:02 +0200, Roger Pau Monne 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.
>> > 
>> > Is it the case on x86 that all devices are behind the IOMMU?
> 
> I suppose I should have said "all DMA capable devices" or some such.
> 
>> All PCI ones are. If someone passes through a device through
>> raw MMIO/PIO/PIRQ ranges, then the IOMMU may or may not
>> be involved. But I don't think we formally consider this model
>> valid/supported/secure for HVM guests (and for PV guests it's
>> insecure anyway, due to not requiring an IOMMU in the first
>> place).
> 
> I was thinking of PVH dom0 here, which is the closest analogue to the
> ARM model.
> 
> Sounds like it might suffer from the same shortcomings as ARM has to
> deal with.

Except that on x86 there are hardly many DMA-capable non-PCI
devices, and even less one may want to consider passing through
to a guest.

Jan

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Jan Beulich]

>>> On 08.04.14 at 11:19, <roger.pau@citrix.com> wrote:
> On 08/04/14 10:30, Jan Beulich wrote:
>>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>>> If the memory map is not shared between HAP and IOMMU we fails to set
>>> correct IOMMU mappings for memory types different than p2m_ram_rw.
>>>
>>> This patchs adds IOMMU support for the following memory types:
>>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
>> 
>> I'm curious about the justification for p2m_map_foreign; the others
>> I agree with.
> 
> Thanks for the review, I didn't see a reason to not allow DMA transfers
> to/from foreign pages, although there's no user of foreign pages that
> use DMA.

They're rather special (tool stack maps of guest pages), and DMAing
into/out of them shouldn't normally be done. I'd clearly suggest
leaving them unmapped for now.

>> I also wonder whether p2m_ram_logdirty shouldn't be treated
>> equally to p2m_ram_rw: It's clearly better to have some video
>> corruption than to kill the guest due to excessive IOMMU faults,
>> should its kernel choose to DMA into the frame buffer. But at the
>> very minimum this needs to be included alongside p2m_ram_ro.
> 
> Ack. I wasn't sure about p2m_ram_logdirty, so I just decided to not
> include it. I will add it to my next revision as a RW zone if that's fine.

Yes please, subject to Tim saying otherwise.

Jan

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

[Ian Campbell]

On Tue, 2014-04-08 at 10:53 +0100, Jan Beulich wrote:
> >>> On 08.04.14 at 10:58, <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2014-04-08 at 09:56 +0100, Jan Beulich wrote:
> >> >>> On 08.04.14 at 10:34, <Ian.Campbell@citrix.com> wrote:
> >> > On Mon, 2014-04-07 at 18:02 +0200, Roger Pau Monne 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.
> >> > 
> >> > Is it the case on x86 that all devices are behind the IOMMU?
> > 
> > I suppose I should have said "all DMA capable devices" or some such.
> > 
> >> All PCI ones are. If someone passes through a device through
> >> raw MMIO/PIO/PIRQ ranges, then the IOMMU may or may not
> >> be involved. But I don't think we formally consider this model
> >> valid/supported/secure for HVM guests (and for PV guests it's
> >> insecure anyway, due to not requiring an IOMMU in the first
> >> place).
> > 
> > I was thinking of PVH dom0 here, which is the closest analogue to the
> > ARM model.
> > 
> > Sounds like it might suffer from the same shortcomings as ARM has to
> > deal with.
> 
> Except that on x86 there are hardly many DMA-capable non-PCI
> devices,

Sure.

> and even less one may want to consider passing through
> to a guest.

Again, PVH dom0 is my concern here. By default you would expect dom0 to
get given almost everything in the platform, including things which you
might not normally pass through to a guest.

Ian.

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

[Jan Beulich]

>>> On 08.04.14 at 11:57, <Ian.Campbell@citrix.com> wrote:
> Again, PVH dom0 is my concern here. By default you would expect dom0 to
> get given almost everything in the platform, including things which you
> might not normally pass through to a guest.

I think all we can do is say such devices (if any exist at all) may not
work with PVH Dom0. I don't see us accepting a 1:1 hack for x86 like
you put in place for ARM, and I take it that you need this hack only
because it's essential devices that may come in this form.

Jan

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

[Ian Campbell]

On Tue, 2014-04-08 at 11:05 +0100, Jan Beulich wrote:
> >>> On 08.04.14 at 11:57, <Ian.Campbell@citrix.com> wrote:
> > Again, PVH dom0 is my concern here. By default you would expect dom0 to
> > get given almost everything in the platform, including things which you
> > might not normally pass through to a guest.
> 
> I think all we can do is say such devices (if any exist at all) may not
> work with PVH Dom0. I don't see us accepting a 1:1 hack for x86 like
> you put in place for ARM, 

That's fair enough for x86 I think.

> and I take it that you need this hack only
> because it's essential devices that may come in this form.

That's right, sometimes it is NICs etc.

Plus we want dom0 to be able to run on systems with no IOMMU at all. I
think PVH is predicated on there being an IOMMU in the system.

Ian.

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

[Jan Beulich]

>>> On 08.04.14 at 12:26, <Ian.Campbell@citrix.com> wrote:
> Plus we want dom0 to be able to run on systems with no IOMMU at all. I
> think PVH is predicated on there being an IOMMU in the system.

Yes, it is.

Jan

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Tim Deegan]

At 09:30 +0100 on 08 Apr (1396945844), Jan Beulich wrote:
> >>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
> > If the memory map is not shared between HAP and IOMMU we fails to set
> > correct IOMMU mappings for memory types different than p2m_ram_rw.
> > 
> > This patchs adds IOMMU support for the following memory types:
> > p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
> 
> I'm curious about the justification for p2m_map_foreign; the others
> I agree with.
> 
> I also wonder whether p2m_ram_logdirty shouldn't be treated
> equally to p2m_ram_rw: It's clearly better to have some video
> corruption than to kill the guest due to excessive IOMMU faults,

Hmmm.  In that case it seems better; if we're absolutely sure that we
can't end up trying to do log-dirty for _migration_ while a guest has
a real device passed through, then yes, we can leave them as r/w to
the IOMMU. 

Maybe make sure at the same time that full log-dirty mode and
needs_iommu are mutually exclusive.

Tim.

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

[Tim Deegan]

At 09:52 +0100 on 08 Apr (1396947164), Jan Beulich wrote:
> >>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -1255,6 +1255,13 @@ 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;
> > +
> 
> I guess at the very least when the option was specified on the
> command line you should issue a warning, or perhaps even stay
> away from enforcing this.
> 
> I also think we ought to consider alternatives before taking a harsh
> step like this (and that's independent of my earlier indication that we
> may want to switch away from sharing these tables): It looks like the
> global bit in the host page tables is unused and ignored, and could
> therefore be used to indicate grant entries. The IW bit could be
> used to distinguish p2m_{ram,grant}_{ro,rw} respectively.
> 
> Curious what Tim's thoughts here are...

Happy to use the global bit to mark grant entries (though I haven't
checked the docs to be sure it's actually safe). OTOH it seems
like the hadres IOMMU tables might be going away for other reasons so
I'm not sure it's worth putting a lot of effort into being clever
around this.

Tim.

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

[Jan Beulich]

>>> On 08.04.14 at 16:16, <tim@xen.org> wrote:
> At 09:52 +0100 on 08 Apr (1396947164), Jan Beulich wrote:
>> >>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>> > --- a/xen/drivers/passthrough/amd/iommu_init.c
>> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> > @@ -1255,6 +1255,13 @@ 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;
>> > +
>> 
>> I guess at the very least when the option was specified on the
>> command line you should issue a warning, or perhaps even stay
>> away from enforcing this.
>> 
>> I also think we ought to consider alternatives before taking a harsh
>> step like this (and that's independent of my earlier indication that we
>> may want to switch away from sharing these tables): It looks like the
>> global bit in the host page tables is unused and ignored, and could
>> therefore be used to indicate grant entries. The IW bit could be
>> used to distinguish p2m_{ram,grant}_{ro,rw} respectively.
>> 
>> Curious what Tim's thoughts here are...
> 
> Happy to use the global bit to mark grant entries (though I haven't
> checked the docs to be sure it's actually safe). OTOH it seems
> like the hadres IOMMU tables might be going away for other reasons so
> I'm not sure it's worth putting a lot of effort into being clever
> around this.

Yeah - I simply wanted to make sure we think of alternatives if
they make sense; if they don't, and if we're settled to drop
shared tables anyway, then the patch is presumably fine as is.

Jan

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Roger Pau Monné]

On 08/04/14 16:12, Tim Deegan wrote:
> At 09:30 +0100 on 08 Apr (1396945844), Jan Beulich wrote:
>>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>>> If the memory map is not shared between HAP and IOMMU we fails to set
>>> correct IOMMU mappings for memory types different than p2m_ram_rw.
>>>
>>> This patchs adds IOMMU support for the following memory types:
>>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
>>
>> I'm curious about the justification for p2m_map_foreign; the others
>> I agree with.
>>
>> I also wonder whether p2m_ram_logdirty shouldn't be treated
>> equally to p2m_ram_rw: It's clearly better to have some video
>> corruption than to kill the guest due to excessive IOMMU faults,
> 
> Hmmm.  In that case it seems better; if we're absolutely sure that we
> can't end up trying to do log-dirty for _migration_ while a guest has
> a real device passed through, then yes, we can leave them as r/w to
> the IOMMU. 
> 
> Maybe make sure at the same time that full log-dirty mode and
> needs_iommu are mutually exclusive.

Would it suffice to add something like:

case p2m_ram_logdirty:
    ASSERT(!paging_mode_log_dirty(p2m->domain));

Or are you referring to a more global check?

Thanks, Roger.

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Jan Beulich]

>>> On 08.04.14 at 17:15, <roger.pau@citrix.com> wrote:
> On 08/04/14 16:12, Tim Deegan wrote:
>> At 09:30 +0100 on 08 Apr (1396945844), Jan Beulich wrote:
>>>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>>>> If the memory map is not shared between HAP and IOMMU we fails to set
>>>> correct IOMMU mappings for memory types different than p2m_ram_rw.
>>>>
>>>> This patchs adds IOMMU support for the following memory types:
>>>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
>>>
>>> I'm curious about the justification for p2m_map_foreign; the others
>>> I agree with.
>>>
>>> I also wonder whether p2m_ram_logdirty shouldn't be treated
>>> equally to p2m_ram_rw: It's clearly better to have some video
>>> corruption than to kill the guest due to excessive IOMMU faults,
>> 
>> Hmmm.  In that case it seems better; if we're absolutely sure that we
>> can't end up trying to do log-dirty for _migration_ while a guest has
>> a real device passed through, then yes, we can leave them as r/w to
>> the IOMMU. 
>> 
>> Maybe make sure at the same time that full log-dirty mode and
>> needs_iommu are mutually exclusive.
> 
> Would it suffice to add something like:
> 
> case p2m_ram_logdirty:
>     ASSERT(!paging_mode_log_dirty(p2m->domain));

Iirc that would also catch the VRAM dirty tracking, i.e. not be suitable.
I'll defer to Tim though to explain what he had in mind instead.

Jan

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

[Roger Pau Monné]

On 08/04/14 10:52, Jan Beulich wrote:
>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -1255,6 +1255,13 @@ 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;
>> +
> 
> I guess at the very least when the option was specified on the
> command line you should issue a warning, or perhaps even stay
> away from enforcing this.

I think we should enforce it even if the user has specified
sharept=true, or else we need to gate the
XENFEAT_hvm_gntmap_supports_iommu feature. Printing a Warning in this
case (user trying to enforce sharept on AMD IOMMU) seems OK to me.

Roger.

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Tim Deegan]

At 17:15 +0200 on 08 Apr (1396973715), Roger Pau Monné wrote:
> On 08/04/14 16:12, Tim Deegan wrote:
> > At 09:30 +0100 on 08 Apr (1396945844), Jan Beulich wrote:
> >>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
> >>> If the memory map is not shared between HAP and IOMMU we fails to set
> >>> correct IOMMU mappings for memory types different than p2m_ram_rw.
> >>>
> >>> This patchs adds IOMMU support for the following memory types:
> >>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
> >>
> >> I'm curious about the justification for p2m_map_foreign; the others
> >> I agree with.
> >>
> >> I also wonder whether p2m_ram_logdirty shouldn't be treated
> >> equally to p2m_ram_rw: It's clearly better to have some video
> >> corruption than to kill the guest due to excessive IOMMU faults,
> > 
> > Hmmm.  In that case it seems better; if we're absolutely sure that we
> > can't end up trying to do log-dirty for _migration_ while a guest has
> > a real device passed through, then yes, we can leave them as r/w to
> > the IOMMU. 
> > 
> > Maybe make sure at the same time that full log-dirty mode and
> > needs_iommu are mutually exclusive.
> 
> Would it suffice to add something like:
> 
> case p2m_ram_logdirty:
>     ASSERT(!paging_mode_log_dirty(p2m->domain));
> 
> Or are you referring to a more global check?

I was thinking that the paths that _set_ full log-dirty mode or
needs_iommu should explicitly test that the other isn't the case
(i.e. a logdirty-enable hypercall or assigning a passthough device
should fail).

Tim.

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Roger Pau Monné]

On 08/04/14 10:30, Jan Beulich wrote:
>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>> If the memory map is not shared between HAP and IOMMU we fails to set
>> correct IOMMU mappings for memory types different than p2m_ram_rw.
>>
>> This patchs adds IOMMU support for the following memory types:
>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
> 
> I'm curious about the justification for p2m_map_foreign; the others
> I agree with.

I've spoken with Stefano about this, because I've seen Qemu use foreign
mapped pages as arguments to systems calls like pread/pwrite, and I'm
quite sure we need IOMMU entries for foreign mappings. If Qemu opens the
disk file with O_DIRECT foreign mappings can end up in DMA requests.

Roger.

Re: [PATCH 1/3] iommu: set correct IOMMU entries when iommu_hap_pt_share == 0

[Jan Beulich]

>>> On 16.04.14 at 16:36, <roger.pau@citrix.com> wrote:
> On 08/04/14 10:30, Jan Beulich wrote:
>>>>> On 07.04.14 at 18:02, <roger.pau@citrix.com> wrote:
>>> If the memory map is not shared between HAP and IOMMU we fails to set
>>> correct IOMMU mappings for memory types different than p2m_ram_rw.
>>>
>>> This patchs adds IOMMU support for the following memory types:
>>> p2m_grant_map_rw, p2m_map_foreign, p2m_ram_ro and p2m_grant_map_ro.
>> 
>> I'm curious about the justification for p2m_map_foreign; the others
>> I agree with.
> 
> I've spoken with Stefano about this, because I've seen Qemu use foreign
> mapped pages as arguments to systems calls like pread/pwrite, and I'm
> quite sure we need IOMMU entries for foreign mappings. If Qemu opens the
> disk file with O_DIRECT foreign mappings can end up in DMA requests.

That's fine then, and at once puts off the option of using the global
bit for something (because now we certainly have more types than
we could encode).

One request though on the changes done: Please put the p2m-type-
to-IOMMU-flags conversion into an inline function, usable by both
callers. And iirc there's a third place (in p2m-pt.c) where you also
need to do that conversion (as replacement to just special casing
p2m_ram_rw).

Jan