Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-09-25 12:47, Alexander Graf wrote:
> 
> On 25.09.2012, at 12:38, Jan Kiszka wrote:
> 
>> On 2012-09-24 16:46, Alexander Graf wrote:
>>>
>>> On 07.09.2012, at 00:56, Scott Wood wrote:
>>>
>>>> On 09/06/2012 09:56 AM, Bhushan Bharat-R65777 wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Wood Scott-B07421
>>>>>> Sent: Thursday, September 06, 2012 4:57 AM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Bhushan Bharat-
>>>>>> R65777
>>>>>> Subject: Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support
>>>>>>
>>>>>> On 09/05/2012 06:23 PM, Scott Wood wrote:
>>>>>>> On 08/21/2012 08:52 AM, Bharat Bhushan wrote:
>>>>>>>> This patch adds the debug stub support on booke/bookehv.
>>>>>>>> Now QEMU debug stub can use hw breakpoint, watchpoint and software
>>>>>>>> breakpoint to debug guest.
>>>>>>>>
>>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>>>> ---
>>>>>>>> arch/powerpc/include/asm/kvm.h        |   29 ++++++-
>>>>>>>> arch/powerpc/include/asm/kvm_host.h   |    5 +
>>>>>>>> arch/powerpc/kernel/asm-offsets.c     |   26 ++++++
>>>>>>>> arch/powerpc/kvm/booke.c              |  144 +++++++++++++++++++++++++++++--
>>>>>> --
>>>>>>>> arch/powerpc/kvm/booke_interrupts.S   |  110 +++++++++++++++++++++++++
>>>>>>>> arch/powerpc/kvm/bookehv_interrupts.S |  141
>>>>>> +++++++++++++++++++++++++++++++-
>>>>>>>> arch/powerpc/kvm/e500mc.c             |    3 +-
>>>>>>>> 7 files changed, 435 insertions(+), 23 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/powerpc/include/asm/kvm.h
>>>>>>>> b/arch/powerpc/include/asm/kvm.h index 61b197e..53479ea 100644
>>>>>>>> --- a/arch/powerpc/include/asm/kvm.h
>>>>>>>> +++ b/arch/powerpc/include/asm/kvm.h
>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>> /* Select powerpc specific features in <linux/kvm.h> */  #define
>>>>>>>> __KVM_HAVE_SPAPR_TCE  #define __KVM_HAVE_PPC_SMT
>>>>>>>> +#define __KVM_HAVE_GUEST_DEBUG
>>>>>>>>
>>>>>>>> struct kvm_regs {
>>>>>>>> 	__u64 pc;
>>>>>>>> @@ -264,7 +265,31 @@ struct kvm_fpu {
>>>>>>>> 	__u64 fpr[32];
>>>>>>>> };
>>>>>>>>
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
>>>>>>>> + * software breakpoint.
>>>>>>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>>>>>>>> + * for KVM_DEBUG_EXIT.
>>>>>>>> + */
>>>>>>>> +#define KVMPPC_DEBUG_NONE		0x0
>>>>>>>> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>>>>>>> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>>>>>>> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>>>>>>> struct kvm_debug_exit_arch {
>>>>>>>
>>>>>>> That says "arch", but it's not in an arch-specific file.
>>>>>>
>>>>>> Sigh, I can't read today apparently.
>>>>>>
>>>>>>>> +	__u64 pc;
>>>>>>>> +	/*
>>>>>>>> +	 * exception -> returns the exception number. If the KVM_DEBUG_EXIT
>>>>>>>> +	 * exit is not handled (say not h/w breakpoint or software breakpoint
>>>>>>>> +	 * set for this address) by qemu then it is supposed to inject this
>>>>>>>> +	 * exception to guest.
>>>>>>>> +	 */
>>>>>>>> +	__u32 exception;
>>>>>>>> +	/*
>>>>>>>> +	 * exiting to userspace because of h/w breakpoint, watchpoint
>>>>>>>> +	 * (read, write or both) and software breakpoint.
>>>>>>>> +	 */
>>>>>>>> +	__u32 status;
>>>>>>>> };
>>>>>>>
>>>>>>> What does "exception number" mean in a generic API?
>>>>>>
>>>>>> Still, "exception number" is not a well-defined concept powerpc-wide.
>>>>>
>>>>> Just for background why we added is that, on x86 this exception number is used to inject the exception to guest if QEMU is not able to handle the debug exception.
>>>>>
>>>>> Should we just through a print with clearing the exception condition? Or something else you would like to suggest?
>>>>
>>>> We can pass up the exception type; it just needs more documentation
>>>> about what exactly you're referring to, and probably some enumeration
>>>> that says which exception numberspace it is.
>>>>
>>>> For booke the exception number should probably be related to the fixed
>>>> offsets rather than the IVOR number, as IVORs are phased out.
>>>
>>> Jan, I would like to get your comment on this one.
>>>
>>> Since we don't have standardized exception vectors like x86 does, we need to convert things between different semantics in user space if we want to make use of the exception type. Do we actually need to know about it in user space or do we only need to store it in case we get a migration at that point?
>>>
>>> If it's the latter, can we maybe keep the reinjection logic internal to KVM and make DEBUG exits non-migratable similar to how we already handle MMIO/PIO exits today?
>>
>> Reinjection depends on userspace. It has to check if the debug event was
>> related to the host debugging the guest or if it was due to a
>> guest-internal debugging event. That's because the kernel does not track
>> individual breakpoints, just controls the trapping. So we need a way to
>> manage the reinjection, and we do this (on x86) by passing the exception
>> up and then using it for SET_VCPU_EVENTS to reinject it if necessary.
>>
>> The general logic (reinjection filter in userspace) should be generic
>> enough, but all the details can, of course, be defined in an
>> arch-specific manner.
> 
> Well, we could always just pass a payload to user space that it passes into an ioctl to the kernel to reinject the interrupt. But that would break migratability as this payload wouldn't get stored in env.

Right.

> 
> But the same thing applies to x86, right? So if just pass a struct kvm_vcpu_events with the debug interrupt as return value, user space can simply use that to reinject it with SET_VCPU_EVENTS.

Sorry, I'm slow today: That's a proposal for booke now?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux