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

[Cédric Le Goater]

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>