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

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

[elena.ufimtseva]

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

Add Xen command line option rmrr to specify RMRR
regions for devices that are not defined in ACPI thus
causing IO Page Fault while booting dom0 in PVH mode.
These additional regions will be added to the list of
RMRR regions parsed from ACPI.

Elena Ufimtseva (2):
  iommu VT-d: separate rmrr addition function
  iommu: add rmrr Xen command line option for misc rmrrs

 docs/misc/xen-command-line.markdown |    7 ++
 xen/drivers/passthrough/iommu.c     |   86 ++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/dmar.c  |  122 +++++++++++++++++++----------------
 xen/drivers/passthrough/vtd/dmar.h  |    1 +
 xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++
 xen/drivers/passthrough/vtd/iommu.h |    1 +
 xen/include/xen/iommu.h             |    9 +++
 7 files changed, 202 insertions(+), 57 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] iommu VT-d: separate rmrr addition function

[elena.ufimtseva]

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

In preparation for auxiliary RMRR data provided on Xen
command line, make RMRR adding a separate function.
No code changes.

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 xen/drivers/passthrough/vtd/dmar.c |  122 +++++++++++++++++++-----------------
 xen/drivers/passthrough/vtd/dmar.h |    1 +
 2 files changed, 66 insertions(+), 57 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..4c85732 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -567,6 +567,66 @@ out:
     return ret;
 }
 
+int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
+{
+    u8 b, d, f;
+    bool_t ignore = 0;
+    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++ )
+    {
+        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(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 = 1;
+        }
+        else
+        {
+            ignore = 0;
+            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);
+        ret = -EFAULT;
+    }
+    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);
+        ret = -EFAULT;
+    }
+    else
+    {
+        if ( iommu_verbose )
+            dprintk(VTDPREFIX,
+                    "  RMRR region: base_addr %"PRIx64
+                    " end_address %"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)
 {
@@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
                                &rmrru->scope, RMRR_TYPE, rmrr->segment);
 
     if ( ret || (rmrru->scope.devices_cnt == 0) )
-        xfree(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) == 0 )
-            {
-                dprintk(XENLOG_WARNING VTDPREFIX,
-                        " Non-existent device (%04x:%02x:%02x.%u) is 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 )
-        {
-            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);
-            xfree(rmrru);
-        }
-        else if ( base_addr > end_addr )
-        {
-            dprintk(XENLOG_WARNING VTDPREFIX,
-                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
-                rmrru->base_address, rmrru->end_address);
-            xfree(rmrru);
-            ret = -EFAULT;
-        }
-        else
-        {
-            if ( iommu_verbose )
-                dprintk(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;
     }
 
+    ret = register_one_rmrr(rmrru);
+    if ( ret )
+        xfree(rmrru);
     return ret;
 }
 
diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
index af1feef..060aa04 100644
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -131,5 +131,6 @@ int vtd_hw_check(void);
 void disable_pmr(struct iommu *iommu);
 int is_usb_device(u16 seg, u8 bus, u8 devfn);
 int is_igd_drhd(struct acpi_drhd_unit *drhd);
+int register_one_rmrr(struct acpi_rmrr_unit *rmrru);
 
 #endif /* _DMAR_H_ */
-- 
1.7.10.4

Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function

[Konrad Rzeszutek Wilk]

On Mon, Mar 09, 2015 at 10:42:56AM -0400, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> In preparation for auxiliary RMRR data provided on Xen
> command line, make RMRR adding a separate function.
> No code changes.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/drivers/passthrough/vtd/dmar.c |  122 +++++++++++++++++++-----------------
>  xen/drivers/passthrough/vtd/dmar.h |    1 +
>  2 files changed, 66 insertions(+), 57 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index 1152c3a..4c85732 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -567,6 +567,66 @@ out:
>      return ret;
>  }
>  
> +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> +{
> +    u8 b, d, f;
> +    bool_t ignore = 0;
> +    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++ )
> +    {
> +        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(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 = 1;
> +        }
> +        else
> +        {
> +            ignore = 0;
> +            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);
> +        ret = -EFAULT;
> +    }
> +    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);
> +        ret = -EFAULT;
> +    }
> +    else
> +    {
> +        if ( iommu_verbose )
> +            dprintk(VTDPREFIX,
> +                    "  RMRR region: base_addr %"PRIx64
> +                    " end_address %"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)
>  {
> @@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>                                 &rmrru->scope, RMRR_TYPE, rmrr->segment);
>  
>      if ( ret || (rmrru->scope.devices_cnt == 0) )
> -        xfree(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) == 0 )
> -            {
> -                dprintk(XENLOG_WARNING VTDPREFIX,
> -                        " Non-existent device (%04x:%02x:%02x.%u) is 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 )
> -        {
> -            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);
> -            xfree(rmrru);
> -        }
> -        else if ( base_addr > end_addr )
> -        {
> -            dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> -                rmrru->base_address, rmrru->end_address);
> -            xfree(rmrru);
> -            ret = -EFAULT;
> -        }
> -        else
> -        {
> -            if ( iommu_verbose )
> -                dprintk(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;
>      }
>  
> +    ret = register_one_rmrr(rmrru);
> +    if ( ret )
> +        xfree(rmrru);
>      return ret;
>  }
>  
> diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
> index af1feef..060aa04 100644
> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -131,5 +131,6 @@ int vtd_hw_check(void);
>  void disable_pmr(struct iommu *iommu);
>  int is_usb_device(u16 seg, u8 bus, u8 devfn);
>  int is_igd_drhd(struct acpi_drhd_unit *drhd);
> +int register_one_rmrr(struct acpi_rmrr_unit *rmrru);
>  
>  #endif /* _DMAR_H_ */
> -- 
> 1.7.10.4
> 

Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function

[Tian, Kevin]

> From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> Sent: Monday, March 09, 2015 10:43 PM
> 
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> In preparation for auxiliary RMRR data provided on Xen
> command line, make RMRR adding a separate function.
> No code changes.
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>, with one small comment.

> ---
>  xen/drivers/passthrough/vtd/dmar.c |  122
> +++++++++++++++++++-----------------
>  xen/drivers/passthrough/vtd/dmar.h |    1 +
>  2 files changed, 66 insertions(+), 57 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c
> b/xen/drivers/passthrough/vtd/dmar.c
> index 1152c3a..4c85732 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -567,6 +567,66 @@ out:
>      return ret;
>  }
> 
> +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> +{
> +    u8 b, d, f;
> +    bool_t ignore = 0;
> +    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++ )
> +    {
> +        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(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 = 1;
> +        }
> +        else
> +        {
> +            ignore = 0;
> +            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);
> +        ret = -EFAULT;
> +    }
> +    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);
> +        ret = -EFAULT;
> +    }
> +    else
> +    {
> +        if ( iommu_verbose )
> +            dprintk(VTDPREFIX,
> +                    "  RMRR region: base_addr %"PRIx64
> +                    " end_address %"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)
>  {
> @@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header
> *header)
>                                 &rmrru->scope, RMRR_TYPE,
> rmrr->segment);
> 
>      if ( ret || (rmrru->scope.devices_cnt == 0) )
> -        xfree(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) == 0 )
> -            {
> -                dprintk(XENLOG_WARNING VTDPREFIX,
> -                        " Non-existent device (%04x:%02x:%02x.%u) is
> 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 )
> -        {
> -            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);
> -            xfree(rmrru);
> -        }
> -        else if ( base_addr > end_addr )
> -        {
> -            dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> -                rmrru->base_address, rmrru->end_address);
> -            xfree(rmrru);
> -            ret = -EFAULT;
> -        }
> -        else
> -        {
> -            if ( iommu_verbose )
> -                dprintk(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;
>      }
> 
> +    ret = register_one_rmrr(rmrru);
> +    if ( ret )
> +        xfree(rmrru);

a bit simplification here:

      if ( !ret && (rmrru->scope.devices_cnt != 0) )
			ret = register_one_rmrr(rmrru);
		if ( ret )
			xfree(rmrru);

>      return ret;
>  }
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.h
> b/xen/drivers/passthrough/vtd/dmar.h
> index af1feef..060aa04 100644
> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -131,5 +131,6 @@ int vtd_hw_check(void);
>  void disable_pmr(struct iommu *iommu);
>  int is_usb_device(u16 seg, u8 bus, u8 devfn);
>  int is_igd_drhd(struct acpi_drhd_unit *drhd);
> +int register_one_rmrr(struct acpi_rmrr_unit *rmrru);
> 
>  #endif /* _DMAR_H_ */
> --
> 1.7.10.4

Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function

