[PATCH 0/2] PCI: endpoint: space allocation fixups

[PATCH 0/2] PCI: endpoint: space allocation fixups

[Jerome Brunet]

This patchset fixes problems while trying to allocate space for PCI
endpoint function.

The problems, and related fixups, have been found while trying to link two
renesas rcar-gen4 r8a779f0-spider devices with the vNTB endpoint
function. This platform has 2 configurable BAR0 and BAR2, with an alignment
of 1MB, and fairly small fixed BAR4 of 256B.

This was tested with
 * BAR0 (1MB):  CTRL+SPAD
 * BAR2 (1MB):  MW0
 * BAR4 (256B): Doorbell

This setup is currently not supported by the vNTB EP driver and requires a
small hack. I'm working on that too.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
Jerome Brunet (2):
      PCI: endpoint: strictly apply bar fixed size to allocate space
      PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
 drivers/pci/endpoint/pci-epf-core.c           |  7 +++----
 2 files changed, 5 insertions(+), 26 deletions(-)
---
base-commit: dea140198b846f7432d78566b7b0b83979c72c2b
change-id: 20250328-pci-ep-size-alignment-9d85b28b8050

Best regards,
-- 
Jerome

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

[Jerome Brunet]

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(-)

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;

-- 
2.47.2

[PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

[Jerome Brunet]

When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
should not try to handle the size quirks for the underlying BAR, whether it
is fixed size or alignment. This is already handled by
pci_epf_alloc_space().

Also, when handling the alignment, this allocate more space than necessary.
For example, with a spad size of 1024B and a ctrl size of 308B, the space
necessary is 1332B. If the alignment is 1MB,
epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
been more than enough.

Just drop all the handling of the BAR size quirks and let
pci_epf_alloc_space() handle that.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
  */
 static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 {
-	size_t align;
 	enum pci_barno barno;
 	struct epf_ntb_ctrl *ctrl;
 	u32 spad_size, ctrl_size;
-	u64 size;
 	struct pci_epf *epf = ntb->epf;
 	struct device *dev = &epf->dev;
 	u32 spad_count;
@@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 								epf->func_no,
 								epf->vfunc_no);
 	barno = ntb->epf_ntb_bar[BAR_CONFIG];
-	size = epc_features->bar[barno].fixed_size;
-	align = epc_features->align;
-
-	if ((!IS_ALIGNED(size, align)))
-		return -EINVAL;
-
 	spad_count = ntb->spad_count;
 
 	ctrl_size = sizeof(struct epf_ntb_ctrl);
 	spad_size = 2 * spad_count * sizeof(u32);
 
-	if (!align) {
-		ctrl_size = roundup_pow_of_two(ctrl_size);
-		spad_size = roundup_pow_of_two(spad_size);
-	} else {
-		ctrl_size = ALIGN(ctrl_size, align);
-		spad_size = ALIGN(spad_size, align);
-	}
-
-	if (!size)
-		size = ctrl_size + spad_size;
-	else if (size < ctrl_size + spad_size)
-		return -EINVAL;
-
-	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
+	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
+				   barno, epc_features, 0);
 	if (!base) {
 		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
 		return -ENOMEM;

-- 
2.47.2

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

[Niklas Cassel]

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

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

[Jerome Brunet]

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

Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

[Frank Li]

On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote:
> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
> should not try to handle the size quirks for the underlying BAR, whether it
> is fixed size or alignment. This is already handled by
> pci_epf_alloc_space().
>
> Also, when handling the alignment, this allocate more space than necessary.
> For example, with a spad size of 1024B and a ctrl size of 308B, the space
> necessary is 1332B. If the alignment is 1MB,
> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
> been more than enough.
>
> Just drop all the handling of the BAR size quirks and let
> pci_epf_alloc_space() handle that.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
>   */
>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  {
> -	size_t align;
>  	enum pci_barno barno;
>  	struct epf_ntb_ctrl *ctrl;
>  	u32 spad_size, ctrl_size;
> -	u64 size;
>  	struct pci_epf *epf = ntb->epf;
>  	struct device *dev = &epf->dev;
>  	u32 spad_count;
> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  								epf->func_no,
>  								epf->vfunc_no);
>  	barno = ntb->epf_ntb_bar[BAR_CONFIG];
> -	size = epc_features->bar[barno].fixed_size;
> -	align = epc_features->align;
> -
> -	if ((!IS_ALIGNED(size, align)))
> -		return -EINVAL;
> -
>  	spad_count = ntb->spad_count;
>
>  	ctrl_size = sizeof(struct epf_ntb_ctrl);

I think keep ctrl_size at least align to 4 bytes. keep align 2^n is more
safe to keep spad area start at align possition.

	ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl));

