PCI: dwc: Add support for 64-bit MSI target addresses
v4 cover PCI: dwc: Add support for 64-bit MSI target addresses
Hi All,
I've updated the series to address the review comments. Refer to the v4 history below for the changes. Please take a look and thanks again for the reviews!
Regards,
Will
Will McVicker (2):
PCI: dwc: Drop dependency on ZONE_DMA32
PCI: dwc: Add support for 64-bit MSI target address
v4:
* Updated commit descriptions.
* Renamed msi_64b -> msi_64bit.
* Dropped msi_64bit ternary use.
* Dropped export of dw_pcie_msi_capabilities.
v3:
* Switched to a managed DMA allocation.
* Simplified the DMA allocation cleanup.
* Dropped msi_page from struct dw_pcie_rp.
* Allocating a u64 instead of a full page.
v2:
* Fixed build error caught by kernel test robot
* Fixed error handling reported by Isaac Manjarres
.../pci/controller/dwc/pcie-designware-host.c | 42 +++++++++---------- drivers/pci/controller/dwc/pcie-designware.c | 8 ++++ drivers/pci/controller/dwc/pcie-designware.h | 2 +- 3 files changed, 28 insertions(+), 24 deletions(-)
base-commit: 2ae08b36c06ea8df73a79f6b80ff7964e006e9e3
Signature · 1 line
2.37.1.559.g78731f0fdb-goog
PATCH v4 1/2 PCI: dwc: Drop dependency on ZONE_DMA32
Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which uses the coherent DMA mask to try to return an allocation within the DMA mask limits. With that, we now can drop the msi_page parameter in struct dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to continue supporting a 32-bit DMA mask. Without this patch, the PCIe host device will fail to probe when ZONE_DMA32 is disabled.
--- .../pci/controller/dwc/pcie-designware-host.c | 28 +++++-------------- drivers/pci/controller/dwc/pcie-designware.h | 1 - 2 files changed, 7 insertions(+), 22 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +7 -21
@@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)irq_domain_remove (pp->msi_domain);irq_domain_remove (pp->irq_domain); - -if (pp->msi_data) { -struct dw_pcie *pci =to_dw_pcie_from_pp (pp); -struct device *dev = pci->dev; - -dma_unmap_page (dev, pp->msi_data,PAGE_SIZE ,DMA_FROM_DEVICE ); -if (pp->msi_page) -__free_page (pp->msi_page); - } }static void dw_pcie_msi_init (struct dw_pcie_rp *pp)
@@ -336,6 +327,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)struct dw_pcie *pci =to_dw_pcie_from_pp (pp);struct device *dev = pci->dev;struct platform_device *pdev =to_platform_device (dev); +u64 *msi_vaddr;int ret;u32 ctrl, num_ctrls;
@@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) dw_chained_msi_isr, pp); } - ret =dma_set_mask (dev,DMA_BIT_MASK (32 )); + ret =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 ));if (ret)dev_warn (dev,"Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n" ); - pp->msi_page =alloc_page (GFP_DMA32 ); - pp->msi_data =dma_map_page (dev, pp->msi_page,0 , -PAGE_SIZE ,DMA_FROM_DEVICE ); - ret =dma_mapping_error (dev, pp->msi_data); -if (ret) { -dev_err (pci->dev,"Failed to map MSI data\n" ); -__free_page (pp->msi_page); - pp->msi_page =NULL ; - pp->msi_data =0 ; + msi_vaddr =dmam_alloc_coherent (dev,sizeof (u64), &pp->msi_data, +GFP_KERNEL ); +if (!msi_vaddr) { +dev_err (dev,"Failed to alloc and map MSI data\n" );dw_pcie_free_msi (pp); - -return ret; +return -ENOMEM ; }return 0 ;
drivers/pci/controller/dwc/pcie-designware.h +0 -1
@@ -243,7 +243,6 @@ struct dw_pcie_rp {struct irq_domain *irq_domain;struct irq_domain *msi_domain;dma_addr_t msi_data; -struct page *msi_page;struct irq_chip *msi_irq_chip;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ];
Signature · 1 line
2.37.1.559.g78731f0fdb-goog
Re: [PATCH v4 1/2] PCI: dwc: Drop dependency on ZONE_DMA32
Quoted 10 lines · Will McVicker
Re-work the
msi_msgDMA allocation logic to usedmam_alloc_coherent()which uses the coherent DMA mask to try to return an allocation within the DMA mask limits. With that, we now can drop themsi_pageparameter in structdw_pcie_rp. This allows kernel configurations that disableZONE_DMA32to continue supporting a 32-bit DMA mask. Without this patch, the PCIe host device will fail to probe whenZONE_DMA32is disabled.Fixes 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")Reported-by Isaac J. Manjarres <[email protected]>Signed-off-by Will McVicker <[email protected]>
Quoted 4 lines
---
.../pci/controller/dwc/pcie-designware-host.c| 28 +++++--------------drivers/pci/controller/dwc/pcie-designware.h| 1 -2 files changed, 7 insertions(+), 22 deletions(-)
…..
Re: [PATCH v4 1/2] PCI: dwc: Drop dependency on ZONE_DMA32
In reply to Will McVickerQuoted 8 lines · Will McVicker
Re-work the
msi_msgDMA allocation logic to usedmam_alloc_coherent()which uses the coherent DMA mask to try to return an allocation within the DMA mask limits. With that, we now can drop themsi_pageparameter in structdw_pcie_rp. This allows kernel configurations that disableZONE_DMA32to continue supporting a 32-bit DMA mask. Without this patch, the PCIe host device will fail to probe whenZONE_DMA32is disabled.
Quoted 7 lines
Fixes 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")Reported-by Isaac J. Manjarres <[email protected]>Signed-off-by Will McVicker <[email protected]>--- .../pci/controller/dwc/pcie-designware-host.c | 28 +++++-------------- drivers/pci/controller/dwc/pcie-designware.h | 1 - 2 files changed, 7 insertions(+), 22 deletions(-)
PATCH v4 2/2 PCI: dwc: Add support for 64-bit MSI target address
In reply to Will McVickerSince not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable ZONE_DMA32 and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate the msi_msg page due to a DMA address overflow (seen in [1]). With this patch, the PCIe device can advertise 64-bit support via its MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.
[1] https://lore.kernel.org/all/[email protected]/
--- drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++++++-- drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +12 -2
@@ -330,6 +330,8 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)u64 *msi_vaddr;int ret;u32 ctrl, num_ctrls; +bool msi_64bit = false; +u16 msi_capabilities;for (ctrl =0 ; ctrl <MAX_MSI_CTRLS ; ctrl++) pp->irq_mask[ctrl] = ~0 ;
@@ -367,9 +369,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) dw_chained_msi_isr, pp); } - ret =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + msi_capabilities =dw_pcie_msi_capabilities (pci); +if (msi_capabilities &PCI_MSI_FLAGS_ENABLE ) + msi_64bit = msi_capabilities &PCI_MSI_FLAGS_64BIT ; + +dev_dbg (dev,"Setting MSI DMA mask to %s-bit.\n" , + msi_64bit ?"64" :"32" ); + ret =dma_set_mask_and_coherent (dev, msi_64bit ? +DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));if (ret) -dev_warn (dev,"Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n" ); +dev_warn (dev,"Failed to set DMA mask to %s-bit.\n" , + msi_64bit ?"64" :"32" ); msi_vaddr =dmam_alloc_coherent (dev,sizeof (u64), &pp->msi_data,GFP_KERNEL );
drivers/pci/controller/dwc/pcie-designware.c +8 -0
@@ -82,6 +82,14 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap) }EXPORT_SYMBOL_GPL (dw_pcie_find_capability); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci) +{ +u8 offset; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_MSI ); +return dw_pcie_readw_dbi (pci, offset +PCI_MSI_FLAGS ); +} +static u16 dw_pcie_find_next_ext_capability (struct dw_pcie *pci,u16 start,u8 cap) {
drivers/pci/controller/dwc/pcie-designware.h +1 -0
@@ -332,6 +332,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);u8 dw_pcie_find_capability (struct dw_pcie *pci,u8 cap);u16 dw_pcie_find_ext_capability (struct dw_pcie *pci,u8 cap); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci);int dw_pcie_read (void __iomem *addr,int size,u32 *val);int dw_pcie_write (void __iomem *addr,int size,u32 val);
Signature · 1 line
2.37.1.559.g78731f0fdb-goog
Re: [PATCH v4 2/2] PCI: dwc: Add support for 64-bit MSI target address
Quoted 11 lines · Will McVicker
Since not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable
ZONE_DMA32and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate themsi_msgpage due to a DMA address overflow (seen in [1]). With this patch, the PCIe device can advertise 64-bit support via its MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.
Quoted 5 lines
[1] https://lore.kernel.org/all/[email protected]/
Reported-by kernel test robot <[email protected]>Signed-off-by Will McVicker <[email protected]>Reviewed-by Rob Herring <[email protected]>
Quoted 5 lines
---
drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++++++-- drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)
…..
Re: [PATCH v4 2/2] PCI: dwc: Add support for 64-bit MSI target address
In reply to Will McVickerQuoted 10 lines · Will McVicker
Since not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable
ZONE_DMA32and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate themsi_msgpage due to a DMA address overflow (seen in [1]). With this patch, the PCIe device can advertise 64-bit support via its MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.
It doesn't matter so much what the host's own capabilities are, though, what matters is that if you configure the MSI doorbell to decode some 64-bit address because you can, and later an endpoint shows up whose DMA mask doesn't reach that address, you've broken MSIs for that endpoint. While the host is probing, it cannot possibly know what future endpoint drivers may or may not do.
Now, in the case where no ZONE_DMA{32} is configured, there's a fair likelihood that addressing-constrained endpoints are probably going to be broken in general, so that should not be fatal for the host. So *if* we fail to allocate a 32-bit MSI address, *then* we should fall back to a 64-bit one (when the host supports it), but to preserve compatibility we still must always attempt the 32-bit allocation first (oh, if there isn't a theme emerging here...)
TBH this has come up enough times, and has enough subtle complexity, that I think it's high time to factor this all out into a helper between the PCI core code and the DMA API for "please give me a bus address that's guaranteed not to conflict with any valid DMA address".
Thanks,
Robin.
Quoted 75 lines
[1] https://lore.kernel.org/all/[email protected]/
Reported-by kernel test robot <[email protected]>Signed-off-by Will McVicker <[email protected]>Reviewed-by Rob Herring <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++++++-- drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)drivers/pci/controller/dwc/pcie-designware-host.c +12
-2@@ -330,6 +330,8 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)u64 *msi_vaddr;int ret;u32 ctrl, num_ctrls; +bool msi_64bit = false; +u16 msi_capabilities;for (ctrl =0 ; ctrl <MAX_MSI_CTRLS ; ctrl++) pp->irq_mask[ctrl] = ~0 ;@@ -367,9 +369,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) dw_chained_msi_isr, pp); } - ret =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + msi_capabilities =dw_pcie_msi_capabilities (pci); +if (msi_capabilities &PCI_MSI_FLAGS_ENABLE ) + msi_64bit = msi_capabilities &PCI_MSI_FLAGS_64BIT ; + +dev_dbg (dev,"Setting MSI DMA mask to %s-bit.\n" , + msi_64bit ?"64" :"32" ); + ret =dma_set_mask_and_coherent (dev, msi_64bit ? +DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));if (ret) -dev_warn (dev,"Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n" ); +dev_warn (dev,"Failed to set DMA mask to %s-bit.\n" , + msi_64bit ?"64" :"32" ); msi_vaddr =dmam_alloc_coherent (dev,sizeof (u64), &pp->msi_data,GFP_KERNEL );drivers/pci/controller/dwc/pcie-designware.c +8
-0@@ -82,6 +82,14 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap) }EXPORT_SYMBOL_GPL (dw_pcie_find_capability); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci) +{ +u8 offset; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_MSI ); +return dw_pcie_readw_dbi (pci, offset +PCI_MSI_FLAGS ); +} +static u16 dw_pcie_find_next_ext_capability (struct dw_pcie *pci,u16 start,u8 cap) {drivers/pci/controller/dwc/pcie-designware.h +1
-0@@ -332,6 +332,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);u8 dw_pcie_find_capability (struct dw_pcie *pci,u8 cap);u16 dw_pcie_find_ext_capability (struct dw_pcie *pci,u8 cap); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci);int dw_pcie_read (void __iomem *addr,int size,u32 *val);int dw_pcie_write (void __iomem *addr,int size,u32 val);
Re: [PATCH v4 2/2] PCI: dwc: Add support for 64-bit MSI target address
Quoted 27 lines · Robin Murphy
Quoted 10 lines · Will McVicker
Since not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable
ZONE_DMA32and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate themsi_msgpage due to a DMA address overflow (seen in [1]). With this patch, the PCIe device can advertise 64-bit support via its MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.It doesn't matter so much what the host's own capabilities are, though, what matters is that if you configure the MSI doorbell to decode some 64-bit address because you can, and later an endpoint shows up whose DMA mask doesn't reach that address, you've broken MSIs for that endpoint. While the host is probing, it cannot possibly know what future endpoint drivers may or may not do.
Now, in the case where no ZONE_DMA{32} is configured, there's a fair likelihood that addressing-constrained endpoints are probably going to be broken in general, so that should not be fatal for the host. So *if* we fail to allocate a 32-bit MSI address, *then* we should fall back to a 64-bit one (when the host supports it), but to preserve compatibility we still must always attempt the 32-bit allocation first (oh, if there isn't a theme emerging here...)
Quoted 7 lines
TBH this has come up enough times, and has enough subtle complexity, that I think it's high time to factor this all out into a helper between the PCI core code and the DMA API for "please give me a bus address that's guaranteed not to conflict with any valid DMA address".
Thanks,
Robin.
Hi Robin,
Thanks for the reviews! I'm happy with your suggestion of trying a 32-bit mask first for the reasons you mentioned. I have tested this out on my Pixel 6 and db845c and will push the updated patchset as v5.
Regards,
Will
Quoted 70 lines
Quoted 70 lines
[1] https://lore.kernel.org/all/[email protected]/
Reported-by kernel test robot <[email protected]>Signed-off-by Will McVicker <[email protected]>Reviewed-by Rob Herring <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++++++-- drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)drivers/pci/controller/dwc/pcie-designware-host.c +12
-2@@ -330,6 +330,8 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)u64 *msi_vaddr;int ret;u32 ctrl, num_ctrls; +bool msi_64bit = false; +u16 msi_capabilities;for (ctrl =0 ; ctrl <MAX_MSI_CTRLS ; ctrl++) pp->irq_mask[ctrl] = ~0 ;@@ -367,9 +369,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) dw_chained_msi_isr, pp); } - ret =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + msi_capabilities =dw_pcie_msi_capabilities (pci); +if (msi_capabilities &PCI_MSI_FLAGS_ENABLE ) + msi_64bit = msi_capabilities &PCI_MSI_FLAGS_64BIT ; + +dev_dbg (dev,"Setting MSI DMA mask to %s-bit.\n" , + msi_64bit ?"64" :"32" ); + ret =dma_set_mask_and_coherent (dev, msi_64bit ? +DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));if (ret) -dev_warn (dev,"Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n" ); +dev_warn (dev,"Failed to set DMA mask to %s-bit.\n" , + msi_64bit ?"64" :"32" ); msi_vaddr =dmam_alloc_coherent (dev,sizeof (u64), &pp->msi_data,GFP_KERNEL );drivers/pci/controller/dwc/pcie-designware.c +8
-0@@ -82,6 +82,14 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap) }EXPORT_SYMBOL_GPL (dw_pcie_find_capability); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci) +{ +u8 offset; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_MSI ); +return dw_pcie_readw_dbi (pci, offset +PCI_MSI_FLAGS ); +} +static u16 dw_pcie_find_next_ext_capability (struct dw_pcie *pci,u16 start,u8 cap) {drivers/pci/controller/dwc/pcie-designware.h +1
-0@@ -332,6 +332,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);u8 dw_pcie_find_capability (struct dw_pcie *pci,u8 cap);u16 dw_pcie_find_ext_capability (struct dw_pcie *pci,u8 cap); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci);int dw_pcie_read (void __iomem *addr,int size,u32 *val);int dw_pcie_write (void __iomem *addr,int size,u32 val);
Lorenzo Pieralisi
maintainerQuoted 5 lines · Will McVicker
Hi All,
I've updated the series to address the review comments. Refer to the v4 history below for the changes. Please take a look and thanks again for the reviews!
This series looks OK to me - it'd be good if Christoph/Robin can have a look though before merging it.
Lorenzo
Quoted 33 lines
Regards,
WillWill McVicker (2):
PCI: dwc: Drop dependency onZONE_DMA32
PCI: dwc: Add support for 64-bit MSI target addressv4:
* Updated commit descriptions.
* Renamedmsi_64b->msi_64bit.
* Droppedmsi_64bitternary use.
* Dropped export ofdw_pcie_msi_capabilities.v3:
* Switched to a managed DMA allocation.
* Simplified the DMA allocation cleanup.
* Droppedmsi_pagefrom structdw_pcie_rp.
* Allocating a u64 instead of a full page.v2:
* Fixed build error caught by kernel test robot
* Fixed error handling reported by Isaac Manjarres.../pci/controller/dwc/pcie-designware-host.c | 42 +++++++++---------- drivers/pci/controller/dwc/pcie-designware.c | 8 ++++ drivers/pci/controller/dwc/pcie-designware.h | 2 +- 3 files changed, 28 insertions(+), 24 deletions(-)base-commit:
2ae08b36c06ea8df73a79f6b80ff7964e006e9e3Signature · 1 line
2.37.1.559.g78731f0fdb-goog