Patchwork [2/2] KVM: PPC: booke/bookehv: Add guest debug support

login
register
mail settings
Submitter Bharat Bhushan
Date July 26, 2012, 5:32 a.m.
Message ID <1343280734-3359-2-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/173342/
State New
Headers show

Comments

Bharat Bhushan - July 26, 2012, 5:32 a.m.
This patch adds:
 1) KVM debug handler added for e500v2.
 2) Guest debug by qemu gdb stub.

Signed-off-by: Liu Yu <yu.liu@freescale.com>
Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
[bharat.bhushan@freescale.com: Substantial changes]
Signed-off-by: Bharat Bhushan <bharat.bhushan@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(-)
Scott Wood - July 27, 2012, 1:29 a.m.
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@freescale.com>
> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> [bharat.bhushan@freescale.com: Substantial changes]
> Signed-off-by: Bharat Bhushan <bharat.bhushan@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_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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - July 30, 2012, 10 p.m.
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; kvm@vger.kernel.org; agraf@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.liu@freescale.com>
>>> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
>>> [bharat.bhushan@freescale.com: Substantial changes]
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@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          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.
> 
> 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 avoid making it worse by adding
random ifdefs for specific CPUs.

-Scott


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan - Aug. 16, 2012, 8:48 a.m.
> >>> 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_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          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.

> >

> > 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
Bharat Bhushan - Aug. 16, 2012, 3:12 p.m.
> -----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; kvm@vger.kernel.org;

> agraf@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; kvm@vger.kernel.org; agraf@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.liu@freescale.com>

> >>> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>

> >>> [bharat.bhushan@freescale.com: Substantial changes]

> >>> Signed-off-by: Bharat Bhushan <bharat.bhushan@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          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.

> >

> > 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 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(0x4000000000000000)

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)

Thanks
-Bharat

> 

> -Scott
Scott Wood - Aug. 20, 2012, 11:53 p.m.
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          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.
>>>
>>> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - Aug. 20, 2012, 11:55 p.m.
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; kvm@vger.kernel.org;
>> agraf@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; kvm@vger.kernel.org; agraf@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(0x4000000000000000)
> 
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
 };
 
 /* for KVM_SET_GUEST_DEBUG */
 struct kvm_guest_debug_arch {
+	struct {
+		__u64 addr;
+		__u32 type;
+		__u32 pad1;
+		__u64 pad2;
+	} bp[16];
 };
 
 /* 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 */
+
+#define KVM_GUESTDBG_USE_SW_BP          0x00010000
+#define KVM_GUESTDBG_USE_HW_BP          0x00020000
+
+#define KVMPPC_DEBUG_NOTYPE             0x0
+#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
+#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
+#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
+
 /* for KVM_CAP_SPAPR_TCE */
 struct kvm_create_spapr_tce {
 	__u64 liobn;
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;
 
 	u64 mmcr[3];
 	u32 pmc[8];
@@ -492,6 +497,7 @@  struct kvm_vcpu_arch {
 	u32 tlbcfg[4];
 	u32 mmucfg;
 	u32 epr;
+	u32 crit_save;
 #endif
 	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 */
 
 #ifdef CONFIG_KVM_BOOK3S_64_HV
 	struct kvm_vcpu_arch_shared shregs;
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 823d563..c97b234 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -115,6 +115,8 @@  extern int kvmppc_core_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn,
 				     ulong val);
 extern int kvmppc_core_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn,
 				     ulong *val);
+extern int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
+					struct kvm_guest_debug *dbg);
 
 extern int kvmppc_booke_init(void);
 extern void kvmppc_booke_exit(void);
diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
index e07e6af..b417de3 100644
--- a/arch/powerpc/include/asm/reg_booke.h
+++ b/arch/powerpc/include/asm/reg_booke.h
@@ -56,6 +56,7 @@ 
 #define SPRN_SPRG7W	0x117	/* Special Purpose Register General 7 Write */
 #define SPRN_EPCR	0x133	/* Embedded Processor Control Register */
 #define SPRN_DBCR2	0x136	/* Debug Control Register 2 */
+#define SPRN_DBCR4	0x233	/* Debug Control Register 4 */
 #define SPRN_MSRP	0x137	/* MSR Protect Register */
 #define SPRN_IAC3	0x13A	/* Instruction Address Compare 3 */
 #define SPRN_IAC4	0x13B	/* Instruction Address Compare 4 */
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 52c7ad7..1310775 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -116,7 +116,7 @@  int main(void)
 #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
 	DEFINE(THREAD_KVM_SVCPU, offsetof(struct thread_struct, kvm_shadow_vcpu));
 #endif
-#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
 
@@ -431,6 +431,9 @@  int main(void)
 
 	DEFINE(VCPU_KVM, offsetof(struct kvm_vcpu, kvm));
 	DEFINE(KVM_LPID, offsetof(struct kvm, arch.lpid));
+#ifdef CONFIG_BOOKE
+	DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save));
+#endif
 
 	/* book3s */
 #ifdef CONFIG_KVM_BOOK3S_64_HV
@@ -562,6 +565,32 @@  int main(void)
 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
 	DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
 	DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
+	DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
+	DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
+	DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
+	DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr0));
+	DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr1));
+	DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr2));
+#ifdef CONFIG_KVM_E500MC
+	DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
+					  dbcr4));
+#endif
+	DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[0]));
+	DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[1]));
+	DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[2]));
+	DEFINE(KVMPPC_DBG_IAC4, offsetof(struct kvmppc_booke_debug_reg,
+					 iac[3]));
+	DEFINE(KVMPPC_DBG_DAC1, offsetof(struct kvmppc_booke_debug_reg,
+					 dac[0]));
+	DEFINE(KVMPPC_DBG_DAC2, offsetof(struct kvmppc_booke_debug_reg,
+					 dac[1]));
+	DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
 #endif /* CONFIG_PPC_BOOK3S */
 #endif /* CONFIG_KVM */
 
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)) {
+		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;
+	}
+
 	er = kvmppc_emulate_instruction(run, vcpu);
 	switch (er) {
 	case EMULATE_DONE:
@@ -714,6 +728,44 @@  static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
 	default:
 		BUG();
 	}
+
+	if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&
+	    (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
+		run->exit_reason = KVM_EXIT_DEBUG;
+		return RESUME_HOST;
+	}
+}
+
+static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
+{
+	u32 dbsr;
+
+#ifndef CONFIG_KVM_BOOKE_HV
+	if (cpu_has_feature(CPU_FTR_DEBUG_LVL_EXC))
+		vcpu->arch.pc = mfspr(SPRN_DSRR0);
+	else
+		vcpu->arch.pc = mfspr(SPRN_CSRR0);
+#endif
+	dbsr = vcpu->arch.dbsr;
+
+	run->debug.arch.pc = vcpu->arch.pc;
+	run->debug.arch.status = 0;
+	vcpu->arch.dbsr = 0;
+
+	if (dbsr & (DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4)) {
+		run->debug.arch.status |= KVMPPC_DEBUG_BREAKPOINT;
+	} else {
+		if (dbsr & (DBSR_DAC1W | DBSR_DAC2W))
+			run->debug.arch.status |= KVMPPC_DEBUG_WATCH_WRITE;
+		else if (dbsr & (DBSR_DAC1R | DBSR_DAC2R))
+			run->debug.arch.status |= KVMPPC_DEBUG_WATCH_READ;
+		if (dbsr & (DBSR_DAC1R | DBSR_DAC1W))
+			run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac[0];
+		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
+			run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac[1];
+	}
+
+	return RESUME_HOST;
 }
 
 static void kvmppc_fill_pt_regs(struct pt_regs *regs)
@@ -856,7 +908,7 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		break;
 
 	case BOOKE_INTERRUPT_HV_PRIV:
-		r = emulation_exit(run, vcpu);
+		r = emulation_exit(run, vcpu, exit_nr);
 		break;
 
 	case BOOKE_INTERRUPT_PROGRAM:
@@ -875,7 +927,7 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			break;
 		}
 
-		r = emulation_exit(run, vcpu);
+		r = emulation_exit(run, vcpu, exit_nr);
 		break;
 
 	case BOOKE_INTERRUPT_FP_UNAVAIL:
@@ -1065,18 +1117,12 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	}
 
 	case BOOKE_INTERRUPT_DEBUG: {
-		u32 dbsr;
-
-		vcpu->arch.pc = mfspr(SPRN_CSRR0);
-
-		/* clear IAC events in DBSR register */
-		dbsr = mfspr(SPRN_DBSR);
-		dbsr &= DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4;
-		mtspr(SPRN_DBSR, dbsr);
-
-		run->exit_reason = KVM_EXIT_DEBUG;
+		r = kvmppc_handle_debug(run, vcpu);
+		if (r == RESUME_HOST) {
+			run->debug.arch.exception = exit_nr;
+			run->exit_reason = KVM_EXIT_DEBUG;
+		}
 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
-		r = RESUME_HOST;
 		break;
 	}
 
@@ -1107,6 +1153,78 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	return r;
 }
 