Frank

>  	spad_size = 2 * spad_count * sizeof(u32);
>
> -	if (!align) {
> -		ctrl_size = roundup_pow_of_two(ctrl_size);
> -		spad_size = roundup_pow_of_two(spad_size);
> -	} else {
> -		ctrl_size = ALIGN(ctrl_size, align);
> -		spad_size = ALIGN(spad_size, align);
> -	}
> -
> -	if (!size)
> -		size = ctrl_size + spad_size;
> -	else if (size < ctrl_size + spad_size)
> -		return -EINVAL;
> -
> -	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
> +	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
> +				   barno, epc_features, 0);
>  	if (!base) {
>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
>  		return -ENOMEM;
>
> --
> 2.47.2
>

Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

[Jerome Brunet]

On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@nxp.com> wrote:

> On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote:
>> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
>> should not try to handle the size quirks for the underlying BAR, whether it
>> is fixed size or alignment. This is already handled by
>> pci_epf_alloc_space().
>>
>> Also, when handling the alignment, this allocate more space than necessary.
>> For example, with a spad size of 1024B and a ctrl size of 308B, the space
>> necessary is 1332B. If the alignment is 1MB,
>> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
>> been more than enough.
>>
>> Just drop all the handling of the BAR size quirks and let
>> pci_epf_alloc_space() handle that.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
>>  1 file changed, 2 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
>>   */
>>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>>  {
>> -	size_t align;
>>  	enum pci_barno barno;
>>  	struct epf_ntb_ctrl *ctrl;
>>  	u32 spad_size, ctrl_size;
>> -	u64 size;
>>  	struct pci_epf *epf = ntb->epf;
>>  	struct device *dev = &epf->dev;
>>  	u32 spad_count;
>> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>>  								epf->func_no,
>>  								epf->vfunc_no);
>>  	barno = ntb->epf_ntb_bar[BAR_CONFIG];
>> -	size = epc_features->bar[barno].fixed_size;
>> -	align = epc_features->align;
>> -
>> -	if ((!IS_ALIGNED(size, align)))
>> -		return -EINVAL;
>> -
>>  	spad_count = ntb->spad_count;
>>
>>  	ctrl_size = sizeof(struct epf_ntb_ctrl);
>
> I think keep ctrl_size at least align to 4 bytes.

Sure, makes sense

> keep align 2^n is more safe to keep spad area start at align
> possition.

That's something else. Both region are registers (or the emulation of
it) so a 32bits aligned is enough, AFAICT.

What purpose would 2^n aligned serve ? If it is safer, what's is the risk
exactly ?