[Jan Beulich]

>>> On 09.03.15 at 15:42, <elena.ufimtseva@oracle.com> wrote:
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -567,6 +567,66 @@ out:
>      return ret;
>  }
>  
> +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)

static - I don't see why the command line processing in patch 2 can't
be put in this file.

> +{
> +    u8 b, d, f;
> +    bool_t ignore = 0;
> +    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++ )
> +    {
> +        b = PCI_BUS(rmrru->scope.devices[i]);
> +        d = PCI_SLOT(rmrru->scope.devices[i]);
> +        f = PCI_FUNC(rmrru->scope.devices[i]);

Please make this the declarations (with initializers) of these variables;
they don't appear to be used outside this scope.

> +
> +        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 = 1;
> +        }
> +        else
> +        {
> +            ignore = 0;
> +            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);
> +        ret = -EFAULT;
> +    }
> +    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);
> +        ret = -EFAULT;
> +    }
> +    else
> +    {
> +        if ( iommu_verbose )
> +            dprintk(VTDPREFIX,
> +                    "  RMRR region: base_addr %"PRIx64
> +                    " end_address %"PRIx64"\n",
> +                    rmrru->base_address, rmrru->end_address);
> +        acpi_register_rmrr_unit(rmrru);
> +    }
> +    return ret;

Blank line before return please.

> @@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>                                 &rmrru->scope, RMRR_TYPE, rmrr->segment);
>  
>      if ( ret || (rmrru->scope.devices_cnt == 0) )
> -        xfree(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) == 0 )
> -            {
> -                dprintk(XENLOG_WARNING VTDPREFIX,
> -                        " Non-existent device (%04x:%02x:%02x.%u) is 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 )
> -        {
> -            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);
> -            xfree(rmrru);
> -        }
> -        else if ( base_addr > end_addr )
> -        {
> -            dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> -                rmrru->base_address, rmrru->end_address);
> -            xfree(rmrru);
> -            ret = -EFAULT;
> -        }
> -        else
> -        {
> -            if ( iommu_verbose )
> -                dprintk(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;
>      }
>  
> +    ret = register_one_rmrr(rmrru);
> +    if ( ret )
> +        xfree(rmrru);
>      return ret;

Please try to make this a single xfree() and a single return (with
again a blank line preceding it).

Jan

Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function

[Elena Ufimtseva]

On Wed, Mar 11, 2015 at 10:29:54AM +0000, Jan Beulich wrote:
> >>> On 09.03.15 at 15:42, <elena.ufimtseva@oracle.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -567,6 +567,66 @@ out:
> >      return ret;
> >  }
> >  
> > +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> 
> static - I don't see why the command line processing in patch 2 can't
> be put in this file.

I wanted to keep it close to iommu command line parser in iommu.c.
Andrew mentioned that the parser should be defined as custom_param.
Did you mean that as well and that it should be moved to dmar.c?

> 
> > +{
> > +    u8 b, d, f;
> > +    bool_t ignore = 0;
> > +    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++ )
> > +    {
> > +        b = PCI_BUS(rmrru->scope.devices[i]);
> > +        d = PCI_SLOT(rmrru->scope.devices[i]);
> > +        f = PCI_FUNC(rmrru->scope.devices[i]);
> 
> Please make this the declarations (with initializers) of these variables;
> they don't appear to be used outside this scope.
> 
> > +
> > +        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 = 1;
> > +        }
> > +        else
> > +        {
> > +            ignore = 0;
> > +            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);
> > +        ret = -EFAULT;
> > +    }
> > +    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);
> > +        ret = -EFAULT;
> > +    }
> > +    else
> > +    {
> > +        if ( iommu_verbose )
> > +            dprintk(VTDPREFIX,
> > +                    "  RMRR region: base_addr %"PRIx64
> > +                    " end_address %"PRIx64"\n",
> > +                    rmrru->base_address, rmrru->end_address);
> > +        acpi_register_rmrr_unit(rmrru);
> > +    }
> > +    return ret;
> 
> Blank line before return please.
> 
> > @@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
> >                                 &rmrru->scope, RMRR_TYPE, rmrr->segment);
> >  
> >      if ( ret || (rmrru->scope.devices_cnt == 0) )
> > -        xfree(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) == 0 )
> > -            {
> > -                dprintk(XENLOG_WARNING VTDPREFIX,
> > -                        " Non-existent device (%04x:%02x:%02x.%u) is 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 )
> > -        {
> > -            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);
> > -            xfree(rmrru);
> > -        }
> > -        else if ( base_addr > end_addr )
> > -        {
> > -            dprintk(XENLOG_WARNING VTDPREFIX,
> > -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> > -                rmrru->base_address, rmrru->end_address);
> > -            xfree(rmrru);
> > -            ret = -EFAULT;
> > -        }
> > -        else
> > -        {
> > -            if ( iommu_verbose )
> > -                dprintk(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;
> >      }
> >  
> > +    ret = register_one_rmrr(rmrru);
> > +    if ( ret )
> > +        xfree(rmrru);
> >      return ret;
> 
> Please try to make this a single xfree() and a single return (with
> again a blank line preceding it).

Got it, I will make changes you mentioned.
> 
> Jan

Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function

[Jan Beulich]

>>> On 12.03.15 at 21:48, <elena.ufimtseva@oracle.com> wrote:
> On Wed, Mar 11, 2015 at 10:29:54AM +0000, Jan Beulich wrote:
>> >>> On 09.03.15 at 15:42, <elena.ufimtseva@oracle.com> wrote:
>> > --- a/xen/drivers/passthrough/vtd/dmar.c
>> > +++ b/xen/drivers/passthrough/vtd/dmar.c
>> > @@ -567,6 +567,66 @@ out:
>> >      return ret;
>> >  }
>> >  
>> > +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>> 
>> static - I don't see why the command line processing in patch 2 can't
>> be put in this file.
> 
> I wanted to keep it close to iommu command line parser in iommu.c.
> Andrew mentioned that the parser should be defined as custom_param.
> Did you mean that as well and that it should be moved to dmar.c?

Yes and yes.

Jan

[PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[elena.ufimtseva]

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

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=[sbdf]start<:end>,[sbdf]start:<end>

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 docs/misc/xen-command-line.markdown |    7 +++
 xen/drivers/passthrough/iommu.c     |   86 +++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
 xen/drivers/passthrough/vtd/iommu.h |    1 +
 xen/include/xen/iommu.h             |    9 ++++
 5 files changed, 136 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bc316be..2e1210f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1392,3 +1392,10 @@ mode.
 > Default: `true`
 
 Permit use of the `xsave/xrstor` instructions.
+
+### rmrr
+> '= [sbdf]start<:end>,[sbdf]start<:end>
+
+Define RMRRs 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.
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index cc12735..b64916e 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
 bool_t __read_mostly iommu_debug;
 bool_t __read_mostly amd_iommu_perdev_intremap = 1;
 
+static char __initdata misc_rmrr[100];
+string_param("rmrr", misc_rmrr);
+
 DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 
 DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
@@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
     .desc = "dump iommu p2m table"
 };
 
+/*
+ * List of command line defined rmrr units.
+ */
+__initdata LIST_HEAD(misc_rmrr_units);
+
+/*
+ * Parse rmrr Xen command line options and add parsed
+ * device and region into apci_rmrr_unit list to mapped
+ * as RMRRs parsed from ACPI.
+ * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
+ * end address can be ommited and one page will be used
+ * for mapping with start pfn.
+ */
+static void __init parse_iommu_extra_rmrr(const char *s)
+{
+    unsigned int idx = 0, found = 0;
+    struct misc_rmrr_unit *rmrru;
+    unsigned int seg, bus, dev, func;
+    const char *str, *cur;
+    u64 start, end;
+
+    do {
+        if ( idx >= 10 )
+            break;
+
+        if ( *s != '[' )
+            break;
+
+        str = s;
+        seg = bus = dev = func = 0;
+        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
+        if ( !str )
+        {
+            str = strchr(s, ']');
+            if ( !str )
+                return;
+        }
+        else
+            found = 1;
+
+        s = str;
+        if ( *s != ']' )
+            return;
+
+        start = simple_strtoull(cur = s + 1, &s, 0);
+        if ( cur == s )
+            break;
+
+        if ( *s == ':' )
+        {
+            end = simple_strtoull(cur = s + 1, &s, 0);
+            if ( cur == s )
+                break;
+        }
+        else
+            end = start;
+
+        if ( !found )
+            continue;
+
+        if ( end >= start )
+        {
+            rmrru = xzalloc(struct misc_rmrr_unit);
+            if ( !rmrru )
+                return;
+            rmrru->base_address = start << PAGE_SHIFT;
+            rmrru->end_address = (end << PAGE_SHIFT) + 1;
+            rmrru->segment = seg;
+            rmrru->device = PCI_BDF(bus, dev, func);
+            list_add(&rmrru->list, &misc_rmrr_units);
+        }
+        else
+        {
+            printk(XENLOG_WARNING "Bad rmrr: start > end, %"PRIx64" > %"PRIx64"\n",
+                   start, end);
+            break;
+        }
+        idx++;
+    } while ( *s++ == ',' );
+}
+
 static void __init parse_iommu_param(char *s)
 {
     char *ss;
@@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
     if ( !iommu_enabled )
         return;
 
+    parse_iommu_extra_rmrr(misc_rmrr);
+
     register_keyhandler('o', &iommu_p2m_table);
     d->need_iommu = !!iommu_dom0_strict;
     if ( need_iommu(d) && !iommu_use_hap_pt(d) )
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 2e113d7..9cb6e73 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain *d)
     return 0;
 }
 
