On 11/27/24 20:23, Niklas Cassel wrote: [...] > IMO, an EPF driver should be able to write to any PCI address. > (Especially since this can be solved trivially by EPF drivers simply using > pci_epc_mem_map()/unmap()) > > E.g. the upcoming NVMe EPF driver uses the NVMe host side driver. > This host side driver does no magic padding on host side for endpoints > (NVMe controllers) that have an iATU with outbound address alignment > restriction. Not to mention that per NVMe specs, there should be no alignment constraints for IO buffers. Anything is OK. And you can see it running this EPF driver while going through a host boot sequence and using nvme-cli: the BIOS probing the NVMe drive, GRUB looking at the NVMe drive and nvme-cli using on-stack buffers result is high randomness of the buffer alignments. Trying to get that to work in all cases led to the pci_epc_mem_map()/unmap() API :) So having the test EPF driver allowing testing such unaligned pattern would definitely help solidify the endpoint framework and its endpoint controller drivers. > Imagine e.g. another EPF driver, for another existing protocol, e.g. AHCI. > Modifying existing generic Linux drivers (e.g. the AHCI driver), simply because > some random EPC driver has a specific outbound alignment requirement (that can > simply be ignored by using pci_epc_mem_map/unmap()) does not make sense IMO. > > >> >> That being said, I really like to get rid of the hardcoded >> 'pci_endpoint_test_data::alignment' field and make the driver learn about it >> dynamically. I'm just thinking out loud here. > > That would certainly be possible, by simply dedicating a new register to that > in the test BAR (struct pci_epf_test_reg). However, I think that that would be > a worse alternative compared to what this series is proposing. > > The only ugliness in my opinion is that we are setting the CAP_UNALIGNED_ACCESS > capability based on if an EPC driver has implemented the .align_addr callback. > > If we could simply implement .align_addr() in the two missing EPC drivers: > drivers/pci/controller/cadence/pcie-cadence-ep.c At least for this one, it is likely exactly the same as for drivers/pci/controller/pcie-rockchip-ep.c. The code for these 2 drivers is suspiciously similar, which strongly hints at the fact that the HW is most likely based on the same IP blocks. > drivers/pci/controller/pcie-rcar-ep.c > > pci-epf-test.c could set the CAP_UNALIGNED_ACCESS capability unconditionally. > > However, I do not have the hardware for those two drivers, so I cannot > implement .align_addr() for those myself. > > So in order to be able to move forward, I think that simply letting > pci-epf-test.c check if the EPC driver which is currently in use has > implemented the .align_addr callback, is a tolerable ugliness. > > Once all EPC drivers have implemented .align_addr(), we could change > pci-epf-test.c to unconditionally set the CAP_UNALIGNED_ACCESS capability. > > > Kind regards, > Niklas -- Damien Le Moal Western Digital Research
