Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on

Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on

[Dmitry Safonov]

+Cc: linux-pci@vger.kernel.org
+Cc: Bjorn Helgaas <bhelgaas@google.com>
+Cc: Logan Gunthorpe <logang@deltatee.com>

On 11/5/19 12:17 PM, James Sewart wrote:
> Any comments on this?
> 
> Cheers,
> James.
> 
>> On 24 Oct 2019, at 13:52, James Sewart <jamessewart@arista.com> wrote:
>>
>> The PLX PEX NTB forwards DMA transactions using Requester ID's that don't exist as
>> PCI devices. The devfn for a transaction is used as an index into a lookup table
>> storing the origin of a transaction on the other side of the bridge.
>>
>> This patch aliases all possible devfn's to the NTB device so that any transaction
>> coming in is governed by the mappings for the NTB.
>>
>> Signed-Off-By: James Sewart <jamessewart@arista.com>
>> ---
>> drivers/pci/quirks.c | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 320255e5e8f8..647f546e427f 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -5315,6 +5315,28 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
>> SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
>> SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
>>
>> +/*
>> + * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
>> + * are used to forward responses to the originator on the other side of the
>> + * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
>> + * turned on.
>> + */
>> +static void quirk_PLX_NTB_dma_alias(struct pci_dev *pdev)
>> +{
>> +	if (!pdev->dma_alias_mask)
>> +		pdev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
>> +					      sizeof(long), GFP_KERNEL);
>> +	if (!pdev->dma_alias_mask) {
>> +		dev_warn(&pdev->dev, "Unable to allocate DMA alias mask\n");
>> +		return;
>> +	}
>> +
>> +	// PLX NTB may use all 256 devfns
>> +	memset(pdev->dma_alias_mask, U8_MAX, (U8_MAX+1)/BITS_PER_BYTE);
>> +}
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_PLX_NTB_dma_alias);
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_PLX_NTB_dma_alias);
>> +
>> /*
>>  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
>>  * not always reset the secondary Nvidia GPU between reboots if the system
>> -- 
>> 2.19.1
>>
>>
> 

Thanks,
          Dmitry

Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on

[Logan Gunthorpe]

On 2019-11-20 10:48 a.m., Dmitry Safonov wrote:
> +Cc: linux-pci@vger.kernel.org
> +Cc: Bjorn Helgaas <bhelgaas@google.com>
> +Cc: Logan Gunthorpe <logang@deltatee.com>
> 
> On 11/5/19 12:17 PM, James Sewart wrote:
>> Any comments on this?
>>
>> Cheers,
>> James.
>>
>>> On 24 Oct 2019, at 13:52, James Sewart <jamessewart@arista.com> wrote:
>>>
>>> The PLX PEX NTB forwards DMA transactions using Requester ID's that don't exist as
>>> PCI devices. The devfn for a transaction is used as an index into a lookup table
>>> storing the origin of a transaction on the other side of the bridge.
>>>
>>> This patch aliases all possible devfn's to the NTB device so that any transaction
>>> coming in is governed by the mappings for the NTB.
>>>
>>> Signed-Off-By: James Sewart <jamessewart@arista.com>
>>> ---
>>> drivers/pci/quirks.c | 22 ++++++++++++++++++++++
>>> 1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index 320255e5e8f8..647f546e427f 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -5315,6 +5315,28 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
>>> SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
>>> SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
>>>
>>> +/*
>>> + * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
>>> + * are used to forward responses to the originator on the other side of the
>>> + * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
>>> + * turned on.
>>> + */
>>> +static void quirk_PLX_NTB_dma_alias(struct pci_dev *pdev)
>>> +{
>>> +	if (!pdev->dma_alias_mask)
>>> +		pdev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
>>> +					      sizeof(long), GFP_KERNEL);
>>> +	if (!pdev->dma_alias_mask) {
>>> +		dev_warn(&pdev->dev, "Unable to allocate DMA alias mask\n");
>>> +		return;
>>> +	}
>>> +
>>> +	// PLX NTB may use all 256 devfns
>>> +	memset(pdev->dma_alias_mask, U8_MAX, (U8_MAX+1)/BITS_PER_BYTE);

I think it would be better to create a pci_add_dma_alias_range()
function instead of directly accessing dma_alias_mask. We could then use
that function to clean up quirk_switchtec_ntb_dma_alias() which is
essentially doing the same thing.

It's also quite ugly that you're allocating with kcalloc here instead of
bitmap_zalloc() seeing it will be free'd with bitmap_free(). Also, you
should probably use bitmap_set() instead of memset().

Logan

Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on

[Bjorn Helgaas]

