* [PATCH v2] target/ppc: Fix 64-bit decrementer
@ 2021-09-14 8:54 Cédric Le Goater
2021-09-14 9:19 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Cédric Le Goater @ 2021-09-14 8:54 UTC (permalink / raw)
To: David Gibson, Greg Kurz
Cc: qemu-ppc, Luis Fernando Fujita Pires, Philippe Mathieu-Daudé,
qemu-devel, Cédric Le Goater
The current way the mask is built can overflow with a 64-bit decrementer.
Use sextract64() instead.
Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
v2: replaced MAKE_64BIT_MASK by sextract64
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..4f14464c9220 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 = sextract64(value, 0, nr_bits);
negative = !!(value & (1ULL << (nr_bits - 1)));
if (negative) {
value |= (0xFFFFFFFFULL << nr_bits);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] target/ppc: Fix 64-bit decrementer
2021-09-14 8:54 [PATCH v2] target/ppc: Fix 64-bit decrementer Cédric Le Goater
@ 2021-09-14 9:19 ` Peter Maydell
2021-09-14 9:47 ` Cédric Le Goater
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-09-14 9:19 UTC (permalink / raw)
To: Cédric Le Goater
Cc: QEMU Developers, Greg Kurz, Luis Fernando Fujita Pires, qemu-ppc,
Philippe Mathieu-Daudé,
David Gibson
On Tue, 14 Sept 2021 at 09:56, Cédric Le Goater <clg@kaod.org> wrote:
>
> The current way the mask is built can overflow with a 64-bit decrementer.
> Use sextract64() instead.
>
> Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
> v2: replaced MAKE_64BIT_MASK by sextract64
>
> 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..4f14464c9220 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 = sextract64(value, 0, nr_bits);
> negative = !!(value & (1ULL << (nr_bits - 1)));
> if (negative) {
> value |= (0xFFFFFFFFULL << nr_bits);
I think these lines that say "if negative then force all the
high bits to one" are also no longer required. That is, this
entire section of code:
value &= ((1ULL << nr_bits) - 1);
negative = !!(value & (1ULL << (nr_bits - 1)));
if (negative) {
value |= (0xFFFFFFFFULL << nr_bits);
}
is an open-coded sign-extension, which can all be replaced with
the single line
value = sextract64(value, 0, nr_bits);
(and also: if nr_bits is 64 then the "<< nr_bits"
is undefined behaviour.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] target/ppc: Fix 64-bit decrementer
2021-09-14 9:19 ` Peter Maydell
@ 2021-09-14 9:47 ` Cédric Le Goater
2021-09-14 10:23 ` Peter Maydell
2021-09-14 13:21 ` Richard Henderson
0 siblings, 2 replies; 7+ messages in thread
From: Cédric Le Goater @ 2021-09-14 9:47 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Greg Kurz, Luis Fernando Fujita Pires, qemu-ppc,
Philippe Mathieu-Daudé,
David Gibson
On 9/14/21 11:19 AM, Peter Maydell wrote:
> On Tue, 14 Sept 2021 at 09:56, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The current way the mask is built can overflow with a 64-bit decrementer.
>> Use sextract64() instead.
>>
>> Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
>> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>> v2: replaced MAKE_64BIT_MASK by sextract64
>>
>> 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..4f14464c9220 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 = sextract64(value, 0, nr_bits);
>> negative = !!(value & (1ULL << (nr_bits - 1)));
>> if (negative) {
>> value |= (0xFFFFFFFFULL << nr_bits);
>
> I think these lines that say "if negative then force all the
> high bits to one" are also no longer required. That is, this
> entire section of code:
> value &= ((1ULL << nr_bits) - 1);
> negative = !!(value & (1ULL << (nr_bits - 1)));
> if (negative) {
> value |= (0xFFFFFFFFULL << nr_bits);
> }
>
> is an open-coded sign-extension, which can all be replaced with
> the single line
> value = sextract64(value, 0, nr_bits);
'negative' is used for more tests afterwards but you are right. I will respin
with more changes.
I am reluctant in changing too much because this is common code for PPC32
and PPC64. But, hey, I will do my best with the images I have.
> (and also: if nr_bits is 64 then the "<< nr_bits"
> is undefined behaviour.)
That's the initial issue raised by the new little PPC FPGA softcore called
microwatt as it's using a 64bit decrementer.
Thanks,
C.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] target/ppc: Fix 64-bit decrementer
2021-09-14 9:47 ` Cédric Le Goater
@ 2021-09-14 10:23 ` Peter Maydell
2021-09-14 13:21 ` Richard Henderson
1 sibling, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2021-09-14 10:23 UTC (permalink / raw)
To: Cédric Le Goater
Cc: QEMU Developers, Greg Kurz, Luis Fernando Fujita Pires, qemu-ppc,
Philippe Mathieu-Daudé,
David Gibson
On Tue, 14 Sept 2021 at 10:47, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 9/14/21 11:19 AM, Peter Maydell wrote:
> > On Tue, 14 Sept 2021 at 09:56, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> The current way the mask is built can overflow with a 64-bit decrementer.
> >> Use sextract64() instead.
> >>
> >> Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
> >> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >> v2: replaced MAKE_64BIT_MASK by sextract64
> >>
> >> 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..4f14464c9220 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 = sextract64(value, 0, nr_bits);
> >> negative = !!(value & (1ULL << (nr_bits - 1)));
> >> if (negative) {
> >> value |= (0xFFFFFFFFULL << nr_bits);
> >
> > I think these lines that say "if negative then force all the
> > high bits to one" are also no longer required. That is, this
> > entire section of code:
> > value &= ((1ULL << nr_bits) - 1);
> > negative = !!(value & (1ULL << (nr_bits - 1)));
> > if (negative) {
> > value |= (0xFFFFFFFFULL << nr_bits);
> > }
> >
> > is an open-coded sign-extension, which can all be replaced with
> > the single line
> > value = sextract64(value, 0, nr_bits);
>
> 'negative' is used for more tests afterwards but you are right. I will respin
> with more changes.
After the sign-extension you can set 'negative' with
negative = ((target_long)value) < 0;
PS: passing a negative value into muldiv64() to set the next
timer event (as the existing code does in some cases) seems a bit
odd; this probably ends up with an arithmetic overflow and setting
the next timeout to something unintended. But that's a separate
issue from the bug you're dealing with here.
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] target/ppc: Fix 64-bit decrementer
2021-09-14 9:47 ` Cédric Le Goater
2021-09-14 10:23 ` Peter Maydell
@ 2021-09-14 13:21 ` Richard Henderson
2021-09-14 13:43 ` Cédric Le Goater
1 sibling, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2021-09-14 13:21 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: QEMU Developers, Greg Kurz, Luis Fernando Fujita Pires, qemu-ppc,
Philippe Mathieu-Daudé,
David Gibson
On 9/14/21 2:47 AM, Cédric Le Goater wrote:
> On 9/14/21 11:19 AM, Peter Maydell wrote:
>>> /* Truncate value to decr_width and sign extend for simplicity */
>>> - value &= ((1ULL << nr_bits) - 1);
>>> + value = sextract64(value, 0, nr_bits);
>>> negative = !!(value & (1ULL << (nr_bits - 1)));
>>> if (negative) {
>>> value |= (0xFFFFFFFFULL << nr_bits);
>>
>> I think these lines that say "if negative then force all the
>> high bits to one" are also no longer required. That is, this
>> entire section of code:
>> value &= ((1ULL << nr_bits) - 1);
>> negative = !!(value & (1ULL << (nr_bits - 1)));
>> if (negative) {
>> value |= (0xFFFFFFFFULL << nr_bits);
>> }
>>
>> is an open-coded sign-extension, which can all be replaced with
>> the single line
>> value = sextract64(value, 0, nr_bits);
>
> 'negative' is used for more tests afterwards but you are right. I will respin
> with more changes.
After the sign-extension, negative needs no complicated expression.
value = sextract64(value, 0, nr_bits);
negative = (target_long)value < 0;
r~
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] target/ppc: Fix 64-bit decrementer
2021-09-14 13:21 ` Richard Henderson
@ 2021-09-14 13:43 ` Cédric Le Goater
2021-09-14 14:41 ` Richard Henderson
0 siblings, 1 reply; 7+ messages in thread
From: Cédric Le Goater @ 2021-09-14 13:43 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell
Cc: QEMU Developers, Greg Kurz, Luis Fernando Fujita Pires, qemu-ppc,
Philippe Mathieu-Daudé,
David Gibson
On 9/14/21 3:21 PM, Richard Henderson wrote:
> On 9/14/21 2:47 AM, Cédric Le Goater wrote:
>> On 9/14/21 11:19 AM, Peter Maydell wrote:
>>>> /* Truncate value to decr_width and sign extend for simplicity */
>>>> - value &= ((1ULL << nr_bits) - 1);
>>>> + value = sextract64(value, 0, nr_bits);
>>>> negative = !!(value & (1ULL << (nr_bits - 1)));
>>>> if (negative) {
>>>> value |= (0xFFFFFFFFULL << nr_bits);
>>>
>>> I think these lines that say "if negative then force all the
>>> high bits to one" are also no longer required. That is, this
>>> entire section of code:
>>> value &= ((1ULL << nr_bits) - 1);
>>> negative = !!(value & (1ULL << (nr_bits - 1)));
>>> if (negative) {
>>> value |= (0xFFFFFFFFULL << nr_bits);
>>> }
>>>
>>> is an open-coded sign-extension, which can all be replaced with
>>> the single line
>>> value = sextract64(value, 0, nr_bits);
>>
>> 'negative' is used for more tests afterwards but you are right. I will respin
>> with more changes.
>
> After the sign-extension, negative needs no complicated expression.
>
> value = sextract64(value, 0, nr_bits);
> negative = (target_long)value < 0;
Yes. I was wondering about the 'target_ulong' type used for the value
and decr variables. The code has below :
...
if ((value < 3) ||
...
and then another sign calculation on a target_ulong
...
&& !(decr & (1ULL << (nr_bits - 1))))) {
...
We should introduce intermediate 'int64_t' variables to extract the
sign values from the target_ulong. That would be cleaner.
Thanks,
C.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] target/ppc: Fix 64-bit decrementer
2021-09-14 13:43 ` Cédric Le Goater
@ 2021-09-14 14:41 ` Richard Henderson
0 siblings, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2021-09-14 14:41 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: QEMU Developers, Greg Kurz, Luis Fernando Fujita Pires, qemu-ppc,
Philippe Mathieu-Daudé,
David Gibson
On 9/14/21 6:43 AM, Cédric Le Goater wrote:
> and then another sign calculation on a target_ulong
>
> ...
> && !(decr & (1ULL << (nr_bits - 1))))) {
I was wondering if that was supposed to be an unsigned test for a "small" value (i.e. decr
< MAKE_64BIT_MASK(0, nr_bits)? Certainly decr should never be negative, since the
decrementer never increments, and I can't figure out what it's supposed to mean otherwise.
> We should introduce intermediate 'int64_t' variables to extract the
> sign values from the target_ulong. That would be cleaner.
Yes it would. The underflow test becomes easier for certain.
r~
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-14 15:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 8:54 [PATCH v2] target/ppc: Fix 64-bit decrementer Cédric Le Goater
2021-09-14 9:19 ` Peter Maydell
2021-09-14 9:47 ` Cédric Le Goater
2021-09-14 10:23 ` Peter Maydell
2021-09-14 13:21 ` Richard Henderson
2021-09-14 13:43 ` Cédric Le Goater
2021-09-14 14:41 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.