Re: [Xen-devel] [PATCH v11 3/3] iommu: add rmrr Xen command line option for extra rmrrs

[Elena Ufimtseva]

On Mon, Oct 26, 2015 at 07:38:06AM -0600, Jan Beulich wrote:
> >>> On 22.10.15 at 19:13,  wrote:
> > From: Elena Ufimtseva 
> > 
> > On some platforms RMRR regions may be not specified in ACPI and thus will 
> > not
> > be mapped 1:1 in dom0.
>

Thanks Jan for review.

> I think this may be misleading to readers: It sounds as if there was
> the option for RMRRs to not be specified in ACPI tables, while in
> fact this is a firmware bug. How about "On some platforms firmware
> fails to specify RMRR regions may in ACPI tables, and thus those
> regions will not be mapped in dom0 or guests the respective device(s)
> get passed through to"?
>
Agree, makes more sense.

> > +static void __init add_extra_rmrr(void)
> > +{
> > +struct acpi_rmrr_unit *acpi_rmrr;
> > +struct acpi_rmrr_unit *rmrru;
> > +unsigned int dev, seg, i;
> > +unsigned long pfn;
> > +bool_t overlap;
> > +
> > +for ( i = 0; i < nr_rmrr; i++ )
> > +{
> > +if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
> > +{
> > +printk(XENLOG_ERR VTDPREFIX
> > +   "Invalid RMRR Range "ERMRRU_FMT"\n",
> > +   ERMRRU_ARG(extra_rmrr_units[i]));
> > +continue;
> > +}
> > +
> > +if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
> > + MAX_EXTRA_RMRR_PAGES )
> > +{
> > +printk(XENLOG_ERR VTDPREFIX
> > +   "RMRR range "ERMRRU_FMT" exceeds 
> > "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> > +   ERMRRU_ARG(extra_rmrr_units[i]));
> > +continue;
> > +}
> > +
> > +overlap = 0;
> > +list_for_each_entry(rmrru, _rmrr_units, list)
> > +{
> > +if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn ) < 
> > rmrru->end_address &&
> 
> Stray blank inside the inner parentheses.
> 
> > + rmrru->base_address < 
> > pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1) )
> > +{
> > +printk(XENLOG_ERR VTDPREFIX
> > +   "Overlapping RMRRs: "ERMRRU_FMT" and [%lx - %lx]\n",
> 
> ERMRRU_FMT doesn't have any blanks inside the square brackets,
> so I'd suggest the other format to nt have them either.
> 
> > +   ERMRRU_ARG(extra_rmrr_units[i]),
> > +   paddr_to_pfn(rmrru->base_address),
> > +   paddr_to_pfn(rmrru->end_address));
> > +overlap = 1;
> > +break;
> > +}
> > +}
> > +/* Dont add overlapping RMRR */
> 
> "Don't" and missing full stop.
> 
> > +if ( overlap )
> > +continue;
> > +
> > +pfn = extra_rmrr_units[i].base_pfn;
> > +do
> > +{
> > +if ( !mfn_valid(pfn) || (pfn >> (paddr_bits - PAGE_SHIFT)) )
> 
> Actually I think the right side is redundant with the max_pfn check
> mfn_valid() does.
> 
> > +{
> > +printk(XENLOG_ERR VTDPREFIX
> > +   "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> > +   ERMRRU_ARG(extra_rmrr_units[i]));
> > +break;
> 
> Wrong indentation.
> 
> > +}
> > +
> > +} while ( pfn++ < extra_rmrr_units[i].end_pfn );
> 
> Stray blank line before the end of the do/while body.
> 
> > +
> > +/* Invalid pfn in range as the loop ended before end_pfn was 
> > reached. */
> > +if ( pfn <= extra_rmrr_units[i].end_pfn )
> > +continue;
> > +
> > +acpi_rmrr = xzalloc(struct acpi_rmrr_unit);
> > +if ( !acpi_rmrr )
> > +return;
> > +
> > +acpi_rmrr->scope.devices = xmalloc_array(u16,
> > + 
> > extra_rmrr_units[i].dev_count);
> > +if ( !acpi_rmrr->scope.devices )
> > +{
> > +xfree(acpi_rmrr);
> > +return;
> > +}
> > +
> > +seg = 0;
> > +for ( dev = 0; dev < extra_rmrr_units[i].dev_count; dev++ )
> > +{
> > +acpi_rmrr->scope.devices[dev] = extra_rmrr_units[i].sbdf[dev];
> > +seg = seg | PCI_SEG(extra_rmrr_units[i].sbdf[dev]);
> 
> |=
> 
> > +static void __init parse_rmrr_param(const char *str)
> > +{
> > +const char *s = str, *cur, *stmp;
> > +unsigned int seg, bus, dev, func;
> > +unsigned long start, end;
> > +
> > +do {
> > +start = simple_strtoul(cur = s, , 0);
> > +if ( cur == s )
> > +break;
> > +
> > +if ( *s == '-' )
> > +{
> > +end = simple_strtoul(cur = s + 1, , 0);
> > +if ( cur == s )
> > +break;
> > +}
> > +else
> > +end = start;
> > +
> > +extra_rmrr_units[nr_rmrr].base_pfn = start;
> > +extra_rmrr_units[nr_rmrr].end_pfn = end;
> > +extra_rmrr_units[nr_rmrr].dev_count = 0;
> 
> 

Re: [Xen-devel] [PATCH v11 3/3] iommu: add rmrr Xen command line option for extra rmrrs

[Jan Beulich]

>>> On 22.10.15 at 19:13,  wrote:
> From: Elena Ufimtseva 
> 
> On some platforms RMRR regions may be not specified in ACPI and thus will not
> be mapped 1:1 in dom0.

I think this may be misleading to readers: It sounds as if there was
the option for RMRRs to not be specified in ACPI tables, while in
fact this is a firmware bug. How about "On some platforms firmware
fails to specify RMRR regions may in ACPI tables, and thus those
regions will not be mapped in dom0 or guests the respective device(s)
get passed through to"?

> +static void __init add_extra_rmrr(void)
> +{
> +struct acpi_rmrr_unit *acpi_rmrr;
> +struct acpi_rmrr_unit *rmrru;
> +unsigned int dev, seg, i;
> +unsigned long pfn;
> +bool_t overlap;
> +
> +for ( i = 0; i < nr_rmrr; i++ )
> +{
> +if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
> +{
> +printk(XENLOG_ERR VTDPREFIX
> +   "Invalid RMRR Range "ERMRRU_FMT"\n",
> +   ERMRRU_ARG(extra_rmrr_units[i]));
> +continue;
> +}
> +
> +if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
> + MAX_EXTRA_RMRR_PAGES )
> +{
> +printk(XENLOG_ERR VTDPREFIX
> +   "RMRR range "ERMRRU_FMT" exceeds 
> "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> +   ERMRRU_ARG(extra_rmrr_units[i]));
> +continue;
> +}
> +
> +overlap = 0;
> +list_for_each_entry(rmrru, _rmrr_units, list)
> +{
> +if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn ) < 
> rmrru->end_address &&

Stray blank inside the inner parentheses.

> + rmrru->base_address < 
> pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1) )
> +{
> +printk(XENLOG_ERR VTDPREFIX
> +   "Overlapping RMRRs: "ERMRRU_FMT" and [%lx - %lx]\n",

ERMRRU_FMT doesn't have any blanks inside the square brackets,
so I'd suggest the other format to nt have them either.

> +   ERMRRU_ARG(extra_rmrr_units[i]),
> +   paddr_to_pfn(rmrru->base_address),
> +   paddr_to_pfn(rmrru->end_address));
> +overlap = 1;
> +break;
> +}
> +}
> +/* Dont add overlapping RMRR */

"Don't" and missing full stop.

> +if ( overlap )
> +continue;
> +
> +pfn = extra_rmrr_units[i].base_pfn;
> +do
> +{
> +if ( !mfn_valid(pfn) || (pfn >> (paddr_bits - PAGE_SHIFT)) )

Actually I think the right side is redundant with the max_pfn check
mfn_valid() does.

> +{
> +printk(XENLOG_ERR VTDPREFIX
> +   "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> +   ERMRRU_ARG(extra_rmrr_units[i]));
> +break;

Wrong indentation.

> +}
> +
> +} while ( pfn++ < extra_rmrr_units[i].end_pfn );

Stray blank line before the end of the do/while body.

> +
> +/* Invalid pfn in range as the loop ended before end_pfn was 
> reached. */
> +if ( pfn <= extra_rmrr_units[i].end_pfn )
> +continue;
> +
> +acpi_rmrr = xzalloc(struct acpi_rmrr_unit);
> +if ( !acpi_rmrr )
> +return;
> +
> +acpi_rmrr->scope.devices = xmalloc_array(u16,
> + 
> extra_rmrr_units[i].dev_count);
> +if ( !acpi_rmrr->scope.devices )
> +{
> +xfree(acpi_rmrr);
> +return;
> +}
> +
> +seg = 0;
> +for ( dev = 0; dev < extra_rmrr_units[i].dev_count; dev++ )
> +{
> +acpi_rmrr->scope.devices[dev] = extra_rmrr_units[i].sbdf[dev];
> +seg = seg | PCI_SEG(extra_rmrr_units[i].sbdf[dev]);

|=

> +static void __init parse_rmrr_param(const char *str)
> +{
> +const char *s = str, *cur, *stmp;
> +unsigned int seg, bus, dev, func;
> +unsigned long start, end;
> +
> +do {
> +start = simple_strtoul(cur = s, , 0);
> +if ( cur == s )
> +break;
> +
> +if ( *s == '-' )
> +{
> +end = simple_strtoul(cur = s + 1, , 0);
> +if ( cur == s )
> +break;
> +}
> +else
> +end = start;
> +
> +extra_rmrr_units[nr_rmrr].base_pfn = start;
> +extra_rmrr_units[nr_rmrr].end_pfn = end;
> +extra_rmrr_units[nr_rmrr].dev_count = 0;

The last assignment isn't really needed.

> +if ( *s != '=' )
> +continue;
> +
> +do {
> +bool_t default_segment = 0;
> +
> +if ( *s == ';' )
> +break;

Can this if() ever be true? *s == '=' on the first iteration, and *s == ','
on any subsequent one afaics.

> +stmp = __parse_pci(s + 1, , , , , 
> _segment);
> +if 

[Xen-devel] [PATCH v11 3/3] iommu: add rmrr Xen command line option for extra rmrrs

[elena . ufimtseva]

From: Elena Ufimtseva 

On some platforms RMRR regions may be not specified in ACPI and thus will not
be mapped 1:1 in dom0. This causes IO Page Faults and prevents dom0 from booting
in PVH mode. New Xen command line option rmrr allows to specify such devices and
memory regions. These regions are added to the list of RMRR defined in ACPI if
the device is present in system. As a result, additional RMRRs will be mapped 
1:1 in dom0 with correct permissions.

Mentioned above problems were discovered during PVH work with ThinkCentre M
and Dell 5600T. No official documentation was found so far in regards to what
devices and why cause this. Experiments show that ThinkCentre M USB devices
with enabled debug port generate DMA read transactions to the regions of
memory marked reserved in host e820 map.

For Dell 5600T the device and faulting addresses are not found yet.
For detailed history of the discussion please check following threads:
http://lists.Xen.org/archives/html/xen-devel/2015-02/msg01724.html
http://lists.Xen.org/archives/html/xen-devel/2015-01/msg02513.html

Format for rmrr Xen command line option:
rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
If grub2 used and multiple ranges are specified, ';' should be
quoted/escaped, refer to grub2 manual for more information.

Signed-off-by: Elena Ufimtseva 
---
 docs/misc/xen-command-line.markdown |  13 +++
 xen/drivers/passthrough/vtd/dmar.c  | 196 +++-
 2 files changed, 208 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 416e559..92c69ea 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1240,6 +1240,19 @@ Specify the host reboot method.
 'efi' instructs Xen to reboot using the EFI reboot call (in EFI mode by
  default it will use that method first).
 
+### rmrr
+> '= 
start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
+
+Define RMRR units that are missing from ACPI table along with device they
+belong to and use them for 1:1 mapping. End addresses can be omitted and one
+page will be mapped. The ranges are inclusive when start and end are specified.
+If segment of the first device is not specified, segment zero will be used.
+If other segments are not specified, first device segment will be used.
+If a segment is specified for other than the first device and it does not match
+the one specified for the first one, an error will be reported.
+Note: grub2 requires to escape or use quotations if special characters are 
used,
+namely ';', refer to the grub2 documentation if multiple ranges are specified.
+
 ### ro-hpet
 > `= `
 
diff --git a/xen/drivers/passthrough/vtd/dmar.c 
b/xen/drivers/passthrough/vtd/dmar.c
index ced3239..8cbed88 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -867,6 +867,132 @@ out:
 return ret;
 }
 
+#define MAX_EXTRA_RMRR_PAGES 16
+#define MAX_EXTRA_RMRR 10
+
+/* RMRR units derived from command line rmrr option. */
+#define MAX_EXTRA_RMRR_DEV 20
+struct extra_rmrr_unit {
+struct list_head list;
+unsigned long base_pfn, end_pfn;
+unsigned int dev_count;
+u32 sbdf[MAX_EXTRA_RMRR_DEV];
+};
+
+static __initdata unsigned int nr_rmrr;
+static struct __initdata extra_rmrr_unit extra_rmrr_units[MAX_EXTRA_RMRR];
+
+/* Macro for RMRR inclusive range formatting. */
+#define ERMRRU_FMT "[%lx-%lx]"
+#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn
+
+static void __init add_extra_rmrr(void)
+{
+struct acpi_rmrr_unit *acpi_rmrr;
+struct acpi_rmrr_unit *rmrru;
+unsigned int dev, seg, i;
+unsigned long pfn;
+bool_t overlap;
+
+for ( i = 0; i < nr_rmrr; i++ )
+{
+if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
+{
+printk(XENLOG_ERR VTDPREFIX
+   "Invalid RMRR Range "ERMRRU_FMT"\n",
+   ERMRRU_ARG(extra_rmrr_units[i]));
+continue;
+}
+
+if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
+ MAX_EXTRA_RMRR_PAGES )
+{
+printk(XENLOG_ERR VTDPREFIX
+   "RMRR range "ERMRRU_FMT" exceeds 
"__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
+   ERMRRU_ARG(extra_rmrr_units[i]));
+continue;
+}
+
+overlap = 0;
+list_for_each_entry(rmrru, _rmrr_units, list)
+{
+if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn ) < 
rmrru->end_address &&
+ rmrru->base_address < 
pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1) )
+{
+printk(XENLOG_ERR VTDPREFIX
+   "Overlapping RMRRs: "ERMRRU_FMT" and [%lx - %lx]\n",
+   ERMRRU_ARG(extra_rmrr_units[i]),
+   paddr_to_pfn(rmrru->base_address),
+