Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Cédric Le Goater's message of April 14, 2021 3:09 am:
> On 4/13/21 5:53 PM, Fabiano Rosas wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>>> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
>>> and it removes support for the LPCR[AIL]=0b10 mode.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>  hw/ppc/spapr_hcall.c            |   5 ++
>>>  target/ppc/cpu-qom.h            |   2 +
>>>  target/ppc/cpu.h                |   5 +-
>>>  target/ppc/excp_helper.c        | 114 ++++++++++++++++++++++++++------
>>>  target/ppc/translate.c          |   3 +-
>>>  target/ppc/translate_init.c.inc |   2 +-
>>>  6 files changed, 107 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index 7b5cd3553c..2f65f20f72 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>>          return H_UNSUPPORTED_FLAG;
>>>      }
>>>  
>>> +    if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
>>> +        /* AIL 2 is also reserved in ISA v3.1 */
>>> +        return H_UNSUPPORTED_FLAG;
>>> +    }
>>> +
>>>      spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>>>  
>>>      return H_SUCCESS;
>>> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
>>> index 118baf8d41..06b6571bc9 100644
>>> --- a/target/ppc/cpu-qom.h
>>> +++ b/target/ppc/cpu-qom.h
>>> @@ -116,6 +116,8 @@ enum powerpc_excp_t {
>>>      POWERPC_EXCP_POWER8,
>>>      /* POWER9 exception model           */
>>>      POWERPC_EXCP_POWER9,
>>> +    /* POWER10 exception model           */
>>> +    POWERPC_EXCP_POWER10,
>>>  };
>>>  
>>>  /*****************************************************************************/
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index e73416da68..24e53e0ac3 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t {
>>>  #define LPCR_PECE_U_SHIFT (63 - 19)
>>>  #define LPCR_PECE_U_MASK  (0x7ull << LPCR_PECE_U_SHIFT)
>>>  #define LPCR_HVEE         PPC_BIT(17) /* Hypervisor Virt Exit Enable */
>>> -#define LPCR_RMLS_SHIFT   (63 - 37)
>>> +#define LPCR_RMLS_SHIFT   (63 - 37)   /* RMLS (removed in ISA v3.0) */
>>>  #define LPCR_RMLS         (0xfull << LPCR_RMLS_SHIFT)
>>> +#define LPCR_HAIL         PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */
>>>  #define LPCR_ILE          PPC_BIT(38)
>>> -#define LPCR_AIL_SHIFT    (63 - 40)      /* Alternate interrupt location */
>>> +#define LPCR_AIL_SHIFT    (63 - 40)   /* Alternate interrupt location */
>>>  #define LPCR_AIL          (3ull << LPCR_AIL_SHIFT)
>>>  #define LPCR_UPRT         PPC_BIT(41) /* Use Process Table */
>>>  #define LPCR_EVIRT        PPC_BIT(42) /* Enhanced Virtualisation */
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index b8881c0f85..e8bf0598b4 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -197,7 +197,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>      CPUState *cs = CPU(cpu);
>>>      CPUPPCState *env = &cpu->env;
>>>      target_ulong msr, new_msr, vector;
>>> -    int srr0, srr1, asrr0, asrr1, lev = -1, ail;
>>> +    int srr0, srr1, asrr0, asrr1, lev = -1;
>>> +    int ail, hail, using_ail;
>>>      bool lpes0;
>>>  
>>>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>>> @@ -239,24 +240,39 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>       * On anything else, we behave as if LPES0 is 1
>>>       * (externals don't alter MSR:HV)
>>>       *
>>> -     * AIL is initialized here but can be cleared by
>>> -     * selected exceptions
>>> +     * ail/hail are initialized here but can be cleared by
>>> +     * selected exceptions, and other conditions afterwards.
>>>       */
>>>  #if defined(TARGET_PPC64)
>>>      if (excp_model == POWERPC_EXCP_POWER7 ||
>>>          excp_model == POWERPC_EXCP_POWER8 ||
>>> -        excp_model == POWERPC_EXCP_POWER9) {
>>> +        excp_model == POWERPC_EXCP_POWER9 ||
>>> +        excp_model == POWERPC_EXCP_POWER10) {
>>>          lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
>>>          if (excp_model != POWERPC_EXCP_POWER7) {
>>>              ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>>>          } else {
>>>              ail = 0;
>>>          }
>>> +        if (excp_model == POWERPC_EXCP_POWER10) {
>>> +            if (AIL_0001_8000) {
>> 
>> if (2)
>> 
>>> +                ail = 0; /* AIL=2 is reserved in ISA v3.1 */
>>> +            }
>>> +
>>> +            if (env->spr[SPR_LPCR] & LPCR_HAIL) {
>>> +                hail = AIL_C000_0000_0000_4000;
>>> +            } else {
>>> +                hail = 0;
>>> +            }
>>> +        } else {
>>> +            hail = ail;
>> 
>> I think it's better if we set hail = 0 here. Read on...
>> 
>>> +        }
>>>      } else
>>>  #endif /* defined(TARGET_PPC64) */
>>>      {
>>>          lpes0 = true;
>>>          ail = 0;
>>> +        hail = 0;
>>>      }
>>>  
>>>      /*
>>> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>              new_msr |= (target_ulong)MSR_HVB;
>>>          }
>>>          ail = 0;
>>> +        hail = 0;
>>>  
>>>          /* machine check exceptions don't have ME set */
>>>          new_msr &= ~((target_ulong)1 << MSR_ME);
>>> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>              }
>>>          }
>>>          ail = 0;
>>> +        hail = 0;
>>>          break;
>>>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>>>      case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
>>> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>          } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
>>>              new_msr |= (target_ulong)1 << MSR_LE;
>>>          }
>>> -    } else if (excp_model == POWERPC_EXCP_POWER9) {
>>> +    } else if (excp_model == POWERPC_EXCP_POWER9 ||
>>> +               excp_model == POWERPC_EXCP_POWER10) {
>>>          if (new_msr & MSR_HVB) {
>>>              if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
>>>                  new_msr |= (target_ulong)1 << MSR_LE;
>>> @@ -791,22 +810,77 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>  #endif
>>>  
>>>      /*
>>> -     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
>>> +     * The ail variable is for AIL behaviour for interrupts that begin
>>> +     * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
>>> +     * interrupts that begin execution with MSR[HV]=1.
>>> +     *
>>> +     * There is a large matrix of behaviours depending on the processor and
>>> +     * the current operating modes when the interrupt is taken, and the
>>> +     * interrupt type. The below tables lists behaviour for interrupts except
>>> +     * for SRESET, machine check, and HMI (which are all taken in real mode
>>> +     * with AIL 0).
>>> +     *
>>> +     * POWER8, POWER9 with LPCR[HR]=0
>>> +     * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
>>> +     * +-----------+-------------+---------+-------------+-----+
>>> +     * | a         | 00/01/10    | x       | x           | 0   |
>>> +     * | a         | 11          | 0       | 1           | 0   |
>>> +     * | a         | 11          | 1       | 1           | a   |
>>> +     * | a         | 11          | 0       | 0           | a   |
>>> +     * +-------------------------------------------------------+
>>> +     *
>>> +     * POWER9 with LPCR[HR]=1
>>> +     * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
>>> +     * +-----------+-------------+---------+-------------+-----+
>>> +     * | a         | 00/01/10    | x       | x           | 0   |
>>> +     * | a         | 11          | x       | x           | a   |
>>> +     * +-------------------------------------------------------+
>>> +     *
>>> +     * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
>>> +     * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
>>> +     * This is good for performance but allows the guest to influence the
>>> +     * AIL of the hypervisor interrupt using its MSR, and also the hypervisor
>>> +     * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the
>>> +     * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts.
>>> +     *
>>> +     * POWER10 addresses those issues with a new LPCR[HAIL] bit that is
>>> +     * applied to interrupt that begin execution with MSR[HV]=1 (so both
>>> +     * MSR[HV] 0->1 and 1->1).
>> 
>> I'd put take these two paragraphs in the commit message as well. Just
>> alter the beginning:
>> s/The difference with POWER9 being that/In POWER9,/
>> 
>>> +     *
>>> +     * POWER10 behaviour is
>>> +     * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
>>> +     * +-----------+------------+-------------+---------+-------------+-----+
>>> +     * | a         | h          | 00/01/10    | 0       | 0           | 0   |
>>> +     * | a         | h          | 11          | 0       | 0           | a   |
>>> +     * | a         | h          | x           | 0       | 1           | h   |
>>> +     * | a         | h          | 00/01/10    | 1       | 1           | 0   |
>>> +     * | a         | h          | 11          | 1       | 1           | h   |
>>> +     * +--------------------------------------------------------------------+
>>>       */
>>> -    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>> -        ail = 0;
>>> -    }
>>>  
>>> -    /*
>>> -     * AIL does not work if there is a MSR[HV] 0->1 transition and the
>>> -     * partition is in HPT mode. For radix guests, such interrupts are
>>> -     * allowed to be delivered to the hypervisor in ail mode.
>>> -     */
>>> -    if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
>>> -        if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
>>> +    if (excp_model == POWERPC_EXCP_POWER8 ||
>>> +        excp_model == POWERPC_EXCP_POWER9) {
>>> +        if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>>              ail = 0;
>>> +            hail = 0;
>>> +        } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
>>> +            if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
>>> +                hail = 0;
>>> +            }
>>> +        }
>>> +    } else if (excp_model == POWERPC_EXCP_POWER10) {
>>> +        if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
>>> +            if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>> +                ail = 0;
>>> +                hail = 0;
>>> +            }
>>>          }
>>>      }
>>> +    if (new_msr & MSR_HVB) {
>>> +        using_ail = hail;
>>> +    } else {
>>> +        using_ail = ail;
>>> +    }
>> 
>> So if at the start of the function we set:
>> 
>> <= P7:
>> ail = 0;
>> hail = 0;
>> 
>> P8,P9:
>> ail = LPCR_AIL;
>> hail = 0;
>> 
>> P10:
>> ail = LPCR_AIL, except AIL2;
>> hail = AIL3;
>> 
>> Then we could do this, which I think is more readable (comments are just
>> for the email):
>> 
>> // If there's an HV transition to 1, then HPT zeroes AIL and radix
>> // doesn't. For P10 we'll use HAIL anyway, so no need to check.
>> 
>> if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) &&
>>     !(env->spr[SPR_LPCR] & LPCR_HR)) {
>>           ail = 0;
>> }
>> 
>> // If P10 and HV=1, use HAIL instead of AIL
>> 
>> if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) {
>>    ail = hail;
>> }
>> 
>> // The IR|DR check always takes precedence and zeroes AIL/HAIL, no
>> matter the processor version.
>> 
>> if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>     ail = 0;
>> }
>> 
>> (...)
>> 
>> vector |= ppc_excp_vector_offset(cs, ail);
>> 
>> I think that should work...*squints* 
>> What do you think?
> 
> The powerpc_excp() routine is insane.
> 
> It feels that we should be duplicating the code in handlers for each 
> of the POWER[7-10] CPUs and keep a default powerpc_excp() for the
> others.

It's getting pretty wild :(

I was trying to come up with something in the way of a "fix" but I agree 
it needs a bigger reorganisation.

What would help a lot actually is moving all the AIL logic together. The 
only reason it's split completely in two like this is so a few 
interrupts can clear it. I think what would work better is just do all
that in one place, in the AIL logic. Let me see what that looks like. It
might actually be a saner fix in the end.

Thanks,
Nick