[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 at 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 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

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