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

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

[Niklas Cassel]

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.

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

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

[Damien Le Moal]

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

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

[Manivannan Sadhasivam]

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

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

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

[Damien Le Moal]

On 10/22/24 12:26, Manivannan Sadhasivam wrote:
>>> 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.

You are talking about the host (RC side) ? I was talking about the EP side...

pci_epf_alloc_space() returns a "void *" pointer, and the same is true for the
dma_alloc_coherent() call that actually allocates the BAR space. So on the EP, a
BAR memory should be treated as regular memory, but "volatile" since it can
change under the driver (due to the host writing to the BAR). Hence
READ_ONCE()/WRITE_ONCE() is the correct way to access a BAR on the endpoint.

And yes, readl() and friends are for the PCI RC (host) side, that I know.

Sorry about the confusing comment. I was thinking about the EP while reading
Niklas's comment :)

-- 
Damien Le Moal
Western Digital Research

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

[Frank Li]

On Mon, Oct 21, 2024 at 09:49:54AM +0200, 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);

struct pci_epf_test_reg {
        u32     magic;
 	^^ should be le32.
	...
} __packed;

So sparse can detect all endian problem.

Frank

>
>
> 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.
>
> 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

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

[Manivannan Sadhasivam]

On Mon, Oct 21, 2024 at 09:49:54AM +0200, 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.
> 
> 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.
> 

Hi Niklas,

Thanks a lot for reporting. Yes, I acknowledge the issue on the ep side. Will
try to spare some cycle to fix it asap.

- Mani

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