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

[Greg Kurz]

On Mon, 25 Mar 2019 17:38:49 +1100
David Gibson <address@hidden> wrote:

> 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?
> 

I must confess this puzzles me as well :-\ ... and the linux commit that
introduces the use of this opcode, ie. ee13cb249fab ("powerpc/64s: Add
support for software count cache flush") doesn't provide any details,
at least for someone ignorant like me :)

Maybe Michael can enlight us ?

> > 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);
> >      }
> >   
>

pgpedYZrB7kQs.pgp
Description: OpenPGP digital signature