Re: [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception

[Cédric Le Goater]

On 12/19/19 6:12 AM, David Gibson wrote:
> On Thu, Nov 28, 2019 at 02:46:59PM +0100, Cédric Le Goater wrote:
>> The privileged message send and clear instructions (msgsndp & msgclrp)
>> are privileged, but will generate a hypervisor facility unavailable
>> exception if not enabled in the HFSCR and executed in privileged
>> non-hypervisor state.
>>
>> Add checks when accessing the DPDES register and when using the
>> msgsndp and msgclrp isntructions.
>>
>> Based on previous work from Suraj Jitindar Singh.
>>
>> Cc: Suraj Jitindar Singh <address@hidden>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  target/ppc/cpu.h                |  6 ++++++
>>  target/ppc/helper.h             |  1 +
>>  target/ppc/excp_helper.c        |  9 +++++++++
>>  target/ppc/misc_helper.c        | 24 ++++++++++++++++++++++++
>>  target/ppc/translate.c          |  4 ++++
>>  target/ppc/translate_init.inc.c | 18 ++++++++++++++++++
>>  6 files changed, 62 insertions(+)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 8ffcfa0ea162..52608dfe6ff4 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -397,6 +397,10 @@ typedef struct ppc_v3_pate_t {
>>  #define PSSCR_ESL         PPC_BIT(42) /* Enable State Loss */
>>  #define PSSCR_EC          PPC_BIT(43) /* Exit Criterion */
>>  
>> +/* HFSCR bits */
>> +#define HFSCR_MSGP     PPC_BIT(53) /* Privileged Message Send Facilities */
>> +#define HFSCR_IC_MSGP  0xA
>> +
>>  #define msr_sf   ((env->msr >> MSR_SF)   & 1)
>>  #define msr_isf  ((env->msr >> MSR_ISF)  & 1)
>>  #define msr_shv  ((env->msr >> MSR_SHV)  & 1)
>> @@ -1333,6 +1337,8 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, 
>> PPCVirtualHypervisor *vhyp);
>>  #endif
>>  
>>  void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
>> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
>> +                              int sprn, int cause);
>>  
>>  static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
>>  {
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 76518a1df6f0..14c9a30a45c9 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -643,6 +643,7 @@ DEF_HELPER_3(store_dcr, void, env, tl, tl)
>>  
>>  DEF_HELPER_2(load_dump_spr, void, env, i32)
>>  DEF_HELPER_2(store_dump_spr, void, env, i32)
>> +DEF_HELPER_4(hfscr_facility_check, void, env, i32, i32, i32)
>>  DEF_HELPER_4(fscr_facility_check, void, env, i32, i32, i32)
>>  DEF_HELPER_4(msr_facility_check, void, env, i32, i32, i32)
>>  DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 5a247945e97f..17dad626b74e 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -469,6 +469,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>> excp_model, int excp)
>>      case POWERPC_EXCP_FU:         /* Facility unavailable exception         
>>  */
>>  #ifdef TARGET_PPC64
>>          env->spr[SPR_FSCR] |= ((target_ulong)env->error_code << 56);
>> +#endif
>> +        break;
>> +    case POWERPC_EXCP_HV_FU:     /* Hypervisor Facility Unavailable 
>> Exception */
>> +#ifdef TARGET_PPC64
>> +        env->spr[SPR_HFSCR] |= ((target_ulong)env->error_code << 
>> FSCR_IC_POS);
>> +        srr0 = SPR_HSRR0;
>> +        srr1 = SPR_HSRR1;
>> +        new_msr |= (target_ulong)MSR_HVB;
>> +        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>>  #endif
>>          break;
>>      case POWERPC_EXCP_PIT:       /* Programmable interval timer interrupt   
>>  */
>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>> index a0e7bd9c32d3..0cd44c6edd82 100644
>> --- a/target/ppc/misc_helper.c
>> +++ b/target/ppc/misc_helper.c
>> @@ -41,6 +41,17 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t 
>> sprn)
>>  }
>>  
>>  #ifdef TARGET_PPC64
>> +static void raise_hv_fu_exception(CPUPPCState *env, uint32_t bit,
>> +                                  uint32_t sprn, uint32_t cause,
>> +                                  uintptr_t raddr)
>> +{
>> +    qemu_log("Facility SPR %d is unavailable (SPR HFSCR:%d)\n", sprn, bit);
> 
> That looks overly verbose.  Leftover debugging?

No. It's inherited from raise_fu_exception(). Should we remove them ?
or use mask CPU_LOG_INT ?


> 
>> +    env->spr[SPR_HFSCR] &= ~((target_ulong)FSCR_IC_MASK << FSCR_IC_POS);
>> +
>> +    raise_exception_err_ra(env, POWERPC_EXCP_HV_FU, cause, raddr);
>> +}
>> +
>>  static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
>>                                 uint32_t sprn, uint32_t cause,
>>                                 uintptr_t raddr)
>> @@ -55,6 +66,17 @@ static void raise_fu_exception(CPUPPCState *env, uint32_t 
>> bit,
>>  }
>>  #endif
>>  
>> +void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
>> +                                 uint32_t sprn, uint32_t cause)
>> +{
>> +#ifdef TARGET_PPC64
>> +    if ((env->msr_mask & MSR_HVB) && !msr_hv &&
>> +                                     !(env->spr[SPR_HFSCR] & (1UL << bit))) 
>> {
>> +        raise_hv_fu_exception(env, bit, sprn, cause, GETPC());
>> +    }
>> +#endif
>> +}
>> +
>>  void helper_fscr_facility_check(CPUPPCState *env, uint32_t bit,
>>                                  uint32_t sprn, uint32_t cause)
>>  {
>> @@ -108,6 +130,8 @@ void helper_store_pcr(CPUPPCState *env, target_ulong 
>> value)
>>  
>>  target_ulong helper_load_dpdes(CPUPPCState *env)
>>  {
>> +    helper_hfscr_facility_check(env, HFSCR_MSGP, SPR_DPDES,
>> +                                HFSCR_IC_MSGP);
>>      if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
>>          return 1;
>>      }
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index ba759ab2bb0f..e9e70ca149fd 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -6652,6 +6652,8 @@ static void gen_msgclrp(DisasContext *ctx)
>>      GEN_PRIV;
>>  #else
>>      CHK_SV;
>> +    gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
>> +                             HFSCR_IC_MSGP);
>>      gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
> 
> Calling a helper for the facility check, then another helper for the
> actual instruction is a bit yucky.  I'd prefer if you either call out
> for the facility check within the instruction helper, or generate the
> instructions necessary for the HFSCR check

