| Message ID | 20240813071355.310710-2-ajones@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv32: Fix masking of physical address | expand |
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. r~
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
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. */
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
On Tue, Aug 13, 2024 at 8:24 PM Andrew Jones <ajones@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 <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. hwaddr seems like the right approach here Alistair > > Thanks, > drew >
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; env->guest_phys_fault_addr = 0;
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(-)