[PATCH 0/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR mapping

[PATCH 0/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR mapping

[Jerome Brunet]

The patchset allows arbitrary BAR mapping for vNTB PCI endpoint function.

This was developed for the Renesas platform with requires a mapping that
was not possible before:
* BAR0 (1MB):  CTRL+SPAD
* BAR2 (1MB):  MW0
* BAR4 (256B): Doorbell

It is possible to setup the host side driver with mapping above without any
functional change but it makes to also arbitrary mapping there.

The patchset should not change anything for existing users.

Possible next steps:
- Align the NTB endpoint function: I'd be happy to propose something there
  but I would only be able to compile test it since I do not have the HW
  to test it.
- Expose BAR configuration in the CTRL registers: I've been doodling with
  the idea to add a few extra registers in the CTRL region to describe
  the BAR mapping of the other regions. That way, there would less chance
  for the 2 sides to become mis-aligned. I'm not certain it makes sense and
  would welcome others opinion on this :)

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
Jerome Brunet (4):
      PCI: endpoint: pci-epf-vntb: return an error code on bar init
      PCI: endpoint: pci-epf-vntb: align mw naming with config names
      PCI: endpoint: pci-epf-vntb: allow arbitrary BAR configuration
      NTB: epf: Allow arbitrary BAR mapping

 drivers/ntb/hw/epf/ntb_hw_epf.c               | 108 ++++++++++----------
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 138 +++++++++++++++++++++++---
 2 files changed, 181 insertions(+), 65 deletions(-)
---
base-commit: 1fa7eab22df331bcc7942a9c0b4569bebc11593b
change-id: 20250505-pci-vntb-bar-mapping-3cc3ff624e76

Best regards,
-- 
Jerome

[PATCH 1/4] PCI: endpoint: pci-epf-vntb: return an error code on bar init

[Jerome Brunet]

According the function documentation of epf_ntb_init_epc_bar(),
the function should return an error code on error. However, it
returns -1 when no BAR is available.

Return -EINVAL instead.

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index e4da3fdb000723e3adad01f0ddf230ecc0e572a7..35fa0a21fc91100a5539bff775e7ebc25e1fb9c1 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -680,7 +680,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 		barno = pci_epc_get_next_free_bar(epc_features, barno);
 		if (barno < 0) {
 			dev_err(dev, "Fail to get NTB function BAR\n");
-			return barno;
+			return -EINVAL;
 		}
 		ntb->epf_ntb_bar[bar] = barno;
 	}

-- 
2.47.2

Re: [PATCH 1/4] PCI: endpoint: pci-epf-vntb: return an error code on bar init

[Frank Li]

On Mon, May 05, 2025 at 07:41:47PM +0200, Jerome Brunet wrote:

subject: s/return/Return

> According the function documentation of epf_ntb_init_epc_bar(),
> the function should return an error code on error. However, it
> returns -1 when no BAR is available.
>
> Return -EINVAL instead.
>
> Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

After fix above small issue about subject.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index e4da3fdb000723e3adad01f0ddf230ecc0e572a7..35fa0a21fc91100a5539bff775e7ebc25e1fb9c1 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -680,7 +680,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>  		barno = pci_epc_get_next_free_bar(epc_features, barno);
>  		if (barno < 0) {
>  			dev_err(dev, "Fail to get NTB function BAR\n");
> -			return barno;
> +			return -EINVAL;
>  		}
>  		ntb->epf_ntb_bar[bar] = barno;
>  	}
>
> --
> 2.47.2
>

[PATCH 2/4] PCI: endpoint: pci-epf-vntb: align mw naming with config names

[Jerome Brunet]

The config file related to the memory windows start the numbering of
the MW from 1. The other NTB function does the same, yet the enumeration
defining the BARs of the vNTB function starts numbering the MW from 0.

Both numbering are fine I suppose but mixing the two is a bit confusing.
The configfs file being the interface with userspace, lets keep that stable
and consistently start the numbering of the MW from 1.

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 35fa0a21fc91100a5539bff775e7ebc25e1fb9c1..f9f4a8bb65f364962dbf1e9011ab0e4479c61034 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -70,9 +70,10 @@ static struct workqueue_struct *kpcintb_workqueue;
 enum epf_ntb_bar {
 	BAR_CONFIG,
 	BAR_DB,
-	BAR_MW0,
 	BAR_MW1,
 	BAR_MW2,
+	BAR_MW3,
+	BAR_MW4,
 };
 
 /*
@@ -576,7 +577,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 
 	for (i = 0; i < ntb->num_mws; i++) {
 		size = ntb->mws_size[i];
-		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
+		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
 
 		ntb->epf->bar[barno].barno = barno;
 		ntb->epf->bar[barno].size = size;
@@ -629,7 +630,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
 	int i;
 
 	for (i = 0; i < num_mws; i++) {
-		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
+		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
 		pci_epc_clear_bar(ntb->epf->epc,
 				  ntb->epf->func_no,
 				  ntb->epf->vfunc_no,
@@ -676,7 +677,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
 
 	/* These are required BARs which are mandatory for NTB functionality */
