Message-ID: <ZqOnkTidYLc0EboJ@ryzen.lan>
Date: Fri, 26 Jul 2024 15:41:37 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Rick Wertenbroek <rick.wertenbroek@...il.com>
Cc: Damien Le Moal <dlemoal@...nel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	rick.wertenbroek@...g-vd.ch, alberto.dassatti@...g-vd.ch,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Frank Li <Frank.Li@....com>,
	Lars-Peter Clausen <lars@...afoo.de>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed
 address BARs in EPC

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

Powered by blists - more mailing lists