[PATCH] PCI/MSI: Improve MSI alias detection

[PATCH] PCI/MSI: Improve MSI alias detection

[Robin Murphy]

Currently, we consider all DMA aliases when calculating MSI requester
IDs. This turns out to be the wrong thing to do in the face of pure DMA
quirks like those of Marvell SATA cards, where we can end up configuring
the MSI doorbell to expect the phantom function alias, such that it then
goes on to happily ignore actual MSI writes coming from the card on the
real RID.

Improve the alias walk to only account for the topological aliases that
matter, based on the logic from the Intel IRQ remapping code.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/msi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index ba44fdfda66b..7b34c434970e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1468,13 +1468,21 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
 
+/*
+ * If the alias device/RID is on a different bus, it's a topological alias
+ * we should care about; otherwise, it's a DMA quirk and we don't.
+ */
 static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
 {
 	u32 *pa = data;
+	u8 bus = PCI_BUS_NUM(*pa);
+
+	if (pdev->bus->number != bus || PCI_BUS_NUM(alias) != bus)
+		*pa = alias;
 
-	*pa = alias;
 	return 0;
 }
+
 /**
  * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
  * @domain:	The interrupt domain
@@ -1488,7 +1496,7 @@ static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
 u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
 {
 	struct device_node *of_node;
-	u32 rid = 0;
+	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
 
 	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
 
@@ -1504,14 +1512,14 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
  * @pdev:	The PCI device
  *
  * Use the firmware data to find a device-specific MSI domain
- * (i.e. not one that is ste as a default).
+ * (i.e. not one that is set as a default).
  *
- * Returns: The coresponding MSI domain or NULL if none has been found.
+ * Returns: The corresponding MSI domain or NULL if none has been found.
  */
 struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
 {
 	struct irq_domain *dom;
-	u32 rid = 0;
+	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
 
 	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
 	dom = of_msi_map_get_device_domain(&pdev->dev, rid);
-- 
2.12.2.dirty

Re: [PATCH] PCI/MSI: Improve MSI alias detection

[Bjorn Helgaas]

[+cc David, Alex]

On Wed, May 31, 2017 at 06:52:28PM +0100, Robin Murphy wrote:
> Currently, we consider all DMA aliases when calculating MSI requester
> IDs. This turns out to be the wrong thing to do in the face of pure DMA
> quirks like those of Marvell SATA cards, where we can end up configuring
> the MSI doorbell to expect the phantom function alias, such that it then
> goes on to happily ignore actual MSI writes coming from the card on the
> real RID.

I guess you're making the assumption that DMA might use phantom
function aliases (aliases on the same bus as the actual device), but
MSI writes will not?

> Improve the alias walk to only account for the topological aliases that
> matter, based on the logic from the Intel IRQ remapping code.

Can you include a specific function reference here?  It sounds like
it'd be useful to compare this with the Intel IRQ remapping code.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/pci/msi.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index ba44fdfda66b..7b34c434970e 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1468,13 +1468,21 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
>  
> +/*
> + * If the alias device/RID is on a different bus, it's a topological alias
> + * we should care about; otherwise, it's a DMA quirk and we don't.
> + */
>  static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	u32 *pa = data;
> +	u8 bus = PCI_BUS_NUM(*pa);
> +
> +	if (pdev->bus->number != bus || PCI_BUS_NUM(alias) != bus)
> +		*pa = alias;
>  
> -	*pa = alias;

Aliases can arise both because of device defects, e.g., the phantom
function quirks, and because of bridges, e.g., all the stuff in
pci_for_each_dma_alias().

Why is it that the users of get_msi_id_cb() can get away with
collapsing any of these bridge aliases into the last one that
pci_for_each_dma_alias() finds?  I guess this is another question
about exactly why DMA and MSI are different here.

>  	return 0;
>  }
> +
>  /**
>   * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
>   * @domain:	The interrupt domain
> @@ -1488,7 +1496,7 @@ static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>  u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>  {
>  	struct device_node *of_node;
> -	u32 rid = 0;
> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>  
> @@ -1504,14 +1512,14 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>   * @pdev:	The PCI device
>   *
>   * Use the firmware data to find a device-specific MSI domain
> - * (i.e. not one that is ste as a default).
> + * (i.e. not one that is set as a default).
>   *
> - * Returns: The coresponding MSI domain or NULL if none has been found.
> + * Returns: The corresponding MSI domain or NULL if none has been found.
>   */
>  struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
>  {
>  	struct irq_domain *dom;
> -	u32 rid = 0;
> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>  	dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> -- 
> 2.12.2.dirty
> 

Re: [PATCH] PCI/MSI: Improve MSI alias detection

[Robin Murphy]

On 19/06/17 22:52, Bjorn Helgaas wrote:
> [+cc David, Alex]
> 
> On Wed, May 31, 2017 at 06:52:28PM +0100, Robin Murphy wrote:
>> Currently, we consider all DMA aliases when calculating MSI requester
>> IDs. This turns out to be the wrong thing to do in the face of pure DMA
>> quirks like those of Marvell SATA cards, where we can end up configuring
>> the MSI doorbell to expect the phantom function alias, such that it then
>> goes on to happily ignore actual MSI writes coming from the card on the
>> real RID.
> 
> I guess you're making the assumption that DMA might use phantom
> function aliases (aliases on the same bus as the actual device), but
> MSI writes will not?

Indeed - that's certainly the case for the Marvell 88SE9128 that I have
here (and it was a report involving a similar Marvell SATA card that
first brought this to our attention). I appreciate that there's not
necessarily a general right answer, but the generic MSI infrastructure
is rather built around the notion of a device having a single ID, so
this is really a case of picking the most likely compromise to cover the
greatest number of cases.

>> Improve the alias walk to only account for the topological aliases that
>> matter, based on the logic from the Intel IRQ remapping code.
> 
> Can you include a specific function reference here?  It sounds like
> it'd be useful to compare this with the Intel IRQ remapping code.

set_msi_sid() in drivers/iommu/intel_irq_remapping.c - one small
difference from VT-d is that here we don't have an equivalent of the
"verify bus" option, only of considering the full ID.

>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/pci/msi.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index ba44fdfda66b..7b34c434970e 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -1468,13 +1468,21 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>>  }
>>  EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
>>  
>> +/*
>> + * If the alias device/RID is on a different bus, it's a topological alias
>> + * we should care about; otherwise, it's a DMA quirk and we don't.
>> + */
>>  static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>>  {
>>  	u32 *pa = data;
>> +	u8 bus = PCI_BUS_NUM(*pa);
>> +
>> +	if (pdev->bus->number != bus || PCI_BUS_NUM(alias) != bus)
>> +		*pa = alias;
>>  
>> -	*pa = alias;
> 
> Aliases can arise both because of device defects, e.g., the phantom
> function quirks, and because of bridges, e.g., all the stuff in
> pci_for_each_dma_alias().
> 
> Why is it that the users of get_msi_id_cb() can get away with
> collapsing any of these bridge aliases into the last one that
> pci_for_each_dma_alias() finds?  I guess this is another question
> about exactly why DMA and MSI are different here.

As before, it's mostly just a case of trying to pick the least-worst
option to fit the constraints of the generic MSI layer (assuming a
bridge in the PCI->PCIe direction that is always going to introduce an
alias) - based on the assumption that the VT-d code has presumably
worked well enough in practice doing a similar thing - even if that
doesn't quite seem to match my reading of the bridge spec (AFAICS it may
be true for reads, but writes are somewhat murkier).

>From the DMA angle[1], we do have the ability to simply consider every
alias for translation at the IOMMU, since the IOMMU layer already has
provisions for devices having multiple IDs. Indeed in the Marvell case
it is certainly necessary to consider both the real and phantom
functions so that both DMA and MSIs even get through in the first place.

Robin.

[1]:https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg17979.html

>>  	return 0;
>>  }
>> +
>>  /**
>>   * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
>>   * @domain:	The interrupt domain
>> @@ -1488,7 +1496,7 @@ static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>>  u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>>  {
>>  	struct device_node *of_node;
>> -	u32 rid = 0;
>> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>>  
>>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>>  
>> @@ -1504,14 +1512,14 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>>   * @pdev:	The PCI device
>>   *
>>   * Use the firmware data to find a device-specific MSI domain
>> - * (i.e. not one that is ste as a default).
>> + * (i.e. not one that is set as a default).
>>   *
>> - * Returns: The coresponding MSI domain or NULL if none has been found.
>> + * Returns: The corresponding MSI domain or NULL if none has been found.
>>   */
>>  struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
>>  {
>>  	struct irq_domain *dom;
>> -	u32 rid = 0;
>> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>>  
>>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>>  	dom = of_msi_map_get_device_domain(&pdev->dev, rid);
>> -- 
>> 2.12.2.dirty
>>

Re: [PATCH] PCI/MSI: Improve MSI alias detection

[Bjorn Helgaas]

Hi Robin,

Can you post a new patch with the updates below, e.g., the fact that
MSI writes don't use phantom RIDs, the set_msi_sid() reference, a
comment about why it's safe for get_msi_id_cb() to ignore all but the
last alias, etc?

These are subtle things that other readers will likely wonder about
(and other platforms may trip over) so I'd like to leave a few
breadcrumbs in the changelog and code comments to help them out.

On Tue, Jun 20, 2017 at 12:03:25PM +0100, Robin Murphy wrote:
> On 19/06/17 22:52, Bjorn Helgaas wrote:
> > [+cc David, Alex]
> > 
> > On Wed, May 31, 2017 at 06:52:28PM +0100, Robin Murphy wrote:
> >> Currently, we consider all DMA aliases when calculating MSI requester
> >> IDs. This turns out to be the wrong thing to do in the face of pure DMA
> >> quirks like those of Marvell SATA cards, where we can end up configuring
> >> the MSI doorbell to expect the phantom function alias, such that it then
> >> goes on to happily ignore actual MSI writes coming from the card on the
> >> real RID.
> > 
> > I guess you're making the assumption that DMA might use phantom
> > function aliases (aliases on the same bus as the actual device), but
> > MSI writes will not?
> 
> Indeed - that's certainly the case for the Marvell 88SE9128 that I have
> here (and it was a report involving a similar Marvell SATA card that
> first brought this to our attention). I appreciate that there's not
> necessarily a general right answer, but the generic MSI infrastructure
> is rather built around the notion of a device having a single ID, so
> this is really a case of picking the most likely compromise to cover the
> greatest number of cases.
> 
> >> Improve the alias walk to only account for the topological aliases that
> >> matter, based on the logic from the Intel IRQ remapping code.
> > 
> > Can you include a specific function reference here?  It sounds like
> > it'd be useful to compare this with the Intel IRQ remapping code.
> 
> set_msi_sid() in drivers/iommu/intel_irq_remapping.c - one small
> difference from VT-d is that here we don't have an equivalent of the
> "verify bus" option, only of considering the full ID.
> 
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>  drivers/pci/msi.c | 18 +++++++++++++-----
> >>  1 file changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> >> index ba44fdfda66b..7b34c434970e 100644
> >> --- a/drivers/pci/msi.c
> >> +++ b/drivers/pci/msi.c
> >> @@ -1468,13 +1468,21 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
> >>  }
> >>  EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
> >>  
> >> +/*
> >> + * If the alias device/RID is on a different bus, it's a topological alias
> >> + * we should care about; otherwise, it's a DMA quirk and we don't.
> >> + */
> >>  static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
> >>  {
> >>  	u32 *pa = data;
> >> +	u8 bus = PCI_BUS_NUM(*pa);
> >> +
> >> +	if (pdev->bus->number != bus || PCI_BUS_NUM(alias) != bus)
> >> +		*pa = alias;
> >>  
> >> -	*pa = alias;
> > 
> > Aliases can arise both because of device defects, e.g., the phantom
> > function quirks, and because of bridges, e.g., all the stuff in
> > pci_for_each_dma_alias().
> > 
> > Why is it that the users of get_msi_id_cb() can get away with
> > collapsing any of these bridge aliases into the last one that
> > pci_for_each_dma_alias() finds?  I guess this is another question
> > about exactly why DMA and MSI are different here.
> 
> As before, it's mostly just a case of trying to pick the least-worst
> option to fit the constraints of the generic MSI layer (assuming a
> bridge in the PCI->PCIe direction that is always going to introduce an
> alias) - based on the assumption that the VT-d code has presumably
> worked well enough in practice doing a similar thing - even if that
> doesn't quite seem to match my reading of the bridge spec (AFAICS it may
> be true for reads, but writes are somewhat murkier).
> 
> From the DMA angle[1], we do have the ability to simply consider every
> alias for translation at the IOMMU, since the IOMMU layer already has
> provisions for devices having multiple IDs. Indeed in the Marvell case
> it is certainly necessary to consider both the real and phantom
> functions so that both DMA and MSIs even get through in the first place.
> 
> Robin.
> 
> [1]:https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg17979.html
> 
> >>  	return 0;
> >>  }
> >> +
> >>  /**
> >>   * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
> >>   * @domain:	The interrupt domain
> >> @@ -1488,7 +1496,7 @@ static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
> >>  u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
> >>  {
> >>  	struct device_node *of_node;
> >> -	u32 rid = 0;
> >> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> >>  
> >>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> >>  
> >> @@ -1504,14 +1512,14 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
> >>   * @pdev:	The PCI device
> >>   *
> >>   * Use the firmware data to find a device-specific MSI domain
> >> - * (i.e. not one that is ste as a default).
> >> + * (i.e. not one that is set as a default).
> >>   *
> >> - * Returns: The coresponding MSI domain or NULL if none has been found.
> >> + * Returns: The corresponding MSI domain or NULL if none has been found.
> >>   */
> >>  struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> >>  {
> >>  	struct irq_domain *dom;
> >> -	u32 rid = 0;
> >> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> >>  
> >>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> >>  	dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> >> -- 
> >> 2.12.2.dirty
> >>
>