+static void add_misc_rmrr(void)
+{
+    struct acpi_rmrr_unit *rmrrn;
+    struct misc_rmrr_unit *rmrru, *r;
+
+    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
+    {
+        rmrrn = xzalloc(struct acpi_rmrr_unit);
+        if ( !rmrrn )
+            goto free;
+
+        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
+        if ( !rmrrn->scope.devices )
+        {
+            xfree(rmrrn);
+            goto free;
+        }
+        rmrrn->scope.devices_cnt = 1;
+        rmrrn->segment = rmrru->segment;
+        rmrrn->scope.devices[0] = rmrru->device;
+
+        if ( register_one_rmrr(rmrrn) )
+        {
+            xfree(rmrrn->scope.devices);
+            xfree(rmrrn);
+        }
+ free:
+        list_del(&rmrru->list);
+        xfree(rmrru);
+    }
+}
+
 static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
 {
     struct acpi_drhd_unit *drhd;
@@ -1243,6 +1275,7 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
     }
 
     setup_hwdom_pci_devices(d, setup_hwdom_device);
+    add_misc_rmrr();
     setup_hwdom_rmrr(d);
 
     iommu_flush_all();
diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
index c3e5181..b3c1b59 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -484,6 +484,7 @@ struct qinval_entry {
 extern struct list_head acpi_drhd_units;
 extern struct list_head acpi_rmrr_units;
 extern struct list_head acpi_ioapic_units;
+extern struct list_head misc_rmrr_units;
 
 struct qi_ctrl {
     u64 qinval_maddr;  /* queue invalidation page machine address */
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 8eb764a..2c43956 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 extern struct spinlock iommu_pt_cleanup_lock;
 extern struct page_list_head iommu_pt_cleanup_list;
 
+/*RMRR units derived from command line rmrr option */
+struct misc_rmrr_unit {
+    struct list_head list;
+    u64    base_address;
+    u64    end_address;
+    u16    segment;
+    u16    device;
+};
+
 #endif /* _IOMMU_H_ */
-- 
1.7.10.4

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Konrad Rzeszutek Wilk]

On Mon, Mar 09, 2015 at 10:42:57AM -0400, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> 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

ThinkCentre M93p

> 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=[sbdf]start<:end>,[sbdf]start:<end>
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
>  docs/misc/xen-command-line.markdown |    7 +++
>  xen/drivers/passthrough/iommu.c     |   86 +++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.h |    1 +
>  xen/include/xen/iommu.h             |    9 ++++
>  5 files changed, 136 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index bc316be..2e1210f 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1392,3 +1392,10 @@ mode.
>  > Default: `true`
>  
>  Permit use of the `xsave/xrstor` instructions.
> +
> +### rmrr
> +> '= [sbdf]start<:end>,[sbdf]start<:end>

You are missing an ` at the end.

Hm, the syntax is interesting. Usually '[' and ']' denote
an optional field (see 'console'). But in your case you require it.

I am not actually sure how to say in a man page that '[' is
to not be optinal.

Perhaps instead of '[]' you could use '()' ? Or just require it
and change the syntax:

 [<seg>:]<bus>:<device>.<func>:start-end[,[<seg>:]<bus>:<device>.<func>:start-end,...]

?
(And of course the code too)

> +
> +Define RMRRs 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.

 * <sbdf> is the PCI location of the device in [<segment>:]<bus>:<device>.<function>
   notation. 

A typical setup might be [00:1a.f]
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index cc12735..b64916e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
>  bool_t __read_mostly iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>  
> +static char __initdata misc_rmrr[100];
> +string_param("rmrr", misc_rmrr);
> +
>  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>  
>  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
>      .desc = "dump iommu p2m table"
>  };
>  
> +/*
> + * List of command line defined rmrr units.
> + */
> +__initdata LIST_HEAD(misc_rmrr_units);
> +
> +/*
> + * Parse rmrr Xen command line options and add parsed
> + * device and region into apci_rmrr_unit list to mapped
> + * as RMRRs parsed from ACPI.
> + * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> + * end address can be ommited and one page will be used
> + * for mapping with start pfn.
> + */
> +static void __init parse_iommu_extra_rmrr(const char *s)
> +{
> +    unsigned int idx = 0, found = 0;
> +    struct misc_rmrr_unit *rmrru;
> +    unsigned int seg, bus, dev, func;
> +    const char *str, *cur;
> +    u64 start, end;
> +
> +    do {
> +        if ( idx >= 10 )

How come 10?

Perhaps swithc 'idx' to a different counter - to the amount of 
characters you have touched - so that you can make sure that
you won't go past the 100 you have allocated?

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Andrew Cooper]

On 09/03/15 14:42, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>
> 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=[sbdf]start<:end>,[sbdf]start:<end>
>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
>  docs/misc/xen-command-line.markdown |    7 +++
>  xen/drivers/passthrough/iommu.c     |   86 +++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.h |    1 +
>  xen/include/xen/iommu.h             |    9 ++++
>  5 files changed, 136 insertions(+)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index bc316be..2e1210f 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1392,3 +1392,10 @@ mode.
>  > Default: `true`
>  
>  Permit use of the `xsave/xrstor` instructions.

This file is sorted in alphabetic order.  Please keep it in order.

> +
> +### rmrr
> +> '= [sbdf]start<:end>,[sbdf]start<:end>
> +
> +Define RMRRs 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.
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index cc12735..b64916e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
>  bool_t __read_mostly iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>  
> +static char __initdata misc_rmrr[100];
> +string_param("rmrr", misc_rmrr);

This wants to be a custom_param, not a string_param.

Furthermore, I think it would be better to match the existing ivrs_hpet[
and ivrs_ioapic[ parameters as closely as possible, seeing as it is the
same kind of overrides being added to ACPI tables.

~Andrew

> +
>  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>  
>  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
>      .desc = "dump iommu p2m table"
>  };
>  
> +/*
> + * List of command line defined rmrr units.
> + */
> +__initdata LIST_HEAD(misc_rmrr_units);
> +
> +/*
> + * Parse rmrr Xen command line options and add parsed
> + * device and region into apci_rmrr_unit list to mapped
> + * as RMRRs parsed from ACPI.
> + * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> + * end address can be ommited and one page will be used
> + * for mapping with start pfn.
> + */
> +static void __init parse_iommu_extra_rmrr(const char *s)
> +{
> +    unsigned int idx = 0, found = 0;
> +    struct misc_rmrr_unit *rmrru;
> +    unsigned int seg, bus, dev, func;
> +    const char *str, *cur;
> +    u64 start, end;
> +
> +    do {
> +        if ( idx >= 10 )
> +            break;
> +
> +        if ( *s != '[' )
> +            break;
> +
> +        str = s;
> +        seg = bus = dev = func = 0;
> +        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
> +        if ( !str )
> +        {
> +            str = strchr(s, ']');
> +            if ( !str )
> +                return;
> +        }
> +        else
> +            found = 1;
> +
> +        s = str;
> +        if ( *s != ']' )
> +            return;
> +
> +        start = simple_strtoull(cur = s + 1, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == ':' )
> +        {
> +            end = simple_strtoull(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +
> +        if ( !found )
> +            continue;
> +
> +        if ( end >= start )
> +        {
> +            rmrru = xzalloc(struct misc_rmrr_unit);
> +            if ( !rmrru )
> +                return;
> +            rmrru->base_address = start << PAGE_SHIFT;
> +            rmrru->end_address = (end << PAGE_SHIFT) + 1;
> +            rmrru->segment = seg;
> +            rmrru->device = PCI_BDF(bus, dev, func);
> +            list_add(&rmrru->list, &misc_rmrr_units);
> +        }
> +        else
> +        {
> +            printk(XENLOG_WARNING "Bad rmrr: start > end, %"PRIx64" > %"PRIx64"\n",
> +                   start, end);
> +            break;
> +        }
> +        idx++;
> +    } while ( *s++ == ',' );
> +}
> +
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> @@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      if ( !iommu_enabled )
>          return;
>  
> +    parse_iommu_extra_rmrr(misc_rmrr);
> +
>      register_keyhandler('o', &iommu_p2m_table);
>      d->need_iommu = !!iommu_dom0_strict;
>      if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 2e113d7..9cb6e73 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain *d)
>      return 0;
>  }
>  
> +static void add_misc_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrrn;
> +    struct misc_rmrr_unit *rmrru, *r;
> +
> +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> +    {
> +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> +        if ( !rmrrn )
> +            goto free;
> +
> +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> +        if ( !rmrrn->scope.devices )
> +        {
> +            xfree(rmrrn);
> +            goto free;
> +        }
> +        rmrrn->scope.devices_cnt = 1;
> +        rmrrn->segment = rmrru->segment;
> +        rmrrn->scope.devices[0] = rmrru->device;
> +
> +        if ( register_one_rmrr(rmrrn) )
> +        {
> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +        }
> + free:
> +        list_del(&rmrru->list);
> +        xfree(rmrru);
> +    }
> +}
> +
>  static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
> @@ -1243,6 +1275,7 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
>      }
>  
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
> +    add_misc_rmrr();
>      setup_hwdom_rmrr(d);
>  
>      iommu_flush_all();
> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
> index c3e5181..b3c1b59 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -484,6 +484,7 @@ struct qinval_entry {
>  extern struct list_head acpi_drhd_units;
>  extern struct list_head acpi_rmrr_units;
>  extern struct list_head acpi_ioapic_units;
> +extern struct list_head misc_rmrr_units;
>  
>  struct qi_ctrl {
>      u64 qinval_maddr;  /* queue invalidation page machine address */
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 8eb764a..2c43956 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>  extern struct spinlock iommu_pt_cleanup_lock;
>  extern struct page_list_head iommu_pt_cleanup_list;
>  
> +/*RMRR units derived from command line rmrr option */
> +struct misc_rmrr_unit {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +    u16    segment;
> +    u16    device;
> +};
> +
>  #endif /* _IOMMU_H_ */

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Elena Ufimtseva]

On Mon, Mar 09, 2015 at 05:16:18PM +0000, Andrew Cooper wrote:
> On 09/03/15 14:42, elena.ufimtseva@oracle.com wrote:
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> >
> > 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=[sbdf]start<:end>,[sbdf]start:<end>
> >
> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > ---
> >  docs/misc/xen-command-line.markdown |    7 +++
> >  xen/drivers/passthrough/iommu.c     |   86 +++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.h |    1 +
> >  xen/include/xen/iommu.h             |    9 ++++
> >  5 files changed, 136 insertions(+)
> >
> > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> > index bc316be..2e1210f 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1392,3 +1392,10 @@ mode.
> >  > Default: `true`
> >  
> >  Permit use of the `xsave/xrstor` instructions.
> 
> This file is sorted in alphabetic order.  Please keep it in order.
> 
> > +
> > +### rmrr
> > +> '= [sbdf]start<:end>,[sbdf]start<:end>
> > +
> > +Define RMRRs 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.
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index cc12735..b64916e 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
> >  bool_t __read_mostly iommu_debug;
> >  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> >  
> > +static char __initdata misc_rmrr[100];
> > +string_param("rmrr", misc_rmrr);
> 
> This wants to be a custom_param, not a string_param.

Andrew, can you please explain why its preferred?
Also, I wont be able to use xen dynamic allocator at this early stage, correct?
> 
> Furthermore, I think it would be better to match the existing ivrs_hpet[
> and ivrs_ioapic[ parameters as closely as possible, seeing as it is the
> same kind of overrides being added to ACPI tables.

Thanks Andrew, looks like preferred way looks like this:
rmrr=start<:end>[sbdf1, sbdf2, ...], ...
 
> 
> ~Andrew
> 
> > +
> >  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> >  
> >  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> > @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
> >      .desc = "dump iommu p2m table"
> >  };
> >  
> > +/*
> > + * List of command line defined rmrr units.
> > + */
> > +__initdata LIST_HEAD(misc_rmrr_units);
> > +
> > +/*
> > + * Parse rmrr Xen command line options and add parsed
> > + * device and region into apci_rmrr_unit list to mapped
> > + * as RMRRs parsed from ACPI.
> > + * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> > + * end address can be ommited and one page will be used
> > + * for mapping with start pfn.
> > + */
> > +static void __init parse_iommu_extra_rmrr(const char *s)
> > +{
> > +    unsigned int idx = 0, found = 0;
> > +    struct misc_rmrr_unit *rmrru;
> > +    unsigned int seg, bus, dev, func;
> > +    const char *str, *cur;
> > +    u64 start, end;
> > +
> > +    do {
> > +        if ( idx >= 10 )
> > +            break;
> > +
> > +        if ( *s != '[' )
> > +            break;
> > +
> > +        str = s;
> > +        seg = bus = dev = func = 0;
> > +        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
> > +        if ( !str )
> > +        {
> > +            str = strchr(s, ']');
> > +            if ( !str )
> > +                return;
> > +        }
> > +        else
> > +            found = 1;
> > +
> > +        s = str;
> > +        if ( *s != ']' )
> > +            return;
> > +
> > +        start = simple_strtoull(cur = s + 1, &s, 0);
> > +        if ( cur == s )
> > +            break;
> > +
> > +        if ( *s == ':' )
> > +        {
> > +            end = simple_strtoull(cur = s + 1, &s, 0);
> > +            if ( cur == s )
> > +                break;
> > +        }
> > +        else
> > +            end = start;
> > +
> > +        if ( !found )
> > +            continue;
> > +
> > +        if ( end >= start )
> > +        {
> > +            rmrru = xzalloc(struct misc_rmrr_unit);
> > +            if ( !rmrru )
> > +                return;
> > +            rmrru->base_address = start << PAGE_SHIFT;
> > +            rmrru->end_address = (end << PAGE_SHIFT) + 1;
> > +            rmrru->segment = seg;
> > +            rmrru->device = PCI_BDF(bus, dev, func);
> > +            list_add(&rmrru->list, &misc_rmrr_units);
> > +        }
> > +        else
> > +        {
> > +            printk(XENLOG_WARNING "Bad rmrr: start > end, %"PRIx64" > %"PRIx64"\n",
> > +                   start, end);
> > +            break;
> > +        }
> > +        idx++;
> > +    } while ( *s++ == ',' );
> > +}
> > +
> >  static void __init parse_iommu_param(char *s)
> >  {
> >      char *ss;
> > @@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >      if ( !iommu_enabled )
> >          return;
> >  
> > +    parse_iommu_extra_rmrr(misc_rmrr);
> > +
> >      register_keyhandler('o', &iommu_p2m_table);
> >      d->need_iommu = !!iommu_dom0_strict;
> >      if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> > index 2e113d7..9cb6e73 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain *d)
> >      return 0;
> >  }
> >  
> > +static void add_misc_rmrr(void)
> > +{
> > +    struct acpi_rmrr_unit *rmrrn;
> > +    struct misc_rmrr_unit *rmrru, *r;
> > +
> > +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> > +    {
> > +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> > +        if ( !rmrrn )
> > +            goto free;
> > +
> > +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> > +        if ( !rmrrn->scope.devices )
> > +        {
> > +            xfree(rmrrn);
> > +            goto free;
> > +        }
> > +        rmrrn->scope.devices_cnt = 1;
> > +        rmrrn->segment = rmrru->segment;
> > +        rmrrn->scope.devices[0] = rmrru->device;
> > +
> > +        if ( register_one_rmrr(rmrrn) )
> > +        {
> > +            xfree(rmrrn->scope.devices);
> > +            xfree(rmrrn);
> > +        }
> > + free:
> > +        list_del(&rmrru->list);
> > +        xfree(rmrru);
> > +    }
> > +}
> > +
> >  static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
> >  {
> >      struct acpi_drhd_unit *drhd;
> > @@ -1243,6 +1275,7 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
> >      }
> >  
> >      setup_hwdom_pci_devices(d, setup_hwdom_device);
> > +    add_misc_rmrr();
> >      setup_hwdom_rmrr(d);
> >  
> >      iommu_flush_all();
> > diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
> > index c3e5181..b3c1b59 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > @@ -484,6 +484,7 @@ struct qinval_entry {
> >  extern struct list_head acpi_drhd_units;
> >  extern struct list_head acpi_rmrr_units;
> >  extern struct list_head acpi_ioapic_units;
> > +extern struct list_head misc_rmrr_units;
> >  
> >  struct qi_ctrl {
> >      u64 qinval_maddr;  /* queue invalidation page machine address */
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 8eb764a..2c43956 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> >  extern struct spinlock iommu_pt_cleanup_lock;
> >  extern struct page_list_head iommu_pt_cleanup_list;
> >  
> > +/*RMRR units derived from command line rmrr option */
> > +struct misc_rmrr_unit {
> > +    struct list_head list;
> > +    u64    base_address;
> > +    u64    end_address;
> > +    u16    segment;
> > +    u16    device;
> > +};
> > +
> >  #endif /* _IOMMU_H_ */
> 
> 

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Jan Beulich]

>>> On 12.03.15 at 21:52, <elena.ufimtseva@oracle.com> wrote:
> On Mon, Mar 09, 2015 at 05:16:18PM +0000, Andrew Cooper wrote:
>> On 09/03/15 14:42, elena.ufimtseva@oracle.com wrote:
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
>> >  bool_t __read_mostly iommu_debug;
>> >  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>> >  
>> > +static char __initdata misc_rmrr[100];
>> > +string_param("rmrr", misc_rmrr);
>> 
>> This wants to be a custom_param, not a string_param.
> 
> Andrew, can you please explain why its preferred?

Consistency.

> Also, I wont be able to use xen dynamic allocator at this early stage, 
> correct?

Yes. But by having a static limit on the string size you had already
limited the number of entries, so having a static array instead won't
make things worse.

>> Furthermore, I think it would be better to match the existing ivrs_hpet[
>> and ivrs_ioapic[ parameters as closely as possible, seeing as it is the
>> same kind of overrides being added to ACPI tables.
> 
> Thanks Andrew, looks like preferred way looks like this:
> rmrr=start<:end>[sbdf1, sbdf2, ...], ...

With the slight adjustment that

	rmrr=start:<end:>sbdf1[,sbdf2[, ...]][; ...]

or even (considering that sbdf necessarily include : too)

	rmrr=start<:end>=sbdf1[,sbdf2[, ...]][; ...]

will likely make it easier to parse unambiguously.

Jan

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Tian, Kevin]

> From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> Sent: Monday, March 09, 2015 10:43 PM
> 
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> 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=[sbdf]start<:end>,[sbdf]start:<end>

how about sticking to rmrr structure, i.e. 

rmrr=start<:end>[sbdf1, sbdf2, ...], ...

> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
>  docs/misc/xen-command-line.markdown |    7 +++
>  xen/drivers/passthrough/iommu.c     |   86
> +++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.h |    1 +
>  xen/include/xen/iommu.h             |    9 ++++
>  5 files changed, 136 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.markdown
> b/docs/misc/xen-command-line.markdown
> index bc316be..2e1210f 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1392,3 +1392,10 @@ mode.
>  > Default: `true`
> 
>  Permit use of the `xsave/xrstor` instructions.
> +
> +### rmrr
> +> '= [sbdf]start<:end>,[sbdf]start<:end>
> +
> +Define RMRRs 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.
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index cc12735..b64916e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
>  bool_t __read_mostly iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> 
> +static char __initdata misc_rmrr[100];

define a macro.

> +string_param("rmrr", misc_rmrr);
> +
>  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> 
>  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
>      .desc = "dump iommu p2m table"
>  };
> 
> +/*
> + * List of command line defined rmrr units.
> + */
> +__initdata LIST_HEAD(misc_rmrr_units);
> +
> +/*
> + * Parse rmrr Xen command line options and add parsed
> + * device and region into apci_rmrr_unit list to mapped
> + * as RMRRs parsed from ACPI.
> + * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> + * end address can be ommited and one page will be used
> + * for mapping with start pfn.
> + */
> +static void __init parse_iommu_extra_rmrr(const char *s)
> +{
> +    unsigned int idx = 0, found = 0;
> +    struct misc_rmrr_unit *rmrru;
> +    unsigned int seg, bus, dev, func;
> +    const char *str, *cur;
> +    u64 start, end;
> +
> +    do {
> +        if ( idx >= 10 )
> +            break;

as Konrad pointed out, using 10 and earlier 100 are not readable
and error prone.

> +
> +        if ( *s != '[' )
> +            break;
> +
> +        str = s;
> +        seg = bus = dev = func = 0;
> +        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
> +        if ( !str )
> +        {
> +            str = strchr(s, ']');
> +            if ( !str )
> +                return;
> +        }
> +        else
> +            found = 1;
> +
> +        s = str;
> +        if ( *s != ']' )
> +            return;

better to have some warn message for malformat.

> +
> +        start = simple_strtoull(cur = s + 1, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == ':' )
> +        {
> +            end = simple_strtoull(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +
> +        if ( !found )
> +            continue;
> +
> +        if ( end >= start )
> +        {
> +            rmrru = xzalloc(struct misc_rmrr_unit);
> +            if ( !rmrru )
> +                return;
> +            rmrru->base_address = start << PAGE_SHIFT;
> +            rmrru->end_address = (end << PAGE_SHIFT) + 1;
> +            rmrru->segment = seg;
> +            rmrru->device = PCI_BDF(bus, dev, func);
> +            list_add(&rmrru->list, &misc_rmrr_units);
> +        }
> +        else
> +        {
> +            printk(XENLOG_WARNING "Bad rmrr: start >
> end, %"PRIx64" > %"PRIx64"\n",
> +                   start, end);
> +            break;
> +        }
> +        idx++;
> +    } while ( *s++ == ',' );
> +}
> +
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> @@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain
> *d)
>      if ( !iommu_enabled )
>          return;
> 
> +    parse_iommu_extra_rmrr(misc_rmrr);
> +
>      register_keyhandler('o', &iommu_p2m_table);
>      d->need_iommu = !!iommu_dom0_strict;
>      if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 2e113d7..9cb6e73 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain
> *d)
>      return 0;
>  }
> 
> +static void add_misc_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrrn;
> +    struct misc_rmrr_unit *rmrru, *r;
> +
> +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> +    {
> +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> +        if ( !rmrrn )
> +            goto free;
> +
> +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> +        if ( !rmrrn->scope.devices )
> +        {
> +            xfree(rmrrn);
> +            goto free;
> +        }
> +        rmrrn->scope.devices_cnt = 1;
> +        rmrrn->segment = rmrru->segment;
> +        rmrrn->scope.devices[0] = rmrru->device;

need handle one-rmrr-multiple-deviecs. even if you don't want
to support it, need capture user attempts at least.

> +
> +        if ( register_one_rmrr(rmrrn) )
> +        {
> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +        }
> + free:
> +        list_del(&rmrru->list);
> +        xfree(rmrru);
> +    }
> +}
> +
>  static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
> @@ -1243,6 +1275,7 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
>      }
> 
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
> +    add_misc_rmrr();
>      setup_hwdom_rmrr(d);
> 
>      iommu_flush_all();
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index c3e5181..b3c1b59 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -484,6 +484,7 @@ struct qinval_entry {
>  extern struct list_head acpi_drhd_units;
>  extern struct list_head acpi_rmrr_units;
>  extern struct list_head acpi_ioapic_units;
> +extern struct list_head misc_rmrr_units;
> 
>  struct qi_ctrl {
>      u64 qinval_maddr;  /* queue invalidation page machine address */
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 8eb764a..2c43956 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t,
> iommu_dont_flush_iotlb);
>  extern struct spinlock iommu_pt_cleanup_lock;
>  extern struct page_list_head iommu_pt_cleanup_list;
> 
> +/*RMRR units derived from command line rmrr option */
> +struct misc_rmrr_unit {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +    u16    segment;
> +    u16    device;
> +};
> +
>  #endif /* _IOMMU_H_ */
> --
> 1.7.10.4

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Elena Ufimtseva]

On Tue, Mar 10, 2015 at 02:47:24AM +0000, Tian, Kevin wrote:
> > From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> > Sent: Monday, March 09, 2015 10:43 PM
> > 
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > 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=[sbdf]start<:end>,[sbdf]start:<end>
> 
> how about sticking to rmrr structure, i.e. 
> 
> rmrr=start<:end>[sbdf1, sbdf2, ...], ...

Looks like there are few options here :)

Konrad proposed 
rmrr=[<seg>:]<bus>:<device>.<func>:start-end[,[<seg>:]<bus>:<device>.<func>:start-end,...] 
Andrew also offered similar format to what Konard proposed.
After getting rid of square brackets its pretty much the same format.

Do you think this will work for everyone?
rmrr=start<-end>:[<seg:>]<bus>:<device>.<func>,...
> 
> > 
> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > ---
> >  docs/misc/xen-command-line.markdown |    7 +++
> >  xen/drivers/passthrough/iommu.c     |   86
> > +++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.h |    1 +
> >  xen/include/xen/iommu.h             |    9 ++++
> >  5 files changed, 136 insertions(+)
> > 
> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index bc316be..2e1210f 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1392,3 +1392,10 @@ mode.
> >  > Default: `true`
> > 
> >  Permit use of the `xsave/xrstor` instructions.
> > +
> > +### rmrr
> > +> '= [sbdf]start<:end>,[sbdf]start<:end>
> > +
> > +Define RMRRs 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.
> > diff --git a/xen/drivers/passthrough/iommu.c
> > b/xen/drivers/passthrough/iommu.c
> > index cc12735..b64916e 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
> >  bool_t __read_mostly iommu_debug;
> >  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> > 
> > +static char __initdata misc_rmrr[100];
> 
> define a macro.
> 
> > +string_param("rmrr", misc_rmrr);
> > +
> >  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> > 
> >  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> > @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
> >      .desc = "dump iommu p2m table"
> >  };
> > 
> > +/*
> > + * List of command line defined rmrr units.
> > + */
> > +__initdata LIST_HEAD(misc_rmrr_units);
> > +
> > +/*
> > + * Parse rmrr Xen command line options and add parsed
> > + * device and region into apci_rmrr_unit list to mapped
> > + * as RMRRs parsed from ACPI.
> > + * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> > + * end address can be ommited and one page will be used
> > + * for mapping with start pfn.
> > + */
> > +static void __init parse_iommu_extra_rmrr(const char *s)
> > +{
> > +    unsigned int idx = 0, found = 0;
> > +    struct misc_rmrr_unit *rmrru;
> > +    unsigned int seg, bus, dev, func;
> > +    const char *str, *cur;
> > +    u64 start, end;
> > +
> > +    do {
> > +        if ( idx >= 10 )
> > +            break;
> 
> as Konrad pointed out, using 10 and earlier 100 are not readable
> and error prone.
> 
> > +
> > +        if ( *s != '[' )
> > +            break;
> > +
> > +        str = s;
> > +        seg = bus = dev = func = 0;
> > +        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
> > +        if ( !str )
> > +        {
> > +            str = strchr(s, ']');
> > +            if ( !str )
> > +                return;
> > +        }
> > +        else
> > +            found = 1;
> > +
> > +        s = str;
> > +        if ( *s != ']' )
> > +            return;
> 
> better to have some warn message for malformat.
> 
> > +
> > +        start = simple_strtoull(cur = s + 1, &s, 0);
> > +        if ( cur == s )
> > +            break;
> > +
> > +        if ( *s == ':' )
> > +        {
> > +            end = simple_strtoull(cur = s + 1, &s, 0);
> > +            if ( cur == s )
> > +                break;
> > +        }
> > +        else
> > +            end = start;
> > +
> > +        if ( !found )
> > +            continue;
> > +
> > +        if ( end >= start )
> > +        {
> > +            rmrru = xzalloc(struct misc_rmrr_unit);
> > +            if ( !rmrru )
> > +                return;
> > +            rmrru->base_address = start << PAGE_SHIFT;
> > +            rmrru->end_address = (end << PAGE_SHIFT) + 1;
> > +            rmrru->segment = seg;
> > +            rmrru->device = PCI_BDF(bus, dev, func);
> > +            list_add(&rmrru->list, &misc_rmrr_units);
> > +        }
> > +        else
> > +        {
> > +            printk(XENLOG_WARNING "Bad rmrr: start >
> > end, %"PRIx64" > %"PRIx64"\n",
> > +                   start, end);
> > +            break;
> > +        }
> > +        idx++;
> > +    } while ( *s++ == ',' );
> > +}
> > +
> >  static void __init parse_iommu_param(char *s)
> >  {
> >      char *ss;
> > @@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain
> > *d)
> >      if ( !iommu_enabled )
> >          return;
> > 
> > +    parse_iommu_extra_rmrr(misc_rmrr);
> > +
> >      register_keyhandler('o', &iommu_p2m_table);
> >      d->need_iommu = !!iommu_dom0_strict;
> >      if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index 2e113d7..9cb6e73 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain
> > *d)
> >      return 0;
> >  }
> > 
> > +static void add_misc_rmrr(void)
> > +{
> > +    struct acpi_rmrr_unit *rmrrn;
> > +    struct misc_rmrr_unit *rmrru, *r;
> > +
> > +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> > +    {
> > +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> > +        if ( !rmrrn )
> > +            goto free;
> > +
> > +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> > +        if ( !rmrrn->scope.devices )
> > +        {
> > +            xfree(rmrrn);
> > +            goto free;
> > +        }
> > +        rmrrn->scope.devices_cnt = 1;
> > +        rmrrn->segment = rmrru->segment;
> > +        rmrrn->scope.devices[0] = rmrru->device;
> 
> need handle one-rmrr-multiple-deviecs. even if you don't want
> to support it, need capture user attempts at least.

It is actually a good point and I think I should support one rmrr with
multiple devices, as ACPI RMRR structure does.

> 
> > +
> > +        if ( register_one_rmrr(rmrrn) )
> > +        {
> > +            xfree(rmrrn->scope.devices);
> > +            xfree(rmrrn);
> > +        }
> > + free:
> > +        list_del(&rmrru->list);
> > +        xfree(rmrru);
> > +    }
> > +}
> > +
> >  static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
> >  {
> >      struct acpi_drhd_unit *drhd;
> > @@ -1243,6 +1275,7 @@ static void __hwdom_init
> > intel_iommu_hwdom_init(struct domain *d)
> >      }
> > 
> >      setup_hwdom_pci_devices(d, setup_hwdom_device);
> > +    add_misc_rmrr();
> >      setup_hwdom_rmrr(d);
> > 
> >      iommu_flush_all();
> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> > b/xen/drivers/passthrough/vtd/iommu.h
> > index c3e5181..b3c1b59 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > @@ -484,6 +484,7 @@ struct qinval_entry {
> >  extern struct list_head acpi_drhd_units;
> >  extern struct list_head acpi_rmrr_units;
> >  extern struct list_head acpi_ioapic_units;
> > +extern struct list_head misc_rmrr_units;
> > 
> >  struct qi_ctrl {
> >      u64 qinval_maddr;  /* queue invalidation page machine address */
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 8eb764a..2c43956 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t,
> > iommu_dont_flush_iotlb);
> >  extern struct spinlock iommu_pt_cleanup_lock;
> >  extern struct page_list_head iommu_pt_cleanup_list;
> > 
> > +/*RMRR units derived from command line rmrr option */
> > +struct misc_rmrr_unit {
> > +    struct list_head list;
> > +    u64    base_address;
> > +    u64    end_address;
> > +    u16    segment;
> > +    u16    device;
> > +};
> > +
> >  #endif /* _IOMMU_H_ */
> > --
> > 1.7.10.4
> 

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Jan Beulich]

>>> On 10.03.15 at 03:47, <kevin.tian@intel.com> wrote:
>>  From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
>> Format for rmrr Xen command line option:
>> rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> 
> how about sticking to rmrr structure, i.e. 
> 
> rmrr=start<:end>[sbdf1, sbdf2, ...], ...

+1

>> +        if ( *s != ']' )
>> +            return;
> 
> better to have some warn message for malformat.

Warning messages from command line argument parsing functions
(which this is to be moved into by making it a custom_param, as
requested by Andrew [which I support])  are at best marginally
useful, as they get issued before any console was set up.

Jan

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Elena Ufimtseva]

On Tue, Mar 10, 2015 at 02:47:24AM +0000, Tian, Kevin wrote:
> > From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> > Sent: Monday, March 09, 2015 10:43 PM
> > 
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > 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=[sbdf]start<:end>,[sbdf]start:<end>
> 
> how about sticking to rmrr structure, i.e. 
> 
> rmrr=start<:end>[sbdf1, sbdf2, ...], ...
> 
> > 
> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > ---
> >  docs/misc/xen-command-line.markdown |    7 +++
> >  xen/drivers/passthrough/iommu.c     |   86
> > +++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.h |    1 +
> >  xen/include/xen/iommu.h             |    9 ++++
> >  5 files changed, 136 insertions(+)
> > 
> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index bc316be..2e1210f 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1392,3 +1392,10 @@ mode.
> >  > Default: `true`
> > 
> >  Permit use of the `xsave/xrstor` instructions.
> > +
> > +### rmrr
> > +> '= [sbdf]start<:end>,[sbdf]start<:end>
> > +
> > +Define RMRRs 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.
> > diff --git a/xen/drivers/passthrough/iommu.c
> > b/xen/drivers/passthrough/iommu.c
> > index cc12735..b64916e 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
> >  bool_t __read_mostly iommu_debug;
> >  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> > 
> > +static char __initdata misc_rmrr[100];
> 
> define a macro.
> 
> > +string_param("rmrr", misc_rmrr);
> > +
> >  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> > 
> >  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> > @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
> >      .desc = "dump iommu p2m table"
> >  };
> > 
> > +/*
> > + * List of command line defined rmrr units.
> > + */
> > +__initdata LIST_HEAD(misc_rmrr_units);
> > +
> > +/*
> > + * Parse rmrr Xen command line options and add parsed
> > + * device and region into apci_rmrr_unit list to mapped
> > + * as RMRRs parsed from ACPI.
> > + * Format: rmrr=[sbdf]start<:end>,[sbdf]start:<end>
> > + * end address can be ommited and one page will be used
> > + * for mapping with start pfn.
> > + */
> > +static void __init parse_iommu_extra_rmrr(const char *s)
> > +{
> > +    unsigned int idx = 0, found = 0;
> > +    struct misc_rmrr_unit *rmrru;
> > +    unsigned int seg, bus, dev, func;
> > +    const char *str, *cur;
> > +    u64 start, end;
> > +
> > +    do {
> > +        if ( idx >= 10 )
> > +            break;
> 
> as Konrad pointed out, using 10 and earlier 100 are not readable
> and error prone.
> 
> > +
> > +        if ( *s != '[' )
> > +            break;
> > +
> > +        str = s;
> > +        seg = bus = dev = func = 0;
> > +        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
> > +        if ( !str )
> > +        {
> > +            str = strchr(s, ']');
> > +            if ( !str )
> > +                return;
> > +        }
> > +        else
> > +            found = 1;
> > +
> > +        s = str;
> > +        if ( *s != ']' )
> > +            return;
> 
> better to have some warn message for malformat.
> 
> > +
> > +        start = simple_strtoull(cur = s + 1, &s, 0);
> > +        if ( cur == s )
> > +            break;
> > +
> > +        if ( *s == ':' )
> > +        {
> > +            end = simple_strtoull(cur = s + 1, &s, 0);
> > +            if ( cur == s )
> > +                break;
> > +        }
> > +        else
> > +            end = start;
> > +
> > +        if ( !found )
> > +            continue;
> > +
> > +        if ( end >= start )
> > +        {
> > +            rmrru = xzalloc(struct misc_rmrr_unit);
> > +            if ( !rmrru )
> > +                return;
> > +            rmrru->base_address = start << PAGE_SHIFT;
> > +            rmrru->end_address = (end << PAGE_SHIFT) + 1;
> > +            rmrru->segment = seg;
> > +            rmrru->device = PCI_BDF(bus, dev, func);
> > +            list_add(&rmrru->list, &misc_rmrr_units);
> > +        }
> > +        else
> > +        {
> > +            printk(XENLOG_WARNING "Bad rmrr: start >
> > end, %"PRIx64" > %"PRIx64"\n",
> > +                   start, end);
> > +            break;
> > +        }
> > +        idx++;
> > +    } while ( *s++ == ',' );
> > +}
> > +
> >  static void __init parse_iommu_param(char *s)
> >  {
> >      char *ss;
> > @@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain
> > *d)
> >      if ( !iommu_enabled )
> >          return;
> > 
> > +    parse_iommu_extra_rmrr(misc_rmrr);
> > +
> >      register_keyhandler('o', &iommu_p2m_table);
> >      d->need_iommu = !!iommu_dom0_strict;
> >      if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index 2e113d7..9cb6e73 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain
> > *d)
> >      return 0;
> >  }
> > 
> > +static void add_misc_rmrr(void)
> > +{
> > +    struct acpi_rmrr_unit *rmrrn;
> > +    struct misc_rmrr_unit *rmrru, *r;
> > +
> > +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> > +    {
> > +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> > +        if ( !rmrrn )
> > +            goto free;
> > +
> > +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> > +        if ( !rmrrn->scope.devices )
> > +        {
> > +            xfree(rmrrn);
> > +            goto free;
> > +        }
> > +        rmrrn->scope.devices_cnt = 1;
> > +        rmrrn->segment = rmrru->segment;
> > +        rmrrn->scope.devices[0] = rmrru->device;
> 
> need handle one-rmrr-multiple-deviecs. even if you don't want
> to support it, need capture user attempts at least.

Kevin, on the second thought, I think to support multiple devices 
per one rmrr one need to put on command line same address/range and
specify unique device each time. 

The specified region will me mapped within iommu only once if start end
end are the same (and will just increase the count of total mappings).
Taking into account that it will be rarely used, we may want to skip
adding more code?
 
> 
> > +
> > +        if ( register_one_rmrr(rmrrn) )
> > +        {
> > +            xfree(rmrrn->scope.devices);
> > +            xfree(rmrrn);
> > +        }
> > + free:
> > +        list_del(&rmrru->list);
> > +        xfree(rmrru);
> > +    }
> > +}
> > +
> >  static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
> >  {
> >      struct acpi_drhd_unit *drhd;
> > @@ -1243,6 +1275,7 @@ static void __hwdom_init
> > intel_iommu_hwdom_init(struct domain *d)
> >      }
> > 
> >      setup_hwdom_pci_devices(d, setup_hwdom_device);
> > +    add_misc_rmrr();
> >      setup_hwdom_rmrr(d);
> > 
> >      iommu_flush_all();
> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> > b/xen/drivers/passthrough/vtd/iommu.h
> > index c3e5181..b3c1b59 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > @@ -484,6 +484,7 @@ struct qinval_entry {
> >  extern struct list_head acpi_drhd_units;
> >  extern struct list_head acpi_rmrr_units;
> >  extern struct list_head acpi_ioapic_units;
> > +extern struct list_head misc_rmrr_units;
> > 
> >  struct qi_ctrl {
> >      u64 qinval_maddr;  /* queue invalidation page machine address */
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 8eb764a..2c43956 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t,
> > iommu_dont_flush_iotlb);
> >  extern struct spinlock iommu_pt_cleanup_lock;
> >  extern struct page_list_head iommu_pt_cleanup_list;
> > 
> > +/*RMRR units derived from command line rmrr option */
> > +struct misc_rmrr_unit {
> > +    struct list_head list;
> > +    u64    base_address;
> > +    u64    end_address;
> > +    u16    segment;
> > +    u16    device;
> > +};
> > +
> >  #endif /* _IOMMU_H_ */
> > --
> > 1.7.10.4
> 

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Jan Beulich]

>>> On 10.03.15 at 17:16, <elena.ufimtseva@oracle.com> wrote:
> On Tue, Mar 10, 2015 at 02:47:24AM +0000, Tian, Kevin wrote:
>> > From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
>> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> > @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain
>> > *d)
>> >      return 0;
>> >  }
>> > 
>> > +static void add_misc_rmrr(void)
>> > +{
>> > +    struct acpi_rmrr_unit *rmrrn;
>> > +    struct misc_rmrr_unit *rmrru, *r;
>> > +
>> > +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
>> > +    {
>> > +        rmrrn = xzalloc(struct acpi_rmrr_unit);
>> > +        if ( !rmrrn )
>> > +            goto free;
>> > +
>> > +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
>> > +        if ( !rmrrn->scope.devices )
>> > +        {
>> > +            xfree(rmrrn);
>> > +            goto free;
>> > +        }
>> > +        rmrrn->scope.devices_cnt = 1;
>> > +        rmrrn->segment = rmrru->segment;
>> > +        rmrrn->scope.devices[0] = rmrru->device;
>> 
>> need handle one-rmrr-multiple-deviecs. even if you don't want
>> to support it, need capture user attempts at least.
> 
> Kevin, on the second thought, I think to support multiple devices 
> per one rmrr one need to put on command line same address/range and
> specify unique device each time. 

Why? Iirc it was you who already proposed a way to properly
express this on the command line without having to repeat the
memory addresses.

Jan

Re: [PATCH 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Elena Ufimtseva]

On Tue, Mar 10, 2015 at 04:27:15PM +0000, Jan Beulich wrote:
> >>> On 10.03.15 at 17:16, <elena.ufimtseva@oracle.com> wrote:
> > On Tue, Mar 10, 2015 at 02:47:24AM +0000, Tian, Kevin wrote:
> >> > From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain
> >> > *d)
> >> >      return 0;
> >> >  }
> >> > 
> >> > +static void add_misc_rmrr(void)
> >> > +{
> >> > +    struct acpi_rmrr_unit *rmrrn;
> >> > +    struct misc_rmrr_unit *rmrru, *r;
> >> > +
> >> > +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> >> > +    {
> >> > +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> >> > +        if ( !rmrrn )
> >> > +            goto free;
> >> > +
> >> > +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> >> > +        if ( !rmrrn->scope.devices )
> >> > +        {
> >> > +            xfree(rmrrn);
> >> > +            goto free;
> >> > +        }
> >> > +        rmrrn->scope.devices_cnt = 1;
> >> > +        rmrrn->segment = rmrru->segment;
> >> > +        rmrrn->scope.devices[0] = rmrru->device;
> >> 
> >> need handle one-rmrr-multiple-deviecs. even if you don't want
> >> to support it, need capture user attempts at least.
> > 
> > Kevin, on the second thought, I think to support multiple devices 
> > per one rmrr one need to put on command line same address/range and
> > specify unique device each time. 
> 
> Why? Iirc it was you who already proposed a way to properly
> express this on the command line without having to repeat the
> memory addresses.

One more thought and exploring options as I dont have strong inclination
to either of the options.

> 
> Jan
> 

Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function

[Elena Ufimtseva]

----- elena.ufimtseva@oracle.com wrote:

> On Wed, Mar 11, 2015 at 10:29:54AM +0000, Jan Beulich wrote:
> > >>> On 09.03.15 at 15:42, <elena.ufimtseva@oracle.com> wrote:
> > > --- a/xen/drivers/passthrough/vtd/dmar.c
> > > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > > @@ -567,6 +567,66 @@ out:
> > >      return ret;
> > >  }
> > >  
> > > +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> > 
> > static - I don't see why the command line processing in patch 2
> can't
> > be put in this file.
> 
> I wanted to keep it close to iommu command line parser in iommu.c.
> Andrew mentioned that the parser should be defined as custom_param.
> Did you mean that as well and that it should be moved to dmar.c?

Never mind, I saw the answer.

> 
> > 
> > > +{
> > > +    u8 b, d, f;
> > > +    bool_t ignore = 0;
> > > +    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++ )
> > > +    {
> > > +        b = PCI_BUS(rmrru->scope.devices[i]);
> > > +        d = PCI_SLOT(rmrru->scope.devices[i]);
> > > +        f = PCI_FUNC(rmrru->scope.devices[i]);
> > 
> > Please make this the declarations (with initializers) of these
> variables;
> > they don't appear to be used outside this scope.
> > 
> > > +
> > > +        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 = 1;
> > > +        }
> > > +        else
> > > +        {
> > > +            ignore = 0;
> > > +            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);
> > > +        ret = -EFAULT;
> > > +    }
> > > +    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);
> > > +        ret = -EFAULT;
> > > +    }
> > > +    else
> > > +    {
> > > +        if ( iommu_verbose )
> > > +            dprintk(VTDPREFIX,
> > > +                    "  RMRR region: base_addr %"PRIx64
> > > +                    " end_address %"PRIx64"\n",
> > > +                    rmrru->base_address, rmrru->end_address);
> > > +        acpi_register_rmrr_unit(rmrru);
> > > +    }
> > > +    return ret;
> > 
> > Blank line before return please.
> > 
> > > @@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header
> *header)
> > >                                 &rmrru->scope, RMRR_TYPE,
> rmrr->segment);
> > >  
> > >      if ( ret || (rmrru->scope.devices_cnt == 0) )
> > > -        xfree(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) == 0
> )
> > > -            {
> > > -                dprintk(XENLOG_WARNING VTDPREFIX,
> > > -                        " Non-existent device (%04x:%02x:%02x.%u)
> is 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 )
> > > -        {
> > > -            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);
> > > -            xfree(rmrru);
> > > -        }
> > > -        else if ( base_addr > end_addr )
> > > -        {
> > > -            dprintk(XENLOG_WARNING VTDPREFIX,
> > > -                "  The RMRR (%"PRIx64", %"PRIx64") is
> incorrect!\n",
> > > -                rmrru->base_address, rmrru->end_address);
> > > -            xfree(rmrru);
> > > -            ret = -EFAULT;
> > > -        }
> > > -        else
> > > -        {
> > > -            if ( iommu_verbose )
> > > -                dprintk(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;
> > >      }
> > >  
> > > +    ret = register_one_rmrr(rmrru);
> > > +    if ( ret )
> > > +        xfree(rmrru);
> > >      return ret;
> > 
> > Please try to make this a single xfree() and a single return (with
> > again a blank line preceding it).
> 
> Got it, I will make changes you mentioned.
> > 
> > Jan