Re: [Xen-devel] [PATCH v5 8/8] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

2018-08-17 Thread Jan Beulich 
>>> On 17.08.18 at 13:17,  wrote:
> On Fri, Aug 17, 2018 at 05:08:08AM -0600, Jan Beulich wrote:
>> >>> On 14.08.18 at 15:43,  wrote:
>> > @@ -185,7 +219,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
>> > *d)
>> >  if ( !hwdom_iommu_map(d, pfn, max_pfn) )
>> >  continue;
>> >  
>> > -rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
>> > +if ( iommu_use_hap_pt(d) )
>> > +{
>> > +ASSERT(is_hvm_domain(d));
>> > +rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
>> > +}
>> > +else
>> > +rc = iommu_map_page(d, pfn, pfn, 
>> > IOMMUF_readable|IOMMUF_writable);
>> 
>> Why iommu_use_hap_pt()? Shouldn't HAP with or without shared
>> page tables as well as shadow all get the same in-sync p2m and
>> IOMMU mappings?
> 
> iommu_map_page is a noop if iommu_use_hap_pt is true (see
> intel_iommu_map_page for example). Hence in the case the IOMMU page
> tables are shared with HAP the pages must be added to the p2m.

This wasn't what I'm after, while ...

> I could switch this to use set_identity_p2m_entry if the guest is
> auto-translated and only use iommu_map_page for non-autotranslated
> guests.

 indeed this is what I would prefer (unless there a reasons
for not doing so).

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 8/8] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

2018-08-17 Thread Roger Pau Monné 
On Fri, Aug 17, 2018 at 05:08:08AM -0600, Jan Beulich wrote:
> >>> On 14.08.18 at 15:43,  wrote:
> > @@ -185,7 +219,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain 
> > *d)
> >  if ( !hwdom_iommu_map(d, pfn, max_pfn) )
> >  continue;
> >  
> > -rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
> > +if ( iommu_use_hap_pt(d) )
> > +{
> > +ASSERT(is_hvm_domain(d));
> > +rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
> > +}
> > +else
> > +rc = iommu_map_page(d, pfn, pfn, 
> > IOMMUF_readable|IOMMUF_writable);
> 
> Why iommu_use_hap_pt()? Shouldn't HAP with or without shared
> page tables as well as shadow all get the same in-sync p2m and
> IOMMU mappings?

iommu_map_page is a noop if iommu_use_hap_pt is true (see
intel_iommu_map_page for example). Hence in the case the IOMMU page
tables are shared with HAP the pages must be added to the p2m.

I could switch this to use set_identity_p2m_entry if the guest is
auto-translated and only use iommu_map_page for non-autotranslated
guests.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 8/8] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

2018-08-17 Thread Jan Beulich 
>>> On 14.08.18 at 15:43,  wrote:
> @@ -138,13 +139,20 @@ static bool __hwdom_init hwdom_iommu_map(const struct 
> domain *d,
>   unsigned long pfn,
>   unsigned long max_pfn)
>  {
> +unsigned int i;
> +
>  /*
>   * Set up 1:1 mapping for dom0. Default to include only conventional RAM
>   * areas and let RMRRs include needed reserved regions. When set, the
>   * inclusive mapping additionally maps in every pfn up to 4GB except 
> those
> - * that fall in unusable ranges.
> + * that fall in unusable ranges for PV Dom0.
>   */
> -if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) )
> +if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) ||
> + /*
> +  * Ignore any address below 1MB, that's already identity mapped by 
> the
> +  * domain builder for HVM.
> +  */
> + (!d->domain_id && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) )

Would you mind clarifying the comment by saying "Dom0 builder"?
The respective source files have been renamed to dom0_build.c
for the same reason, quite some time ago.

> +/* Check that it doesn't overlap with the LAPIC */
> +if ( has_vlapic(d) )
> +{
> +const struct vcpu *v;
> +
> +for_each_vcpu(d, v)
> +if ( pfn == PFN_DOWN(vlapic_base_address(vcpu_vlapic(v))) )
> +return false;
> +}
> +/* ... or the IO-APIC */
> +for ( i = 0; has_vioapic(d) && i < d->arch.hvm_domain.nr_vioapics; i++ )
> +if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> +return false;
> +/*
> + * ... or the PCIe MCFG regions.
> + * TODO: runtime added MMCFG regions are not checked to make sure they
> + * don't overlap with already mapped regions, thus preventing trapping.
> + */
> +if ( has_vpci(d) && vpci_is_mmcfg_address(d, pfn_to_paddr(pfn)) )
> +return false;

Didn't we agree to have such a TODO also in the LAPIC loop?

