diff mbox

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

Message ID FC301BB5-4DE3-48C1-A429-B4258C667A30@suse.de
State New, archived
Headers show

Commit Message

Alexander Graf Jan. 31, 2013, 6:21 p.m. UTC 
On 31.01.2013, at 18:59, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of
>> Alexander Graf
>> Sent: Thursday, January 31, 2013 5:34 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> 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@vger.kernel.org
>>>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
>>>> Sent: Friday, January 25, 2013 5:44 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; 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@freescale.com>
>>>>> ---
>>>>> 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



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

Patch

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,