Re: [Xen-devel] [acooks@xxxxxxxxx: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.]

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxx> -----
>
> Date: Fri,  1 Mar 2013 16:26:13 +0800
> From: Andrew Cooks <acooks@xxxxxxxxx>
> To: acooks@xxxxxxxxx, joro@xxxxxxxxxx, xjtuychu@xxxxxxxxxxx, 
> gm.ychu@xxxxxxxxx, alex.williamson@xxxxxxxxxx, bhelgaas@xxxxxxxxxx, 
> jpiszcz@xxxxxxxxxxxxxxx,
>       dwmw2@xxxxxxxxxxxxx
> Cc: "open list:PCI SUBSYSTEM" <linux-pci@xxxxxxxxxxxxxxx>, "open list:INTEL 
> IOMMU VT-d" <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>, open list 
> <linux-kernel@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> ---
>  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


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