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@xxxxxxxxxx/ 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
