| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aCuwy5ZjkhAiCPoZ@lizhi-Precision-Tower-5810>
Date: Mon, 19 May 2025 18:29:31 -0400
From: Frank Li <Frank.li@....com>
To: Jerome Brunet <jbrunet@...libre.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, 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@...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
>
> 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
>
Powered by blists - more mailing lists