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

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(+)

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);

On Tue, Jul 23, 2024 at 2:03 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> 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 ?
>

The reason to use a bool is to force the use of 'get_bar', get_bar will fill
the 'pci_epf_bar' structure and memory map the BAR. This ensures the
'pci_epf_bar' structure is filled correctly and usable, same as after a
'pci_epf_alloc_space' operation. This also removes a burden to the
endpoint function (i.e., map the memory, handle errors, set the fields
of the structure etc.). This will likely avoid errors in the endpoint functions
as this code is quite sensitive and possibly controller specific (e.g.,
memremap for virtual controllers vs ioremap for real controllers). Also,
this code would be duplicated for each endpoint function, therefore I think
it is better to just call 'get_bar' instead of rewriting all corresponding lines
in each endpoint function (which would be very error prone).

There could also be other cases where the PCIe controller is behind a
specific bus and the BAR doesn't have a physical address and needs
to be accessed in a specific way. E.g., one could make a BAR accessible
via a serial interface in the FPGA (probably no one will do this, but it is
a possible architecture).

That's why I believe it is important to let the controller fill the
'pci_epf_bar'
structure and do the necessary io/mem remapping internally.

> >       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
>

Thank you for your insights.
Rick

On 7/23/24 16:06, Rick Wertenbroek wrote:
> On Tue, Jul 23, 2024 at 2:03 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> 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 ?
>>
> 
> The reason to use a bool is to force the use of 'get_bar', get_bar will fill
> the 'pci_epf_bar' structure and memory map the BAR. This ensures the
> 'pci_epf_bar' structure is filled correctly and usable, same as after a
> 'pci_epf_alloc_space' operation. This also removes a burden to the
> endpoint function (i.e., map the memory, handle errors, set the fields
> of the structure etc.). This will likely avoid errors in the endpoint functions
> as this code is quite sensitive and possibly controller specific (e.g.,
> memremap for virtual controllers vs ioremap for real controllers). Also,
> this code would be duplicated for each endpoint function, therefore I think
> it is better to just call 'get_bar' instead of rewriting all corresponding lines
> in each endpoint function (which would be very error prone).
> 
> There could also be other cases where the PCIe controller is behind a
> specific bus and the BAR doesn't have a physical address and needs
> to be accessed in a specific way. E.g., one could make a BAR accessible
> via a serial interface in the FPGA (probably no one will do this, but it is
> a possible architecture).
> 
> That's why I believe it is important to let the controller fill the
> 'pci_epf_bar'
> structure and do the necessary io/mem remapping internally.

OK. All fair points. I asked because I am not a fan of the code we end up
needing in the epf, such as you have in the test driver changes in patch 2:

+	if (!epc_features->bar[test_reg_bar].fixed_addr)
+		base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
+					   epc_features, PRIMARY_INTERFACE);
+	else {
+		ret = pci_epc_get_bar(epf->epc, epf->func_no, epf->vfunc_no,
+				      test_reg_bar, &epf->bar[test_reg_bar]);
+		if (ret < 0) {
+			dev_err(dev, "Failed to get fixed address BAR\n");
+			return ret;
+		}
+		base = epf->bar[test_reg_bar].addr;
+	}
+

It would be a lot nicer if we could have a single epf function that does the
alloc space call OR the get bar called based on the type of the bar.

I was thinking of something like:

	base = pci_epf_alloc_bar(epf, &epf->bar[test_reg_bar], test_reg_size,
				 PRIMARY_INTERFACE);

(we do not need to pass the epc_features as we can get that through epf->epc)

That would greatly simplify the epf driver code. And of course we need the
reverse pci_epf_free_bar() which would call either pci_epf_free_space() or
pci_epc_release_bar() for fixed address bars. This last function is needed
either way I think so that we can have a clean teardown of the epc resources
used for an epf.

> 
>>>       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
>>
> 
> Thank you for your insights.
> Rick

On Tue, Jul 23, 2024 at 9:16 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 7/23/24 16:06, Rick Wertenbroek wrote:
> > On Tue, Jul 23, 2024 at 2:03 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> >>
> >> 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 ?
> >>
> >
> > The reason to use a bool is to force the use of 'get_bar', get_bar will fill
> > the 'pci_epf_bar' structure and memory map the BAR. This ensures the
> > 'pci_epf_bar' structure is filled correctly and usable, same as after a
> > 'pci_epf_alloc_space' operation. This also removes a burden to the
> > endpoint function (i.e., map the memory, handle errors, set the fields
> > of the structure etc.). This will likely avoid errors in the endpoint functions
> > as this code is quite sensitive and possibly controller specific (e.g.,
> > memremap for virtual controllers vs ioremap for real controllers). Also,
> > this code would be duplicated for each endpoint function, therefore I think
> > it is better to just call 'get_bar' instead of rewriting all corresponding lines
> > in each endpoint function (which would be very error prone).
> >
> > There could also be other cases where the PCIe controller is behind a
> > specific bus and the BAR doesn't have a physical address and needs
> > to be accessed in a specific way. E.g., one could make a BAR accessible
> > via a serial interface in the FPGA (probably no one will do this, but it is
> > a possible architecture).
> >
> > That's why I believe it is important to let the controller fill the
> > 'pci_epf_bar'
> > structure and do the necessary io/mem remapping internally.
>
> OK. All fair points. I asked because I am not a fan of the code we end up
> needing in the epf, such as you have in the test driver changes in patch 2:
>
> +       if (!epc_features->bar[test_reg_bar].fixed_addr)
> +               base = pci_epf_alloc_space(epf, test_reg_size, test_reg_bar,
> +                                          epc_features, PRIMARY_INTERFACE);
> +       else {
> +               ret = pci_epc_get_bar(epf->epc, epf->func_no, epf->vfunc_no,
> +                                     test_reg_bar, &epf->bar[test_reg_bar]);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to get fixed address BAR\n");
> +                       return ret;
> +               }
> +               base = epf->bar[test_reg_bar].addr;
> +       }
> +
>
> It would be a lot nicer if we could have a single epf function that does the
> alloc space call OR the get bar called based on the type of the bar.
>
> I was thinking of something like:
>
>         base = pci_epf_alloc_bar(epf, &epf->bar[test_reg_bar], test_reg_size,
>                                  PRIMARY_INTERFACE);
>
> (we do not need to pass the epc_features as we can get that through epf->epc)
>
> That would greatly simplify the epf driver code. And of course we need the
> reverse pci_epf_free_bar() which would call either pci_epf_free_space() or
> pci_epc_release_bar() for fixed address bars. This last function is needed
> either way I think so that we can have a clean teardown of the epc resources
> used for an epf.
>

Interesting. I like this approach, this would also make it possible to
combine the current 'pci_epf_alloc_space' and 'pci_epc_set_bar' into a
single function which would simplify the endpoint functions.

The reason why it is separate now is so that 'pci_epf_alloc_space' can
be called before the endpoint function is bound to a controller, and
therefore the BAR memory can be filled and prepared before bind() is
called. Merging 'pci_epf_alloc_bar' with 'pci_epc_set_bar' into a
'pci_epc_alloc_bar' (epc because it is dependent on the endpoint
controller and prior to be bound to a specific controller we don't
know if we need to allocate locally or remap) would make it impossible
to access the BAR prior to be bound to a controller (because the
function 'pci_epc_alloc_bar' would fail without a specific
controller). I don't think this is a problem as an unbound endpoint
function is kind of useless on its own, but this means the BAR memory
could only be allocated/remapped/accessed once the endpoint is bound
to a controller.

This would also mean that unbinding an endpoint function from
controller X and rebinding it to controller Y would require refilling
the whole BAR memory (so if the current state of the BAR memory is
important it needs to be saved and restored). While this is a scenario
we will probably not see often, it is impacted by the change, and with
the current architecture one could rebind the function to another
controller and the BAR contents would remain in their current state.
(Note: to rebind, remove symlink in PCI endpoint controller X
configfs, and symlink it in another controller Y configfs).

Unless someone relies on unbinding/rebinding (unlikely), I think
'pci_epc_alloc_bar' could be introduced without breaking anything and
would simplify the endpoint functions as well (remove duplicate checks
that are currently in the endpoint bind and epc core init functions,
because both check epc features for the 'pci_epf_alloc_space' and for
'pci_epc_set_bar'.). And for the future in the unlikely case where
someone wants to implement an endpoint function that should be
unbound-rebound while preserving the BAR state, they can add the BAR
memory save/restore mechanism.

