On Mon, Oct 21, 2024 at 06:49:11PM +0900, Damien Le Moal wrote: > On 10/21/24 16:49, Niklas Cassel wrote: > > Hello PCI endpoint maintainers, > > > > > > While looking at the pci-epf-test.c driver, I noticed that > > pci-epf-test is completely broken with regards to endianness. > > > > As you probably know, PCI devices are inherently little-endian, > > and the data stored in the PCI BARs should be in little-endian. > > > > However, pci-epf-test does no conversion before storing the data > > to backing memory, and no conversion after reading the data from > > backing memory. > > > > For the data backing test_reg BAR (usually BAR0), which has the > > format as defined by struct pci_epf_test_reg, is simply stored > > to memory using e.g.: > > reg->status = STATUS_WRITE_SUCCESS; > > > > Surely, this should be: > > reg->status = cpu_to_le32(STATUS_WRITE_SUCCESS); > > > > > > Likewise the src and dst address is accessed simply by > > reg->dst_addr and reg->src_addr. > > > > Surely, this should be accessed using: > > dst_addr = le64_to_cpu(reg->dst_addr); > > src_addr = le64_to_cpu(reg->src_addr); > > > > So bottom line, pci-epf-test will currently not behave correctly > > on big-endian. > > > > > > > > Looking at pci-endpoint-test however, it does all its accesses using > > readl() and writel(), and if you look at the implementations of > > readl()/writel(): > > https://github.com/torvalds/linux/blob/v6.12-rc4/include/asm-generic/io.h#L181-L184 > > > > They convert to CPU native after reading, and convert to little-endian > > before writing, so pci-endpoint-test (RC side driver) is okay, it is > > just pci-epf-test (EP side driver) that is broken. > > That in itself is another problem. The use of readl/writel for things in the EPF > BAR memory is also *wrong*, because that memory is NOT a real mmio memory. We > should be using READ_ONCE()/WRITE_ONCE() to treat the BAR as volatile memory but > not use readl/writel. > Not at all. The memory returned by pci_ioremap_bar() is annotated with __iomem, which means it should *only* be accessed with the relevant accessors like readl(), ioread32() etc... The memory is still treated as MMIO, so all the restrictions (alignment) applies to it also. - Mani -- மணிவண்ணன் சதாசிவம்
