Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
On 08/16/2012 03:48 AM, Bhushan Bharat-R65777 wrote:
diff --git a/arch/powerpc/include/asm/kvm.h
b/arch/powerpc/include/asm/kvm.h index 3c14202..da71c84 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;
@@ -265,10 +266,19 @@ struct kvm_fpu { };
struct kvm_debug_exit_arch {
+ __u32 exception;
+ __u32 pc;
+ __u32 status;
};
PC must be 64-bit. What goes in status and exception?
status - exit because of h/w breakpoint, watchpoint (read, write or
both) and software breakpoint.
exception - returns the exception number. If the exit is not handled
(say not h/w breakpoint or software breakpoint set for this address)
by qemu then it is supposed to inject the exception to guest. This is
how it is implemented for x86.
Where is this documented (including the specific values that are possible)?
+#define KVM_GUESTDBG_USE_SW_BP 0x0001
+#define KVM_GUESTDBG_USE_HW_BP 0x0002
Where do these get used? Any reason for these particular values? If
you're trying to create a partition where the upper half is generic
and the lower half is arch-specific, say so.
KVM_SET_GUEST_DEBUG ioctl used to set/unset debug interrupts, which
have a u32 control element. We have inherited this mechanism from
x86 implementation and it looks like lower 16 bits are generic (like
KVM_GUESTDBG_ENBLE, KVM_GUESTDBG_SINGLESTEP etc and upper 16 bits are
Architecture specific.
I will add a comment to describe this.
I don't think the sw/hw distinction belongs here -- it should be per
breakpoint.
KVM does not track the software breakpoint, so it is not per breakpoint.
In KVM, when KVM_GUESTDBG_USE_SW_BP flag is set and special trap instruction
is executed by guest then exit to userspace.
Can both types of breakpoint be set at the same time?
+ run-exit_reason = KVM_EXIT_DEBUG;
+ run-debug.arch.pc = vcpu-arch.pc;
+ run-debug.arch.exception = exit_nr;
+ run-debug.arch.status = 0;
+ kvmppc_account_exit(vcpu, DEBUG_EXITS);
+ return RESUME_HOST;
The interface isn't (clearly labelled as) booke specific, but you
return booke- specific exception numbers. How's userspace supposed
to know what to do with them? What do you plan on doing with them in QEMU?
This is booke specific.
Then put booke in the name,
Which data structure name should have booke?
Anything that's booke specific.
-Scott
--
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
Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
On 08/16/2012 10:12 AM, Bhushan Bharat-R65777 wrote: -Original Message- From: Wood Scott-B07421 Sent: Tuesday, July 31, 2012 3:31 AM To: Bhushan Bharat-R65777 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de Subject: Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support On 07/30/2012 02:37 AM, Bhushan Bharat-R65777 wrote: -Original Message- From: Wood Scott-B07421 Sent: Friday, July 27, 2012 7:00 AM To: Bhushan Bharat-R65777 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de; Bhushan Bharat- R65777 Subject: Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support +#ifndef CONFIG_PPC_FSL_BOOK3E + PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4) + PPC_LD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4) + mtspr SPRN_IAC3, r7 + mtspr SPRN_IAC4, r8 +#endif Can you handle this at runtime with a feature section? Why you want this to make run time? Removing config_ ? Currently KVM hardcodes the target hardware in a way that is unacceptable in much of the rest of the kernel. We have a long term goal to stop doing that, and we should avoid making it worse by adding random ifdefs for specific CPUs. I do not see any CPU_FTR_* which I can use directly. Should I define a new FTR, something like: #define CPU_FTR_DEBUG_E500 LONG_ASM_CONST(0x4000) Use this in: CPU_FTRS_E500_2, CPU_FTRS_E500MC, CPU_FTRS_E5500 etc BEGIN_FTR_SECTION PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4) PPC_LD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4) mtspr SPRN_IAC3, r7 mtspr SPRN_IAC4, r8 END_FTR_SECTION_IFCLR(CPU_FTR_DEBUG_E500) It looks like other parts of the kernel use CONFIG_PPC_ADV_DEBUG_IACS for this, though ideally it would be made runtime in the future. -Scott -- 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
RE: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
diff --git a/arch/powerpc/include/asm/kvm.h
b/arch/powerpc/include/asm/kvm.h index 3c14202..da71c84 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;
@@ -265,10 +266,19 @@ struct kvm_fpu { };
struct kvm_debug_exit_arch {
+ __u32 exception;
+ __u32 pc;
+ __u32 status;
};
PC must be 64-bit. What goes in status and exception?
status - exit because of h/w breakpoint, watchpoint (read, write or both) and
software breakpoint.
exception - returns the exception number. If the exit is not handled (say not
h/w breakpoint or software breakpoint set for this address) by qemu then it is
supposed to inject the exception to guest. This is how it is implemented for
x86.
ok
/* for KVM_SET_GUEST_DEBUG */
struct kvm_guest_debug_arch {
+ struct {
+ __u64 addr;
+ __u32 type;
+ __u32 pad1;
+ __u64 pad2;
+ } bp[16];
};
What goes in type?
Type denote breakpoint, read watchpoint, write watchpoint or watchpoint
(both
read and write). Will adding a comment to describe this is ok?
Yes, please make sure all of this is well documented.
/* definition of registers in kvm_run */ @@ -285,6 +295,17 @@
struct kvm_sync_regs {
#define KVM_CPU_3S_644
#define KVM_CPU_E500MC 5
+/* Debug related defines */
+#define KVM_INST_GUESTGDB 0x7C00021C /* ehpriv OC=0 */
Will this work on all PPC?
It certainly won't work on other architectures, so at a minimum it's
KVM_PPC_INST_GUEST_GDB, but maybe it needs to be determined at runtime.
How to determine at run time? adding another ioctl ?
Or extend an existing one. Is there any other information about debug
capabilities that you expose -- number of hardware breakpoints supported, etc
+#define KVM_GUESTDBG_USE_SW_BP 0x0001
+#define KVM_GUESTDBG_USE_HW_BP 0x0002
Where do these get used? Any reason for these particular values? If
you're trying to create a partition where the upper half is generic
and the lower half is arch-specific, say so.
KVM_SET_GUEST_DEBUG ioctl used to set/unset debug interrupts, which
have a u32 control element. We have inherited this mechanism from
x86 implementation and it looks like lower 16 bits are generic (like
KVM_GUESTDBG_ENBLE, KVM_GUESTDBG_SINGLESTEP etc and upper 16 bits are
Architecture specific.
I will add a comment to describe this.
I don't think the sw/hw distinction belongs here -- it should be per
breakpoint.
KVM does not track the software breakpoint, so it is not per breakpoint.
In KVM, when KVM_GUESTDBG_USE_SW_BP flag is set and special trap instruction is
executed by guest then exit to userspace.
+ run-exit_reason = KVM_EXIT_DEBUG;
+ run-debug.arch.pc = vcpu-arch.pc;
+ run-debug.arch.exception = exit_nr;
+ run-debug.arch.status = 0;
+ kvmppc_account_exit(vcpu, DEBUG_EXITS);
+ return RESUME_HOST;
The interface isn't (clearly labelled as) booke specific, but you
return booke- specific exception numbers. How's userspace supposed
to know what to do with them? What do you plan on doing with them in QEMU?
This is booke specific.
Then put booke in the name,
Which data structure name should have booke?
but what about it really needs to be booke specific?
Why does QEMU care about the exception type?
Explained above.
Thanks
-Bharat
+#ifndef CONFIG_PPC_FSL_BOOK3E
+ PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
+ PPC_LD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
+ mtspr SPRN_IAC3, r7
+ mtspr SPRN_IAC4, r8
+#endif
Can you handle this at runtime with a feature section?
Why you want this to make run time? Removing config_ ?
Currently KVM hardcodes the target hardware in a way that is unacceptable in
much of the rest of the kernel. We have a long term goal to stop doing that,
and we should avoid making it worse by adding random ifdefs for specific CPUs.
-Scott
Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
On 07/30/2012 02:37 AM, Bhushan Bharat-R65777 wrote:
-Original Message-
From: Wood Scott-B07421
Sent: Friday, July 27, 2012 7:00 AM
To: Bhushan Bharat-R65777
Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de; Bhushan
Bharat-
R65777
Subject: Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
On 07/26/2012 12:32 AM, Bharat Bhushan wrote:
This patch adds:
1) KVM debug handler added for e500v2.
2) Guest debug by qemu gdb stub.
Does it make sense for these to both be in the same patch? If there's common
code used by both, that could be added first.
ok
Signed-off-by: Liu Yu yu@freescale.com
Signed-off-by: Varun Sethi varun.se...@freescale.com
[bharat.bhus...@freescale.com: Substantial changes]
Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
---
arch/powerpc/include/asm/kvm.h| 21 +
arch/powerpc/include/asm/kvm_host.h |7 ++
arch/powerpc/include/asm/kvm_ppc.h|2 +
arch/powerpc/include/asm/reg_booke.h |1 +
arch/powerpc/kernel/asm-offsets.c | 31 ++-
arch/powerpc/kvm/booke.c | 146 +++---
arch/powerpc/kvm/booke_interrupts.S | 160
-
arch/powerpc/kvm/bookehv_interrupts.S | 141 -
arch/powerpc/kvm/e500mc.c |3 +-
arch/powerpc/kvm/powerpc.c|2 +-
10 files changed, 492 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm.h
b/arch/powerpc/include/asm/kvm.h index 3c14202..da71c84 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;
@@ -265,10 +266,19 @@ struct kvm_fpu { };
struct kvm_debug_exit_arch {
+ __u32 exception;
+ __u32 pc;
+ __u32 status;
};
PC must be 64-bit. What goes in status and exception?
ok
/* for KVM_SET_GUEST_DEBUG */
struct kvm_guest_debug_arch {
+ struct {
+ __u64 addr;
+ __u32 type;
+ __u32 pad1;
+ __u64 pad2;
+ } bp[16];
};
What goes in type?
Type denote breakpoint, read watchpoint, write watchpoint or watchpoint (both
read and write). Will adding a comment to describe this is ok?
Yes, please make sure all of this is well documented.
/* definition of registers in kvm_run */ @@ -285,6 +295,17 @@ struct
kvm_sync_regs {
#define KVM_CPU_3S_64 4
#define KVM_CPU_E500MC 5
+/* Debug related defines */
+#define KVM_INST_GUESTGDB 0x7C00021C /* ehpriv OC=0 */
Will this work on all PPC?
It certainly won't work on other architectures, so at a minimum it's
KVM_PPC_INST_GUEST_GDB, but maybe it needs to be determined at runtime.
How to determine at run time? adding another ioctl ?
Or extend an existing one. Is there any other information about debug
capabilities that you expose -- number of hardware breakpoints
supported, etc?
+#define KVM_GUESTDBG_USE_SW_BP 0x0001
+#define KVM_GUESTDBG_USE_HW_BP 0x0002
Where do these get used? Any reason for these particular values? If you're
trying to create a partition where the upper half is generic and the lower
half
is arch-specific, say so.
KVM_SET_GUEST_DEBUG ioctl used to set/unset debug interrupts, which
have a u32 control element. We have inherited this mechanism from
x86 implementation and it looks like lower 16 bits are generic (like
KVM_GUESTDBG_ENBLE, KVM_GUESTDBG_SINGLESTEP etc and upper 16 bits are
Architecture specific.
I will add a comment to describe this.
I don't think the sw/hw distinction belongs here -- it should be per
breakpoint.
+ run-exit_reason = KVM_EXIT_DEBUG;
+ run-debug.arch.pc = vcpu-arch.pc;
+ run-debug.arch.exception = exit_nr;
+ run-debug.arch.status = 0;
+ kvmppc_account_exit(vcpu, DEBUG_EXITS);
+ return RESUME_HOST;
The interface isn't (clearly labelled as) booke specific, but you return
booke-
specific exception numbers. How's userspace supposed to know what to do with
them? What do you plan on doing with them in QEMU?
This is booke specific.
Then put booke in the name, but what about it really needs to be booke
specific? Why does QEMU care about the exception type?
+#ifndef CONFIG_PPC_FSL_BOOK3E
+ PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
+ PPC_LD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
+ mtspr SPRN_IAC3, r7
+ mtspr SPRN_IAC4, r8
+#endif
Can you handle this at runtime with a feature section?
Why you want this to make run time? Removing config_ ?
Currently KVM hardcodes the target hardware in a way that is
unacceptable in much of the rest of the kernel. We have a long term
goal to stop doing that, and we should
Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
On 07/26/2012 12:32 AM, Bharat Bhushan wrote:
This patch adds:
1) KVM debug handler added for e500v2.
2) Guest debug by qemu gdb stub.
Does it make sense for these to both be in the same patch? If there's
common code used by both, that could be added first.
Signed-off-by: Liu Yu yu@freescale.com
Signed-off-by: Varun Sethi varun.se...@freescale.com
[bharat.bhus...@freescale.com: Substantial changes]
Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
---
arch/powerpc/include/asm/kvm.h| 21 +
arch/powerpc/include/asm/kvm_host.h |7 ++
arch/powerpc/include/asm/kvm_ppc.h|2 +
arch/powerpc/include/asm/reg_booke.h |1 +
arch/powerpc/kernel/asm-offsets.c | 31 ++-
arch/powerpc/kvm/booke.c | 146 +++---
arch/powerpc/kvm/booke_interrupts.S | 160
-
arch/powerpc/kvm/bookehv_interrupts.S | 141 -
arch/powerpc/kvm/e500mc.c |3 +-
arch/powerpc/kvm/powerpc.c|2 +-
10 files changed, 492 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
index 3c14202..da71c84 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;
@@ -265,10 +266,19 @@ struct kvm_fpu {
};
struct kvm_debug_exit_arch {
+ __u32 exception;
+ __u32 pc;
+ __u32 status;
};
PC must be 64-bit. What goes in status and exception?
/* for KVM_SET_GUEST_DEBUG */
struct kvm_guest_debug_arch {
+ struct {
+ __u64 addr;
+ __u32 type;
+ __u32 pad1;
+ __u64 pad2;
+ } bp[16];
};
What goes in type?
/* definition of registers in kvm_run */
@@ -285,6 +295,17 @@ struct kvm_sync_regs {
#define KVM_CPU_3S_644
#define KVM_CPU_E500MC 5
+/* Debug related defines */
+#define KVM_INST_GUESTGDB 0x7C00021C /* ehpriv OC=0 */
Will this work on all PPC?
It certainly won't work on other architectures, so at a minimum it's
KVM_PPC_INST_GUEST_GDB, but maybe it needs to be determined at runtime.
+#define KVM_GUESTDBG_USE_SW_BP 0x0001
+#define KVM_GUESTDBG_USE_HW_BP 0x0002
Where do these get used? Any reason for these particular values? If
you're trying to create a partition where the upper half is generic and
the lower half is arch-specific, say so.
diff --git a/arch/powerpc/include/asm/kvm_host.h
b/arch/powerpc/include/asm/kvm_host.h
index 7a45194..524af7a 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -458,7 +458,12 @@ struct kvm_vcpu_arch {
u32 ccr0;
u32 ccr1;
u32 dbsr;
+ /* guest debug regiters*/
struct kvmppc_booke_debug_reg dbg_reg;
+ /* shadow debug registers */
+ struct kvmppc_booke_debug_reg shadow_dbg_reg;
+ /* host debug regiters*/
+ struct kvmppc_booke_debug_reg host_dbg_reg;
s/regiter/register/g
...and put a space before */
@@ -492,6 +497,7 @@ struct kvm_vcpu_arch {
u32 tlbcfg[4];
u32 mmucfg;
u32 epr;
+ u32 crit_save;
#endif
What is crit_save?
gpa_t paddr_accessed;
gva_t vaddr_accessed;
@@ -533,6 +539,7 @@ struct kvm_vcpu_arch {
struct kvm_vcpu_arch_shared *shared;
unsigned long magic_page_pa; /* phys addr to map the magic page to */
unsigned long magic_page_ea; /* effect. addr to map the magic page to */
+ struct kvm_guest_debug_arch dbg; /* debug arg between kvm and qemu */
Is kvm_guest_debug_arch generic or PPC-specific? If the former, why is
it in a ppc struct? If the latter, why doesn't it have ppc in the name?
Please separate out generic things in one patch, then PPC-wide things,
then booke things (but keep things bisectable by adding stubs along the
way if necessary).
-#ifdef CONFIG_KVM_BOOKE_HV
+#if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
DEFINE(THREAD_KVM_VCPU, offsetof(struct thread_struct, kvm_vcpu));
#endif
Why not all booke?
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 6fbdcfc..784a6bf 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -130,6 +130,9 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
#ifdef CONFIG_KVM_BOOKE_HV
new_msr |= MSR_GS;
+
+ if (vcpu-guest_debug)
+ new_msr |= MSR_DE;
#endif
vcpu-arch.shared-msr = new_msr;
@@ -684,10 +687,21 @@ out:
return ret;
}
-static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
+static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
+ int exit_nr)