So, I totally agree it would be better to rely on a single
'pci_epc_alloc_bar' function.

> >
> >>>       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
> >>
> >
> > Thank you for your insights.
> > Rick
>
> --
> Damien Le Moal
> Western Digital Research
>

Regards,
Rick

On Tue, Jul 23, 2024 at 09:41:14AM +0200, Rick Wertenbroek wrote:
> 
> Interesting. I like this approach, this would also make it possible to
> combine the current 'pci_epf_alloc_space' and 'pci_epc_set_bar' into a
> single function which would simplify the endpoint functions.
> 
> The reason why it is separate now is so that 'pci_epf_alloc_space' can
> be called before the endpoint function is bound to a controller, and
> therefore the BAR memory can be filled and prepared before bind() is
> called. Merging 'pci_epf_alloc_bar' with 'pci_epc_set_bar' into a
> 'pci_epc_alloc_bar' (epc because it is dependent on the endpoint
> controller and prior to be bound to a specific controller we don't
> know if we need to allocate locally or remap) would make it impossible
> to access the BAR prior to be bound to a controller (because the
> function 'pci_epc_alloc_bar' would fail without a specific
> controller). I don't think this is a problem as an unbound endpoint
> function is kind of useless on its own, but this means the BAR memory
> could only be allocated/remapped/accessed once the endpoint is bound
> to a controller.

Back when I was looking it to this, I came to the conclusion:
"The proper place is to allocate them when receiving PERST,
but not all drivers have a PERST handler."

However, since:
https://github.com/torvalds/linux/commit/a01e7214bef904723d26d293eb17586078610379

All EPF drivers should get a epc_init event, either directly when the EPC
driver is loaded, or when PERST# is asserted.

So I definitely agree that it makes sense for all EPF function drivers to do
both alloc_space() + set_bar() is a single call, now when that is possible.

This will simplify a lot of PCIe endpoint code considerably.


Kind regards,
Niklas

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

On Tue, Jul 23, 2024 at 4:17 PM Niklas Cassel <cassel@kernel.org> wrote:
>
> 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.

Thank you for the comments,

This is not skipped, it is done in the new 'get_bar()' function,
depending on the hardware the flags are either fixed, then they are
set inside the 'get_bar()' function, either they are settable and we
could use the ones that come from the 'pci_epf_bar' structure as an
"in-out" parameter. This is dependent on the controller and will be
set in 'get_bar'. The structure returned by 'get_bar' will be filled.
It also means get_bar() will call ioremap() to set the virtual
address, as well as the physical address.

>
> 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.)

That is why I suggested at first to have either the option to use
'set_bar' (translate TLPs to the BAR address to the phys address from
the pci_epf_bar struct) or 'get_bar' (because the translation of TLPs
to BAR is fixed by hardware, and you want to fill the pci_epf_bar
struct with the correct addresses), having both allows to choose the
one adequate for the controller hardware based on features.

>
> 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
>

Yes, this is very similar to what I suggested initially, with the enum
type instead of a boolean, and we need the address for
pci_epf_alloc_space to do the ioremap, which is not needed if done in
pci_epc_get_bar because the EPC itself knows about the fixed address.

>
> 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
>

Yes, exactly. But that needs your change suggested below (if not the
caller should not call 'pci_epf_alloc_space' before calling
'pci_epc_set_bar' and 'pci_epc_set_bar' should still ioremap the fixed
physical address to provide to get the virtual address and provide
both in the 'pci_epf_bar' struct (which should not be pre-filled by
pci_epf_alloc_space)).

>
> 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;
>
>

This seems like a sane approach, I thought that because
'pci_epf_alloc_space' was in the EPF part, the EPF may not have been
linked yet to a controller, but here as the features are passed, they
EPF section already knows about the EPC features, so it makes sense to
to do the ioremap() here instead of in 'set/get_bar()'. This would
also be compatible with the current API.

I really like this because it doesn't require a new function. Also it
will not alloc/fill if it is a fixed BAR so less risk of errors when
writing the endpoint function driver.

And I like passing the BAR fixed address in the features, it makes sense.

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

My initial thought was that because it was really EPC dependent it
would be in an EPC function, thus I suggested get_bar.

>
> 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.)
>

Yes, if we merge both, the code will need to be in the EPC code
(because of the set_bar), and then the pci_epf_alloc_space (if needed)
would be called internally in the EPC code and not in the endpoint
function code.

The only downside, as I said in my other mail, is the very niche case
where the contents of a BAR should be moved and remain unchanged when
rebinding a given endpoint function from one controller to another.
But this is not expected in any endpoint function currently, and with
the new changes, the endpoint could simply copy the BAR contents to a
local buffer and then set the contents in the BAR of the new
controller.
Anyways, probably no one is moving live functions between controllers,
and if needed it still can be done, so no problem here...

>
> Kind regards,
> Niklas

Thank you very much for your insights.
I really like the approach of setting the type and fixed address in
the features.

By doing so we can then merge the alloc/set_bar functions and simplify
the endpoint function drivers while at the same time support fixed
address BARs.

Best regards,
Rick

Hello Rick,

On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> >
> > 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.)
> >
> 
> Yes, if we merge both, the code will need to be in the EPC code
> (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> would be called internally in the EPC code and not in the endpoint
> function code.
> 
> The only downside, as I said in my other mail, is the very niche case
> where the contents of a BAR should be moved and remain unchanged when
> rebinding a given endpoint function from one controller to another.
> But this is not expected in any endpoint function currently, and with
> the new changes, the endpoint could simply copy the BAR contents to a
> local buffer and then set the contents in the BAR of the new
> controller.
> Anyways, probably no one is moving live functions between controllers,
> and if needed it still can be done, so no problem here...

I think we need to wait for Mani's opinion, but I've never heard of anyone
doing so, and I agree with your suggested feature to copy the BAR contents
in case anyone actually changes the backing EPC controller to an EPF.
(You would need to stop the EPC to unbind + bind anyway, IIRC.)

Converting pci-epf-test to a new alloc_and_set_bar() API/helper which is
done at .epc_init()-time instead of at .bind()-time shouldn't be too hard.

pci-epf-ntb and pci-epf-vntb looks a lot less trivial, but perhaps it is
okay to convert pci-epf-test, and let the other more complex EPF drivers
continue to use the lower-level APIs.


Kind regards,
Niklas

On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
> Hello Rick,
> 
> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> > >
> > > 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.)
> > >
> > 
> > Yes, if we merge both, the code will need to be in the EPC code
> > (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> > would be called internally in the EPC code and not in the endpoint
> > function code.
> > 
> > The only downside, as I said in my other mail, is the very niche case
> > where the contents of a BAR should be moved and remain unchanged when
> > rebinding a given endpoint function from one controller to another.
> > But this is not expected in any endpoint function currently, and with
> > the new changes, the endpoint could simply copy the BAR contents to a
> > local buffer and then set the contents in the BAR of the new
> > controller.
> > Anyways, probably no one is moving live functions between controllers,
> > and if needed it still can be done, so no problem here...
> 
> I think we need to wait for Mani's opinion, but I've never heard of anyone
> doing so, and I agree with your suggested feature to copy the BAR contents
> in case anyone actually changes the backing EPC controller to an EPF.
> (You would need to stop the EPC to unbind + bind anyway, IIRC.)
> 

Hi Rick/Niklas,

TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
we do not need to worry until the actual requirement comes.

But I really like combining alloc() and set_bar() APIs. Something I wanted to do
for so long but never got around to it. We can use the API name something like
pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
have it to align with existing APIs.

And regarding the implementation, the use of fixed address for BAR is not new.
If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
location that is derived from the controller DT node (MMIO region).

But I was thinking of moving this region to EPF node once we add DT support for
EPF driver. Because, there can be many EPFs in a single endpoint and each can
have upto 6 BARs. We cannot really describe each resource in the controller DT
node.

Given that you really have a usecase now for multiple BARs, I think it is best
if we can add DT support for the EPF drivers and describe the BAR resources in
each EPF node. With that, we can hide all the resource allocation within the EPC
core without exposing any flag to the EPF drivers.

So if the EPF node has a fixed location for BAR and defined in DT, then the new
API pci_epf_alloc_set_bar() API will use that address and configure it
accordingly. If not, it will just call pci_epf_alloc_space() internally to
allocate the memory from coherent region and use it.

Wdyt?

- Mani

On 7/25/24 14:33, Manivannan Sadhasivam wrote:
> On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
>> Hello Rick,
>>
>> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
>>>>
>>>> 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.)
>>>>
>>>
>>> Yes, if we merge both, the code will need to be in the EPC code
>>> (because of the set_bar), and then the pci_epf_alloc_space (if needed)
>>> would be called internally in the EPC code and not in the endpoint
>>> function code.
>>>
>>> The only downside, as I said in my other mail, is the very niche case
>>> where the contents of a BAR should be moved and remain unchanged when
>>> rebinding a given endpoint function from one controller to another.
>>> But this is not expected in any endpoint function currently, and with
>>> the new changes, the endpoint could simply copy the BAR contents to a
>>> local buffer and then set the contents in the BAR of the new
>>> controller.
>>> Anyways, probably no one is moving live functions between controllers,
>>> and if needed it still can be done, so no problem here...
>>
>> I think we need to wait for Mani's opinion, but I've never heard of anyone
>> doing so, and I agree with your suggested feature to copy the BAR contents
>> in case anyone actually changes the backing EPC controller to an EPF.
>> (You would need to stop the EPC to unbind + bind anyway, IIRC.)
>>
> 
> Hi Rick/Niklas,
> 
> TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
> we do not need to worry until the actual requirement comes.
> 
> But I really like combining alloc() and set_bar() APIs. Something I wanted to do
> for so long but never got around to it. We can use the API name something like
> pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
> have it to align with existing APIs.
> 
> And regarding the implementation, the use of fixed address for BAR is not new.
> If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
> location that is derived from the controller DT node (MMIO region).
> 
> But I was thinking of moving this region to EPF node once we add DT support for
> EPF driver. Because, there can be many EPFs in a single endpoint and each can
> have upto 6 BARs. We cannot really describe each resource in the controller DT
> node.
> 
> Given that you really have a usecase now for multiple BARs, I think it is best
> if we can add DT support for the EPF drivers and describe the BAR resources in
> each EPF node. With that, we can hide all the resource allocation within the EPC
> core without exposing any flag to the EPF drivers.
> 
> So if the EPF node has a fixed location for BAR and defined in DT, then the new
> API pci_epf_alloc_set_bar() API will use that address and configure it
> accordingly. If not, it will just call pci_epf_alloc_space() internally to
> allocate the memory from coherent region and use it.

