Re: PCI endpoint: pci-epf-test is broken on big-endian

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

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

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