[PATCH v13 0/3] iommu: add rmrr Xen command line option

[PATCH v13 0/3] iommu: add rmrr Xen command line option

[Venu Busireddy]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Add Xen command line option rmrr to specify RMRR regions that are not
defined in ACPI thus causing IO Page Faults and prevent dom0 from booting
if "iommu=dom0-strict" option is specified on the Xen command line.
These additional regions will be added to the list of RMRR regions parsed
from ACPI.

Changes in v13:
 - Implement feedback from Kevin Tian.
   https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html
   https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html
   https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html
 - Limit all source lines and comments to 80 characters per line.
 - Implement coding style suggestions from Konrad Wilk.
 - Changed the Author to Elena Ufimtseva <elena.ufimtseva@oracle.com>

Changes in v12:
 - Mostly cosmetic fixes from Jan's review on v11.

Changes in v11:
 - changed macro to print extra RMRR ranges and added argument macro;
 - fixed the overlapping check if condition error;
 - fixed the loop exit condition when checking pfn in RMRR region;

Elena Ufimtseva (3):
  iommu VT-d: separate rmrr addition function.
  pci: add wrapper for parse_pci.
  iommu: add rmrr Xen command line option for extra rmrrs

 docs/misc/xen-command-line.markdown |  13 ++
 xen/drivers/passthrough/vtd/dmar.c  | 324 +++++++++++++++++++++++++++++-------
 xen/drivers/pci/pci.c               |  11 ++
 xen/include/xen/pci.h               |   3 +
 4 files changed, 292 insertions(+), 59 deletions(-)


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

[PATCH v13 1/3] iommu VT-d: separate rmrr addition function.

[Venu Busireddy]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

In preparation for auxiliary RMRR data provided on Xen command line,
make RMRR adding a separate function.
Also free memery for rmrr device scope in error path.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: Venu Busireddy <venu.busireddy@oracle.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
 xen/drivers/passthrough/vtd/dmar.c | 123 ++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 58 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 08c1d2d..84bf63d 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -579,6 +579,68 @@ out:
     return ret;
 }
 
+static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
+{
+    bool ignore = false;
+    unsigned int i = 0;
+    int ret = 0;
+
+    /* Skip checking if segment is not accessible yet. */
+    if ( !pci_known_segment(rmrru->segment) )
+        i = UINT_MAX;
+
+    for ( ; i < rmrru->scope.devices_cnt; i++ )
+    {
+        u8 b = PCI_BUS(rmrru->scope.devices[i]);
+        u8 d = PCI_SLOT(rmrru->scope.devices[i]);
+        u8 f = PCI_FUNC(rmrru->scope.devices[i]);
+
+        if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
+        {
+            dprintk(XENLOG_WARNING VTDPREFIX,
+                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
+                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
+                    rmrru->segment, b, d, f,
+                    rmrru->base_address, rmrru->end_address);
+            ignore = true;
+        }
+        else
+        {
+            ignore = false;
+            break;
+        }
+    }
+
+    if ( ignore )
+    {
+        dprintk(XENLOG_WARNING VTDPREFIX,
+                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
+                "devices under its scope are not PCI discoverable!\n",
+                rmrru->base_address, rmrru->end_address);
+        scope_devices_free(&rmrru->scope);
+        xfree(rmrru);
+    }
+    else if ( rmrru->base_address > rmrru->end_address )
+    {
+        dprintk(XENLOG_WARNING VTDPREFIX,
+                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
+                rmrru->base_address, rmrru->end_address);
+        scope_devices_free(&rmrru->scope);
+        xfree(rmrru);
+        ret = -EFAULT;
+    }
+    else
+    {
+        if ( iommu_verbose )
+            dprintk(VTDPREFIX,
+                    "  RMRR region: base_addr %"PRIx64" end_addr %"PRIx64"\n",
+                    rmrru->base_address, rmrru->end_address);
+        acpi_register_rmrr_unit(rmrru);
+    }
+
+    return ret;
+}
+
 static int __init
 acpi_parse_one_rmrr(struct acpi_dmar_header *header)
 {
@@ -628,65 +690,10 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
     ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
                                &rmrru->scope, RMRR_TYPE, rmrr->segment);
 