EPF node ? Did you perhaps mean EPC node ?
Using DT for endpoint functions is nonsense in my opinion: if something needs to
be configured, an epf has the configfs interface to get the information it may
need. And forcing EPF to have DT node is not scalable anyway.

So assuming you meant EPC, I am not sure if defining a fixed bar address in the
DT works for all cases. E.g. said address may depend on other hardware on the
platform (ex: memory nodes). So the ->get_bar() EPC operation that Rick is
proposing makes a lot more sense to me and will can be scaled to many many
configurations. Given that the EPF will (indirectly) call that operation, some
epf dependent parameters could even be passed.

And I agree (I think we all do) that combing alloc bar and get bar under a
single API is a lot cleaner. Though I am not a big fan of the name
pci_epc_alloc_set_bar() as it implies an allocation, which may not be the case
for fixed bars. So a simpler and more generic name like pci_epf_set_bar(),
pci_epf_init_bar(), pci_epf_config_bar()... would be way better in my opinion.

On Thu, Jul 25, 2024 at 03:30:57PM +0900, Damien Le Moal wrote:
> On 7/25/24 14:33, Manivannan Sadhasivam wrote:
> > On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
> >> Hello Rick,
> >>
> >> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> >>>>
> >>>> 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.)
> >>>>
> >>>
> >>> Yes, if we merge both, the code will need to be in the EPC code
> >>> (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> >>> would be called internally in the EPC code and not in the endpoint
> >>> function code.
> >>>
> >>> The only downside, as I said in my other mail, is the very niche case
> >>> where the contents of a BAR should be moved and remain unchanged when
> >>> rebinding a given endpoint function from one controller to another.
> >>> But this is not expected in any endpoint function currently, and with
> >>> the new changes, the endpoint could simply copy the BAR contents to a
> >>> local buffer and then set the contents in the BAR of the new
> >>> controller.
> >>> Anyways, probably no one is moving live functions between controllers,
> >>> and if needed it still can be done, so no problem here...
> >>
> >> I think we need to wait for Mani's opinion, but I've never heard of anyone
> >> doing so, and I agree with your suggested feature to copy the BAR contents
> >> in case anyone actually changes the backing EPC controller to an EPF.
> >> (You would need to stop the EPC to unbind + bind anyway, IIRC.)
> >>
> > 
> > Hi Rick/Niklas,
> > 
> > TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
> > we do not need to worry until the actual requirement comes.
> > 
> > But I really like combining alloc() and set_bar() APIs. Something I wanted to do
> > for so long but never got around to it. We can use the API name something like
> > pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
> > have it to align with existing APIs.
> > 
> > And regarding the implementation, the use of fixed address for BAR is not new.
> > If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
> > location that is derived from the controller DT node (MMIO region).
> > 
> > But I was thinking of moving this region to EPF node once we add DT support for
> > EPF driver. Because, there can be many EPFs in a single endpoint and each can
> > have upto 6 BARs. We cannot really describe each resource in the controller DT
> > node.
> > 
> > Given that you really have a usecase now for multiple BARs, I think it is best
> > if we can add DT support for the EPF drivers and describe the BAR resources in
> > each EPF node. With that, we can hide all the resource allocation within the EPC
> > core without exposing any flag to the EPF drivers.
> > 
> > So if the EPF node has a fixed location for BAR and defined in DT, then the new
> > API pci_epf_alloc_set_bar() API will use that address and configure it
> > accordingly. If not, it will just call pci_epf_alloc_space() internally to
> > allocate the memory from coherent region and use it.
> 
> EPF node ? Did you perhaps mean EPC node ?

No, EPF node. We already have the EPC node that is the PCIe controller node.

> Using DT for endpoint functions is nonsense in my opinion: if something needs to
> be configured, an epf has the configfs interface to get the information it may
> need. And forcing EPF to have DT node is not scalable anyway.
> 

Why? We are not using to DT to configure anything. If you try to use DT for that
purpose then it would be wrong. DT is meant to provide hardware description. In
this case, the EPF DT node can be used to describe the BAR address that is
allocated (fixed) in the hardware. I did propose this in the previous Plumbers
conf [1]. As I said earlier, this information is coming from the EPC node right
now for MHI which has a hardware entity, but moving it to the dedicated EPF node
would be the correct approach for scalability.

And ofc, that DT node *cannot* be used for anything else (like describing
VID:PID in configfs etc...). As a nice side effect of the EPF node, we can get
rid of configfs to create the link between EPC and EPF and start the controller.
Again, this won't be applicable to non-hw backed EPF.

> So assuming you meant EPC, I am not sure if defining a fixed bar address in the
> DT works for all cases. E.g. said address may depend on other hardware on the
> platform (ex: memory nodes). So the ->get_bar() EPC operation that Rick is
> proposing makes a lot more sense to me and will can be scaled to many many
> configurations. Given that the EPF will (indirectly) call that operation, some
> epf dependent parameters could even be passed.
> 
> And I agree (I think we all do) that combing alloc bar and get bar under a
> single API is a lot cleaner. Though I am not a big fan of the name
> pci_epc_alloc_set_bar() as it implies an allocation, which may not be the case
> for fixed bars. So a simpler and more generic name like pci_epf_set_bar(),
> pci_epf_init_bar(), pci_epf_config_bar()... would be way better in my opinion.
> 

Well, the EPF driver doesn't need to know if the underlying address if fixed or
dynamic IMO. It should just request BAR memory and the EPC core/controller
drivers should take care of that.

- Mani

[1] https://lpc.events/event/17/contributions/1419/attachments/1276/2636/LPC2023-PCI%20Endpoint%20Subsystem%20Open%20Items%20Discussion.pdf

On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
> > Hello Rick,
> >
> > On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> > > >
> > > > 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.)
> > > >
> > >
> > > Yes, if we merge both, the code will need to be in the EPC code
> > > (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> > > would be called internally in the EPC code and not in the endpoint
> > > function code.
> > >
> > > The only downside, as I said in my other mail, is the very niche case
> > > where the contents of a BAR should be moved and remain unchanged when
> > > rebinding a given endpoint function from one controller to another.
> > > But this is not expected in any endpoint function currently, and with
> > > the new changes, the endpoint could simply copy the BAR contents to a
> > > local buffer and then set the contents in the BAR of the new
> > > controller.
> > > Anyways, probably no one is moving live functions between controllers,
> > > and if needed it still can be done, so no problem here...
> >
> > I think we need to wait for Mani's opinion, but I've never heard of anyone
> > doing so, and I agree with your suggested feature to copy the BAR contents
> > in case anyone actually changes the backing EPC controller to an EPF.
> > (You would need to stop the EPC to unbind + bind anyway, IIRC.)
> >
>
> Hi Rick/Niklas,
>
> TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
> we do not need to worry until the actual requirement comes.
>
> But I really like combining alloc() and set_bar() APIs. Something I wanted to do
> for so long but never got around to it. We can use the API name something like
> pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
> have it to align with existing APIs.
>
> And regarding the implementation, the use of fixed address for BAR is not new.
> If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
> location that is derived from the controller DT node (MMIO region).
>
> But I was thinking of moving this region to EPF node once we add DT support for
> EPF driver. Because, there can be many EPFs in a single endpoint and each can
> have upto 6 BARs. We cannot really describe each resource in the controller DT
> node.
>
> Given that you really have a usecase now for multiple BARs, I think it is best
> if we can add DT support for the EPF drivers and describe the BAR resources in
> each EPF node. With that, we can hide all the resource allocation within the EPC
> core without exposing any flag to the EPF drivers.
>
> So if the EPF node has a fixed location for BAR and defined in DT, then the new
> API pci_epf_alloc_set_bar() API will use that address and configure it
> accordingly. If not, it will just call pci_epf_alloc_space() internally to
> allocate the memory from coherent region and use it.
>
> Wdyt?
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Hello Mani, thank you for your feedback.

I don't know if the EPF node in the DT is the right place for the
following reasons. First, it describes the requirements of the EPF and
not the restrictions imposed by the EPC (so for example one function
requires the BAR to use a given physical address and this is described
in the DT EPF node, but the controller could use any address, it just
configures the controller to use the address the EPF wants, but in the
other case (e.g., on FPGA), the EPC can only handle a BAR at a given
physical address and no other address so this should be in the EPC
node). Second, it is very static, so the EPC relation EPF would be
bound in the DT, I prefer being able to bind-unbind from configfs so
that I can make changes without reboot (e.g., alternate between two or
more endpoint functions, which may have different BAR requirements and
configurations).

For the EPFs I really think it is good to keep the BAR requirements in
the code for the moment, this makes it easier to swap endpoint
functions and makes development easier as well (e.g., binding several
different EPFs without reboot of the SoC they run on. In my typical
tests I bind one function, turn-on the host, do tests, turn-off the
host, make changes to an EPF, scp the new .ko on the SoC, rebind,
turn-on the host, etc.). For example, I alternate between pci-epf-test
(6 bars) and pci-epf-nvme (1 bar) of different sizes.

However, I can see a world where both binding and configuring EPF from
DT and the way it is currently done (alloc/set bar in code) and bind
in configfs could exist together as each have their use cases. But
forcing the use of DT seems impractical.

For combining pci_epf_alloc_space and pci_epc_set_bar into a single
function, everyone seems to agree on this one.

Best regards,
Rick

On 7/25/24 16:40, Manivannan Sadhasivam wrote:
>> Using DT for endpoint functions is nonsense in my opinion: if something needs to
>> be configured, an epf has the configfs interface to get the information it may
>> need. And forcing EPF to have DT node is not scalable anyway.
>>
> 
> Why? We are not using to DT to configure anything. If you try to use DT for that
> purpose then it would be wrong. DT is meant to provide hardware description. In
> this case, the EPF DT node can be used to describe the BAR address that is
> allocated (fixed) in the hardware. I did propose this in the previous Plumbers
> conf [1]. As I said earlier, this information is coming from the EPC node right
> now for MHI which has a hardware entity, but moving it to the dedicated EPF node
> would be the correct approach for scalability.

That does not make any sense. If we have a controller that has fixed addresses
for bars, then that is a hw property and that can go into the epc (PCI
controller) node. EPF is a software driver which has absolutely no business
having DT properties. configfs is made for that. And for special EPF drivers
that work only with particular controllers, e.g. your MHI EPF driver, the epf
can access properties of the epc if it needs information about the hardware
(e.g. a fixed bar address). The epf does not need to have its own node at ll in
the DT.

As a parallel, think of a DT describing a storage controller. The DT does not
describe the storage itself and much less the file system on that storage,
simply because everything can be probed at runtime from the controller DT and
the controller driver. Same thing here. EPF drivers go on top of EPC drivers and
their DT node describing the HW.

> 
> And ofc, that DT node *cannot* be used for anything else (like describing
> VID:PID in configfs etc...). As a nice side effect of the EPF node, we can get
> rid of configfs to create the link between EPC and EPF and start the controller.
> Again, this won't be applicable to non-hw backed EPF.
> 
>> So assuming you meant EPC, I am not sure if defining a fixed bar address in the
>> DT works for all cases. E.g. said address may depend on other hardware on the
>> platform (ex: memory nodes). So the ->get_bar() EPC operation that Rick is
>> proposing makes a lot more sense to me and will can be scaled to many many
>> configurations. Given that the EPF will (indirectly) call that operation, some
>> epf dependent parameters could even be passed.
>>
>> And I agree (I think we all do) that combing alloc bar and get bar under a
>> single API is a lot cleaner. Though I am not a big fan of the name
>> pci_epc_alloc_set_bar() as it implies an allocation, which may not be the case
>> for fixed bars. So a simpler and more generic name like pci_epf_set_bar(),
>> pci_epf_init_bar(), pci_epf_config_bar()... would be way better in my opinion.
>>
> 
> Well, the EPF driver doesn't need to know if the underlying address if fixed or
> dynamic IMO. It should just request BAR memory and the EPC core/controller
> drivers should take care of that.

Yes, I agree, and that is why I said that the name pci_epc_alloc_set_bar() is
not great because it implies allocation of memory, which may not always be
needed depending on the controller and may be on the function driver (e.g. fixed
bars using special memory).

On 7/25/24 17:06, Rick Wertenbroek wrote:
> On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
>>
>> On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
>>> Hello Rick,
>>>
>>> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
>>>>>
>>>>> 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.)
>>>>>
>>>>
>>>> Yes, if we merge both, the code will need to be in the EPC code
>>>> (because of the set_bar), and then the pci_epf_alloc_space (if needed)
>>>> would be called internally in the EPC code and not in the endpoint
>>>> function code.
>>>>
>>>> The only downside, as I said in my other mail, is the very niche case
>>>> where the contents of a BAR should be moved and remain unchanged when
>>>> rebinding a given endpoint function from one controller to another.
>>>> But this is not expected in any endpoint function currently, and with
>>>> the new changes, the endpoint could simply copy the BAR contents to a
>>>> local buffer and then set the contents in the BAR of the new
>>>> controller.
>>>> Anyways, probably no one is moving live functions between controllers,
>>>> and if needed it still can be done, so no problem here...
>>>
>>> I think we need to wait for Mani's opinion, but I've never heard of anyone
>>> doing so, and I agree with your suggested feature to copy the BAR contents
>>> in case anyone actually changes the backing EPC controller to an EPF.
>>> (You would need to stop the EPC to unbind + bind anyway, IIRC.)
>>>
>>
>> Hi Rick/Niklas,
>>
>> TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
>> we do not need to worry until the actual requirement comes.
>>
>> But I really like combining alloc() and set_bar() APIs. Something I wanted to do
>> for so long but never got around to it. We can use the API name something like
>> pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
>> have it to align with existing APIs.
>>
>> And regarding the implementation, the use of fixed address for BAR is not new.
>> If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
>> location that is derived from the controller DT node (MMIO region).
>>
>> But I was thinking of moving this region to EPF node once we add DT support for
>> EPF driver. Because, there can be many EPFs in a single endpoint and each can
>> have upto 6 BARs. We cannot really describe each resource in the controller DT
>> node.
>>
>> Given that you really have a usecase now for multiple BARs, I think it is best
>> if we can add DT support for the EPF drivers and describe the BAR resources in
>> each EPF node. With that, we can hide all the resource allocation within the EPC
>> core without exposing any flag to the EPF drivers.
>>
>> So if the EPF node has a fixed location for BAR and defined in DT, then the new
>> API pci_epf_alloc_set_bar() API will use that address and configure it
>> accordingly. If not, it will just call pci_epf_alloc_space() internally to
>> allocate the memory from coherent region and use it.
>>
>> Wdyt?
>>
>> - Mani
>>
>> --
>> மணிவண்ணன் சதாசிவம்
> 
> Hello Mani, thank you for your feedback.
> 
> I don't know if the EPF node in the DT is the right place for the
> following reasons. First, it describes the requirements of the EPF and
> not the restrictions imposed by the EPC (so for example one function
> requires the BAR to use a given physical address and this is described
> in the DT EPF node, but the controller could use any address, it just
> configures the controller to use the address the EPF wants, but in the
> other case (e.g., on FPGA), the EPC can only handle a BAR at a given
> physical address and no other address so this should be in the EPC
> node). Second, it is very static, so the EPC relation EPF would be
> bound in the DT, I prefer being able to bind-unbind from configfs so
> that I can make changes without reboot (e.g., alternate between two or
> more endpoint functions, which may have different BAR requirements and
> configurations).
> 
> For the EPFs I really think it is good to keep the BAR requirements in
> the code for the moment, this makes it easier to swap endpoint
> functions and makes development easier as well (e.g., binding several
> different EPFs without reboot of the SoC they run on. In my typical
> tests I bind one function, turn-on the host, do tests, turn-off the
> host, make changes to an EPF, scp the new .ko on the SoC, rebind,
> turn-on the host, etc.). For example, I alternate between pci-epf-test
> (6 bars) and pci-epf-nvme (1 bar) of different sizes.
> 
> However, I can see a world where both binding and configuring EPF from
> DT and the way it is currently done (alloc/set bar in code) and bind
> in configfs could exist together as each have their use cases. But
> forcing the use of DT seems impractical.

I do not think using DT for configuring an EPF *ever* make sense. An init script
on the EP platform can take care of whatever needs to be configured. DT is for
and should be restricted to describing the HW, not things that can be configured
at runtime using the HW information.

Doing it this way also makes the EPF code independent on the platform. E.g. if
we ever add an EPF node in the DT, that same EPF would not work on an endpoint
capable platform using UEFI. That is not acceptable for HW generic EPFs (e.g.
nvme, virtio, etc).

On Thu, Jul 25, 2024 at 10:06:38AM +0200, Rick Wertenbroek wrote:
> On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
> > > Hello Rick,
> > >
> > > On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> > > > >
> > > > > 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.)
> > > > >
> > > >
> > > > Yes, if we merge both, the code will need to be in the EPC code
> > > > (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> > > > would be called internally in the EPC code and not in the endpoint
> > > > function code.
> > > >
> > > > The only downside, as I said in my other mail, is the very niche case
> > > > where the contents of a BAR should be moved and remain unchanged when
> > > > rebinding a given endpoint function from one controller to another.
> > > > But this is not expected in any endpoint function currently, and with
> > > > the new changes, the endpoint could simply copy the BAR contents to a
> > > > local buffer and then set the contents in the BAR of the new
> > > > controller.
> > > > Anyways, probably no one is moving live functions between controllers,
> > > > and if needed it still can be done, so no problem here...
> > >
> > > I think we need to wait for Mani's opinion, but I've never heard of anyone
> > > doing so, and I agree with your suggested feature to copy the BAR contents
> > > in case anyone actually changes the backing EPC controller to an EPF.
> > > (You would need to stop the EPC to unbind + bind anyway, IIRC.)
> > >
> >
> > Hi Rick/Niklas,
> >
> > TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
> > we do not need to worry until the actual requirement comes.
> >
> > But I really like combining alloc() and set_bar() APIs. Something I wanted to do
> > for so long but never got around to it. We can use the API name something like
> > pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
> > have it to align with existing APIs.
> >
> > And regarding the implementation, the use of fixed address for BAR is not new.
> > If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
> > location that is derived from the controller DT node (MMIO region).
> >
> > But I was thinking of moving this region to EPF node once we add DT support for
> > EPF driver. Because, there can be many EPFs in a single endpoint and each can
> > have upto 6 BARs. We cannot really describe each resource in the controller DT
> > node.
> >
> > Given that you really have a usecase now for multiple BARs, I think it is best
> > if we can add DT support for the EPF drivers and describe the BAR resources in
> > each EPF node. With that, we can hide all the resource allocation within the EPC
> > core without exposing any flag to the EPF drivers.
> >
> > So if the EPF node has a fixed location for BAR and defined in DT, then the new
> > API pci_epf_alloc_set_bar() API will use that address and configure it
> > accordingly. If not, it will just call pci_epf_alloc_space() internally to
> > allocate the memory from coherent region and use it.
> >
> > Wdyt?
> >
> > - Mani
> >
> > --
> > மணிவண்ணன் சதாசிவம்
> 
> Hello Mani, thank you for your feedback.
> 
> I don't know if the EPF node in the DT is the right place for the
> following reasons. First, it describes the requirements of the EPF and
> not the restrictions imposed by the EPC (so for example one function
> requires the BAR to use a given physical address and this is described
> in the DT EPF node, but the controller could use any address, it just
> configures the controller to use the address the EPF wants, but in the
> other case (e.g., on FPGA), the EPC can only handle a BAR at a given
> physical address and no other address so this should be in the EPC
> node). Second, it is very static, so the EPC relation EPF would be
> bound in the DT, I prefer being able to bind-unbind from configfs so
> that I can make changes without reboot (e.g., alternate between two or
> more endpoint functions, which may have different BAR requirements and
> configurations).
> 
> For the EPFs I really think it is good to keep the BAR requirements in
> the code for the moment, this makes it easier to swap endpoint
> functions and makes development easier as well (e.g., binding several
> different EPFs without reboot of the SoC they run on. In my typical
> tests I bind one function, turn-on the host, do tests, turn-off the
> host, make changes to an EPF, scp the new .ko on the SoC, rebind,
> turn-on the host, etc.). For example, I alternate between pci-epf-test
> (6 bars) and pci-epf-nvme (1 bar) of different sizes.
> 

Ok, clearly the usecase I have for MHI is not the same as yours. MHI is a
hardware entity and it has the registers at a fixed address. So defining it
in the DT node made sense to me. But I haven't followed up with my proposal
since I thought it is not worth the effort until I see more usecases for DT.
That's why I was interested for a moment as I thought I got one :)

Anyway, thanks for clearing it up. I now agree that we don't need DT node for
your usecase.

> However, I can see a world where both binding and configuring EPF from
> DT and the way it is currently done (alloc/set bar in code) and bind
> in configfs could exist together as each have their use cases. But
> forcing the use of DT seems impractical.
> 
> For combining pci_epf_alloc_space and pci_epc_set_bar into a single
> function, everyone seems to agree on this one.
> 

Yes.

- Mani

On Thu, Jul 25, 2024 at 05:20:00PM +0900, Damien Le Moal wrote:
> On 7/25/24 17:06, Rick Wertenbroek wrote:
> > On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> >>
> >> On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
> >>> Hello Rick,
> >>>
> >>> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> >>>>>
> >>>>> 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.)
> >>>>>
> >>>>
> >>>> Yes, if we merge both, the code will need to be in the EPC code
> >>>> (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> >>>> would be called internally in the EPC code and not in the endpoint
> >>>> function code.
> >>>>
> >>>> The only downside, as I said in my other mail, is the very niche case
> >>>> where the contents of a BAR should be moved and remain unchanged when
> >>>> rebinding a given endpoint function from one controller to another.
> >>>> But this is not expected in any endpoint function currently, and with
> >>>> the new changes, the endpoint could simply copy the BAR contents to a
> >>>> local buffer and then set the contents in the BAR of the new
> >>>> controller.
> >>>> Anyways, probably no one is moving live functions between controllers,
> >>>> and if needed it still can be done, so no problem here...
> >>>
> >>> I think we need to wait for Mani's opinion, but I've never heard of anyone
> >>> doing so, and I agree with your suggested feature to copy the BAR contents
> >>> in case anyone actually changes the backing EPC controller to an EPF.
> >>> (You would need to stop the EPC to unbind + bind anyway, IIRC.)
> >>>
> >>
> >> Hi Rick/Niklas,
> >>
> >> TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
> >> we do not need to worry until the actual requirement comes.
> >>
> >> But I really like combining alloc() and set_bar() APIs. Something I wanted to do
> >> for so long but never got around to it. We can use the API name something like
> >> pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
> >> have it to align with existing APIs.
> >>
> >> And regarding the implementation, the use of fixed address for BAR is not new.
> >> If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
> >> location that is derived from the controller DT node (MMIO region).
> >>
> >> But I was thinking of moving this region to EPF node once we add DT support for
> >> EPF driver. Because, there can be many EPFs in a single endpoint and each can
> >> have upto 6 BARs. We cannot really describe each resource in the controller DT
> >> node.
> >>
> >> Given that you really have a usecase now for multiple BARs, I think it is best
> >> if we can add DT support for the EPF drivers and describe the BAR resources in
> >> each EPF node. With that, we can hide all the resource allocation within the EPC
> >> core without exposing any flag to the EPF drivers.
> >>
> >> So if the EPF node has a fixed location for BAR and defined in DT, then the new
> >> API pci_epf_alloc_set_bar() API will use that address and configure it
> >> accordingly. If not, it will just call pci_epf_alloc_space() internally to
> >> allocate the memory from coherent region and use it.
> >>
> >> Wdyt?
> >>
> >> - Mani
> >>
> >> --
> >> மணிவண்ணன் சதாசிவம்
> > 
> > Hello Mani, thank you for your feedback.
> > 
> > I don't know if the EPF node in the DT is the right place for the
> > following reasons. First, it describes the requirements of the EPF and
> > not the restrictions imposed by the EPC (so for example one function
> > requires the BAR to use a given physical address and this is described
> > in the DT EPF node, but the controller could use any address, it just
> > configures the controller to use the address the EPF wants, but in the
> > other case (e.g., on FPGA), the EPC can only handle a BAR at a given
> > physical address and no other address so this should be in the EPC
> > node). Second, it is very static, so the EPC relation EPF would be
> > bound in the DT, I prefer being able to bind-unbind from configfs so
> > that I can make changes without reboot (e.g., alternate between two or
> > more endpoint functions, which may have different BAR requirements and
> > configurations).
> > 
> > For the EPFs I really think it is good to keep the BAR requirements in
> > the code for the moment, this makes it easier to swap endpoint
> > functions and makes development easier as well (e.g., binding several
> > different EPFs without reboot of the SoC they run on. In my typical
> > tests I bind one function, turn-on the host, do tests, turn-off the
> > host, make changes to an EPF, scp the new .ko on the SoC, rebind,
> > turn-on the host, etc.). For example, I alternate between pci-epf-test
> > (6 bars) and pci-epf-nvme (1 bar) of different sizes.
> > 
> > However, I can see a world where both binding and configuring EPF from
> > DT and the way it is currently done (alloc/set bar in code) and bind
> > in configfs could exist together as each have their use cases. But
> > forcing the use of DT seems impractical.
> 
> I do not think using DT for configuring an EPF *ever* make sense. An init script
> on the EP platform can take care of whatever needs to be configured. DT is for
> and should be restricted to describing the HW, not things that can be configured
> at runtime using the HW information.
> 

No, MHI is a hardware entity. So atleast defining it in DT make sense. But as I
mentioned in reply to Rick, I haven't implemented it since it is really not
required now (MHI just needs a single BAR and right now the address is derived
from EPC node).

But for Rick's usecase, I agree with you/Rick that DT doesn't make sense. Thanks
for clearing it up.

> Doing it this way also makes the EPF code independent on the platform. E.g. if
> we ever add an EPF node in the DT, that same EPF would not work on an endpoint
> capable platform using UEFI. That is not acceptable for HW generic EPFs (e.g.
> nvme, virtio, etc).
> 

Well, those anyway cannot be defined in DT as they are software implementations
around hw.

- Mani

On Thu, Jul 25, 2024 at 10:06:38AM +0200, Rick Wertenbroek wrote:
> 
> I don't know if the EPF node in the DT is the right place for the
> following reasons. First, it describes the requirements of the EPF and
> not the restrictions imposed by the EPC (so for example one function
> requires the BAR to use a given physical address and this is described
> in the DT EPF node, but the controller could use any address, it just
> configures the controller to use the address the EPF wants, but in the
> other case (e.g., on FPGA), the EPC can only handle a BAR at a given
> physical address and no other address so this should be in the EPC
> node). Second, it is very static, so the EPC relation EPF would be
> bound in the DT, I prefer being able to bind-unbind from configfs so
> that I can make changes without reboot (e.g., alternate between two or
> more endpoint functions, which may have different BAR requirements and
> configurations).

I agree that the MHI case (EPF requires a specific host address for the BAR)
and the FPGA case (EPC requires a specific host address for the BAR),
is different.

Right now, for EPC requirements, we have epc_features in the driver that
describes hardware for this EPC (e.g. fixed size BARs). Right now, I don't
see a reason to move this to DT (or let a DT alternative co-exist).


For EPF requirements, the MHI EPF driver exposes several different
versions (pci_epf_mhi_sa8775p, pci_epf_mhi_sdx55, pci_epf_mhi_sm8450)
in configfs, and each have their own specific driver data.

The only negative I can see with this is that it might clutter the
/sys/kernel/config/pci_ep/functions/ directory. Perhaps it is possible
to create a /sys/kernel/config/pci_ep/functions/pci_epf_mhi/ directory
where all the different versions reside?

Keeping this per-platform data in the MHI EPF driver is fine IMO.

If you would prefer to create a "pci_epf_mhi" generic version, that instead
takes this information from configfs, that would be fine too. I would also
be fine if you created a "pci_epf_mhi" generic version that tries to take
this information from device tree (as long as it is also possible to supply
the same information via configfs).

Good luck getting it accepted by the DT maintainers though. The configfs
interface was chosen because some developers (including Arnd Bergmann, IIRC)
didn't like the idea of having EPF specific information in DT.


> For combining pci_epf_alloc_space and pci_epc_set_bar into a single
> function, everyone seems to agree on this one.

Indeed, but as usual, good naming is one of the hardest problems :)

