* [acooks@gmail.com: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.]
@ 2013-03-01 14:04 Konrad Rzeszutek Wilk
2013-03-01 14:28 ` Andrew Cooper
0 siblings, 1 reply; 3+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-03-01 14:04 UTC (permalink / raw)
To: xen-devel
Should we add a similar quirk to Xen?
----- Forwarded message from Andrew Cooks <acooks@gmail.com> -----
Date: Fri, 1 Mar 2013 16:26:13 +0800
From: Andrew Cooks <acooks@gmail.com>
To: acooks@gmail.com, joro@8bytes.org, xjtuychu@hotmail.com, gm.ychu@gmail.com, alex.williamson@redhat.com, bhelgaas@google.com, jpiszcz@lucidpixels.com,
dwmw2@infradead.org
Cc: "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>, "open list:INTEL IOMMU VT-d" <iommu@lists.linux-foundation.org>, open list <linux-kernel@vger.kernel.org>
Subject: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.
This is my third submitted patch to make Marvell 88SE91xx SATA controllers work when IOMMU is enabled.[1][2]
What's changed:
* Adopt David Woodhouse's terminology by referring to the quirky functions as 'ghost' functions.
* Unmap ghost functions when device is detached from IOMMU.
* Stub function for when CONFIG_PCI_QUIRKS is not enabled.
The bad:
* Still no AMD support.
* The table of affected chip IDs is as complete as I can make it by googling for bug reports.
This patch was generated against commit b0af9cd9aab60ceb17d3ebabb9fdf4ff0a99cf50, but will also apply cleanly to 3.7.10.
Bug reports:
1. https://bugzilla.redhat.com/show_bug.cgi?id=757166
2. https://bugzilla.kernel.org/show_bug.cgi?id=42679
Signed-off-by: Andrew Cooks <acooks@gmail.com>
---
drivers/iommu/intel-iommu.c | 50 +++++++++++++++++++++++++++++++++++++++++++
drivers/pci/quirks.c | 47 +++++++++++++++++++++++++++++++++++++++-
include/linux/pci.h | 5 ++++
include/linux/pci_ids.h | 1 +
4 files changed, 102 insertions(+), 1 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0099667..13323f2 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1674,6 +1674,50 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
return 0;
}
+/* For quirky devices like Marvell 88SE91xx chips that use ghost functions. */
+static int map_ghost_dma_fn(struct dmar_domain *domain,
+ struct pci_dev *pdev,
+ int translation)
+{
+ u8 fn, fn_map;
+ int err = 0;
+
+ fn_map = pci_get_dma_source_map(pdev);
+
+ for (fn = 1; fn < 8; fn++) {
+ if (fn_map & (1 << fn)) {
+ err = domain_context_mapping_one(domain,
+ pci_domain_nr(pdev->bus),
+ pdev->bus->number,
+ PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
+ translation);
+ if (err)
+ return err;
+ dev_dbg(&pdev->dev, "dma quirk; func %d mapped", fn);
+ }
+ }
+ return 0;
+}
+
+static void iommu_detach_dev(struct intel_iommu *iommu, u8 bus, u8 devfn);
+
+static void unmap_ghost_dma_fn(struct intel_iommu *iommu,
+ struct pci_dev *pdev)
+{
+ u8 fn, fn_map;
+
+ fn_map = pci_get_dma_source_map(pdev);
+
+ for (fn = 1; fn < 8; fn++) {
+ if (fn_map & (1 << fn)) {
+ iommu_detach_dev(iommu,
+ pdev->bus->number,
+ PCI_DEVFN(PCI_SLOT(pdev->devfn), fn));
+ dev_dbg(&pdev->dev, "dma quirk; func %d unmapped", fn);
+ }
+ }
+}
+
static int
domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
int translation)
@@ -1687,6 +1731,11 @@ domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
if (ret)
return ret;
+ /* quirk for undeclared/ghost pci functions */
+ ret = map_ghost_dma_fn(domain, pdev, translation);
+ if (ret)
+ return ret;
+
/* dependent device mapping */
tmp = pci_find_upstream_pcie_bridge(pdev);
if (!tmp)
@@ -3786,6 +3835,7 @@ static void domain_remove_one_dev_info(struct dmar_domain *domain,
iommu_disable_dev_iotlb(info);
iommu_detach_dev(iommu, info->bus, info->devfn);
iommu_detach_dependent_devices(iommu, pdev);
+ unmap_ghost_dma_fn(iommu, pdev);
free_devinfo_mem(info);
spin_lock_irqsave(&device_domain_lock, flags);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0369fb6..d311100 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3249,6 +3249,10 @@ static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev)
return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
}
+/* Table of source functions for real devices. The DMA requests for the
+ * device are tagged with a different real function as source. This is
+ * relevant to multifunction devices.
+ */
static const struct pci_dev_dma_source {
u16 vendor;
u16 device;
@@ -3275,7 +3279,8 @@ static const struct pci_dev_dma_source {
* the device doing the DMA, but sometimes hardware is broken and will
* tag the DMA as being sourced from a different device. This function
* allows that translation. Note that the reference count of the
- * returned device is incremented on all paths.
+ * returned device is incremented on all paths. Translation is done when
+ * the device is added to an IOMMU group.
*/
struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
{
@@ -3292,6 +3297,46 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
return pci_dev_get(dev);
}
+/* Table of multiple (ghost) source functions. This is similar to the
+ * translated sources above, but with the following differences:
+ * 1. the device may use multiple functions as DMA sources,
+ * 2. these functions cannot be assumed to be actual devices,
+ * 3. the specific ghost function for a request can not be exactly predicted.
+ * The bitmap only contains the additional quirk functions.
+ */
+static const struct pci_dev_dma_multi_func_sources {
+ u16 vendor;
+ u16 device;
+ u8 func_map; /* bit map. lsb is fn 0. */
+} pci_dev_dma_multi_func_sources[] = {
+ { PCI_VENDOR_ID_MARVELL_2, 0x9123, (1<<0)|(1<<1)},
+ { PCI_VENDOR_ID_MARVELL_2, 0x9125, (1<<0)|(1<<1)},
+ { PCI_VENDOR_ID_MARVELL_2, 0x9128, (1<<0)|(1<<1)},
+ { PCI_VENDOR_ID_MARVELL_2, 0x9130, (1<<0)|(1<<1)},
+ { PCI_VENDOR_ID_MARVELL_2, 0x9172, (1<<0)|(1<<1)},
+ { 0 }
+};
+
+/*
+ * The mapping of fake/ghost functions is used when the real device is
+ * attached to an IOMMU domain. IOMMU groups are not aware of these
+ * functions, because they're not real devices.
+ */
+u8 pci_get_dma_source_map(struct pci_dev *dev)
+{
+ const struct pci_dev_dma_multi_func_sources *i;
+
+ for (i = pci_dev_dma_multi_func_sources; i->func_map; i++) {
+ if ((i->vendor == dev->vendor ||
+ i->vendor == (u16)PCI_ANY_ID) &&
+ (i->device == dev->device ||
+ i->device == (u16)PCI_ANY_ID)) {
+ return i->func_map;
+ }
+ }
+ return 0;
+}
+
static const struct pci_dev_acs_enabled {
u16 vendor;
u16 device;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2461033..5ad3822 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1578,6 +1578,7 @@ 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_get_dma_source(struct pci_dev *dev);
+u8 pci_get_dma_source_map(struct pci_dev *dev);
int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
#else
static inline void pci_fixup_device(enum pci_fixup_pass pass,
@@ -1586,6 +1587,10 @@ static inline struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
{
return pci_dev_get(dev);
}
+u8 pci_get_dma_source_map(struct pci_dev *dev)
+{
+ return 0;
+}
static inline int pci_dev_specific_acs_enabled(struct pci_dev *dev,
u16 acs_flags)
{
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index f11c1c2..df57496 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1604,6 +1604,7 @@
#define PCI_SUBDEVICE_ID_KEYSPAN_SX2 0x5334
#define PCI_VENDOR_ID_MARVELL 0x11ab
+#define PCI_VENDOR_ID_MARVELL_2 0x1b4b
#define PCI_DEVICE_ID_MARVELL_GT64111 0x4146
#define PCI_DEVICE_ID_MARVELL_GT64260 0x6430
#define PCI_DEVICE_ID_MARVELL_MV64360 0x6460
--
1.7.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
----- End forwarded message -----
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [acooks@gmail.com: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.]
2013-03-01 14:04 [acooks@gmail.com: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.] Konrad Rzeszutek Wilk
@ 2013-03-01 14:28 ` Andrew Cooper
2013-03-01 15:46 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2013-03-01 14:28 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel@lists.xensource.com, Jan Beulich
On 01/03/13 14:04, Konrad Rzeszutek Wilk wrote:
> Should we add a similar quirk to Xen?
CC'd Jan.
Jan: Didn't you already implement PCI phantom function quirks in Xen for
exactly this reason?
http://xenbits.xen.org/hg/staging/xen-unstable.hg/rev/23c4bbc0111d does
reference Marvell SATA controllers
~Andrew
>
> ----- Forwarded message from Andrew Cooks <acooks@gmail.com> -----
>
> Date: Fri, 1 Mar 2013 16:26:13 +0800
> From: Andrew Cooks <acooks@gmail.com>
> To: acooks@gmail.com, joro@8bytes.org, xjtuychu@hotmail.com, gm.ychu@gmail.com, alex.williamson@redhat.com, bhelgaas@google.com, jpiszcz@lucidpixels.com,
> dwmw2@infradead.org
> Cc: "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>, "open list:INTEL IOMMU VT-d" <iommu@lists.linux-foundation.org>, open list <linux-kernel@vger.kernel.org>
> Subject: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.
>
> This is my third submitted patch to make Marvell 88SE91xx SATA controllers work when IOMMU is enabled.[1][2]
>
> What's changed:
> * Adopt David Woodhouse's terminology by referring to the quirky functions as 'ghost' functions.
> * Unmap ghost functions when device is detached from IOMMU.
> * Stub function for when CONFIG_PCI_QUIRKS is not enabled.
>
> The bad:
> * Still no AMD support.
> * The table of affected chip IDs is as complete as I can make it by googling for bug reports.
>
> This patch was generated against commit b0af9cd9aab60ceb17d3ebabb9fdf4ff0a99cf50, but will also apply cleanly to 3.7.10.
>
> Bug reports:
> 1. https://bugzilla.redhat.com/show_bug.cgi?id=757166
> 2. https://bugzilla.kernel.org/show_bug.cgi?id=42679
>
> Signed-off-by: Andrew Cooks <acooks@gmail.com>
> ---
> drivers/iommu/intel-iommu.c | 50 +++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/quirks.c | 47 +++++++++++++++++++++++++++++++++++++++-
> include/linux/pci.h | 5 ++++
> include/linux/pci_ids.h | 1 +
> 4 files changed, 102 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 0099667..13323f2 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1674,6 +1674,50 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
> return 0;
> }
>
> +/* For quirky devices like Marvell 88SE91xx chips that use ghost functions. */
> +static int map_ghost_dma_fn(struct dmar_domain *domain,
> + struct pci_dev *pdev,
> + int translation)
> +{
> + u8 fn, fn_map;
> + int err = 0;
> +
> + fn_map = pci_get_dma_source_map(pdev);
> +
> + for (fn = 1; fn < 8; fn++) {
> + if (fn_map & (1 << fn)) {
> + err = domain_context_mapping_one(domain,
> + pci_domain_nr(pdev->bus),
> + pdev->bus->number,
> + PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
> + translation);
> + if (err)
> + return err;
> + dev_dbg(&pdev->dev, "dma quirk; func %d mapped", fn);
> + }
> + }
> + return 0;
> +}
> +
> +static void iommu_detach_dev(struct intel_iommu *iommu, u8 bus, u8 devfn);
> +
> +static void unmap_ghost_dma_fn(struct intel_iommu *iommu,
> + struct pci_dev *pdev)
> +{
> + u8 fn, fn_map;
> +
> + fn_map = pci_get_dma_source_map(pdev);
> +
> + for (fn = 1; fn < 8; fn++) {
> + if (fn_map & (1 << fn)) {
> + iommu_detach_dev(iommu,
> + pdev->bus->number,
> + PCI_DEVFN(PCI_SLOT(pdev->devfn), fn));
> + dev_dbg(&pdev->dev, "dma quirk; func %d unmapped", fn);
> + }
> + }
> +}
> +
> static int
> domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
> int translation)
> @@ -1687,6 +1731,11 @@ domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
> if (ret)
> return ret;
>
> + /* quirk for undeclared/ghost pci functions */
> + ret = map_ghost_dma_fn(domain, pdev, translation);
> + if (ret)
> + return ret;
> +
> /* dependent device mapping */
> tmp = pci_find_upstream_pcie_bridge(pdev);
> if (!tmp)
> @@ -3786,6 +3835,7 @@ static void domain_remove_one_dev_info(struct dmar_domain *domain,
> iommu_disable_dev_iotlb(info);
> iommu_detach_dev(iommu, info->bus, info->devfn);
> iommu_detach_dependent_devices(iommu, pdev);
> + unmap_ghost_dma_fn(iommu, pdev);
> free_devinfo_mem(info);
>
> spin_lock_irqsave(&device_domain_lock, flags);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 0369fb6..d311100 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3249,6 +3249,10 @@ static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev)
> return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> }
>
> +/* Table of source functions for real devices. The DMA requests for the
> + * device are tagged with a different real function as source. This is
> + * relevant to multifunction devices.
> + */
> static const struct pci_dev_dma_source {
> u16 vendor;
> u16 device;
> @@ -3275,7 +3279,8 @@ static const struct pci_dev_dma_source {
> * the device doing the DMA, but sometimes hardware is broken and will
> * tag the DMA as being sourced from a different device. This function
> * allows that translation. Note that the reference count of the
> - * returned device is incremented on all paths.
> + * returned device is incremented on all paths. Translation is done when
> + * the device is added to an IOMMU group.
> */
> struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
> {
> @@ -3292,6 +3297,46 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
> return pci_dev_get(dev);
> }
>
> +/* Table of multiple (ghost) source functions. This is similar to the
> + * translated sources above, but with the following differences:
> + * 1. the device may use multiple functions as DMA sources,
> + * 2. these functions cannot be assumed to be actual devices,
> + * 3. the specific ghost function for a request can not be exactly predicted.
> + * The bitmap only contains the additional quirk functions.
> + */
> +static const struct pci_dev_dma_multi_func_sources {
> + u16 vendor;
> + u16 device;
> + u8 func_map; /* bit map. lsb is fn 0. */
> +} pci_dev_dma_multi_func_sources[] = {
> + { PCI_VENDOR_ID_MARVELL_2, 0x9123, (1<<0)|(1<<1)},
> + { PCI_VENDOR_ID_MARVELL_2, 0x9125, (1<<0)|(1<<1)},
> + { PCI_VENDOR_ID_MARVELL_2, 0x9128, (1<<0)|(1<<1)},
> + { PCI_VENDOR_ID_MARVELL_2, 0x9130, (1<<0)|(1<<1)},
> + { PCI_VENDOR_ID_MARVELL_2, 0x9172, (1<<0)|(1<<1)},
> + { 0 }
> +};
> +
> +/*
> + * The mapping of fake/ghost functions is used when the real device is
> + * attached to an IOMMU domain. IOMMU groups are not aware of these
> + * functions, because they're not real devices.
> + */
> +u8 pci_get_dma_source_map(struct pci_dev *dev)
> +{
> + const struct pci_dev_dma_multi_func_sources *i;
> +
> + for (i = pci_dev_dma_multi_func_sources; i->func_map; i++) {
> + if ((i->vendor == dev->vendor ||
> + i->vendor == (u16)PCI_ANY_ID) &&
> + (i->device == dev->device ||
> + i->device == (u16)PCI_ANY_ID)) {
> + return i->func_map;
> + }
> + }
> + return 0;
> +}
> +
> static const struct pci_dev_acs_enabled {
> u16 vendor;
> u16 device;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033..5ad3822 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1578,6 +1578,7 @@ 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_get_dma_source(struct pci_dev *dev);
> +u8 pci_get_dma_source_map(struct pci_dev *dev);
> int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
> #else
> static inline void pci_fixup_device(enum pci_fixup_pass pass,
> @@ -1586,6 +1587,10 @@ static inline struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
> {
> return pci_dev_get(dev);
> }
> +u8 pci_get_dma_source_map(struct pci_dev *dev)
> +{
> + return 0;
> +}
> static inline int pci_dev_specific_acs_enabled(struct pci_dev *dev,
> u16 acs_flags)
> {
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index f11c1c2..df57496 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1604,6 +1604,7 @@
> #define PCI_SUBDEVICE_ID_KEYSPAN_SX2 0x5334
>
> #define PCI_VENDOR_ID_MARVELL 0x11ab
> +#define PCI_VENDOR_ID_MARVELL_2 0x1b4b
> #define PCI_DEVICE_ID_MARVELL_GT64111 0x4146
> #define PCI_DEVICE_ID_MARVELL_GT64260 0x6430
> #define PCI_DEVICE_ID_MARVELL_MV64360 0x6460
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [acooks@gmail.com: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.]
2013-03-01 14:28 ` Andrew Cooper
@ 2013-03-01 15:46 ` Jan Beulich
0 siblings, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2013-03-01 15:46 UTC (permalink / raw)
To: Andrew Cooper, Konrad Rzeszutek Wilk; +Cc: xen-devel@lists.xensource.com
>>> On 01.03.13 at 15:28, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 01/03/13 14:04, Konrad Rzeszutek Wilk wrote:
>> Should we add a similar quirk to Xen?
>
> Jan: Didn't you already implement PCI phantom function quirks in Xen for
> exactly this reason?
>
> http://xenbits.xen.org/hg/staging/xen-unstable.hg/rev/23c4bbc0111d does
> reference Marvell SATA controllers
Correct, and in a much more generic way, and also covering the
AMD case. The only thing we don't do is automatically apply the
workaround - we require a commend line option to be specified. If
there is a list of devices we firmly know need this, then would
could certainly add something not requiring user intervention.
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-03-01 15:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-01 14:04 [acooks@gmail.com: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.] Konrad Rzeszutek Wilk
2013-03-01 14:28 ` Andrew Cooper
2013-03-01 15:46 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.