DWC PCIe endpoint iATU vs BAR MASK ordering

DWC PCIe endpoint iATU vs BAR MASK ordering

[Niklas Cassel]

Hello DWC PCIe endpoint developers,


As I wrote in patch [1] (please review):
The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
"Field size depends on log2(BAR_MASK+1) in BAR match mode."

I.e. only the upper bits are writable, and the number of writable bits is
dependent on the configured BAR_MASK.

While patch [1] is a nice fail-safe that we definitely want to have...
I can see that the DWC PCIe EP driver is currently broken (and has been
since the beginning).

We are programming the iATU _before_ configuring the BAR:
https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L232-L247

The problem is of course that the iATU registers depend on the BAR mask
(the number of read-only bits depends on the currently set BAR mask),
so the iATU registers should be done _after_ configuring the BAR.

Doing it in this was makes a lot of sense.
First you configure the BAR, then you configure the address translation
for that BAR. (Since the iATU in BAR match mode obviously depends on how
the BAR is configured.)


Now, I would like to send a patch to change this order, so that we actually
write things in the only order that makes sense, my problem is this line:
https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L236-L237

This code makes no sense to me whatsoever.

If I look at the commit that introduced this early return:
4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")

It is not signed off by any of the regular PCI maintainers, neither does it
have any Acked-by by any of the regular PCI maintainers, so I kind of wonder
why this patch is there is the first place...
(Taking something via a different tree is fine, but that would still require
an Acked-by by one of the maintainers for the tree which owns that file.)

While I am tempted to simply send a revert for this commit, so that we can
write the registers in the correct order (iATU after BAR mask), I still do
not want to regress the NTB EPF driver (even if the commit appears to have
bypassed the regular PCI maintainers).

Frank, since you are the author of:
4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")

Please advice on what to do here. The only thing that makes sense to me is
that dw_pcie_ep_set_bar() always writes the BAR MASK. If dw_pcie_ep_set_bar()
does NOT write the BAR MASK, we depend on whatever BAR MASK was there before
dw_pcie_ep_set_bar(), which no matter how I look at it, seems horribly wrong.


Kind regards,
Niklas

[1] https://lore.kernel.org/linux-pci/20241116163558.2606874-10-cassel@kernel.org/

Re: DWC PCIe endpoint iATU vs BAR MASK ordering

[Frank Li]

On Tue, Nov 19, 2024 at 10:44:38AM +0100, Niklas Cassel wrote:
> Hello DWC PCIe endpoint developers,
>
>
> As I wrote in patch [1] (please review):
> The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."
>
> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
>
> While patch [1] is a nice fail-safe that we definitely want to have...
> I can see that the DWC PCIe EP driver is currently broken (and has been
> since the beginning).
>
> We are programming the iATU _before_ configuring the BAR:
> https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L232-L247
>
> The problem is of course that the iATU registers depend on the BAR mask
> (the number of read-only bits depends on the currently set BAR mask),
> so the iATU registers should be done _after_ configuring the BAR.
>
> Doing it in this was makes a lot of sense.
> First you configure the BAR, then you configure the address translation
> for that BAR. (Since the iATU in BAR match mode obviously depends on how
> the BAR is configured.)
>
>
> Now, I would like to send a patch to change this order, so that we actually
> write things in the only order that makes sense, my problem is this line:
> https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L236-L237
>
> This code makes no sense to me whatsoever.
>
> If I look at the commit that introduced this early return:
> 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
>
> It is not signed off by any of the regular PCI maintainers, neither does it
> have any Acked-by by any of the regular PCI maintainers, so I kind of wonder
> why this patch is there is the first place...
> (Taking something via a different tree is fine, but that would still require
> an Acked-by by one of the maintainers for the tree which owns that file.)
>
> While I am tempted to simply send a revert for this commit, so that we can
> write the registers in the correct order (iATU after BAR mask), I still do
> not want to regress the NTB EPF driver (even if the commit appears to have
> bypassed the regular PCI maintainers).
>
> Frank, since you are the author of:
> 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
>
> Please advice on what to do here. The only thing that makes sense to me is
> that dw_pcie_ep_set_bar() always writes the BAR MASK. If dw_pcie_ep_set_bar()
> does NOT write the BAR MASK, we depend on whatever BAR MASK was there before
> dw_pcie_ep_set_bar(), which no matter how I look at it, seems horribly wrong.

dw_pcie_ep_set_bar() don't change BAR MASK because host side also use it
to allocate resource when probe. But it just change iATU map address, for
example 1M BAR1, map to EP's side 0x1000_0000, it may change to another
address 0x2000_0000.  In doorbell MSI case, you tested, it dynamtic change
to ITS address, then change back when do normal bar test, which easiest
way to understand the behavior.

I think the order write iATU and mask doesn't matter because hardware
should just ignores some bits according to mask register.

If there are another APIs, which change inbound map address, should be
better.

Maybe off topic, I think if support combine some segmement address to
one bar should be wonderful, such as combine MSI ITS address and other
register to BAR0. It is not worth to waste one bar, which just for
doorbell.

Frank

>
>
> Kind regards,
> Niklas
>
> [1] https://lore.kernel.org/linux-pci/20241116163558.2606874-10-cassel@kernel.org/

Re: DWC PCIe endpoint iATU vs BAR MASK ordering

[Niklas Cassel]

Hello Frank,

