Re: [PATCH v4 10/17] target/ppc: Create helper_scv

[Richard Henderson <richard.henderson@linaro.org>]

On 3/21/21 10:00 PM, David Gibson wrote:
> On Mon, Mar 15, 2021 at 12:46:08PM -0600, Richard Henderson wrote:
>> Perform the test against FSCR_SCV at runtime, in the helper.
>>
>> This means we can remove the incorrect set against SCV in
>> ppc_tr_init_disas_context and do not need to add an HFLAGS bit.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/ppc/helper.h      |  1 +
>>   target/ppc/excp_helper.c |  9 +++++++++
>>   target/ppc/translate.c   | 20 +++++++-------------
>>   3 files changed, 17 insertions(+), 13 deletions(-)
>>
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 6a4dccf70c..513066d54d 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -13,6 +13,7 @@ DEF_HELPER_1(rfci, void, env)
>>   DEF_HELPER_1(rfdi, void, env)
>>   DEF_HELPER_1(rfmci, void, env)
>>   #if defined(TARGET_PPC64)
>> +DEF_HELPER_2(scv, noreturn, env, i32)
>>   DEF_HELPER_2(pminsn, void, env, i32)
>>   DEF_HELPER_1(rfid, void, env)
>>   DEF_HELPER_1(rfscv, void, env)
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 85de7e6c90..5c95e0c103 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -1130,6 +1130,15 @@ void helper_store_msr(CPUPPCState *env, target_ulong val)
>>   }
>>   
>>   #if defined(TARGET_PPC64)
>> +void helper_scv(CPUPPCState *env, uint32_t lev)
>> +{
>> +    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
>> +        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
>> +    } else {
>> +        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
>> +    }
>> +}
>> +
>>   void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>>   {
>>       CPUState *cs;
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 7912495f28..d48c554290 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -173,7 +173,6 @@ struct DisasContext {
>>       bool vsx_enabled;
>>       bool spe_enabled;
>>       bool tm_enabled;
>> -    bool scv_enabled;
>>       bool gtse;
>>       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>>       int singlestep_enabled;
>> @@ -4081,15 +4080,16 @@ static void gen_sc(DisasContext *ctx)
>>   #if !defined(CONFIG_USER_ONLY)
>>   static void gen_scv(DisasContext *ctx)
>>   {
>> -    uint32_t lev;
>> +    uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>>   
>> -    if (unlikely(!ctx->scv_enabled)) {
>> -        gen_exception_err(ctx, POWERPC_EXCP_FU, FSCR_IC_SCV);
>> -        return;
>> +    /* Set the PC back to the faulting instruction. */
>> +    if (ctx->exception == POWERPC_EXCP_NONE) {
>> +        gen_update_nip(ctx, ctx->base.pc_next - 4);
>>       }
> 
> I don't quite understand this.  Don't we need the NIP to be on the scv
> instruction itself for the case where we get a facility unavailable
> exception, but on the next instruction if we actually take the system
> call?  This appears to be unconditional.
> 
>> +    gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>>   
>> -    lev = (ctx->opcode >> 5) & 0x7F;
>> -    gen_exception_err(ctx, POWERPC_SYSCALL_VECTORED, lev);

Hmm.  In the old code, both paths use gen_exception_err, without otherwise 
manipulating NIP.  That suggests to me that both exceptions receive the same 
value in NIP.

Is there an adjustment to NIP when delivering the SCV exception?  Yep:

     case POWERPC_EXCP_SYSCALL_VECTORED:
         lev = env->error_code;
         dump_syscall_vectored(env);
         env->nip += 4;
         new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
         new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
         break;


r~