-    if ( ret || (rmrru->scope.devices_cnt == 0) )
-        xfree(rmrru);
+    if ( !ret && (rmrru->scope.devices_cnt != 0) )
+        register_one_rmrr(rmrru);
     else
-    {
-        u8 b, d, f;
-        bool_t ignore = 0;
-        unsigned int i = 0;
-
-        /* Skip checking if segment is not accessible yet. */
-        if ( !pci_known_segment(rmrr->segment) )
-            i = UINT_MAX;
-
-        for ( ; i < rmrru->scope.devices_cnt; i++ )
-        {
-            b = PCI_BUS(rmrru->scope.devices[i]);
-            d = PCI_SLOT(rmrru->scope.devices[i]);
-            f = PCI_FUNC(rmrru->scope.devices[i]);
-
-            if ( !pci_device_detect(rmrr->segment, b, d, f) )
-            {
-                printk(XENLOG_WARNING VTDPREFIX
-                       " Non-existent device (%04x:%02x:%02x.%u) reported in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
-                       rmrr->segment, b, d, f,
-                       rmrru->base_address, rmrru->end_address);
-                ignore = 1;
-            }
-            else
-            {
-                ignore = 0;
-                break;
-            }
-        }
-
-        if ( ignore )
-        {
-            printk(XENLOG_WARNING VTDPREFIX
-                   "  Ignore RMRR (%"PRIx64", %"PRIx64") (some devices in its scope are not PCI discoverable)\n",
-                   rmrru->base_address, rmrru->end_address);
-            scope_devices_free(&rmrru->scope);
-            xfree(rmrru);
-        }
-        else if ( base_addr > end_addr )
-        {
-            printk(XENLOG_WARNING VTDPREFIX
-                   "  RMRR (%"PRIx64", %"PRIx64") is incorrect\n",
-                   rmrru->base_address, rmrru->end_address);
-            scope_devices_free(&rmrru->scope);
-            xfree(rmrru);
-            ret = -EFAULT;
-        }
-        else
-        {
-            if ( iommu_verbose )
-                printk(VTDPREFIX
-                       "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
-                       rmrru->base_address, rmrru->end_address);
-            acpi_register_rmrr_unit(rmrru);
-        }
-    }
+        xfree(rmrru);
 
     return ret;
 }

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

[PATCH v13 2/3] pci: add wrapper for parse_pci.

[Venu Busireddy]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

For sbdf's parsing in RMRR command line, add parse_pci_seg with additional
parameter def_seg. parse_pci_seg will help to identify if segment was
found in string being parsed or default segment was used.
Make a wrapper parse_pci so the rest of the callers are not affected.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: Venu Busireddy <venu.busireddy@oracle.com>
---
 xen/drivers/pci/pci.c | 11 +++++++++++
 xen/include/xen/pci.h |  3 +++
 2 files changed, 14 insertions(+)

 v13:
   - Renamed __parse_pci() to parse_pci_seg() as suggested by Konrad Wilk.

diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index ca07ed0..13d3309 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -119,11 +119,21 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p,
                              unsigned int *bus_p, unsigned int *dev_p,
                              unsigned int *func_p)
 {
+    bool def_seg;
+
+    return parse_pci_seg(s, seg_p, bus_p, dev_p, func_p, &def_seg);
+}
+
+const char *__init parse_pci_seg(const char *s, unsigned int *seg_p,
+                                 unsigned int *bus_p, unsigned int *dev_p,
+                                 unsigned int *func_p, bool *def_seg)
+{
     unsigned long seg = simple_strtoul(s, &s, 16), bus, dev, func;
 
     if ( *s != ':' )
         return NULL;
     bus = simple_strtoul(s + 1, &s, 16);
+    *def_seg = false;
     if ( *s == ':' )
         dev = simple_strtoul(s + 1, &s, 16);
     else
@@ -131,6 +141,7 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p,
         dev = bus;
         bus = seg;
         seg = 0;
+        *def_seg = true;
     }
     if ( func_p )
     {
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 0872401..59b6e8a 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -159,6 +159,9 @@ int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
 int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap);
 const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
                       unsigned int *dev, unsigned int *func);
