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@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 -- மணிவண்ணன் சதாசிவம்
