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

[Damien Le Moal <dlemoal@kernel.org>]

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.

> 
> I'm not planning on spending time on this, but I thought that I ought to at
> least report it, such that maintainers/developers/users are aware of it.
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research