+#define BP_NUM	KVMPPC_BOOKE_IAC_NUM
+#define WP_NUM	KVMPPC_BOOKE_DAC_NUM
+
+int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
+				struct kvm_guest_debug *dbg)
+{
+	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
+		vcpu->guest_debug = 0;
+		return 0;
+	}
+
+	vcpu->guest_debug = dbg->control;
+	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+		struct kvmppc_booke_debug_reg *gdbgr =
+				&(vcpu->arch.shadow_dbg_reg);
+		int n, b = 0, w = 0;
+		const u32 bp_code[] = {
+			DBCR0_IAC1 | DBCR0_IDM,
+			DBCR0_IAC2 | DBCR0_IDM,
+			DBCR0_IAC3 | DBCR0_IDM,
+			DBCR0_IAC4 | DBCR0_IDM
+		};
+		const u32 wp_code[] = {
+			DBCR0_DAC1W | DBCR0_IDM,
+			DBCR0_DAC2W | DBCR0_IDM,
+			DBCR0_DAC1R | DBCR0_IDM,
+			DBCR0_DAC2R | DBCR0_IDM
+		};
+
+#ifndef CONFIG_KVM_BOOKE_HV
+		gdbgr->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US |
+				DBCR1_IAC3US | DBCR1_IAC4US;
+		gdbgr->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
+#else
+		gdbgr->dbcr1 = 0;
+		gdbgr->dbcr2 = 0;
+#endif
+
+		for (n = 0; n < (BP_NUM + WP_NUM); n++) {
+			u32 type = dbg->arch.bp[n].type;
+
+			if (!type)
+				break;
+
+			if (type & (KVMPPC_DEBUG_WATCH_READ |
+				    KVMPPC_DEBUG_WATCH_WRITE)) {
+				if (w < WP_NUM) {
+					if (type & KVMPPC_DEBUG_WATCH_READ)
+						gdbgr->dbcr0 |= wp_code[w + 2];
+					if (type & KVMPPC_DEBUG_WATCH_WRITE)
+						gdbgr->dbcr0 |= wp_code[w];
+					gdbgr->dac[w] = dbg->arch.bp[n].addr;
+					w++;
+				}
+			} else if (type & KVMPPC_DEBUG_BREAKPOINT) {
+				if (b < BP_NUM) {
+					gdbgr->dbcr0 |= bp_code[b];
+					gdbgr->iac[b] = dbg->arch.bp[n].addr;
+					b++;
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
 /* Initial guest state: 16MB mapping 0 -> 0, PC = 0, MSR = 0, R1 = 16MB */
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index bcb34ea..fb85606 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -40,6 +40,8 @@ 
 #define HOST_MIN_STACK_SIZE (HOST_NV_GPR(31) + 4)
 #define HOST_STACK_SIZE (((HOST_MIN_STACK_SIZE + 15) / 16) * 16) /* Align. */
 #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
+#define DBCR0_AC_BITS	(DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
+			 DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
 
 #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
                         (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
@@ -53,11 +55,17 @@ 
                        (1<<BOOKE_INTERRUPT_PROGRAM) | \
                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
 
+#define NEED_DEBUG_SAVE (1<<BOOKE_INTERRUPT_DEBUG)
+
+#define GET_VCPU_POINT(regd)                 \
+	mfspr   regd, SPRN_SPRG_THREAD;      \
+	lwz	regd, THREAD_KVM_VCPU(regd)
+
 .macro KVM_HANDLER ivor_nr scratch srr0
 _GLOBAL(kvmppc_handler_\ivor_nr)
 	/* Get pointer to vcpu and record exit number. */
 	mtspr	\scratch , r4
-	mfspr	r4, SPRN_SPRG_RVCPU
+	GET_VCPU_POINT(r4)
 	stw	r3, VCPU_GPR(r3)(r4)
 	stw	r5, VCPU_GPR(r5)(r4)
 	stw	r6, VCPU_GPR(r6)(r4)
@@ -74,6 +82,48 @@  _GLOBAL(kvmppc_handler_\ivor_nr)
 	bctr
 .endm
 
+.macro KVM_DBG_HANDLER ivor_nr scratch srr0
+_GLOBAL(kvmppc_handler_\ivor_nr)
+	mtspr   \scratch, r4
+	GET_VCPU_POINT(r4)
+	stw	r3, VCPU_CRIT_SAVE(r4)
+	mfcr	r3
+	mfspr	r4, SPRN_CSRR1
+	andi.	r4, r4, MSR_PR
+	bne	1f
+	/* debug interrupt happened in enter/exit path */
+	mfspr   r4, SPRN_CSRR1
+	rlwinm  r4, r4, 0, ~MSR_DE
+	mtspr   SPRN_CSRR1, r4
+	lis	r4, 0xffff
+	ori	r4, r4, 0xffff
+	mtspr	SPRN_DBSR, r4
+	GET_VCPU_POINT(r4)
+	mtcr	r3
+	lwz     r3, VCPU_CRIT_SAVE(r4)
+	mfspr   r4, \scratch
+	rfci
+1:	/* debug interrupt happened in guest */
+	mfspr   r4, \scratch
+	mtcr	r3
+	mr	r3, r4
+	GET_VCPU_POINT(r4)
+	stw	r3, VCPU_GPR(r4)(r4)
+	stw	r5, VCPU_GPR(r5)(r4)
+	stw	r6, VCPU_GPR(r6)(r4)
+	lwz     r3, VCPU_CRIT_SAVE(r4)
+	mfspr	r5, \srr0
+	stw	r3, VCPU_GPR(r3)(r4)
+	stw	r5, VCPU_PC(r4)
+	mfctr	r5
+	lis	r6, kvmppc_resume_host@h
+	stw	r5, VCPU_CTR(r4)
+	li	r5, \ivor_nr
+	ori	r6, r6, kvmppc_resume_host@l
+	mtctr	r6
+	bctr
+.endm
+
 .macro KVM_HANDLER_ADDR ivor_nr
 	.long	kvmppc_handler_\ivor_nr
 .endm
@@ -94,7 +144,7 @@  KVM_HANDLER BOOKE_INTERRUPT_FIT SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_WATCHDOG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
 KVM_HANDLER BOOKE_INTERRUPT_DTLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_ITLB_MISS SPRN_SPRG_RSCRATCH0 SPRN_SRR0
-KVM_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
+KVM_DBG_HANDLER BOOKE_INTERRUPT_DEBUG SPRN_SPRG_RSCRATCH_CRIT SPRN_CSRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_UNAVAIL SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_DATA SPRN_SPRG_RSCRATCH0 SPRN_SRR0
 KVM_HANDLER BOOKE_INTERRUPT_SPE_FP_ROUND SPRN_SPRG_RSCRATCH0 SPRN_SRR0
@@ -176,6 +226,61 @@  _GLOBAL(kvmppc_resume_host)
 	stw	r9, VCPU_FAULT_ESR(r4)
 ..skip_esr:
 
+	addi	r7, r4, VCPU_HOST_DBG
+	mfspr	r9, SPRN_DBCR0
+	lwz	r8, KVMPPC_DBG_DBCR0(r7)
+	andis.	r9, r9, DBCR0_AC_BITS@h
+	beq	..skip_load_host_debug
+	li	r9, 0
+	mtspr	SPRN_DBCR0, r9		/* disable all debug event */
+	lwz	r9, KVMPPC_DBG_DBCR1(r7)
+	mtspr	SPRN_DBCR1, r9
+	lwz	r9, KVMPPC_DBG_DBCR2(r7)
+	mtspr	SPRN_DBCR2, r9
+	lwz	r9, KVMPPC_DBG_IAC1+4(r7)
+	mtspr	SPRN_IAC1, r9
+	lwz	r9, KVMPPC_DBG_IAC2+4(r7)
+	mtspr	SPRN_IAC2, r9
+#ifndef CONFIG_PPC_FSL_BOOK3E
+	lwz	r9, KVMPPC_DBG_IAC3+4(r7)
+	mtspr	SPRN_IAC3, r9
+	lwz	r9, KVMPPC_DBG_IAC4+4(r7)
+	mtspr	SPRN_IAC4, r9
+#endif
+	lwz	r9, KVMPPC_DBG_DAC1+4(r7)
+	mtspr	SPRN_DAC1, r9
+	lwz	r9, KVMPPC_DBG_DAC2+4(r7)
+	mtspr	SPRN_DAC2, r9
+..skip_load_host_debug:
+	/* Clear h/w DBSR and save current(guest) DBSR */
+	mfspr	r9, SPRN_DBSR
+	mtspr	SPRN_DBSR, r9
+	isync
+	andi.	r7, r6, NEED_DEBUG_SAVE
+	beq	..skip_dbsr_save
+	/*
+	 * If vcpu->guest_debug flag is set then do not check for
+	 * shared->msr.DE as this debugging (say by QEMU) does not
+	 * depends on shared->msr.de. In these scanerios MSR.DE is
+	 * always set using shared_msr and should be handled always.
+	 */
+	lwz	r7, VCPU_GUEST_DEBUG(r4)
+	cmpwi	r7, 0
+	bne	..skip_save_trap_event
+	PPC_LL	r3, VCPU_SHARED(r4)
+#ifndef CONFIG_64BIT
+	lwz	r3, (VCPU_SHARED_MSR + 4)(r3)
+#else
+	ld	r3, (VCPU_SHARED_MSR)(r3)
+#endif
+	andi.	r3, r3, MSR_DE
+	bne	..skip_save_trap_event
+	andis.	r9, r9, DBSR_TIE@h
+..skip_save_trap_event:
+	stw	r9, VCPU_DBSR(r4)
+..skip_dbsr_save:
+	mtspr	SPRN_DBCR0, r8
+
 	/* Save remaining volatile guest register state to vcpu. */
 	stw	r0, VCPU_GPR(r0)(r4)
 	stw	r1, VCPU_GPR(r1)(r4)
@@ -432,6 +537,57 @@  lightweight_exit:
 	PPC_LD(r3, VCPU_SHARED_SPRG7, r5)
 	mtspr	SPRN_SPRG7W, r3
 
+	mfmsr	r7
+	rlwinm	r7, r7, 0, ~MSR_DE
+	mtmsr	r7
+	addi	r5, r4, VCPU_SHADOW_DBG
+	addi	r7, r4, VCPU_HOST_DBG
+	lwz	r6, 0(r5)
+	mfspr	r8, SPRN_DBCR0
+	andis.	r3, r6, DBCR0_AC_BITS@h
+	stw	r8, KVMPPC_DBG_DBCR0(r7)
+	beq	..skip_load_guest_debug
+	mfspr	r8, SPRN_DBCR1
+	stw	r8, KVMPPC_DBG_DBCR1(r7)
+	mfspr	r8, SPRN_DBCR2
+	stw	r8, KVMPPC_DBG_DBCR2(r7)
+	mfspr	r8, SPRN_IAC1
+	stw	r8, KVMPPC_DBG_IAC1+4(r7)
+	mfspr	r8, SPRN_IAC2
+	stw	r8, KVMPPC_DBG_IAC2+4(r7)
+#ifndef CONFIG_PPC_FSL_BOOK3E
+	mfspr	r8, SPRN_IAC3
+	stw	r8, KVMPPC_DBG_IAC3+4(r7)
+	mfspr	r8, SPRN_IAC4
+	stw	r8, KVMPPC_DBG_IAC4+4(r7)
+#endif
+	mfspr	r8, SPRN_DAC1
+	stw	r8, KVMPPC_DBG_DAC1+4(r7)
+	mfspr	r8, SPRN_DAC2
+	stw	r8, KVMPPC_DBG_DAC2+4(r7)
+	li	r8, 0
+	mtspr	SPRN_DBCR0, r8		/* disable all debug event */
+	lwz	r8, KVMPPC_DBG_DBCR1(r5)
+	mtspr	SPRN_DBCR1, r8
+	lwz	r8, KVMPPC_DBG_DBCR2(r5)
+	mtspr	SPRN_DBCR2, r8
+	lwz	r8, KVMPPC_DBG_IAC1+4(r5)
+	mtspr	SPRN_IAC1, r8
+	lwz	r8, KVMPPC_DBG_IAC2+4(r5)
+	mtspr	SPRN_IAC2, r8
+#ifndef CONFIG_PPC_FSL_BOOK3E
+	lwz	r8, KVMPPC_DBG_IAC3+4(r5)
+	mtspr	SPRN_IAC3, r8
+	lwz	r8, KVMPPC_DBG_IAC4+4(r5)
+	mtspr	SPRN_IAC4, r8
+#endif
+	lwz	r8, KVMPPC_DBG_DAC1+4(r5)
+	mtspr	SPRN_DAC1, r8
+	lwz	r8, KVMPPC_DBG_DAC2+4(r5)
+	mtspr	SPRN_DAC2, r8
+..skip_load_guest_debug:
+	mtspr	SPRN_DBCR0, r6
+
 #ifdef CONFIG_KVM_EXIT_TIMING
 	/* save enter time */
 1:
diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
index 0fa2ef7..32b9a41 100644
--- a/arch/powerpc/kvm/bookehv_interrupts.S
+++ b/arch/powerpc/kvm/bookehv_interrupts.S
@@ -59,6 +59,10 @@ 
 #define NEED_EMU		0x00000001 /* emulation -- save nv regs */
 #define NEED_DEAR		0x00000002 /* save faulting DEAR */
 #define NEED_ESR		0x00000004 /* save faulting ESR */
+#define NEED_DBSR		0x00000008 /* save DBSR */
+
+#define DBCR0_AC_BITS	(DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | \
+			 DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
 
 /*
  * On entry:
@@ -202,6 +206,11 @@ 
 	PPC_STL	r9, VCPU_FAULT_DEAR(r4)
 	.endif
 
+	.if	\flags & NEED_DBSR
+	mfspr	r9, SPRN_DBSR
+	stw	r9, VCPU_DBSR(r4)
+	.endif
+
 	b	kvmppc_resume_host
 .endm
 
@@ -296,9 +305,9 @@  kvm_handler BOOKE_INTERRUPT_GUEST_DBELL, SPRN_GSRR0, SPRN_GSRR1, 0
 kvm_lvl_handler BOOKE_INTERRUPT_GUEST_DBELL_CRIT, \
 	SPRN_SPRG_RSCRATCH_CRIT, SPRN_CSRR0, SPRN_CSRR1, 0
 kvm_lvl_handler BOOKE_INTERRUPT_DEBUG, \
-	SPRN_SPRG_RSCRATCH_CRIT, SPRN_CSRR0, SPRN_CSRR1, 0
+	SPRN_SPRG_RSCRATCH_CRIT, SPRN_CSRR0, SPRN_CSRR1, NEED_DBSR
 kvm_lvl_handler BOOKE_INTERRUPT_DEBUG, \
-	SPRN_SPRG_RSCRATCH_DBG, SPRN_DSRR0, SPRN_DSRR1, 0
+	SPRN_SPRG_RSCRATCH_DBG, SPRN_DSRR0, SPRN_DSRR1, NEED_DBSR
 
 
 /* Registers:
@@ -308,6 +317,56 @@  kvm_lvl_handler BOOKE_INTERRUPT_DEBUG, \
  *  r14: KVM exit number
  */
 _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
+	lwz	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
+	lwz	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
+	lwz	r9, VCPU_HOST_DBG+KVMPPC_DBG_DBCR4(r4)
+	mtspr	SPRN_DBCR1, r7
+	PPC_LD(r6, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
+	PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
+	mtspr	SPRN_DBCR2, r8
+	mtspr	SPRN_DBCR4, r9
+	mtspr	SPRN_IAC1, r6
+	mtspr	SPRN_IAC2, r7
+#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
+	PPC_LD(r8, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
+	PPC_LD(r9, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
+	mtspr	SPRN_DAC1, r8
+	mtspr	SPRN_DAC2, r9
+..skip_load_hw_bkpts:
+	isync
+	/* Clear h/w DBSR and save current(guest) DBSR */
+	mfspr	r8, SPRN_DBSR
+	mtspr	SPRN_DBSR, r8
+	isync
+	/* Clear EPCR.DUVD and set host DBCR0 */
+	mfspr	r8, SPRN_EPCR
+	rlwinm	r8, r8, 0, ~SPRN_EPCR_DUVD
+	mtspr	SPRN_EPCR, r8
+	isync
+	mtspr	SPRN_DBCR0, r3
+	isync
+skip_load_host_debug:
+
 	/* Save remaining volatile guest register state to vcpu. */
 	mfspr	r3, SPRN_VRSAVE
 	PPC_STL	r0, VCPU_GPR(r0)(r4)
@@ -547,6 +606,84 @@  lightweight_exit:
 	mtspr	SPRN_SPRG6W, r7
 	mtspr	SPRN_SPRG7W, r8
 
+	mfmsr	r7
+	rlwinm	r7, r7, 0, ~MSR_DE
+	mtmsr	r7
+	/*
+	 * Load hw debug registers with guest(shadow) debug registers
+	 * if guest is using the debug facility and also set EPCR.DUVD
+	 * to not allow debug events in HV mode. Do not change the
+	 * debug registers if guest is not using the debug facility.
+	 */
+	lwz	r6, VCPU_SHADOW_DBG(r4)
+	rlwinm.	r7, r6, 0, ~DBCR0_IDM
+	beq	..skip_load_guest_debug
+	/* Save host DBCR0 */
+	mfspr	r8, SPRN_DBCR0
+	stw	r8, VCPU_HOST_DBG(r4)
+	/*
+	 * Save host DBCR1/2, IACx and DACx and load guest DBCR1/2,
+	 * IACx and DACx if guest using hw breakpoint/watchpoints.
+	 */
+	andis.	r3, r6, DBCR0_AC_BITS@h
+	beq	..skip_hw_bkpts
+	mfspr	r7, SPRN_DBCR1
+	stw	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR1(r4)
+	mfspr	r8, SPRN_DBCR2
+	stw	r8, VCPU_HOST_DBG+KVMPPC_DBG_DBCR2(r4)
+	mfspr	r7, SPRN_DBCR4
+	stw	r7, VCPU_HOST_DBG+KVMPPC_DBG_DBCR4(r4)
+	mfspr	r8, SPRN_IAC1
+	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC1, r4)
+	mfspr	r7, SPRN_IAC2
+	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC2, r4)
+#ifndef CONFIG_PPC_FSL_BOOK3E
+	mfspr	r8, SPRN_IAC3
+	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
+	mfspr	r7, SPRN_IAC4
+	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
+#endif
+	mfspr	r8, SPRN_DAC1
+	PPC_STD(r8, VCPU_HOST_DBG+KVMPPC_DBG_DAC1, r4)
+	mfspr	r7, SPRN_DAC2
+	PPC_STD(r7, VCPU_HOST_DBG+KVMPPC_DBG_DAC2, r4)
+	li	r8, 0
+	mtspr	SPRN_DBCR0, r8		/* disable all debug event */
+	lwz	r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR1(r4)
+	lwz	r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR2(r4)
+	lwz	r9, VCPU_SHADOW_DBG+KVMPPC_DBG_DBCR4(r4)
+	mtspr	SPRN_DBCR1, r7
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC1, r4)
+	PPC_LD(r3, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC2, r4)
+	mtspr	SPRN_DBCR2, r8
+	mtspr	SPRN_DBCR4, r9
+	mtspr	SPRN_IAC1, r7
+	mtspr	SPRN_IAC2, r3
+#ifndef CONFIG_PPC_FSL_BOOK3E
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC3, r4)
+	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_IAC4, r4)
+	mtspr	SPRN_IAC3, r7
+	mtspr	SPRN_IAC4, r8
+#endif
+	PPC_LD(r7, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC1, r4)
+	PPC_LD(r8, VCPU_SHADOW_DBG+KVMPPC_DBG_DAC2, r4)
+	mtspr	SPRN_DAC1, r7
+	mtspr	SPRN_DAC2, r8
+..skip_hw_bkpts:
+	/* Set EPCR.DUVD and guest DBCR0 */
+	mfspr	r7, SPRN_EPCR
+	oris	r7, r7, SPRN_EPCR_DUVD@h
+	mtspr	SPRN_EPCR, r7
+	isync
+	/* Clear if any deferred debug event */
+	mfspr	r8, SPRN_DBSR
+	mtspr	SPRN_DBSR, r8
+	isync
+	/* Restore guest DBCR */
+	mtspr	SPRN_DBCR0, r6
+	isync
+..skip_load_guest_debug:
+
 	/* Load some guest volatiles. */
 	PPC_LL	r3, VCPU_LR(r4)
 	PPC_LL	r5, VCPU_XER(r4)
diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
index 1f89d26..f5fc6f5 100644
--- a/arch/powerpc/kvm/e500mc.c
+++ b/arch/powerpc/kvm/e500mc.c
@@ -182,8 +182,7 @@  int kvmppc_core_vcpu_setup(struct kvm_vcpu *vcpu)
 {
 	struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
 
-	vcpu->arch.shadow_epcr = SPRN_EPCR_DSIGS | SPRN_EPCR_DGTMI | \
-				 SPRN_EPCR_DUVD;
+	vcpu->arch.shadow_epcr = SPRN_EPCR_DSIGS | SPRN_EPCR_DGTMI;
 #ifdef CONFIG_64BIT
 	vcpu->arch.shadow_epcr |= SPRN_EPCR_ICM;
 #endif
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);
 }
 
 static void kvmppc_complete_dcr_load(struct kvm_vcpu *vcpu,