[PATCH v2] Intel IOMMU patch to reprocess RMRR info

[PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Tom Mingarelli]

When a 32bit PCI device is removed from the SI Domain, the RMRR information
for this device becomes invalid and needs to be reprocessed to avoid DMA
Read errors. These errors are evidenced by the Present bit being cleared in
the device's context entry. Fixing this problem with an enhancement to process
the RMRR info when the device is assigned to another domain. The Bus Master bit
is cleared during the move to another domain and during the reprocessing of
the RMRR info so no DMA can take place at this time.
----
PATCH v1: https://lkml.org/lkml/2012/6/15/204

drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 44 insertions(+), 3 deletions(-)

Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>

diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
--- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
+++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
@@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
 	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
 }
 
+static int reprocess_rmrr(struct device *dev)
+{
+	struct dmar_rmrr_unit *rmrr;
+	struct pci_dev *pdev;
+	int i, ret;
+
+	pdev = to_pci_dev(dev);
+
+	for_each_rmrr_units(rmrr) {
+		for (i = 0; i < rmrr->devices_cnt; i++) {
+			/*
+			 * Here we are just concerned with
+			 * finding the one device that was
+			 * removed from the si_domain and
+			 * re-evaluating its RMRR info.
+			 */
+			if (rmrr->devices[i] != pdev)
+				continue;
+			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
+				pci_name(pdev));
+			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
+			if (ret)
+				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
+		}
+	}
+return 0;
+}
+
 /* Check if the pdev needs to go through non-identity map and unmap process.*/
 static int iommu_no_mapping(struct device *dev)
 {
 	struct pci_dev *pdev;
-	int found;
+	int found, current_bus_master;
 
 	if (unlikely(dev->bus != &pci_bus_type))
 		return 1;
@@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
 			 * 32 bit DMA is removed from si_domain and fall back
 			 * to non-identity mapping.
 			 */
-			domain_remove_one_dev_info(si_domain, pdev);
 			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
-			       pci_name(pdev));
+				pci_name(pdev));
+			/*
+			 * If a device gets this far we need to clear the Bus
+			 * Master bit before we start moving devices from domain
+			 * to domain. We will also reset the Bus Master bit
+			 * after reprocessing the RMRR info. However, we only
+			 * do both the clearing and setting if needed.
+			 */
+			current_bus_master = pdev->is_busmaster;
+			if (current_bus_master)
+				pci_clear_master(pdev);
+			domain_remove_one_dev_info(si_domain, pdev);
+			reprocess_rmrr(dev);
+			if (current_bus_master)
+				pci_set_master(pdev);
 			return 0;
 		}
 	} else {

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Don Dutile]

On 09/18/2012 12:49 PM, Tom Mingarelli wrote:
> When a 32bit PCI device is removed from the SI Domain, the RMRR information
> for this device becomes invalid and needs to be reprocessed to avoid DMA
> Read errors. These errors are evidenced by the Present bit being cleared in
> the device's context entry. Fixing this problem with an enhancement to process
> the RMRR info when the device is assigned to another domain. The Bus Master bit
> is cleared during the move to another domain and during the reprocessing of
> the RMRR info so no DMA can take place at this time.
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>
> drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 44 insertions(+), 3 deletions(-)
>
> Signed-off-by: Thomas Mingarelli<thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
> +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>   	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>   }
>
> +static int reprocess_rmrr(struct device *dev)
> +{
> +	struct dmar_rmrr_unit *rmrr;
> +	struct pci_dev *pdev;
> +	int i, ret;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	for_each_rmrr_units(rmrr) {
> +		for (i = 0; i<  rmrr->devices_cnt; i++) {
> +			/*
> +			 * Here we are just concerned with
> +			 * finding the one device that was
> +			 * removed from the si_domain and
> +			 * re-evaluating its RMRR info.
> +			 */
> +			if (rmrr->devices[i] != pdev)
> +				continue;
> +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
> +				pci_name(pdev));
> +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> +			if (ret)
> +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
> +		}
> +	}
> +return 0;
> +}
> +
>   /* Check if the pdev needs to go through non-identity map and unmap process.*/
>   static int iommu_no_mapping(struct device *dev)
>   {
>   	struct pci_dev *pdev;
> -	int found;
> +	int found, current_bus_master;
>
>   	if (unlikely(dev->bus !=&pci_bus_type))
>   		return 1;
> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>   			 * 32 bit DMA is removed from si_domain and fall back
>   			 * to non-identity mapping.
>   			 */
> -			domain_remove_one_dev_info(si_domain, pdev);
>   			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> -			       pci_name(pdev));
> +				pci_name(pdev));
> +			/*
> +			 * If a device gets this far we need to clear the Bus
> +			 * Master bit before we start moving devices from domain
> +			 * to domain. We will also reset the Bus Master bit
> +			 * after reprocessing the RMRR info. However, we only
> +			 * do both the clearing and setting if needed.
> +			 */
> +			current_bus_master = pdev->is_busmaster;
> +			if (current_bus_master)
> +				pci_clear_master(pdev);
> +			domain_remove_one_dev_info(si_domain, pdev);
> +			reprocess_rmrr(dev);
> +			if (current_bus_master)
> +				pci_set_master(pdev);
>   			return 0;
>   		}
>   	} else {

appears to have recommended changes from v1, so looks good wrt handling devices w/rmrr.

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

[adding David Woodhouse]

On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote:
> When a 32bit PCI device is removed from the SI Domain, the RMRR information
> for this device becomes invalid and needs to be reprocessed to avoid DMA
> Read errors. These errors are evidenced by the Present bit being cleared in
> the device's context entry. Fixing this problem with an enhancement to process
> the RMRR info when the device is assigned to another domain. The Bus Master bit
> is cleared during the move to another domain and during the reprocessing of
> the RMRR info so no DMA can take place at this time.
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> 
> drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 44 insertions(+), 3 deletions(-)
> 
> Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
> 
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
> +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>  	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>  }
>  
> +static int reprocess_rmrr(struct device *dev)
> +{
> +	struct dmar_rmrr_unit *rmrr;
> +	struct pci_dev *pdev;
> +	int i, ret;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	for_each_rmrr_units(rmrr) {
> +		for (i = 0; i < rmrr->devices_cnt; i++) {
> +			/*
> +			 * Here we are just concerned with
> +			 * finding the one device that was
> +			 * removed from the si_domain and
> +			 * re-evaluating its RMRR info.
> +			 */

I'm still not sure why this comment is wrapped so tightly.

> +			if (rmrr->devices[i] != pdev)
> +				continue;
> +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
> +				pci_name(pdev));
> +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> +			if (ret)
> +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");

This could be "if (iommu_prepare_rmrr...)" because...

> +		}
> +	}
> +return 0;

Why does return anything?  Looks like it could be a void function since
you're not returning the only possible error case above and not checking
the return value below.  You're missing an indent here anyway.

> +}
> +
>  /* Check if the pdev needs to go through non-identity map and unmap process.*/
>  static int iommu_no_mapping(struct device *dev)
>  {
>  	struct pci_dev *pdev;
> -	int found;
> +	int found, current_bus_master;
>  
>  	if (unlikely(dev->bus != &pci_bus_type))
>  		return 1;
> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>  			 * 32 bit DMA is removed from si_domain and fall back
>  			 * to non-identity mapping.
>  			 */
> -			domain_remove_one_dev_info(si_domain, pdev);
>  			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> -			       pci_name(pdev));
> +				pci_name(pdev));

White space damage?  Change this to a pr_info if you really want to
touch it.

> +			/*
> +			 * If a device gets this far we need to clear the Bus
> +			 * Master bit before we start moving devices from domain
> +			 * to domain. We will also reset the Bus Master bit
> +			 * after reprocessing the RMRR info. However, we only
> +			 * do both the clearing and setting if needed.
> +			 */
> +			current_bus_master = pdev->is_busmaster;
> +			if (current_bus_master)
> +				pci_clear_master(pdev);
> +			domain_remove_one_dev_info(si_domain, pdev);
> +			reprocess_rmrr(dev);
> +			if (current_bus_master)
> +				pci_set_master(pdev);

I don't know any better way to halt DMA since we can't move the device
to a new domain atomically, but what about the other cases where we
switch domains?  For instance, what if some unsuspecting user tries to
assign the device to a guest?  I think it's generally inadvisable to
assign a devices with RMRRs to a guest, but if they do, they're going to
run into the same problem.  The RMRRs aren't reprocessed for the VM
domain and again aren't reprocessed when returned to a standard device
domain.  Even more fun, if assigned to a VM domain, RMRRs should be
added with the device and removed if the device is later detached from
the domain.

The lazy solution might be to disallow devices with RMRRs from being
attached via the IOMMU API interfaces (do we need more reasons not to
use RMRRs?).  Otherwise we need to be proactive about setting up a new
domain with RMRR entries for every case and correctly tracking RMRRs for
VM domains.  Thanks,

Alex

>  			return 0;
>  		}
>  	} else {

RE: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Mingarelli, Thomas]

Alex,

Are you suggesting that a solution is to prevent devices with RMRRs from being placed in the SI Domain in the first place (when pt mode is used)?


Tom

-----Original Message-----
From: Alex Williamson [mailto:alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org] 
Sent: Thursday, September 27, 2012 3:37 PM
To: Mingarelli, Thomas
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Knippers, Linda; Khan, Shuah; Don Dutile; David Woodhouse
Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[adding David Woodhouse]

On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote:
> When a 32bit PCI device is removed from the SI Domain, the RMRR information
> for this device becomes invalid and needs to be reprocessed to avoid DMA
> Read errors. These errors are evidenced by the Present bit being cleared in
> the device's context entry. Fixing this problem with an enhancement to process
> the RMRR info when the device is assigned to another domain. The Bus Master bit
> is cleared during the move to another domain and during the reprocessing of
> the RMRR info so no DMA can take place at this time.
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> 
> drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 44 insertions(+), 3 deletions(-)
> 
> Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
> 
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
> +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>  	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>  }
>  
> +static int reprocess_rmrr(struct device *dev)
> +{
> +	struct dmar_rmrr_unit *rmrr;
> +	struct pci_dev *pdev;
> +	int i, ret;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	for_each_rmrr_units(rmrr) {
> +		for (i = 0; i < rmrr->devices_cnt; i++) {
> +			/*
> +			 * Here we are just concerned with
> +			 * finding the one device that was
> +			 * removed from the si_domain and
> +			 * re-evaluating its RMRR info.
> +			 */

I'm still not sure why this comment is wrapped so tightly.

> +			if (rmrr->devices[i] != pdev)
> +				continue;
> +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
> +				pci_name(pdev));
> +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> +			if (ret)
> +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");

This could be "if (iommu_prepare_rmrr...)" because...

> +		}
> +	}
> +return 0;

Why does return anything?  Looks like it could be a void function since
you're not returning the only possible error case above and not checking
the return value below.  You're missing an indent here anyway.

> +}
> +
>  /* Check if the pdev needs to go through non-identity map and unmap process.*/
>  static int iommu_no_mapping(struct device *dev)
>  {
>  	struct pci_dev *pdev;
> -	int found;
> +	int found, current_bus_master;
>  
>  	if (unlikely(dev->bus != &pci_bus_type))
>  		return 1;
> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>  			 * 32 bit DMA is removed from si_domain and fall back
>  			 * to non-identity mapping.
>  			 */
> -			domain_remove_one_dev_info(si_domain, pdev);
>  			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> -			       pci_name(pdev));
> +				pci_name(pdev));

White space damage?  Change this to a pr_info if you really want to
touch it.

> +			/*
> +			 * If a device gets this far we need to clear the Bus
> +			 * Master bit before we start moving devices from domain
> +			 * to domain. We will also reset the Bus Master bit
> +			 * after reprocessing the RMRR info. However, we only
> +			 * do both the clearing and setting if needed.
> +			 */
> +			current_bus_master = pdev->is_busmaster;
> +			if (current_bus_master)
> +				pci_clear_master(pdev);
> +			domain_remove_one_dev_info(si_domain, pdev);
> +			reprocess_rmrr(dev);
> +			if (current_bus_master)
> +				pci_set_master(pdev);

I don't know any better way to halt DMA since we can't move the device
to a new domain atomically, but what about the other cases where we
switch domains?  For instance, what if some unsuspecting user tries to
assign the device to a guest?  I think it's generally inadvisable to
assign a devices with RMRRs to a guest, but if they do, they're going to
run into the same problem.  The RMRRs aren't reprocessed for the VM
domain and again aren't reprocessed when returned to a standard device
domain.  Even more fun, if assigned to a VM domain, RMRRs should be
added with the device and removed if the device is later detached from
the domain.

The lazy solution might be to disallow devices with RMRRs from being
attached via the IOMMU API interfaces (do we need more reasons not to
use RMRRs?).  Otherwise we need to be proactive about setting up a new
domain with RMRR entries for every case and correctly tracking RMRRs for
VM domains.  Thanks,

Alex

>  			return 0;
>  		}
>  	} else {

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

On Thu, 2012-09-27 at 21:10 +0000, Mingarelli, Thomas wrote:
> Alex,
> 
> Are you suggesting that a solution is to prevent devices with RMRRs
> from being placed in the SI Domain in the first place (when pt mode is
> used)?

No, it seems like it's preferable that devices with RMRRs stay in the si
domain if the device supports 64bit DMA.  They're likely to cause less
problems there and we can ignore the RMRRs in the si domain.  The
problem I'm trying to address is that the domain you're setting up with
RMRRs is only used for dma_ops (ie. host driver use).  If the device is
attached to a guest, that domain gets discarded and the device is added
to yet another domain, potentially with other devices.  That domain is
missing RMRRs, so now your device is going to generate VT-d faults.  The
device can then get removed from that domain, in which case the RMRRs
should be removed, and again a new dma_ops domain needs to get created
with RMRRs.

It really seems like RMRRs are incompatible with IOMMU API use though.
If an RMRR is setup for a VM domain, that's bad because a) it gives the
VM direct access to that range of host memory, and b) it interferes with
the guest use of the address space.  a) is also bad for isolating
devices on the host, but the spec makes it available for abuse.  For b),
it's not hard to imagine an RMRR range on the host that overlaps with
DMA'able space on the guest.  Data is read or written to the host memory
instead of the guest memory.  So maybe the right answer is to make
intel_iommu_attach_device return error if requested to act on a device
with RMRR ranges.  Thanks,

Alex

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org] 
> Sent: Thursday, September 27, 2012 3:37 PM
> To: Mingarelli, Thomas
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Knippers, Linda; Khan, Shuah; Don Dutile; David Woodhouse
> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
> 
> [adding David Woodhouse]
> 
> On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote:
> > When a 32bit PCI device is removed from the SI Domain, the RMRR information
> > for this device becomes invalid and needs to be reprocessed to avoid DMA
> > Read errors. These errors are evidenced by the Present bit being cleared in
> > the device's context entry. Fixing this problem with an enhancement to process
> > the RMRR info when the device is assigned to another domain. The Bus Master bit
> > is cleared during the move to another domain and during the reprocessing of
> > the RMRR info so no DMA can take place at this time.
> > ----
> > PATCH v1: https://lkml.org/lkml/2012/6/15/204
> > 
> > drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
> > 
> > diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> > --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
> > +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
> > @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
> >  	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
> >  }
> >  
> > +static int reprocess_rmrr(struct device *dev)
> > +{
> > +	struct dmar_rmrr_unit *rmrr;
> > +	struct pci_dev *pdev;
> > +	int i, ret;
> > +
> > +	pdev = to_pci_dev(dev);
> > +
> > +	for_each_rmrr_units(rmrr) {
> > +		for (i = 0; i < rmrr->devices_cnt; i++) {
> > +			/*
> > +			 * Here we are just concerned with
> > +			 * finding the one device that was
> > +			 * removed from the si_domain and
> > +			 * re-evaluating its RMRR info.
> > +			 */
> 
> I'm still not sure why this comment is wrapped so tightly.
> 
> > +			if (rmrr->devices[i] != pdev)
> > +				continue;
> > +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
> > +				pci_name(pdev));
> > +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> > +			if (ret)
> > +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
> 
> This could be "if (iommu_prepare_rmrr...)" because...
> 
> > +		}
> > +	}
> > +return 0;
> 
> Why does return anything?  Looks like it could be a void function since
> you're not returning the only possible error case above and not checking
> the return value below.  You're missing an indent here anyway.
> 
> > +}
> > +
> >  /* Check if the pdev needs to go through non-identity map and unmap process.*/
> >  static int iommu_no_mapping(struct device *dev)
> >  {
> >  	struct pci_dev *pdev;
> > -	int found;
> > +	int found, current_bus_master;
> >  
> >  	if (unlikely(dev->bus != &pci_bus_type))
> >  		return 1;
> > @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
> >  			 * 32 bit DMA is removed from si_domain and fall back
> >  			 * to non-identity mapping.
> >  			 */
> > -			domain_remove_one_dev_info(si_domain, pdev);
> >  			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> > -			       pci_name(pdev));
> > +				pci_name(pdev));
> 
> White space damage?  Change this to a pr_info if you really want to
> touch it.
> 
> > +			/*
> > +			 * If a device gets this far we need to clear the Bus
> > +			 * Master bit before we start moving devices from domain
> > +			 * to domain. We will also reset the Bus Master bit
> > +			 * after reprocessing the RMRR info. However, we only
> > +			 * do both the clearing and setting if needed.
> > +			 */
> > +			current_bus_master = pdev->is_busmaster;
> > +			if (current_bus_master)
> > +				pci_clear_master(pdev);
> > +			domain_remove_one_dev_info(si_domain, pdev);
> > +			reprocess_rmrr(dev);
> > +			if (current_bus_master)
> > +				pci_set_master(pdev);
> 
> I don't know any better way to halt DMA since we can't move the device
> to a new domain atomically, but what about the other cases where we
> switch domains?  For instance, what if some unsuspecting user tries to
> assign the device to a guest?  I think it's generally inadvisable to
> assign a devices with RMRRs to a guest, but if they do, they're going to
> run into the same problem.  The RMRRs aren't reprocessed for the VM
> domain and again aren't reprocessed when returned to a standard device
> domain.  Even more fun, if assigned to a VM domain, RMRRs should be
> added with the device and removed if the device is later detached from
> the domain.
> 
> The lazy solution might be to disallow devices with RMRRs from being
> attached via the IOMMU API interfaces (do we need more reasons not to
> use RMRRs?).  Otherwise we need to be proactive about setting up a new
> domain with RMRR entries for every case and correctly tracking RMRRs for
> VM domains.  Thanks,
> 
> Alex
> 
> >  			return 0;
> >  		}
> >  	} else {
> 
> 
> 

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1875 bytes --]

On Thu, 2012-09-27 at 15:34 -0600, Alex Williamson wrote:
> It really seems like RMRRs are incompatible with IOMMU API use though.
> If an RMRR is setup for a VM domain, that's bad because a) it gives the
> VM direct access to that range of host memory, and b) it interferes with
> the guest use of the address space.  a) is also bad for isolating
> devices on the host, but the spec makes it available for abuse.  For b),
> it's not hard to imagine an RMRR range on the host that overlaps with
> DMA'able space on the guest.  Data is read or written to the host memory
> instead of the guest memory.  So maybe the right answer is to make
> intel_iommu_attach_device return error if requested to act on a device
> with RMRR ranges.

That would include fairly much any USB host controller. The whole RMRR
concept is completely broken and should never have been invented. The
idea that firmware-controlled DMA should continue to happen *after* the
operating system has been booted and taken control of the hardware is
just insane.

The majority of RMRR use is for USB controllers, so that firmware can
emulate a legacy keyboard for the benefit of pre-USB operating systems.
But no operating system that old is ever going to support the IOMMU
anyway, so that's just mad. But that's why we've managed to get away
with setting up the RMRRs and then tearing them down when a native
driver has actually taken control of the hardware — because at that
point, it should have been reset and whatever the firmware had
configured to do has been abandoned.

Perhaps we should have a special case for USB controllers, which are
quite happy when you drop their RMRR regions when the OS takes over, and
for any *other* device with RMRRs, have a TAINT_YOUR_FIRMWARE_IS_INSANE
and just refuse to let a native driver do anything at all with them?

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Joerg Roedel]

On Thu, Sep 27, 2012 at 10:50:05PM +0100, David Woodhouse wrote:
> That would include fairly much any USB host controller. The whole RMRR
> concept is completely broken and should never have been invented. The
> idea that firmware-controlled DMA should continue to happen *after* the
> operating system has been booted and taken control of the hardware is
> just insane.
> 
> The majority of RMRR use is for USB controllers, so that firmware can
> emulate a legacy keyboard for the benefit of pre-USB operating systems.
> But no operating system that old is ever going to support the IOMMU
> anyway, so that's just mad. But that's why we've managed to get away
> with setting up the RMRRs and then tearing them down when a native
> driver has actually taken control of the hardware — because at that
> point, it should have been reset and whatever the firmware had
> configured to do has been abandoned.

Even on modern hardware with modern (IOMMU aware) kernels there is still
this small time window when the OS has enabled the IOMMU and the USB
driver is not initialized yet. In this time window the RMRR memory
region is still necessary, no?

As I said already in another mail, I think it is safe to ignore any RMRR
requirements when we start to use a device in the OS.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1007 bytes --]

On Fri, 2012-09-28 at 11:46 +0200, Joerg Roedel wrote:
> Even on modern hardware with modern (IOMMU aware) kernels there is still
> this small time window when the OS has enabled the IOMMU and the USB
> driver is not initialized yet. In this time window the RMRR memory
> region is still necessary, no?

Yes but there's no *reason* for that. It wouldn't be that hard to ask
the firmware to quiesce all its own DMA *before* we enable the IOMMU.

> As I said already in another mail, I think it is safe to ignore any RMRR
> requirements when we start to use a device in the OS.

I think the whole point in this patch is that there is some brain-dead
hardware out there (vendor 'value subtract' I think) on which that
common-sense observation isn't actually true.

I'm all for handling that broken hardware with quirks, giving clear
messages to the user that the device(+firmware) in question is broken,
and refusing to let either the kernel *or* VM guests do any DMA with it.

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Joerg Roedel]

On Fri, Sep 28, 2012 at 11:23:23AM +0100, David Woodhouse wrote:
> On Fri, 2012-09-28 at 11:46 +0200, Joerg Roedel wrote:
> > Even on modern hardware with modern (IOMMU aware) kernels there is still
> > this small time window when the OS has enabled the IOMMU and the USB
> > driver is not initialized yet. In this time window the RMRR memory
> > region is still necessary, no?
> 
> Yes but there's no *reason* for that. It wouldn't be that hard to ask
> the firmware to quiesce all its own DMA *before* we enable the IOMMU.

True. That would have been a better approach. Some kind of IOMMU
handover from firmware to the OS.

> > As I said already in another mail, I think it is safe to ignore any RMRR
> > requirements when we start to use a device in the OS.
> 
> I think the whole point in this patch is that there is some brain-dead
> hardware out there (vendor 'value subtract' I think) on which that
> common-sense observation isn't actually true.
> 
> I'm all for handling that broken hardware with quirks, giving clear
> messages to the user that the device(+firmware) in question is broken,
> and refusing to let either the kernel *or* VM guests do any DMA with it.

Well, it is probably mostly about southbridge devices and add-on USB
controllers. Or is there any other way a given device can communicate
RMRR requirements to the firmware?

Anyway, I am fine with completly blocking DMA for those devices too. 


	Joerg

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Linda Knippers]

Alex Williamson wrote:
> On Thu, 2012-09-27 at 21:10 +0000, Mingarelli, Thomas wrote:
>> Alex,
>>
>> Are you suggesting that a solution is to prevent devices with RMRRs
>> from being placed in the SI Domain in the first place (when pt mode is
>> used)?
> 
> No, it seems like it's preferable that devices with RMRRs stay in the si
> domain if the device supports 64bit DMA.  They're likely to cause less
> problems there and we can ignore the RMRRs in the si domain.  The
> problem I'm trying to address is that the domain you're setting up with
> RMRRs is only used for dma_ops (ie. host driver use).  If the device is
> attached to a guest, that domain gets discarded and the device is added
> to yet another domain, potentially with other devices.  That domain is
> missing RMRRs, so now your device is going to generate VT-d faults.  The
> device can then get removed from that domain, in which case the RMRRs
> should be removed, and again a new dma_ops domain needs to get created
> with RMRRs.
> 
> It really seems like RMRRs are incompatible with IOMMU API use though.
> If an RMRR is setup for a VM domain, that's bad because a) it gives the
> VM direct access to that range of host memory, and b) it interferes with
> the guest use of the address space.  a) is also bad for isolating
> devices on the host, but the spec makes it available for abuse.  For b),
> it's not hard to imagine an RMRR range on the host that overlaps with
> DMA'able space on the guest.  Data is read or written to the host memory
> instead of the guest memory.  So maybe the right answer is to make
> intel_iommu_attach_device return error if requested to act on a device
> with RMRR ranges.  

That sounds like the right answer to me.  Tom, can you make that
change when you address the rest of the comments?   Or should it
be a separate patch?

-- ljk

> Thanks,
> 
> Alex
> 
>> -----Original Message-----
>> From: Alex Williamson [mailto:alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org] 
>> Sent: Thursday, September 27, 2012 3:37 PM
>> To: Mingarelli, Thomas
>> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Knippers, Linda; Khan, Shuah; Don Dutile; David Woodhouse
>> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
>>
>> [adding David Woodhouse]
>>
>> On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote:
>>> When a 32bit PCI device is removed from the SI Domain, the RMRR information
>>> for this device becomes invalid and needs to be reprocessed to avoid DMA
>>> Read errors. These errors are evidenced by the Present bit being cleared in
>>> the device's context entry. Fixing this problem with an enhancement to process
>>> the RMRR info when the device is assigned to another domain. The Bus Master bit
>>> is cleared during the move to another domain and during the reprocessing of
>>> the RMRR info so no DMA can take place at this time.
>>> ----
>>> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>>>
>>> drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
>>>  1 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
>>>
>>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
>>> --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
>>> +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
>>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>>>  	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>>>  }
>>>  
>>> +static int reprocess_rmrr(struct device *dev)
>>> +{
>>> +	struct dmar_rmrr_unit *rmrr;
>>> +	struct pci_dev *pdev;
>>> +	int i, ret;
>>> +
>>> +	pdev = to_pci_dev(dev);
>>> +
>>> +	for_each_rmrr_units(rmrr) {
>>> +		for (i = 0; i < rmrr->devices_cnt; i++) {
>>> +			/*
>>> +			 * Here we are just concerned with
>>> +			 * finding the one device that was
>>> +			 * removed from the si_domain and
>>> +			 * re-evaluating its RMRR info.
>>> +			 */
>> I'm still not sure why this comment is wrapped so tightly.
>>
>>> +			if (rmrr->devices[i] != pdev)
>>> +				continue;
>>> +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
>>> +				pci_name(pdev));
>>> +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
>>> +			if (ret)
>>> +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
>> This could be "if (iommu_prepare_rmrr...)" because...
>>
>>> +		}
>>> +	}
>>> +return 0;
>> Why does return anything?  Looks like it could be a void function since
>> you're not returning the only possible error case above and not checking
>> the return value below.  You're missing an indent here anyway.
>>
>>> +}
>>> +
>>>  /* Check if the pdev needs to go through non-identity map and unmap process.*/
>>>  static int iommu_no_mapping(struct device *dev)
>>>  {
>>>  	struct pci_dev *pdev;
>>> -	int found;
>>> +	int found, current_bus_master;
>>>  
>>>  	if (unlikely(dev->bus != &pci_bus_type))
>>>  		return 1;
>>> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>>>  			 * 32 bit DMA is removed from si_domain and fall back
>>>  			 * to non-identity mapping.
>>>  			 */
>>> -			domain_remove_one_dev_info(si_domain, pdev);
>>>  			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
>>> -			       pci_name(pdev));
>>> +				pci_name(pdev));
>> White space damage?  Change this to a pr_info if you really want to
>> touch it.
>>
>>> +			/*
>>> +			 * If a device gets this far we need to clear the Bus
>>> +			 * Master bit before we start moving devices from domain
>>> +			 * to domain. We will also reset the Bus Master bit
>>> +			 * after reprocessing the RMRR info. However, we only
>>> +			 * do both the clearing and setting if needed.
>>> +			 */
>>> +			current_bus_master = pdev->is_busmaster;
>>> +			if (current_bus_master)
>>> +				pci_clear_master(pdev);
>>> +			domain_remove_one_dev_info(si_domain, pdev);
>>> +			reprocess_rmrr(dev);
>>> +			if (current_bus_master)
>>> +				pci_set_master(pdev);
>> I don't know any better way to halt DMA since we can't move the device
>> to a new domain atomically, but what about the other cases where we
>> switch domains?  For instance, what if some unsuspecting user tries to
>> assign the device to a guest?  I think it's generally inadvisable to
>> assign a devices with RMRRs to a guest, but if they do, they're going to
>> run into the same problem.  The RMRRs aren't reprocessed for the VM
>> domain and again aren't reprocessed when returned to a standard device
>> domain.  Even more fun, if assigned to a VM domain, RMRRs should be
>> added with the device and removed if the device is later detached from
>> the domain.
>>
>> The lazy solution might be to disallow devices with RMRRs from being
>> attached via the IOMMU API interfaces (do we need more reasons not to
>> use RMRRs?).  Otherwise we need to be proactive about setting up a new
>> domain with RMRR entries for every case and correctly tracking RMRRs for
>> VM domains.  Thanks,
>>
>> Alex
>>
>>>  			return 0;
>>>  		}
>>>  	} else {
>>
>>
> 
> 
> 

RE: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Mingarelli, Thomas]

I think this sounds like a separate patch.


Tom

-----Original Message-----
From: Knippers, Linda 
Sent: Thursday, September 27, 2012 4:51 PM
To: Alex Williamson
Cc: Mingarelli, Thomas; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Khan, Shuah; Don Dutile; David Woodhouse
Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

Alex Williamson wrote:
> On Thu, 2012-09-27 at 21:10 +0000, Mingarelli, Thomas wrote:
>> Alex,
>>
>> Are you suggesting that a solution is to prevent devices with RMRRs
>> from being placed in the SI Domain in the first place (when pt mode is
>> used)?
> 
> No, it seems like it's preferable that devices with RMRRs stay in the si
> domain if the device supports 64bit DMA.  They're likely to cause less
> problems there and we can ignore the RMRRs in the si domain.  The
> problem I'm trying to address is that the domain you're setting up with
> RMRRs is only used for dma_ops (ie. host driver use).  If the device is
> attached to a guest, that domain gets discarded and the device is added
> to yet another domain, potentially with other devices.  That domain is
> missing RMRRs, so now your device is going to generate VT-d faults.  The
> device can then get removed from that domain, in which case the RMRRs
> should be removed, and again a new dma_ops domain needs to get created
> with RMRRs.
> 
> It really seems like RMRRs are incompatible with IOMMU API use though.
> If an RMRR is setup for a VM domain, that's bad because a) it gives the
> VM direct access to that range of host memory, and b) it interferes with
> the guest use of the address space.  a) is also bad for isolating
> devices on the host, but the spec makes it available for abuse.  For b),
> it's not hard to imagine an RMRR range on the host that overlaps with
> DMA'able space on the guest.  Data is read or written to the host memory
> instead of the guest memory.  So maybe the right answer is to make
> intel_iommu_attach_device return error if requested to act on a device
> with RMRR ranges.  

That sounds like the right answer to me.  Tom, can you make that
change when you address the rest of the comments?   Or should it
be a separate patch?

-- ljk

> Thanks,
> 
> Alex
> 
>> -----Original Message-----
>> From: Alex Williamson [mailto:alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org] 
>> Sent: Thursday, September 27, 2012 3:37 PM
>> To: Mingarelli, Thomas
>> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Knippers, Linda; Khan, Shuah; Don Dutile; David Woodhouse
>> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
>>
>> [adding David Woodhouse]
>>
>> On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote:
>>> When a 32bit PCI device is removed from the SI Domain, the RMRR information
>>> for this device becomes invalid and needs to be reprocessed to avoid DMA
>>> Read errors. These errors are evidenced by the Present bit being cleared in
>>> the device's context entry. Fixing this problem with an enhancement to process
>>> the RMRR info when the device is assigned to another domain. The Bus Master bit
>>> is cleared during the move to another domain and during the reprocessing of
>>> the RMRR info so no DMA can take place at this time.
>>> ----
>>> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>>>
>>> drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
>>>  1 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
>>>
>>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
>>> --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
>>> +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
>>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>>>  	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>>>  }
>>>  
>>> +static int reprocess_rmrr(struct device *dev)
>>> +{
>>> +	struct dmar_rmrr_unit *rmrr;
>>> +	struct pci_dev *pdev;
>>> +	int i, ret;
>>> +
>>> +	pdev = to_pci_dev(dev);
>>> +
>>> +	for_each_rmrr_units(rmrr) {
>>> +		for (i = 0; i < rmrr->devices_cnt; i++) {
>>> +			/*
>>> +			 * Here we are just concerned with
>>> +			 * finding the one device that was
>>> +			 * removed from the si_domain and
>>> +			 * re-evaluating its RMRR info.
>>> +			 */
>> I'm still not sure why this comment is wrapped so tightly.
>>
>>> +			if (rmrr->devices[i] != pdev)
>>> +				continue;
>>> +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
>>> +				pci_name(pdev));
>>> +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
>>> +			if (ret)
>>> +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
>> This could be "if (iommu_prepare_rmrr...)" because...
>>
>>> +		}
>>> +	}
>>> +return 0;
>> Why does return anything?  Looks like it could be a void function since
>> you're not returning the only possible error case above and not checking
>> the return value below.  You're missing an indent here anyway.
>>
>>> +}
>>> +
>>>  /* Check if the pdev needs to go through non-identity map and unmap process.*/
>>>  static int iommu_no_mapping(struct device *dev)
>>>  {
>>>  	struct pci_dev *pdev;
>>> -	int found;
>>> +	int found, current_bus_master;
>>>  
>>>  	if (unlikely(dev->bus != &pci_bus_type))
>>>  		return 1;
>>> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>>>  			 * 32 bit DMA is removed from si_domain and fall back
>>>  			 * to non-identity mapping.
>>>  			 */
>>> -			domain_remove_one_dev_info(si_domain, pdev);
>>>  			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
>>> -			       pci_name(pdev));
>>> +				pci_name(pdev));
>> White space damage?  Change this to a pr_info if you really want to
>> touch it.
>>
>>> +			/*
>>> +			 * If a device gets this far we need to clear the Bus
>>> +			 * Master bit before we start moving devices from domain
>>> +			 * to domain. We will also reset the Bus Master bit
>>> +			 * after reprocessing the RMRR info. However, we only
>>> +			 * do both the clearing and setting if needed.
>>> +			 */
>>> +			current_bus_master = pdev->is_busmaster;
>>> +			if (current_bus_master)
>>> +				pci_clear_master(pdev);
>>> +			domain_remove_one_dev_info(si_domain, pdev);
>>> +			reprocess_rmrr(dev);
>>> +			if (current_bus_master)
>>> +				pci_set_master(pdev);
>> I don't know any better way to halt DMA since we can't move the device
>> to a new domain atomically, but what about the other cases where we
>> switch domains?  For instance, what if some unsuspecting user tries to
>> assign the device to a guest?  I think it's generally inadvisable to
>> assign a devices with RMRRs to a guest, but if they do, they're going to
>> run into the same problem.  The RMRRs aren't reprocessed for the VM
>> domain and again aren't reprocessed when returned to a standard device
>> domain.  Even more fun, if assigned to a VM domain, RMRRs should be
>> added with the device and removed if the device is later detached from
>> the domain.
>>
>> The lazy solution might be to disallow devices with RMRRs from being
>> attached via the IOMMU API interfaces (do we need more reasons not to
>> use RMRRs?).  Otherwise we need to be proactive about setting up a new
>> domain with RMRR entries for every case and correctly tracking RMRRs for
>> VM domains.  Thanks,
>>
>> Alex
>>
>>>  			return 0;
>>>  		}
>>>  	} else {
>>
>>
> 
> 
> 

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

On Thu, 2012-09-27 at 17:50 -0400, Linda Knippers wrote:
> Alex Williamson wrote:
> > On Thu, 2012-09-27 at 21:10 +0000, Mingarelli, Thomas wrote:
> >> Alex,
> >>
> >> Are you suggesting that a solution is to prevent devices with RMRRs
> >> from being placed in the SI Domain in the first place (when pt mode is
> >> used)?
> > 
> > No, it seems like it's preferable that devices with RMRRs stay in the si
> > domain if the device supports 64bit DMA.  They're likely to cause less
> > problems there and we can ignore the RMRRs in the si domain.  The
> > problem I'm trying to address is that the domain you're setting up with
> > RMRRs is only used for dma_ops (ie. host driver use).  If the device is
> > attached to a guest, that domain gets discarded and the device is added
> > to yet another domain, potentially with other devices.  That domain is
> > missing RMRRs, so now your device is going to generate VT-d faults.  The
> > device can then get removed from that domain, in which case the RMRRs
> > should be removed, and again a new dma_ops domain needs to get created
> > with RMRRs.
> > 
> > It really seems like RMRRs are incompatible with IOMMU API use though.
> > If an RMRR is setup for a VM domain, that's bad because a) it gives the
> > VM direct access to that range of host memory, and b) it interferes with
> > the guest use of the address space.  a) is also bad for isolating
> > devices on the host, but the spec makes it available for abuse.  For b),
> > it's not hard to imagine an RMRR range on the host that overlaps with
> > DMA'able space on the guest.  Data is read or written to the host memory
> > instead of the guest memory.  So maybe the right answer is to make
> > intel_iommu_attach_device return error if requested to act on a device
> > with RMRR ranges.  
> 
> That sounds like the right answer to me.  Tom, can you make that
> change when you address the rest of the comments?   Or should it
> be a separate patch?

Please include a pr_info when intel_iommu_attach_device takes this path
or we're going to have a really hard time figuring out why a device
won't attach to a guest.  Thanks,

Alex

> >> -----Original Message-----
> >> From: Alex Williamson [mailto:alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org] 
> >> Sent: Thursday, September 27, 2012 3:37 PM
> >> To: Mingarelli, Thomas
> >> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Knippers, Linda; Khan, Shuah; Don Dutile; David Woodhouse
> >> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
> >>
> >> [adding David Woodhouse]
> >>
> >> On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote:
> >>> When a 32bit PCI device is removed from the SI Domain, the RMRR information
> >>> for this device becomes invalid and needs to be reprocessed to avoid DMA
> >>> Read errors. These errors are evidenced by the Present bit being cleared in
> >>> the device's context entry. Fixing this problem with an enhancement to process
> >>> the RMRR info when the device is assigned to another domain. The Bus Master bit
> >>> is cleared during the move to another domain and during the reprocessing of
> >>> the RMRR info so no DMA can take place at this time.
> >>> ----
> >>> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> >>>
> >>> drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
> >>>  1 files changed, 44 insertions(+), 3 deletions(-)
> >>>
> >>> Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
> >>>
> >>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> >>> --- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
> >>> +++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
> >>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
> >>>  	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
> >>>  }
> >>>  
> >>> +static int reprocess_rmrr(struct device *dev)
> >>> +{
> >>> +	struct dmar_rmrr_unit *rmrr;
> >>> +	struct pci_dev *pdev;
> >>> +	int i, ret;
> >>> +
> >>> +	pdev = to_pci_dev(dev);
> >>> +
> >>> +	for_each_rmrr_units(rmrr) {
> >>> +		for (i = 0; i < rmrr->devices_cnt; i++) {
> >>> +			/*
> >>> +			 * Here we are just concerned with
> >>> +			 * finding the one device that was
> >>> +			 * removed from the si_domain and
> >>> +			 * re-evaluating its RMRR info.
> >>> +			 */
> >> I'm still not sure why this comment is wrapped so tightly.
> >>
> >>> +			if (rmrr->devices[i] != pdev)
> >>> +				continue;
> >>> +			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
> >>> +				pci_name(pdev));
> >>> +			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> >>> +			if (ret)
> >>> +				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
> >> This could be "if (iommu_prepare_rmrr...)" because...
> >>
> >>> +		}
> >>> +	}
> >>> +return 0;
> >> Why does return anything?  Looks like it could be a void function since
> >> you're not returning the only possible error case above and not checking
> >> the return value below.  You're missing an indent here anyway.
> >>
> >>> +}
> >>> +
> >>>  /* Check if the pdev needs to go through non-identity map and unmap process.*/
> >>>  static int iommu_no_mapping(struct device *dev)
> >>>  {
> >>>  	struct pci_dev *pdev;
> >>> -	int found;
> >>> +	int found, current_bus_master;
> >>>  
> >>>  	if (unlikely(dev->bus != &pci_bus_type))
> >>>  		return 1;
> >>> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
> >>>  			 * 32 bit DMA is removed from si_domain and fall back
> >>>  			 * to non-identity mapping.
> >>>  			 */
> >>> -			domain_remove_one_dev_info(si_domain, pdev);
> >>>  			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> >>> -			       pci_name(pdev));
> >>> +				pci_name(pdev));
> >> White space damage?  Change this to a pr_info if you really want to
> >> touch it.
> >>
> >>> +			/*
> >>> +			 * If a device gets this far we need to clear the Bus
> >>> +			 * Master bit before we start moving devices from domain
> >>> +			 * to domain. We will also reset the Bus Master bit
> >>> +			 * after reprocessing the RMRR info. However, we only
> >>> +			 * do both the clearing and setting if needed.
> >>> +			 */
> >>> +			current_bus_master = pdev->is_busmaster;
> >>> +			if (current_bus_master)
> >>> +				pci_clear_master(pdev);
> >>> +			domain_remove_one_dev_info(si_domain, pdev);
> >>> +			reprocess_rmrr(dev);
> >>> +			if (current_bus_master)
> >>> +				pci_set_master(pdev);
> >> I don't know any better way to halt DMA since we can't move the device
> >> to a new domain atomically, but what about the other cases where we
> >> switch domains?  For instance, what if some unsuspecting user tries to
> >> assign the device to a guest?  I think it's generally inadvisable to
> >> assign a devices with RMRRs to a guest, but if they do, they're going to
> >> run into the same problem.  The RMRRs aren't reprocessed for the VM
> >> domain and again aren't reprocessed when returned to a standard device
> >> domain.  Even more fun, if assigned to a VM domain, RMRRs should be
> >> added with the device and removed if the device is later detached from
> >> the domain.
> >>
> >> The lazy solution might be to disallow devices with RMRRs from being
> >> attached via the IOMMU API interfaces (do we need more reasons not to
> >> use RMRRs?).  Otherwise we need to be proactive about setting up a new
> >> domain with RMRR entries for every case and correctly tracking RMRRs for
> >> VM domains.  Thanks,
> >>
> >> Alex
> >>
> >>>  			return 0;
> >>>  		}
> >>>  	} else {
> >>
> >>
> > 
> > 
> > 
> 

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Joerg Roedel]

On Thu, Sep 27, 2012 at 03:34:07PM -0600, Alex Williamson wrote:

> It really seems like RMRRs are incompatible with IOMMU API use
> though.

I don't think so. The concept of RMRR is just not defined well enough
(like the concept of unity mappings on the AMD side which is similar to
 RMRR). The definition says, that any memory region must be mapped at
any time for the device. But that is not true (at least I have no
counter-example yet). The right definition would be, that the RMRR
regions are only necessary as long as the operating system does not
control the particular device. And assigning a device to a guest also
counts a 'taking control over the device'.


	Joerg

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote:
> On Thu, Sep 27, 2012 at 03:34:07PM -0600, Alex Williamson wrote:
> 
> > It really seems like RMRRs are incompatible with IOMMU API use
> > though.
> 
> I don't think so. The concept of RMRR is just not defined well enough
> (like the concept of unity mappings on the AMD side which is similar to
>  RMRR). The definition says, that any memory region must be mapped at
> any time for the device. But that is not true (at least I have no
> counter-example yet). The right definition would be, that the RMRR
> regions are only necessary as long as the operating system does not
> control the particular device. And assigning a device to a guest also
> counts a 'taking control over the device'.

I think HP folks would be very unhappy with that definition.  As David
indicates, that's how things like USB use RMRR, but the actual
definition in the spec leaves much more room for abuse.  Thanks,

Alex

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Joerg Roedel]

On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote:
> On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote:

> > I don't think so. The concept of RMRR is just not defined well enough
> > (like the concept of unity mappings on the AMD side which is similar to
> >  RMRR). The definition says, that any memory region must be mapped at
> > any time for the device. But that is not true (at least I have no
> > counter-example yet). The right definition would be, that the RMRR
> > regions are only necessary as long as the operating system does not
> > control the particular device. And assigning a device to a guest also
> > counts a 'taking control over the device'.
> 
> I think HP folks would be very unhappy with that definition.  As David
> indicates, that's how things like USB use RMRR, but the actual
> definition in the spec leaves much more room for abuse.  Thanks,

To my experience, for a hardware designer, existing software overrides
any Spec because it is much worse to break existing software than it is
to break a Spec :) So, unless we break existing hardware/firmware, I
still suggest that we use the assumption that OS controlled devices do
not need RMRR/unity-mapped regions anymore.

Is HP doing anything in their firmware which would not work with that?

For the USB controlers, they only generate DMA to the RMRR/unity-mapped
region until the OS takes over control from the firmware. After
the USB driver is initialized the RMRR region should not be necessary
anymore.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

On Fri, 2012-09-28 at 14:52 +0200, Joerg Roedel wrote:
> On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote:
> > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote:
> 
> > > I don't think so. The concept of RMRR is just not defined well enough
> > > (like the concept of unity mappings on the AMD side which is similar to
> > >  RMRR). The definition says, that any memory region must be mapped at
> > > any time for the device. But that is not true (at least I have no
> > > counter-example yet). The right definition would be, that the RMRR
> > > regions are only necessary as long as the operating system does not
> > > control the particular device. And assigning a device to a guest also
> > > counts a 'taking control over the device'.
> > 
> > I think HP folks would be very unhappy with that definition.  As David
> > indicates, that's how things like USB use RMRR, but the actual
> > definition in the spec leaves much more room for abuse.  Thanks,
> 
> To my experience, for a hardware designer, existing software overrides
> any Spec because it is much worse to break existing software than it is
> to break a Spec :) So, unless we break existing hardware/firmware, I
> still suggest that we use the assumption that OS controlled devices do
> not need RMRR/unity-mapped regions anymore.

HP has been shipping hardware that makes use of RMRRs for other purposes
for a while.

> Is HP doing anything in their firmware which would not work with that?

Yes, I'll let them fill in the details.

> For the USB controlers, they only generate DMA to the RMRR/unity-mapped
> region until the OS takes over control from the firmware. After
> the USB driver is initialized the RMRR region should not be necessary
> anymore.

I agree that was probably the intent, but vendors have found loopholes
as their opportunity to innovate.  Thanks,

Alex

[PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Tom Mingarelli]

When a 32bit PCI device is removed from the SI Domain, the RMRR information
for this device becomes invalid and needs to be reprocessed to avoid DMA
Read errors. These errors are evidenced by the Present bit being cleared in
the device's context entry. Fixing this problem with an enhancement to process
the RMRR info when the device is assigned to another domain. The Bus Master bit
is cleared during the move to another domain and during the reprocessing of
the RMRR info so no DMA can take place at this time.
----
PATCH v1: https://lkml.org/lkml/2012/6/15/204

drivers/iommu/intel-iommu.c |   47 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 44 insertions(+), 3 deletions(-)

Signed-off-by: Thomas Mingarelli <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>

diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
--- ./drivers/iommu/intel-iommu.c.ORIG	2012-09-18 09:58:25.147976889 -0500
+++ ./drivers/iommu/intel-iommu.c	2012-09-18 10:39:43.286672765 -0500
@@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
 	return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
 }
 
+static int reprocess_rmrr(struct device *dev)
+{
+	struct dmar_rmrr_unit *rmrr;
+	struct pci_dev *pdev;
+	int i, ret;
+
+	pdev = to_pci_dev(dev);
+
+	for_each_rmrr_units(rmrr) {
+		for (i = 0; i < rmrr->devices_cnt; i++) {
+			/*
+			 * Here we are just concerned with
+			 * finding the one device that was
+			 * removed from the si_domain and
+			 * re-evaluating its RMRR info.
+			 */
+			if (rmrr->devices[i] != pdev)
+				continue;
+			pr_info("IOMMU: Reprocess RMRR information for device %s.\n",
+				pci_name(pdev));
+			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
+			if (ret)
+				pr_err("IOMMU: Reprocessing RMRR reserved region for device failed");
+		}
+	}
+return 0;
+}
+
 /* Check if the pdev needs to go through non-identity map and unmap process.*/
 static int iommu_no_mapping(struct device *dev)
 {
 	struct pci_dev *pdev;
-	int found;
+	int found, current_bus_master;
 
 	if (unlikely(dev->bus != &pci_bus_type))
 		return 1;
@@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
 			 * 32 bit DMA is removed from si_domain and fall back
 			 * to non-identity mapping.
 			 */
-			domain_remove_one_dev_info(si_domain, pdev);
 			printk(KERN_INFO "32bit %s uses non-identity mapping\n",
-			       pci_name(pdev));
+				pci_name(pdev));
+			/*
+			 * If a device gets this far we need to clear the Bus
+			 * Master bit before we start moving devices from domain
+			 * to domain. We will also reset the Bus Master bit
+			 * after reprocessing the RMRR info. However, we only
+			 * do both the clearing and setting if needed.
+			 */
+			current_bus_master = pdev->is_busmaster;
+			if (current_bus_master)
+				pci_clear_master(pdev);
+			domain_remove_one_dev_info(si_domain, pdev);
+			reprocess_rmrr(dev);
+			if (current_bus_master)
+				pci_set_master(pdev);
 			return 0;
 		}
 	} else {

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 2665 bytes --]


HP may have been shipping such things 'for a while' but it's never actually worked right, yes? This thread is about the patch that's intended to *fix* that?

If they could just manage to make their firmware-owned DMA appear to be from a different PCIe device/function from the one the OS gets to own, that would make things "just work", right? Hell, their laptops already have Ricoh multi-function devices that do their DMA  from the 'wrong' function.... take that concept and make it useful...

If the DMA could be hidden from the IOMMU altogether, all the better. But at least coming from its own dedicated devfn would avoid the issues this patch attempts to solve.



-- 
dwmw2

(Apologies for HTML and top-posting; Android mailer is broken.)Alex Williamson <alex.williamson@redhat.com> wrote:On Fri, 2012-09-28 at 14:52 +0200, Joerg Roedel wrote:
> On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote:
> > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote:
> 
> > > I don't think so. The concept of RMRR is just not defined well enough
> > > (like the concept of unity mappings on the AMD side which is similar to
> > >  RMRR). The definition says, that any memory region must be mapped at
> > > any time for the device. But that is not true (at least I have no
> > > counter-example yet). The right definition would be, that the RMRR
> > > regions are only necessary as long as the operating system does not
> > > control the particular device. And assigning a device to a guest also
> > > counts a 'taking control over the device'.
> > 
> > I think HP folks would be very unhappy with that definition.  As David
> > indicates, that's how things like USB use RMRR, but the actual
> > definition in the spec leaves much more room for abuse.  Thanks,
> 
> To my experience, for a hardware designer, existing software overrides
> any Spec because it is much worse to break existing software than it is
> to break a Spec :) So, unless we break existing hardware/firmware, I
> still suggest that we use the assumption that OS controlled devices do
> not need RMRR/unity-mapped regions anymore.

HP has been shipping hardware that makes use of RMRRs for other purposes
for a while.

> Is HP doing anything in their firmware which would not work with that?

Yes, I'll let them fill in the details.

> For the USB controlers, they only generate DMA to the RMRR/unity-mapped
> region until the OS takes over control from the firmware. After
> the USB driver is initialized the RMRR region should not be necessary
> anymore.

I agree that was probably the intent, but vendors have found loopholes
as their opportunity to innovate.  Thanks,

Alex



[-- Attachment #1.2: Type: text/html, Size: 3232 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

On Fri, 2012-09-28 at 16:52 +0100, David Woodhouse wrote:
> HP may have been shipping such things 'for a while' but it's never
> actually worked right, yes? This thread is about the patch that's
> intended to *fix* that?

I believe it currently works in non-passthrough mode since the RMRRs
stick for that.  It hasn't worked in passthrough mode since we disabled
swiotlb and started kicking 32bit dma mask devices out of the si domain.
It has probably never worked for VM usage if anyone has made the mistake
of attempting that.

> If they could just manage to make their firmware-owned DMA appear to
> be from a different PCIe device/function from the one the OS gets to
> own, that would make things "just work", right? Hell, their laptops
> already have Ricoh multi-function devices that do their DMA  from the
> 'wrong' function.... take that concept and make it useful...
> 
> If the DMA could be hidden from the IOMMU altogether, all the better.
> But at least coming from its own dedicated devfn would avoid the
> issues this patch attempts to solve.

Hmm, an RMRR for a non-existent device would cause problems when we try
to figure out the dma mask... if nothing else.  The proposal seems to
open pandora's box just a little further :-\  Maybe it would have to
live outside of device ranges claimed by the DRHDs, but that implies
physical topology changes.  Thanks,

Alex

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Linda Knippers]

David Woodhouse wrote:
> 
> HP may have been shipping such things 'for a while' but it's never
> actually worked right, yes? This thread is about the patch that's
> intended to *fix* that?

I can only speak to the HP servers.  We have been shipping devices
'for a while' that provide sensor-type data to the platform.  The
device does DMA writes to a range of memory (the RMRR) and
iLO does DMA reads of that data.

This works in general but not when the 'iommu=pt' boot option is
used.  This patch associates the RMRR with the devices when
they are moved out of the "si" domain.

Based on Alex's comments about moving RMRR devices between domains,
it sounds like we also have a problem without the 'iommu=pt' boot
option if someone assigns one of those devices to a guest.

Tom, Shuah, Alex - please correct me if I've gotten any of this wrong.

> If they could just manage to make their firmware-owned DMA appear to be
> from a different PCIe device/function from the one the OS gets to own,
> that would make things "just work", right? Hell, their laptops already
> have Ricoh multi-function devices that do their DMA  from the 'wrong'
> function.... take that concept and make it useful...
> 
> If the DMA could be hidden from the IOMMU altogether, all the better.
> But at least coming from its own dedicated devfn would avoid the issues
> this patch attempts to solve.

That does sound like a good idea and may be something that we can suggest
for the future but that won't help us today.

If we address Alex's comments and we make a change to disallow the
devices (non-USB devices?) with RMRRs from being assigned to
a guest, will those changes be considered?

-- ljk
> 
> 
> 
> -- 
> dwmw2
> 
> (Apologies for HTML and top-posting; Android mailer is broken.)
> 
> Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 2012-09-28 at 14:52 +0200, Joerg Roedel wrote:
>> On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote:
>> > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote:
>>
>> > > I don't think so. The concept of RMRR is just not defined well enough
>> > > (like the concept of unity mappings on the AMD side which is
> similar to
>> > >  RMRR). The definition says, that any memory region must be mapped at
>> > > any time for the device. But that is not true (at least I have no
>> > > counter-example yet). The right definition would be, that the RMRR
>> > > regions are only necessary as long as the operating system does not
>> > > control the particular device. And assigning a device to a guest also
>> > > counts a 'taking control over the device'.
>> >
>> > I think HP folks would be very unhappy with that definition.  As David
>> > indicates, that's how things like USB use RMRR, but the actual
>> > definition in the spec leaves much more room for abuse.  Thanks,
>>
>> To my experience, for a hardware designer, existing software overrides
>> any Spec because it is much worse to break existing software than it is
>> to break a Spec :) So, unless we break existing hardware/firmware, I
>> still suggest that we use the assumption that OS controlled devices do
>> not need RMRR/unity-mapped regions anymore.
> 
> HP has been shipping hardware that makes use of RMRRs for other purposes
> for a while.
> 
>> Is HP doing anything in their firmware which would not work with that?
> 
> Yes, I'll let them fill in the details.
> 
>> For the USB controlers, they only generate DMA to the RMRR/unity-mapped
>> region until the OS takes over control from the firmware. After
>> the USB driver is initialized the RMRR region should not be necessary
>> anymore.
> 
> I agree that was probably the intent, but vendors have found loopholes
> as their opportunity to innovate.  Thanks,
> 
> Alex
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Joerg Roedel]

On Fri, Sep 28, 2012 at 12:36:05PM -0400, Linda Knippers wrote:
> I can only speak to the HP servers.  We have been shipping devices
> 'for a while' that provide sensor-type data to the platform.  The
> device does DMA writes to a range of memory (the RMRR) and
> iLO does DMA reads of that data.

And what PCI request-ids are used for these DMA transfers? Are this
request-ids which belong to devices Linux handles on its own?

> If we address Alex's comments and we make a change to disallow the
> devices (non-USB devices?) with RMRRs from being assigned to
> a guest, will those changes be considered?

This is overkill in my eyes. It means that *any* device which has an
RMRR defined, whether it is on your platform or not, can not be assigned
to a guest.

I think it is better to have the RMRR regions mapped in the domains used
for DMA-API mappings and disallow to assign these devices to guests. For
devices where this breaks we can implement some quirk-solution and
disallow guest assignment. But disallowing assignment of devices with
RMRR defined in general is pure overkill.


	Joerg

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Joerg Roedel]

On Fri, Sep 28, 2012 at 07:01:06PM +0200, Joerg Roedel wrote:
> On Fri, Sep 28, 2012 at 12:36:05PM -0400, Linda Knippers wrote:
> > I can only speak to the HP servers.  We have been shipping devices
> > 'for a while' that provide sensor-type data to the platform.  The
> > device does DMA writes to a range of memory (the RMRR) and
> > iLO does DMA reads of that data.
> 
> And what PCI request-ids are used for these DMA transfers? Are this
> request-ids which belong to devices Linux handles on its own?
> 
> > If we address Alex's comments and we make a change to disallow the
> > devices (non-USB devices?) with RMRRs from being assigned to
> > a guest, will those changes be considered?
> 
> This is overkill in my eyes. It means that *any* device which has an
> RMRR defined, whether it is on your platform or not, can not be assigned
> to a guest.
> 
> I think it is better to have the RMRR regions mapped in the domains used
> for DMA-API mappings and disallow to assign these devices to guests.

