RE: [PATCH v3] target/ppc: Fix 64-bit decrementer

[Luis Fernando Fujita Pires]

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>