+const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
+                          unsigned int *dev, unsigned int *func, bool *def_seg);
+
 
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
 

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

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

[Venu Busireddy]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

On some platforms firmware fails to specify RMRR regions in ACPI tables and
thus those regions will not be mapped in dom0 or guests and may cause IO
Page Faults and prevent dom0 from booting if "iommu=dom0-strict" option is
specified on the Xen command line.

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.

The above mentioned problems were discovered during the 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[,...]]
    For example, for Lenovo ThinkCentre M, use:
        rmrr=0xd5d45=0:0:1d.0;0xd5d46=0:0:1a.0
    If grub2 used and multiple ranges are specified, ';' should be
    quoted/escaped, refer to grub2 manual for more information.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: Venu Busireddy <venu.busireddy@oracle.com>
---
 docs/misc/xen-command-line.markdown |  13 +++
 xen/drivers/passthrough/vtd/dmar.c  | 201 +++++++++++++++++++++++++++++++++++-
 2 files changed, 213 insertions(+), 1 deletion(-)

Changes in v13:
 - Implement feedback from Kevin Tian.
   https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html
   https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html
   https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html
 - Limit all source lines and comments to 80 characters per line.
 - Implement coding style suggestions from Konrad Wilk.

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 0138978..a11fdf9 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1377,6 +1377,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
 > `= <boolean>`
 
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 84bf63d..23ba7a6 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -859,6 +859,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 user_rmrr {
+    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 user_rmrr user_rmrrs[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 int __init add_user_rmrr(void)
+{
+    struct acpi_rmrr_unit *rmrr, *rmrru;
+    unsigned int idx, seg, i;
+    unsigned long base, end;
+    bool overlap;
+
+    for ( i = 0; i < nr_rmrr; i++ )
+    {
+        base = user_rmrrs[i].base_pfn;
+        end = user_rmrrs[i].end_pfn;
+
+        if ( base > end )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "Invalid RMRR Range "ERMRRU_FMT"\n",
+                   ERMRRU_ARG(user_rmrrs[i]));
+            continue;
+        }
+
+        if ( (end - base) >= MAX_EXTRA_RMRR_PAGES )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "RMRR range "ERMRRU_FMT" exceeds "\
+                   __stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
+                   ERMRRU_ARG(user_rmrrs[i]));
+            continue;
+        }
+
+        overlap = false;
+        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+        {
+            if ( pfn_to_paddr(base) < rmrru->end_address &&
+                 rmrru->base_address < pfn_to_paddr(end + 1) )
+            {
+                printk(XENLOG_ERR VTDPREFIX
+                       "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
+                       ERMRRU_ARG(user_rmrrs[i]),
+                       paddr_to_pfn(rmrru->base_address),
+                       paddr_to_pfn(rmrru->end_address));
+                overlap = true;
+                break;
+            }
+        }
+        /* Don't add overlapping RMRR. */
+        if ( overlap )
+            continue;
+
+        do
+        {
+            if ( !mfn_valid(base) )
+            {
+                printk(XENLOG_ERR VTDPREFIX
+                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
+                       ERMRRU_ARG(user_rmrrs[i]));
+                break;
+            }
+        } while ( base++ < end );
+
+        /* Invalid pfn in range as the loop ended before end_pfn was reached. */
+        if ( base <= end )
+            continue;
+
+        rmrr = xzalloc(struct acpi_rmrr_unit);
+        if ( !rmrr )
+            return -ENOMEM;
+
+        rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count);
+        if ( !rmrr->scope.devices )
+        {
+            xfree(rmrr);
+            return -ENOMEM;
+        }
+
+        seg = 0;
+        for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ )
+        {
+            rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx];
+            seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]);
+        }
+        if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
+                   ERMRRU_ARG(user_rmrrs[i]));
+            scope_devices_free(&rmrr->scope);
+            xfree(rmrr);
+            continue;
+        }
+
+        rmrr->segment = seg;
+        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
+        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);
+        rmrr->scope.devices_cnt = user_rmrrs[i].dev_count;
+
+        if ( register_one_rmrr(rmrr) )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                   "Could not register RMMR range "ERMRRU_FMT"\n",
+                   ERMRRU_ARG(user_rmrrs[i]));
+            scope_devices_free(&rmrr->scope);
+            xfree(rmrr);
+        }
+    }
+    return 0;
+}
+
 #include <asm/tboot.h>
 /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */
 /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */
@@ -868,6 +994,7 @@ int __init acpi_dmar_init(void)
 {
     acpi_physical_address dmar_addr;
     acpi_native_uint dmar_len;
+    int ret;
 
     if ( ACPI_SUCCESS(acpi_get_table_phys(ACPI_SIG_DMAR, 0,
                                           &dmar_addr, &dmar_len)) )
@@ -878,7 +1005,12 @@ int __init acpi_dmar_init(void)
         dmar_table = __va(dmar_addr);
     }
 
-    return parse_dmar_table(acpi_parse_dmar);
+    ret = parse_dmar_table(acpi_parse_dmar);
+
+    if ( !ret )
+        return add_user_rmrr();
+
+    return ret;
 }
 
 void acpi_dmar_reinstate(void)
@@ -937,3 +1069,70 @@ int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
 
     return 0;
 }
+
+/*
+ * Parse rmrr Xen command line options and add parsed devices and regions into
+ * acpi_rmrr_unit list to mapped as RMRRs parsed from ACPI.
+ * Format:
+ * rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
+ * If the 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.
+ */
+static void __init parse_rmrr_param(const char *str)
+{
+    const char *s = str, *cur, *stmp;
+    unsigned int seg, bus, dev, func, dev_count;
+    unsigned long start, end;
+
+    do {
+        start = simple_strtoul(cur = s, &s, 0);
+        if ( cur == s )
+            break;
+
+        if ( *s == '-' )
+        {
+            end = simple_strtoul(cur = s + 1, &s, 0);
+            if ( cur == s )
+                break;
+        }
+        else
+            end = start;
+
+        user_rmrrs[nr_rmrr].base_pfn = start;
+        user_rmrrs[nr_rmrr].end_pfn = end;
+
+        if ( *s != '=' )
+            continue;
+
+        do {
+            bool def_seg = false;
+
+            stmp = parse_pci_seg(s + 1, &seg, &bus, &dev, &func, &def_seg);
+            if ( !stmp )
+                break;
+
+            /*
+             * Not specified.
+             * Segment will be replaced with one from first device.
+             */
+            if ( user_rmrrs[nr_rmrr].dev_count && def_seg )
+                seg = PCI_SEG(user_rmrrs[nr_rmrr].sbdf[0]);
+
+            /* Keep sbdf's even if they differ and later report an error. */
+            dev_count = user_rmrrs[nr_rmrr].dev_count;
+            user_rmrrs[nr_rmrr].sbdf[dev_count] = PCI_SBDF(seg, bus, dev, func);
+
+            user_rmrrs[nr_rmrr].dev_count++;
+            s = stmp;
+        } while ( *s == ',' &&
+                  user_rmrrs[nr_rmrr].dev_count < MAX_EXTRA_RMRR_DEV );
+
+        if ( user_rmrrs[nr_rmrr].dev_count )
+            nr_rmrr++;
+
+    } while ( *s++ == ';' && nr_rmrr < MAX_EXTRA_RMRR );
+}
+custom_param("rmrr", parse_rmrr_param);

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

Re: [PATCH v13 0/3] iommu: add rmrr Xen command line option

[Tian, Kevin]

> From: Venu Busireddy
> Sent: Wednesday, January 11, 2017 6:58 AM
> 
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> Add Xen command line option rmrr to specify RMRR regions that are not
> defined in ACPI thus causing IO Page Faults and prevent dom0 from booting
> if "iommu=dom0-strict" option is specified on the Xen command line.
> These additional regions will be added to the list of RMRR regions parsed
> from ACPI.
> 
> Changes in v13:
>  - Implement feedback from Kevin Tian.
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html