OK. I will take a look. 

C.

> 
>>  #endif /* defined(CONFIG_USER_ONLY) */
>>  }
>> @@ -6662,6 +6664,8 @@ static void gen_msgsndp(DisasContext *ctx)
>>      GEN_PRIV;
>>  #else
>>      CHK_SV;
>> +    gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
>> +                             HFSCR_IC_MSGP);
>>      gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
>>  #endif /* defined(CONFIG_USER_ONLY) */
>>  }
>> diff --git a/target/ppc/translate_init.inc.c 
>> b/target/ppc/translate_init.inc.c
>> index 7c74a763ba66..154e01451270 100644
>> --- a/target/ppc/translate_init.inc.c
>> +++ b/target/ppc/translate_init.inc.c
>> @@ -468,11 +468,15 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, 
>> int gprn)
>>  /* DPDES */
>>  static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
>>  {
>> +    gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
>> +                             HFSCR_IC_MSGP);
>>      gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
>>  }
>>  
>>  static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
>>  {
>> +    gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
>> +                             HFSCR_IC_MSGP);
>>      gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
>>  }
>>  #endif
>> @@ -7523,6 +7527,20 @@ POWERPC_FAMILY(e600)(ObjectClass *oc, void *data)
>>  #define POWERPC970_HID5_INIT 0x00000000
>>  #endif
>>  
>> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
>> +                              int sprn, int cause)
>> +{
>> +    TCGv_i32 t1 = tcg_const_i32(bit);
>> +    TCGv_i32 t2 = tcg_const_i32(sprn);
>> +    TCGv_i32 t3 = tcg_const_i32(cause);
>> +
>> +    gen_helper_hfscr_facility_check(cpu_env, t1, t2, t3);
>> +
>> +    tcg_temp_free_i32(t3);
>> +    tcg_temp_free_i32(t2);
>> +    tcg_temp_free_i32(t1);
>> +}
>> +
>>  static void gen_fscr_facility_check(DisasContext *ctx, int facility_sprn,
>>                                      int bit, int sprn, int cause)
>>  {
>