On Wed, Nov 20, 2019 at 12:30:48PM -0700, Logan Gunthorpe wrote:
> On 2019-11-20 10:48 a.m., Dmitry Safonov wrote:
> > On 11/5/19 12:17 PM, James Sewart wrote:
> >>
> >>> On 24 Oct 2019, at 13:52, James Sewart <jamessewart@arista.com> wrote:
> >>>
> >>> The PLX PEX NTB forwards DMA transactions using Requester ID's that don't exist as
> >>> PCI devices. The devfn for a transaction is used as an index into a lookup table
> >>> storing the origin of a transaction on the other side of the bridge.
> >>>
> >>> This patch aliases all possible devfn's to the NTB device so that any transaction
> >>> coming in is governed by the mappings for the NTB.
> >>>
> >>> Signed-Off-By: James Sewart <jamessewart@arista.com>
> >>> ---
> >>> drivers/pci/quirks.c | 22 ++++++++++++++++++++++
> >>> 1 file changed, 22 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >>> index 320255e5e8f8..647f546e427f 100644
> >>> --- a/drivers/pci/quirks.c
> >>> +++ b/drivers/pci/quirks.c
> >>> @@ -5315,6 +5315,28 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
> >>> SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
> >>> SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
> >>>
> >>> +/*
> >>> + * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
> >>> + * are used to forward responses to the originator on the other side of the
> >>> + * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
> >>> + * turned on.
> >>> + */
> >>> +static void quirk_PLX_NTB_dma_alias(struct pci_dev *pdev)
> >>> +{
> >>> +	if (!pdev->dma_alias_mask)
> >>> +		pdev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> >>> +					      sizeof(long), GFP_KERNEL);
> >>> +	if (!pdev->dma_alias_mask) {
> >>> +		dev_warn(&pdev->dev, "Unable to allocate DMA alias mask\n");
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	// PLX NTB may use all 256 devfns
> >>> +	memset(pdev->dma_alias_mask, U8_MAX, (U8_MAX+1)/BITS_PER_BYTE);
> 
> I think it would be better to create a pci_add_dma_alias_range()
> function instead of directly accessing dma_alias_mask. We could then use
> that function to clean up quirk_switchtec_ntb_dma_alias() which is
> essentially doing the same thing.

Great idea!

Bjorn

Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on

[Bjorn Helgaas]

[+cc Alex]

Hi James,

Thanks for the patch, and thanks, Dmitry for the cc!

"scripts/get_maintainer.pl -f drivers/pci/quirks.c" will give you a
list of relevant email addresses to post patches.  It was a good idea
to augment that list with related addresses, e.g., Logan and the iommu
list.

Follow existing style for subject, e.g.,

  PCI: Add DMA alias quirk for Microsemi Switchtec NTB

for a recent similar patch.

On Wed, Nov 20, 2019 at 05:48:45PM +0000, Dmitry Safonov wrote:
> On 11/5/19 12:17 PM, James Sewart wrote:
> >> On 24 Oct 2019, at 13:52, James Sewart <jamessewart@arista.com> wrote:
> >>
> >> The PLX PEX NTB forwards DMA transactions using Requester ID's that don't exist as
> >> PCI devices. The devfn for a transaction is used as an index into a lookup table
> >> storing the origin of a transaction on the other side of the bridge.
> >>
> >> This patch aliases all possible devfn's to the NTB device so that any transaction
> >> coming in is governed by the mappings for the NTB.
> >>
> >> Signed-Off-By: James Sewart <jamessewart@arista.com>

Conventionally capitalized as:

  Signed-off-by: James Sewart <jamessewart@arista.com>

> >> ---
> >> drivers/pci/quirks.c | 22 ++++++++++++++++++++++
> >> 1 file changed, 22 insertions(+)
> >>
> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> index 320255e5e8f8..647f546e427f 100644
> >> --- a/drivers/pci/quirks.c
> >> +++ b/drivers/pci/quirks.c
> >> @@ -5315,6 +5315,28 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
> >> SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
> >> SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
> >>
> >> +/*
> >> + * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
> >> + * are used to forward responses to the originator on the other side of the
> >> + * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
> >> + * turned on.
> >> + */
> >> +static void quirk_PLX_NTB_dma_alias(struct pci_dev *pdev)

Conventional style is all lower-case (e.g.
quirk_switchtec_ntb_dma_alias()) for function and variable names, and
upper-case in English text.

> >> +{
> >> +	if (!pdev->dma_alias_mask)
> >> +		pdev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> >> +					      sizeof(long), GFP_KERNEL);
> >> +	if (!pdev->dma_alias_mask) {
> >> +		dev_warn(&pdev->dev, "Unable to allocate DMA alias mask\n");

pci_warn()

> >> +		return;
> >> +	}
> >> +
> >> +	// PLX NTB may use all 256 devfns
> >> +	memset(pdev->dma_alias_mask, U8_MAX, (U8_MAX+1)/BITS_PER_BYTE);

Use C (not C++) comment style, as the rest of the file does.

I was about to suggest using pci_add_dma_alias(), but as currently
implemented that would generate 256 messages in dmesg, which seems
like overkill.

But I think it would still be good to allocate the mask the same way
(using bitmap_zalloc()) and to set the bits using bitmap_set().

It would also be nice to have one line in dmesg about these aliases,
as a hint when debugging IOMMU faults.

> >> +}
> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_PLX_NTB_dma_alias);
> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_PLX_NTB_dma_alias);
> >> +
> >> /*
> >>  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
> >>  * not always reset the secondary Nvidia GPU between reboots if the system

[PATCH v2] PCI: Add DMA alias quirk for PLX PEX NTB

[James Sewart]

The PLX PEX NTB forwards DMA transactions using Requester ID's that
don't exist as PCI devices. The devfn for a transaction is used as an
index into a lookup table storing the origin of a transaction on the
other side of the bridge.

Add helper pci_add_dma_alias_range that can alias a range of devfns in 
dma_alias_mask.

This patch aliases all possible devfn's to the NTB device so that any
transaction coming in is governed by the mappings for the NTB.

Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/pci.c    | 29 ++++++++++++++++++++++-------
 drivers/pci/quirks.c | 15 +++++++++++++++
 include/linux/pci.h  |  1 +
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a527..f502af1b5d10 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5854,6 +5854,18 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 	return 0;
 }
 
+int _pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
+{
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		pci_warn(dev, "Unable to allocate DMA alias mask\n");
+		return -ENOMEM;
+	}
+	bitmap_set(dev->dma_alias_mask, devfn_from, len);
+	return 0;
+}
+
 /**
  * pci_add_dma_alias - Add a DMA devfn alias for a device
  * @dev: the PCI device for which alias is added
@@ -5875,18 +5887,21 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	if (!dev->dma_alias_mask)
-		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
-	if (!dev->dma_alias_mask) {
-		pci_warn(dev, "Unable to allocate DMA alias mask\n");
+	if (_pci_add_dma_alias_range(dev, devfn, 1) != 0)
 		return;
-	}
-
-	set_bit(devfn, dev->dma_alias_mask);
 	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
+void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
+{
+	int devfn_to = devfn_from + len - 1;
+	if (_pci_add_dma_alias_range(dev, devfn_from, len) != 0)
+		return;
+	pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
+		 PCI_SLOT(devfn_from), PCI_FUNC(devfn_from), PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
+}
+
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
 {
 	return (dev1->dma_alias_mask &&
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 320255e5e8f8..1ed230f739a4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5315,6 +5315,21 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
 SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
 SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
 
+/*
+ * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
+ * are used to forward responses to the originator on the other side of the
+ * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
+ * turned on.
+ */
+static void quirk_plx_ntb_dma_alias(struct pci_dev *pdev)
+{
+	pci_info(pdev, "Setting PLX NTB proxy ID aliases\n");
+	/* PLX NTB may use all 256 devfns */
+	pci_add_dma_alias_range(pdev, 0, 256);
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_plx_ntb_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_plx_ntb_dma_alias);
+
 /*
  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
  * not always reset the secondary Nvidia GPU between reboots if the system
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1a6cf19eac2d..6765f3d0102b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2324,6 +2324,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 #endif
 
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len);
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
-- 
2.24.0

Re: [PATCH v2] PCI: Add DMA alias quirk for PLX PEX NTB

[Logan Gunthorpe]

On 2019-11-26 10:03 a.m., James Sewart wrote:
> The PLX PEX NTB forwards DMA transactions using Requester ID's that
> don't exist as PCI devices. The devfn for a transaction is used as an
> index into a lookup table storing the origin of a transaction on the
> other side of the bridge.
> 
> Add helper pci_add_dma_alias_range that can alias a range of devfns in 
> dma_alias_mask.
> 
> This patch aliases all possible devfn's to the NTB device so that any
> transaction coming in is governed by the mappings for the NTB.
> 
> Signed-off-by: James Sewart <jamessewart@arista.com>

Looks good to me, save a nit below; and you may want to split this into
two patches: one that introduces the new interface and one that uses it.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> ---
>  drivers/pci/pci.c    | 29 ++++++++++++++++++++++-------
>  drivers/pci/quirks.c | 15 +++++++++++++++
>  include/linux/pci.h  |  1 +
>  3 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a97e2571a527..f502af1b5d10 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5854,6 +5854,18 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  	return 0;
>  }
>  
> +int _pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
> +{
> +	if (!dev->dma_alias_mask)
> +		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
> +	if (!dev->dma_alias_mask) {
> +		pci_warn(dev, "Unable to allocate DMA alias mask\n");
> +		return -ENOMEM;
> +	}
> +	bitmap_set(dev->dma_alias_mask, devfn_from, len);
> +	return 0;
> +}
> +
>  /**
>   * pci_add_dma_alias - Add a DMA devfn alias for a device
>   * @dev: the PCI device for which alias is added
> @@ -5875,18 +5887,21 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
> -	if (!dev->dma_alias_mask)
> -		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
> -	if (!dev->dma_alias_mask) {
> -		pci_warn(dev, "Unable to allocate DMA alias mask\n");
> +	if (_pci_add_dma_alias_range(dev, devfn, 1) != 0)
>  		return;
> -	}
> -
> -	set_bit(devfn, dev->dma_alias_mask);
>  	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
>  		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
>  
> +void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
> +{
> +	int devfn_to = devfn_from + len - 1;

Nit: there should be a blank line between the variable declarations and
the code in the functions.

> +	if (_pci_add_dma_alias_range(dev, devfn_from, len) != 0)
> +		return;
> +	pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
> +		 PCI_SLOT(devfn_from), PCI_FUNC(devfn_from), PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
> +}
> +
>  bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
>  {
>  	return (dev1->dma_alias_mask &&
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 320255e5e8f8..1ed230f739a4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5315,6 +5315,21 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
>  SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
>  SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
>  
> +/*
> + * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
> + * are used to forward responses to the originator on the other side of the
> + * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
> + * turned on.
> + */
> +static void quirk_plx_ntb_dma_alias(struct pci_dev *pdev)
> +{
> +	pci_info(pdev, "Setting PLX NTB proxy ID aliases\n");
> +	/* PLX NTB may use all 256 devfns */
> +	pci_add_dma_alias_range(pdev, 0, 256);
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_plx_ntb_dma_alias);
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_plx_ntb_dma_alias);
> +
>  /*
>   * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
>   * not always reset the secondary Nvidia GPU between reboots if the system
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1a6cf19eac2d..6765f3d0102b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2324,6 +2324,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  #endif
>  
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len);
>  bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
>  int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			   int (*fn)(struct pci_dev *pdev,
> 

[PATCH v3 1/2] PCI: Add helper pci_add_dma_alias_range

[James Sewart]

pci_add_dma_alias_range can be used to create a dma alias for range of
devfns.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/pci.c   | 30 +++++++++++++++++++++++-------
 include/linux/pci.h |  1 +
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a527..68339309c0f4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5854,6 +5854,18 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 	return 0;
 }
 
+int _pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
+{
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		pci_warn(dev, "Unable to allocate DMA alias mask\n");
+		return -ENOMEM;
+	}
+	bitmap_set(dev->dma_alias_mask, devfn_from, len);
+	return 0;
+}
+
 /**
  * pci_add_dma_alias - Add a DMA devfn alias for a device
  * @dev: the PCI device for which alias is added
@@ -5875,18 +5887,22 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	if (!dev->dma_alias_mask)
-		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
-	if (!dev->dma_alias_mask) {
-		pci_warn(dev, "Unable to allocate DMA alias mask\n");
+	if (_pci_add_dma_alias_range(dev, devfn, 1) != 0)
 		return;
-	}
-
-	set_bit(devfn, dev->dma_alias_mask);
 	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
+void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
+{
+	int devfn_to = devfn_from + len - 1;
+
+	if (_pci_add_dma_alias_range(dev, devfn_from, len) != 0)
+		return;
+	pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
+		 PCI_SLOT(devfn_from), PCI_FUNC(devfn_from), PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
+}
+
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
 {
 	return (dev1->dma_alias_mask &&
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1a6cf19eac2d..6765f3d0102b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2324,6 +2324,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 #endif
 
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len);
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
-- 
2.24.0

[PATCH v3 2/2] PCI: Add DMA alias quirk for PLX PEX NTB

[James Sewart]

The PLX PEX NTB forwards DMA transactions using Requester ID's that
don't exist as PCI devices. The devfn for a transaction is used as an
index into a lookup table storing the origin of a transaction on the
other side of the bridge.

This patch aliases all possible devfn's to the NTB device so that any
transaction coming in is governed by the mappings for the NTB.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 320255e5e8f8..1ed230f739a4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5315,6 +5315,21 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
 SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
 SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
 
+/*
+ * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
+ * are used to forward responses to the originator on the other side of the
+ * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
+ * turned on.
+ */
+static void quirk_plx_ntb_dma_alias(struct pci_dev *pdev)
+{
+	pci_info(pdev, "Setting PLX NTB proxy ID aliases\n");
+	/* PLX NTB may use all 256 devfns */
+	pci_add_dma_alias_range(pdev, 0, 256);
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_plx_ntb_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_plx_ntb_dma_alias);
+
 /*
  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
  * not always reset the secondary Nvidia GPU between reboots if the system
-- 
2.24.0

Re: [PATCH v2] PCI: Add DMA alias quirk for PLX PEX NTB

[Christoph Hellwig]

> +int _pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)

This should be mrked static.  Also single underscore prefixes are rather
unusual in Linux.  Either use two or use a more descriptive name.


> @@ -5875,18 +5887,21 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
> -	if (!dev->dma_alias_mask)
> -		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
> -	if (!dev->dma_alias_mask) {
> -		pci_warn(dev, "Unable to allocate DMA alias mask\n");
> +	if (_pci_add_dma_alias_range(dev, devfn, 1) != 0)
>  		return;
> -	}
> -
> -	set_bit(devfn, dev->dma_alias_mask);
>  	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
>  		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
>  
> +void pci_add_dma_alias_range(struct pci_dev *dev, u8 devfn_from, int len)
> +{
> +	int devfn_to = devfn_from + len - 1;
> +	if (_pci_add_dma_alias_range(dev, devfn_from, len) != 0)
> +		return;
> +	pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
> +		 PCI_SLOT(devfn_from), PCI_FUNC(devfn_from), PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
> +}

This adds a non-string constant line over 80 chars that should be fixed
up.

But can't you just add the len argument (which really should be
nr_devfns or so) to pci_add_dma_alias and switch the 8 existing
callers over?

[PATCH v3 1/2] PCI: Add parameter nr_devfns to pci_add_dma_alias

[James Sewart]

pci_add_dma_alias can now be used to create a dma alias for a range of
devfns.

Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/pci.c    | 21 ++++++++++++++++-----
 drivers/pci/quirks.c | 14 +++++++-------
 include/linux/pci.h  |  2 +-
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a527..71dbabf5ee3d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 /**
  * pci_add_dma_alias - Add a DMA devfn alias for a device
  * @dev: the PCI device for which alias is added
- * @devfn: alias slot and function
+ * @devfn_from: alias slot and function
+ * @nr_devfns: Number of subsequent devfns to alias
  *
  * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
  * which is used to program permissible bus-devfn source addresses for DMA
@@ -5873,8 +5874,12 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  * cannot be left as a userspace activity).  DMA aliases should therefore
  * be configured via quirks, such as the PCI fixup header quirk.
  */
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, int nr_devfns)
 {
+	int devfn_to = devfn_from + nr_devfns - 1;
+
+	BUG_ON(nr_devfns < 1);
+
 	if (!dev->dma_alias_mask)
 		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
 	if (!dev->dma_alias_mask) {
@@ -5882,9 +5887,15 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 		return;
 	}
 
-	set_bit(devfn, dev->dma_alias_mask);
-	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
-		 PCI_SLOT(devfn), PCI_FUNC(devfn));
+	bitmap_set(dev->dma_alias_mask, devfn_from, nr_devfns);
+
+	if (nr_devfns == 1)
+		pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
+				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from));
+	else
+		pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
+				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from),
+				PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
 }
 
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 320255e5e8f8..0f3f5afc73fd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3932,7 +3932,7 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 static void quirk_dma_func0_alias(struct pci_dev *dev)
 {
 	if (PCI_FUNC(dev->devfn) != 0)
-		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0), 1);
 }
 
 /*
@@ -3946,7 +3946,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
 static void quirk_dma_func1_alias(struct pci_dev *dev)
 {
 	if (PCI_FUNC(dev->devfn) != 1)
-		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1), 1);
 }
 
 /*
@@ -4031,7 +4031,7 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
 	if (id)
-		pci_add_dma_alias(dev, id->driver_data);
+		pci_add_dma_alias(dev, id->driver_data, 1);
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
@@ -4073,9 +4073,9 @@ DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
  */
 static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
 {
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0), 1);
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0), 1);
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3), 1);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
@@ -5273,7 +5273,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev *pdev)
 			pci_dbg(pdev,
 				"Aliasing Partition %d Proxy ID %02x.%d\n",
 				pp, PCI_SLOT(devfn), PCI_FUNC(devfn));
