On 07.02.2013, at 16:00, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Saturday, February 02, 2013 4:09 AM
>> To: Alexander Graf
>> Cc: Bhushan Bharat-R65777; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest
>>
>> 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 }
>
> Once MSR_DE set because of vcpu->guest_debug, it will remains set even if vcpu->guest_debug is not set, until guest clears msr_de itself. And guest may not clear MSR_DE because it have never set it.
That would create a time frame where the guest maybe doesn't want debug interrupts, but would get them. I'm not sure how bad that would be though.
Alex
--
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
