On 25.09.2012, at 13:06, Jan Kiszka wrote:
> On 2012-09-25 12:58, Alexander Graf wrote:
>>
>> On 25.09.2012, at 12:56, Jan Kiszka wrote:
>>
>>> 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@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; agraf@xxxxxxx; 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@xxxxxxxxxxxxx>
>>>>>>>>>>> ---
>>>>>>>>>>> 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?
>>
>> Yes, that's a proposal for PPC in general, because we need to be extensible enough to transmit all data that Book3S one day might also want to have.
>>
>> The thing I was saying is that if we make the payload magical, we need to make debug exits atomic. Otherwise QEMU could try to live migrate away from us and we lose state. I suppose x86 doesn't have this problem because it shoehorns the exception vectors into env?
>
> It's no problem on x86 as our VCPU state can encode pending (debug)
> exceptions, so we can store them also for migration. If PPC doesn't
> allow to read/write such kind of state from/to the kernel, you can't
> easily do the same. If debugging would be the only reason to do this, it
> might be ok to declare that state atomic.
Well, we do have such state as well. But it's quite different depending on the board, so it'd be a bunch of code that I'd rather not have around if it doesn't buy us too much.
Though I guess it's the cleanest way to do it. Sigh.
So we pass a struct kvm_vcpu_events up with the debug interrupt. That way a different user space can simply reinject it if it wants to. For QEMU, we would interpret that thing, shove all the fields into their respective env counterparts with the QEMU internal exception vector definitions and do the reverse in kvm_put_vcpu_events.
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
