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

[Cédric Le Goater]

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.

C. 

>> +            return;
>> +        }
>> +        bar = TCG_MO_ST_LD;
>> +    }
>> +
>> +    tcg_gen_mb(bar | TCG_BAR_SC);
>>  }
>>  
>>  #if !defined(CONFIG_USER_ONLY)
>> @@ -6483,7 +6498,7 @@ GEN_HANDLER(lswi, 0x1F, 0x15, 0x12, 0x00000001, 
>> PPC_STRING),
>>  GEN_HANDLER(lswx, 0x1F, 0x15, 0x10, 0x00000001, PPC_STRING),
>>  GEN_HANDLER(stswi, 0x1F, 0x15, 0x16, 0x00000001, PPC_STRING),
>>  GEN_HANDLER(stswx, 0x1F, 0x15, 0x14, 0x00000001, PPC_STRING),
>> -GEN_HANDLER(eieio, 0x1F, 0x16, 0x1A, 0x03FFF801, PPC_MEM_EIEIO),
>> +GEN_HANDLER(eieio, 0x1F, 0x16, 0x1A, 0x01FFF801, PPC_MEM_EIEIO),
>>  GEN_HANDLER(isync, 0x13, 0x16, 0x04, 0x03FFF801, PPC_MEM),
>>  GEN_HANDLER_E(lbarx, 0x1F, 0x14, 0x01, 0, PPC_NONE, PPC2_ATOMIC_ISA206),
>>  GEN_HANDLER_E(lharx, 0x1F, 0x14, 0x03, 0, PPC_NONE, PPC2_ATOMIC_ISA206),
>> diff --git a/target/ppc/translate_init.inc.c 
>> b/target/ppc/translate_init.inc.c
>> index 1a89017ddea8..6b70f6dc5a2c 100644
>> --- a/target/ppc/translate_init.inc.c
>> +++ b/target/ppc/translate_init.inc.c
>> @@ -8847,7 +8847,8 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>>                          PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207 |
>>                          PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 |
>>                          PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
>> -                        PPC2_TM | PPC2_PM_ISA206 | PPC2_ISA300 | 
>> PPC2_PRCNTL;
>> +                        PPC2_TM | PPC2_PM_ISA206 | PPC2_ISA300 | 
>> PPC2_PRCNTL |
>> +                        PPC2_MEM_EIEIO2;
>>      pcc->msr_mask = (1ull << MSR_SF) |
>>                      (1ull << MSR_TM) |
>>                      (1ull << MSR_VR) |
>