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.liu@xxxxxxxxxxxxx>
> Signed-off-by: Varun Sethi <Varun.Sethi@xxxxxxxxxxxxx>
> [bharat.bhushan@xxxxxxxxxxxxx: Substantial changes]
> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
> ---
> 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_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.
> +#define KVM_GUESTDBG_USE_SW_BP 0x00010000
> +#define KVM_GUESTDBG_USE_HW_BP 0x00020000
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)
> {
> enum emulation_result er;
>
> + if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) &&
> + (vcpu->arch.last_inst == KVM_INST_GUESTGDB)) {
Unnecessary parens.
> + 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?
> +#define GET_VCPU_POINT(regd) \
> + mfspr regd, SPRN_SPRG_THREAD; \
> + lwz regd, THREAD_KVM_VCPU(regd)
"Point" is not an idiomatic abbreviation for pointer. Does this really
need to be macroized, which prevents optimization? IIRC, the 64-bit
patchset gets rid of that on bookehv (where it was called GET_VCPU).
> _GLOBAL(kvmppc_resume_host)
> + /*
> + * If guest not used debug facility then hw debug registers
> + * already have proper host values. If guest used debug
> + * facility then restore host debug registers.
> + * No Need to save guest debug registers as they are already intact
> + * in guest/shadow registers.
> + */
> + lwz r9, VCPU_SHADOW_DBG(r4)
> + rlwinm. r8, r9, 0, ~DBCR0_IDM
> + beq skip_load_host_debug
> + lwz r3, VCPU_HOST_DBG(r4)
> + andis. r9, r9, DBCR0_AC_BITS@h
> + li r9, 0
> + mtspr SPRN_DBCR0, r9 /* disable all debug event */
> + beq ..skip_load_hw_bkpts
We don't currently have that weird leading ".." in the bookehv code --
please don't introduce it.
> +#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?
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 685829a..38b5d02 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -427,7 +427,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> struct kvm_guest_debug *dbg)
> {
> - return -EINVAL;
> + return kvmppc_core_set_guest_debug(vcpu, dbg);
> }
I don't see a stub implementation for non-booke.
-Scott
--
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
