On 7/19/24 20:57, 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.
>
> Therefore, introduce 'get_bar' which allows to get access to a BAR
> without calling 'pci_epf_alloc_space'. Controllers with pre-assigned
> bars can therefore implement 'get_bar' which will assign the BAR
> pyhsical address, virtual address through ioremap, size, and flags.
>
> PCI endpoint functions can query the endpoint controller through the
> 'fixed_addr' boolean in the 'pci_epc_bar_desc' structure. Similarly
> to the BAR type, fixed size or fixed 64-bit descriptions. With this
> information they can either call 'pci_epf_alloc_space' and 'set_bar'
> as is currently the case, or call the new 'get_bar'. Both will provide
> a working, memory mapped BAR, that can be used in the endpoint
> function.
>
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@xxxxxxxxx>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 37 +++++++++++++++++++++++++++++
> include/linux/pci-epc.h | 7 ++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 84309dfe0c68..fcef848876fe 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -544,6 +544,43 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> }
> EXPORT_SYMBOL_GPL(pci_epc_set_bar);
>
> +/**
> + * pci_epc_get_bar - get BAR configuration from a fixed address BAR
> + * @epc: the EPC device on which BAR resides
> + * @func_no: the physical endpoint function number in the EPC device
> + * @vfunc_no: the virtual endpoint function number in the physical function
> + * @bar: the BAR number to get
> + * @epf_bar: the struct epf_bar to fill
> + *
> + * Invoke to get the configuration of the endpoint device fixed address BAR
> + */
> +int pci_epc_get_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> + enum pci_barno bar, struct pci_epf_bar *epf_bar)
> +{
> + int ret;
> +
> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> + return -EINVAL;
> +
> + if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> + return -EINVAL;
> +
> + if (bar < 0 || bar >= PCI_STD_NUM_BARS)
> + return -EINVAL;
> +
> + if (!epc->ops->get_bar)
> + return -EINVAL;
> +
> + epf_bar->barno = bar;
> +
> + mutex_lock(&epc->lock);
> + ret = epc->ops->get_bar(epc, func_no, vfunc_no, bar, epf_bar);
> + mutex_unlock(&epc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_get_bar);
> +
> /**
> * pci_epc_write_header() - write standard configuration header
> * @epc: the EPC device to which the configuration header should be written
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 85bdf2adb760..a5ea50dd49ba 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -37,6 +37,7 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
> * @write_header: ops to populate configuration space header
> * @set_bar: ops to configure the BAR
> * @clear_bar: ops to reset the BAR
> + * @get_bar: ops to get a fixed address BAR that cannot be set/cleared
> * @map_addr: ops to map CPU address to PCI address
> * @unmap_addr: ops to unmap CPU address and PCI address
> * @set_msi: ops to set the requested number of MSI interrupts in the MSI
> @@ -61,6 +62,8 @@ struct pci_epc_ops {
> struct pci_epf_bar *epf_bar);
> void (*clear_bar)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> struct pci_epf_bar *epf_bar);
> + int (*get_bar)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> + enum pci_barno, struct pci_epf_bar *epf_bar);
> int (*map_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> phys_addr_t addr, u64 pci_addr, size_t size);
> void (*unmap_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> @@ -163,6 +166,7 @@ enum pci_epc_bar_type {
> * 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_addr: indicates that the BAR has a fixed address in memory map.
> * @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
> @@ -176,6 +180,7 @@ enum pci_epc_bar_type {
> struct pci_epc_bar_desc {
> enum pci_epc_bar_type type;
> u64 fixed_size;
> + bool fixed_addr;
Why make this a bool instead of a 64-bits address ?
If the controller sets this to a non-zero value, we will know it is a fixed
address bar. And that can avoid adding the get_bar operations, no ?
> bool only_64bit;
> };
>
> @@ -238,6 +243,8 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> struct pci_epf_bar *epf_bar);
> void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> struct pci_epf_bar *epf_bar);
> +int pci_epc_get_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> + enum pci_barno, struct pci_epf_bar *epf_bar);
> int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> phys_addr_t phys_addr,
> u64 pci_addr, size_t size);
--
Damien Le Moal
Western Digital Research
