On 31.01.2013, at 18:59, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@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; k...@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-ow...@vger.kernel.org
>>>> [mailto:kvm-ppc-ow...@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; k...@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.bhus...@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
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-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