Looks I gave my ack/review to all three patches. But you didn't put my acked-by
in patch [3/3]. Is there substantial change against v12 which requires my further
review?

>  - Limit all source lines and comments to 80 characters per line.
>  - Implement coding style suggestions from Konrad Wilk.
>  - Changed the Author to Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> Changes in v12:
>  - Mostly cosmetic fixes from Jan's review on v11.
> 
> Changes in v11:
>  - changed macro to print extra RMRR ranges and added argument macro;
>  - fixed the overlapping check if condition error;
>  - fixed the loop exit condition when checking pfn in RMRR region;
> 
> Elena Ufimtseva (3):
>   iommu VT-d: separate rmrr addition function.
>   pci: add wrapper for parse_pci.
>   iommu: add rmrr Xen command line option for extra rmrrs
> 
>  docs/misc/xen-command-line.markdown |  13 ++
>  xen/drivers/passthrough/vtd/dmar.c  | 324
> +++++++++++++++++++++++++++++-------
>  xen/drivers/pci/pci.c               |  11 ++
>  xen/include/xen/pci.h               |   3 +
>  4 files changed, 292 insertions(+), 59 deletions(-)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v13 1/3] iommu VT-d: separate rmrr addition function.

[Jan Beulich]

>>> On 10.01.17 at 23:57, <venu.busireddy@oracle.com> wrote:
> @@ -628,65 +690,10 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>      ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
>                                 &rmrru->scope, RMRR_TYPE, rmrr->segment);
>  
> -    if ( ret || (rmrru->scope.devices_cnt == 0) )
> -        xfree(rmrru);
> +    if ( !ret && (rmrru->scope.devices_cnt != 0) )
> +        register_one_rmrr(rmrru);

You've lost the return value here.

Jan


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

Re: [PATCH v13 0/3] iommu: add rmrr Xen command line option

[Venu Busireddy]

On Wed, Jan 11, 2017 at 05:55:58AM +0000, Tian, Kevin wrote:
> > From: Venu Busireddy
> > Sent: Wednesday, January 11, 2017 6:58 AM
> > 
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > Add Xen command line option rmrr to specify RMRR regions that are not
> > defined in ACPI thus causing IO Page Faults and prevent dom0 from booting
> > if "iommu=dom0-strict" option is specified on the Xen command line.
> > These additional regions will be added to the list of RMRR regions parsed
> > from ACPI.
> > 
> > Changes in v13:
> >  - Implement feedback from Kevin Tian.
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html
> 
> Looks I gave my ack/review to all three patches. But you didn't put my acked-by
> in patch [3/3]. Is there substantial change against v12 which requires my further
> review?

Functionally, nothing changed. But quite a few changes (coding style,
renaming of structures and variables, introduction of new variables to
make code more readable, and such) are made, which changed the
appearance of the code. As a result, a suggestion was made to remove
your "Acked-by", so that you could review the new format. If you are
fine with syntactic changes, you don't need to review it again. 

Regards,

Venu

> 
> >  - Limit all source lines and comments to 80 characters per line.
> >  - Implement coding style suggestions from Konrad Wilk.
> >  - Changed the Author to Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > Changes in v12:
> >  - Mostly cosmetic fixes from Jan's review on v11.
> > 
> > Changes in v11:
> >  - changed macro to print extra RMRR ranges and added argument macro;
> >  - fixed the overlapping check if condition error;
> >  - fixed the loop exit condition when checking pfn in RMRR region;
> > 
> > Elena Ufimtseva (3):
> >   iommu VT-d: separate rmrr addition function.
> >   pci: add wrapper for parse_pci.
> >   iommu: add rmrr Xen command line option for extra rmrrs
> > 
> >  docs/misc/xen-command-line.markdown |  13 ++
> >  xen/drivers/passthrough/vtd/dmar.c  | 324
> > +++++++++++++++++++++++++++++-------
> >  xen/drivers/pci/pci.c               |  11 ++
> >  xen/include/xen/pci.h               |   3 +
> >  4 files changed, 292 insertions(+), 59 deletions(-)
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel

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

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