-	for (bar = BAR_CONFIG; bar <= BAR_MW0; bar++, barno++) {
+	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
 		barno = pci_epc_get_next_free_bar(epc_features, barno);
 		if (barno < 0) {
 			dev_err(dev, "Fail to get NTB function BAR\n");
@@ -1048,7 +1049,7 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
 	struct device *dev;
 
 	dev = &ntb->ntb.dev;
-	barno = ntb->epf_ntb_bar[BAR_MW0 + idx];
+	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
 	epf_bar = &ntb->epf->bar[barno];
 	epf_bar->phys_addr = addr;
 	epf_bar->barno = barno;

-- 
2.47.2

Re: [PATCH 2/4] PCI: endpoint: pci-epf-vntb: align mw naming with config names

[Frank Li]

On Mon, May 05, 2025 at 07:41:48PM +0200, Jerome Brunet wrote:

PCI tree require keep consistent at subject
git log --oneline drivers/pci/endpoint/functions/pci-epf-vntb.c

require first char is UP case.

Align memory window naming with configfs names.

> The config file related to the memory windows start the numbering of

                                 memory windows (MW)
 then you can use MW later.

> the MW from 1. The other NTB function does the same, yet the enumeration
> defining the BARs of the vNTB function starts numbering the MW from 0.
>
> Both numbering are fine I suppose but mixing the two is a bit confusing.
> The configfs file being the interface with userspace, lets keep that stable
> and consistently start the numbering of the MW from 1.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 35fa0a21fc91100a5539bff775e7ebc25e1fb9c1..f9f4a8bb65f364962dbf1e9011ab0e4479c61034 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -70,9 +70,10 @@ static struct workqueue_struct *kpcintb_workqueue;
>  enum epf_ntb_bar {
>  	BAR_CONFIG,
>  	BAR_DB,
> -	BAR_MW0,
>  	BAR_MW1,
>  	BAR_MW2,
> +	BAR_MW3,
> +	BAR_MW4,

where use BAR_MW3 and BAR_MW4?

Frank
>  };
>
>  /*
> @@ -576,7 +577,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
>
>  	for (i = 0; i < ntb->num_mws; i++) {
>  		size = ntb->mws_size[i];
> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
>
>  		ntb->epf->bar[barno].barno = barno;
>  		ntb->epf->bar[barno].size = size;
> @@ -629,7 +630,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
>  	int i;
>
>  	for (i = 0; i < num_mws; i++) {
> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
>  		pci_epc_clear_bar(ntb->epf->epc,
>  				  ntb->epf->func_no,
>  				  ntb->epf->vfunc_no,
> @@ -676,7 +677,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
>
>  	/* These are required BARs which are mandatory for NTB functionality */
> -	for (bar = BAR_CONFIG; bar <= BAR_MW0; bar++, barno++) {
> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
>  		barno = pci_epc_get_next_free_bar(epc_features, barno);
>  		if (barno < 0) {
>  			dev_err(dev, "Fail to get NTB function BAR\n");
> @@ -1048,7 +1049,7 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
>  	struct device *dev;
>
>  	dev = &ntb->ntb.dev;
> -	barno = ntb->epf_ntb_bar[BAR_MW0 + idx];
> +	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>  	epf_bar = &ntb->epf->bar[barno];
>  	epf_bar->phys_addr = addr;
>  	epf_bar->barno = barno;
>
> --
> 2.47.2
>

Re: [PATCH 2/4] PCI: endpoint: pci-epf-vntb: align mw naming with config names

[Jerome Brunet]

On Mon 19 May 2025 at 17:21, Frank Li <Frank.li@nxp.com> wrote:

> On Mon, May 05, 2025 at 07:41:48PM +0200, Jerome Brunet wrote:
>
> PCI tree require keep consistent at subject
> git log --oneline drivers/pci/endpoint/functions/pci-epf-vntb.c
>
> require first char is UP case.

Noted

>
> Align memory window naming with configfs names.
>
>> The config file related to the memory windows start the numbering of
>
>                                  memory windows (MW)
>  then you can use MW later.

Sure

>
>> the MW from 1. The other NTB function does the same, yet the enumeration
>> defining the BARs of the vNTB function starts numbering the MW from 0.
>>
>> Both numbering are fine I suppose but mixing the two is a bit confusing.
>> The configfs file being the interface with userspace, lets keep that stable
>> and consistently start the numbering of the MW from 1.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> index 35fa0a21fc91100a5539bff775e7ebc25e1fb9c1..f9f4a8bb65f364962dbf1e9011ab0e4479c61034 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> @@ -70,9 +70,10 @@ static struct workqueue_struct *kpcintb_workqueue;
>>  enum epf_ntb_bar {
>>  	BAR_CONFIG,
>>  	BAR_DB,
>> -	BAR_MW0,
>>  	BAR_MW1,
>>  	BAR_MW2,
>> +	BAR_MW3,
>> +	BAR_MW4,
>
> where use BAR_MW3 and BAR_MW4?

This is aligned with the file available in configfs and what is possible
in theory with the function, same as the NTB function and NTB host driver.

Stopping at MW1 because it is only one used in the driver would be weird
and the number later introduced would be wrong.


>
> Frank
>>  };
>>
>>  /*
>> @@ -576,7 +577,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
>>
>>  	for (i = 0; i < ntb->num_mws; i++) {
>>  		size = ntb->mws_size[i];
>> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
>> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
>>
>>  		ntb->epf->bar[barno].barno = barno;
>>  		ntb->epf->bar[barno].size = size;
>> @@ -629,7 +630,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
>>  	int i;
>>
>>  	for (i = 0; i < num_mws; i++) {
>> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
>> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
>>  		pci_epc_clear_bar(ntb->epf->epc,
>>  				  ntb->epf->func_no,
>>  				  ntb->epf->vfunc_no,
>> @@ -676,7 +677,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
>>
>>  	/* These are required BARs which are mandatory for NTB functionality */
>> -	for (bar = BAR_CONFIG; bar <= BAR_MW0; bar++, barno++) {
>> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
>>  		barno = pci_epc_get_next_free_bar(epc_features, barno);
>>  		if (barno < 0) {
>>  			dev_err(dev, "Fail to get NTB function BAR\n");
>> @@ -1048,7 +1049,7 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
>>  	struct device *dev;
>>
>>  	dev = &ntb->ntb.dev;
>> -	barno = ntb->epf_ntb_bar[BAR_MW0 + idx];
>> +	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>>  	epf_bar = &ntb->epf->bar[barno];
>>  	epf_bar->phys_addr = addr;
>>  	epf_bar->barno = barno;
>>
>> --
>> 2.47.2
>>

-- 
Jerome

Re: [PATCH 2/4] PCI: endpoint: pci-epf-vntb: align mw naming with config names

[Frank Li]

On Tue, May 20, 2025 at 09:47:14AM +0200, Jerome Brunet wrote:
> On Mon 19 May 2025 at 17:21, Frank Li <Frank.li@nxp.com> wrote:
>
> > On Mon, May 05, 2025 at 07:41:48PM +0200, Jerome Brunet wrote:
> >
> > PCI tree require keep consistent at subject
> > git log --oneline drivers/pci/endpoint/functions/pci-epf-vntb.c
> >
> > require first char is UP case.
>
> Noted
>
> >
> > Align memory window naming with configfs names.
> >
> >> The config file related to the memory windows start the numbering of
> >
> >                                  memory windows (MW)
> >  then you can use MW later.
>
> Sure
>
> >
> >> the MW from 1. The other NTB function does the same, yet the enumeration
> >> defining the BARs of the vNTB function starts numbering the MW from 0.
> >>
> >> Both numbering are fine I suppose but mixing the two is a bit confusing.
> >> The configfs file being the interface with userspace, lets keep that stable
> >> and consistently start the numbering of the MW from 1.
> >>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
> >>  1 file changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> index 35fa0a21fc91100a5539bff775e7ebc25e1fb9c1..f9f4a8bb65f364962dbf1e9011ab0e4479c61034 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> @@ -70,9 +70,10 @@ static struct workqueue_struct *kpcintb_workqueue;
> >>  enum epf_ntb_bar {
> >>  	BAR_CONFIG,
> >>  	BAR_DB,
> >> -	BAR_MW0,
> >>  	BAR_MW1,
> >>  	BAR_MW2,
> >> +	BAR_MW3,
> >> +	BAR_MW4,
> >
> > where use BAR_MW3 and BAR_MW4?
>
> This is aligned with the file available in configfs and what is possible
> in theory with the function, same as the NTB function and NTB host driver.
>
> Stopping at MW1 because it is only one used in the driver would be weird
> and the number later introduced would be wrong.

Yes, but BAR_MW3 and BAR_MW4 should be added only when both was used in code
actaully.

Frank
>
>
> >
> > Frank
> >>  };
> >>
> >>  /*
> >> @@ -576,7 +577,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
> >>
> >>  	for (i = 0; i < ntb->num_mws; i++) {
> >>  		size = ntb->mws_size[i];
> >> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
> >> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
> >>
> >>  		ntb->epf->bar[barno].barno = barno;
> >>  		ntb->epf->bar[barno].size = size;
> >> @@ -629,7 +630,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
> >>  	int i;
> >>
> >>  	for (i = 0; i < num_mws; i++) {
> >> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
> >> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
> >>  		pci_epc_clear_bar(ntb->epf->epc,
> >>  				  ntb->epf->func_no,
> >>  				  ntb->epf->vfunc_no,
> >> @@ -676,7 +677,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
> >>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
> >>
> >>  	/* These are required BARs which are mandatory for NTB functionality */
> >> -	for (bar = BAR_CONFIG; bar <= BAR_MW0; bar++, barno++) {
> >> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
> >>  		barno = pci_epc_get_next_free_bar(epc_features, barno);
> >>  		if (barno < 0) {
> >>  			dev_err(dev, "Fail to get NTB function BAR\n");
> >> @@ -1048,7 +1049,7 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
> >>  	struct device *dev;
> >>
> >>  	dev = &ntb->ntb.dev;
> >> -	barno = ntb->epf_ntb_bar[BAR_MW0 + idx];
> >> +	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
> >>  	epf_bar = &ntb->epf->bar[barno];
> >>  	epf_bar->phys_addr = addr;
> >>  	epf_bar->barno = barno;
> >>
> >> --
> >> 2.47.2
> >>
>
> --
> Jerome

Re: [PATCH 2/4] PCI: endpoint: pci-epf-vntb: align mw naming with config names

[Jerome Brunet]

On Wed 21 May 2025 at 12:28, Frank Li <Frank.li@nxp.com> wrote:

> On Tue, May 20, 2025 at 09:47:14AM +0200, Jerome Brunet wrote:
>> On Mon 19 May 2025 at 17:21, Frank Li <Frank.li@nxp.com> wrote:
>>
>> > On Mon, May 05, 2025 at 07:41:48PM +0200, Jerome Brunet wrote:
>> >
>> > PCI tree require keep consistent at subject
>> > git log --oneline drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >
>> > require first char is UP case.
>>
>> Noted
>>
>> >
>> > Align memory window naming with configfs names.
>> >
>> >> The config file related to the memory windows start the numbering of
>> >
>> >                                  memory windows (MW)
>> >  then you can use MW later.
>>
>> Sure
>>
>> >
>> >> the MW from 1. The other NTB function does the same, yet the enumeration
>> >> defining the BARs of the vNTB function starts numbering the MW from 0.
>> >>
>> >> Both numbering are fine I suppose but mixing the two is a bit confusing.
>> >> The configfs file being the interface with userspace, lets keep that stable
>> >> and consistently start the numbering of the MW from 1.
>> >>
>> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> >> ---
>> >>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 11 ++++++-----
>> >>  1 file changed, 6 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >> index 35fa0a21fc91100a5539bff775e7ebc25e1fb9c1..f9f4a8bb65f364962dbf1e9011ab0e4479c61034 100644
>> >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> >> @@ -70,9 +70,10 @@ static struct workqueue_struct *kpcintb_workqueue;
>> >>  enum epf_ntb_bar {
>> >>  	BAR_CONFIG,
>> >>  	BAR_DB,
>> >> -	BAR_MW0,
>> >>  	BAR_MW1,
>> >>  	BAR_MW2,
>> >> +	BAR_MW3,
>> >> +	BAR_MW4,
>> >
>> > where use BAR_MW3 and BAR_MW4?
>>
>> This is aligned with the file available in configfs and what is possible
>> in theory with the function, same as the NTB function and NTB host driver.
>>
>> Stopping at MW1 because it is only one used in the driver would be weird
>> and the number later introduced would be wrong.
>
> Yes, but BAR_MW3 and BAR_MW4 should be added only when both was used in code
> actaully.

If you want to be strict about that then BAR_MW4 should go to the next change,
it is needed in the enum.

BAR_MW3 shall stay here because the purpose of this change is to shift the MW
naming by one. MW2 which was present becomes MW3.

>
> Frank
>>
>>
>> >
>> > Frank
>> >>  };
>> >>
>> >>  /*
>> >> @@ -576,7 +577,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
>> >>
>> >>  	for (i = 0; i < ntb->num_mws; i++) {
>> >>  		size = ntb->mws_size[i];
>> >> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
>> >> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
>> >>
>> >>  		ntb->epf->bar[barno].barno = barno;
>> >>  		ntb->epf->bar[barno].size = size;
>> >> @@ -629,7 +630,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
>> >>  	int i;
>> >>
>> >>  	for (i = 0; i < num_mws; i++) {
>> >> -		barno = ntb->epf_ntb_bar[BAR_MW0 + i];
>> >> +		barno = ntb->epf_ntb_bar[BAR_MW1 + i];
>> >>  		pci_epc_clear_bar(ntb->epf->epc,
>> >>  				  ntb->epf->func_no,
>> >>  				  ntb->epf->vfunc_no,
>> >> @@ -676,7 +677,7 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>> >>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
>> >>
>> >>  	/* These are required BARs which are mandatory for NTB functionality */
>> >> -	for (bar = BAR_CONFIG; bar <= BAR_MW0; bar++, barno++) {
>> >> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
>> >>  		barno = pci_epc_get_next_free_bar(epc_features, barno);
>> >>  		if (barno < 0) {
>> >>  			dev_err(dev, "Fail to get NTB function BAR\n");
>> >> @@ -1048,7 +1049,7 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
>> >>  	struct device *dev;
>> >>
>> >>  	dev = &ntb->ntb.dev;
>> >> -	barno = ntb->epf_ntb_bar[BAR_MW0 + idx];
>> >> +	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>> >>  	epf_bar = &ntb->epf->bar[barno];
>> >>  	epf_bar->phys_addr = addr;
>> >>  	epf_bar->barno = barno;
>> >>
>> >> --
>> >> 2.47.2
>> >>
>>
>> --
>> Jerome

-- 
Jerome

[PATCH 3/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR configuration

[Jerome Brunet]

The BAR configuration used by the PCI vNTB endpoint function is rather
fixed and does not allow to account for platform quirks. It simply
allocate BAR in order.

This is a problem on the Renesas platforms which have a 256B fixed BAR_4
which end-up being the MW1 BAR. While this BAR is not ideal for a MW, it
is adequate for the doorbells.

Add more configfs attributes to allow arbitrary BAR configuration to be
provided through the driver configfs. If not configuration is provided,
the driver should retain the old behaviour and allocate BARs in order.
This should keep existing userspace scripts working.

In the Renesas case mentioned above, the change allows to use BAR_2 as for
the MW1 region and BAR_4 for the doorbells.

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index f9f4a8bb65f364962dbf1e9011ab0e4479c61034..3cdccfe870e0cf738c93ca7c525fa2daa7c87fcb 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -74,6 +74,7 @@ enum epf_ntb_bar {
 	BAR_MW2,
 	BAR_MW3,
 	BAR_MW4,
+	VNTB_BAR_NUM,
 };
 
 /*
@@ -133,7 +134,7 @@ struct epf_ntb {
 	bool linkup;
 	u32 spad_size;
 
-	enum pci_barno epf_ntb_bar[6];
+	enum pci_barno epf_ntb_bar[VNTB_BAR_NUM];
 
 	struct epf_ntb_ctrl *reg;
 
@@ -655,6 +656,59 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
 	pci_epc_put(ntb->epf->epc);
 }
 
+
+/**
+ * epf_ntb_is_bar_used() - Check if a bar is used in the ntb configuration
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: 0 if unused, 1 if used.
+ */
+static int epf_ntb_is_bar_used(struct epf_ntb *ntb,
+			   enum pci_barno barno)
+{
+	int i;
+
+	for (i = 0; i < VNTB_BAR_NUM; i++) {
+		if (ntb->epf_ntb_bar[i] == barno)
+			return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * epf_ntb_set_bar() - Assign BAR number when no configuration is provided
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * When the BAR configuration was not provided through the userspace
+ * configuration, automatically assign BAR as it has been historically
+ * done by this endpoint function.
+ *
+ * Returns: the BAR number found, if any. -1 otherwise
+ */
+static int epf_ntb_set_bar(struct epf_ntb *ntb,
+			   const struct pci_epc_features *epc_features,
+			   enum epf_ntb_bar bar,
+			   enum pci_barno barno)
+{
+	while (ntb->epf_ntb_bar[bar] < 0) {
+		barno = pci_epc_get_next_free_bar(epc_features, barno);
+		if (barno < 0)
+			break; /* No more BAR available */
+
+		/*
+		 * Verify if the BAR found is not already assigned
+		 * through the provided configuration
+		 */
+		if (!epf_ntb_is_bar_used(ntb, barno))
+			ntb->epf_ntb_bar[bar] = barno;
+
+		barno += 1;
+	}
+
+	return barno;
+}
+
 /**
  * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
  * constructs (scratchpad region, doorbell, memorywindow)
@@ -677,23 +731,21 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
 
 	/* These are required BARs which are mandatory for NTB functionality */
-	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
-		barno = pci_epc_get_next_free_bar(epc_features, barno);
+	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++) {
+		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
 		if (barno < 0) {
 			dev_err(dev, "Fail to get NTB function BAR\n");
 			return -EINVAL;
 		}
-		ntb->epf_ntb_bar[bar] = barno;
 	}
 
 	/* These are optional BARs which don't impact NTB functionality */
-	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, barno++, i++) {
-		barno = pci_epc_get_next_free_bar(epc_features, barno);
+	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, i++) {
+		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
 		if (barno < 0) {
 			ntb->num_mws = i;
 			dev_dbg(dev, "BAR not available for > MW%d\n", i + 1);
 		}
-		ntb->epf_ntb_bar[bar] = barno;
 	}
 
 	return 0;
@@ -861,6 +913,37 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
 	return len;							\
 }
 
+#define EPF_NTB_BAR_R(_name, _id)					\
+	static ssize_t epf_ntb_##_name##_show(struct config_item *item,	\
+					      char *page)		\
+	{								\
+		struct config_group *group = to_config_group(item);	\
+		struct epf_ntb *ntb = to_epf_ntb(group);		\
+									\
+		return sprintf(page, "%d\n", ntb->epf_ntb_bar[_id]);	\
+	}
+
+#define EPF_NTB_BAR_W(_name, _id)					\
+	static ssize_t epf_ntb_##_name##_store(struct config_item *item, \
+					       const char *page, size_t len) \
+	{								\
+	struct config_group *group = to_config_group(item);		\
+	struct epf_ntb *ntb = to_epf_ntb(group);			\
+	int val;							\
+	int ret;							\
+									\
+	ret = kstrtoint(page, 0, &val);					\
+	if (ret)							\
+		return ret;						\
+									\
+	if (val < NO_BAR || val > BAR_5)				\
+		return -EINVAL;						\
+									\
+	ntb->epf_ntb_bar[_id] = val;					\
+									\
+	return len;							\
+	}
+
 static ssize_t epf_ntb_num_mws_store(struct config_item *item,
 				     const char *page, size_t len)
 {
@@ -900,6 +983,18 @@ EPF_NTB_MW_R(mw3)
 EPF_NTB_MW_W(mw3)
 EPF_NTB_MW_R(mw4)
 EPF_NTB_MW_W(mw4)
+EPF_NTB_BAR_R(ctrl_bar, BAR_CONFIG)
+EPF_NTB_BAR_W(ctrl_bar, BAR_CONFIG)
+EPF_NTB_BAR_R(db_bar, BAR_DB)
+EPF_NTB_BAR_W(db_bar, BAR_DB)
+EPF_NTB_BAR_R(mw1_bar, BAR_MW1)
+EPF_NTB_BAR_W(mw1_bar, BAR_MW1)
+EPF_NTB_BAR_R(mw2_bar, BAR_MW1)
+EPF_NTB_BAR_W(mw2_bar, BAR_MW1)
+EPF_NTB_BAR_R(mw3_bar, BAR_MW3)
+EPF_NTB_BAR_W(mw3_bar, BAR_MW3)
+EPF_NTB_BAR_R(mw4_bar, BAR_MW4)
+EPF_NTB_BAR_W(mw4_bar, BAR_MW4)
 
 CONFIGFS_ATTR(epf_ntb_, spad_count);
 CONFIGFS_ATTR(epf_ntb_, db_count);
@@ -911,6 +1006,12 @@ CONFIGFS_ATTR(epf_ntb_, mw4);
 CONFIGFS_ATTR(epf_ntb_, vbus_number);
 CONFIGFS_ATTR(epf_ntb_, vntb_pid);
 CONFIGFS_ATTR(epf_ntb_, vntb_vid);
+CONFIGFS_ATTR(epf_ntb_, ctrl_bar);
+CONFIGFS_ATTR(epf_ntb_, db_bar);
+CONFIGFS_ATTR(epf_ntb_, mw1_bar);
+CONFIGFS_ATTR(epf_ntb_, mw2_bar);
+CONFIGFS_ATTR(epf_ntb_, mw3_bar);
+CONFIGFS_ATTR(epf_ntb_, mw4_bar);
 
 static struct configfs_attribute *epf_ntb_attrs[] = {
 	&epf_ntb_attr_spad_count,
@@ -923,6 +1024,12 @@ static struct configfs_attribute *epf_ntb_attrs[] = {
 	&epf_ntb_attr_vbus_number,
 	&epf_ntb_attr_vntb_pid,
 	&epf_ntb_attr_vntb_vid,
+	&epf_ntb_attr_ctrl_bar,
+	&epf_ntb_attr_db_bar,
+	&epf_ntb_attr_mw1_bar,
+	&epf_ntb_attr_mw2_bar,
+	&epf_ntb_attr_mw3_bar,
+	&epf_ntb_attr_mw4_bar,
 	NULL,
 };
 
@@ -1380,6 +1487,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
 {
 	struct epf_ntb *ntb;
 	struct device *dev;
+	int i;
 
 	dev = &epf->dev;
 
@@ -1390,6 +1498,11 @@ static int epf_ntb_probe(struct pci_epf *epf,
 	epf->header = &epf_ntb_header;
 	ntb->epf = epf;
 	ntb->vbus_number = 0xff;
+
+	/* Initially, no bar is assigned */
+	for (i = 0; i < VNTB_BAR_NUM; i++)
+		ntb->epf_ntb_bar[i] = NO_BAR;
+
 	epf_set_drvdata(epf, ntb);
 
 	dev_info(dev, "pci-ep epf driver loaded\n");

-- 
2.47.2

Re: [PATCH 3/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR configuration

[Frank Li]

On Mon, May 05, 2025 at 07:41:49PM +0200, Jerome Brunet wrote:
> The BAR configuration used by the PCI vNTB endpoint function is rather
> fixed and does not allow to account for platform quirks. It simply
> allocate BAR in order.
>
> This is a problem on the Renesas platforms which have a 256B fixed BAR_4
> which end-up being the MW1 BAR. While this BAR is not ideal for a MW, it
> is adequate for the doorbells.
>
> Add more configfs attributes to allow arbitrary BAR configuration to be
> provided through the driver configfs. If not configuration is provided,
> the driver should retain the old behaviour and allocate BARs in order.
> This should keep existing userspace scripts working.
>
> In the Renesas case mentioned above, the change allows to use BAR_2 as for
> the MW1 region and BAR_4 for the doorbells.

Suggest commit message.

PCI: endpoint: pci-epf-vntb: Allow configurable BAR assignment via configfs

The current BAR configuration for the PCI vNTB endpoint function allocates
BARs in order, which lacks flexibility and does not account for
platform-specific quirks. This is problematic on Renesas platforms, where
BAR_4 is a fixed 256B region that ends up being used for MW1, despite being
better suited for doorbells.

Add new configfs attributes to allow users to specify arbitrary BAR
assignments. If no configuration is provided, the driver retains its
original behavior of sequential BAR allocation, preserving compatibility
with existing userspace setups.

This enables use cases such as assigning BAR_2 for MW1 and using the
limited BAR_4 for doorbells on Renesas platforms.

>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 127 ++++++++++++++++++++++++--
>  1 file changed, 120 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index f9f4a8bb65f364962dbf1e9011ab0e4479c61034..3cdccfe870e0cf738c93ca7c525fa2daa7c87fcb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -74,6 +74,7 @@ enum epf_ntb_bar {
>  	BAR_MW2,
>  	BAR_MW3,
>  	BAR_MW4,
> +	VNTB_BAR_NUM,
>  };
>
>  /*
> @@ -133,7 +134,7 @@ struct epf_ntb {
>  	bool linkup;
>  	u32 spad_size;
>
> -	enum pci_barno epf_ntb_bar[6];
> +	enum pci_barno epf_ntb_bar[VNTB_BAR_NUM];

It should be PCI_STD_NUM_BARS

>
>  	struct epf_ntb_ctrl *reg;
>
> @@ -655,6 +656,59 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
>  	pci_epc_put(ntb->epf->epc);
>  }
>
> +
> +/**
> + * epf_ntb_is_bar_used() - Check if a bar is used in the ntb configuration

epf_ntb_is_bar_pre_reverved()?

> + * @ntb: NTB device that facilitates communication between HOST and VHOST

missed @barno

> + *
> + * Returns: 0 if unused, 1 if used.
> + */
> +static int epf_ntb_is_bar_used(struct epf_ntb *ntb,
> +			   enum pci_barno barno)

return value bool is better

> +{
> +	int i;
> +
> +	for (i = 0; i < VNTB_BAR_NUM; i++) {

PCI_STD_NUM_BARS

> +		if (ntb->epf_ntb_bar[i] == barno)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * epf_ntb_set_bar() - Assign BAR number when no configuration is provided

Look like it is find a free bar number, which have not reserved by configfs.
so
epf_ntb_find_bar() or epf_ntb_alloc_bar()?

> + * @ntb: NTB device that facilitates communication between HOST and VHOST

missed bar and barno

> + *
> + * When the BAR configuration was not provided through the userspace
> + * configuration, automatically assign BAR as it has been historically
> + * done by this endpoint function.
> + *
> + * Returns: the BAR number found, if any. -1 otherwise
> + */
> +static int epf_ntb_set_bar(struct epf_ntb *ntb,
> +			   const struct pci_epc_features *epc_features,
> +			   enum epf_ntb_bar bar,
> +			   enum pci_barno barno)
> +{
> +	while (ntb->epf_ntb_bar[bar] < 0) {
> +		barno = pci_epc_get_next_free_bar(epc_features, barno);
> +		if (barno < 0)
> +			break; /* No more BAR available */
> +
> +		/*
> +		 * Verify if the BAR found is not already assigned
> +		 * through the provided configuration
> +		 */
> +		if (!epf_ntb_is_bar_used(ntb, barno))
> +			ntb->epf_ntb_bar[bar] = barno;

missed "break" ? you find one free bar.

> +
> +		barno += 1;
> +	}
> +
> +	return barno;


return ntb->epf_ntb_bar[bar] ?

if pre reserved, while loop will be skipped.  reversed bar number should be
return, instead of input barno.

> +}
> +
>  /**
>   * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
>   * constructs (scratchpad region, doorbell, memorywindow)
> @@ -677,23 +731,21 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
>
>  	/* These are required BARs which are mandatory for NTB functionality */
> -	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
> -		barno = pci_epc_get_next_free_bar(epc_features, barno);
> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++) {
> +		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
>  		if (barno < 0) {
>  			dev_err(dev, "Fail to get NTB function BAR\n");
>  			return -EINVAL;
>  		}
> -		ntb->epf_ntb_bar[bar] = barno;
>  	}
>
>  	/* These are optional BARs which don't impact NTB functionality */
> -	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, barno++, i++) {
> -		barno = pci_epc_get_next_free_bar(epc_features, barno);
> +	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, i++) {
> +		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
>  		if (barno < 0) {
>  			ntb->num_mws = i;
>  			dev_dbg(dev, "BAR not available for > MW%d\n", i + 1);
>  		}
> -		ntb->epf_ntb_bar[bar] = barno;
>  	}
>
>  	return 0;
> @@ -861,6 +913,37 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
>  	return len;							\
>  }
>
> +#define EPF_NTB_BAR_R(_name, _id)					\
> +	static ssize_t epf_ntb_##_name##_show(struct config_item *item,	\
> +					      char *page)		\
> +	{								\
> +		struct config_group *group = to_config_group(item);	\
> +		struct epf_ntb *ntb = to_epf_ntb(group);		\
> +									\
> +		return sprintf(page, "%d\n", ntb->epf_ntb_bar[_id]);	\
> +	}
> +
> +#define EPF_NTB_BAR_W(_name, _id)					\
> +	static ssize_t epf_ntb_##_name##_store(struct config_item *item, \
> +					       const char *page, size_t len) \
> +	{								\
> +	struct config_group *group = to_config_group(item);		\
> +	struct epf_ntb *ntb = to_epf_ntb(group);			\
> +	int val;							\
> +	int ret;							\
> +									\
> +	ret = kstrtoint(page, 0, &val);					\
> +	if (ret)							\
> +		return ret;						\
> +									\
> +	if (val < NO_BAR || val > BAR_5)				\
> +		return -EINVAL;						\
> +									\
> +	ntb->epf_ntb_bar[_id] = val;					\

do you need check the same val to assign two difference ntb bar?

Frank

> +									\
> +	return len;							\
> +	}
> +
>  static ssize_t epf_ntb_num_mws_store(struct config_item *item,
>  				     const char *page, size_t len)
>  {
> @@ -900,6 +983,18 @@ EPF_NTB_MW_R(mw3)
>  EPF_NTB_MW_W(mw3)
>  EPF_NTB_MW_R(mw4)
>  EPF_NTB_MW_W(mw4)
> +EPF_NTB_BAR_R(ctrl_bar, BAR_CONFIG)
> +EPF_NTB_BAR_W(ctrl_bar, BAR_CONFIG)
> +EPF_NTB_BAR_R(db_bar, BAR_DB)
> +EPF_NTB_BAR_W(db_bar, BAR_DB)
> +EPF_NTB_BAR_R(mw1_bar, BAR_MW1)
> +EPF_NTB_BAR_W(mw1_bar, BAR_MW1)
> +EPF_NTB_BAR_R(mw2_bar, BAR_MW1)
> +EPF_NTB_BAR_W(mw2_bar, BAR_MW1)
> +EPF_NTB_BAR_R(mw3_bar, BAR_MW3)
> +EPF_NTB_BAR_W(mw3_bar, BAR_MW3)
> +EPF_NTB_BAR_R(mw4_bar, BAR_MW4)
> +EPF_NTB_BAR_W(mw4_bar, BAR_MW4)
>
>  CONFIGFS_ATTR(epf_ntb_, spad_count);
>  CONFIGFS_ATTR(epf_ntb_, db_count);
> @@ -911,6 +1006,12 @@ CONFIGFS_ATTR(epf_ntb_, mw4);
>  CONFIGFS_ATTR(epf_ntb_, vbus_number);
>  CONFIGFS_ATTR(epf_ntb_, vntb_pid);
>  CONFIGFS_ATTR(epf_ntb_, vntb_vid);
> +CONFIGFS_ATTR(epf_ntb_, ctrl_bar);
> +CONFIGFS_ATTR(epf_ntb_, db_bar);
> +CONFIGFS_ATTR(epf_ntb_, mw1_bar);
> +CONFIGFS_ATTR(epf_ntb_, mw2_bar);
> +CONFIGFS_ATTR(epf_ntb_, mw3_bar);
> +CONFIGFS_ATTR(epf_ntb_, mw4_bar);
>
>  static struct configfs_attribute *epf_ntb_attrs[] = {
>  	&epf_ntb_attr_spad_count,
> @@ -923,6 +1024,12 @@ static struct configfs_attribute *epf_ntb_attrs[] = {
>  	&epf_ntb_attr_vbus_number,
>  	&epf_ntb_attr_vntb_pid,
>  	&epf_ntb_attr_vntb_vid,
> +	&epf_ntb_attr_ctrl_bar,
> +	&epf_ntb_attr_db_bar,
> +	&epf_ntb_attr_mw1_bar,
> +	&epf_ntb_attr_mw2_bar,
> +	&epf_ntb_attr_mw3_bar,
> +	&epf_ntb_attr_mw4_bar,
>  	NULL,
>  };
>
> @@ -1380,6 +1487,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
>  {
>  	struct epf_ntb *ntb;
>  	struct device *dev;
> +	int i;
>
>  	dev = &epf->dev;
>
> @@ -1390,6 +1498,11 @@ static int epf_ntb_probe(struct pci_epf *epf,
>  	epf->header = &epf_ntb_header;
>  	ntb->epf = epf;
>  	ntb->vbus_number = 0xff;
> +
> +	/* Initially, no bar is assigned */
> +	for (i = 0; i < VNTB_BAR_NUM; i++)
> +		ntb->epf_ntb_bar[i] = NO_BAR;
> +
>  	epf_set_drvdata(epf, ntb);
>
>  	dev_info(dev, "pci-ep epf driver loaded\n");
>
> --
> 2.47.2
>

Re: [PATCH 3/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR configuration

[Jerome Brunet]

On Mon 19 May 2025 at 18:29, Frank Li <Frank.li@nxp.com> wrote:

> On Mon, May 05, 2025 at 07:41:49PM +0200, Jerome Brunet wrote:
>> The BAR configuration used by the PCI vNTB endpoint function is rather
>> fixed and does not allow to account for platform quirks. It simply
>> allocate BAR in order.
>>
>> This is a problem on the Renesas platforms which have a 256B fixed BAR_4
>> which end-up being the MW1 BAR. While this BAR is not ideal for a MW, it
>> is adequate for the doorbells.
>>
>> Add more configfs attributes to allow arbitrary BAR configuration to be
>> provided through the driver configfs. If not configuration is provided,
>> the driver should retain the old behaviour and allocate BARs in order.
>> This should keep existing userspace scripts working.
>>
>> In the Renesas case mentioned above, the change allows to use BAR_2 as for
>> the MW1 region and BAR_4 for the doorbells.
>
> Suggest commit message.
>
> PCI: endpoint: pci-epf-vntb: Allow configurable BAR assignment via configfs
>
> The current BAR configuration for the PCI vNTB endpoint function allocates
> BARs in order, which lacks flexibility and does not account for
> platform-specific quirks. This is problematic on Renesas platforms, where
> BAR_4 is a fixed 256B region that ends up being used for MW1, despite being
> better suited for doorbells.
>
> Add new configfs attributes to allow users to specify arbitrary BAR
> assignments. If no configuration is provided, the driver retains its
> original behavior of sequential BAR allocation, preserving compatibility
> with existing userspace setups.
>
> This enables use cases such as assigning BAR_2 for MW1 and using the
> limited BAR_4 for doorbells on Renesas platforms.

Great, thanks

>
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 127 ++++++++++++++++++++++++--
>>  1 file changed, 120 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> index f9f4a8bb65f364962dbf1e9011ab0e4479c61034..3cdccfe870e0cf738c93ca7c525fa2daa7c87fcb 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> @@ -74,6 +74,7 @@ enum epf_ntb_bar {
>>  	BAR_MW2,
>>  	BAR_MW3,
>>  	BAR_MW4,
>> +	VNTB_BAR_NUM,
>>  };
>>
>>  /*
>> @@ -133,7 +134,7 @@ struct epf_ntb {
>>  	bool linkup;
>>  	u32 spad_size;
>>
>> -	enum pci_barno epf_ntb_bar[6];
>> +	enum pci_barno epf_ntb_bar[VNTB_BAR_NUM];
>
> It should be PCI_STD_NUM_BARS

I thought so too initially but that's actually not the same thing and
wrong, if it happens to be 6 here.

This tracks the mapping of function to bar number, not which function is
assigned to a BAR.

>
>>
>>  	struct epf_ntb_ctrl *reg;
>>
>> @@ -655,6 +656,59 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
>>  	pci_epc_put(ntb->epf->epc);
>>  }
>>
>> +
>> +/**
>> + * epf_ntb_is_bar_used() - Check if a bar is used in the ntb configuration
>
> epf_ntb_is_bar_pre_reverved()?

That would be mis-leading because the result change as the sequential
allocation goes, so it is not limited to pre-reservation.

>
>> + * @ntb: NTB device that facilitates communication between HOST and VHOST
>
> missed @barno
>
>> + *
>> + * Returns: 0 if unused, 1 if used.
>> + */
>> +static int epf_ntb_is_bar_used(struct epf_ntb *ntb,
>> +			   enum pci_barno barno)
>
> return value bool is better

Fine by me

>
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < VNTB_BAR_NUM; i++) {
>
> PCI_STD_NUM_BARS

As noted above, it is easy to get confused on this but that would be incorrect.

>
>> +		if (ntb->epf_ntb_bar[i] == barno)
>> +			return 1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * epf_ntb_set_bar() - Assign BAR number when no configuration is provided
>
> Look like it is find a free bar number, which have not reserved by configfs.
> so
> epf_ntb_find_bar() or epf_ntb_alloc_bar()?

I'll replace with find_bar sure.

>
>> + * @ntb: NTB device that facilitates communication between HOST and VHOST
>
> missed bar and barno
>
>> + *
>> + * When the BAR configuration was not provided through the userspace
>> + * configuration, automatically assign BAR as it has been historically
>> + * done by this endpoint function.
>> + *
>> + * Returns: the BAR number found, if any. -1 otherwise
>> + */
>> +static int epf_ntb_set_bar(struct epf_ntb *ntb,
>> +			   const struct pci_epc_features *epc_features,
>> +			   enum epf_ntb_bar bar,
>> +			   enum pci_barno barno)
>> +{
>> +	while (ntb->epf_ntb_bar[bar] < 0) {
>> +		barno = pci_epc_get_next_free_bar(epc_features, barno);
>> +		if (barno < 0)
>> +			break; /* No more BAR available */
>> +
>> +		/*
>> +		 * Verify if the BAR found is not already assigned
>> +		 * through the provided configuration
>> +		 */
>> +		if (!epf_ntb_is_bar_used(ntb, barno))
>> +			ntb->epf_ntb_bar[bar] = barno;
>
> missed "break" ? you find one free bar.

No ... the while exit condition is already correct I think

>
>> +
>> +		barno += 1;
>> +	}
>> +
>> +	return barno;
>
>
> return ntb->epf_ntb_bar[bar] ?
>
> if pre reserved, while loop will be skipped.  reversed bar number should be
> return, instead of input barno.

I don't think so.

Say a config sets DB on BAR6, while still having everything unused from
2 to 5, you'd get stuck with what you are proposing. What's done here
emulate the old behavior while making sure we iterate over all BARs

That being said, mixing the old ways with explicit config would be weird
but it is possible.

>
>> +}
>> +
>>  /**
>>   * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
>>   * constructs (scratchpad region, doorbell, memorywindow)
>> @@ -677,23 +731,21 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
>>
>>  	/* These are required BARs which are mandatory for NTB functionality */
>> -	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
>> -		barno = pci_epc_get_next_free_bar(epc_features, barno);
>> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++) {
>> +		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
>>  		if (barno < 0) {
>>  			dev_err(dev, "Fail to get NTB function BAR\n");
>>  			return -EINVAL;
>>  		}
>> -		ntb->epf_ntb_bar[bar] = barno;
>>  	}
>>
>>  	/* These are optional BARs which don't impact NTB functionality */
>> -	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, barno++, i++) {
>> -		barno = pci_epc_get_next_free_bar(epc_features, barno);
>> +	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, i++) {
>> +		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
>>  		if (barno < 0) {
>>  			ntb->num_mws = i;
>>  			dev_dbg(dev, "BAR not available for > MW%d\n", i + 1);
>>  		}
>> -		ntb->epf_ntb_bar[bar] = barno;
>>  	}
>>
>>  	return 0;
>> @@ -861,6 +913,37 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
>>  	return len;							\
>>  }
>>
>> +#define EPF_NTB_BAR_R(_name, _id)					\
>> +	static ssize_t epf_ntb_##_name##_show(struct config_item *item,	\
>> +					      char *page)		\
>> +	{								\
>> +		struct config_group *group = to_config_group(item);	\
>> +		struct epf_ntb *ntb = to_epf_ntb(group);		\
>> +									\
>> +		return sprintf(page, "%d\n", ntb->epf_ntb_bar[_id]);	\
>> +	}
>> +
>> +#define EPF_NTB_BAR_W(_name, _id)					\
>> +	static ssize_t epf_ntb_##_name##_store(struct config_item *item, \
>> +					       const char *page, size_t len) \
>> +	{								\
>> +	struct config_group *group = to_config_group(item);		\
>> +	struct epf_ntb *ntb = to_epf_ntb(group);			\
>> +	int val;							\
>> +	int ret;							\
>> +									\
>> +	ret = kstrtoint(page, 0, &val);					\
>> +	if (ret)							\
>> +		return ret;						\
>> +									\
>> +	if (val < NO_BAR || val > BAR_5)				\
>> +		return -EINVAL;						\
>> +									\
>> +	ntb->epf_ntb_bar[_id] = val;					\
>
> do you need check the same val to assign two difference ntb bar?

I rely on the user input being correct indeed. Worst case, an allocation
will fail later on. I could try to implement something in that direction
but will get complex. For example, I would eventually like to allow
sharing the BAR for DB and MW1, as done on the NTB function. (The idea
is to get 2nd MW and enable MSI on the ntb transport but I'm not there yet)

>
> Frank
>
>> +									\
>> +	return len;							\
>> +	}
>> +
>>  static ssize_t epf_ntb_num_mws_store(struct config_item *item,
>>  				     const char *page, size_t len)
>>  {
>> @@ -900,6 +983,18 @@ EPF_NTB_MW_R(mw3)
>>  EPF_NTB_MW_W(mw3)
>>  EPF_NTB_MW_R(mw4)
>>  EPF_NTB_MW_W(mw4)
>> +EPF_NTB_BAR_R(ctrl_bar, BAR_CONFIG)
>> +EPF_NTB_BAR_W(ctrl_bar, BAR_CONFIG)
>> +EPF_NTB_BAR_R(db_bar, BAR_DB)
>> +EPF_NTB_BAR_W(db_bar, BAR_DB)
>> +EPF_NTB_BAR_R(mw1_bar, BAR_MW1)
>> +EPF_NTB_BAR_W(mw1_bar, BAR_MW1)
>> +EPF_NTB_BAR_R(mw2_bar, BAR_MW1)
>> +EPF_NTB_BAR_W(mw2_bar, BAR_MW1)
>> +EPF_NTB_BAR_R(mw3_bar, BAR_MW3)
>> +EPF_NTB_BAR_W(mw3_bar, BAR_MW3)
>> +EPF_NTB_BAR_R(mw4_bar, BAR_MW4)
>> +EPF_NTB_BAR_W(mw4_bar, BAR_MW4)
>>
>>  CONFIGFS_ATTR(epf_ntb_, spad_count);
>>  CONFIGFS_ATTR(epf_ntb_, db_count);
>> @@ -911,6 +1006,12 @@ CONFIGFS_ATTR(epf_ntb_, mw4);
>>  CONFIGFS_ATTR(epf_ntb_, vbus_number);
>>  CONFIGFS_ATTR(epf_ntb_, vntb_pid);
>>  CONFIGFS_ATTR(epf_ntb_, vntb_vid);
>> +CONFIGFS_ATTR(epf_ntb_, ctrl_bar);
>> +CONFIGFS_ATTR(epf_ntb_, db_bar);
>> +CONFIGFS_ATTR(epf_ntb_, mw1_bar);
>> +CONFIGFS_ATTR(epf_ntb_, mw2_bar);
>> +CONFIGFS_ATTR(epf_ntb_, mw3_bar);
>> +CONFIGFS_ATTR(epf_ntb_, mw4_bar);
>>
>>  static struct configfs_attribute *epf_ntb_attrs[] = {
>>  	&epf_ntb_attr_spad_count,
>> @@ -923,6 +1024,12 @@ static struct configfs_attribute *epf_ntb_attrs[] = {
>>  	&epf_ntb_attr_vbus_number,
>>  	&epf_ntb_attr_vntb_pid,
>>  	&epf_ntb_attr_vntb_vid,
>> +	&epf_ntb_attr_ctrl_bar,
>> +	&epf_ntb_attr_db_bar,
>> +	&epf_ntb_attr_mw1_bar,
>> +	&epf_ntb_attr_mw2_bar,
>> +	&epf_ntb_attr_mw3_bar,
>> +	&epf_ntb_attr_mw4_bar,
>>  	NULL,
>>  };
>>
>> @@ -1380,6 +1487,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
>>  {
>>  	struct epf_ntb *ntb;
>>  	struct device *dev;
>> +	int i;
>>
>>  	dev = &epf->dev;
>>
>> @@ -1390,6 +1498,11 @@ static int epf_ntb_probe(struct pci_epf *epf,
>>  	epf->header = &epf_ntb_header;
>>  	ntb->epf = epf;
>>  	ntb->vbus_number = 0xff;
>> +
>> +	/* Initially, no bar is assigned */
>> +	for (i = 0; i < VNTB_BAR_NUM; i++)
>> +		ntb->epf_ntb_bar[i] = NO_BAR;
>> +
>>  	epf_set_drvdata(epf, ntb);
>>
>>  	dev_info(dev, "pci-ep epf driver loaded\n");
>>
>> --
>> 2.47.2
>>

-- 
Jerome

Re: [PATCH 3/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR configuration

[Frank Li]

On Tue, May 20, 2025 at 10:06:35AM +0200, Jerome Brunet wrote:
> On Mon 19 May 2025 at 18:29, Frank Li <Frank.li@nxp.com> wrote:
>
> > On Mon, May 05, 2025 at 07:41:49PM +0200, Jerome Brunet wrote:
> >> The BAR configuration used by the PCI vNTB endpoint function is rather
> >> fixed and does not allow to account for platform quirks. It simply
> >> allocate BAR in order.
> >>
> >> This is a problem on the Renesas platforms which have a 256B fixed BAR_4
> >> which end-up being the MW1 BAR. While this BAR is not ideal for a MW, it
> >> is adequate for the doorbells.
> >>
> >> Add more configfs attributes to allow arbitrary BAR configuration to be
> >> provided through the driver configfs. If not configuration is provided,
> >> the driver should retain the old behaviour and allocate BARs in order.
> >> This should keep existing userspace scripts working.
> >>
> >> In the Renesas case mentioned above, the change allows to use BAR_2 as for
> >> the MW1 region and BAR_4 for the doorbells.
> >
> > Suggest commit message.
> >
> > PCI: endpoint: pci-epf-vntb: Allow configurable BAR assignment via configfs
> >
> > The current BAR configuration for the PCI vNTB endpoint function allocates
> > BARs in order, which lacks flexibility and does not account for
> > platform-specific quirks. This is problematic on Renesas platforms, where
> > BAR_4 is a fixed 256B region that ends up being used for MW1, despite being
> > better suited for doorbells.
> >
> > Add new configfs attributes to allow users to specify arbitrary BAR
> > assignments. If no configuration is provided, the driver retains its
> > original behavior of sequential BAR allocation, preserving compatibility
> > with existing userspace setups.
> >
> > This enables use cases such as assigning BAR_2 for MW1 and using the
> > limited BAR_4 for doorbells on Renesas platforms.
>
> Great, thanks
>
> >
> >>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 127 ++++++++++++++++++++++++--
> >>  1 file changed, 120 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> index f9f4a8bb65f364962dbf1e9011ab0e4479c61034..3cdccfe870e0cf738c93ca7c525fa2daa7c87fcb 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> @@ -74,6 +74,7 @@ enum epf_ntb_bar {
> >>  	BAR_MW2,
> >>  	BAR_MW3,
> >>  	BAR_MW4,
> >> +	VNTB_BAR_NUM,
> >>  };
> >>
> >>  /*
> >> @@ -133,7 +134,7 @@ struct epf_ntb {
> >>  	bool linkup;
> >>  	u32 spad_size;
> >>
> >> -	enum pci_barno epf_ntb_bar[6];
> >> +	enum pci_barno epf_ntb_bar[VNTB_BAR_NUM];
> >
> > It should be PCI_STD_NUM_BARS
>
> I thought so too initially but that's actually not the same thing and
> wrong, if it happens to be 6 here.
>
> This tracks the mapping of function to bar number, not which function is
> assigned to a BAR.

Okay.

>
> >
> >>
> >>  	struct epf_ntb_ctrl *reg;
> >>
> >> @@ -655,6 +656,59 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
> >>  	pci_epc_put(ntb->epf->epc);
> >>  }
> >>
> >> +
> >> +/**
> >> + * epf_ntb_is_bar_used() - Check if a bar is used in the ntb configuration
> >
> > epf_ntb_is_bar_pre_reverved()?
>
> That would be mis-leading because the result change as the sequential
> allocation goes, so it is not limited to pre-reservation.
>
> >
> >> + * @ntb: NTB device that facilitates communication between HOST and VHOST
> >
> > missed @barno
> >
> >> + *
> >> + * Returns: 0 if unused, 1 if used.
> >> + */
> >> +static int epf_ntb_is_bar_used(struct epf_ntb *ntb,
> >> +			   enum pci_barno barno)
> >
> > return value bool is better
>
> Fine by me
>
> >
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < VNTB_BAR_NUM; i++) {
> >
> > PCI_STD_NUM_BARS
>
> As noted above, it is easy to get confused on this but that would be incorrect.
>
> >
> >> +		if (ntb->epf_ntb_bar[i] == barno)
> >> +			return 1;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * epf_ntb_set_bar() - Assign BAR number when no configuration is provided
> >
> > Look like it is find a free bar number, which have not reserved by configfs.
> > so
> > epf_ntb_find_bar() or epf_ntb_alloc_bar()?
>
> I'll replace with find_bar sure.
>
> >
> >> + * @ntb: NTB device that facilitates communication between HOST and VHOST
> >
> > missed bar and barno
> >
> >> + *
> >> + * When the BAR configuration was not provided through the userspace
> >> + * configuration, automatically assign BAR as it has been historically
> >> + * done by this endpoint function.
> >> + *
> >> + * Returns: the BAR number found, if any. -1 otherwise
> >> + */
> >> +static int epf_ntb_set_bar(struct epf_ntb *ntb,
> >> +			   const struct pci_epc_features *epc_features,
> >> +			   enum epf_ntb_bar bar,
> >> +			   enum pci_barno barno)
> >> +{
> >> +	while (ntb->epf_ntb_bar[bar] < 0) {
> >> +		barno = pci_epc_get_next_free_bar(epc_features, barno);
> >> +		if (barno < 0)
> >> +			break; /* No more BAR available */
> >> +
> >> +		/*
> >> +		 * Verify if the BAR found is not already assigned
> >> +		 * through the provided configuration
> >> +		 */
> >> +		if (!epf_ntb_is_bar_used(ntb, barno))
> >> +			ntb->epf_ntb_bar[bar] = barno;
> >
> > missed "break" ? you find one free bar.
>
> No ... the while exit condition is already correct I think

You are right.

>
> >
> >> +
> >> +		barno += 1;
> >> +	}
> >> +
> >> +	return barno;
> >
> >
> > return ntb->epf_ntb_bar[bar] ?
> >
> > if pre reserved, while loop will be skipped.  reversed bar number should be
> > return, instead of input barno.
>
> I don't think so.
>
> Say a config sets DB on BAR6, while still having everything unused from
> 2 to 5, you'd get stuck with what you are proposing. What's done here
> emulate the old behavior while making sure we iterate over all BARs
>
> That being said, mixing the old ways with explicit config would be weird
> but it is possible.
>
> >
> >> +}
> >> +
> >>  /**
> >>   * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
> >>   * constructs (scratchpad region, doorbell, memorywindow)
> >> @@ -677,23 +731,21 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
> >>  	epc_features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no);
> >>
> >>  	/* These are required BARs which are mandatory for NTB functionality */
> >> -	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++, barno++) {
> >> -		barno = pci_epc_get_next_free_bar(epc_features, barno);
> >> +	for (bar = BAR_CONFIG; bar <= BAR_MW1; bar++) {
> >> +		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
> >>  		if (barno < 0) {
> >>  			dev_err(dev, "Fail to get NTB function BAR\n");
> >>  			return -EINVAL;
> >>  		}
> >> -		ntb->epf_ntb_bar[bar] = barno;
> >>  	}
> >>
> >>  	/* These are optional BARs which don't impact NTB functionality */
> >> -	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, barno++, i++) {
> >> -		barno = pci_epc_get_next_free_bar(epc_features, barno);
> >> +	for (bar = BAR_MW1, i = 1; i < num_mws; bar++, i++) {
> >> +		barno = epf_ntb_set_bar(ntb, epc_features, bar, barno);
> >>  		if (barno < 0) {
> >>  			ntb->num_mws = i;
> >>  			dev_dbg(dev, "BAR not available for > MW%d\n", i + 1);
> >>  		}
> >> -		ntb->epf_ntb_bar[bar] = barno;
> >>  	}
> >>
> >>  	return 0;
> >> @@ -861,6 +913,37 @@ static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
> >>  	return len;							\
> >>  }
> >>
> >> +#define EPF_NTB_BAR_R(_name, _id)					\
> >> +	static ssize_t epf_ntb_##_name##_show(struct config_item *item,	\
> >> +					      char *page)		\
> >> +	{								\
> >> +		struct config_group *group = to_config_group(item);	\
> >> +		struct epf_ntb *ntb = to_epf_ntb(group);		\
> >> +									\
> >> +		return sprintf(page, "%d\n", ntb->epf_ntb_bar[_id]);	\
> >> +	}
> >> +
> >> +#define EPF_NTB_BAR_W(_name, _id)					\
> >> +	static ssize_t epf_ntb_##_name##_store(struct config_item *item, \
> >> +					       const char *page, size_t len) \
> >> +	{								\
> >> +	struct config_group *group = to_config_group(item);		\
> >> +	struct epf_ntb *ntb = to_epf_ntb(group);			\
> >> +	int val;							\
> >> +	int ret;							\
> >> +									\
> >> +	ret = kstrtoint(page, 0, &val);					\
> >> +	if (ret)							\
> >> +		return ret;						\
> >> +									\
> >> +	if (val < NO_BAR || val > BAR_5)				\
> >> +		return -EINVAL;						\
> >> +									\
> >> +	ntb->epf_ntb_bar[_id] = val;					\
> >
> > do you need check the same val to assign two difference ntb bar?
>
> I rely on the user input being correct indeed. Worst case, an allocation
> will fail later on. I could try to implement something in that direction
> but will get complex. For example, I would eventually like to allow
> sharing the BAR for DB and MW1, as done on the NTB function. (The idea
> is to get 2nd MW and enable MSI on the ntb transport but I'm not there yet)

Okay.

Frank
>
> >
> > Frank
> >
> >> +									\
> >> +	return len;							\
> >> +	}
> >> +
> >>  static ssize_t epf_ntb_num_mws_store(struct config_item *item,
> >>  				     const char *page, size_t len)
> >>  {
> >> @@ -900,6 +983,18 @@ EPF_NTB_MW_R(mw3)
> >>  EPF_NTB_MW_W(mw3)
> >>  EPF_NTB_MW_R(mw4)
> >>  EPF_NTB_MW_W(mw4)
> >> +EPF_NTB_BAR_R(ctrl_bar, BAR_CONFIG)
> >> +EPF_NTB_BAR_W(ctrl_bar, BAR_CONFIG)
> >> +EPF_NTB_BAR_R(db_bar, BAR_DB)
> >> +EPF_NTB_BAR_W(db_bar, BAR_DB)
> >> +EPF_NTB_BAR_R(mw1_bar, BAR_MW1)
> >> +EPF_NTB_BAR_W(mw1_bar, BAR_MW1)
> >> +EPF_NTB_BAR_R(mw2_bar, BAR_MW1)
> >> +EPF_NTB_BAR_W(mw2_bar, BAR_MW1)
> >> +EPF_NTB_BAR_R(mw3_bar, BAR_MW3)
> >> +EPF_NTB_BAR_W(mw3_bar, BAR_MW3)
> >> +EPF_NTB_BAR_R(mw4_bar, BAR_MW4)
> >> +EPF_NTB_BAR_W(mw4_bar, BAR_MW4)
> >>
> >>  CONFIGFS_ATTR(epf_ntb_, spad_count);
> >>  CONFIGFS_ATTR(epf_ntb_, db_count);
> >> @@ -911,6 +1006,12 @@ CONFIGFS_ATTR(epf_ntb_, mw4);
> >>  CONFIGFS_ATTR(epf_ntb_, vbus_number);
> >>  CONFIGFS_ATTR(epf_ntb_, vntb_pid);
> >>  CONFIGFS_ATTR(epf_ntb_, vntb_vid);
> >> +CONFIGFS_ATTR(epf_ntb_, ctrl_bar);
> >> +CONFIGFS_ATTR(epf_ntb_, db_bar);
> >> +CONFIGFS_ATTR(epf_ntb_, mw1_bar);
> >> +CONFIGFS_ATTR(epf_ntb_, mw2_bar);
> >> +CONFIGFS_ATTR(epf_ntb_, mw3_bar);
> >> +CONFIGFS_ATTR(epf_ntb_, mw4_bar);
> >>
> >>  static struct configfs_attribute *epf_ntb_attrs[] = {
> >>  	&epf_ntb_attr_spad_count,
> >> @@ -923,6 +1024,12 @@ static struct configfs_attribute *epf_ntb_attrs[] = {
> >>  	&epf_ntb_attr_vbus_number,
> >>  	&epf_ntb_attr_vntb_pid,
> >>  	&epf_ntb_attr_vntb_vid,
> >> +	&epf_ntb_attr_ctrl_bar,
> >> +	&epf_ntb_attr_db_bar,
> >> +	&epf_ntb_attr_mw1_bar,
> >> +	&epf_ntb_attr_mw2_bar,
> >> +	&epf_ntb_attr_mw3_bar,
> >> +	&epf_ntb_attr_mw4_bar,
> >>  	NULL,
> >>  };
> >>
> >> @@ -1380,6 +1487,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
> >>  {
> >>  	struct epf_ntb *ntb;
> >>  	struct device *dev;
> >> +	int i;
> >>
> >>  	dev = &epf->dev;
> >>
> >> @@ -1390,6 +1498,11 @@ static int epf_ntb_probe(struct pci_epf *epf,
> >>  	epf->header = &epf_ntb_header;
> >>  	ntb->epf = epf;
> >>  	ntb->vbus_number = 0xff;
> >> +
> >> +	/* Initially, no bar is assigned */
> >> +	for (i = 0; i < VNTB_BAR_NUM; i++)
> >> +		ntb->epf_ntb_bar[i] = NO_BAR;
> >> +
> >>  	epf_set_drvdata(epf, ntb);
> >>
> >>  	dev_info(dev, "pci-ep epf driver loaded\n");
> >>
> >> --
> >> 2.47.2
> >>
>
> --
> Jerome

[PATCH 4/4] NTB: epf: Allow arbitrary BAR mapping

[Jerome Brunet]

The NTB epf host driver assumes the BAR number associated with a memory
window is just incremented from the BAR number associated with MW1. This
seems to have been enough so far but this is not really how the endpoint
side work and the two could easily become mis-aligned.

ntb_epf_mw_to_bar() even assumes that the BAR number is the memory window
index + 2, which means the function only returns a proper result if BAR_2
is associated with MW1.

Instead, fully describe and allow arbitrary NTB BAR mapping.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/ntb/hw/epf/ntb_hw_epf.c | 108 ++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 53 deletions(-)

diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
index 00f0e78f685bf7917b02dd8a52b5b35f68d5bb64..9539cdcd0f8fa4b5c5e66477672f8f97d5ec4e52 100644
--- a/drivers/ntb/hw/epf/ntb_hw_epf.c
+++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
@@ -49,6 +49,7 @@
 #define NTB_EPF_COMMAND_TIMEOUT	1000 /* 1 Sec */
 
 enum pci_barno {
+	NO_BAR = -1,
 	BAR_0,
 	BAR_1,
 	BAR_2,
@@ -57,16 +58,26 @@ enum pci_barno {
 	BAR_5,
 };
 
+enum epf_ntb_bar {
+	BAR_CONFIG,
+	BAR_PEER_SPAD,
+	BAR_DB,
+	BAR_MW1,
+	BAR_MW2,
+	BAR_MW3,
+	BAR_MW4,
+	NTB_BAR_NUM,
+};
+
+#define NTB_EPF_MAX_MW_COUNT	(NTB_BAR_NUM - BAR_MW1)
+
 struct ntb_epf_dev {
 	struct ntb_dev ntb;
 	struct device *dev;
 	/* Mutex to protect providing commands to NTB EPF */
 	struct mutex cmd_lock;
 
-	enum pci_barno ctrl_reg_bar;
-	enum pci_barno peer_spad_reg_bar;
-	enum pci_barno db_reg_bar;
-	enum pci_barno mw_bar;
+	const enum pci_barno *barno;
 
 	unsigned int mw_count;
 	unsigned int spad_count;
@@ -85,17 +96,6 @@ struct ntb_epf_dev {
 
 #define ntb_ndev(__ntb) container_of(__ntb, struct ntb_epf_dev, ntb)
 
-struct ntb_epf_data {
-	/* BAR that contains both control region and self spad region */
-	enum pci_barno ctrl_reg_bar;
-	/* BAR that contains peer spad region */
-	enum pci_barno peer_spad_reg_bar;
-	/* BAR that contains Doorbell region and Memory window '1' */
-	enum pci_barno db_reg_bar;
-	/* BAR that contains memory windows*/
-	enum pci_barno mw_bar;
-};
-
 static int ntb_epf_send_command(struct ntb_epf_dev *ndev, u32 command,
 				u32 argument)
 {
@@ -144,7 +144,7 @@ static int ntb_epf_mw_to_bar(struct ntb_epf_dev *ndev, int idx)
 		return -EINVAL;
 	}
 
-	return idx + 2;
+	return ndev->barno[BAR_MW1 + idx];
 }
 
 static int ntb_epf_mw_count(struct ntb_dev *ntb, int pidx)
@@ -413,7 +413,9 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
 		return -EINVAL;
 	}
 
-	bar = idx + ndev->mw_bar;
+	bar = ntb_epf_mw_to_bar(ndev, idx);
+	if (bar < 0)
+		return bar;
 
 	mw_size = pci_resource_len(ntb->pdev, bar);
 
@@ -455,7 +457,9 @@ static int ntb_epf_peer_mw_get_addr(struct ntb_dev *ntb, int idx,
 	if (idx == 0)
 		offset = readl(ndev->ctrl_reg + NTB_EPF_MW1_OFFSET);
 
-	bar = idx + ndev->mw_bar;
+	bar = ntb_epf_mw_to_bar(ndev, idx);
+	if (bar < 0)
+		return bar;
 
 	if (base)
 		*base = pci_resource_start(ndev->ntb.pdev, bar) + offset;
@@ -557,8 +561,13 @@ static int ntb_epf_init_dev(struct ntb_epf_dev *ndev)
 	}
 
 	ndev->db_valid_mask = BIT_ULL(ndev->db_count) - 1;
-	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
 	ndev->spad_count = readl(ndev->ctrl_reg + NTB_EPF_SPAD_COUNT);
+	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
+
+	if (ndev->mw_count > NTB_EPF_MAX_MW_COUNT) {
+		dev_err(dev, "Unsupported MW count: %u\n", ndev->mw_count);
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -596,14 +605,14 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
 		dev_warn(&pdev->dev, "Cannot DMA highmem\n");
 	}
 
-	ndev->ctrl_reg = pci_iomap(pdev, ndev->ctrl_reg_bar, 0);
+	ndev->ctrl_reg = pci_iomap(pdev, ndev->barno[BAR_CONFIG], 0);
 	if (!ndev->ctrl_reg) {
 		ret = -EIO;
 		goto err_pci_regions;
 	}
 
-	if (ndev->peer_spad_reg_bar) {
-		ndev->peer_spad_reg = pci_iomap(pdev, ndev->peer_spad_reg_bar, 0);
+	if (ndev->barno[BAR_PEER_SPAD] != ndev->barno[BAR_CONFIG]) {
+		ndev->peer_spad_reg = pci_iomap(pdev, ndev->barno[BAR_PEER_SPAD], 0);
 		if (!ndev->peer_spad_reg) {
 			ret = -EIO;
 			goto err_pci_regions;
@@ -614,7 +623,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
 		ndev->peer_spad_reg = ndev->ctrl_reg + spad_off  + spad_sz;
 	}
 
-	ndev->db_reg = pci_iomap(pdev, ndev->db_reg_bar, 0);
+	ndev->db_reg = pci_iomap(pdev, ndev->barno[BAR_DB], 0);
 	if (!ndev->db_reg) {
 		ret = -EIO;
 		goto err_pci_regions;
@@ -656,15 +665,20 @@ static void ntb_epf_cleanup_isr(struct ntb_epf_dev *ndev)
 	pci_free_irq_vectors(pdev);
 }
 
+static const enum pci_barno ntb_epf_default_barno[NTB_BAR_NUM] = {
+	[BAR_CONFIG]	= BAR_0,
+	[BAR_PEER_SPAD]	= BAR_1,
+	[BAR_DB]	= BAR_2,
+	[BAR_MW1]	= BAR_2,
+	[BAR_MW2]	= BAR_3,
+	[BAR_MW3]	= BAR_4,
+	[BAR_MW4]	= BAR_5
+};
+
 static int ntb_epf_pci_probe(struct pci_dev *pdev,
 			     const struct pci_device_id *id)
 {
-	enum pci_barno peer_spad_reg_bar = BAR_1;
-	enum pci_barno ctrl_reg_bar = BAR_0;
-	enum pci_barno db_reg_bar = BAR_2;
-	enum pci_barno mw_bar = BAR_2;
 	struct device *dev = &pdev->dev;
-	struct ntb_epf_data *data;
 	struct ntb_epf_dev *ndev;
 	int ret;
 
@@ -675,18 +689,10 @@ static int ntb_epf_pci_probe(struct pci_dev *pdev,
 	if (!ndev)
 		return -ENOMEM;
 
-	data = (struct ntb_epf_data *)id->driver_data;
-	if (data) {
-		peer_spad_reg_bar = data->peer_spad_reg_bar;
-		ctrl_reg_bar = data->ctrl_reg_bar;
-		db_reg_bar = data->db_reg_bar;
-		mw_bar = data->mw_bar;
-	}
+	ndev->barno = (const enum pci_barno *)id->driver_data;
+	if (!ndev->barno)
+		ndev->barno = ntb_epf_default_barno;
 
-	ndev->peer_spad_reg_bar = peer_spad_reg_bar;
-	ndev->ctrl_reg_bar = ctrl_reg_bar;
-	ndev->db_reg_bar = db_reg_bar;
-	ndev->mw_bar = mw_bar;
 	ndev->dev = dev;
 
 	ntb_epf_init_struct(ndev, pdev);
@@ -730,30 +736,26 @@ static void ntb_epf_pci_remove(struct pci_dev *pdev)
 	ntb_epf_deinit_pci(ndev);
 }
 
-static const struct ntb_epf_data j721e_data = {
-	.ctrl_reg_bar = BAR_0,
-	.peer_spad_reg_bar = BAR_1,
-	.db_reg_bar = BAR_2,
-	.mw_bar = BAR_2,
-};
-
-static const struct ntb_epf_data mx8_data = {
-	.ctrl_reg_bar = BAR_0,
-	.peer_spad_reg_bar = BAR_0,
-	.db_reg_bar = BAR_2,
-	.mw_bar = BAR_4,
+static const enum pci_barno mx8_barno[NTB_BAR_NUM] = {
+	[BAR_CONFIG]	= BAR_0,
+	[BAR_PEER_SPAD]	= BAR_0,
+	[BAR_DB]	= BAR_2,
+	[BAR_MW1]	= BAR_4,
+	[BAR_MW2]	= BAR_5,
+	[BAR_MW3]	= NO_BAR,
+	[BAR_MW4]	= NO_BAR,
 };
 
 static const struct pci_device_id ntb_epf_pci_tbl[] = {
 	{
 		PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
 		.class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
-		.driver_data = (kernel_ulong_t)&j721e_data,
+		.driver_data = (kernel_ulong_t)ntb_epf_default_barno,
 	},
 	{
 		PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x0809),
 		.class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
-		.driver_data = (kernel_ulong_t)&mx8_data,
+		.driver_data = (kernel_ulong_t)mx8_barno,
 	},
 	{ },
 };

-- 
2.47.2

Re: [PATCH 4/4] NTB: epf: Allow arbitrary BAR mapping

[Frank Li]

On Mon, May 05, 2025 at 07:41:50PM +0200, Jerome Brunet wrote:
> The NTB epf host driver assumes the BAR number associated with a memory
> window is just incremented from the BAR number associated with MW1. This
> seems to have been enough so far but this is not really how the endpoint
> side work and the two could easily become mis-aligned.
>
> ntb_epf_mw_to_bar() even assumes that the BAR number is the memory window
> index + 2, which means the function only returns a proper result if BAR_2
> is associated with MW1.
>
> Instead, fully describe and allow arbitrary NTB BAR mapping.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/ntb/hw/epf/ntb_hw_epf.c | 108 ++++++++++++++++++++--------------------
>  1 file changed, 55 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> index 00f0e78f685bf7917b02dd8a52b5b35f68d5bb64..9539cdcd0f8fa4b5c5e66477672f8f97d5ec4e52 100644
> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
> @@ -49,6 +49,7 @@
>  #define NTB_EPF_COMMAND_TIMEOUT	1000 /* 1 Sec */
>
>  enum pci_barno {
> +	NO_BAR = -1,

Not related with this patch, but there are too many place to define
enum pci_barno. it need be consolidate.

>  	BAR_0,
>  	BAR_1,
>  	BAR_2,
> @@ -57,16 +58,26 @@ enum pci_barno {
>  	BAR_5,
>  };
>
> +enum epf_ntb_bar {
> +	BAR_CONFIG,
> +	BAR_PEER_SPAD,
> +	BAR_DB,
> +	BAR_MW1,
> +	BAR_MW2,
> +	BAR_MW3,
> +	BAR_MW4,
> +	NTB_BAR_NUM,
> +};
> +
> +#define NTB_EPF_MAX_MW_COUNT	(NTB_BAR_NUM - BAR_MW1)
> +
>  struct ntb_epf_dev {
>  	struct ntb_dev ntb;
>  	struct device *dev;
>  	/* Mutex to protect providing commands to NTB EPF */
>  	struct mutex cmd_lock;
>
> -	enum pci_barno ctrl_reg_bar;
> -	enum pci_barno peer_spad_reg_bar;
> -	enum pci_barno db_reg_bar;
> -	enum pci_barno mw_bar;
> +	const enum pci_barno *barno;

barno_map?

>
>  	unsigned int mw_count;
>  	unsigned int spad_count;
> @@ -85,17 +96,6 @@ struct ntb_epf_dev {
>
>  #define ntb_ndev(__ntb) container_of(__ntb, struct ntb_epf_dev, ntb)
>
> -struct ntb_epf_data {
> -	/* BAR that contains both control region and self spad region */
> -	enum pci_barno ctrl_reg_bar;
> -	/* BAR that contains peer spad region */
> -	enum pci_barno peer_spad_reg_bar;
> -	/* BAR that contains Doorbell region and Memory window '1' */
> -	enum pci_barno db_reg_bar;
> -	/* BAR that contains memory windows*/
> -	enum pci_barno mw_bar;
> -};
> -
>  static int ntb_epf_send_command(struct ntb_epf_dev *ndev, u32 command,
>  				u32 argument)
>  {
> @@ -144,7 +144,7 @@ static int ntb_epf_mw_to_bar(struct ntb_epf_dev *ndev, int idx)
>  		return -EINVAL;
>  	}
>
> -	return idx + 2;
> +	return ndev->barno[BAR_MW1 + idx];
>  }
>
>  static int ntb_epf_mw_count(struct ntb_dev *ntb, int pidx)
> @@ -413,7 +413,9 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
>  		return -EINVAL;
>  	}
>
> -	bar = idx + ndev->mw_bar;
> +	bar = ntb_epf_mw_to_bar(ndev, idx);
> +	if (bar < 0)
> +		return bar;
>
>  	mw_size = pci_resource_len(ntb->pdev, bar);
>
> @@ -455,7 +457,9 @@ static int ntb_epf_peer_mw_get_addr(struct ntb_dev *ntb, int idx,
>  	if (idx == 0)
>  		offset = readl(ndev->ctrl_reg + NTB_EPF_MW1_OFFSET);
>
> -	bar = idx + ndev->mw_bar;
> +	bar = ntb_epf_mw_to_bar(ndev, idx);
> +	if (bar < 0)
> +		return bar;
>
>  	if (base)
>  		*base = pci_resource_start(ndev->ntb.pdev, bar) + offset;
> @@ -557,8 +561,13 @@ static int ntb_epf_init_dev(struct ntb_epf_dev *ndev)
>  	}
>
>  	ndev->db_valid_mask = BIT_ULL(ndev->db_count) - 1;
> -	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
>  	ndev->spad_count = readl(ndev->ctrl_reg + NTB_EPF_SPAD_COUNT);
> +	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
> +
> +	if (ndev->mw_count > NTB_EPF_MAX_MW_COUNT) {
> +		dev_err(dev, "Unsupported MW count: %u\n", ndev->mw_count);
> +		return -EINVAL;
> +	}
>
>  	return 0;
>  }
> @@ -596,14 +605,14 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>  		dev_warn(&pdev->dev, "Cannot DMA highmem\n");
>  	}
>
> -	ndev->ctrl_reg = pci_iomap(pdev, ndev->ctrl_reg_bar, 0);
> +	ndev->ctrl_reg = pci_iomap(pdev, ndev->barno[BAR_CONFIG], 0);
>  	if (!ndev->ctrl_reg) {
>  		ret = -EIO;
>  		goto err_pci_regions;
>  	}
>
> -	if (ndev->peer_spad_reg_bar) {
> -		ndev->peer_spad_reg = pci_iomap(pdev, ndev->peer_spad_reg_bar, 0);
> +	if (ndev->barno[BAR_PEER_SPAD] != ndev->barno[BAR_CONFIG]) {
> +		ndev->peer_spad_reg = pci_iomap(pdev, ndev->barno[BAR_PEER_SPAD], 0);
>  		if (!ndev->peer_spad_reg) {
>  			ret = -EIO;
>  			goto err_pci_regions;
> @@ -614,7 +623,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>  		ndev->peer_spad_reg = ndev->ctrl_reg + spad_off  + spad_sz;
>  	}
>
> -	ndev->db_reg = pci_iomap(pdev, ndev->db_reg_bar, 0);
> +	ndev->db_reg = pci_iomap(pdev, ndev->barno[BAR_DB], 0);
>  	if (!ndev->db_reg) {
>  		ret = -EIO;
>  		goto err_pci_regions;
> @@ -656,15 +665,20 @@ static void ntb_epf_cleanup_isr(struct ntb_epf_dev *ndev)
>  	pci_free_irq_vectors(pdev);
>  }
>
> +static const enum pci_barno ntb_epf_default_barno[NTB_BAR_NUM] = {
> +	[BAR_CONFIG]	= BAR_0,
> +	[BAR_PEER_SPAD]	= BAR_1,
> +	[BAR_DB]	= BAR_2,
> +	[BAR_MW1]	= BAR_2,
> +	[BAR_MW2]	= BAR_3,
> +	[BAR_MW3]	= BAR_4,
> +	[BAR_MW4]	= BAR_5
> +};
> +
>  static int ntb_epf_pci_probe(struct pci_dev *pdev,
>  			     const struct pci_device_id *id)
>  {
> -	enum pci_barno peer_spad_reg_bar = BAR_1;
> -	enum pci_barno ctrl_reg_bar = BAR_0;
> -	enum pci_barno db_reg_bar = BAR_2;
> -	enum pci_barno mw_bar = BAR_2;
>  	struct device *dev = &pdev->dev;
> -	struct ntb_epf_data *data;
>  	struct ntb_epf_dev *ndev;
>  	int ret;
>
> @@ -675,18 +689,10 @@ static int ntb_epf_pci_probe(struct pci_dev *pdev,
>  	if (!ndev)
>  		return -ENOMEM;
>
> -	data = (struct ntb_epf_data *)id->driver_data;
> -	if (data) {
> -		peer_spad_reg_bar = data->peer_spad_reg_bar;
> -		ctrl_reg_bar = data->ctrl_reg_bar;
> -		db_reg_bar = data->db_reg_bar;
> -		mw_bar = data->mw_bar;
> -	}
> +	ndev->barno = (const enum pci_barno *)id->driver_data;
> +	if (!ndev->barno)
> +		ndev->barno = ntb_epf_default_barno;

I think needn't check it because all .driver_data already set in ntb_epf_pci_tbl

Frank
>
> -	ndev->peer_spad_reg_bar = peer_spad_reg_bar;
> -	ndev->ctrl_reg_bar = ctrl_reg_bar;
> -	ndev->db_reg_bar = db_reg_bar;
> -	ndev->mw_bar = mw_bar;
>  	ndev->dev = dev;
>
>  	ntb_epf_init_struct(ndev, pdev);
> @@ -730,30 +736,26 @@ static void ntb_epf_pci_remove(struct pci_dev *pdev)
>  	ntb_epf_deinit_pci(ndev);
>  }
>
> -static const struct ntb_epf_data j721e_data = {
> -	.ctrl_reg_bar = BAR_0,
> -	.peer_spad_reg_bar = BAR_1,
> -	.db_reg_bar = BAR_2,
> -	.mw_bar = BAR_2,
> -};
> -
> -static const struct ntb_epf_data mx8_data = {
> -	.ctrl_reg_bar = BAR_0,
> -	.peer_spad_reg_bar = BAR_0,
> -	.db_reg_bar = BAR_2,
> -	.mw_bar = BAR_4,
> +static const enum pci_barno mx8_barno[NTB_BAR_NUM] = {
> +	[BAR_CONFIG]	= BAR_0,
> +	[BAR_PEER_SPAD]	= BAR_0,
> +	[BAR_DB]	= BAR_2,
> +	[BAR_MW1]	= BAR_4,
> +	[BAR_MW2]	= BAR_5,
> +	[BAR_MW3]	= NO_BAR,
> +	[BAR_MW4]	= NO_BAR,
>  };
>
>  static const struct pci_device_id ntb_epf_pci_tbl[] = {
>  	{
>  		PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
>  		.class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
> -		.driver_data = (kernel_ulong_t)&j721e_data,
> +		.driver_data = (kernel_ulong_t)ntb_epf_default_barno,
>  	},
>  	{
>  		PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x0809),
>  		.class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
> -		.driver_data = (kernel_ulong_t)&mx8_data,
> +		.driver_data = (kernel_ulong_t)mx8_barno,
>  	},
>  	{ },
>  };
>
> --
> 2.47.2
>

Re: [PATCH 4/4] NTB: epf: Allow arbitrary BAR mapping

[Jerome Brunet]

On Mon 19 May 2025 at 18:44, Frank Li <Frank.li@nxp.com> wrote:

> On Mon, May 05, 2025 at 07:41:50PM +0200, Jerome Brunet wrote:
>> The NTB epf host driver assumes the BAR number associated with a memory
>> window is just incremented from the BAR number associated with MW1. This
>> seems to have been enough so far but this is not really how the endpoint
>> side work and the two could easily become mis-aligned.
>>
>> ntb_epf_mw_to_bar() even assumes that the BAR number is the memory window
>> index + 2, which means the function only returns a proper result if BAR_2
>> is associated with MW1.
>>
>> Instead, fully describe and allow arbitrary NTB BAR mapping.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/ntb/hw/epf/ntb_hw_epf.c | 108 ++++++++++++++++++++--------------------
>>  1 file changed, 55 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
>> index 00f0e78f685bf7917b02dd8a52b5b35f68d5bb64..9539cdcd0f8fa4b5c5e66477672f8f97d5ec4e52 100644
>> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
>> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
>> @@ -49,6 +49,7 @@
>>  #define NTB_EPF_COMMAND_TIMEOUT	1000 /* 1 Sec */
>>
>>  enum pci_barno {
>> +	NO_BAR = -1,
>
> Not related with this patch, but there are too many place to define
> enum pci_barno. it need be consolidate.

I agree it needs to consilidated at some point but that's another topic
and there are tiny differences between the 3 definitions so it won't be
as trivial as one might initially think

>
>>  	BAR_0,
>>  	BAR_1,
>>  	BAR_2,
>> @@ -57,16 +58,26 @@ enum pci_barno {
>>  	BAR_5,
>>  };
>>
>> +enum epf_ntb_bar {
>> +	BAR_CONFIG,
>> +	BAR_PEER_SPAD,
>> +	BAR_DB,
>> +	BAR_MW1,
>> +	BAR_MW2,
>> +	BAR_MW3,
>> +	BAR_MW4,
>> +	NTB_BAR_NUM,
>> +};
>> +
>> +#define NTB_EPF_MAX_MW_COUNT	(NTB_BAR_NUM - BAR_MW1)
>> +
>>  struct ntb_epf_dev {
>>  	struct ntb_dev ntb;
>>  	struct device *dev;
>>  	/* Mutex to protect providing commands to NTB EPF */
>>  	struct mutex cmd_lock;
>>
>> -	enum pci_barno ctrl_reg_bar;
>> -	enum pci_barno peer_spad_reg_bar;
>> -	enum pci_barno db_reg_bar;
>> -	enum pci_barno mw_bar;
>> +	const enum pci_barno *barno;
>
> barno_map?

ok

>
>>
>>  	unsigned int mw_count;
>>  	unsigned int spad_count;
>> @@ -85,17 +96,6 @@ struct ntb_epf_dev {
>>
>>  #define ntb_ndev(__ntb) container_of(__ntb, struct ntb_epf_dev, ntb)
>>
>> -struct ntb_epf_data {
>> -	/* BAR that contains both control region and self spad region */
>> -	enum pci_barno ctrl_reg_bar;
>> -	/* BAR that contains peer spad region */
>> -	enum pci_barno peer_spad_reg_bar;
>> -	/* BAR that contains Doorbell region and Memory window '1' */
>> -	enum pci_barno db_reg_bar;
>> -	/* BAR that contains memory windows*/
>> -	enum pci_barno mw_bar;
>> -};
>> -
>>  static int ntb_epf_send_command(struct ntb_epf_dev *ndev, u32 command,
>>  				u32 argument)
>>  {
>> @@ -144,7 +144,7 @@ static int ntb_epf_mw_to_bar(struct ntb_epf_dev *ndev, int idx)
>>  		return -EINVAL;
>>  	}
>>
>> -	return idx + 2;
>> +	return ndev->barno[BAR_MW1 + idx];
>>  }
>>
>>  static int ntb_epf_mw_count(struct ntb_dev *ntb, int pidx)
>> @@ -413,7 +413,9 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
>>  		return -EINVAL;
>>  	}
>>
>> -	bar = idx + ndev->mw_bar;
>> +	bar = ntb_epf_mw_to_bar(ndev, idx);
>> +	if (bar < 0)
>> +		return bar;
>>
>>  	mw_size = pci_resource_len(ntb->pdev, bar);
>>
>> @@ -455,7 +457,9 @@ static int ntb_epf_peer_mw_get_addr(struct ntb_dev *ntb, int idx,
>>  	if (idx == 0)
>>  		offset = readl(ndev->ctrl_reg + NTB_EPF_MW1_OFFSET);
>>
>> -	bar = idx + ndev->mw_bar;
>> +	bar = ntb_epf_mw_to_bar(ndev, idx);
>> +	if (bar < 0)
>> +		return bar;
>>
>>  	if (base)
>>  		*base = pci_resource_start(ndev->ntb.pdev, bar) + offset;
>> @@ -557,8 +561,13 @@ static int ntb_epf_init_dev(struct ntb_epf_dev *ndev)
>>  	}
>>
>>  	ndev->db_valid_mask = BIT_ULL(ndev->db_count) - 1;
>> -	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
>>  	ndev->spad_count = readl(ndev->ctrl_reg + NTB_EPF_SPAD_COUNT);
>> +	ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
>> +
>> +	if (ndev->mw_count > NTB_EPF_MAX_MW_COUNT) {
>> +		dev_err(dev, "Unsupported MW count: %u\n", ndev->mw_count);
>> +		return -EINVAL;
>> +	}
>>
>>  	return 0;
>>  }
>> @@ -596,14 +605,14 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>>  		dev_warn(&pdev->dev, "Cannot DMA highmem\n");
>>  	}
>>
>> -	ndev->ctrl_reg = pci_iomap(pdev, ndev->ctrl_reg_bar, 0);
>> +	ndev->ctrl_reg = pci_iomap(pdev, ndev->barno[BAR_CONFIG], 0);
>>  	if (!ndev->ctrl_reg) {
>>  		ret = -EIO;
>>  		goto err_pci_regions;
>>  	}
>>
>> -	if (ndev->peer_spad_reg_bar) {
>> -		ndev->peer_spad_reg = pci_iomap(pdev, ndev->peer_spad_reg_bar, 0);
>> +	if (ndev->barno[BAR_PEER_SPAD] != ndev->barno[BAR_CONFIG]) {
>> +		ndev->peer_spad_reg = pci_iomap(pdev, ndev->barno[BAR_PEER_SPAD], 0);
>>  		if (!ndev->peer_spad_reg) {
>>  			ret = -EIO;
>>  			goto err_pci_regions;
>> @@ -614,7 +623,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>>  		ndev->peer_spad_reg = ndev->ctrl_reg + spad_off  + spad_sz;
>>  	}
>>
>> -	ndev->db_reg = pci_iomap(pdev, ndev->db_reg_bar, 0);
>> +	ndev->db_reg = pci_iomap(pdev, ndev->barno[BAR_DB], 0);
>>  	if (!ndev->db_reg) {
>>  		ret = -EIO;
>>  		goto err_pci_regions;
>> @@ -656,15 +665,20 @@ static void ntb_epf_cleanup_isr(struct ntb_epf_dev *ndev)
>>  	pci_free_irq_vectors(pdev);
>>  }
>>
>> +static const enum pci_barno ntb_epf_default_barno[NTB_BAR_NUM] = {
>> +	[BAR_CONFIG]	= BAR_0,
>> +	[BAR_PEER_SPAD]	= BAR_1,
>> +	[BAR_DB]	= BAR_2,
>> +	[BAR_MW1]	= BAR_2,
>> +	[BAR_MW2]	= BAR_3,
>> +	[BAR_MW3]	= BAR_4,
>> +	[BAR_MW4]	= BAR_5
>> +};
>> +
>>  static int ntb_epf_pci_probe(struct pci_dev *pdev,
>>  			     const struct pci_device_id *id)
>>  {
>> -	enum pci_barno peer_spad_reg_bar = BAR_1;
>> -	enum pci_barno ctrl_reg_bar = BAR_0;
>> -	enum pci_barno db_reg_bar = BAR_2;
>> -	enum pci_barno mw_bar = BAR_2;
>>  	struct device *dev = &pdev->dev;
>> -	struct ntb_epf_data *data;
>>  	struct ntb_epf_dev *ndev;
>>  	int ret;
>>
>> @@ -675,18 +689,10 @@ static int ntb_epf_pci_probe(struct pci_dev *pdev,
>>  	if (!ndev)
>>  		return -ENOMEM;
>>
>> -	data = (struct ntb_epf_data *)id->driver_data;
>> -	if (data) {
>> -		peer_spad_reg_bar = data->peer_spad_reg_bar;
>> -		ctrl_reg_bar = data->ctrl_reg_bar;
>> -		db_reg_bar = data->db_reg_bar;
>> -		mw_bar = data->mw_bar;
>> -	}
>> +	ndev->barno = (const enum pci_barno *)id->driver_data;
>> +	if (!ndev->barno)
>> +		ndev->barno = ntb_epf_default_barno;
>
> I think needn't check it because all .driver_data already set in ntb_epf_pci_tbl
>

A check was there before, I'm not changing what was done in that regard.
I'll another patch to implement your suggestion seperately.

> Frank
>>
>> -	ndev->peer_spad_reg_bar = peer_spad_reg_bar;
>> -	ndev->ctrl_reg_bar = ctrl_reg_bar;
>> -	ndev->db_reg_bar = db_reg_bar;
>> -	ndev->mw_bar = mw_bar;
>>  	ndev->dev = dev;
>>
>>  	ntb_epf_init_struct(ndev, pdev);
>> @@ -730,30 +736,26 @@ static void ntb_epf_pci_remove(struct pci_dev *pdev)
>>  	ntb_epf_deinit_pci(ndev);
>>  }
>>
>> -static const struct ntb_epf_data j721e_data = {
>> -	.ctrl_reg_bar = BAR_0,
>> -	.peer_spad_reg_bar = BAR_1,
>> -	.db_reg_bar = BAR_2,
>> -	.mw_bar = BAR_2,
>> -};
>> -
>> -static const struct ntb_epf_data mx8_data = {
>> -	.ctrl_reg_bar = BAR_0,
>> -	.peer_spad_reg_bar = BAR_0,
>> -	.db_reg_bar = BAR_2,
>> -	.mw_bar = BAR_4,
>> +static const enum pci_barno mx8_barno[NTB_BAR_NUM] = {
>> +	[BAR_CONFIG]	= BAR_0,
>> +	[BAR_PEER_SPAD]	= BAR_0,
>> +	[BAR_DB]	= BAR_2,
>> +	[BAR_MW1]	= BAR_4,
>> +	[BAR_MW2]	= BAR_5,
>> +	[BAR_MW3]	= NO_BAR,
>> +	[BAR_MW4]	= NO_BAR,
>>  };
>>
>>  static const struct pci_device_id ntb_epf_pci_tbl[] = {
>>  	{
>>  		PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
>>  		.class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
>> -		.driver_data = (kernel_ulong_t)&j721e_data,
>> +		.driver_data = (kernel_ulong_t)ntb_epf_default_barno,
>>  	},
>>  	{
>>  		PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x0809),
>>  		.class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
>> -		.driver_data = (kernel_ulong_t)&mx8_data,
>> +		.driver_data = (kernel_ulong_t)mx8_barno,
>>  	},
>>  	{ },
>>  };
>>
>> --
>> 2.47.2
>>

-- 
Jerome