Re: [Qemu-devel] [Qemu-ppc] [PATCH 07/12] ppc: Better figure out if processor has HV mode

["Cédric Le Goater" <clg@kaod.org>]

On 05/27/2016 05:38 AM, David Gibson wrote:
> On Tue, May 03, 2016 at 06:03:29PM +0200, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> We use an env. flag which is set to the initial value of MSR_HVB in
>> the msr_mask. We also adjust the POWER8 mask to set SHV.
>>
>> Also use this to adjust ctx.hv so that it is *set* when the processor
>> doesn't have an HV mode (970 with Apple mode for example), thus enabling
>> hypervisor instructions/SPRs.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  target-ppc/cpu.h            |  4 ++++
>>  target-ppc/translate.c      |  4 +++-
>>  target-ppc/translate_init.c | 21 ++++++++++++++++-----
>>  3 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index 2a96efcbf813..02f2e72e6d14 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -1161,6 +1161,10 @@ struct CPUPPCState {
>>      hwaddr mpic_iack;
>>      /* true when the external proxy facility mode is enabled */
>>      bool mpic_proxy;
>> +    /* set when the processor has an HV mode, thus HV priv
>> +     * instructions and SPRs are diallowed if MSR:HV is 0
>> +     */
>> +    bool has_hv_mode;
>>  #endif
>>  
>>      /* Those resources are used only during code translation */
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index 7a672cba796d..6f55bcd34a74 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -11495,8 +11495,10 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
>>      ctx.exception = POWERPC_EXCP_NONE;
>>      ctx.spr_cb = env->spr_cb;
>>      ctx.pr = msr_pr;
>> -    ctx.hv = !msr_pr && msr_hv;
> 
> The test for msr_pr has been removed in the new version.  Maybe that's
> safe, but I think it needs some justification.

I don't know if it is safe to run with (MSR_HV|MSR_PR) = 11
 
There is a note in the Power ISA book describing such a case but I am not 
sure qemu-ppc supports that. So I will keep the msr_pr check in the next 
version :

	ctx.hv = !msr_pr && (msr_hv || !env->has_hv_mode);
 

Thanks,

C.

>>      ctx.mem_idx = env->dmmu_idx;
>> +#if !defined(CONFIG_USER_ONLY)
>> +    ctx.hv = msr_hv || !env->has_hv_mode;
>> +#endif
>>      ctx.insns_flags = env->insns_flags;
>>      ctx.insns_flags2 = env->insns_flags2;
>>      ctx.access_type = -1;
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 10a92fdbbdd7..df656e6021b4 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -8579,7 +8579,8 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>>                          PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
>>                          PPC2_TM;
>>      pcc->msr_mask = (1ull << MSR_SF) |
>> -                    (1ull << MSR_TM) |
>> +                    (1ull << MSR_SHV) |
>> +		    (1ull << MSR_TM) |
>>                      (1ull << MSR_VR) |
>>                      (1ull << MSR_VSX) |
>>                      (1ull << MSR_EE) |
>> @@ -9975,10 +9976,7 @@ static void ppc_cpu_reset(CPUState *s)
>>      pcc->parent_reset(s);
>>  
>>      msr = (target_ulong)0;
>> -    if (0) {
>> -        /* XXX: find a suitable condition to enable the hypervisor mode */
>> -        msr |= (target_ulong)MSR_HVB;
>> -    }
>> +    msr |= (target_ulong)MSR_HVB;
>>      msr |= (target_ulong)0 << MSR_AP; /* TO BE CHECKED */
>>      msr |= (target_ulong)0 << MSR_SA; /* TO BE CHECKED */
>>      msr |= (target_ulong)1 << MSR_EP;
>> @@ -10079,6 +10077,19 @@ static void ppc_cpu_initfn(Object *obj)
>>      env->bfd_mach = pcc->bfd_mach;
>>      env->check_pow = pcc->check_pow;
>>  
>> +    /* Mark HV mode as supported if the CPU has an MSR_HV bit
>> +     * in the msr_mask. The mask can later be cleared by PAPR
>> +     * mode but the hv mode support will remain, thus enforcing
>> +     * that we cannot use priv. instructions in guest in PAPR
>> +     * mode. For 970 we currently simply don't set HV in msr_mask
>> +     * thus simulating an "Apple mode" 970. If we ever want to
>> +     * support 970 HV mode, we'll have to add a processor attribute
>> +     * of some sort.
>> +     */
>> +#if !defined(CONFIG_USER_ONLY)
>> +    env->has_hv_mode = !!(env->msr_mask & MSR_HVB);
>> +#endif
>> +
>>  #if defined(TARGET_PPC64)
>>      if (pcc->sps) {
>>          env->sps = *pcc->sps;
>