Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed address BARs in EPC

[Damien Le Moal <dlemoal@kernel.org>]

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@gmail.com>
> ---
>  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