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

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

Powered by blists - more mailing lists