On Tue, Aug 13, 2024 at 8:24 PM Andrew Jones <ajo...@ventanamicro.com> wrote:
>
> 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 <ajo...@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.
hwaddr seems like the right approach here
Alistair
>
> Thanks,
> drew
>
