Re: [Qemu-devel] [PATCH] target/ppc: extend eieio for POWER9

[David Gibson]

On Wed, Jun 06, 2018 at 08:42:08AM +0200, Cédric Le Goater wrote:
> On 06/06/2018 08:32 AM, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 08:08:30AM +0200, Cédric Le Goater wrote:
> >> POWER9 introduced a new variant of the eieio instruction using bit 6
> >> as a hint to tell the CPU it is a store-forwarding barrier.
> >>
> >> The usage of this eieio extension was recently added in Linux 4.17
> >> which activated the "support for a store forwarding barrier at kernel
> >> entry/exit".
> >>
> >> Unfortunately, it is not possible to insert this new eieio instruction
> >> without considerable change in ppc_tr_translate_insn(). So instead we
> >> loosen the QEMU eieio instruction mask. The gen_eieio() helper is
> >> modified to test for bit6 and a custom instruction flag to catch
> >> invalid eieio opcodes on non-POWER9 CPUs.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  target/ppc/cpu.h                |  5 ++++-
> >>  target/ppc/translate.c          | 19 +++++++++++++++++--
> >>  target/ppc/translate_init.inc.c |  3 ++-
> >>  3 files changed, 23 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >> index 0247c1f04c37..021c9b2f10d1 100644
> >> --- a/target/ppc/cpu.h
> >> +++ b/target/ppc/cpu.h
> >> @@ -2206,6 +2206,9 @@ enum {
> >>      /* POWER ISA 3.0                                                      
> >>    */
> >>      PPC2_ISA300        = 0x0000000000080000ULL,
> >>  
> >> +    /* POWER ISA 3.0 eieio variants                                       
> >>    */
> >> +    PPC2_MEM_EIEIO2    = 0x0000000000100000ULL,
> >> +
> >>  #define PPC_TCG_INSNS2 (PPC2_BOOKE206 | PPC2_VSX | PPC2_PRCNTL | 
> >> PPC2_DBRX | \
> >>                          PPC2_ISA205 | PPC2_VSX207 | PPC2_PERM_ISA206 | \
> >>                          PPC2_DIVE_ISA206 | PPC2_ATOMIC_ISA206 | \
> >> @@ -2213,7 +2216,7 @@ enum {
> >>                          PPC2_BCTAR_ISA207 | PPC2_LSQ_ISA207 | \
> >>                          PPC2_ALTIVEC_207 | PPC2_ISA207S | PPC2_DFP | \
> >>                          PPC2_FP_CVT_S64 | PPC2_TM | PPC2_PM_ISA206 | \
> >> -                        PPC2_ISA300)
> >> +                        PPC2_ISA300 | PPC2_MEM_EIEIO2)
> >>  };
> >>  
> >>  
> >> /*****************************************************************************/
> >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> >> index 8ba8f67dc513..a73ef02aef1d 100644
> >> --- a/target/ppc/translate.c
> >> +++ b/target/ppc/translate.c
> >> @@ -2967,7 +2967,22 @@ static void gen_stswx(DisasContext *ctx)
> >>  /* eieio */
> >>  static void gen_eieio(DisasContext *ctx)
> >>  {
> >> -    tcg_gen_mb(TCG_MO_LD_ST | TCG_BAR_SC);
> >> +    TCGBar bar = TCG_MO_LD_ST;
> >> +
> >> +    /*
> >> +     * POWER9 has a eieio instruction variant using bit 6 as a hint to
> >> +     * tell the CPU it is a store-forwarding barrier.
> >> +     */
> >> +    if (ctx->opcode & 0x2000000) {
> >> +        if (!(ctx->insns_flags2 & PPC2_MEM_EIEIO2)) {
> > 
> > Since we have to adapt the gen_eieio code anyway we don't really need
> > the new instruction flag - we can just reuse PPC2_ISA300 for it.
> 
> OK. That seems now a little superfluous, indeed. Unless there are more
> or these instructions which depend on CPU revision. I don't know so
> let's stick to  PPC2_ISA300.
> 
> > 
> >> +            qemu_log_mask(LOG_GUEST_ERROR, "invalid eieio using bit 6 at 
> >> @"
> >> +                          TARGET_FMT_lx "\n", ctx->base.pc_next - 4);
> > 
> > Logging is ok, but it's not enough in this context - that will turn
> > the variant eieio into a no-op.  You need to actually generate an
> > invalid instruction exception here, which is what I assume will happen
> > if you try to execute the new variant on a POWER8:
> >     gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> 
> well, I didn't try but ISA says that :
> 
>       "Reserved fields in instructions are ignored by the processor."
> 
> So it should be a no-op.

Ah, ok.  So, no 0x700, but.. that doesn't mean a no-op, that means it
should act as a normal eieio.

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