Re: [PATCH 1/2] PCI: endpoint: strictly apply bar fixed size to allocate space

[Jerome Brunet <jbrunet@baylibre.com>]

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

-- 
Jerome