Message-ID: <CAAEEuho08Taw3v2BeCjNDQZ0BRU0oweiLuOuhfrLd7PqAyzSCQ@mail.gmail.com>
Date: Tue, 23 Jul 2024 18:06:26 +0200
From: Rick Wertenbroek <rick.wertenbroek@...il.com>
To: Niklas Cassel <cassel@...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>, Frank Li <Frank.Li@....com>, 
	Damien Le Moal <dlemoal@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed
 address BARs in EPC

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

Powered by blists - more mailing lists