On Wed, Nov 20, 2024 at 12:37:17PM -0500, Frank Li wrote:
> On Tue, Nov 19, 2024 at 10:44:38AM +0100, Niklas Cassel wrote:
> >
> > Frank, since you are the author of:
> > 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> >
> > Please advice on what to do here. The only thing that makes sense to me is
> > that dw_pcie_ep_set_bar() always writes the BAR MASK. If dw_pcie_ep_set_bar()
> > does NOT write the BAR MASK, we depend on whatever BAR MASK was there before
> > dw_pcie_ep_set_bar(), which no matter how I look at it, seems horribly wrong.
> 
> dw_pcie_ep_set_bar() don't change BAR MASK because host side also use it
> to allocate resource when probe. But it just change iATU map address, for
> example 1M BAR1, map to EP's side 0x1000_0000, it may change to another
> address 0x2000_0000.  In doorbell MSI case, you tested, it dynamtic change
> to ITS address, then change back when do normal bar test, which easiest
> way to understand the behavior.
> 
> I think the order write iATU and mask doesn't matter because hardware
> should just ignores some bits according to mask register.

Well, since LWR_TARGET_RW and LWR_TARGET_HW field sizes depends on the
value in BAR_MASK, of course the order matters.

BAR_MASK will always have some value, and if we write the iATU registers
before writing the BAR_MASK, then we will be at the mercy of the reset
value of the BAR_MASK register.

I sent a fix for this here:
https://lore.kernel.org/linux-pci/20241122115709.2949703-8-cassel@kernel.org/

I now understand why you did the early return.
I cleaned up the code, and added some comments explaining how the support
for dynamically changing the physical address of a BAR works.
It would be nice if you could test so that your vNTB use case still works.


Kind regards,
Niklas

Re: DWC PCIe endpoint iATU vs BAR MASK ordering

[Niklas Cassel]

On Wed, Nov 20, 2024 at 12:37:17PM -0500, Frank Li wrote:
> Maybe off topic, I think if support combine some segmement address to
> one bar should be wonderful, such as combine MSI ITS address and other
> register to BAR0. It is not worth to waste one bar, which just for
> doorbell.

I was thinking about this as well.

If we want a single BAR to redirect to two different physical memory areas,
then we can no longer use BAR Match Mode.

Sure, we could use two different iATUs and use Address Match Mode.
However, the problem is that the minimum size of an Address Translation
Region (CX_ATU_MIN_REGION_SIZE). E.g. on rk3588 CX_ATU_MIN_REGION_SIZE
is 64k...

So we would need to split the BAR in chunks of CX_ATU_MIN_REGION_SIZE.

If we look at e.g. the NVMe doorbells in BAR0, they are defined to
start at offset 0x1000 (4k).
See "Figure 4: PCI Express Specific Controller Property Definitions" in:
https://nvmexpress.org/wp-content/uploads/NVM-Express-PCI-Express-Transport-Specification-Revision-1.1-2024.08.05-Ratified.pdf

So, since that fixed offset is smaller than CX_ATU_MIN_REGION_SIZE,
we are out of luck... (at least for rk3588).

But.. perhaps the idea is still worth it?
For controllers that have CX_ATU_MIN_REGION_SIZE == 4k,
they could use one iATU for offset 0x-0xfff, and
another iATU for 0x1000-0x1fff.

(And of course, NVMe is just one specification out of many...)

I have no idea how CX_ATU_MIN_REGION_SIZE is set on NXP DWC PCIe
controllers.


Kind regards,
Niklas

Re: DWC PCIe endpoint iATU vs BAR MASK ordering

[Manivannan Sadhasivam]

On Tue, Nov 19, 2024 at 10:44:38AM +0100, Niklas Cassel wrote:
> Hello DWC PCIe endpoint developers,
> 
> 
> As I wrote in patch [1] (please review):
> The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields
> in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that:
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."
> 
> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
> 
> While patch [1] is a nice fail-safe that we definitely want to have...
> I can see that the DWC PCIe EP driver is currently broken (and has been
> since the beginning).
> 
> We are programming the iATU _before_ configuring the BAR:
> https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L232-L247
> 
> The problem is of course that the iATU registers depend on the BAR mask
> (the number of read-only bits depends on the currently set BAR mask),
> so the iATU registers should be done _after_ configuring the BAR.
> 
> Doing it in this was makes a lot of sense.
> First you configure the BAR, then you configure the address translation
> for that BAR. (Since the iATU in BAR match mode obviously depends on how
> the BAR is configured.)
> 
> 
> Now, I would like to send a patch to change this order, so that we actually
> write things in the only order that makes sense, my problem is this line:
> https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L236-L237
> 
> This code makes no sense to me whatsoever.
> 
> If I look at the commit that introduced this early return:
> 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> 
> It is not signed off by any of the regular PCI maintainers, neither does it
> have any Acked-by by any of the regular PCI maintainers, so I kind of wonder
> why this patch is there is the first place...
> (Taking something via a different tree is fine, but that would still require
> an Acked-by by one of the maintainers for the tree which owns that file.)
> 

It was one such occurence where the PCI EP maintainer didn't respond promptly
(not me) and the NTB maintainer merged the patch. I did complain about it:
https://lore.kernel.org/linux-pci/20220818060230.GA12008@thinkpad

- Mani

-- 
மணிவண்ணன் சதாசிவம்