Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on
On 2019-11-20 10:48 a.m., Dmitry Safonov wrote:
> +Cc: linux-...@vger.kernel.org
> +Cc: Bjorn Helgaas
> +Cc: Logan Gunthorpe
>
> 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 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
>>> ---
>>> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on
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 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
> >>> ---
> >>> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on
[+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 +, Dmitry Safonov wrote:
> On 11/5/19 12:17 PM, James Sewart wrote:
> >> On 24 Oct 2019, at 13:52, 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.
> >>
> >> 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
Conventionally capitalized as:
Signed-off-by: James Sewart
> >> ---
> >> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on
+Cc: linux-...@vger.kernel.org
+Cc: Bjorn Helgaas
+Cc: Logan Gunthorpe
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 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
>> ---
>> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on
Any comments on this?
Cheers,
James.
> On 24 Oct 2019, at 13:52, 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.
>
> 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
> ---
> 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
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] Ensure pci transactions coming from PLX NTB are handled when IOMMU is turned on
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
---
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
