From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest Date: Fri, 1 Feb 2013 16:38:50 -0600 Message-ID: <1359758330.23561.8@snotra> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: Bhushan Bharat-R65777 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" To: Alexander Graf Return-path: Received: from ch1ehsobe005.messaging.microsoft.com ([216.32.181.185]:48281 "EHLO ch1outboundpool.messaging.microsoft.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757779Ab3BAWi4 convert rfc822-to-8bit (ORCPT ); Fri, 1 Feb 2013 17:38:56 -0500 In-Reply-To: (from agraf@suse.de on Thu Jan 31 18:11:32 2013) Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: 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 } > 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