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: Thu, 31 Jan 2013 16:40:19 -0600 Message-ID: <1359672019.31540.29@snotra> References: <16DEE4DC-E963-49FB-B211-810D1068E6C4@suse.de> 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: In-Reply-To: <16DEE4DC-E963-49FB-B211-810D1068E6C4@suse.de> (from agraf@suse.de on Thu Jan 31 13:20:39 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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. 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. Speaking of naming issues, "guest_debug" is very ambiguous... > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index 38a62ef..9bdb845 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu > *vcpu) > #endif > } > > +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) > +{ > + u32 is_debug = vcpu->arch.shared->msr & MSR_DE; > + > + /* Force debug to on in guest space when user space wants to > debug */ > + if (vcpu->guest_debug) > + is_debug = MSR_DE; > + > +#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; > + vcpu->arch.shared->msr |= is_debug; > +#endif > + > +#ifndef CONFIG_KVM_BOOKE_HV > + vcpu->arch.shadow_msr &= ~MSR_DE; > + vcpu->arch.shadow_msr |= is_debug; > +#endif > +} 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. -Scott