Re: [PATCH 4/4 v2] KVM: PPC: Add userspace debug stub support

[Alexander Graf <agraf@suse.de>]

On 03.04.2013, at 17:18, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, April 03, 2013 7:39 PM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH 4/4 v2] KVM: PPC: Add userspace debug stub support
>> 
>> 
>> On 03.04.2013, at 15:50, 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: Wednesday, April 03, 2013 3:58 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH 4/4 v2] KVM: PPC: Add userspace debug stub
>>>> support
>>>> 
>>>> 
>>>> 
>>>> Am 03.04.2013 um 12:03 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:
>>>> 
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Wood Scott-B07421
>>>>>> Sent: Tuesday, April 02, 2013 11:30 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: Alexander Graf; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
>>>>>> Wood Scott-
>>>>>> B07421
>>>>>> Subject: Re: [PATCH 4/4 v2] KVM: PPC: Add userspace debug stub
>>>>>> support
>>>>>> 
>>>>>> On 04/02/2013 09:09:34 AM, Bhushan Bharat-R65777 wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> -----Original Message-----
>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>> Sent: Tuesday, April 02, 2013 1:57 PM
>>>>>>>> To: Bhushan Bharat-R65777
>>>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood
>>>>>>>> Scott-B07421
>>>>>>>> Subject: Re: [PATCH 4/4 v2] KVM: PPC: Add userspace debug stub
>>>>>>> support
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 29.03.2013, at 07:04, Bhushan Bharat-R65777 wrote:
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>>>> Sent: Thursday, March 28, 2013 10:06 PM
>>>>>>>>>> To: Bhushan Bharat-R65777
>>>>>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood
>>>>>>> Scott-B07421;
>>>>>>>>>> Bhushan
>>>>>>>>>> Bharat-R65777
>>>>>>>>>> Subject: Re: [PATCH 4/4 v2] KVM: PPC: Add userspace debug stub
>>>>>>>>>> support
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> How does the normal debug register switching code work in Linux?
>>>>>>>>>> Can't we just reuse that? Or rely on it to restore working
>>>>>>>>>> state
>>>>>>> when
>>>>>>>>>> another process gets scheduled in?
>>>>>>>>> 
>>>>>>>>> Good point, I can see debug registers loading in function
>>>>>>> __switch_to()-
>>>>>>>>> switch_booke_debug_regs() in file arch/powerpc/kernel/process.c.
>>>>>>>>> So as long as assume that host will not use debug resources we
>>>>>>> can rely on
>>>>>>>> this restore. But I am not sure that this is a fare assumption.
>>>>>>>> As
>>>>>>> Scott earlier
>>>>>>>> mentioned someone can use debug resource for kernel debugging also.
>>>>>>>> 
>>>>>>>> Someone in the kernel can also use floating point registers. But
>>>>>>> then it's his
>>>>>>>> responsibility to clean up the mess he leaves behind.
>>>>>>> 
>>>>>>> I am neither convinced by what you said and nor even have much
>>>>>>> reason to oppose :)
>>>>>>> 
>>>>>>> Scott,
>>>>>>>  I remember you mentioned that host can use debug resources, you
>>>>>>> comment on this ?
>>>>>> 
>>>>>> I thought the conclusion we reached was that it was OK as long as
>>>>>> KVM waits until it actually needs the debug resources to mess with the
>> registers.
>>>>> 
>>>>> Right,  Are we also agreeing on that KVM will not save/restore host
>>>>> debug
>>>> context on vcpu_load/vcpu_put()? KVM will load its context in
>>>> vcpu_load() if needed and on vcpu_put() it will clear DBCR0 and DBSR.
>>>> 
>>>> That depends on whether the kernel restores the debug registers.
>>>> Please double- check that.
>>> 
>>> Currently the kernel code restore the debug state of new schedule process in
>> context_switch().
>>> 
>>> switch_booke_debug_regs() from __switch_to() and defined as :
>>> /*
>>> * Unless neither the old or new thread are making use of the
>>> * debug registers, set the debug registers from the values
>>> * stored in the new thread.
>>> */
>>> static void switch_booke_debug_regs(struct thread_struct *new_thread)
>>> {
>>>       if ((current->thread.dbcr0 & DBCR0_IDM)
>>>               || (new_thread->dbcr0 & DBCR0_IDM))
>>>                       prime_debug_regs(new_thread); }
>>> 
>>> static void prime_debug_regs(struct thread_struct *thread) {
>>>       mtspr(SPRN_IAC1, thread->iac1);
>>>       mtspr(SPRN_IAC2, thread->iac2); #if CONFIG_PPC_ADV_DEBUG_IACS >
>>> 2
>>>       mtspr(SPRN_IAC3, thread->iac3);
>>>       mtspr(SPRN_IAC4, thread->iac4); #endif
>>>       mtspr(SPRN_DAC1, thread->dac1);
>>>       mtspr(SPRN_DAC2, thread->dac2); #if CONFIG_PPC_ADV_DEBUG_DVCS >
>>> 0
>>>       mtspr(SPRN_DVC1, thread->dvc1);
>>>       mtspr(SPRN_DVC2, thread->dvc2); #endif
>>>       mtspr(SPRN_DBCR0, thread->dbcr0);
>>>       mtspr(SPRN_DBCR1, thread->dbcr1); #ifdef CONFIG_BOOKE
>>>       mtspr(SPRN_DBCR2, thread->dbcr2); #endif } This is analogous to
>>> moving from guest to/from QEMU. so we can make prime_debug_regs() available to
>> kvm code for heavyweight_exit. And vcpu_load() will load guest state and save
>> host state (update thread->debug_registers).
>> 
>> I don't think we need to do anything on vcpu_load if we just swap the thread-
>>> debug_registers. Just make sure to restore them before you return from a heavy
>> weight exit.
> 
> My understanding is :
> 
> 1)
> When VCPU is running      -> h/w debug registers have vcpu->arch.debug_registers
> Goes for heavyweight_exit -> h/w debug registers are loaded with thread->debug_registers
> Return from heavyweight_exit and run vcpu -> h/w debug registers have vcpu->arch.debug_registers. Do we need to save thread->debug_registers , probably no as the thread->debug_registers are upto date.

I think I had a thinko here :). I figured that when we put the guest debug register state into thread->debug_registers, we don't need to worry about save/restore on vcpu_load/put. But that won't update the register values in thread->debug_registers when the guest modifies them, so we would need to write them back on vcpu_put again, rendering the whole exercise pointless.

So yes, we should only make sure that we set thread->dbcr0 = 0 on vcpu_run and return it to its old value on a heavy weight exit. That way we don't swap in debug registers we don't care about if someone debugs the QEMU process.

> 
> 2)
> KVM to kernel -> clear DBSR and DBCR0

if debugging is active

> Kernel to KVM -> load vcpu->arch.debug_registers.

if debugging is active :)


Alex