RE: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, February 02, 2013 4:09 AM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest
> 
> On 01/31/2013 06:11:32 PM, Alexander Graf wrote:
> >
> > On 31.01.2013, at 23:40, Scott Wood wrote:
> >
> > > On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
> > >> On 31.01.2013, at 20:05, Alexander Graf wrote:
> > >> >
> > >> > On 31.01.2013, at 19:54, Scott Wood wrote:
> > >> >
> > >> >> On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
> > >> >>> On 31.01.2013, at 19:43, Scott Wood wrote:
> > >> >>>> On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
> > >> >>>>> How about something like this? Then both targets at least
> > suck as much :).
> > >> >>>>
> > >> >>>> I'm not sure that should be the goal...
> > >> >>>>
> > >> >>>>> Thanks to e500mc's awful hardware design, we don't know who
> > sets the MSR_DE bit. Once we forced it onto the guest, we have no
> > change to know whether the guest also set it or not. We could only
> > guess.
> > >> >>>>
> > >> >>>> MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but
> > we still need to set it in the first place.
> > >> >>>>
> > >> >>>> According to ISA V2.06B, the hypervisor should set DBCR0[EDM]
> > to let the guest know that the debug resources are not available, and
> > that "the value of MSR[DE] is not specified and not modifiable".
> > >> >>> So what would the guest do then to tell the hypervisor that it
> > actually wants to know about debug events?
> > >> >>
> > >> >> The guest is out of luck, just as if a JTAG were in use.
> > >> >
> > >> > Hrm.
> > >> >
> > >> > Can we somehow generalize this "out of luck" behavior?
> > >> >
> > >> > Every time we would set or clear an MSR bit in shadow_msr on
> > e500v2, we would instead set or clear it in the real MSR. That way
> > only e500mc is out of luck, but the code would still be shared.
> > >
> > > I don't follow.  e500v2 is just as out-of-luck.  The mechanism
> > simply does not support sharing debug resources.
> >
> > For e500v2 we have 2 fields
> >
> >   * MSR as the guest sees it
> >   * MSR as we execute when the guest runs
> >
> > Since we know the MSR when the guest sees it, we can decide what to do
> > when we get an unhandled debug interrupt.
> 
> That's not the same thing as making the real MSR[DE] show up in the guest
> MSR[DE].
> 
> There are other problems with sharing -- what happens when both host and guest
> try to write to a particular IAC or DAC?
> 
> Also, performance would be pretty awful if the guest has e.g. single stepping in
> DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single stepping
> (but does want debugging enabled in general).
> 
> > > What do you mean by "the real MSR"?  The real MSR is shadow_msr,
> > and MSR_DE must always be set there if the host is debugging the
> > guest.  As for reflecting it into the guest MSR, we could, but I don't
> > really see the point.  We're never going to actually send a debug
> > exception to the guest when the host owns the debug resources.
> >
> > Why not? That's the whole point of jumping through user space.
> 
> That's still needed for software breakpoints, which don't rely on the debug
> resources.
> 
> >   1) guest exits with debug interrupt
> >   2) QEMU gets a debug exit
> >   3) QEMU checks in its list whether it belongs to its own debug
> > points
> >   4) if not, it reinjects the interrupt into the guest
> >
> > Step 4 is pretty difficult to do when we don't know whether the guest
> > is actually capable of handling debug interrupts at that moment.
> 
> Software breakpoints take a Program interrupt rather than a Debug interrupt,
> unless MSR[DE]=1 and DBCR0[TRAP]=1.  If the guest does not own debug resources
> we should always send it to the Program interrupt, so MSR[DE] doesn't matter.
> 
> > > The "&= ~MSR_DE" line is pointless on bookehv, and makes it harder
> > to read.  I had to stare at it a while before noticing that you
> > initially set is_debug from the guest MSR and that you'd never really
> > clear MSR_DE here on bookehv.
> >
> > Well, I'm mostly bouncing ideas here to find a way to express what
> > we're trying to say in a way that someone who hasn't read this email
> > thread would still understand what's going on :).
> 
> I think it's already straightforward enough if you accept that shared debug
> resources aren't supported, and that we are either in a mode where the real
> MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is always
> on in guest mode and the guest MSR[DE] is irrelevant.
> 
> > How about this version?
> >
> >
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > 38a62ef..9929c41 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
> > *vcpu)
> >  #endif
> >  }
> >
> > +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { #ifndef
> > +CONFIG_KVM_BOOKE_HV
> > +	/* Synchronize guest's desire to get debug interrupts into
> > shadow MSR */
> > +	vcpu->arch.shadow_msr &= ~MSR_DE;
> > +	vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif
> > +
> > +	/* Force enable debug interrupts when user space wants to debug
> > */
> > +	if (vcpu->guest_debug) {
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +		/*
> > +		 * Since there is no shadow MSR, sync MSR_DE into the
> > guest
> > +		 * visible MSR.
> > +		 */
> > +		vcpu->arch.shared->msr |= MSR_DE;
> > +#else
> > +		vcpu->arch.shadow_msr |= MSR_DE;
> > +#endif
> > +	}
> > +}
> 
> This shows "guest's desire to get debug interrupts" in a context that is not
> specifically for !vcpu->guest_debug, which is misleading.
> 
> > +
> >  /*
> >   * Helper function for "full" MSR writes.  No need to call this if
> > only
> >   * EE/CE/ME/DE/RI are changing.
> > @@ -150,6 +172,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32
> > new_msr)
> >  	kvmppc_mmu_msr_notify(vcpu, old_msr);
> >  	kvmppc_vcpu_sync_spe(vcpu);
> >  	kvmppc_vcpu_sync_fpu(vcpu);
> > +	kvmppc_vcpu_sync_debug(vcpu);
> >  }
> >
> >  static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
> >
> >
> > My main concern here is that we don't know when to remove MSR_DE again
> > from the (shadow) MSR. So how about this one instead?
> 
> Why wouldn't you know this?  if (vcpu->guest_debug) { you never remove it } else
> { just copy whatever's in guest MSR }

I think we are ok with shadow_msr on e500v2 but we can have problem on bookehv where we do not know when to clear MSR_DE in shared->msr.

How it works on e500mc:
	(1) User-space makes ioctl to use debug resource, we set vcpu->guest_debug.
	(2) Before entering into the guest we check vcpu->guest_debug flag and if set we set MSR_DE in shared->msr.
	(3) Sometime later user-space releases the debug resource then in ioctl handling will clear vcpu->guest_debug.
	(4) Now when entering to guest we do not know what to do with MSR_DE in shared->msr as we do now know if guest might have tried to set/clear MSR_DE in between step (2) and step(3). What should be safe thing to do? Can we leave MSR_DE set or clear MSR_DE. If we want to clear MSR_DE then will it be good idea to clear this in step (3) above (in ioctl where we clear vcpu->guest_debug).


On e500v2 we have separate shadow_msr and shared->msr so the flow can be:
	(1) User-space makes ioctl to use debug resource, we set vcpu->guest_debug.
	(2) Before entering into the guest we check vcpu->guest_debug flag and we set MSR_DE in vcpu->arch.shadow_msr. And if vcpu->guest_debug flag is not set then use vcpu->arch.shared->msr for MSR_DE in shadow_msr.
	(3) User-space releases the debug resource then in ioctl handling will clear vcpu->guest_debug.
	(4) Now when entering to guest set shadow_msr as per vcpu->arch.shared->msr. 

Thanks
-Bharat


> 
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > 38a62ef..2676703 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -142,7 +142,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32
> > new_msr)
> >  	u32 old_msr = vcpu->arch.shared->msr;
> >
> >  #ifdef CONFIG_KVM_BOOKE_HV
> > -	new_msr |= MSR_GS;
> > +	new_msr |= MSR_GS | MSR_DE;
> >  #endif
> >
> >  	vcpu->arch.shared->msr = new_msr;
> >
> >
> > That would semantically move e500mc to the same logic as e500v2. With
> > the main difference that we have no idea what MSR_DE value the guest
> > really wanted to have set.
> 
> This would break the case where the guest owns the debug resources.
> 
> > If I read the spec correctly, rfci traps.
> 
> rfdi is the relevant one for e500mc, but yes.
> 
> > So we know the time frame from [inject debug interrupt ... rfci].
> > During that time we know for sure that the guest thinks MSR_DE is 0.
> 
> No, we don't.  The guest could have tried to use mtmsr or rfi to enable MSR[DE].
> It could have seen the context it came from was userspace, and scheduled to
> another process, etc.
> 
> > Outside of that context, we just have to assume the guest can always
> > receive debug interrupts if it configured them.
> 
> No.
> 
> -Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux