Patchwork [04/13] pci: New pci_dma_quirk()

login
register
mail settings
Submitter Alex Williamson
Date May 11, 2012, 10:55 p.m.
Message ID <20120511225555.30496.80525.stgit@bling.home>
Download mbox | patch
Permalink /patch/158671/
State New
Headers show

Comments

Alex Williamson - May 11, 2012, 10:55 p.m.
Integrating IOMMU groups more closely into the driver core allows
us to more easily work around DMA quirks.  The Ricoh multifunction
controller is a favorite example of devices that are currently
incompatible with IOMMU isolation as all the functions use the
requestor ID of function 0 for DMA.  Passing this device into
pci_dma_quirk returns the PCI device to use for DMA.  The IOMMU
driver can then construct an IOMMU group including both devices.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/quirks.c |   22 ++++++++++++++++++++++
 include/linux/pci.h  |    2 ++
 2 files changed, 24 insertions(+), 0 deletions(-)
David Gibson - May 17, 2012, 3:39 a.m.
On Fri, May 11, 2012 at 04:55:55PM -0600, Alex Williamson wrote:
> Integrating IOMMU groups more closely into the driver core allows
> us to more easily work around DMA quirks.  The Ricoh multifunction
> controller is a favorite example of devices that are currently
> incompatible with IOMMU isolation as all the functions use the
> requestor ID of function 0 for DMA.  Passing this device into
> pci_dma_quirk returns the PCI device to use for DMA.  The IOMMU
> driver can then construct an IOMMU group including both devices.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  drivers/pci/quirks.c |   22 ++++++++++++++++++++++
>  include/linux/pci.h  |    2 ++
>  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 4bf7102..6f9f7f9 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3109,3 +3109,25 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  
>  	return -ENOTTY;
>  }
> +
> +struct pci_dev *pci_dma_quirk(struct pci_dev *dev)
> +{
> +	struct pci_dev *dma_dev = dev;
> +
> +	/*
> +	 * https://bugzilla.redhat.com/show_bug.cgi?id=605888
> +	 *
> +	 * Some Ricoh devices use the function 0 source ID for DMA on
> +	 * other functions of a multifunction device.  The DMA devices
> +	 * is therefore function 0, which will have implications of the
> +	 * iommu grouping of these devices.
> +	 */
> +	if (dev->vendor == PCI_VENDOR_ID_RICOH &&
> +	    (dev->device == 0xe822 || dev->device == 0xe230 ||
> +	     dev->device == 0xe832 || dev->device == 0xe476)) {
> +		dma_dev = pci_get_slot(dev->bus,
> +				       PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> +	}

Hrm.  This seems like a very generic name for a function performing a
very specific test.  We could well have devices with the same problem
in future, so shouldn't this be set up so the same quirk can be easily
added to new device ids without changing the function code itself.
Alex Williamson - May 17, 2012, 4:06 a.m.
On Thu, 2012-05-17 at 13:39 +1000, David Gibson wrote:
> On Fri, May 11, 2012 at 04:55:55PM -0600, Alex Williamson wrote:
> > Integrating IOMMU groups more closely into the driver core allows
> > us to more easily work around DMA quirks.  The Ricoh multifunction
> > controller is a favorite example of devices that are currently
> > incompatible with IOMMU isolation as all the functions use the
> > requestor ID of function 0 for DMA.  Passing this device into
> > pci_dma_quirk returns the PCI device to use for DMA.  The IOMMU
> > driver can then construct an IOMMU group including both devices.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  drivers/pci/quirks.c |   22 ++++++++++++++++++++++
> >  include/linux/pci.h  |    2 ++
> >  2 files changed, 24 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 4bf7102..6f9f7f9 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3109,3 +3109,25 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
> >  
> >  	return -ENOTTY;
> >  }
> > +
> > +struct pci_dev *pci_dma_quirk(struct pci_dev *dev)
> > +{
> > +	struct pci_dev *dma_dev = dev;
> > +
> > +	/*
> > +	 * https://bugzilla.redhat.com/show_bug.cgi?id=605888
> > +	 *
> > +	 * Some Ricoh devices use the function 0 source ID for DMA on
> > +	 * other functions of a multifunction device.  The DMA devices
> > +	 * is therefore function 0, which will have implications of the
> > +	 * iommu grouping of these devices.
> > +	 */
> > +	if (dev->vendor == PCI_VENDOR_ID_RICOH &&
> > +	    (dev->device == 0xe822 || dev->device == 0xe230 ||
> > +	     dev->device == 0xe832 || dev->device == 0xe476)) {
> > +		dma_dev = pci_get_slot(dev->bus,
> > +				       PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> > +	}
> 
> Hrm.  This seems like a very generic name for a function performing a
> very specific test.  We could well have devices with the same problem
> in future, so shouldn't this be set up so the same quirk can be easily
> added to new device ids without changing the function code itself.

I've since added a USB quirk here to group all the USB functions in a
slot.  I'll take a closer look at the quirk helpers to see if anything
makes this easier, but I didn't see much point in spending a lot of time
over-optimizing this for 1 or 2 quirks that we can just step through in
a monolithic function.  Thanks,

Alex
Anonymous - May 17, 2012, 7:19 a.m.
Alex,

On Sat, May 12, 2012 at 6:55 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> Integrating IOMMU groups more closely into the driver core allows
> us to more easily work around DMA quirks.  The Ricoh multifunction
> controller is a favorite example of devices that are currently
> incompatible with IOMMU isolation as all the functions use the
> requestor ID of function 0 for DMA.  Passing this device into
> pci_dma_quirk returns the PCI device to use for DMA.  The IOMMU
> driver can then construct an IOMMU group including both devices.
>

Please give some thought to the Marvell SATA controller quirk as well.

Instead of multiple visible functions using the same requester ID of
function 0, the Marvell device only makes function 0 visible, but uses
the requestor ID of function 1 as well during DMA.

See https://bugzilla.redhat.com/show_bug.cgi?id=757166

--
A.
Alex Williamson - May 17, 2012, 3:22 p.m.
On Thu, 2012-05-17 at 15:19 +0800, Anonymous wrote:
> Alex,
> 
> On Sat, May 12, 2012 at 6:55 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > Integrating IOMMU groups more closely into the driver core allows
> > us to more easily work around DMA quirks.  The Ricoh multifunction
> > controller is a favorite example of devices that are currently
> > incompatible with IOMMU isolation as all the functions use the
> > requestor ID of function 0 for DMA.  Passing this device into
> > pci_dma_quirk returns the PCI device to use for DMA.  The IOMMU
> > driver can then construct an IOMMU group including both devices.
> >
> 
> Please give some thought to the Marvell SATA controller quirk as well.
> 
> Instead of multiple visible functions using the same requester ID of
> function 0, the Marvell device only makes function 0 visible, but uses
> the requestor ID of function 1 as well during DMA.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=757166

Wow.  That one isn't quite as easy to deal with since there's no
existing device in the kernel to point to.  This comment might be on the
right track:

https://bugzilla.kernel.org/show_bug.cgi?id=42679#c11

Perhaps David Woodhouse can comment on support for phantom functions.
If we had infrastructure for that it might be easy for the quirk to
update the pci_dev struct, inserting a new phantom function.  Otherwise
we'd need to create a new device in the kernel for it.  Thanks,

Alex

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 4bf7102..6f9f7f9 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3109,3 +3109,25 @@  int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 
 	return -ENOTTY;
 }
+
+struct pci_dev *pci_dma_quirk(struct pci_dev *dev)
+{
+	struct pci_dev *dma_dev = dev;
+
+	/*
+	 * https://bugzilla.redhat.com/show_bug.cgi?id=605888
+	 *
+	 * Some Ricoh devices use the function 0 source ID for DMA on
+	 * other functions of a multifunction device.  The DMA devices
+	 * is therefore function 0, which will have implications of the
+	 * iommu grouping of these devices.
+	 */
+	if (dev->vendor == PCI_VENDOR_ID_RICOH &&
+	    (dev->device == 0xe822 || dev->device == 0xe230 ||
+	     dev->device == 0xe832 || dev->device == 0xe476)) {
+		dma_dev = pci_get_slot(dev->bus,
+				       PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
+	}
+
+	return dma_dev;
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e444f5b..9910b5c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1479,9 +1479,11 @@  enum pci_fixup_pass {
 
 #ifdef CONFIG_PCI_QUIRKS
 void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev);
+struct pci_dev *pci_dma_quirk(struct pci_dev *dev);
 #else
 static inline void pci_fixup_device(enum pci_fixup_pass pass,
 				    struct pci_dev *dev) {}
+struct pci_dev *pci_dma_quirk(struct pci_dev *dev) { return dev }
 #endif
 
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);