From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59978) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxWyI-0008B4-FN for qemu-devel@nongnu.org; Fri, 21 Oct 2016 06:22:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxWyD-0006EZ-Fr for qemu-devel@nongnu.org; Fri, 21 Oct 2016 06:22:42 -0400 Received: from 9.mo69.mail-out.ovh.net ([46.105.56.78]:39146) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxWyD-0006DE-7f for qemu-devel@nongnu.org; Fri, 21 Oct 2016 06:22:37 -0400 Received: from player798.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo69.mail-out.ovh.net (Postfix) with ESMTP id 7DE1C7F76 for ; Fri, 21 Oct 2016 12:22:34 +0200 (CEST) References: <20161020065912.16132-1-npiggin@gmail.com> <20161020065912.16132-3-npiggin@gmail.com> <20161021004058.074a7769@roar.ozlabs.ibm.com> <20161021010954.GY11140@umbus.fritz.box> <20161021153543.294dfa9d@roar.ozlabs.ibm.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: Date: Fri, 21 Oct 2016 12:22:28 +0200 MIME-Version: 1.0 In-Reply-To: <20161021153543.294dfa9d@roar.ozlabs.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nicholas Piggin , David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Benjamin Herrenschmidt On 10/21/2016 06:35 AM, Nicholas Piggin wrote: > On Fri, 21 Oct 2016 12:09:54 +1100 > David Gibson wrote: >=20 >> On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote: >>> On Thu, 20 Oct 2016 15:08:07 +0200 >>> C=C3=A9dric Le Goater wrote: >>> =20 >>>> On 10/20/2016 08:59 AM, Nicholas Piggin wrote: =20 >>>>> Signed-off-by: Nicholas Piggin >>>>> --- >>>>> target-ppc/excp_helper.c | 8 ++++++-- >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >>>>> index 53c4075..477af10 100644 >>>>> --- a/target-ppc/excp_helper.c >>>>> +++ b/target-ppc/excp_helper.c >>>>> @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cp= u, int excp_model, int excp) >>>>> /* indicate that we resumed from power save mode */ >>>>> msr |=3D 0x10000; >>>>> new_msr |=3D ((target_ulong)1 << MSR_ME); >>>>> + new_msr |=3D (target_ulong)MSR_HVB; >>>>> + } else { >>>>> + /* The ISA specifies the HV bit is set when the hardware inte= rrupt >>>>> + * is raised, however when hypervisors deliver the exception = to >>>>> + * guests, it should not be set. >>>>> + */ >>>>> } >>>>> - >>>>> - new_msr |=3D (target_ulong)MSR_HVB; >>>>> ail =3D 0; >>>>> break; >>>>> case POWERPC_EXCP_DSEG: /* Data segment exception = */ >>>>> =20 >>>> >>>> should not that be cleared later on in powerpc_excp() by : >>>> >>>> env->msr =3D new_msr & env->msr_mask; >>>> >>>> ? but the routine is rather long so I might be missing a branch. =20 >>> >>> No you're right, so it can't leak into the guest, phew! >>> >>> The problem I get is the interrupt code doing some things differently >>> depending on on the HV bit. For example what I noticed is the guest >>> losing its LE bit upon entry. >>> >>> Perhaps a cleaner way is for the system reset case to set new_msr >>> according to the ISA, and then apply the msr_mask (or at least mask >>> out HV) before calculating the exception model? Any preference? =20 >> >> I think the proposed revision makes sense. >> >=20 > What do you think of this version? This fixes up machine check guest > delivery as well. I'm sending this ahead of the new hcall patch, becaus= e > it's a bugfix for existing code. I'll get around to the hcall again nex= t > week. >=20 > Thanks, > Nick >=20 >=20 > ppc hypervisors have delivered system reset and machine check exception > interrupts to guests in some situations (e.g., see FWNMI feature of LoP= APR, > or NMI injection in QEMU). >=20 > These exceptions are architected to set the HV bit in hardware, however > when injected into a guest, the HV bit should be cleared. Current code > masks off the HV bit before setting the new MSR, however this happens a= fter > the interrupt delivery model has calculated delivery mode for the excep= tion. > This can result in the guest's MSR LE bit being lost. >=20 > Provide a new flag for HV exceptions to allow delivery to guests. The > exception model masks out the HV bit. >=20 > Also add another sanity check to ensure other such exceptions don't try > to set HV in guest without setting guest_hv_excp >=20 > Signed-off-by: Nicholas Piggin Looks good to me and I gave it a try on the pnv platform which runs in HV mode. Reviewed-by: C=C3=A9dric Le Goater Thanks, C.=20 > --- > target-ppc/excp_helper.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) >=20 > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > index 53c4075..1b18433 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) > CPUState *cs =3D CPU(cpu); > CPUPPCState *env =3D &cpu->env; > target_ulong msr, new_msr, vector; > - int srr0, srr1, asrr0, asrr1, lev, ail; > + int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp; > bool lpes0; > =20 > qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx > @@ -149,6 +149,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, i= nt excp_model, int excp) > * > * AIL is initialized here but can be cleared by > * selected exceptions > + * > + * guest_hv_excp is a provision to raise HV architected > + * exceptions in guests by delivering them with HV bit > + * clear. System reset and machine check use this. > */ > #if defined(TARGET_PPC64) > if (excp_model =3D=3D POWERPC_EXCP_POWER7 || > @@ -165,6 +169,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, in= t excp_model, int excp) > lpes0 =3D true; > ail =3D 0; > } > + guest_hv_excp =3D 0; > =20 > /* Hypervisor emulation assistance interrupt only exists on server > * arch 2.05 server or later. We also don't want to generate it if > @@ -214,6 +219,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, in= t excp_model, int excp) > cs->interrupt_request |=3D CPU_INTERRUPT_EXITTB; > } > new_msr |=3D (target_ulong)MSR_HVB; > + guest_hv_excp =3D 1; > ail =3D 0; > =20 > /* machine check exceptions don't have ME set */ > @@ -391,8 +397,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, in= t excp_model, int excp) > msr |=3D 0x10000; > new_msr |=3D ((target_ulong)1 << MSR_ME); > } > - > new_msr |=3D (target_ulong)MSR_HVB; > + guest_hv_excp =3D 1; > ail =3D 0; > break; > case POWERPC_EXCP_DSEG: /* Data segment exception = */ > @@ -610,10 +616,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, = int excp_model, int excp) > =20 > /* Sanity check */ > if (!(env->msr_mask & MSR_HVB) && (srr0 =3D=3D SPR_HSRR0)) { > - cpu_abort(cs, "Trying to deliver HV exception %d with " > + cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with " > "no HV support\n", excp); > } > =20 > + /* The ISA specifies the HV bit is set when the hardware interrupt > + * is raised, however in some cases hypervisors deliver the except= ion > + * to guests. HV should be cleared in that case. > + */ > + if (!(env->msr_mask & MSR_HVB) && (new_msr & MSR_HVB)) { > + if (guest_hv_excp) { > + new_msr &=3D ~MSR_HVB; > + } else { > + cpu_abort(cs, "Trying to deliver HV exception (MSR) %d wit= h " > + "no HV support\n", excp); > + } > + } > + > /* If any alternate SRR register are defined, duplicate saved valu= es */ > if (asrr0 !=3D -1) { > env->spr[asrr0] =3D env->spr[srr0]; >=20