The current way the mask is built can overflow with a 64-bit decrementer.
Use sextract64() to extract the signed values and remove the logic to
handle negative values which has become useless.
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>
---
Tested on pSeries, PowerNV, MAC and MicroWatt (64bit dec) platforms.
hw/ppc/ppc.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 7375bf4fa910..777e5756edfa 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -873,17 +873,15 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
CPUPPCState *env = &cpu->env;
ppc_tb_t *tb_env = env->tb_env;
uint64_t now, next;
- bool negative;
+ target_long signed_value;
+ target_long signed_decr;
/* Truncate value to decr_width and sign extend for simplicity */
- value &= ((1ULL << nr_bits) - 1);
- negative = !!(value & (1ULL << (nr_bits - 1)));
- if (negative) {
- value |= (0xFFFFFFFFULL << nr_bits);
- }
+ signed_value = sextract64(value, 0, nr_bits);
+ signed_decr = sextract64(decr, 0, nr_bits);
LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
- decr, value);
+ decr, signed_value);
if (kvm_enabled()) {
/* KVM handles decrementer exceptions, we don't need our own timer */
@@ -903,16 +901,16 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
* On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
* an edge interrupt, so raise it here too.
*/
- if ((value < 3) ||
- ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && negative) ||
- ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && negative
- && !(decr & (1ULL << (nr_bits - 1))))) {
+ if ((signed_value < 3) ||
+ ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
+ ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
+ && signed_decr >= 0)) {
(*raise_excp)(cpu);
return;
}
/* On MSB level based systems a 0 for the MSB stops interrupt delivery */
- if (!negative && (tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL)) {
+ if (signed_value >= 0 && (tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL)) {
(*lower_excp)(cpu);
}
--
2.31.1
Hi Cédric,
These changes look good and seem to replicate the exact behavior we had before, while fixing the 64-bit dec from MicroWatt.
A few notes, in case they help others, too:
According to the Power ISA:
When not in Large Decrementer mode:
- the maximum positive value for the decrementer is the maximum possible signed 32-bit int (2^31 - 1)
- the minimum possible value for the decrementer is 0x00000000FFFFFFFF
When in Large Decrementer mode:
- the maximum positive value for the decrementer is 2^(nr_bits - 1) - 1
- the minimum possible value for the decrementer is 0xFFFFFFFFFFFFFFFF
And the decrementer is decremented until its value becomes 0, and then once more, to the minimum possible value (0x00000000FFFFFFFF or 0xFFFFFFFFFFFFFFFF, depending on the Large Decrementer mode).
From what I understood from the code, the 'decr' value passed to __cpu_ppc_store_decr is the value of DECR before the change, and 'value' is the new one.
Now, back to the code... :)
> + target_long signed_value;
> + target_long signed_decr;
Since these values will be the results of sextract64, it's probably better to define them as int64_t.
> LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
> - decr, value);
> + decr, signed_value);
While this reproduces the behavior we previously had, I think it would be more consistent if we logged what we had before the sign-extension ('value' instead of 'signed_value'). And 'decr' is okay, which is also not sign-extended.
> ||
> + ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value
> < 0
> + && signed_decr >= 0)) {
The 'signed_decr >= 0' is ok. It catches the case where the previous value (now sign-extended as signed_decr) was >= 0 and we signed_value just got negative (which should be exactly 0xFFFFFFFFFFFFFF, after being sign-extended to 64 bits).
One point that I found odd, but not directly related to your patch, is the implementation of _cpu_ppc_load_decr:
static inline int64_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
{
ppc_tb_t *tb_env = env->tb_env;
int64_t decr, diff;
diff = next - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
if (diff >= 0) {
decr = muldiv64(diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
} else if (tb_env->flags & PPC_TIMER_BOOKE) {
decr = 0;
} else {
decr = -muldiv64(-diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
}
LOG_TB("%s: %016" PRIx64 "\n", __func__, decr);
return decr;
}
The diff < 0 case:
decr = -muldiv64(-diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
should probably just be:
decr = -1;
to comply with the minimum possible values specified by the ISA.
But, again, this doesn't impact your patch directly.
And also:
Reviewed-by: Luis Pires <luis.pires@eldorado.org.br>
--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Hello,
On 9/16/21 20:22, Luis Fernando Fujita Pires wrote:
> Hi Cédric,
>
> These changes look good and seem to replicate the exact behavior we had before, while fixing the 64-bit dec from MicroWatt.
Yes. That's the goal. I would like to minimize the possible impact on other
platforms. I checked MAC, pSeries and PowerNV but there are many more.
> A few notes, in case they help others, too:
>
> According to the Power ISA:
> When not in Large Decrementer mode:
> - the maximum positive value for the decrementer is the maximum possible signed 32-bit int (2^31 - 1)
> - the minimum possible value for the decrementer is 0x00000000FFFFFFFF
> When in Large Decrementer mode:
> - the maximum positive value for the decrementer is 2^(nr_bits - 1) - 1
> - the minimum possible value for the decrementer is 0xFFFFFFFFFFFFFFFF
>
> And the decrementer is decremented until its value becomes 0, and then once more, to the minimum possible value (0x00000000FFFFFFFF or 0xFFFFFFFFFFFFFFFF, depending on the Large Decrementer mode).
>
> From what I understood from the code, the 'decr' value passed to __cpu_ppc_store_decr is the value of DECR before the change, and 'value' is the new one.
>
> Now, back to the code... :)
>
>> + target_long signed_value;
>> + target_long signed_decr;
>
> Since these values will be the results of sextract64, it's probably better to define them as int64_t.
but then it breaks the code doing the logging on PPC32 targets :/
>
>> LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
>> - decr, value);
>> + decr, signed_value);
>
> While this reproduces the behavior we previously had, I think it would be more consistent if we logged what we had before the sign-extension ('value' instead of 'signed_value'). And 'decr' is okay, which is also not sign-extended.
>
>> ||
>> + ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value
>> < 0
>> + && signed_decr >= 0)) {
>
> The 'signed_decr >= 0' is ok. It catches the case where the previous value (now sign-extended as signed_decr) was >= 0 and we signed_value just got negative (which should be exactly 0xFFFFFFFFFFFFFF, after being sign-extended to 64 bits).
>
> One point that I found odd, but not directly related to your patch, is the implementation of _cpu_ppc_load_decr:
>
> static inline int64_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
> {
> ppc_tb_t *tb_env = env->tb_env;
> int64_t decr, diff;
>
> diff = next - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> if (diff >= 0) {
> decr = muldiv64(diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
> } else if (tb_env->flags & PPC_TIMER_BOOKE) {
> decr = 0;
> } else {
> decr = -muldiv64(-diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
> }
> LOG_TB("%s: %016" PRIx64 "\n", __func__, decr);
>
> return decr;
> }
>
> The diff < 0 case:
> decr = -muldiv64(-diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
> should probably just be:
> decr = -1;
> to comply with the minimum possible values specified by the ISA.
> But, again, this doesn't impact your patch directly.
We can try to address that in a followup patch.
> And also:
> Reviewed-by: Luis Pires <luis.pires@eldorado.org.br>
Thanks,
C.
> --
> Luis Pires
> Instituto de Pesquisas ELDORADO
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
>
From: Cédric Le Goater <clg@kaod.org>
> >> + target_long signed_value;
> >> + target_long signed_decr;
> >
> > Since these values will be the results of sextract64, it's probably better to
> define them as int64_t.
>
> but then it breaks the code doing the logging on PPC32 targets :/
You mean here?
> >> LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
> >> - decr, value);
> >> + decr, signed_value);
> >
> > While this reproduces the behavior we previously had, I think it would be more
> consistent if we logged what we had before the sign-extension ('value' instead
> of 'signed_value'). And 'decr' is okay, which is also not sign-extended.
It won't break if you log 'value' instead of 'signed_value', right?
> > The diff < 0 case:
> > decr = -muldiv64(-diff, tb_env->decr_freq,
> > NANOSECONDS_PER_SECOND); should probably just be:
> > decr = -1;
> > to comply with the minimum possible values specified by the ISA.
> > But, again, this doesn't impact your patch directly.
>
> We can try to address that in a followup patch.
Agreed.
Thanks!
--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On 9/17/21 16:00, Luis Fernando Fujita Pires wrote:
> From: Cédric Le Goater <clg@kaod.org>
>>>> + target_long signed_value;
>>>> + target_long signed_decr;
>>>
>>> Since these values will be the results of sextract64, it's probably better to
>> define them as int64_t.
>>
>> but then it breaks the code doing the logging on PPC32 targets :/
>
> You mean here?
You need to define PPC_DEBUG_TB and compile the ppc-softmmu :
../hw/ppc/ppc.c: In function ‘__cpu_ppc_store_decr’:
../hw/ppc/ppc.c:883:12: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘int64_t’ {aka ‘long int’} [-Werror=format=]
883 | LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
| ^~~~~~
884 | decr, signed_value);
| ~~~~~~~~~~~~
| |
| int64_t {aka long int}
../hw/ppc/ppc.c:51:32: note: in definition of macro ‘LOG_TB’
51 | # define LOG_TB(...) qemu_log(__VA_ARGS__)
| ^~~~~~~~~~~
cc1: all warnings being treated as errors
>>>> LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
>>>> - decr, value);
>>>> + decr, signed_value);
>>>
>>> While this reproduces the behavior we previously had, I think it would be more
>> consistent if we logged what we had before the sign-extension ('value' instead
>> of 'signed_value'). And 'decr' is okay, which is also not sign-extended.
>
> It won't break if you log 'value' instead of 'signed_value', right?
Yes but it's extra change ...
Thanks,
C.
On 9/17/21 7:10 AM, Cédric Le Goater wrote:
> On 9/17/21 16:00, Luis Fernando Fujita Pires wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>>>> + target_long signed_value;
>>>>> + target_long signed_decr;
>>>>
>>>> Since these values will be the results of sextract64, it's probably better to
>>> define them as int64_t.
>>>
>>> but then it breaks the code doing the logging on PPC32 targets :/
>>
>> You mean here?
>
> You need to define PPC_DEBUG_TB and compile the ppc-softmmu :
>
> ../hw/ppc/ppc.c: In function ‘__cpu_ppc_store_decr’:
> ../hw/ppc/ppc.c:883:12: error: format ‘%x’ expects argument of type ‘unsigned int’, but
> argument 4 has type ‘int64_t’ {aka ‘long int’} [-Werror=format=]
> 883 | LOG_TB("%s: " TARGET_FMT_lx " => " TARGET_FMT_lx "\n", __func__,
> | ^~~~~~
> 884 | decr, signed_value);
> | ~~~~~~~~~~~~
> | |
> | int64_t {aka long int}
> ../hw/ppc/ppc.c:51:32: note: in definition of macro ‘LOG_TB’
> 51 | # define LOG_TB(...) qemu_log(__VA_ARGS__)
> | ^~~~~~~~~~~
> cc1: all warnings being treated as errors
So use PRIx64, as you're supposed to do.
Or, in fact, change this to a tracepoint.
r~
© 2016 - 2024 Red Hat, Inc.