Re: [Qemu-ppc] [PATCH for-4.0 2/3] target/ppc: Enable "decrement and test CTR" version of bcctr

[David Gibson]

On Fri, Mar 22, 2019 at 07:03:46PM +0100, Greg Kurz wrote:
> Even if all ISAs up to v3 indeed mention:
> 
>     If the "decrement and test CTR" option is specified (BO2=0), the
>     instruction form is invalid.
> 
> The UMs of all existing 64-bit server class processors say:

I've applied this series because it fixes an immediate problem, but I
have some significant reservations about it, read on..

>     If BO[2] = 0, the contents of CTR (before any update) are used as the
>     target address and for the test of the contents of CTR to resolve the
>     branch. The contents of the CTR are then decremented and written back
>     to the CTR.

So, if that's what the hardware does, I guess that's what we need to
do.  That behaviour seems totally bizarre though - how can it make
sense for the same register value to act as both the branch target and
a flag/counter?  Or am I misreading something?

> The linux kernel has spectre v2 mitigation code that relies on a
> BO[2] = 0 variant of bcctr, which is now activated by default on
> spapr, even with TCG. This causes linux guests to panic with
> the default machine type under TCG.
> 
> Since any CPU model can provide its own behaviour for invalid forms,
> we could possibly introduce a new instruction flag to handle this.
> In practice, since the behaviour is shared by all 64-bit server
> processors starting with 970 up to POWER9, let's reuse the
> PPC_SEGMENT_64B flag. Caveat: this may have to be fixed later if
> POWER10 introduces a different behaviour.

Yeah.. this makes me nervous.  It's going to be very non-obvious that
a flag about MMU behaviour is linked to an obscure conditional branch
behaviour, so I suspect the chances of forgetting to fix that later if
necessary are close to 100%.

> The existing behaviour of throwing a program interrupt is kept for
> all other CPU models.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  target/ppc/translate.c |   52 
> ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index aaafa3a715d8..d3aaa6482c6a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3747,22 +3747,44 @@ static void gen_bcond(DisasContext *ctx, int type)
>      if ((bo & 0x4) == 0) {
>          /* Decrement and test CTR */
>          TCGv temp = tcg_temp_new();
> -        if (unlikely(type == BCOND_CTR)) {
> -            gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> -            tcg_temp_free(temp);
> -            tcg_temp_free(target);
> -            return;
> -        }
> -        tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1);
> -        if (NARROW_MODE(ctx)) {
> -            tcg_gen_ext32u_tl(temp, cpu_ctr);
> -        } else {
> -            tcg_gen_mov_tl(temp, cpu_ctr);
> -        }
> -        if (bo & 0x2) {
> -            tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1);
> +
> +        if (type == BCOND_CTR) {
> +            /*
> +             * All ISAs up to v3 describe this form of bcctr as invalid but
> +             * some processors, ie. 64-bit server processors compliant with
> +             * arch 2.x, do implement a "test and decrement" logic instead,
> +             * as described in their respective UMs.
> +             */
> +            if (unlikely(!(ctx->insns_flags & PPC_SEGMENT_64B))) {
> +                gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> +                tcg_temp_free(temp);
> +                tcg_temp_free(target);
> +                return;
> +            }
> +
> +            if (NARROW_MODE(ctx)) {
> +                tcg_gen_ext32u_tl(temp, cpu_ctr);
> +            } else {
> +                tcg_gen_mov_tl(temp, cpu_ctr);
> +            }
> +            if (bo & 0x2) {
> +                tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1);
> +            } else {
> +                tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1);
> +            }
> +            tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1);
>          } else {
> -            tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1);
> +            tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1);
> +            if (NARROW_MODE(ctx)) {
> +                tcg_gen_ext32u_tl(temp, cpu_ctr);
> +            } else {
> +                tcg_gen_mov_tl(temp, cpu_ctr);
> +            }
> +            if (bo & 0x2) {
> +                tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1);
> +            } else {
> +                tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1);
> +            }
>          }
>          tcg_temp_free(temp);
>      }
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature