[PATCH] PCI: endpoint: cast the page number to phys_addr_t

[PATCH] PCI: endpoint: cast the page number to phys_addr_t

[Alan Mikhak]

From: Alan Mikhak <alan.mikhak@sifive.com>

Modify pci_epc_mem_alloc_addr() to cast the variable 'pageno'
from type 'int' to 'phys_addr_t' before shifting left. This
cast is needed to avoid treating bit 31 of 'pageno' as the
sign bit which would otherwise get sign-extended to produce
a negative value. When added to the base address of PCI memory
space, the negative value would produce an invalid physical
address which falls before the start of the PCI memory space.

Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
---
 drivers/pci/endpoint/pci-epc-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
index 2bf8bd1f0563..d2b174ce15de 100644
--- a/drivers/pci/endpoint/pci-epc-mem.c
+++ b/drivers/pci/endpoint/pci-epc-mem.c
@@ -134,7 +134,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
 	if (pageno < 0)
 		return NULL;
 
-	*phys_addr = mem->phys_base + (pageno << page_shift);
+	*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
 	virt_addr = ioremap(*phys_addr, size);
 	if (!virt_addr)
 		bitmap_release_region(mem->bitmap, pageno, order);
-- 
2.7.4

Re: [PATCH] PCI: endpoint: cast the page number to phys_addr_t

[Alan Mikhak]

On Fri, Oct 4, 2019 at 6:49 PM Alan Mikhak <alan.mikhak@sifive.com> wrote:
>
> From: Alan Mikhak <alan.mikhak@sifive.com>
>
> Modify pci_epc_mem_alloc_addr() to cast the variable 'pageno'
> from type 'int' to 'phys_addr_t' before shifting left. This
> cast is needed to avoid treating bit 31 of 'pageno' as the
> sign bit which would otherwise get sign-extended to produce
> a negative value. When added to the base address of PCI memory
> space, the negative value would produce an invalid physical
> address which falls before the start of the PCI memory space.
>
> Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> ---
>  drivers/pci/endpoint/pci-epc-mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> index 2bf8bd1f0563..d2b174ce15de 100644
> --- a/drivers/pci/endpoint/pci-epc-mem.c
> +++ b/drivers/pci/endpoint/pci-epc-mem.c
> @@ -134,7 +134,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>         if (pageno < 0)
>                 return NULL;
>
> -       *phys_addr = mem->phys_base + (pageno << page_shift);
> +       *phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
>         virt_addr = ioremap(*phys_addr, size);
>         if (!virt_addr)
>                 bitmap_release_region(mem->bitmap, pageno, order);
> --
> 2.7.4
>

Hi Kishon,

This issue was observed when requesting pci_epc_mem_alloc_addr()
to allocate a region of size 0x40010000ULL (1GB + 64KB) from a
128GB PCI address space with page sizes being 64KB. This resulted
in 'pageno' value of '0x8000' as the first available page in a
contiguous region for the requested size due to other smaller
regions having been allocated earlier. With 64KB page sizes,
the variable 'page_shift' holds a value of 0x10. Shifting 'pageno'
16 bits to the left results in an 'int' value whose bit 31 is set.

[   10.565256] __pci_epc_mem_init: mem size 0x2000000000 page_size 0x10000
[   10.571613] __pci_epc_mem_init: mem pages 0x200000 bitmap_size
0x40000 page_shift 0x10

PCI memory base 0x2000000000
PCI memory size 128M 0x2000000000
page_size 64K 0x10000
page_shift  16 0x10
pages 2M 0x200000
bitmap_size 256K 0x40000

