* Re: [PATCH v2] target/riscv32: Fix masking of physical address
2024-09-09 8:32 [PATCH v2] target/riscv32: Fix masking of physical address Andrew Jones
@ 2024-09-09 15:46 ` Richard Henderson
2024-09-10 3:03 ` Alistair Francis
2024-09-10 3:46 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Richard Henderson @ 2024-09-09 15:46 UTC (permalink / raw)
To: Andrew Jones, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bmeng.cn, zong.li, liwei1518, cwshu,
dbarboza, philmd
On 9/9/24 01:32, 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
> use hwaddr for the mask, which, even on rv32, is 64-bits wide.
>
> Fixes: af3fc195e3c8 ("target/riscv: Change the TLB page size depends on PMP entries.")
> Signed-off-by: Andrew Jones<ajones@ventanamicro.com>
> ---
> -v2: Switch from signed long to hwaddr
>
> target/riscv/cpu_helper.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] target/riscv32: Fix masking of physical address
2024-09-09 8:32 [PATCH v2] target/riscv32: Fix masking of physical address Andrew Jones
2024-09-09 15:46 ` Richard Henderson
@ 2024-09-10 3:03 ` Alistair Francis
2024-09-10 3:46 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2024-09-10 3:03 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
zong.li, liwei1518, cwshu, dbarboza, richard.henderson, philmd
On Mon, Sep 9, 2024 at 6:33 PM Andrew Jones <ajones@ventanamicro.com> 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
> use hwaddr for the mask, which, even on rv32, is 64-bits wide.
>
> Fixes: af3fc195e3c8 ("target/riscv: Change the TLB page size depends on PMP entries.")
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> -v2: Switch from signed long to hwaddr
>
> target/riscv/cpu_helper.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 395a1d914061..4b2c72780c36 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;
> + hwaddr tlb_size = TARGET_PAGE_SIZE;
>
> env->guest_phys_fault_addr = 0;
>
> @@ -1375,7 +1375,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
> qemu_log_mask(CPU_LOG_MMU,
> "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> - " %d tlb_size " TARGET_FMT_lu "\n",
> + " %d tlb_size %" HWADDR_PRIu "\n",
> __func__, pa, ret, prot_pmp, tlb_size);
>
> prot &= prot_pmp;
> @@ -1409,7 +1409,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
> qemu_log_mask(CPU_LOG_MMU,
> "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> - " %d tlb_size " TARGET_FMT_lu "\n",
> + " %d tlb_size %" HWADDR_PRIu "\n",
> __func__, pa, ret, prot_pmp, tlb_size);
>
> prot &= prot_pmp;
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] target/riscv32: Fix masking of physical address
2024-09-09 8:32 [PATCH v2] target/riscv32: Fix masking of physical address Andrew Jones
2024-09-09 15:46 ` Richard Henderson
2024-09-10 3:03 ` Alistair Francis
@ 2024-09-10 3:46 ` Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2024-09-10 3:46 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bmeng.cn,
zong.li, liwei1518, cwshu, dbarboza, richard.henderson, philmd
On Mon, Sep 9, 2024 at 6:33 PM Andrew Jones <ajones@ventanamicro.com> 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
> use hwaddr for the mask, which, even on rv32, is 64-bits wide.
>
> Fixes: af3fc195e3c8 ("target/riscv: Change the TLB page size depends on PMP entries.")
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> -v2: Switch from signed long to hwaddr
>
> target/riscv/cpu_helper.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 395a1d914061..4b2c72780c36 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;
> + hwaddr tlb_size = TARGET_PAGE_SIZE;
>
> env->guest_phys_fault_addr = 0;
>
> @@ -1375,7 +1375,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
> qemu_log_mask(CPU_LOG_MMU,
> "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> - " %d tlb_size " TARGET_FMT_lu "\n",
> + " %d tlb_size %" HWADDR_PRIu "\n",
> __func__, pa, ret, prot_pmp, tlb_size);
>
> prot &= prot_pmp;
> @@ -1409,7 +1409,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
> qemu_log_mask(CPU_LOG_MMU,
> "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> - " %d tlb_size " TARGET_FMT_lu "\n",
> + " %d tlb_size %" HWADDR_PRIu "\n",
> __func__, pa, ret, prot_pmp, tlb_size);
>
> prot &= prot_pmp;
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread