[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 at 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 at 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 at 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv