From: Nicholas Piggin <npiggin@gmail.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-devel@nongnu.org, "Cédric Le Goater" <clg@fr.ibm.com>,
qemu-ppc@nongnu.org, "Cédric Le Goater" <clg@kaod.org>,
"Fabiano Rosas" <farosas@linux.ibm.com>
Subject: Re: [PATCH v2 3/4] target/ppc: Rework AIL logic in interrupt delivery
Date: Sat, 17 Apr 2021 13:17:01 +1000 [thread overview]
Message-ID: <1618627370.x7ngc3hv1o.astroid@bobo.none> (raw)
In-Reply-To: <YHkRk653hdZwUZiq@yekko.fritz.box>
Excerpts from David Gibson's message of April 16, 2021 2:24 pm:
> On Thu, Apr 15, 2021 at 03:42:26PM +1000, Nicholas Piggin wrote:
>> The AIL logic is becoming unmanageable spread all over powerpc_excp(),
>> and it is slated to get even worse with POWER10 support.
>>
>> Move it all to a new helper function.
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> Tested-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
> Looks like a nice cleanup overall, just a few minor comments.
>
>> ---
>> hw/ppc/spapr_hcall.c | 3 +-
>> target/ppc/cpu.h | 8 --
>> target/ppc/excp_helper.c | 159 ++++++++++++++++++++------------
>> target/ppc/translate_init.c.inc | 2 +-
>> 4 files changed, 102 insertions(+), 70 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 7b5cd3553c..2fbe04a689 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1395,7 +1395,8 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>> return H_P4;
>> }
>>
>> - if (mflags == AIL_RESERVED) {
>> + if (mflags == 1) {
>> + /* AIL=1 is reserved */
>> return H_UNSUPPORTED_FLAG;
>> }
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index e73416da68..5200a16d23 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -2375,14 +2375,6 @@ enum {
>> HMER_XSCOM_STATUS_MASK = PPC_BITMASK(21, 23),
>> };
>>
>> -/* Alternate Interrupt Location (AIL) */
>> -enum {
>> - AIL_NONE = 0,
>> - AIL_RESERVED = 1,
>> - AIL_0001_8000 = 2,
>> - AIL_C000_0000_0000_4000 = 3,
>> -};
>
> Yeah, I always thought these particular constants were a but
> pointless.
>
>> -
>> /*****************************************************************************/
>>
>> #define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index b8881c0f85..964a58cfdc 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -136,25 +136,105 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
>> return POWERPC_EXCP_RESET;
>> }
>>
>> -static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
>> +/*
>> + * AIL - Alternate Interrupt Location, a mode that allows interrupts to be
>> + * taken with the MMU on, and which uses an alternate location (e.g., so the
>> + * kernel/hv can map the vectors there with an effective address).
>> + *
>> + * An interrupt is considered to be taken "with AIL" or "AIL applies" if they
>> + * are delivered in this way. AIL requires the LPCR to be set to enable this
>> + * mode, and then a number of conditions have to be true for AIL to apply.
>> + *
>> + * First of all, SRESET, MCE, and HMI are always delivered without AIL, because
>> + * they specifically want to be in real mode (e.g., the MCE might be signaling
>> + * a SLB multi-hit which requires SLB flush before the MMU can be enabled).
>> + *
>> + * After that, behaviour depends on the current MSR[IR], MSR[DR], MSR[HV],
>> + * whether or not the interrupt changes MSR[HV] from 0 to 1, and the current
>> + * radix mode (LPCR[HR]).
>> + *
>> + * 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.
>> + */
>> +static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
>> + target_ulong msr,
>> + target_ulong *new_msr,
>> + target_ulong *vector)
>> {
>> - uint64_t offset = 0;
>> +#if defined(TARGET_PPC64)
>> + CPUPPCState *env = &cpu->env;
>> + bool mmu_all_on = ((msr >> MSR_IR) & 1) && ((msr >> MSR_DR) & 1);
>> + bool hv_escalation = !(msr & MSR_HVB) && (*new_msr & MSR_HVB);
>> + int ail = 0;
>> +
>> + if (excp == POWERPC_EXCP_MCHECK ||
>> + excp == POWERPC_EXCP_RESET ||
>> + excp == POWERPC_EXCP_HV_MAINT) {
>> + /* SRESET, MCE, HMI never apply AIL */
>> + return;
>> + }
>>
>> - switch (ail) {
>> - case AIL_NONE:
>> - break;
>> - case AIL_0001_8000:
>> - offset = 0x18000;
>> - break;
>> - case AIL_C000_0000_0000_4000:
>> - offset = 0xc000000000004000ull;
>> - break;
>> - default:
>> - cpu_abort(cs, "Invalid AIL combination %d\n", ail);
>> - break;
>> + if (excp_model == POWERPC_EXCP_POWER8 ||
>> + excp_model == POWERPC_EXCP_POWER9) {
>> + if (!mmu_all_on) {
>> + /* AIL only works if MSR[IR] and MSR[DR] are both enabled. */
>> + return;
>> + }
>> + if (hv_escalation && !(env->spr[SPR_LPCR] & LPCR_HR)) {
>> + /*
>> + * 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.
>> + */
>> + return;
>> + }
>> +
>> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> + if (ail != 2 && ail != 3) {
>> + /* AIL=1 is reserved */
>
> So, AIL==0 and AIL==1 are treated the same here, but for kinda
> different reasons. AIL==0 means no offset should be applied. AIL==1
> is invalid, so we're just ignoring AIL in that case.
Could comment that specifically at least.
> I wonder if it would make things clearer to filter the AIL==1 case at
> LPCR write time, and just assert() it's not the case here.
Let's discuss that in the next mail.
>
>> + return;
>> + }
>> + } else {
>> + /* Other processors do not support AIL */
>> + return;
>> }
>>
>> - return offset;
>> + /*
>> + * AIL applies, so the new MSR gets IR and DR set, and an offset applied
>> + * to the new IP.
>> + */
>> + *new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>> +
>> + if (excp != POWERPC_EXCP_SYSCALL_VECTORED) {
>> + if (ail == 2) {
>> + *vector |= 0x0000000000018000ull;
>> + } else if (ail == 3) {
>> + *vector |= 0xc000000000004000ull;
>> + }
>> + } else {
>> + /* scv AIL is a little different */
>
> What happens with AIL==2 and an SCV? I mean, here it's as if AIL==0,
> but is that right? If so, I think we should comment it to make it
> clear that's not an omission.
Yes as far as I can tell that's what the ISA specifies (i.e., NIA is
unchanged).
Sure a comment can be added.
Thanks,
Nick
next prev parent reply other threads:[~2021-04-17 3:18 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-15 5:42 [PATCH v2 0/4] ppc: rework AIL logic, add POWER10 exception model Nicholas Piggin
2021-04-15 5:42 ` [PATCH v2 1/4] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
2021-04-15 12:12 ` Fabiano Rosas
2021-04-16 4:13 ` David Gibson
2021-04-15 5:42 ` [PATCH v2 2/4] target/ppc: POWER10 supports scv Nicholas Piggin
2021-04-15 7:43 ` [EXTERNAL] " Cédric Le Goater
2021-04-16 4:15 ` David Gibson
2021-04-15 5:42 ` [PATCH v2 3/4] target/ppc: Rework AIL logic in interrupt delivery Nicholas Piggin
2021-04-16 4:24 ` David Gibson
2021-04-17 3:17 ` Nicholas Piggin [this message]
2021-04-15 5:42 ` [PATCH v2 4/4] target/ppc: Add POWER10 exception model Nicholas Piggin
2021-04-16 4:28 ` David Gibson
2021-04-17 3:31 ` Nicholas Piggin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1618627370.x7ngc3hv1o.astroid@bobo.none \
--to=npiggin@gmail.com \
--cc=clg@fr.ibm.com \
--cc=clg@kaod.org \
--cc=david@gibson.dropbear.id.au \
--cc=farosas@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).