Message-ID: <Zp+6TU/nn/Ea6xqq@x1-carbon.lan>
Date: Tue, 23 Jul 2024 16:17:32 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Rick Wertenbroek <rick.wertenbroek@...il.com>
Cc: rick.wertenbroek@...g-vd.ch, alberto.dassatti@...g-vd.ch,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Frank Li <Frank.Li@....com>,
	Damien Le Moal <dlemoal@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed
 address BARs in EPC

On Fri, Jul 19, 2024 at 01:57:38PM +0200, Rick Wertenbroek wrote:
> The current mechanism for BARs is as follows: The endpoint function
> allocates memory with 'pci_epf_alloc_space' which calls
> 'dma_alloc_coherent' to allocate memory for the BAR and fills a
> 'pci_epf_bar' structure with the physical address, virtual address,
> size, BAR number and flags. This 'pci_epf_bar' structure is passed
> to the endpoint controller driver through 'set_bar'. The endpoint
> controller driver configures the actual endpoint to reroute PCI
> read/write TLPs to the BAR memory space allocated.
> 
> The problem with this is that not all PCI endpoint controllers can
> be configured to reroute read/write TLPs to their BAR to a given
> address in memory space. Some PCI endpoint controllers e.g., FPGA
> IPs for Intel/Altera and AMD/Xilinx PCI endpoints. These controllers
> come with pre-assigned memory for the BARs (e.g., in FPGA BRAM),
> because of this the endpoint controller driver has no way to tell
> these controllers to reroute the read/write TLPs to the memory
> allocated by 'pci_epf_alloc_space' and no way to get access to the
> memory pre-assigned to the BARs through the current API.

Looking at your series, it seems that you skip not only setting up the
PCI address to internal address translation, you also skip the whole
call to set_bar(). set_bar() takes a 'pci_epf_bar' struct, and configures
the hardware accordingly, that means setting the flags for the BARs,
configuring it as 32 or 64-bit etc.

I think you should still call set_bar(). Your PCIe EPC .set_bar() callback
can then detect that the type is fixed address, and skip setting up the
internal address translation. (Although I can imagine someone in the
future might need a fixed internal address for the BAR, but they still
need to setup internal address translation.)

Maybe something like this:
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 85bdf2adb760..50ad728b3b3e 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -151,18 +151,22 @@ struct pci_epc {
 /**
  * @BAR_PROGRAMMABLE: The BAR mask can be configured by the EPC.
  * @BAR_FIXED: The BAR mask is fixed by the hardware.
+ * @BAR_FIXED_ADDR: The BAR mask and physical address is fixed by the hardware.
  * @BAR_RESERVED: The BAR should not be touched by an EPF driver.
  */
 enum pci_epc_bar_type {
        BAR_PROGRAMMABLE = 0,
        BAR_FIXED,
+       BAR_FIXED_ADDR,
        BAR_RESERVED,
 };
 
 /**
  * struct pci_epc_bar_desc - hardware description for a BAR
  * @type: the type of the BAR
- * @fixed_size: the fixed size, only applicable if type is BAR_FIXED_MASK.
+ * @fixed_size: the fixed size, only applicable if type is BAR_FIXED or
+ *             BAR_FIXED_ADDRESS.
+ * @fixed_addr: the fixed address, only applicable if type is BAR_FIXED_ADDRESS.
  * @only_64bit: if true, an EPF driver is not allowed to choose if this BAR
  *             should be configured as 32-bit or 64-bit, the EPF driver must
  *             configure this BAR as 64-bit. Additionally, the BAR succeeding


I know you are using a FPGA, but for e.g. DWC, you would simply
ignore:
https://github.com/torvalds/linux/blob/master/drivers/pci/controller/dwc/pcie-designware-ep.c#L232-L234


Perhaps we even want the EPF drivers to keep calling pci_epf_alloc_space(),
by doing something like:

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 323f2a60ab16..35f7a9b68006 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -273,7 +273,9 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
        if (size < 128)
                size = 128;
 
-       if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
+       if ((epc_features->bar[bar].type == BAR_FIXED ||
+            epc_features->bar[bar].type == BAR_FIXED_ADDR)
+           && bar_fixed_size) {
                if (size > bar_fixed_size) {
                        dev_err(&epf->dev,
                                "requested BAR size is larger than fixed size\n");
@@ -296,10 +298,15 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
        }
 
        dev = epc->dev.parent;
-       space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
-       if (!space) {
-               dev_err(dev, "failed to allocate mem space\n");
-               return NULL;
+       if (epc_features->bar[bar].type == BAR_FIXED_ADDR) {
+               request_mem_region(...);
+               ioremap(...);
+       } else {
+               space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
+               if (!space) {
+                       dev_err(dev, "failed to allocate mem space\n");
+                       return NULL;
+               }
        }
 
        epf_bar[bar].phys_addr = phys_addr;



I could also see some logic in the request_mem_region() and ioremap() call
being in the EPC driver's set_bar() callback.

But like you suggested in the other mail, the right thing is to merge
alloc_space() and set_bar() anyway. (Basically instead of where EPF drivers
currently call set_bar(), the should call alloc_and_set_bar() (or whatever)
instead.)


Kind regards,
Niklas

Powered by blists - more mailing lists