> @@ -185,7 +219,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>  if ( !hwdom_iommu_map(d, pfn, max_pfn) )
>  continue;
>  
> -rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
> +if ( iommu_use_hap_pt(d) )
> +{
> +ASSERT(is_hvm_domain(d));
> +rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
> +}
> +else
> +rc = iommu_map_page(d, pfn, pfn, 
> IOMMUF_readable|IOMMUF_writable);

Why iommu_use_hap_pt()? Shouldn't HAP with or without shared
page tables as well as shadow all get the same in-sync p2m and
IOMMU mappings?

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 8/8] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

2018-08-14 Thread Roger Pau Monne 
Several people have reported hardware issues (malfunctioning USB
controllers) due to iommu page faults on Intel hardware. Those faults
are caused by missing RMRR (VTd) entries in the ACPI tables. Those can
be worked around on VTd hardware by manually adding RMRR entries on
the command line, this is however limited to Intel hardware and quite
cumbersome to do.

In order to solve those issues add a new dom0-iommu=map-reserved
option that identity maps all regions marked as reserved in the memory
map.  Note that regions used by devices emulated by Xen (LAPIC,
IO-APIC or PCIe MCFG regions) are specifically avoided. Note that this
option is available to a PVH Dom0 (as opposed to the inclusive option
which only works for PV Dom0).

Signed-off-by: Roger Pau Monné 
---
Changes since v4:
 - Use pfn_to_paddr.
 - Rebase on top of previous changes.
 - Change the default option setting to use if instead of a ternary
   operator.
 - Rename to map-reserved.

Changes since v3:
 - Add mappings if the iommu page tables are shared.

Changes since v2:
 - Fix comment regarding dom0-strict.
 - Change documentation style of xen command line.
 - Rename iommu_map to hwdom_iommu_map.
 - Move all the checks to hwdom_iommu_map.

Changes since v1:
 - Introduce a new reserved option instead of abusing the inclusive
   option.
 - Use the same helper function for PV and PVH in order to decide if a
   page should be added to the domain page tables.
 - Use the data inside of the domain struct to detect overlaps with
   emulated MMIO regions.
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
---
 docs/misc/xen-command-line.markdown |  9 
 xen/drivers/passthrough/amd/pci_amd_iommu.c |  3 ++
 xen/drivers/passthrough/arm/smmu.c  |  1 +
 xen/drivers/passthrough/iommu.c |  3 ++
 xen/drivers/passthrough/vtd/iommu.c |  3 ++
 xen/drivers/passthrough/x86/iommu.c | 50 ++---
 xen/include/xen/iommu.h |  2 +-
 7 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 11b11a017e..0601d3005d 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -702,6 +702,15 @@ This list of booleans control the iommu usage by Dom0:
   option is only applicable to a PV Dom0 and is enabled by default on Intel
   hardware.
 
+* `map-reserved`: sets up DMA remapping for all the reserved regions in the
+  memory map for Dom0. Use this to work around firmware issues providing
+  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
+  accesses for Dom0, all memory regions marked as reserved in the memory map
+  that don't overlap with any MMIO region from emulated devices will be
+  identity mapped. This option maps a subset of the memory that would be
+  mapped when using the `map-inclusive` option. This option is available to a
+  PVH Dom0 and is enabled by default on Intel hardware.
+
 ### dom0\_ioports\_disable (x86)
 > `= List of -`
 
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c 
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 27eb49619d..49d934e1ac 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -256,6 +256,9 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain 
*d)
 /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
 if ( iommu_hwdom_inclusive == -1 )
 iommu_hwdom_inclusive = false;
+/* Reserved IOMMU mappings are disabled by default on AMD hardware. */
+if ( iommu_hwdom_reserved == -1 )
+iommu_hwdom_reserved = false;
 
 if ( allocate_domain_resources(dom_iommu(d)) )
 BUG();
diff --git a/xen/drivers/passthrough/arm/smmu.c 
b/xen/drivers/passthrough/arm/smmu.c
index b142677b8c..8ea39659d1 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2729,6 +2729,7 @@ static void __hwdom_init arm_smmu_iommu_hwdom_init(struct 
domain *d)
 {
/* Set to false options not supported on ARM. */
iommu_hwdom_inclusive = false;
+   iommu_hwdom_reserved = false;
 }
 
 static void arm_smmu_iommu_domain_teardown(struct domain *d)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 4cc29eeb2c..c33ddb9ff7 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -62,6 +62,7 @@ bool_t __read_mostly iommu_intremap = 1;
 bool __hwdom_initdata iommu_hwdom_strict;
 bool __read_mostly iommu_hwdom_passthrough;
 int8_t __hwdom_initdata iommu_hwdom_inclusive = -1;
+int8_t __hwdom_initdata iommu_hwdom_reserved = -1;
 
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme
@@ -155,6 +156,8 @@ static int __init parse_dom0_iommu_param(const char *s)