On 31.01.2013, at 18:59, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On Behalf Of >> Alexander Graf >> Sent: Thursday, January 31, 2013 5:34 PM >> To: Bhushan Bharat-R65777 >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest >> >> >> On 30.01.2013, at 12:12, Bhushan Bharat-R65777 wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: kvm-ppc-owner@xxxxxxxxxxxxxxx >>>> [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander Graf >>>> Sent: Friday, January 25, 2013 5:44 PM >>>> To: Bhushan Bharat-R65777 >>>> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Bhushan >>>> Bharat-R65777 >>>> Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt >>>> injection to guest >>>> >>>> >>>> On 16.01.2013, at 09:24, Bharat Bhushan wrote: >>>> >>>>> Allow userspace to inject debug interrupt to guest. QEMU can >>>> >>>> s/QEMU/user space. >>>> >>>>> inject the debug interrupt to guest if it is not able to handle the >>>>> debug interrupt. >>>>> >>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx> >>>>> --- >>>>> arch/powerpc/kvm/booke.c | 32 +++++++++++++++++++++++++++++++- >>>>> arch/powerpc/kvm/e500mc.c | 10 +++++++++- >>>>> 2 files changed, 40 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >>>>> index faa0a0b..547797f 100644 >>>>> --- a/arch/powerpc/kvm/booke.c >>>>> +++ b/arch/powerpc/kvm/booke.c >>>>> @@ -133,6 +133,13 @@ static void kvmppc_vcpu_sync_fpu(struct >>>>> kvm_vcpu >>>>> *vcpu) #endif } >>>>> >>>>> +#ifdef CONFIG_KVM_BOOKE_HV >>>>> +static int kvmppc_core_pending_debug(struct kvm_vcpu *vcpu) { >>>>> + return test_bit(BOOKE_IRQPRIO_DEBUG, >>>>> +&vcpu->arch.pending_exceptions); } #endif >>>>> + >>>>> /* >>>>> * Helper function for "full" MSR writes. No need to call this if >>>>> only >>>>> * EE/CE/ME/DE/RI are changing. >>>>> @@ -144,7 +151,11 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 >>>>> new_msr) #ifdef CONFIG_KVM_BOOKE_HV >>>>> new_msr |= MSR_GS; >>>>> >>>>> - if (vcpu->guest_debug) >>>>> + /* >>>>> + * Set MSR_DE if the hardware debug resources are owned by user-space >>>>> + * and there is no debug interrupt pending for guest to handle. >>>> >>>> Why? >>> >>> QEMU is using the IAC/DAC registers to set hardware breakpoint/watchpoints via >> debug ioctls. As debug events are enabled/gated by MSR_DE so somehow we need to >> set MSR_DE on hardware MSR when guest is running in this case. >> >> Reading this 5 times I still have no idea what you're really checking for here. >> Maybe the naming for kvmppc_core_pending_debug is just unnatural? What does that >> function do really? >> >>> >>> On bookehv this is how I am controlling the MSR_DE in hardware MSR. >>> >>>> And why is this whole thing only executed on HV? >>> >>> On e500v2 we always enable MSR_DE using vcpu->arch.shadow_msr in >>> e500.c #ifndef CONFIG_KVM_BOOKE_HV >>> - vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS; >>> + vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS; > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index b340a62..1e2d663 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -151,10 +151,14 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr) > > /* > * Set MSR_DE if the hardware debug resources are owned by user-space > - * and there is no debug interrupt pending for guest to handle. > */ > - if (vcpu->guest_debug && !kvmppc_core_pending_debug(vcpu)) > + if (vcpu->guest_debug) > new_msr |= MSR_DE; > +#else > + if (vcpu->guest_debug) > + vcpu->arch.shadow_msr |= MSR_DE; > #endif > > But do not when I should clear? How about something like this? Then both targets at least suck as much :). 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. So I would assume it's for the best to just treat both the same: always expose MSR_DE into guest visibility. This will break when the guest disables MSR_DE. But I have no good idea on how to solve this properly - except for hypercalls to tell us that MSR_DE is set or not. Scott, do you have an idea? Alex diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 38a62ef..3f8cbbd 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -133,6 +133,19 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu) #endif } +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) +{ + /* Force debug to on in guest space when user space wants to debug */ + if (vcpu->guest_debug) + vcpu->arch.shared->msr |= MSR_DE; + +#if !defined(CONFIG_KVM_BOOKE_HV) + /* Synchronize MSR_DE into shadow MSR */ + vcpu->arch.shadow_msr &= ~MSR_DE; + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; +#endif +} + /* * Helper function for "full" MSR writes. No need to call this if only * EE/CE/ME/DE/RI are changing. @@ -150,6 +163,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, -- 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