Of course I meant "allow to assign these devices to guests."

> For devices where this breaks we can implement some quirk-solution and
> disallow guest assignment. But disallowing assignment of devices with
> RMRR defined in general is pure overkill.
> 
> 
> 	Joerg
> 
> 
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Alex Williamson]

On Fri, 2012-09-28 at 19:01 +0200, Joerg Roedel wrote:
> On Fri, Sep 28, 2012 at 12:36:05PM -0400, Linda Knippers wrote:
> > I can only speak to the HP servers.  We have been shipping devices
> > 'for a while' that provide sensor-type data to the platform.  The
> > device does DMA writes to a range of memory (the RMRR) and
> > iLO does DMA reads of that data.
> 
> And what PCI request-ids are used for these DMA transfers? Are this
> request-ids which belong to devices Linux handles on its own?
> 
> > If we address Alex's comments and we make a change to disallow the
> > devices (non-USB devices?) with RMRRs from being assigned to
> > a guest, will those changes be considered?
> 
> This is overkill in my eyes. It means that *any* device which has an
> RMRR defined, whether it is on your platform or not, can not be assigned
> to a guest.
> 
> I think it is better to have the RMRR regions mapped in the domains used
> for DMA-API mappings and disallow to assign these devices to guests. For
> devices where this breaks we can implement some quirk-solution and
> disallow guest assignment. But disallowing assignment of devices with
> RMRR defined in general is pure overkill.

On the other hand, a blanket blacklist of no assignment of RMRR devices
deters creative uses of RMRRs, which seems like a good thing.  USB
probably needs to be an exception due to widespread usage of RMRRs that
are known to not be needed.  Anything else risks the assigned device
doing autonomous writes to memory, potentially causing corruption.  The
only safe way to prevent that would be to insert a reserved memory range
into the guest to match the RMRR, maybe even identity map it if we care
about the RMRR continuing to work, but that makes hotplug of those
devices nearly impossible.  Thanks,

Alex

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1881 bytes --]

On Fri, 2012-09-28 at 12:36 -0400, Linda Knippers wrote:
> I can only speak to the HP servers.  We have been shipping devices
> 'for a while' that provide sensor-type data to the platform.  The
> device does DMA writes to a range of memory (the RMRR) and
> iLO does DMA reads of that data.
> 
> This works in general but not when the 'iommu=pt' boot option is
> used.  This patch associates the RMRR with the devices when
> they are moved out of the "si" domain.

That much makes sense, I think, because they're moved out of the
hardware SI domain *early*, when we realise they're actually only
capable of 32-bit DMA and we have >4GiB of RAM. Right?

It sounds like this isn't a case of the device being used by a native
driver or given to KVM, and subsequently released. This is just booting
up and losing the RMRR regions on a device which the OS *hasn't* really
touched. So that should be fixed.

> Based on Alex's comments about moving RMRR devices between domains,
> it sounds like we also have a problem without the 'iommu=pt' boot
> option if someone assigns one of those devices to a guest.

Yeah... but why *would* they? What possible reason would we have to
assign either the sensor device, or the iLO, to a KVM guest. Or to have
a native driver that attempts to do DMA from them?

Obviously, in an ideal world we'd have proper native drivers for the
sensor device. But I'm guessing that's not the case here; it's used by
the firmware and we're not supposed to be touching it?

And yes, obviously a better hardware design (from the OS/IOMMU point of
view) would be to have a path for the sensor data that *doesn't* go via
host RAM and thus via the IOMMU twice. But while that's a lesson that's
hopefully been learned and will be implemented in future, we have to
deal with the existing hardware and its (ab)use of RMRRs.

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

RE: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Mingarelli, Thomas]

That is correct, David. It is moved out early on when it is found to be a 32bit DMA device and its RMRR info is lost.
The movement of devices between the si domain and *others* only happens in iommu=pt mode. This si domain doesn't come into play when simply booting with intel_iommu=on.

Thanks,
Tom

-----Original Message-----
From: David Woodhouse [mailto:dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org]
Sent: Friday, September 28, 2012 2:15 PM
To: Knippers, Linda
Cc: Alex Williamson; Joerg Roedel; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Khan, Shuah; Mingarelli, Thomas
Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

On Fri, 2012-09-28 at 12:36 -0400, Linda Knippers wrote:
> I can only speak to the HP servers.  We have been shipping devices
> 'for a while' that provide sensor-type data to the platform.  The
> device does DMA writes to a range of memory (the RMRR) and
> iLO does DMA reads of that data.
>
> This works in general but not when the 'iommu=pt' boot option is
> used.  This patch associates the RMRR with the devices when
> they are moved out of the "si" domain.

That much makes sense, I think, because they're moved out of the
hardware SI domain *early*, when we realise they're actually only
capable of 32-bit DMA and we have >4GiB of RAM. Right?

It sounds like this isn't a case of the device being used by a native
driver or given to KVM, and subsequently released. This is just booting
up and losing the RMRR regions on a device which the OS *hasn't* really
touched. So that should be fixed.

> Based on Alex's comments about moving RMRR devices between domains,
> it sounds like we also have a problem without the 'iommu=pt' boot
> option if someone assigns one of those devices to a guest.

Yeah... but why *would* they? What possible reason would we have to
assign either the sensor device, or the iLO, to a KVM guest. Or to have
a native driver that attempts to do DMA from them?

Obviously, in an ideal world we'd have proper native drivers for the
sensor device. But I'm guessing that's not the case here; it's used by
the firmware and we're not supposed to be touching it?

And yes, obviously a better hardware design (from the OS/IOMMU point of
view) would be to have a path for the sensor data that *doesn't* go via
host RAM and thus via the IOMMU twice. But while that's a lesson that's
hopefully been learned and will be implemented in future, we have to
deal with the existing hardware and its (ab)use of RMRRs.

--
dwmw2

Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Shuah Khan]

On Fri, 2012-09-28 at 20:15 +0100, David Woodhouse wrote:
> On Fri, 2012-09-28 at 12:36 -0400, Linda Knippers wrote:
> > I can only speak to the HP servers.  We have been shipping devices
> > 'for a while' that provide sensor-type data to the platform.  The
> > device does DMA writes to a range of memory (the RMRR) and
> > iLO does DMA reads of that data.
> > 
> > This works in general but not when the 'iommu=pt' boot option is
> > used.  This patch associates the RMRR with the devices when
> > they are moved out of the "si" domain.
> 
> That much makes sense, I think, because they're moved out of the
> hardware SI domain *early*, when we realise they're actually only
> capable of 32-bit DMA and we have >4GiB of RAM. Right?

Correct. By default all devices are added to SI domain assuming that
these devices are 64-bit devices. When we detect the device is a 32-bit
device based on the requested dma-mask, it gets removed from SI domain,
hence looses its RMRR association. In the meantime dma continues causing
DMA errors. This patch is re-processing RMRRs for the device in question
and doing re-assignment.

> 
> It sounds like this isn't a case of the device being used by a native
> driver or given to KVM, and subsequently released. This is just booting
> up and losing the RMRR regions on a device which the OS *hasn't* really
> touched. So that should be fixed.
> 
> > Based on Alex's comments about moving RMRR devices between domains,
> > it sounds like we also have a problem without the 'iommu=pt' boot
> > option if someone assigns one of those devices to a guest.
> 
> Yeah... but why *would* they? What possible reason would we have to
> assign either the sensor device, or the iLO, to a KVM guest. Or to have
> a native driver that attempts to do DMA from them?
> 
> Obviously, in an ideal world we'd have proper native drivers for the
> sensor device. But I'm guessing that's not the case here; it's used by
> the firmware and we're not supposed to be touching it?
> 
> And yes, obviously a better hardware design (from the OS/IOMMU point of
> view) would be to have a path for the sensor data that *doesn't* go via
> host RAM and thus via the IOMMU twice. But while that's a lesson that's
> hopefully been learned and will be implemented in future, we have to
> deal with the existing hardware and its (ab)use of RMRRs.
> 

Right. We do have hardware that is relying on being able to do dma from
devices to a system RAM via RMRR.

-- Shuah