Re: [Qemu-devel] [Qemu-ppc] [PULL 1/3] ppc: Rework POWER7 & POWER8 exception model

[Cédric Le Goater]

On 04/07/2016 12:45 PM, Laurent Vivier wrote:
> 
> 
> On 07/04/2016 12:27, Cédric Le Goater wrote:
>> Hello Laurent,
>>
>> On 04/07/2016 11:13 AM, Laurent Vivier wrote:
>>>
>>>
>>> On 05/04/2016 04:17, David Gibson wrote:
>>>> From: Cédric Le Goater <address@hidden>
>>>>
>>>> From: Benjamin Herrenschmidt <address@hidden>
>>>>
>>>> This patch fixes the current AIL implementation for POWER8. The
>>>> interrupt vector address can be calculated directly from LPCR when the
>>>> exception is handled. The excp_prefix update becomes useless and we
>>>> can cleanup the H_SET_MODE hcall.
>>>
>>> I know it's a little bit late to comment this patch but:
>>>
>>> what about the initialization of the NIP in ppc_cpu_reset()?
>>>
>>>     env->nip = env->hreset_vector | env->excp_prefix;
>>>
>>> on POWER8 "env->excp_prefix" is always 0, but LPCR can have an AIL defined?
>>
>> yes. env->spr[SPR_LPCR] still has the previous value at that time and 
>> it is reseted right below in the same routine. 
>>
>> The cpu should restart in a valid state after that and later on, use the 
>> H_SET_MODE hcall from the guest kernel to set the AIL bits back in LPCR. 
>> It looks fine to me but I might be missing something. 
> 
> What I mean is if we want to keep the previous behavior we should have
> something like:
> 
>     env->nip = env->hreset_vector | env->excp_prefix;
> #if defined(TARGET_PPC64)
>     switch((env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT) {
>     case AIL_0001_8000:
>         env->nip |= 0x18000;
>         break;
>     case AIL_C000_0000_0000_4000:
>         env->nip |= 0xc000000000004000ull;
>         break;
>     }
> #endif

Yes. This is correct but the env->nip assigned in ppc_cpu_reset() is ignored 
and replaced with another value.

On spapr guests, the first CPU is started on 0x100, see ppc_spapr_reset(),
and then, this CPU starts the others with a firmware call: rtas_start_cpu().

Previously, when env->excp_prefix used to hold the interrupt vector address, 
the env->nip value was bogus but no one cared. Hopefully, the current state 
is a little better, the values are correct even if they are not used. But 
we still need to do a cleanup in that area.

Thanks,

C.
 
> But I don't know how behaves really a POWER8.
>
> Laurent
> 
>> Thanks,
>>
>> C.
>>
>>>
>>> Laurent
>>>>
>>>> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>>>> [clg: Removed LPES0/1 handling for HV vs. !HV
>>>>       Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ]
>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>> [dwg: This was written as a cleanup, but it also fixes a real bug
>>>>       where setting an alternative interrupt location would not be
>>>>       correctly migrated]
>>>> Signed-off-by: David Gibson <address@hidden>
>>>> ---
>>>>  hw/ppc/spapr_hcall.c        | 16 +--------------
>>>>  include/hw/ppc/spapr.h      |  5 -----
>>>>  target-ppc/cpu.h            | 10 +++++++++
>>>>  target-ppc/excp_helper.c    | 49 
>>>> +++++++++++++++++++++++++++++++++++++++++++--
>>>>  target-ppc/translate_init.c |  2 +-
>>>>  5 files changed, 59 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>>> index 2dcb676..8f40602 100644
>>>> --- a/hw/ppc/spapr_hcall.c
>>>> +++ b/hw/ppc/spapr_hcall.c
>>>> @@ -824,7 +824,6 @@ static target_ulong 
>>>> h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>>>  {
>>>>      CPUState *cs;
>>>>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>> -    target_ulong prefix;
>>>>  
>>>>      if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
>>>>          return H_P2;
>>>> @@ -836,25 +835,12 @@ static target_ulong 
>>>> h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>>>          return H_P4;
>>>>      }
>>>>  
>>>> -    switch (mflags) {
>>>> -    case H_SET_MODE_ADDR_TRANS_NONE:
>>>> -        prefix = 0;
>>>> -        break;
>>>> -    case H_SET_MODE_ADDR_TRANS_0001_8000:
>>>> -        prefix = 0x18000;
>>>> -        break;
>>>> -    case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>>>> -        prefix = 0xC000000000004000ULL;
>>>> -        break;
>>>> -    default:
>>>> +    if (mflags == AIL_RESERVED) {
>>>>          return H_UNSUPPORTED_FLAG;
>>>>      }
>>>>  
>>>>      CPU_FOREACH(cs) {
>>>> -        CPUPPCState *env = &POWERPC_CPU(cpu)->env;
>>>> -
>>>>          set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>>>> -        env->excp_prefix = prefix;
>>>>      }
>>>>  
>>>>      return H_SUCCESS;
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index 098d85d..815d5ee 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
>>>>  #define H_SET_MODE_ENDIAN_BIG    0
>>>>  #define H_SET_MODE_ENDIAN_LITTLE 1
>>>>  
>>>> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
>>>> -#define H_SET_MODE_ADDR_TRANS_NONE                  0
>>>> -#define H_SET_MODE_ADDR_TRANS_0001_8000             2
>>>> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000   3
>>>> -
>>>>  /* VASI States */
>>>>  #define H_VASI_INVALID    0
>>>>  #define H_VASI_ENABLED    1
>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>> index 676081e..9d4e43c 100644
>>>> --- a/target-ppc/cpu.h
>>>> +++ b/target-ppc/cpu.h
>>>> @@ -167,6 +167,8 @@ enum powerpc_excp_t {
>>>>      POWERPC_EXCP_970,
>>>>      /* POWER7 exception model           */
>>>>      POWERPC_EXCP_POWER7,
>>>> +    /* POWER8 exception model           */
>>>> +    POWERPC_EXCP_POWER8,
>>>>  #endif /* defined(TARGET_PPC64) */
>>>>  };
>>>>  
>>>> @@ -2277,6 +2279,14 @@ enum {
>>>>      HMER_XSCOM_STATUS_LSH       = (63 - 23),
>>>>  };
>>>>  
>>>> +/* Alternate Interrupt Location (AIL) */
>>>> +enum {
>>>> +    AIL_NONE                = 0,
>>>> +    AIL_RESERVED            = 1,
>>>> +    AIL_0001_8000           = 2,
>>>> +    AIL_C000_0000_0000_4000 = 3,
>>>> +};
>>>> +
>>>>  
>>>> /*****************************************************************************/
>>>>  
>>>>  static inline target_ulong cpu_read_xer(CPUPPCState *env)
>>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>>>> index c890853..ca4ffe8 100644
>>>> --- a/target-ppc/excp_helper.c
>>>> +++ b/target-ppc/excp_helper.c
>>>> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>> excp_model, int excp)
>>>>      CPUPPCState *env = &cpu->env;
>>>>      target_ulong msr, new_msr, vector;
>>>>      int srr0, srr1, asrr0, asrr1;
>>>> -    int lpes0, lpes1, lev;
>>>> +    int lpes0, lpes1, lev, ail;
>>>>  
>>>>      if (0) {
>>>>          /* XXX: find a suitable condition to enable the hypervisor mode */
>>>> @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>> excp_model, int excp)
>>>>      asrr0 = -1;
>>>>      asrr1 = -1;
>>>>  
>>>> +    /* Exception targetting modifiers
>>>> +     *
>>>> +     * AIL is initialized here but can be cleared by
>>>> +     * selected exceptions
>>>> +     */
>>>> +#if defined(TARGET_PPC64)
>>>> +    if (excp_model == POWERPC_EXCP_POWER7 ||
>>>> +        excp_model == POWERPC_EXCP_POWER8) {
>>>> +        if (excp_model == POWERPC_EXCP_POWER8) {
>>>> +            ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>>>> +        } else {
>>>> +            ail = 0;
>>>> +        }
>>>> +    } else
>>>> +#endif /* defined(TARGET_PPC64) */
>>>> +    {
>>>> +        ail = 0;
>>>> +    }
>>>> +
>>>>      switch (excp) {
>>>>      case POWERPC_EXCP_NONE:
>>>>          /* Should never happen */
>>>> @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>> excp_model, int excp)
>>>>              /* XXX: find a suitable condition to enable the hypervisor 
>>>> mode */
>>>>              new_msr |= (target_ulong)MSR_HVB;
>>>>          }
>>>> +        ail = 0;
>>>>  
>>>>          /* machine check exceptions don't have ME set */
>>>>          new_msr &= ~((target_ulong)1 << MSR_ME);
>>>> @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>> excp_model, int excp)
>>>>              /* XXX: find a suitable condition to enable the hypervisor 
>>>> mode */
>>>>              new_msr |= (target_ulong)MSR_HVB;
>>>>          }
>>>> +        ail = 0;
>>>>          goto store_next;
>>>>      case POWERPC_EXCP_DSEG:      /* Data segment exception                
>>>>    */
>>>>          if (lpes1 == 0) {
>>>> @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>> excp_model, int excp)
>>>>      }
>>>>  
>>>>  #ifdef TARGET_PPC64
>>>> -    if (excp_model == POWERPC_EXCP_POWER7) {
>>>> +    if (excp_model == POWERPC_EXCP_POWER7 ||
>>>> +        excp_model == POWERPC_EXCP_POWER8) {
>>>>          if (env->spr[SPR_LPCR] & LPCR_ILE) {
>>>>              new_msr |= (target_ulong)1 << MSR_LE;
>>>>          }
>>>> @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>> excp_model, int excp)
>>>>                    excp);
>>>>      }
>>>>      vector |= env->excp_prefix;
>>>> +
>>>> +    /* AIL only works if there is no HV transition and we are running with
>>>> +     * translations enabled
>>>> +     */
>>>> +    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>>> +        ail = 0;
>>>> +    }
>>>> +    /* Handle AIL */
>>>> +    if (ail) {
>>>> +        new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>>>> +        switch(ail) {
>>>> +        case AIL_0001_8000:
>>>> +            vector |= 0x18000;
>>>> +            break;
>>>> +        case AIL_C000_0000_0000_4000:
>>>> +            vector |= 0xc000000000004000ull;
>>>> +            break;
>>>> +        default:
>>>> +            cpu_abort(cs, "Invalid AIL combination %d\n", ail);
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>>  #if defined(TARGET_PPC64)
>>>>      if (excp_model == POWERPC_EXCP_BOOKE) {
>>>>          if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>>> index 0a33597..f515725 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>>>>      pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
>>>>      pcc->sps = &POWER7_POWER8_sps;
>>>>  #endif
>>>> -    pcc->excp_model = POWERPC_EXCP_POWER7;
>>>> +    pcc->excp_model = POWERPC_EXCP_POWER8;
>>>>      pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
>>>>      pcc->bfd_mach = bfd_mach_ppc64;
>>>>      pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
>>>>
>>>
>>
>