On Fri, Oct 21, 2016 at 03:35:43PM +1100, Nicholas Piggin wrote:
> On Fri, 21 Oct 2016 12:09:54 +1100
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
> > On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:
> > > On Thu, 20 Oct 2016 15:08:07 +0200
> > > Cédric Le Goater <c...@kaod.org> wrote:
> > >
> > > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:
> > > > > Signed-off-by: Nicholas Piggin <npig...@gmail.com>
> > > > > ---
> > > > > 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 *cpu,
> > > > > int excp_model, int excp)
> > > > > /* indicate that we resumed from power save mode */
> > > > > msr |= 0x10000;
> > > > > new_msr |= ((target_ulong)1 << MSR_ME);
> > > > > + new_msr |= (target_ulong)MSR_HVB;
> > > > > + } else {
> > > > > + /* The ISA specifies the HV bit is set when the hardware
> > > > > interrupt
> > > > > + * is raised, however when hypervisors deliver the
> > > > > exception to
> > > > > + * guests, it should not be set.
> > > > > + */
> > > > > }
> > > > > -
> > > > > - new_msr |= (target_ulong)MSR_HVB;
> > > > > ail = 0;
> > > > > break;
> > > > > case POWERPC_EXCP_DSEG: /* Data segment exception
> > > > > */
> > > > >
> > > >
> > > > should not that be cleared later on in powerpc_excp() by :
> > > >
> > > > env->msr = new_msr & env->msr_mask;
> > > >
> > > > ? but the routine is rather long so I might be missing a branch.
> > >
> > > 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?
> >
> > I think the proposed revision makes sense.
> >
>
> 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, because
> it's a bugfix for existing code. I'll get around to the hcall again next
> week.
>
> Thanks,
> Nick
>
>
> ppc hypervisors have delivered system reset and machine check exception
> interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
> or NMI injection in QEMU).
>
> 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 after
> the interrupt delivery model has calculated delivery mode for the exception.
> This can result in the guest's MSR LE bit being lost.
>
> Provide a new flag for HV exceptions to allow delivery to guests. The
> exception model masks out the HV bit.
>
> Also add another sanity check to ensure other such exceptions don't try
> to set HV in guest without setting guest_hv_excp
>
> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
> ---
> target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> 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 = CPU(cpu);
> CPUPPCState *env = &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;
So, to clarify my understanding of this.
The guest_hv_excp flag indicates that this is a normally-HV exception
which *could* be delivered to a guest with HV clear, *not* that we're
actually doing so in this instance. Yes?
> bool lpes0;
>
> qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -149,6 +149,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> 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 == POWERPC_EXCP_POWER7 ||
> @@ -165,6 +169,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> excp_model, int excp)
> lpes0 = true;
> ail = 0;
> }
> + guest_hv_excp = 0;
>
> /* 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, int
> excp_model, int excp)
> cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> }
> new_msr |= (target_ulong)MSR_HVB;
> + guest_hv_excp = 1;
> ail = 0;
>
> /* machine check exceptions don't have ME set */
> @@ -391,8 +397,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> excp_model, int excp)
> msr |= 0x10000;
> new_msr |= ((target_ulong)1 << MSR_ME);
> }
> -
> new_msr |= (target_ulong)MSR_HVB;
> + guest_hv_excp = 1;
> ail = 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)
>
> /* Sanity check */
> if (!(env->msr_mask & MSR_HVB) && (srr0 == 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);
> }
>
> + /* The ISA specifies the HV bit is set when the hardware interrupt
> + * is raised, however in some cases hypervisors deliver the exception
> + * 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 &= ~MSR_HVB;
> + } else {
> + cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
> + "no HV support\n", excp);
> + }
> + }
> +
> /* If any alternate SRR register are defined, duplicate saved values */
> if (asrr0 != -1) {
> env->spr[asrr0] = env->spr[srr0];
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