Instead of pci_epf_alloc_set_bar(), perhaps pci_epf_setup_bar() ?
If the EPC has a fixed address requirement, it will use that instead of
allocating memory.

If the EPF has a fixed address requirement, the API call will only succeed
if EPC does not have a fixed address requirement.

Perhaps EPF drivers that have a fixed address requirement could supply
that as a parameter to the API (and the EPC driver will fail the request
if it itself has fixed address requirement).

We already supply 'struct pci_epf_bar' to .set_bar(). I think the simplest
is just to extend this struct with more members (e.g. requires_fixed_addr,
fixed_addr). Basically 'struct pci_epf_bar' has all the wishes of the EPF
(32-bit/64-bit, IO/MEM BAR, BAR size, etc.).

It is already up to the EPC driver to fail the .set_bar() request if it
can't be satisfied, so pci_epf_setup_bar() could behave like .set_bar().


Kind regards,
Niklas

On Thu, Jul 25, 2024 at 04:17:03PM +0200, Niklas Cassel wrote:
> On Thu, Jul 25, 2024 at 10:06:38AM +0200, Rick Wertenbroek wrote:
> > 
> > I don't know if the EPF node in the DT is the right place for the
> > following reasons. First, it describes the requirements of the EPF and
> > not the restrictions imposed by the EPC (so for example one function
> > requires the BAR to use a given physical address and this is described
> > in the DT EPF node, but the controller could use any address, it just
> > configures the controller to use the address the EPF wants, but in the
> > other case (e.g., on FPGA), the EPC can only handle a BAR at a given
> > physical address and no other address so this should be in the EPC
> > node). Second, it is very static, so the EPC relation EPF would be
> > bound in the DT, I prefer being able to bind-unbind from configfs so
> > that I can make changes without reboot (e.g., alternate between two or
> > more endpoint functions, which may have different BAR requirements and
> > configurations).
> 
> I agree that the MHI case (EPF requires a specific host address for the BAR)
> and the FPGA case (EPC requires a specific host address for the BAR),
> is different.
> 
> Right now, for EPC requirements, we have epc_features in the driver that
> describes hardware for this EPC (e.g. fixed size BARs). Right now, I don't
> see a reason to move this to DT (or let a DT alternative co-exist).
> 
> 
> For EPF requirements, the MHI EPF driver exposes several different
> versions (pci_epf_mhi_sa8775p, pci_epf_mhi_sdx55, pci_epf_mhi_sm8450)
> in configfs, and each have their own specific driver data.
> 
> The only negative I can see with this is that it might clutter the
> /sys/kernel/config/pci_ep/functions/ directory. Perhaps it is possible
> to create a /sys/kernel/config/pci_ep/functions/pci_epf_mhi/ directory
> where all the different versions reside?
> 
> Keeping this per-platform data in the MHI EPF driver is fine IMO.
> 
> If you would prefer to create a "pci_epf_mhi" generic version, that instead
> takes this information from configfs, that would be fine too. I would also
> be fine if you created a "pci_epf_mhi" generic version that tries to take
> this information from device tree (as long as it is also possible to supply
> the same information via configfs).
> 
> Good luck getting it accepted by the DT maintainers though. The configfs
> interface was chosen because some developers (including Arnd Bergmann, IIRC)
> didn't like the idea of having EPF specific information in DT.
> 

I know the story and they were right (Arnd/Rob) as the initial version tried to
use DT for everything. But MHI is different as it is a hardware entity, which
not only uses a fixed address, it uses a fixed region that has MHI registers in
hw.

Anyway, I don't have a *strong* motivation to upstream the DT support for it as
the current approach is serving good for now, unless new usecases show up.

> 
> > For combining pci_epf_alloc_space and pci_epc_set_bar into a single
> > function, everyone seems to agree on this one.
> 
> Indeed, but as usual, good naming is one of the hardest problems :)
> 

Respectively agree with you :)

> Instead of pci_epf_alloc_set_bar(), perhaps pci_epf_setup_bar() ?
> If the EPC has a fixed address requirement, it will use that instead of
> allocating memory.
> 
> If the EPF has a fixed address requirement, the API call will only succeed
> if EPC does not have a fixed address requirement.
> 
> Perhaps EPF drivers that have a fixed address requirement could supply
> that as a parameter to the API (and the EPC driver will fail the request
> if it itself has fixed address requirement).
> 

I vary with you here. IMO EPF drivers have no business in knowing the BAR
location as they are independent of controller (mostly except drivers like MHI).
So an EPF driver should call a single API that just allocates/configures the
BAR. For fixed address BAR, EPC core should be able to figure it out using the
EPC features.

For naming, we have 3 proposals as of now:

1. pci_epf_setup_bar() - This looks good, but somewhat collides with the
existing pci_epc_set_bar() API.

2. pci_epc_alloc_set_bar() - Looks ugly, but aligns with the existing APIs.

3. pci_epc_get_bar() - Also looks good, but the usage of 'get' gives the
impression that the BAR is fetched from somewhere, which is true for fixed
address BAR, but not for dynamic BAR.

After looking at these APIs multiple times (confusing at its best), I tend to
feel that pci_epc_get_bar() is the better of the 3.

- Mani

On Thu, Jul 25, 2024 at 10:06:52PM +0530, Manivannan Sadhasivam wrote:
> 
> I vary with you here. IMO EPF drivers have no business in knowing the BAR
> location as they are independent of controller (mostly except drivers like MHI).
> So an EPF driver should call a single API that just allocates/configures the
> BAR. For fixed address BAR, EPC core should be able to figure it out using the
> EPC features.
> 
> For naming, we have 3 proposals as of now:
> 
> 1. pci_epf_setup_bar() - This looks good, but somewhat collides with the
> existing pci_epc_set_bar() API.
> 
> 2. pci_epc_alloc_set_bar() - Looks ugly, but aligns with the existing APIs.
> 
> 3. pci_epc_get_bar() - Also looks good, but the usage of 'get' gives the
> impression that the BAR is fetched from somewhere, which is true for fixed
> address BAR, but not for dynamic BAR.

pci_epc_configure_bar() ?
we could name the 'struct pci_epf_bar *' param 'conf'


Kind regards,
Niklas

On 7/26/24 06:52, Niklas Cassel wrote:
> On Thu, Jul 25, 2024 at 10:06:52PM +0530, Manivannan Sadhasivam wrote:
>>
>> I vary with you here. IMO EPF drivers have no business in knowing the BAR
>> location as they are independent of controller (mostly except drivers like MHI).
>> So an EPF driver should call a single API that just allocates/configures the
>> BAR. For fixed address BAR, EPC core should be able to figure it out using the
>> EPC features.
>>
>> For naming, we have 3 proposals as of now:
>>
>> 1. pci_epf_setup_bar() - This looks good, but somewhat collides with the
>> existing pci_epc_set_bar() API.
>>
>> 2. pci_epc_alloc_set_bar() - Looks ugly, but aligns with the existing APIs.
>>
>> 3. pci_epc_get_bar() - Also looks good, but the usage of 'get' gives the
>> impression that the BAR is fetched from somewhere, which is true for fixed
>> address BAR, but not for dynamic BAR.
> 
> pci_epc_configure_bar() ?
> we could name the 'struct pci_epf_bar *' param 'conf'

+1

But let's spell this out: pci_epc_configure_bar(), to be sure to avoid any
possible confusion (config could mean configure or configuration...).

On Fri, Jul 26, 2024 at 12:53 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 7/26/24 06:52, Niklas Cassel wrote:
> > On Thu, Jul 25, 2024 at 10:06:52PM +0530, Manivannan Sadhasivam wrote:
> >>
> >> I vary with you here. IMO EPF drivers have no business in knowing the BAR
> >> location as they are independent of controller (mostly except drivers like MHI).
> >> So an EPF driver should call a single API that just allocates/configures the
> >> BAR. For fixed address BAR, EPC core should be able to figure it out using the
> >> EPC features.
> >>
> >> For naming, we have 3 proposals as of now:
> >>
> >> 1. pci_epf_setup_bar() - This looks good, but somewhat collides with the
> >> existing pci_epc_set_bar() API.
> >>
> >> 2. pci_epc_alloc_set_bar() - Looks ugly, but aligns with the existing APIs.
> >>
> >> 3. pci_epc_get_bar() - Also looks good, but the usage of 'get' gives the
> >> impression that the BAR is fetched from somewhere, which is true for fixed
> >> address BAR, but not for dynamic BAR.
> >
> > pci_epc_configure_bar() ?
> > we could name the 'struct pci_epf_bar *' param 'conf'
>
> +1
>
> But let's spell this out: pci_epc_configure_bar(), to be sure to avoid any
> possible confusion (config could mean configure or configuration...).
>
> --
> Damien Le Moal
> Western Digital Research
>

+1

One thing to keep in mind is that 'struct pci_epf_bar 'conf' would be
an 'inout' parameter, where 'conf' gets changed in case of a fixed
address BAR or fixed 64-bit etc. This means the EPF code needs to
check 'conf' after the call. Also, if the caller sets flags and the
controller only handles different flags, do we return an error, or
configure the BAR with the only possible flags and let the caller
check if those flags are ok for the endpoint function ?

This is a bit unclear for me for the moment.

One solution could be to pass two 'pci_epf_bar' structs, one is to be
filed by the function call, and the other an optional parameter
'desired_conf' (i.e. can be 'NULL') if NULL it would mean let the
controller decide, if not NULL 'desired_conf' describes the EPF
requirements (e.g., chosen BAR address or certain flags, size, etc.)
and 'pci_epc_configure_bar()' would fail if it cannot satisfy the
'desired_conf' requirements.

But still 'desired_conf' is a structure with several things that could
be independently desired, e.g., a given address or BAR size, flags,
but not all of them might be a necessity.

Let me know what you think.

Have a nice week-end.

Rick

On Fri, Jul 26, 2024 at 01:21:32PM +0200, Rick Wertenbroek wrote:
> 
> One thing to keep in mind is that 'struct pci_epf_bar 'conf' would be
> an 'inout' parameter, where 'conf' gets changed in case of a fixed
> address BAR or fixed 64-bit etc. This means the EPF code needs to
> check 'conf' after the call. Also, if the caller sets flags and the
> controller only handles different flags, do we return an error, or
> configure the BAR with the only possible flags and let the caller
> check if those flags are ok for the endpoint function ?
> 
> This is a bit unclear for me for the moment.

Indeed, it is quite messy at the moment, which is why we should try
to do better, and clearly document the cases where the API should
fail, and when it is okay for the API to set things automatically.


How the current pci_epf_alloc_space() (which is used to allocate space
for a BAR) works:
- Takes a enum pci_barno bar.

