[PATCH] riscv: set the width of the physical address/size data type based on arch

[Sughosh Ganu]

On Tue, 6 May 2025 at 15:19, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 5/6/25 11:24, Sughosh Ganu wrote:
> > U-Boot has support for both the 32-bit and 64-bit RiscV platforms. Set
> > the width of the phys_{addr,size}_t data types based on the register
> > size of the architecture.
> >
> > Currently, even the 32-bit RiscV platforms have a 64-bit
> > phys_{addr,size}_t data types. This causes issues on the 32-bit
> > platforms, where the upper 32-bits of the variables of these types
> > can have junk data, and that can cause all kinds of side-effects.
>
> How could it be that the upper 32-bit have junk data?
>
> When we convert from a shorter variable the compiler should fill the
> upper bits with zero.

That does not seem to be happening. The efi_fit test fails on the
qemu-riscv32 platform, when attempting to boot the OS from the FIT
image.

These are the values of the base address that I see in the
_lmb_alloc_addr() function.

_lmb_alloc_addr: 755, rgn => -1, base => 0x1a1c0e00802000bc, size => 0x50b1

The actual value that gets passed is 0x802000bc. The upper 32-bits do
not get zeroed out. Which causes the _lmb_alloc_addr() to return an
error. You can check my branch [1], where I have put a temporary
commit to print the values that cause the issue.

-sughosh

[1] - https://source.denx.de/u-boot/contributors/sng/u-boot/-/commits/lmb_apis_cleanup_bisectable_v2

>
> >
> > This was discovered on the qemu Riscv 32-bit platform  when the return
> > value of an LMB API was checked, and some LMB allocation that ought
> > not to have failed, was failing. The upper 32-bits of the address
> > variable contained garbage, resulting in failures.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> >
> > Note:
> > Although the LMB API cleanup series depends on this patch, I am
> > sending it separately so that it gets noticed by the RiscV
> > maintainers. Sometimes a patch may not get the required attention when
> > sent as part of another seemingly unrelated series.
> >
> >
> >   arch/riscv/include/asm/types.h | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/types.h b/arch/riscv/include/asm/types.h
> > index 49f7a5d6b3a..45d806c83eb 100644
> > --- a/arch/riscv/include/asm/types.h
> > +++ b/arch/riscv/include/asm/types.h
> > @@ -35,8 +35,13 @@ typedef u64 dma_addr_t;
> >   typedef u32 dma_addr_t;
> >   #endif
> >
> > -typedef unsigned long long phys_addr_t;
> > -typedef unsigned long long phys_size_t;
> > +#ifdef CONFIG_PHYS_64BIT
> > +typedef u64 phys_addr_t;
> > +typedef u64 phys_size_t;
> > +#else
> > +typedef u32 phys_addr_t;
> > +typedef u32 phys_size_t;
>
> This matches what has been done for ARM.
>
> 86c915628d58 ("riscv: Change phys_addr_t and phys_size_t to 64-bit")
> changed the definition to 64bit phys addr_t to support the 34bit
> physical addresses (similar to LPAE on arm32).
>
> I don't think that we need 34bit support currently. But I would prefer
> if we could find the root cause why the upper 32bit gets messed up as
> this might point to a generic problem.
>
> Best regards
>
> Heinrich
>
> > +#endif
> >
> >   #endif /* __KERNEL__ */
> >
>