Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]

On Mon, Oct 24, 2016 at 05:56:22PM +1100, Nicholas Piggin wrote:
> On Mon, 24 Oct 2016 12:16:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Oct 21, 2016 at 03:35:43PM +1100, Nicholas Piggin wrote:
> > > On Fri, 21 Oct 2016 12:09:54 +1100
> > > David Gibson <david@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 <clg@kaod.org> wrote:
> > > > >     
> > > > > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:    
> > > > > > > Signed-off-by: Nicholas Piggin <npiggin@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 <npiggin@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?
> 
> Correct.

Ok.  Hmm.  Could you please do a minor respin and:
  - change the flag to a bool
  - add a comment explaining this meaning
  - if you can think of a name which makes the meaning more obvious (I
    can't, quickly), change that too

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]