- Will modify the epf_bar[bar] array of structs. (For either primary
  interface array of BARs or secondary interface array of BARs.)
  Perhaps it would be better if this was an array of pointers instead,
  so that an EPF driver cannot modify a BAR that has not been allocated,
  and that the new API allocates a 'struct pci_epf_bar', and sets the
  pointer. (But perhaps better to leave it like it is to start with.)

- Uses |= to set flags, which means that if an EPF has modified
  epf_bar[bar].flags before calling pci_epf_alloc_space(), these
  flags would still be set. (I wouldn't recommend any EPF driver to do so.)
  It would be much better if we provided 'flags' to the new API, so that
  the new API can set the flags using = instead of |=.

- Flag PCI_BASE_ADDRESS_MEM_TYPE_64 will automatically get set if the BAR
  can only be a 64-bit BAR according to epc_features.
  This is a bit debatable. For some EPF drivers, getting a 64-bit BAR even
  if you only requested a 32-bit BAR, might be fine. But for some EPF
  drivers, I can imagine that it is not okay. (Perhaps we need a
  "bool strict" that gives errors more often instead of implicitly setting
  flags not that was not requested.

- Will set PCI_BASE_ADDRESS_MEM_TYPE_64 if the requested size is larger
  than 2 GB. The new API should simply give an error if flag
  PCI_BASE_ADDRESS_MEM_TYPE_64 is not set when size is larger than 2 GB.

- If the bar is a fixed size BAR according to epc_features, it will set a
  size larger than the requested size. It will however give an error if the
  requested size is larger than the fixed size BAR. (Should a possible
  "bool strict" give an error if you cannot set the exact requested size,
  or is it usually okay to have a BAR size that is larger than requested?)


How the current pci_epc_set_bar() works:
- Takes 'struct pci_epf_bar *epf_bar'

- This function will give an error if PCI_BASE_ADDRESS_MEM_TYPE_64 is not set
  when size is larger than 2 GB, or if you try to set BAR5 as a 64-bit BAR.

- Calls epc->ops->set_bar() will should return errors if it cannot satisfy
  the 'struct pci_epf_bar *epf_bar'.


How the epc->ops->set_bar() works:
- A EPC might have additional restrictions that are controller specific,
  which isn't/couldn't be described in epc_features. E.g. pcie-designware-ep.c
  requires a 64-bit BAR to start at a even BAR number. (The PCIe spec allows
  a 64-bit BAR to start both at an odd or even BAR number.)


So it seems right now, alloc_space() might result in a 'struct pci_epf_bar'
that wasn't exactly what was requested, but set_bar() should always fail if
an EPC driver cannot fullfil exactly what was requested in the
'struct pci_epf_bar' (that was returned by alloc_space()).


We all agree that this is a good idea, but does anyone actually intend to
take on the effort of trying to create a new API that is basically
pci_epf_alloc_space() + pci_epc_set_bar() combined?

Personally, my plan is to respin/improve Damien's "improved PCI endpoint
memory mapping API" series:
https://lore.kernel.org/linux-pci/20240330041928.1555578-1-dlemoal@kernel.org/

But I'm also going away on two weeks vacation starting today, so it will
take a while before I send something out...


Kind regards,
Niklas

On Fri, Jul 26, 2024 at 03:41:37PM +0200, Niklas Cassel wrote:
> On Fri, Jul 26, 2024 at 01:21:32PM +0200, Rick Wertenbroek wrote:
> > 
> > One thing to keep in mind is that 'struct pci_epf_bar 'conf' would be
> > an 'inout' parameter, where 'conf' gets changed in case of a fixed
> > address BAR or fixed 64-bit etc. This means the EPF code needs to
> > check 'conf' after the call. Also, if the caller sets flags and the
> > controller only handles different flags, do we return an error, or
> > configure the BAR with the only possible flags and let the caller
> > check if those flags are ok for the endpoint function ?
> > 
> > This is a bit unclear for me for the moment.
> 

+1 for the new API name: pci_epc_configure_bar()

> Indeed, it is quite messy at the moment, which is why we should try
> to do better, and clearly document the cases where the API should
> fail, and when it is okay for the API to set things automatically.
> 
> 
> How the current pci_epf_alloc_space() (which is used to allocate space
> for a BAR) works:
> - Takes a enum pci_barno bar.
> 
> - Will modify the epf_bar[bar] array of structs. (For either primary
>   interface array of BARs or secondary interface array of BARs.)
>   Perhaps it would be better if this was an array of pointers instead,
>   so that an EPF driver cannot modify a BAR that has not been allocated,
>   and that the new API allocates a 'struct pci_epf_bar', and sets the
>   pointer. (But perhaps better to leave it like it is to start with.)
> 

I like this idea. But yeah, there is no pressing need to implement this for
the new API.

> - Uses |= to set flags, which means that if an EPF has modified
>   epf_bar[bar].flags before calling pci_epf_alloc_space(), these
>   flags would still be set. (I wouldn't recommend any EPF driver to do so.)
>   It would be much better if we provided 'flags' to the new API, so that
>   the new API can set the flags using = instead of |=.
> 

Well, with the new API I'd like to allow EPF drivers to set the flags to be able
to request 32/64 bit BAR of their preference. Because, the EPF driver may know
its own limitation.

> - Flag PCI_BASE_ADDRESS_MEM_TYPE_64 will automatically get set if the BAR
>   can only be a 64-bit BAR according to epc_features.
>   This is a bit debatable. For some EPF drivers, getting a 64-bit BAR even
>   if you only requested a 32-bit BAR, might be fine. But for some EPF
>   drivers, I can imagine that it is not okay. (Perhaps we need a
>   "bool strict" that gives errors more often instead of implicitly setting
>   flags not that was not requested.
> 

EPF drivers cannot explicitly request 32/64 bit BAR using alloc_space(). Perhaps
you are mixing set_bar() implementation?

But only if the EPF driver has explicitly set the flags, then returning error
makes sense if the EPC core cannot satisfy the requirements.

> - Will set PCI_BASE_ADDRESS_MEM_TYPE_64 if the requested size is larger
>   than 2 GB. The new API should simply give an error if flag
>   PCI_BASE_ADDRESS_MEM_TYPE_64 is not set when size is larger than 2 GB.
> 

I would prefer to return error _only_ if EPC core cannot satisfy the requirement
from the EPF driver.

> - If the bar is a fixed size BAR according to epc_features, it will set a
>   size larger than the requested size. It will however give an error if the
>   requested size is larger than the fixed size BAR. (Should a possible
>   "bool strict" give an error if you cannot set the exact requested size,
>   or is it usually okay to have a BAR size that is larger than requested?)
> 

I think it is fine. Most of the EPF drivers expose a register region and that
size is usually less than the standard BAR size.

- Mani

> 
> How the current pci_epc_set_bar() works:
> - Takes 'struct pci_epf_bar *epf_bar'
> 
> - This function will give an error if PCI_BASE_ADDRESS_MEM_TYPE_64 is not set
>   when size is larger than 2 GB, or if you try to set BAR5 as a 64-bit BAR.
> 
> - Calls epc->ops->set_bar() will should return errors if it cannot satisfy
>   the 'struct pci_epf_bar *epf_bar'.
> 
> 
> How the epc->ops->set_bar() works:
> - A EPC might have additional restrictions that are controller specific,
>   which isn't/couldn't be described in epc_features. E.g. pcie-designware-ep.c
>   requires a 64-bit BAR to start at a even BAR number. (The PCIe spec allows
>   a 64-bit BAR to start both at an odd or even BAR number.)
> 
> 
> So it seems right now, alloc_space() might result in a 'struct pci_epf_bar'
> that wasn't exactly what was requested, but set_bar() should always fail if
> an EPC driver cannot fullfil exactly what was requested in the
> 'struct pci_epf_bar' (that was returned by alloc_space()).
> 
> 
> We all agree that this is a good idea, but does anyone actually intend to
> take on the effort of trying to create a new API that is basically
> pci_epf_alloc_space() + pci_epc_set_bar() combined?
> 
> Personally, my plan is to respin/improve Damien's "improved PCI endpoint
> memory mapping API" series:
> https://lore.kernel.org/linux-pci/20240330041928.1555578-1-dlemoal@kernel.org/
> 
> But I'm also going away on two weeks vacation starting today, so it will
> take a while before I send something out...
> 
> 
> Kind regards,
> Niklas