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

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

[Andrew Jones]

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;
 
     env->guest_phys_fault_addr = 0;
 
-- 
2.45.2

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

[Richard Henderson]

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~

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

[Andrew Jones]

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

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

[Philippe Mathieu-Daudé]

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.
  */

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

[Andrew Jones]

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

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

[Alistair Francis]

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
>