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

[Suraj Jitindar Singh]

On Fri, 2019-03-22 at 19:03 +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:
> 
>     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.
> 
> 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.
> 
> The existing behaviour of throwing a program interrupt is kept for
> all other CPU models.
> 
> Signed-off-by: Greg Kurz <address@hidden>

Tested-by: Suraj Jitindar Singh <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);
>      }
> 
>