[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
From: |
Nicholas Piggin |
Subject: |
Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model |
Date: |
Wed, 14 Apr 2021 11:56:53 +1000 |
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
- [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour, (continued)