[Jan Beulich]

>>> On 10.01.17 at 23:57, <venu.busireddy@oracle.com> wrote:
> Changes in v13:
>  - Implement feedback from Kevin Tian.
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html 
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html 
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html 

Any reason some of the review comments I had given were left
un-addressed? I'll reproduce them in quotes below.

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -859,6 +859,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

So you've kept "extra" in these, but ...

> +struct user_rmrr {

... switched to "user" here and below. Please be consistent.

> +static int __init add_user_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrr, *rmrru;
> +    unsigned int idx, seg, i;
> +    unsigned long base, end;
> +    bool overlap;
> +
> +    for ( i = 0; i < nr_rmrr; i++ )
> +    {
> +        base = user_rmrrs[i].base_pfn;
> +        end = user_rmrrs[i].end_pfn;
> +
> +        if ( base > end )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Invalid RMRR Range "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            continue;
> +        }
> +
> +        if ( (end - base) >= MAX_EXTRA_RMRR_PAGES )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range "ERMRRU_FMT" exceeds "\
> +                   __stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            continue;
> +        }
> +
> +        overlap = false;
> +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> +        {
> +            if ( pfn_to_paddr(base) < rmrru->end_address &&
> +                 rmrru->base_address < pfn_to_paddr(end + 1) )

"Aren't both ranges inclusive? I.e. shouldn't the first one be <= (and
 the second one could be <= too when dropping the +1), matching
 the check acpi_parse_one_rmrr() does?"

> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
> +                       ERMRRU_ARG(user_rmrrs[i]),
> +                       paddr_to_pfn(rmrru->base_address),
> +                       paddr_to_pfn(rmrru->end_address));
> +                overlap = true;
> +                break;
> +            }
> +        }
> +        /* Don't add overlapping RMRR. */
> +        if ( overlap )
> +            continue;
> +
> +        do
> +        {
> +            if ( !mfn_valid(base) )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> +                       ERMRRU_ARG(user_rmrrs[i]));
> +                break;
> +            }
> +        } while ( base++ < end );
> +
> +        /* Invalid pfn in range as the loop ended before end_pfn was reached. */
> +        if ( base <= end )
> +            continue;
> +
> +        rmrr = xzalloc(struct acpi_rmrr_unit);
> +        if ( !rmrr )
> +            return -ENOMEM;
> +
> +        rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count);
> +        if ( !rmrr->scope.devices )
> +        {
> +            xfree(rmrr);
> +            return -ENOMEM;
> +        }
> +
> +        seg = 0;
> +        for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ )
> +        {
> +            rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx];
> +            seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]);
> +        }
> +        if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            scope_devices_free(&rmrr->scope);
> +            xfree(rmrr);
> +            continue;
> +        }
> +
> +        rmrr->segment = seg;
> +        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
> +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);

"And this seems wrong too, unless I'm mistaken with the inclusive-ness."

> +        rmrr->scope.devices_cnt = user_rmrrs[i].dev_count;
> +
> +        if ( register_one_rmrr(rmrr) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Could not register RMMR range "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            scope_devices_free(&rmrr->scope);
> +            xfree(rmrr);
> +        }
> +    }
> +    return 0;

Blank line please before a function's final return statement.

Jan

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

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

[Elena Ufimtseva]

On Thu, Jan 12, 2017 at 04:44:42AM -0700, Jan Beulich wrote:
> >>> On 10.01.17 at 23:57, <venu.busireddy@oracle.com> wrote:
> > Changes in v13:
> >  - Implement feedback from Kevin Tian.
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html 
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html 
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html 
> 
> Any reason some of the review comments I had given were left
> un-addressed? I'll reproduce them in quotes below.
>

Hi Jan

