Re: [Qemu-devel] [PATCH 06/17] ppc: Create cpu_ppc_set_papr() helper

[Cédric Le Goater]

On 03/17/2016 03:34 AM, David Gibson wrote:
> On Mon, Mar 14, 2016 at 05:56:29PM +0100, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <address@hidden>
>>
>> And move the code adjusting the MSR mask and calling kvmppc_set_papr()
>> to it. This allows us to add a few more things such as disabling setting
>> of MSR:HV and appropriate LPCR bits which will be used when fixing
>> the exception model.
>>
>> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>> Reviewed-by: David Gibson <address@hidden>
> 
> I'm a little nervous about applying this before 2.6.  This affects the
> value of the LPCR which is used to control exception behaviour in some
> cases.  I'm pretty sure the current behaviour is wrong, but we do know
> it doesn't break horribly for existing machines, which we'd have to
> retest with the new behaviour.

Yes. I agree.

> I'm certainly willing to hear a case for this if it makes other
> patches in the series significantly easier though.

I think we should split this patch in two. Put the cpu_ppc_set_papr() helper 
and the MSR change in the first one and keep the LPCR changes for the second. 
The latter belong to another set of fixes related the exception models.

C.

>> ---
>>  hw/ppc/spapr.c              | 11 ++---------
>>  target-ppc/cpu.h            |  1 +
>>  target-ppc/translate_init.c | 37 ++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 43708a2a9086..9c01872ce4d3 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1612,15 +1612,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, 
>> PowerPCCPU *cpu,
>>      /* Set time-base frequency to 512 MHz */
>>      cpu_ppc_tb_init(env, TIMEBASE_FREQ);
>>  
>> -    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
>> -     * MSR[IP] should never be set.
>> -     */
>> -    env->msr_mask &= ~(1 << 6);
>> -
>> -    /* Tell KVM that we're in PAPR mode */
>> -    if (kvm_enabled()) {
>> -        kvmppc_set_papr(cpu);
>> -    }
>> +    /* Enable PAPR mode in TCG or KVM */
>> +    cpu_ppc_set_papr(cpu);
>>  
>>      if (cpu->max_compat) {
>>          Error *local_err = NULL;
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index 9ce301f18922..a7da0d3e95a9 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -1268,6 +1268,7 @@ void store_booke_tcr (CPUPPCState *env, target_ulong 
>> val);
>>  void store_booke_tsr (CPUPPCState *env, target_ulong val);
>>  void ppc_tlb_invalidate_all (CPUPPCState *env);
>>  void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
>> +void cpu_ppc_set_papr(PowerPCCPU *cpu);
>>  #endif
>>  #endif
>>  
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 46dabe58783a..093ef036320d 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -8496,8 +8496,43 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
>>      pcc->threads_per_core = 8;
>>  }
>> -#endif /* defined (TARGET_PPC64) */
>>  
>> +#if !defined(CONFIG_USER_ONLY)
>> +
>> +void cpu_ppc_set_papr(PowerPCCPU *cpu)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    ppc_spr_t *lpcr = &env->spr_cb[SPR_LPCR];
>> +
>> +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
>> +     * MSR[IP] should never be set.
>> +     *
>> +     * We also disallow setting of MSR_HV
>> +     */
>> +    env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);
>> +
>> +    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
>> +     * under KVM, the actual HW LPCR will be set differently by KVM itself,
>> +     * the settings below ensure proper operations with TCG in absence of
>> +     * a real hypervisor
>> +     */
>> +    lpcr->default_value &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
>> +    lpcr->default_value |= LPCR_LPES0 | LPCR_LPES1;
>> +
>> +    /* We should be followed by a CPU reset but update the active value
>> +     * just in case...
>> +     */
>> +    env->spr[SPR_LPCR] = lpcr->default_value;
>> +
>> +    /* Tell KVM that we're in PAPR mode */
>> +    if (kvm_enabled()) {
>> +        kvmppc_set_papr(cpu);
>> +    }
>> +}
>> +
>> +#endif /* !defined(CONFIG_USER_ONLY) */
>> +
>> +#endif /* defined (TARGET_PPC64) */
>>  
>>  
>> /*****************************************************************************/
>>  /* Generic CPU instantiation routine                                        
>>  */
>