Re: [PATCH] target/riscv32: Fix masking of physical address

[Andrew Jones <ajones@ventanamicro.com>]

On Tue, Aug 13, 2024 at 10:21:13AM GMT, Philippe Mathieu-Daudé wrote:
> On 13/8/24 10:00, Andrew Jones wrote:
> > On Tue, Aug 13, 2024 at 05:43:07PM GMT, Richard Henderson wrote:
> > > On 8/13/24 17:13, Andrew Jones wrote:
> > > > C doesn't extend the sign bit for unsigned types since there isn't a
> > > > sign bit to extend. This means a promotion of a u32 to a u64 results
> > > > in the upper 32 bits of the u64 being zero. If that result is then
> > > > used as a mask on another u64 the upper 32 bits will be cleared. rv32
> > > > physical addresses may be up to 34 bits wide, so we don't want to
> > > > clear the high bits while page aligning the address. The fix is to
> > > > revert to using target_long, since a signed type will get extended.
> > > > 
> > > > Fixes: af3fc195e3c8 ("target/riscv: Change the TLB page size depends on PMP entries.")
> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > ---
> > > >    target/riscv/cpu_helper.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > index 395a1d914061..dfef1b20d1e8 100644
> > > > --- a/target/riscv/cpu_helper.c
> > > > +++ b/target/riscv/cpu_helper.c
> > > > @@ -1323,7 +1323,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > > >        int ret = TRANSLATE_FAIL;
> > > >        int mode = mmuidx_priv(mmu_idx);
> > > >        /* default TLB page size */
> > > > -    target_ulong tlb_size = TARGET_PAGE_SIZE;
> > > > +    target_long tlb_size = TARGET_PAGE_SIZE;
> > > 
> > > If rv32 physical addresses are 34 bits, then you probably didn't want target_*long at all.
> > 
> > Yes, just using hwaddr for everything that only touches physical addresses
> > would probably be best, but, ifaict, it's pretty common to use target_long
> > for masks used on both virtual and physical addresses (TARGET_PAGE_MASK,
> > for example). This 'tlb_size' variable is used on both as well.
> 
> Then maybe you want vaddr ("exec/vaddr.h"):
> 
> /**
>  * vaddr:
>  * Type wide enough to contain any #target_ulong virtual address.
>  */
>

I think hwaddr would fit better in this case since riscv32 virtual
addresses are 32-bit, but I see vaddr is a u64, so it would work too. I
personally don't mind changing the type of tlb_size to hwaddr, but I went
with target_long in this patch since that's what it was originally and
masking with a signed long mask appears to be a common pattern in QEMU.

Thanks,
drew