| Message ID | 20250328-pci-ep-size-alignment-v1-1-ee5b78b15a9a@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: endpoint: space allocation fixups | expand |
Hello Jerome, On Fri, Mar 28, 2025 at 03:53:42PM +0100, Jerome Brunet wrote: > When trying to allocate space for an endpoint function on a BAR with a > fixed size, that size should be used regardless of the alignment. Why? > > Some controller may have specified an alignment, but do have a BAR with a > fixed size smaller that alignment. In such case, pci_epf_alloc_space() > tries to allocate a space that matches the alignment and it won't work. Could you please elaborate "won't work". > > When the BAR size is fixed, pci_epf_alloc_space() should not deviate > from this fixed size. I think that this commit is wrong. In your specific SoC: .msix_capable = false, .bar[BAR_1] = { .type = BAR_RESERVED, }, .bar[BAR_3] = { .type = BAR_RESERVED, }, .bar[BAR_4] = { .type = BAR_FIXED, .fixed_size = 256 }, .bar[BAR_5] = { .type = BAR_RESERVED, }, .align = SZ_1M, fixed_size is 256B, inbound iATU alignment is 1 MB, which means that the smallest area that the iATU can map is 1 MB. I do think that it makes sense to have backing memory for the whole area that the iATU will have mapped. The reason why the the ALIGN() is done, is so that the size sent in to dma_alloc_coherent() will return addresses that are aligned to the inbound iATU alignment requirement. I guess the problem is that your driver has a fixed size BAR that is smaller than the inbound iATU alignment requirement, something that has never been a problem before, because no SoC has previously defined such a fixed size BAR. I doubt the problem is allocating such a BAR, so where is it you actually encounter a problem? My guess is in .set_bar(). Perhaps the solution is to add another struct member to struct pci_epf_bar, size (meaning actual BAR size, which will be written to the BAR mask register) and backing_mem_size. Or.. we modify pci_epf_alloc_space() to allocate an aligned size, but the size that we store in (struct pci_epf_bar).size is the unaligned size. Kind regards, Niklas
On Mon 31 Mar 2025 at 10:14, Niklas Cassel <cassel@kernel.org> wrote: > Hello Jerome, > > On Fri, Mar 28, 2025 at 03:53:42PM +0100, Jerome Brunet wrote: >> When trying to allocate space for an endpoint function on a BAR with a >> fixed size, that size should be used regardless of the alignment. > > Why? > > >> >> Some controller may have specified an alignment, but do have a BAR with a >> fixed size smaller that alignment. In such case, pci_epf_alloc_space() >> tries to allocate a space that matches the alignment and it won't work. > > Could you please elaborate "won't work". > As I explained in the cover letter, I'm trying to enable vNTB on the renesas platform. It started off with different Oopses, apparently accessing unmapped area, so I started digging in the code for anything that looked fishy. There was several problems leading to this but it ended with errors in pci_epc_set_bar() as you are pointing out bellow. > >> >> When the BAR size is fixed, pci_epf_alloc_space() should not deviate >> from this fixed size. > > I think that this commit is wrong. > > In your specific SoC: > .msix_capable = false, > .bar[BAR_1] = { .type = BAR_RESERVED, }, > .bar[BAR_3] = { .type = BAR_RESERVED, }, > .bar[BAR_4] = { .type = BAR_FIXED, .fixed_size = 256 }, > .bar[BAR_5] = { .type = BAR_RESERVED, }, > .align = SZ_1M, > > fixed_size is 256B, inbound iATU alignment is 1 MB, which means that the > smallest area that the iATU can map is 1 MB. > > I do think that it makes sense to have backing memory for the whole area > that the iATU will have mapped. > > The reason why the the ALIGN() is done, is so that the size sent in to > dma_alloc_coherent() will return addresses that are aligned to the inbound > iATU alignment requirement. > Makes sense and thanks a lot for the detailed explanation. Much appreciated. > > I guess the problem is that your driver has a fixed size BAR that is smaller > than the inbound iATU alignment requirement, something that has never been a > problem before, because no SoC has previously defined such a fixed size BAR. > There is always a first I guess ;) > I doubt the problem is allocating such a BAR, so where is it you actually > encounter a problem? My guess is in .set_bar(). pci_epc_set_bar() indeed. It seems the underlying dwc-ep driver does not care too much what it is given for a fixed bar: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c#n409 > > Perhaps the solution is to add another struct member to struct pci_epf_bar, > size (meaning actual BAR size, which will be written to the BAR mask register) > and backing_mem_size. > > Or.. we modify pci_epf_alloc_space() to allocate an aligned size, but the > size that we store in (struct pci_epf_bar).size is the unaligned size. I tried this and it works. As pointed above, as long as pci_epc_set_bar() is happy, it will work for me since the dwc-ep driver does not really care for the size given with fixed BARs. However, when doing so, it gets a bit trick to properly call dma_free_coherent() as we don't have the size actually allocated anymore. It is possible to compute it again but it is rather ugly. It would probably be best to add a parameter indeed, to track the size allocated with dma_alloc_coherent(). What about .aligned_size ? Keeping .size to track the actual bar size requires less modification I think. > > > Kind regards, > Niklas
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c index 394395c7f8decfa2010469655a4bd58a002993fd..cb985b172ed041c6f319c083f412e51e25b0a157 100644 --- a/drivers/pci/endpoint/pci-epf-core.c +++ b/drivers/pci/endpoint/pci-epf-core.c @@ -285,12 +285,11 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar, return NULL; } size = bar_fixed_size; - } - - if (align) + } else if (align) { size = ALIGN(size, align); - else + } else { size = roundup_pow_of_two(size); + } if (type == PRIMARY_INTERFACE) { epc = epf->epc;
When trying to allocate space for an endpoint function on a BAR with a fixed size, that size should be used regardless of the alignment. Some controller may have specified an alignment, but do have a BAR with a fixed size smaller that alignment. In such case, pci_epf_alloc_space() tries to allocate a space that matches the alignment and it won't work. When the BAR size is fixed, pci_epf_alloc_space() should not deviate from this fixed size. Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs") Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/pci/endpoint/pci-epf-core.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)