Message-ID: <1j8qmrn1tg.fsf@starbuckisacylon.baylibre.com>
Date: Tue, 20 May 2025 10:06:35 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Frank Li <Frank.li@....com>
Cc: Jon Mason <jdmason@...zu.us>,  Dave Jiang <dave.jiang@...el.com>,  Allen
 Hubbe <allenbh@...il.com>,  Manivannan Sadhasivam
 <manivannan.sadhasivam@...aro.org>,  Krzysztof Wilczyński
 <kw@...ux.com>,
  Kishon Vijay Abraham I <kishon@...nel.org>,  Bjorn Helgaas
 <bhelgaas@...gle.com>,  ntb@...ts.linux.dev,  linux-pci@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] PCI: endpoint: pci-epf-vntb: allow arbitrary BAR
 configuration

On Mon 19 May 2025 at 18:29, Frank Li <Frank.li@....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@...libre.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

Powered by blists - more mailing lists