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(-)

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

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
>
>

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
>
>

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~