[  702.050299] pci_epc_mem_alloc_addr: size 0x10000 order 0x0 pageno
0x4 virt_add 0xffffffd0047b0000 phys_addr 0x2000040000
[  702.061424] pci_epc_mem_alloc_addr: size 0x10000 order 0x0 pageno
0x5 virt_add 0xffffffd0047d0000 phys_addr 0x2000050000
[  702.203933] pci_epc_mem_alloc_addr: size 0x40010000 order 0xf
pageno 0x8000 virt_add 0xffffffd004800000 phys_addr 0x1f80000000
[  702.216547] Oops - store (or AMO) access fault [#1]
:::
[  702.310198] sstatus: 0000000200000120 sbadaddr: ffffffd004804000
scause: 0000000000000007

Regards,
Alan

Re: [PATCH] PCI: endpoint: cast the page number to phys_addr_t

[Alan Mikhak]

> PCI memory size 128M 0x2000000000

Correction:
PCI memory size 128G 0x2000000000

Re: [PATCH] PCI: endpoint: cast the page number to phys_addr_t

[Kishon Vijay Abraham I]

Hi Alan,

On 07/10/19 11:14 PM, Alan Mikhak wrote:
> On Fri, Oct 4, 2019 at 6:49 PM Alan Mikhak <alan.mikhak@sifive.com> wrote:
>>
>> From: Alan Mikhak <alan.mikhak@sifive.com>
>>
>> Modify pci_epc_mem_alloc_addr() to cast the variable 'pageno'
>> from type 'int' to 'phys_addr_t' before shifting left. This
>> cast is needed to avoid treating bit 31 of 'pageno' as the
>> sign bit which would otherwise get sign-extended to produce
>> a negative value. When added to the base address of PCI memory
>> space, the negative value would produce an invalid physical
>> address which falls before the start of the PCI memory space.
>>
>> Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>

Thanks for the patch.

The change-log title should start with "capitalized verb"

linux-pci follows certain guidelines listed here

https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/

Once that gets fixed
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

>> ---
>>  drivers/pci/endpoint/pci-epc-mem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
>> index 2bf8bd1f0563..d2b174ce15de 100644
>> --- a/drivers/pci/endpoint/pci-epc-mem.c
>> +++ b/drivers/pci/endpoint/pci-epc-mem.c
>> @@ -134,7 +134,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>>         if (pageno < 0)
>>                 return NULL;
>>
>> -       *phys_addr = mem->phys_base + (pageno << page_shift);
>> +       *phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
>>         virt_addr = ioremap(*phys_addr, size);
>>         if (!virt_addr)
>>                 bitmap_release_region(mem->bitmap, pageno, order);
>> --
>> 2.7.4
>>
> 
> Hi Kishon,
> 
> This issue was observed when requesting pci_epc_mem_alloc_addr()
> to allocate a region of size 0x40010000ULL (1GB + 64KB) from a
> 128GB PCI address space with page sizes being 64KB. This resulted
> in 'pageno' value of '0x8000' as the first available page in a
> contiguous region for the requested size due to other smaller
> regions having been allocated earlier. With 64KB page sizes,
> the variable 'page_shift' holds a value of 0x10. Shifting 'pageno'
> 16 bits to the left results in an 'int' value whose bit 31 is set.
> 
> [   10.565256] __pci_epc_mem_init: mem size 0x2000000000 page_size 0x10000
> [   10.571613] __pci_epc_mem_init: mem pages 0x200000 bitmap_size
> 0x40000 page_shift 0x10
> 
> PCI memory base 0x2000000000
> PCI memory size 128M 0x2000000000
> page_size 64K 0x10000
> page_shift  16 0x10
> pages 2M 0x200000
> bitmap_size 256K 0x40000
> 
> [  702.050299] pci_epc_mem_alloc_addr: size 0x10000 order 0x0 pageno
> 0x4 virt_add 0xffffffd0047b0000 phys_addr 0x2000040000
> [  702.061424] pci_epc_mem_alloc_addr: size 0x10000 order 0x0 pageno
> 0x5 virt_add 0xffffffd0047d0000 phys_addr 0x2000050000
> [  702.203933] pci_epc_mem_alloc_addr: size 0x40010000 order 0xf
> pageno 0x8000 virt_add 0xffffffd004800000 phys_addr 0x1f80000000
> [  702.216547] Oops - store (or AMO) access fault [#1]
> :::
> [  702.310198] sstatus: 0000000200000120 sbadaddr: ffffffd004804000
> scause: 0000000000000007

Thank you Alan for testing this and sending a patch to fix it.

Cheers
Kishon