hw/ppc/ppc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
The current way the mask is built can overflow with a 64-bit decrementer.
Use MAKE_64BIT_MASK instead.
Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
This was found with the QEMU Microwatt machine which uses a 64bit
decrementer. Here is an experimental tree:
https://github.com/legoater/qemu/tree/microwatt
hw/ppc/ppc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 7375bf4fa910..a86125c50ff9 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -876,7 +876,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
bool negative;
/* Truncate value to decr_width and sign extend for simplicity */
- value &= ((1ULL << nr_bits) - 1);
+ value &= MAKE_64BIT_MASK(0, nr_bits);
negative = !!(value & (1ULL << (nr_bits - 1)));
if (negative) {
value |= (0xFFFFFFFFULL << nr_bits);
--
2.31.1
On 9/13/21 6:27 PM, Cédric Le Goater wrote: > The current way the mask is built can overflow with a 64-bit decrementer. > Use MAKE_64BIT_MASK instead. > > Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG") > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > This was found with the QEMU Microwatt machine which uses a 64bit > decrementer. Here is an experimental tree: > > https://github.com/legoater/qemu/tree/microwatt > > hw/ppc/ppc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index 7375bf4fa910..a86125c50ff9 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -876,7 +876,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp, > bool negative; > > /* Truncate value to decr_width and sign extend for simplicity */ > - value &= ((1ULL << nr_bits) - 1); > + value &= MAKE_64BIT_MASK(0, nr_bits); What about: value = extract64(value, 0, nr_bits); if (value != sextract64(value, 0, nr_bits)) { ... > negative = !!(value & (1ULL << (nr_bits - 1))); > if (negative) { > value |= (0xFFFFFFFFULL << nr_bits); >
> > bool negative; > > > > /* Truncate value to decr_width and sign extend for simplicity */ > > - value &= ((1ULL << nr_bits) - 1); > > + value &= MAKE_64BIT_MASK(0, nr_bits); > > What about: > > value = extract64(value, 0, nr_bits); > if (value != sextract64(value, 0, nr_bits)) { ... Or: value = extract64(value, 0, nr_bits); value = ((target_long)value << (64 - nr_bits)) >> (64 - nr_bits); Also avoiding the problem with an invalid 64-bit shift with: > > value |= (0xFFFFFFFFULL << nr_bits); -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
> value = extract64(value, 0, nr_bits); > value = ((target_long)value << (64 - nr_bits)) >> (64 - nr_bits); Oops, sorry. 64 might not be correct here. It would depend on the target being either 32 or 64. -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
> > value = extract64(value, 0, nr_bits); > > value = ((target_long)value << (64 - nr_bits)) >> (64 - nr_bits); > > Oops, sorry. 64 might not be correct here. It would depend on the target being > either 32 or 64. In fact, sextract already does the sign extension, so this should be all that's needed, right? value = sextract<32,64>(value, 0, nr_bits); -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On 9/13/21 8:05 PM, Luis Fernando Fujita Pires wrote: >>> value = extract64(value, 0, nr_bits); >>> value = ((target_long)value << (64 - nr_bits)) >> (64 - nr_bits); >> >> Oops, sorry. 64 might not be correct here. It would depend on the target being >> either 32 or 64. > > In fact, sextract already does the sign extension, so this should be all that's needed, right? > value = sextract<32,64>(value, 0, nr_bits); I am fine with any solution ! Please give a try to this machine : https://github.com/legoater/qemu/tree/microwatt It's the only one with a 64 bit decrementer :) (We should come up with a simpler test case) Thanks, C.
On Mon, 13 Sept 2021 at 19:09, Luis Fernando Fujita Pires <luis.pires@eldorado.org.br> wrote: > > > > value = extract64(value, 0, nr_bits); > > > value = ((target_long)value << (64 - nr_bits)) >> (64 - nr_bits); > > > > Oops, sorry. 64 might not be correct here. It would depend on the target being > > either 32 or 64. > > In fact, sextract already does the sign extension, so this should be all that's needed, right? > value = sextract<32,64>(value, 0, nr_bits); Indeed, sextract64() is the preferred way to do a sign extension. (The one thing to watch out for is that you mustn't try to extract a zero-width field; it will assert if you do. It also asserts if you specify a field whose start,length would put either end to the left of bit 63 or the right of bit 0, but that's less likely than the zero-width case.) -- PMM
© 2016 - 2024 Red Hat, Inc.