Thanks for reminding!
That was my fault that I did not tell this to Venu when transferring
this patchset to him.
 
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -859,6 +859,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
> 
> So you've kept "extra" in these, but ...
> 
> > +struct user_rmrr {
> 
> ... switched to "user" here and below. Please be consistent.
> 
> > +static int __init add_user_rmrr(void)
> > +{
> > +    struct acpi_rmrr_unit *rmrr, *rmrru;
> > +    unsigned int idx, seg, i;
> > +    unsigned long base, end;
> > +    bool overlap;
> > +
> > +    for ( i = 0; i < nr_rmrr; i++ )
> > +    {
> > +        base = user_rmrrs[i].base_pfn;
> > +        end = user_rmrrs[i].end_pfn;
> > +
> > +        if ( base > end )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Invalid RMRR Range "ERMRRU_FMT"\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            continue;
> > +        }
> > +
> > +        if ( (end - base) >= MAX_EXTRA_RMRR_PAGES )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "RMRR range "ERMRRU_FMT" exceeds "\
> > +                   __stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            continue;
> > +        }
> > +
> > +        overlap = false;
> > +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> > +        {
> > +            if ( pfn_to_paddr(base) < rmrru->end_address &&
> > +                 rmrru->base_address < pfn_to_paddr(end + 1) )
> 
> "Aren't both ranges inclusive? I.e. shouldn't the first one be <= (and
>  the second one could be <= too when dropping the +1), matching
>  the check acpi_parse_one_rmrr() does?"

I agree. The ranges in acpu_rmrr_units and user_rmrrs are inclusive.
If this is fixed, then there is another part where I am not sure what
would be the better way to fix this. If fix is needed.

I am looking at rmrr_identity_mapping where the RMRR paddr get converted
to pfn and then mapped with iommu.
If ( rmrr->end_address & ~PAGE_SHIFT_MASK_4K ) == 0, the while loop
    while ( base_pfn < end_pfn )
 will not map that inclusive end_address of rmrr.
Does it seem wrong?


> 
> > +            {
> > +                printk(XENLOG_ERR VTDPREFIX
> > +                       "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
> > +                       ERMRRU_ARG(user_rmrrs[i]),
> > +                       paddr_to_pfn(rmrru->base_address),
> > +                       paddr_to_pfn(rmrru->end_address));
> > +                overlap = true;
> > +                break;
> > +            }
> > +        }
> > +        /* Don't add overlapping RMRR. */
> > +        if ( overlap )
> > +            continue;
> > +
> > +        do
> > +        {
> > +            if ( !mfn_valid(base) )
> > +            {
> > +                printk(XENLOG_ERR VTDPREFIX
> > +                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> > +                       ERMRRU_ARG(user_rmrrs[i]));
> > +                break;
> > +            }
> > +        } while ( base++ < end );
> > +
> > +        /* Invalid pfn in range as the loop ended before end_pfn was reached. */
> > +        if ( base <= end )
> > +            continue;
> > +
> > +        rmrr = xzalloc(struct acpi_rmrr_unit);
> > +        if ( !rmrr )
> > +            return -ENOMEM;
> > +
> > +        rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count);
> > +        if ( !rmrr->scope.devices )
> > +        {
> > +            xfree(rmrr);
> > +            return -ENOMEM;
> > +        }
> > +
> > +        seg = 0;
> > +        for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ )
> > +        {
> > +            rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx];
> > +            seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]);
> > +        }
> > +        if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            scope_devices_free(&rmrr->scope);
> > +            xfree(rmrr);
> > +            continue;
> > +        }
> > +
> > +        rmrr->segment = seg;
> > +        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
> > +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);
> 
> "And this seems wrong too, unless I'm mistaken with the inclusive-ness."
> 
This one is the avoidance of the while loop mapping in
rmrr_identity_mapping.

> > +        rmrr->scope.devices_cnt = user_rmrrs[i].dev_count;
> > +
> > +        if ( register_one_rmrr(rmrr) )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Could not register RMMR range "ERMRRU_FMT"\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            scope_devices_free(&rmrr->scope);
> > +            xfree(rmrr);
> > +        }
> > +    }
> > +    return 0;
> 
> Blank line please before a function's final return statement.
> 
> Jan

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

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

[Jan Beulich]

>>> On 18.01.17 at 20:56, <elena.ufimtseva@oracle.com> wrote:
> I am looking at rmrr_identity_mapping where the RMRR paddr get converted
> to pfn and then mapped with iommu.
> If ( rmrr->end_address & ~PAGE_SHIFT_MASK_4K ) == 0, the while loop
>     while ( base_pfn < end_pfn )
>  will not map that inclusive end_address of rmrr.
> Does it seem wrong?

I don't think so, no. end_pfn is being calculated using
PAGE_ALIGN_4K(), i.e. rounding up.

>> > +        rmrr->segment = seg;
>> > +        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
>> > +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);
>> 
>> "And this seems wrong too, unless I'm mistaken with the inclusive-ness."
>> 
> This one is the avoidance of the while loop mapping in
> rmrr_identity_mapping.

Well, that's the purpose you describe, but the comment was about
the calculation itself, which I think is lacking a "- 1", but even better
would be - for avoiding boundary issues -

        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) | ~PAGE_MASK;

or some such.

Jan


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

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

[Elena Ufimtseva]

On Thu, Jan 19, 2017 at 01:29:15AM -0700, Jan Beulich wrote:
> >>> On 18.01.17 at 20:56, <elena.ufimtseva@oracle.com> wrote:
> > I am looking at rmrr_identity_mapping where the RMRR paddr get converted
> > to pfn and then mapped with iommu.
> > If ( rmrr->end_address & ~PAGE_SHIFT_MASK_4K ) == 0, the while loop
> >     while ( base_pfn < end_pfn )
> >  will not map that inclusive end_address of rmrr.
> > Does it seem wrong?
> 
> I don't think so, no. end_pfn is being calculated using
> PAGE_ALIGN_4K(), i.e. rounding up.

I mean to say, if the end address is already aligned, then the page wont
be mapped.
For example, if end paddr is 0x00000000000ed000, end_pfn will be
0x00000000000ed and wont be mapped in the loop
while ( base_pfn < end_pfn ).
And we will have mapped RMRR end address saved in arch.mapped_rmrrs
as 0x00000000000ed000.
Looks like parsed ACPI RMRR end addresses are extended to end of the
page though. Not sure if there is somewhere same boundary alignment in
code similar to what you proposed below.

> 
> >> > +        rmrr->segment = seg;
> >> > +        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
> >> > +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);
> >> 
> >> "And this seems wrong too, unless I'm mistaken with the inclusive-ness."
> >> 
> > This one is the avoidance of the while loop mapping in
> > rmrr_identity_mapping.
> 
> Well, that's the purpose you describe, but the comment was about
> the calculation itself, which I think is lacking a "- 1", but even better
> would be - for avoiding boundary issues -
> 
>         rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) | ~PAGE_MASK;

Yes, this will eliminate this problem. This will need to be accounted
for in overlapping condition as well.

> 
> or some such.
> 
> Jan
> 

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

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

[Jan Beulich]

>>> On 19.01.17 at 18:44, <elena.ufimtseva@oracle.com> wrote:
> On Thu, Jan 19, 2017 at 01:29:15AM -0700, Jan Beulich wrote:
>> >>> On 18.01.17 at 20:56, <elena.ufimtseva@oracle.com> wrote:
>> > I am looking at rmrr_identity_mapping where the RMRR paddr get converted
>> > to pfn and then mapped with iommu.
>> > If ( rmrr->end_address & ~PAGE_SHIFT_MASK_4K ) == 0, the while loop
>> >     while ( base_pfn < end_pfn )
>> >  will not map that inclusive end_address of rmrr.
>> > Does it seem wrong?
>> 
>> I don't think so, no. end_pfn is being calculated using
>> PAGE_ALIGN_4K(), i.e. rounding up.
> 
> I mean to say, if the end address is already aligned, then the page wont
> be mapped.

Ah, that would be a problem, but would only affect systems with
non-spec compliant firmware, as the doc says: "The reserved
memory region size (Limit - Base + 1) must be an integer multiple of
4KB", along with requiring the base address to be 4k-aligned.

Jan


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