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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

-- 
மணிவண்ணன் சதாசிவம்