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

[Andrew Jones <ajones@ventanamicro.com>]

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.

Thanks,
drew