-			pci_add_dma_alias(pdev, devfn);
+			pci_add_dma_alias(pdev, devfn, 1);
 		}
 	}
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1a6cf19eac2d..f51bdda49e9a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2323,7 +2323,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 }
 #endif
 
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, int nr_devfns);
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
-- 
2.24.0

[PATCH v3 2/2] PCI: Add DMA alias quirk for PLX PEX NTB

[James Sewart]

The PLX PEX NTB forwards DMA transactions using Requester ID's that
don't exist as PCI devices. The devfn for a transaction is used as an
index into a lookup table storing the origin of a transaction on the
other side of the bridge.

This patch aliases all possible devfn's to the NTB device so that any
transaction coming in is governed by the mappings for the NTB.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0f3f5afc73fd..3a67049ca630 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5315,6 +5315,21 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
 SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
 SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
 
+/*
+ * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
+ * are used to forward responses to the originator on the other side of the
+ * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
+ * turned on.
+ */
+static void quirk_plx_ntb_dma_alias(struct pci_dev *pdev)
+{
+	pci_info(pdev, "Setting PLX NTB proxy ID aliases\n");
+	/* PLX NTB may use all 256 devfns */
+	pci_add_dma_alias(pdev, 0, 256);
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_plx_ntb_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_plx_ntb_dma_alias);
+
 /*
  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
  * not always reset the secondary Nvidia GPU between reboots if the system
-- 
2.24.0

Re: [PATCH v3 1/2] PCI: Add parameter nr_devfns to pci_add_dma_alias

[Logan Gunthorpe]

On 2019-11-27 6:27 a.m., James Sewart wrote:
>   * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
>   * which is used to program permissible bus-devfn source addresses for DMA
> @@ -5873,8 +5874,12 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   * cannot be left as a userspace activity).  DMA aliases should therefore
>   * be configured via quirks, such as the PCI fixup header quirk.
>   */
> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, int nr_devfns)
>  {
> +	int devfn_to = devfn_from + nr_devfns - 1;
> +
> +	BUG_ON(nr_devfns < 1);

Why not just make nr_devfns unsigned and do nothing if it's zero? It
might also be worth checking that nr_devfns + devfn_from is less than
U8_MAX... But I'd probably avoid the BUG_ON and just truncate it.

Logan

[PATCH v4 1/2] PCI: Add parameter nr_devfns to pci_add_dma_alias

[James Sewart]

pci_add_dma_alias can now be used to create a dma alias for a range of
devfns.

Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/pci.c    | 23 ++++++++++++++++++-----
 drivers/pci/quirks.c | 14 +++++++-------
 include/linux/pci.h  |  2 +-
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a527..9b0e3481fe17 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 /**
  * pci_add_dma_alias - Add a DMA devfn alias for a device
  * @dev: the PCI device for which alias is added
- * @devfn: alias slot and function
+ * @devfn_from: alias slot and function
+ * @nr_devfns: Number of subsequent devfns to alias
  *
  * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
  * which is used to program permissible bus-devfn source addresses for DMA
@@ -5873,8 +5874,14 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  * cannot be left as a userspace activity).  DMA aliases should therefore
  * be configured via quirks, such as the PCI fixup header quirk.
  */
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
 {
+	int devfn_to;
+
+	if (nr_devfns > U8_MAX+1)
+		nr_devfns = U8_MAX+1;
+	devfn_to = devfn_from + nr_devfns - 1;
+
 	if (!dev->dma_alias_mask)
 		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
 	if (!dev->dma_alias_mask) {
@@ -5882,9 +5889,15 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 		return;
 	}
 
-	set_bit(devfn, dev->dma_alias_mask);
-	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
-		 PCI_SLOT(devfn), PCI_FUNC(devfn));
+	bitmap_set(dev->dma_alias_mask, devfn_from, nr_devfns);
+
+	if (nr_devfns == 1)
+		pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
+				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from));
+	else if(nr_devfns > 1)
+		pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
+				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from),
+				PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
 }
 
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 320255e5e8f8..0f3f5afc73fd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3932,7 +3932,7 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 static void quirk_dma_func0_alias(struct pci_dev *dev)
 {
 	if (PCI_FUNC(dev->devfn) != 0)
-		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0), 1);
 }
 
 /*
@@ -3946,7 +3946,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
 static void quirk_dma_func1_alias(struct pci_dev *dev)
 {
 	if (PCI_FUNC(dev->devfn) != 1)
-		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1), 1);
 }
 
 /*
@@ -4031,7 +4031,7 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
 	if (id)
-		pci_add_dma_alias(dev, id->driver_data);
+		pci_add_dma_alias(dev, id->driver_data, 1);
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
@@ -4073,9 +4073,9 @@ DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
  */
 static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
 {
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0), 1);
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0), 1);
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3), 1);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
@@ -5273,7 +5273,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev *pdev)
 			pci_dbg(pdev,
 				"Aliasing Partition %d Proxy ID %02x.%d\n",
 				pp, PCI_SLOT(devfn), PCI_FUNC(devfn));
-			pci_add_dma_alias(pdev, devfn);
+			pci_add_dma_alias(pdev, devfn, 1);
 		}
 	}
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1a6cf19eac2d..84a8d4c2b24e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2323,7 +2323,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 }
 #endif
 
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns);
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
-- 
2.24.0

[PATCH v4 2/2] PCI: Add DMA alias quirk for PLX PEX NTB

[James Sewart]

The PLX PEX NTB forwards DMA transactions using Requester ID's that
don't exist as PCI devices. The devfn for a transaction is used as an
index into a lookup table storing the origin of a transaction on the
other side of the bridge.

This patch aliases all possible devfn's to the NTB device so that any
transaction coming in is governed by the mappings for the NTB.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0f3f5afc73fd..3a67049ca630 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5315,6 +5315,21 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
 SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
 SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
 
+/*
+ * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
+ * are used to forward responses to the originator on the other side of the
+ * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
+ * turned on.
+ */
+static void quirk_plx_ntb_dma_alias(struct pci_dev *pdev)
+{
+	pci_info(pdev, "Setting PLX NTB proxy ID aliases\n");
+	/* PLX NTB may use all 256 devfns */
+	pci_add_dma_alias(pdev, 0, 256);
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_plx_ntb_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_plx_ntb_dma_alias);
+
 /*
  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
  * not always reset the secondary Nvidia GPU between reboots if the system
-- 
2.24.0

Re: [PATCH v4 1/2] PCI: Add parameter nr_devfns to pci_add_dma_alias

[Logan Gunthorpe]

On 2019-11-29 5:49 a.m., James Sewart wrote:
> pci_add_dma_alias can now be used to create a dma alias for a range of
> devfns.
> 
> Signed-off-by: James Sewart <jamessewart@arista.com>
> ---
>  drivers/pci/pci.c    | 23 ++++++++++++++++++-----
>  drivers/pci/quirks.c | 14 +++++++-------
>  include/linux/pci.h  |  2 +-
>  3 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a97e2571a527..9b0e3481fe17 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  /**
>   * pci_add_dma_alias - Add a DMA devfn alias for a device
>   * @dev: the PCI device for which alias is added
> - * @devfn: alias slot and function
> + * @devfn_from: alias slot and function
> + * @nr_devfns: Number of subsequent devfns to alias
>   *
>   * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
>   * which is used to program permissible bus-devfn source addresses for DMA
> @@ -5873,8 +5874,14 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   * cannot be left as a userspace activity).  DMA aliases should therefore
>   * be configured via quirks, such as the PCI fixup header quirk.
>   */
> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
>  {
> +	int devfn_to;
> +
> +	if (nr_devfns > U8_MAX+1)
> +		nr_devfns = U8_MAX+1;

Why +1? That doesn't seem right to me....

> +	devfn_to = devfn_from + nr_devfns - 1;
> +
>  	if (!dev->dma_alias_mask)
>  		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
>  	if (!dev->dma_alias_mask) {
> @@ -5882,9 +5889,15 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  		return;
>  	}
>  
> -	set_bit(devfn, dev->dma_alias_mask);
> -	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
> -		 PCI_SLOT(devfn), PCI_FUNC(devfn));
> +	bitmap_set(dev->dma_alias_mask, devfn_from, nr_devfns);
> +
> +	if (nr_devfns == 1)
> +		pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
> +				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from));
> +	else if(nr_devfns > 1)
> +		pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
> +				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from),
> +				PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
>  }
>  
>  bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 320255e5e8f8..0f3f5afc73fd 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3932,7 +3932,7 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  static void quirk_dma_func0_alias(struct pci_dev *dev)
>  {
>  	if (PCI_FUNC(dev->devfn) != 0)
> -		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0), 1);
>  }
>  
>  /*
> @@ -3946,7 +3946,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
>  static void quirk_dma_func1_alias(struct pci_dev *dev)
>  {
>  	if (PCI_FUNC(dev->devfn) != 1)
> -		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1), 1);
>  }
>  
>  /*
> @@ -4031,7 +4031,7 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
>  	if (id)
> -		pci_add_dma_alias(dev, id->driver_data);
> +		pci_add_dma_alias(dev, id->driver_data, 1);
>  }
>  
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
> @@ -4073,9 +4073,9 @@ DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>   */
>  static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
>  {
> -	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
> -	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
> -	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0), 1);
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0), 1);
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3), 1);
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
> @@ -5273,7 +5273,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev *pdev)
>  			pci_dbg(pdev,
>  				"Aliasing Partition %d Proxy ID %02x.%d\n",
>  				pp, PCI_SLOT(devfn), PCI_FUNC(devfn));
> -			pci_add_dma_alias(pdev, devfn);
> +			pci_add_dma_alias(pdev, devfn, 1);
>  		}
>  	}
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1a6cf19eac2d..84a8d4c2b24e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2323,7 +2323,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  }
>  #endif
>  
> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns);
>  bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
>  int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			   int (*fn)(struct pci_dev *pdev,
> 

Re: [PATCH v4 1/2] PCI: Add parameter nr_devfns to pci_add_dma_alias

[James Sewart]

> On 29 Nov 2019, at 16:50, Logan Gunthorpe <logang@deltatee.com> wrote:
> 
> 
> 
> On 2019-11-29 5:49 a.m., James Sewart wrote:
>> pci_add_dma_alias can now be used to create a dma alias for a range of
>> devfns.
>> 
>> Signed-off-by: James Sewart <jamessewart@arista.com>
>> ---
>> drivers/pci/pci.c    | 23 ++++++++++++++++++-----
>> drivers/pci/quirks.c | 14 +++++++-------
>> include/linux/pci.h  |  2 +-
>> 3 files changed, 26 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index a97e2571a527..9b0e3481fe17 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>> /**
>>  * pci_add_dma_alias - Add a DMA devfn alias for a device
>>  * @dev: the PCI device for which alias is added
>> - * @devfn: alias slot and function
>> + * @devfn_from: alias slot and function
>> + * @nr_devfns: Number of subsequent devfns to alias
>>  *
>>  * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
>>  * which is used to program permissible bus-devfn source addresses for DMA
>> @@ -5873,8 +5874,14 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>>  * cannot be left as a userspace activity).  DMA aliases should therefore
>>  * be configured via quirks, such as the PCI fixup header quirk.
>>  */
>> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
>> {
>> +	int devfn_to;
>> +
>> +	if (nr_devfns > U8_MAX+1)
>> +		nr_devfns = U8_MAX+1;
> 
> Why +1? That doesn't seem right to me….

U8_MAX is the max number U8 can represent(255) but is one less than the number 
of values it can represent(256). devfns can be 0.0 to 1f.7 inclusive(I think) 
so the number of possible devfns is 256. Thinking about it, maybe the 
zalloc should be U8_MAX+1 too?

I might be wrong though, what do you reckon?

> 
>> +	devfn_to = devfn_from + nr_devfns - 1;
>> +
>> 	if (!dev->dma_alias_mask)
>> 		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
>> 	if (!dev->dma_alias_mask) {
>> @@ -5882,9 +5889,15 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>> 		return;
>> 	}
>> 
>> -	set_bit(devfn, dev->dma_alias_mask);
>> -	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
>> -		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +	bitmap_set(dev->dma_alias_mask, devfn_from, nr_devfns);
>> +
>> +	if (nr_devfns == 1)
>> +		pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
>> +				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from));
>> +	else if(nr_devfns > 1)
>> +		pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
>> +				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from),
>> +				PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
>> }
>> 
>> bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 320255e5e8f8..0f3f5afc73fd 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3932,7 +3932,7 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>> static void quirk_dma_func0_alias(struct pci_dev *dev)
>> {
>> 	if (PCI_FUNC(dev->devfn) != 0)
>> -		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
>> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0), 1);
>> }
>> 
>> /*
>> @@ -3946,7 +3946,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
>> static void quirk_dma_func1_alias(struct pci_dev *dev)
>> {
>> 	if (PCI_FUNC(dev->devfn) != 1)
>> -		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
>> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1), 1);
>> }
>> 
>> /*
>> @@ -4031,7 +4031,7 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>> 
>> 	id = pci_match_id(fixed_dma_alias_tbl, dev);
>> 	if (id)
>> -		pci_add_dma_alias(dev, id->driver_data);
>> +		pci_add_dma_alias(dev, id->driver_data, 1);
>> }
>> 
>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
>> @@ -4073,9 +4073,9 @@ DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>>  */
>> static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
>> {
>> -	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
>> -	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
>> -	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
>> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0), 1);
>> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0), 1);
>> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3), 1);
>> }
>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
>> @@ -5273,7 +5273,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev *pdev)
>> 			pci_dbg(pdev,
>> 				"Aliasing Partition %d Proxy ID %02x.%d\n",
>> 				pp, PCI_SLOT(devfn), PCI_FUNC(devfn));
>> -			pci_add_dma_alias(pdev, devfn);
>> +			pci_add_dma_alias(pdev, devfn, 1);
>> 		}
>> 	}
>> 
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 1a6cf19eac2d..84a8d4c2b24e 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -2323,7 +2323,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>> }
>> #endif
>> 
>> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
>> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns);
>> bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
>> int pci_for_each_dma_alias(struct pci_dev *pdev,
>> 			   int (*fn)(struct pci_dev *pdev,
>> 

Re: [PATCH v4 1/2] PCI: Add parameter nr_devfns to pci_add_dma_alias

[Logan Gunthorpe]

On 2019-11-29 10:19 a.m., James Sewart wrote:
> 
> 
>> On 29 Nov 2019, at 16:50, Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>>
>>
>> On 2019-11-29 5:49 a.m., James Sewart wrote:
>>> pci_add_dma_alias can now be used to create a dma alias for a range of
>>> devfns.
>>>
>>> Signed-off-by: James Sewart <jamessewart@arista.com>
>>> ---
>>> drivers/pci/pci.c    | 23 ++++++++++++++++++-----
>>> drivers/pci/quirks.c | 14 +++++++-------
>>> include/linux/pci.h  |  2 +-
>>> 3 files changed, 26 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index a97e2571a527..9b0e3481fe17 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>>> /**
>>>  * pci_add_dma_alias - Add a DMA devfn alias for a device
>>>  * @dev: the PCI device for which alias is added
>>> - * @devfn: alias slot and function
>>> + * @devfn_from: alias slot and function
>>> + * @nr_devfns: Number of subsequent devfns to alias
>>>  *
>>>  * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
>>>  * which is used to program permissible bus-devfn source addresses for DMA
>>> @@ -5873,8 +5874,14 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>>>  * cannot be left as a userspace activity).  DMA aliases should therefore
>>>  * be configured via quirks, such as the PCI fixup header quirk.
>>>  */
>>> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>>> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
>>> {
>>> +	int devfn_to;
>>> +
>>> +	if (nr_devfns > U8_MAX+1)
>>> +		nr_devfns = U8_MAX+1;
>>
>> Why +1? That doesn't seem right to me….
> 
> U8_MAX is the max number U8 can represent(255) but is one less than the number 
> of values it can represent(256). devfns can be 0.0 to 1f.7 inclusive(I think) 
> so the number of possible devfns is 256. Thinking about it, maybe the 
> zalloc should be U8_MAX+1 too?
> 
> I might be wrong though, what do you reckon?

Right, yes, but I actually think the original code is wrong. It's only
allocating a bitmap that holds 255 values and you're trying to insert
256 ones into that bitmap. It's probably ok, because there's no way to
allocate an odd sized bitmap, but it's probably worth fixing for clarity.

It also looks wrong in pci_for_each_dma_alias() as well, so I'd probably
fix both those up in a separate prep-patch.

Logan

[PATCH v5 1/3] PCI: Fix off by one in dma_alias_mask allocation size

[James Sewart]

The number of possible devfns is 256 so the size of the bitmap for
allocations should be U8_MAX+1.

Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/pci.c    | 2 +-
 drivers/pci/search.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a527..0a4449a30ace 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5876,7 +5876,7 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
 	if (!dev->dma_alias_mask)
-		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
+		dev->dma_alias_mask = bitmap_zalloc(U8_MAX+1, GFP_KERNEL);
 	if (!dev->dma_alias_mask) {
 		pci_warn(dev, "Unable to allocate DMA alias mask\n");
 		return;
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index bade14002fd8..b3633af1743b 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -43,7 +43,7 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 	if (unlikely(pdev->dma_alias_mask)) {
 		u8 devfn;
 
-		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
+		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX+1) {
 			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
 				 data);
 			if (ret)
-- 
2.24.0

[PATCH v5 2/3] PCI: Add parameter nr_devfns to pci_add_dma_alias

[James Sewart]

pci_add_dma_alias can now be used to create a dma alias for a range of
devfns.

Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/pci.c    | 23 ++++++++++++++++++-----
 drivers/pci/quirks.c | 14 +++++++-------
 include/linux/pci.h  |  2 +-
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0a4449a30ace..f9800a610ca1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 /**
  * pci_add_dma_alias - Add a DMA devfn alias for a device
  * @dev: the PCI device for which alias is added
- * @devfn: alias slot and function
+ * @devfn_from: alias slot and function
+ * @nr_devfns: Number of subsequent devfns to alias
  *
  * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
  * which is used to program permissible bus-devfn source addresses for DMA
@@ -5873,8 +5874,14 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  * cannot be left as a userspace activity).  DMA aliases should therefore
  * be configured via quirks, such as the PCI fixup header quirk.
  */
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
 {
+	int devfn_to;
+
+	if (nr_devfns > U8_MAX+1)
+		nr_devfns = U8_MAX+1;
+	devfn_to = devfn_from + nr_devfns - 1;
+
 	if (!dev->dma_alias_mask)
 		dev->dma_alias_mask = bitmap_zalloc(U8_MAX+1, GFP_KERNEL);
 	if (!dev->dma_alias_mask) {
@@ -5882,9 +5889,15 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 		return;
 	}
 
-	set_bit(devfn, dev->dma_alias_mask);
-	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
-		 PCI_SLOT(devfn), PCI_FUNC(devfn));
+	bitmap_set(dev->dma_alias_mask, devfn_from, nr_devfns);
+
+	if (nr_devfns == 1)
+		pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
+				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from));
+	else if (nr_devfns > 1)
+		pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
+				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from),
+				PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
 }
 
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 320255e5e8f8..0f3f5afc73fd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3932,7 +3932,7 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 static void quirk_dma_func0_alias(struct pci_dev *dev)
 {
 	if (PCI_FUNC(dev->devfn) != 0)
-		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0), 1);
 }
 
 /*
@@ -3946,7 +3946,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
 static void quirk_dma_func1_alias(struct pci_dev *dev)
 {
 	if (PCI_FUNC(dev->devfn) != 1)
-		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1), 1);
 }
 
 /*
@@ -4031,7 +4031,7 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
 	if (id)
-		pci_add_dma_alias(dev, id->driver_data);
+		pci_add_dma_alias(dev, id->driver_data, 1);
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
@@ -4073,9 +4073,9 @@ DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
  */
 static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
 {
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
-	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0), 1);
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0), 1);
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3), 1);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
@@ -5273,7 +5273,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev *pdev)
 			pci_dbg(pdev,
 				"Aliasing Partition %d Proxy ID %02x.%d\n",
 				pp, PCI_SLOT(devfn), PCI_FUNC(devfn));
-			pci_add_dma_alias(pdev, devfn);
+			pci_add_dma_alias(pdev, devfn, 1);
 		}
 	}
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1a6cf19eac2d..84a8d4c2b24e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2323,7 +2323,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 }
 #endif
 
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns);
 bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
-- 
2.24.0

[PATCH v5 3/3] PCI: Add DMA alias quirk for PLX PEX NTB

[James Sewart]

The PLX PEX NTB forwards DMA transactions using Requester ID's that
don't exist as PCI devices. The devfn for a transaction is used as an
index into a lookup table storing the origin of a transaction on the
other side of the bridge.

This patch aliases all possible devfn's to the NTB device so that any
transaction coming in is governed by the mappings for the NTB.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: James Sewart <jamessewart@arista.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0f3f5afc73fd..3a67049ca630 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5315,6 +5315,21 @@ SWITCHTEC_QUIRK(0x8574);  /* PFXI 64XG3 */
 SWITCHTEC_QUIRK(0x8575);  /* PFXI 80XG3 */
 SWITCHTEC_QUIRK(0x8576);  /* PFXI 96XG3 */
 
+/*
+ * PLX NTB uses devfn proxy IDs to move TLPs between NT endpoints. These IDs
+ * are used to forward responses to the originator on the other side of the
+ * NTB. Alias all possible IDs to the NTB to permit access when the IOMMU is
+ * turned on.
+ */
+static void quirk_plx_ntb_dma_alias(struct pci_dev *pdev)
+{
+	pci_info(pdev, "Setting PLX NTB proxy ID aliases\n");
+	/* PLX NTB may use all 256 devfns */
+	pci_add_dma_alias(pdev, 0, 256);
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b0, quirk_plx_ntb_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, 0x87b1, quirk_plx_ntb_dma_alias);
+
 /*
  * On Lenovo Thinkpad P50 SKUs with a Nvidia Quadro M1000M, the BIOS does
  * not always reset the secondary Nvidia GPU between reboots if the system
-- 
2.24.0

Re: [PATCH v5 2/3] PCI: Add parameter nr_devfns to pci_add_dma_alias

[Christoph Hellwig]

On Fri, Nov 29, 2019 at 05:56:55PM +0000, James Sewart wrote:
> pci_add_dma_alias can now be used to create a dma alias for a range of
> devfns.
> 
> Signed-off-by: James Sewart <jamessewart@arista.com>
> ---
>  drivers/pci/pci.c    | 23 ++++++++++++++++++-----
>  drivers/pci/quirks.c | 14 +++++++-------
>  include/linux/pci.h  |  2 +-
>  3 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0a4449a30ace..f9800a610ca1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  /**
>   * pci_add_dma_alias - Add a DMA devfn alias for a device
>   * @dev: the PCI device for which alias is added
> - * @devfn: alias slot and function
> + * @devfn_from: alias slot and function
> + * @nr_devfns: Number of subsequent devfns to alias
>   *
>   * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
>   * which is used to program permissible bus-devfn source addresses for DMA
> @@ -5873,8 +5874,14 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   * cannot be left as a userspace activity).  DMA aliases should therefore
>   * be configured via quirks, such as the PCI fixup header quirk.
>   */
> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
>  {
> +	int devfn_to;
> +
> +	if (nr_devfns > U8_MAX+1)
> +		nr_devfns = U8_MAX+1;

Missing whitespaces here as well.  Also this could use max() and I
think you want a documented constants for MAX_NR_DEVFNS that documents
this "not off by one".

Re: [PATCH v5 1/3] PCI: Fix off by one in dma_alias_mask allocation size

[Logan Gunthorpe]

On 2019-11-29 10:56 a.m., James Sewart wrote:
> The number of possible devfns is 256 so the size of the bitmap for
> allocations should be U8_MAX+1.
> 
> Signed-off-by: James Sewart <jamessewart@arista.com>

Looks good to me. Thanks! For the whole series:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> ---
>  drivers/pci/pci.c    | 2 +-
>  drivers/pci/search.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a97e2571a527..0a4449a30ace 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5876,7 +5876,7 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
>  	if (!dev->dma_alias_mask)
> -		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
> +		dev->dma_alias_mask = bitmap_zalloc(U8_MAX+1, GFP_KERNEL);
>  	if (!dev->dma_alias_mask) {
>  		pci_warn(dev, "Unable to allocate DMA alias mask\n");
>  		return;
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index bade14002fd8..b3633af1743b 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -43,7 +43,7 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>  	if (unlikely(pdev->dma_alias_mask)) {
>  		u8 devfn;
>  
> -		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
> +		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX+1) {
>  			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
>  				 data);
>  			if (ret)
> 

Re: [PATCH v5 1/3] PCI: Fix off by one in dma_alias_mask allocation size

[Christoph Hellwig]

On Fri, Nov 29, 2019 at 05:56:19PM +0000, James Sewart wrote:
> The number of possible devfns is 256 so the size of the bitmap for
> allocations should be U8_MAX+1.
> 
> Signed-off-by: James Sewart <jamessewart@arista.com>
> ---
>  drivers/pci/pci.c    | 2 +-
>  drivers/pci/search.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a97e2571a527..0a4449a30ace 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5876,7 +5876,7 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
>  	if (!dev->dma_alias_mask)
> -		dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL);
> +		dev->dma_alias_mask = bitmap_zalloc(U8_MAX+1, GFP_KERNEL);

Missing whitespaces around the "+".

> -		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
> +		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX+1) {

Same here.