>
> 	ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl));
>
> Frank
>
>>  	spad_size = 2 * spad_count * sizeof(u32);
>>
>> -	if (!align) {
>> -		ctrl_size = roundup_pow_of_two(ctrl_size);
>> -		spad_size = roundup_pow_of_two(spad_size);
>> -	} else {
>> -		ctrl_size = ALIGN(ctrl_size, align);
>> -		spad_size = ALIGN(spad_size, align);
>> -	}
>> -
>> -	if (!size)
>> -		size = ctrl_size + spad_size;
>> -	else if (size < ctrl_size + spad_size)
>> -		return -EINVAL;
>> -
>> -	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
>> +	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
>> +				   barno, epc_features, 0);
>>  	if (!base) {
>>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
>>  		return -ENOMEM;
>>
>> --
>> 2.47.2
>>

-- 
Jerome

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

[Niklas Cassel]

On Mon, Mar 31, 2025 at 04:39:33PM +0200, Jerome Brunet wrote:
> On Mon 31 Mar 2025 at 10:14, Niklas Cassel <cassel@kernel.org> wrote:
> >
> > 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.

You are right.


> 
> 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.

Your problem should be able to be reproducible also for BAR type
BAR_PROGRAMMABLE, when sending in a size smaller than epc_features.align
to pci_epf_alloc_space(), which will then modify epf_bar.size to be aligned.

The call to set_bar() will then use the aligned size instead of the
"BAR size" when writing the BAR mask register, which is wrong.
(This means that the BAR size seen by the host is the aligned size and not
"BAR size".)

So yes, I think having both size and aligned_size (or whatever name) is the
way to go.


Kind regards,
Niklas

Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

[Frank Li]

On Tue, Apr 01, 2025 at 09:39:10AM +0200, Jerome Brunet wrote:
> On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@nxp.com> wrote:
>
> > On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote:
> >> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
> >> should not try to handle the size quirks for the underlying BAR, whether it
> >> is fixed size or alignment. This is already handled by
> >> pci_epf_alloc_space().
> >>
> >> Also, when handling the alignment, this allocate more space than necessary.
> >> For example, with a spad size of 1024B and a ctrl size of 308B, the space
> >> necessary is 1332B. If the alignment is 1MB,
> >> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
> >> been more than enough.
> >>
> >> Just drop all the handling of the BAR size quirks and let
> >> pci_epf_alloc_space() handle that.
> >>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
> >>  1 file changed, 2 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
> >>   */
> >>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
> >>  {
> >> -	size_t align;
> >>  	enum pci_barno barno;
> >>  	struct epf_ntb_ctrl *ctrl;
> >>  	u32 spad_size, ctrl_size;
> >> -	u64 size;
> >>  	struct pci_epf *epf = ntb->epf;
> >>  	struct device *dev = &epf->dev;
> >>  	u32 spad_count;
> >> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
> >>  								epf->func_no,
> >>  								epf->vfunc_no);
> >>  	barno = ntb->epf_ntb_bar[BAR_CONFIG];
> >> -	size = epc_features->bar[barno].fixed_size;
> >> -	align = epc_features->align;
> >> -
> >> -	if ((!IS_ALIGNED(size, align)))
> >> -		return -EINVAL;
> >> -
> >>  	spad_count = ntb->spad_count;
> >>
> >>  	ctrl_size = sizeof(struct epf_ntb_ctrl);
> >
> > I think keep ctrl_size at least align to 4 bytes.
>
> Sure, makes sense
>
> > keep align 2^n is more safe to keep spad area start at align
> > possition.
>
> That's something else. Both region are registers (or the emulation of
> it) so a 32bits aligned is enough, AFAICT.
>
> What purpose would 2^n aligned serve ? If it is safer, what's is the risk
> exactly ?

After second think, it should be fine if 4 bytes align.

Frank

>
> >
> > 	ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl));
> >
> > Frank
> >
> >>  	spad_size = 2 * spad_count * sizeof(u32);
> >>
> >> -	if (!align) {
> >> -		ctrl_size = roundup_pow_of_two(ctrl_size);
> >> -		spad_size = roundup_pow_of_two(spad_size);
> >> -	} else {
> >> -		ctrl_size = ALIGN(ctrl_size, align);
> >> -		spad_size = ALIGN(spad_size, align);
> >> -	}
> >> -
> >> -	if (!size)
> >> -		size = ctrl_size + spad_size;
> >> -	else if (size < ctrl_size + spad_size)
> >> -		return -EINVAL;
> >> -
> >> -	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
> >> +	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
> >> +				   barno, epc_features, 0);
> >>  	if (!base) {
> >>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
> >>  		return -ENOMEM;
> >>
> >> --
> >> 2.47.2
> >>
>
> --
> Jerome

Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

[Jerome Brunet]

On Tue 01 Apr 2025 at 10:55, Frank Li <Frank.li@nxp.com> wrote:

> On Tue, Apr 01, 2025 at 09:39:10AM +0200, Jerome Brunet wrote:
>> On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@nxp.com> wrote:
>>
>> > On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote:
>> >> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
>> >> should not try to handle the size quirks for the underlying BAR, whether it
>> >> is fixed size or alignment. This is already handled by
>> >> pci_epf_alloc_space().
>> >>
>> >> Also, when handling the alignment, this allocate more space than necessary.
>> >> For example, with a spad size of 1024B and a ctrl size of 308B, the space
>> >> necessary is 1332B. If the alignment is 1MB,
>> >> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
>> >> been more than enough.
>> >>
>> >> Just drop all the handling of the BAR size quirks and let
>> >> pci_epf_alloc_space() handle that.
>> >>
>> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> >> ---
>> >>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
>> >>  1 file changed, 2 insertions(+), 22 deletions(-)
>> >>
>> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
>> >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
>> >>   */
>> >>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>> >>  {
>> >> -	size_t align;
>> >>  	enum pci_barno barno;
>> >>  	struct epf_ntb_ctrl *ctrl;
>> >>  	u32 spad_size, ctrl_size;
>> >> -	u64 size;
>> >>  	struct pci_epf *epf = ntb->epf;
>> >>  	struct device *dev = &epf->dev;
>> >>  	u32 spad_count;
>> >> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>> >>  								epf->func_no,
>> >>  								epf->vfunc_no);
>> >>  	barno = ntb->epf_ntb_bar[BAR_CONFIG];
>> >> -	size = epc_features->bar[barno].fixed_size;
>> >> -	align = epc_features->align;
>> >> -
>> >> -	if ((!IS_ALIGNED(size, align)))
>> >> -		return -EINVAL;
>> >> -
>> >>  	spad_count = ntb->spad_count;
>> >>
>> >>  	ctrl_size = sizeof(struct epf_ntb_ctrl);
>> >
>> > I think keep ctrl_size at least align to 4 bytes.
>>
>> Sure, makes sense
>>
>> > keep align 2^n is more safe to keep spad area start at align
>> > possition.
>>
>> That's something else. Both region are registers (or the emulation of
>> it) so a 32bits aligned is enough, AFAICT.
>>
>> What purpose would 2^n aligned serve ? If it is safer, what's is the risk
>> exactly ?
>
> After second think, it should be fine if 4 bytes align.
>
> Frank

Ok. Thanks for the feedback.

I think the same type of change should probably be applied to the NTB
endpoint function. It also tries to handle the alignment on its own, but
that's mixed up with msix doorbell things

I don't have the necessary HW to test that function so it would be a bit
risky for me to modify it, but it would be nice for the two endpoint
functions to be somehow aligned, especially since they share the same RC
side driver.

If anyone is able to help on this, that would be greatly appreciated :)

>
>>
>> >
>> > 	ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl));
>> >
>> > Frank
>> >
>> >>  	spad_size = 2 * spad_count * sizeof(u32);
>> >>
>> >> -	if (!align) {
>> >> -		ctrl_size = roundup_pow_of_two(ctrl_size);
>> >> -		spad_size = roundup_pow_of_two(spad_size);
>> >> -	} else {
>> >> -		ctrl_size = ALIGN(ctrl_size, align);
>> >> -		spad_size = ALIGN(spad_size, align);
>> >> -	}
>> >> -
>> >> -	if (!size)
>> >> -		size = ctrl_size + spad_size;
>> >> -	else if (size < ctrl_size + spad_size)
>> >> -		return -EINVAL;
>> >> -
>> >> -	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
>> >> +	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
>> >> +				   barno, epc_features, 0);
>> >>  	if (!base) {
>> >>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
>> >>  		return -ENOMEM;
>> >>
>> >> --
>> >> 2.47.2
>> >>
>>
>> --
>> Jerome

-- 
Jerome