[PATCH] RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t

[PATCH] RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t

[Palmer Dabbelt]

I recently ended up with a warning on some compilers along the lines of

      CC      kernel/resource.o
    In file included from include/linux/ioport.h:16,
                     from kernel/resource.c:15:
    kernel/resource.c: In function 'gfr_start':
    include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
       49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
          |                                     ^
    include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
       52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
          |         ^~~~~~~~~~~~~~~~~
    include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
      161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
          |                           ^~~~~~~~~~
    kernel/resource.c:1829:23: note: in expansion of macro 'min_t'
     1829 |                 end = min_t(resource_size_t, base->end,
          |                       ^~~~~
    kernel/resource.c: In function 'gfr_continue':
    include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
       49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
          |                                     ^
    include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
       52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
          |         ^~~~~~~~~~~~~~~~~
    include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
      161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
          |                           ^~~~~~~~~~
    kernel/resource.c:1847:24: note: in expansion of macro 'min_t'
     1847 |                addr <= min_t(resource_size_t, base->end,
          |                        ^~~~~
    cc1: all warnings being treated as errors

which looks like a real problem: our phys_addr_t is only 32 bits now, so
having 34-bit masks is just going to result in overflows.

Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
This is sort of a v2 of
https://lore.kernel.org/r/20240729151652.15063-2-palmer@rivosinc.com,
but I think that was just bogus.
---
 arch/riscv/include/asm/sparsemem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/sparsemem.h b/arch/riscv/include/asm/sparsemem.h
index 63acaecc3374..2f901a410586 100644
--- a/arch/riscv/include/asm/sparsemem.h
+++ b/arch/riscv/include/asm/sparsemem.h
@@ -7,7 +7,7 @@
 #ifdef CONFIG_64BIT
 #define MAX_PHYSMEM_BITS	56
 #else
-#define MAX_PHYSMEM_BITS	34
+#define MAX_PHYSMEM_BITS	32
 #endif /* CONFIG_64BIT */
 #define SECTION_SIZE_BITS	27
 #endif /* CONFIG_SPARSEMEM */
-- 
2.45.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t

[Charlie Jenkins]

On Wed, Jul 31, 2024 at 09:22:00AM -0700, Palmer Dabbelt wrote:
> I recently ended up with a warning on some compilers along the lines of
> 
>       CC      kernel/resource.o
>     In file included from include/linux/ioport.h:16,
>                      from kernel/resource.c:15:
>     kernel/resource.c: In function 'gfr_start':
>     include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>        49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>           |                                     ^
>     include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>        52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>           |         ^~~~~~~~~~~~~~~~~
>     include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>       161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>           |                           ^~~~~~~~~~
>     kernel/resource.c:1829:23: note: in expansion of macro 'min_t'
>      1829 |                 end = min_t(resource_size_t, base->end,
>           |                       ^~~~~
>     kernel/resource.c: In function 'gfr_continue':
>     include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>        49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>           |                                     ^
>     include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>        52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>           |         ^~~~~~~~~~~~~~~~~
>     include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>       161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>           |                           ^~~~~~~~~~
>     kernel/resource.c:1847:24: note: in expansion of macro 'min_t'
>      1847 |                addr <= min_t(resource_size_t, base->end,
>           |                        ^~~~~
>     cc1: all warnings being treated as errors
> 
> which looks like a real problem: our phys_addr_t is only 32 bits now, so
> having 34-bit masks is just going to result in overflows.
> 
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
> This is sort of a v2 of
> https://lore.kernel.org/r/20240729151652.15063-2-palmer@rivosinc.com,
> but I think that was just bogus.
> ---
>  arch/riscv/include/asm/sparsemem.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/sparsemem.h b/arch/riscv/include/asm/sparsemem.h
> index 63acaecc3374..2f901a410586 100644
> --- a/arch/riscv/include/asm/sparsemem.h
> +++ b/arch/riscv/include/asm/sparsemem.h
> @@ -7,7 +7,7 @@
>  #ifdef CONFIG_64BIT
>  #define MAX_PHYSMEM_BITS	56
>  #else
> -#define MAX_PHYSMEM_BITS	34

I was trying to figure out why this was set to 34. It looks like that
comes from this patch (which heavily changed over the course of review)
[1] and the following text:

"On the Sifive hardware this allows us to provide struct pages for
the lower I/O TileLink address ranges, the 32-bit and 34-bit DRAM areas
and 172GB of 240GB of the high I/O TileLink region. Once we progress to
Sv48 we should be able to cover all the available memory regions."

Seems like the max should be 32 though so:

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>

Link: https://lore.kernel.org/all/20190327213643.23789-4-logang@deltatee.com/ [1]


> +#define MAX_PHYSMEM_BITS	32
>  #endif /* CONFIG_64BIT */
>  #define SECTION_SIZE_BITS	27
>  #endif /* CONFIG_SPARSEMEM */
> -- 
> 2.45.2
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t

[Alexandre Ghiti]

Hi Charlie,

On 01/08/2024 03:24, Charlie Jenkins wrote:
> On Wed, Jul 31, 2024 at 09:22:00AM -0700, Palmer Dabbelt wrote:
>> I recently ended up with a warning on some compilers along the lines of
>>
>>        CC      kernel/resource.o
>>      In file included from include/linux/ioport.h:16,
>>                       from kernel/resource.c:15:
>>      kernel/resource.c: In function 'gfr_start':
>>      include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>>         49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>>            |                                     ^
>>      include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>>         52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>>            |         ^~~~~~~~~~~~~~~~~
>>      include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>>        161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>>            |                           ^~~~~~~~~~
>>      kernel/resource.c:1829:23: note: in expansion of macro 'min_t'
>>       1829 |                 end = min_t(resource_size_t, base->end,
>>            |                       ^~~~~
>>      kernel/resource.c: In function 'gfr_continue':
>>      include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>>         49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>>            |                                     ^
>>      include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>>         52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>>            |         ^~~~~~~~~~~~~~~~~
>>      include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>>        161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>>            |                           ^~~~~~~~~~
>>      kernel/resource.c:1847:24: note: in expansion of macro 'min_t'
>>       1847 |                addr <= min_t(resource_size_t, base->end,
>>            |                        ^~~~~
>>      cc1: all warnings being treated as errors
>>
>> which looks like a real problem: our phys_addr_t is only 32 bits now, so
>> having 34-bit masks is just going to result in overflows.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> ---
>> This is sort of a v2 of
>> https://lore.kernel.org/r/20240729151652.15063-2-palmer@rivosinc.com,
>> but I think that was just bogus.
>> ---
>>   arch/riscv/include/asm/sparsemem.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/sparsemem.h b/arch/riscv/include/asm/sparsemem.h
>> index 63acaecc3374..2f901a410586 100644
>> --- a/arch/riscv/include/asm/sparsemem.h
>> +++ b/arch/riscv/include/asm/sparsemem.h
>> @@ -7,7 +7,7 @@
>>   #ifdef CONFIG_64BIT
>>   #define MAX_PHYSMEM_BITS	56
>>   #else
>> -#define MAX_PHYSMEM_BITS	34
> I was trying to figure out why this was set to 34. It looks like that
> comes from this patch (which heavily changed over the course of review)
> [1] and the following text:
>
> "On the Sifive hardware this allows us to provide struct pages for
> the lower I/O TileLink address ranges, the 32-bit and 34-bit DRAM areas
> and 172GB of 240GB of the high I/O TileLink region. Once we progress to
> Sv48 we should be able to cover all the available memory regions."
>
> Seems like the max should be 32 though so:
>
> Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
>
> Link: https://lore.kernel.org/all/20190327213643.23789-4-logang@deltatee.com/ [1]


Actually sv32 supports 34-bit of physical address because of the PTE 
format which allows for 22-bits of PPN, so we should in theory set 
MAX_PHYSMEM_BITS to 34. Then it would work with CONFIG_PHYS_ADDR_T_64BIT 
enabled, but @Palmer told me that it breaks something and then it was 
disabled (I don 't have time to look into that). But even if that 
worked, we are not currently not implementing HIGHMEM so we cannot 
access all the memory anyway (and I think HIGHMEM was deprecated some 
time ago).

So, as long as someone tackles the above, I agree with this patch:

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex


>
>
>> +#define MAX_PHYSMEM_BITS	32
>>   #endif /* CONFIG_64BIT */
>>   #define SECTION_SIZE_BITS	27
>>   #endif /* CONFIG_SPARSEMEM */
>> -- 
>> 2.45.2
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t

[Palmer Dabbelt]

On Thu, 01 Aug 2024 00:35:01 PDT (-0700), alex@ghiti.fr wrote:
> Hi Charlie,
>
> On 01/08/2024 03:24, Charlie Jenkins wrote:
>> On Wed, Jul 31, 2024 at 09:22:00AM -0700, Palmer Dabbelt wrote:
>>> I recently ended up with a warning on some compilers along the lines of
>>>
>>>        CC      kernel/resource.o
>>>      In file included from include/linux/ioport.h:16,
>>>                       from kernel/resource.c:15:
>>>      kernel/resource.c: In function 'gfr_start':
>>>      include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>>>         49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>>>            |                                     ^
>>>      include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>>>         52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>>>            |         ^~~~~~~~~~~~~~~~~
>>>      include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>>>        161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>>>            |                           ^~~~~~~~~~
>>>      kernel/resource.c:1829:23: note: in expansion of macro 'min_t'
>>>       1829 |                 end = min_t(resource_size_t, base->end,
>>>            |                       ^~~~~
>>>      kernel/resource.c: In function 'gfr_continue':
>>>      include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>>>         49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>>>            |                                     ^
>>>      include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>>>         52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>>>            |         ^~~~~~~~~~~~~~~~~
>>>      include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>>>        161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>>>            |                           ^~~~~~~~~~
>>>      kernel/resource.c:1847:24: note: in expansion of macro 'min_t'
>>>       1847 |                addr <= min_t(resource_size_t, base->end,
>>>            |                        ^~~~~
>>>      cc1: all warnings being treated as errors
>>>
>>> which looks like a real problem: our phys_addr_t is only 32 bits now, so
>>> having 34-bit masks is just going to result in overflows.
>>>
>>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>> ---
>>> This is sort of a v2 of
>>> https://lore.kernel.org/r/20240729151652.15063-2-palmer@rivosinc.com,
>>> but I think that was just bogus.
>>> ---
>>>   arch/riscv/include/asm/sparsemem.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/include/asm/sparsemem.h b/arch/riscv/include/asm/sparsemem.h
>>> index 63acaecc3374..2f901a410586 100644
>>> --- a/arch/riscv/include/asm/sparsemem.h
>>> +++ b/arch/riscv/include/asm/sparsemem.h
>>> @@ -7,7 +7,7 @@
>>>   #ifdef CONFIG_64BIT
>>>   #define MAX_PHYSMEM_BITS	56
>>>   #else
>>> -#define MAX_PHYSMEM_BITS	34
>> I was trying to figure out why this was set to 34. It looks like that
>> comes from this patch (which heavily changed over the course of review)
>> [1] and the following text:
>>
>> "On the Sifive hardware this allows us to provide struct pages for
>> the lower I/O TileLink address ranges, the 32-bit and 34-bit DRAM areas
>> and 172GB of 240GB of the high I/O TileLink region. Once we progress to
>> Sv48 we should be able to cover all the available memory regions."
>>
>> Seems like the max should be 32 though so:
>>
>> Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
>>
>> Link: https://lore.kernel.org/all/20190327213643.23789-4-logang@deltatee.com/ [1]
>
>
> Actually sv32 supports 34-bit of physical address because of the PTE
> format which allows for 22-bits of PPN, so we should in theory set
> MAX_PHYSMEM_BITS to 34. Then it would work with CONFIG_PHYS_ADDR_T_64BIT
> enabled, but @Palmer told me that it breaks something and then it was
> disabled (I don 't have time to look into that). But even if that

IIRC the breakages were nothing exciting, just the usual pile of drivers 
that didn't support the 64-bit phys_addr_t on 32-bit targets.  There 
aren't a lot of 32-bit targets left these days, so rv32 picks up a bunch 
of noise.

> worked, we are not currently not implementing HIGHMEM so we cannot
> access all the memory anyway (and I think HIGHMEM was deprecated some
> time ago).
>
> So, as long as someone tackles the above, I agree with this patch:

You mean until someone tackles the above?  IIUC we'd end up broken 
without this patch, if anyone had 34-bit PAs they'd just get truncated.

>
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>
> Thanks,
>
> Alex
>
>
>>
>>
>>> +#define MAX_PHYSMEM_BITS	32
>>>   #endif /* CONFIG_64BIT */
>>>   #define SECTION_SIZE_BITS	27
>>>   #endif /* CONFIG_SPARSEMEM */
>>> --
>>> 2.45.2
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed, 31 Jul 2024 09:22:00 -0700 you wrote:
> I recently ended up with a warning on some compilers along the lines of
> 
>       CC      kernel/resource.o
>     In file included from include/linux/ioport.h:16,
>                      from kernel/resource.c:15:
>     kernel/resource.c: In function 'gfr_start':
>     include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>        49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>           |                                     ^
>     include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>        52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>           |         ^~~~~~~~~~~~~~~~~
>     include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>       161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>           |                           ^~~~~~~~~~
>     kernel/resource.c:1829:23: note: in expansion of macro 'min_t'
>      1829 |                 end = min_t(resource_size_t, base->end,
>           |                       ^~~~~
>     kernel/resource.c: In function 'gfr_continue':
>     include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>        49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>           |                                     ^
>     include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>        52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>           |         ^~~~~~~~~~~~~~~~~
>     include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>       161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>           |                           ^~~~~~~~~~
>     kernel/resource.c:1847:24: note: in expansion of macro 'min_t'
>      1847 |                addr <= min_t(resource_size_t, base->end,
>           |                        ^~~~~
>     cc1: all warnings being treated as errors
> 
> [...]

Here is the summary with links:
  - RISC-V: Don't have MAX_PHYSMEM_BITS exceed phys_addr_t
    https://git.kernel.org/riscv/c/ad380f6a0a5e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv