Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-02-07 Thread Alexander Graf 

On 07.02.2013, at 16:00, Bhushan Bharat-R65777 wrote:

 
 
 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, February 02, 2013 4:09 AM
 To: Alexander Graf
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 On 01/31/2013 06:11:32 PM, Alexander Graf wrote:
 
 On 31.01.2013, at 23:40, Scott Wood wrote:
 
 On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
 On 31.01.2013, at 20:05, Alexander Graf wrote:
 
 On 31.01.2013, at 19:54, Scott Wood wrote:
 
 On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
 On 31.01.2013, at 19:43, Scott Wood wrote:
 On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
 How about something like this? Then both targets at least
 suck as much :).
 
 I'm not sure that should be the goal...
 
 Thanks to e500mc's awful hardware design, we don't know who
 sets the MSR_DE bit. Once we forced it onto the guest, we have no
 change to know whether the guest also set it or not. We could only
 guess.
 
 MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but
 we still need to set it in the first place.
 
 According to ISA V2.06B, the hypervisor should set DBCR0[EDM]
 to let the guest know that the debug resources are not available, and
 that the value of MSR[DE] is not specified and not modifiable.
 So what would the guest do then to tell the hypervisor that it
 actually wants to know about debug events?
 
 The guest is out of luck, just as if a JTAG were in use.
 
 Hrm.
 
 Can we somehow generalize this out of luck behavior?
 
 Every time we would set or clear an MSR bit in shadow_msr on
 e500v2, we would instead set or clear it in the real MSR. That way
 only e500mc is out of luck, but the code would still be shared.
 
 I don't follow.  e500v2 is just as out-of-luck.  The mechanism
 simply does not support sharing debug resources.
 
 For e500v2 we have 2 fields
 
  * MSR as the guest sees it
  * MSR as we execute when the guest runs
 
 Since we know the MSR when the guest sees it, we can decide what to do
 when we get an unhandled debug interrupt.
 
 That's not the same thing as making the real MSR[DE] show up in the guest
 MSR[DE].
 
 There are other problems with sharing -- what happens when both host and 
 guest
 try to write to a particular IAC or DAC?
 
 Also, performance would be pretty awful if the guest has e.g. single 
 stepping in
 DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single stepping
 (but does want debugging enabled in general).
 
 What do you mean by the real MSR?  The real MSR is shadow_msr,
 and MSR_DE must always be set there if the host is debugging the
 guest.  As for reflecting it into the guest MSR, we could, but I don't
 really see the point.  We're never going to actually send a debug
 exception to the guest when the host owns the debug resources.
 
 Why not? That's the whole point of jumping through user space.
 
 That's still needed for software breakpoints, which don't rely on the debug
 resources.
 
  1) guest exits with debug interrupt
  2) QEMU gets a debug exit
  3) QEMU checks in its list whether it belongs to its own debug
 points
  4) if not, it reinjects the interrupt into the guest
 
 Step 4 is pretty difficult to do when we don't know whether the guest
 is actually capable of handling debug interrupts at that moment.
 
 Software breakpoints take a Program interrupt rather than a Debug interrupt,
 unless MSR[DE]=1 and DBCR0[TRAP]=1.  If the guest does not own debug 
 resources
 we should always send it to the Program interrupt, so MSR[DE] doesn't matter.
 
 The = ~MSR_DE line is pointless on bookehv, and makes it harder
 to read.  I had to stare at it a while before noticing that you
 initially set is_debug from the guest MSR and that you'd never really
 clear MSR_DE here on bookehv.
 
 Well, I'm mostly bouncing ideas here to find a way to express what
 we're trying to say in a way that someone who hasn't read this email
 thread would still understand what's going on :).
 
 I think it's already straightforward enough if you accept that shared debug
 resources aren't supported, and that we are either in a mode where the real
 MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is 
 always
 on in guest mode and the guest MSR[DE] is irrelevant.
 
 How about this version?
 
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
 38a62ef..9929c41 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
 *vcpu)
 #endif
 }
 
 +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { #ifndef
 +CONFIG_KVM_BOOKE_HV
 +   /* Synchronize guest's desire to get debug interrupts into
 shadow MSR */
 +   vcpu-arch.shadow_msr = ~MSR_DE;
 +   vcpu-arch.shadow_msr |= vcpu-arch.shared-msr  MSR_DE; #endif
 +
 +   /* Force enable debug interrupts when user space wants to debug
 */
 +   if (vcpu-guest_debug

RE: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-02-07 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On
 Behalf Of Alexander Graf
 Sent: Thursday, February 07, 2013 8:29 PM
 To: Wood Scott-B07421
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 
 On 01.02.2013, at 23:38, Scott Wood wrote:
 
  On 01/31/2013 06:11:32 PM, Alexander Graf wrote:
  On 31.01.2013, at 23:40, Scott Wood wrote:
   On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
   On 31.01.2013, at 20:05, Alexander Graf wrote:
   
On 31.01.2013, at 19:54, Scott Wood wrote:
   
On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
On 31.01.2013, at 19:43, Scott Wood wrote:
On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
How about something like this? Then both targets at least suck as
 much :).
   
I'm not sure that should be the goal...
   
Thanks to e500mc's awful hardware design, we don't know who sets 
the
 MSR_DE bit. Once we forced it onto the guest, we have no change to know 
 whether
 the guest also set it or not. We could only guess.
   
MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we
 still need to set it in the first place.
   
According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to 
let
 the guest know that the debug resources are not available, and that the value
 of MSR[DE] is not specified and not modifiable.
So what would the guest do then to tell the hypervisor that it
 actually wants to know about debug events?
   
The guest is out of luck, just as if a JTAG were in use.
   
Hrm.
   
Can we somehow generalize this out of luck behavior?
   
Every time we would set or clear an MSR bit in shadow_msr on e500v2, 
we
 would instead set or clear it in the real MSR. That way only e500mc is out of
 luck, but the code would still be shared.
  
   I don't follow.  e500v2 is just as out-of-luck.  The mechanism simply 
   does
 not support sharing debug resources.
  For e500v2 we have 2 fields
   * MSR as the guest sees it
   * MSR as we execute when the guest runs Since we know the MSR when
  the guest sees it, we can decide what to do when we get an unhandled debug
 interrupt.
 
  That's not the same thing as making the real MSR[DE] show up in the guest
 MSR[DE].
 
  There are other problems with sharing -- what happens when both host and 
  guest
 try to write to a particular IAC or DAC?
 
  Also, performance would be pretty awful if the guest has e.g. single 
  stepping
 in DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single 
 stepping
 (but does want debugging enabled in general).
 
   What do you mean by the real MSR?  The real MSR is shadow_msr, and 
   MSR_DE
 must always be set there if the host is debugging the guest.  As for 
 reflecting
 it into the guest MSR, we could, but I don't really see the point.  We're 
 never
 going to actually send a debug exception to the guest when the host owns the
 debug resources.
  Why not? That's the whole point of jumping through user space.
 
  That's still needed for software breakpoints, which don't rely on the debug
 resources.
 
   1) guest exits with debug interrupt
   2) QEMU gets a debug exit
   3) QEMU checks in its list whether it belongs to its own debug
  points
   4) if not, it reinjects the interrupt into the guest Step 4 is
  pretty difficult to do when we don't know whether the guest is actually
 capable of handling debug interrupts at that moment.
 
  Software breakpoints take a Program interrupt rather than a Debug interrupt,
 unless MSR[DE]=1 and DBCR0[TRAP]=1.  If the guest does not own debug resources
 we should always send it to the Program interrupt, so MSR[DE] doesn't matter.
 
   The = ~MSR_DE line is pointless on bookehv, and makes it harder to 
   read.
 I had to stare at it a while before noticing that you initially set is_debug
 from the guest MSR and that you'd never really clear MSR_DE here on bookehv.
  Well, I'm mostly bouncing ideas here to find a way to express what we're
 trying to say in a way that someone who hasn't read this email thread would
 still understand what's going on :).
 
  I think it's already straightforward enough if you accept that shared debug
 resources aren't supported, and that we are either in a mode where the real
 MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is always
 on in guest mode and the guest MSR[DE] is irrelevant.
 
 I think I'm starting to grasp what you're suggesting:
 
 On e500mc, have 2 modes
 
   1) guest owns debug
 
   This is the normal operation. Here the guest defines the value of MSR_DE. 
 The
 guest gets debug interrupts directly.
 
   2) host owns debug
 
   In this case, take away any debug capabilities from the guest. Everything
 debug related goes straight to QEMU.
 
 
 On e500v2, have 2 modes
 
   1) guest owns debug
 
   This is the normal operation. Here the guest

Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-02-07 Thread Alexander Graf 

On 07.02.2013, at 16:25, Bhushan Bharat-R65777 wrote:

 
 
 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On
 Behalf Of Alexander Graf
 Sent: Thursday, February 07, 2013 8:29 PM
 To: Wood Scott-B07421
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 
 On 01.02.2013, at 23:38, Scott Wood wrote:
 
 On 01/31/2013 06:11:32 PM, Alexander Graf wrote:
 On 31.01.2013, at 23:40, Scott Wood wrote:
 On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
 On 31.01.2013, at 20:05, Alexander Graf wrote:
 
 On 31.01.2013, at 19:54, Scott Wood wrote:
 
 On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
 On 31.01.2013, at 19:43, Scott Wood wrote:
 On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
 How about something like this? Then both targets at least suck as
 much :).
 
 I'm not sure that should be the goal...
 
 Thanks to e500mc's awful hardware design, we don't know who sets the
 MSR_DE bit. Once we forced it onto the guest, we have no change to know 
 whether
 the guest also set it or not. We could only guess.
 
 MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we
 still need to set it in the first place.
 
 According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to let
 the guest know that the debug resources are not available, and that the 
 value
 of MSR[DE] is not specified and not modifiable.
 So what would the guest do then to tell the hypervisor that it
 actually wants to know about debug events?
 
 The guest is out of luck, just as if a JTAG were in use.
 
 Hrm.
 
 Can we somehow generalize this out of luck behavior?
 
 Every time we would set or clear an MSR bit in shadow_msr on e500v2, we
 would instead set or clear it in the real MSR. That way only e500mc is out of
 luck, but the code would still be shared.
 
 I don't follow.  e500v2 is just as out-of-luck.  The mechanism simply does
 not support sharing debug resources.
 For e500v2 we have 2 fields
 * MSR as the guest sees it
 * MSR as we execute when the guest runs Since we know the MSR when
 the guest sees it, we can decide what to do when we get an unhandled debug
 interrupt.
 
 That's not the same thing as making the real MSR[DE] show up in the guest
 MSR[DE].
 
 There are other problems with sharing -- what happens when both host and 
 guest
 try to write to a particular IAC or DAC?
 
 Also, performance would be pretty awful if the guest has e.g. single 
 stepping
 in DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single 
 stepping
 (but does want debugging enabled in general).
 
 What do you mean by the real MSR?  The real MSR is shadow_msr, and 
 MSR_DE
 must always be set there if the host is debugging the guest.  As for 
 reflecting
 it into the guest MSR, we could, but I don't really see the point.  We're 
 never
 going to actually send a debug exception to the guest when the host owns the
 debug resources.
 Why not? That's the whole point of jumping through user space.
 
 That's still needed for software breakpoints, which don't rely on the debug
 resources.
 
 1) guest exits with debug interrupt
 2) QEMU gets a debug exit
 3) QEMU checks in its list whether it belongs to its own debug
 points
 4) if not, it reinjects the interrupt into the guest Step 4 is
 pretty difficult to do when we don't know whether the guest is actually
 capable of handling debug interrupts at that moment.
 
 Software breakpoints take a Program interrupt rather than a Debug interrupt,
 unless MSR[DE]=1 and DBCR0[TRAP]=1.  If the guest does not own debug 
 resources
 we should always send it to the Program interrupt, so MSR[DE] doesn't matter.
 
 The = ~MSR_DE line is pointless on bookehv, and makes it harder to 
 read.
 I had to stare at it a while before noticing that you initially set is_debug
 from the guest MSR and that you'd never really clear MSR_DE here on bookehv.
 Well, I'm mostly bouncing ideas here to find a way to express what we're
 trying to say in a way that someone who hasn't read this email thread would
 still understand what's going on :).
 
 I think it's already straightforward enough if you accept that shared debug
 resources aren't supported, and that we are either in a mode where the real
 MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is 
 always
 on in guest mode and the guest MSR[DE] is irrelevant.
 
 I think I'm starting to grasp what you're suggesting:
 
 On e500mc, have 2 modes
 
  1) guest owns debug
 
  This is the normal operation. Here the guest defines the value of MSR_DE. 
 The
 guest gets debug interrupts directly.
 
  2) host owns debug
 
  In this case, take away any debug capabilities from the guest. Everything
 debug related goes straight to QEMU.
 
 
 On e500v2, have 2 modes
 
  1) guest owns debug
 
  This is the normal operation. Here the guest defines the value of MSR_DE. 
 The
 guest gets debug interrupts

Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-02-04 Thread Scott Wood 

On 02/03/2013 10:48:29 PM, Bhushan Bharat-R65777 wrote:



 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, February 02, 2013 4:09 AM
 To: Alexander Graf
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;  
k...@vger.kernel.org
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt  
injection to guest


 On 01/31/2013 06:11:32 PM, Alexander Graf wrote:
  My main concern here is that we don't know when to remove MSR_DE  
again

  from the (shadow) MSR. So how about this one instead?

 Why wouldn't you know this?  if (vcpu-guest_debug) { you never  
remove it } else

 { just copy whatever's in guest MSR }

I think we are ok with shadow_msr on e500v2 but we can have problem  
on bookehv where we do not know when to clear MSR_DE in shared-msr.


How it works on e500mc:
	(1) User-space makes ioctl to use debug resource, we set  
vcpu-guest_debug.
	(2) Before entering into the guest we check vcpu-guest_debug  
flag and if set we set MSR_DE in shared-msr.
	(3) Sometime later user-space releases the debug resource then  
in ioctl handling will clear vcpu-guest_debug.
	(4) Now when entering to guest we do not know what to do with  
MSR_DE in shared-msr as we do now know if guest might have tried to  
set/clear MSR_DE in between step (2) and step(3). What should be safe  
thing to do? Can we leave MSR_DE set or clear MSR_DE. If we want to  
clear MSR_DE then will it be good idea to clear this in step (3)  
above (in ioctl where we clear vcpu-guest_debug).


Oh, you want to support dynamically changing the debug mode?  The  
hardware can't really deal with that, unless you paravirt MSR[DE],  
which doesn't seem worth it.  There's also the issue of confusing the  
guest if it checks EDM before you give debug to the host (this one  
applies to e500v2 as well).


-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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-02-03 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, February 02, 2013 4:09 AM
 To: Alexander Graf
 Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 On 01/31/2013 06:11:32 PM, Alexander Graf wrote:
 
  On 31.01.2013, at 23:40, Scott Wood wrote:
 
   On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
   On 31.01.2013, at 20:05, Alexander Graf wrote:
   
On 31.01.2013, at 19:54, Scott Wood wrote:
   
On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
On 31.01.2013, at 19:43, Scott Wood wrote:
On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
How about something like this? Then both targets at least
  suck as much :).
   
I'm not sure that should be the goal...
   
Thanks to e500mc's awful hardware design, we don't know who
  sets the MSR_DE bit. Once we forced it onto the guest, we have no
  change to know whether the guest also set it or not. We could only
  guess.
   
MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but
  we still need to set it in the first place.
   
According to ISA V2.06B, the hypervisor should set DBCR0[EDM]
  to let the guest know that the debug resources are not available, and
  that the value of MSR[DE] is not specified and not modifiable.
So what would the guest do then to tell the hypervisor that it
  actually wants to know about debug events?
   
The guest is out of luck, just as if a JTAG were in use.
   
Hrm.
   
Can we somehow generalize this out of luck behavior?
   
Every time we would set or clear an MSR bit in shadow_msr on
  e500v2, we would instead set or clear it in the real MSR. That way
  only e500mc is out of luck, but the code would still be shared.
  
   I don't follow.  e500v2 is just as out-of-luck.  The mechanism
  simply does not support sharing debug resources.
 
  For e500v2 we have 2 fields
 
* MSR as the guest sees it
* MSR as we execute when the guest runs
 
  Since we know the MSR when the guest sees it, we can decide what to do
  when we get an unhandled debug interrupt.
 
 That's not the same thing as making the real MSR[DE] show up in the guest
 MSR[DE].
 
 There are other problems with sharing -- what happens when both host and guest
 try to write to a particular IAC or DAC?
 
 Also, performance would be pretty awful if the guest has e.g. single stepping 
 in
 DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single stepping
 (but does want debugging enabled in general).
 
   What do you mean by the real MSR?  The real MSR is shadow_msr,
  and MSR_DE must always be set there if the host is debugging the
  guest.  As for reflecting it into the guest MSR, we could, but I don't
  really see the point.  We're never going to actually send a debug
  exception to the guest when the host owns the debug resources.
 
  Why not? That's the whole point of jumping through user space.
 
 That's still needed for software breakpoints, which don't rely on the debug
 resources.
 
1) guest exits with debug interrupt
2) QEMU gets a debug exit
3) QEMU checks in its list whether it belongs to its own debug
  points
4) if not, it reinjects the interrupt into the guest
 
  Step 4 is pretty difficult to do when we don't know whether the guest
  is actually capable of handling debug interrupts at that moment.
 
 Software breakpoints take a Program interrupt rather than a Debug interrupt,
 unless MSR[DE]=1 and DBCR0[TRAP]=1.  If the guest does not own debug resources
 we should always send it to the Program interrupt, so MSR[DE] doesn't matter.
 
   The = ~MSR_DE line is pointless on bookehv, and makes it harder
  to read.  I had to stare at it a while before noticing that you
  initially set is_debug from the guest MSR and that you'd never really
  clear MSR_DE here on bookehv.
 
  Well, I'm mostly bouncing ideas here to find a way to express what
  we're trying to say in a way that someone who hasn't read this email
  thread would still understand what's going on :).
 
 I think it's already straightforward enough if you accept that shared debug
 resources aren't supported, and that we are either in a mode where the real
 MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is always
 on in guest mode and the guest MSR[DE] is irrelevant.
 
  How about this version?
 
 
  diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
  38a62ef..9929c41 100644
  --- a/arch/powerpc/kvm/booke.c
  +++ b/arch/powerpc/kvm/booke.c
  @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
  *vcpu)
   #endif
   }
 
  +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { #ifndef
  +CONFIG_KVM_BOOKE_HV
  +   /* Synchronize guest's desire to get debug interrupts into
  shadow MSR */
  +   vcpu-arch.shadow_msr = ~MSR_DE;
  +   vcpu-arch.shadow_msr |= vcpu-arch.shared-msr  MSR_DE; #endif
  +
  +   /* Force enable debug

Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-02-01 Thread Scott Wood 

On 01/31/2013 06:11:32 PM, Alexander Graf wrote:


On 31.01.2013, at 23:40, Scott Wood wrote:

 On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
 On 31.01.2013, at 20:05, Alexander Graf wrote:
 
  On 31.01.2013, at 19:54, Scott Wood wrote:
 
  On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
  On 31.01.2013, at 19:43, Scott Wood wrote:
  On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
  How about something like this? Then both targets at least  
suck as much :).

 
  I'm not sure that should be the goal...
 
  Thanks to e500mc's awful hardware design, we don't know who  
sets the MSR_DE bit. Once we forced it onto the guest, we have no  
change to know whether the guest also set it or not. We could only  
guess.

 
  MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but  
we still need to set it in the first place.

 
  According to ISA V2.06B, the hypervisor should set DBCR0[EDM]  
to let the guest know that the debug resources are not available, and  
that the value of MSR[DE] is not specified and not modifiable.
  So what would the guest do then to tell the hypervisor that it  
actually wants to know about debug events?

 
  The guest is out of luck, just as if a JTAG were in use.
 
  Hrm.
 
  Can we somehow generalize this out of luck behavior?
 
  Every time we would set or clear an MSR bit in shadow_msr on  
e500v2, we would instead set or clear it in the real MSR. That way  
only e500mc is out of luck, but the code would still be shared.


 I don't follow.  e500v2 is just as out-of-luck.  The mechanism  
simply does not support sharing debug resources.


For e500v2 we have 2 fields

  * MSR as the guest sees it
  * MSR as we execute when the guest runs

Since we know the MSR when the guest sees it, we can decide what to  
do when we get an unhandled debug interrupt.


That's not the same thing as making the real MSR[DE] show up in the  
guest MSR[DE].


There are other problems with sharing -- what happens when both host  
and guest try to write to a particular IAC or DAC?


Also, performance would be pretty awful if the guest has e.g. single  
stepping in DBCR0 enabled but MSR[DE]=0, and the host doesn't care  
about single stepping (but does want debugging enabled in general).


 What do you mean by the real MSR?  The real MSR is shadow_msr,  
and MSR_DE must always be set there if the host is debugging the  
guest.  As for reflecting it into the guest MSR, we could, but I  
don't really see the point.  We're never going to actually send a  
debug exception to the guest when the host owns the debug resources.


Why not? That's the whole point of jumping through user space.


That's still needed for software breakpoints, which don't rely on the  
debug resources.



  1) guest exits with debug interrupt
  2) QEMU gets a debug exit
  3) QEMU checks in its list whether it belongs to its own debug  
points

  4) if not, it reinjects the interrupt into the guest

Step 4 is pretty difficult to do when we don't know whether the guest  
is actually capable of handling debug interrupts at that moment.


Software breakpoints take a Program interrupt rather than a Debug  
interrupt, unless MSR[DE]=1 and DBCR0[TRAP]=1.  If the guest does not  
own debug resources we should always send it to the Program interrupt,  
so MSR[DE] doesn't matter.


 The = ~MSR_DE line is pointless on bookehv, and makes it harder  
to read.  I had to stare at it a while before noticing that you  
initially set is_debug from the guest MSR and that you'd never really  
clear MSR_DE here on bookehv.


Well, I'm mostly bouncing ideas here to find a way to express what  
we're trying to say in a way that someone who hasn't read this email  
thread would still understand what's going on :).


I think it's already straightforward enough if you accept that shared  
debug resources aren't supported, and that we are either in a mode  
where the real MSR[DE] reflects the guest MSR[DE], or a mode where the  
real MSR[DE] is always on in guest mode and the guest MSR[DE] is  
irrelevant.



How about this version?


diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 38a62ef..9929c41 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu  
*vcpu)

 #endif
 }

+static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
+{
+#ifndef CONFIG_KVM_BOOKE_HV
+	/* Synchronize guest's desire to get debug interrupts into  
shadow MSR */

+   vcpu-arch.shadow_msr = ~MSR_DE;
+   vcpu-arch.shadow_msr |= vcpu-arch.shared-msr  MSR_DE;
+#endif
+
+	/* Force enable debug interrupts when user space wants to debug  
*/

+   if (vcpu-guest_debug) {
+#ifdef CONFIG_KVM_BOOKE_HV
+   /*
+		 * Since there is no shadow MSR, sync MSR_DE into the  
guest

+* visible MSR.
+*/
+   vcpu-arch.shared-msr |= MSR_DE;
+#else
+   vcpu-arch.shadow_msr |= MSR_DE;
+#endif
+   }
+}


This shows

Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Alexander Graf 

On 30.01.2013, at 12:12, Bhushan Bharat-R65777 wrote:

 
 
 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On
 Behalf Of Alexander Graf
 Sent: Friday, January 25, 2013 5:44 PM
 To: Bhushan Bharat-R65777
 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Bhushan Bharat-R65777
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 
 On 16.01.2013, at 09:24, Bharat Bhushan wrote:
 
 Allow userspace to inject debug interrupt to guest. QEMU can
 
 s/QEMU/user space.
 
 inject the debug interrupt to guest if it is not able to handle the
 debug interrupt.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 arch/powerpc/kvm/booke.c  |   32 +++-
 arch/powerpc/kvm/e500mc.c |   10 +-
 2 files changed, 40 insertions(+), 2 deletions(-)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
 faa0a0b..547797f 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -133,6 +133,13 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
 *vcpu) #endif }
 
 +#ifdef CONFIG_KVM_BOOKE_HV
 +static int kvmppc_core_pending_debug(struct kvm_vcpu *vcpu) {
 +   return test_bit(BOOKE_IRQPRIO_DEBUG,
 +vcpu-arch.pending_exceptions); } #endif
 +
 /*
 * Helper function for full MSR writes.  No need to call this if only
 * EE/CE/ME/DE/RI are changing.
 @@ -144,7 +151,11 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
 #ifdef CONFIG_KVM_BOOKE_HV
 new_msr |= MSR_GS;
 
 -   if (vcpu-guest_debug)
 +   /*
 +* Set MSR_DE if the hardware debug resources are owned by user-space
 +* and there is no debug interrupt pending for guest to handle.
 
 Why?
 
 QEMU is using the IAC/DAC registers to set hardware breakpoint/watchpoints 
 via debug ioctls. As debug events are enabled/gated by MSR_DE so somehow we 
 need to set MSR_DE on hardware MSR when guest is running in this case.

Reading this 5 times I still have no idea what you're really checking for here. 
Maybe the naming for kvmppc_core_pending_debug is just unnatural? What does 
that function do really?

 
 On bookehv this is how I am controlling the MSR_DE in hardware MSR.  
 
 And why is this whole thing only executed on HV?
 
 On e500v2 we always enable MSR_DE using vcpu-arch.shadow_msr in e500.c
 #ifndef CONFIG_KVM_BOOKE_HV
 -   vcpu-arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
 +   vcpu-arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;

Why? How is e500v2 any different wrt debug? And why wouldn't that work for 
e500mc?


Alex

--
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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf 
 Of
 Alexander Graf
 Sent: Thursday, January 31, 2013 5:34 PM
 To: Bhushan Bharat-R65777
 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 
 On 30.01.2013, at 12:12, Bhushan Bharat-R65777 wrote:
 
 
 
  -Original Message-
  From: kvm-ppc-ow...@vger.kernel.org
  [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
  Sent: Friday, January 25, 2013 5:44 PM
  To: Bhushan Bharat-R65777
  Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Bhushan
  Bharat-R65777
  Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt
  injection to guest
 
 
  On 16.01.2013, at 09:24, Bharat Bhushan wrote:
 
  Allow userspace to inject debug interrupt to guest. QEMU can
 
  s/QEMU/user space.
 
  inject the debug interrupt to guest if it is not able to handle the
  debug interrupt.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  arch/powerpc/kvm/booke.c  |   32 +++-
  arch/powerpc/kvm/e500mc.c |   10 +-
  2 files changed, 40 insertions(+), 2 deletions(-)
 
  diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
  index faa0a0b..547797f 100644
  --- a/arch/powerpc/kvm/booke.c
  +++ b/arch/powerpc/kvm/booke.c
  @@ -133,6 +133,13 @@ static void kvmppc_vcpu_sync_fpu(struct
  kvm_vcpu
  *vcpu) #endif }
 
  +#ifdef CONFIG_KVM_BOOKE_HV
  +static int kvmppc_core_pending_debug(struct kvm_vcpu *vcpu) {
  + return test_bit(BOOKE_IRQPRIO_DEBUG,
  +vcpu-arch.pending_exceptions); } #endif
  +
  /*
  * Helper function for full MSR writes.  No need to call this if
  only
  * EE/CE/ME/DE/RI are changing.
  @@ -144,7 +151,11 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32
  new_msr) #ifdef CONFIG_KVM_BOOKE_HV
new_msr |= MSR_GS;
 
  - if (vcpu-guest_debug)
  + /*
  +  * Set MSR_DE if the hardware debug resources are owned by user-space
  +  * and there is no debug interrupt pending for guest to handle.
 
  Why?
 
  QEMU is using the IAC/DAC registers to set hardware breakpoint/watchpoints 
  via
 debug ioctls. As debug events are enabled/gated by MSR_DE so somehow we need 
 to
 set MSR_DE on hardware MSR when guest is running in this case.
 
 Reading this 5 times I still have no idea what you're really checking for 
 here.
 Maybe the naming for kvmppc_core_pending_debug is just unnatural? What does 
 that
 function do really?
 
 
  On bookehv this is how I am controlling the MSR_DE in hardware MSR.
 
  And why is this whole thing only executed on HV?
 
  On e500v2 we always enable MSR_DE using vcpu-arch.shadow_msr in
  e500.c #ifndef CONFIG_KVM_BOOKE_HV
  -   vcpu-arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
  +   vcpu-arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;


diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index b340a62..1e2d663 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -151,10 +151,14 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)

/*
 * Set MSR_DE if the hardware debug resources are owned by user-space
-* and there is no debug interrupt pending for guest to handle.
 */
-   if (vcpu-guest_debug  !kvmppc_core_pending_debug(vcpu))
+   if (vcpu-guest_debug)
new_msr |= MSR_DE;
+#else
+   if (vcpu-guest_debug)
+   vcpu-arch.shadow_msr |= MSR_DE;
#endif

But do not when I should clear?

 
 Why? How is e500v2 any different wrt debug? And why wouldn't that work for
 e500mc?
 
 
 Alex
 
 --
 To unsubscribe from this list: send the line unsubscribe kvm in the body of 
 a
 message to majord...@vger.kernel.org More majordomo info at
 http://vger.kernel.org/majordomo-info.html


--
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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Scott Wood 

On 01/31/2013 06:04:29 AM, Alexander Graf wrote:


On 30.01.2013, at 12:12, Bhushan Bharat-R65777 wrote:

 On bookehv this is how I am controlling the MSR_DE in hardware MSR.

 And why is this whole thing only executed on HV?

 On e500v2 we always enable MSR_DE using vcpu-arch.shadow_msr in  
e500.c

 #ifndef CONFIG_KVM_BOOKE_HV
 -   vcpu-arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
 +   vcpu-arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;

Why? How is e500v2 any different wrt debug? And why wouldn't that  
work for e500mc?


shadow_msr isn't used at all on bookehv.

-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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Scott Wood 

On 01/31/2013 12:52:41 PM, Alexander Graf wrote:


On 31.01.2013, at 19:43, Scott Wood wrote:

 On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
 How about something like this? Then both targets at least suck as  
much :).


 I'm not sure that should be the goal...

 Thanks to e500mc's awful hardware design, we don't know who sets  
the MSR_DE bit. Once we forced it onto the guest, we have no change  
to know whether the guest also set it or not. We could only guess.


 MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we  
still need to set it in the first place.


 According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to  
let the guest know that the debug resources are not available, and  
that the value of MSR[DE] is not specified and not modifiable.


So what would the guest do then to tell the hypervisor that it  
actually wants to know about debug events?


The guest is out of luck, just as if a JTAG were in use.

-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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Alexander Graf 

On 31.01.2013, at 19:54, Scott Wood wrote:

 On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
 On 31.01.2013, at 19:43, Scott Wood wrote:
  On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
  How about something like this? Then both targets at least suck as much :).
 
  I'm not sure that should be the goal...
 
  Thanks to e500mc's awful hardware design, we don't know who sets the 
  MSR_DE bit. Once we forced it onto the guest, we have no change to know 
  whether the guest also set it or not. We could only guess.
 
  MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we still 
  need to set it in the first place.
 
  According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to let the 
  guest know that the debug resources are not available, and that the value 
  of MSR[DE] is not specified and not modifiable.
 So what would the guest do then to tell the hypervisor that it actually 
 wants to know about debug events?
 
 The guest is out of luck, just as if a JTAG were in use.

Hrm.

Can we somehow generalize this out of luck behavior?

Every time we would set or clear an MSR bit in shadow_msr on e500v2, we would 
instead set or clear it in the real MSR. That way only e500mc is out of luck, 
but the code would still be shared.


Alex

--
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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Alexander Graf 

On 31.01.2013, at 20:05, Alexander Graf wrote:

 
 On 31.01.2013, at 19:54, Scott Wood wrote:
 
 On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
 On 31.01.2013, at 19:43, Scott Wood wrote:
 On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
 How about something like this? Then both targets at least suck as much :).
 
 I'm not sure that should be the goal...
 
 Thanks to e500mc's awful hardware design, we don't know who sets the 
 MSR_DE bit. Once we forced it onto the guest, we have no change to know 
 whether the guest also set it or not. We could only guess.
 
 MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we still 
 need to set it in the first place.
 
 According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to let the 
 guest know that the debug resources are not available, and that the value 
 of MSR[DE] is not specified and not modifiable.
 So what would the guest do then to tell the hypervisor that it actually 
 wants to know about debug events?
 
 The guest is out of luck, just as if a JTAG were in use.
 
 Hrm.
 
 Can we somehow generalize this out of luck behavior?
 
 Every time we would set or clear an MSR bit in shadow_msr on e500v2, we would 
 instead set or clear it in the real MSR. That way only e500mc is out of luck, 
 but the code would still be shared.


Something like this. We could also define a SHADOW_MSR(vcpu) macro to hide the 
glorious details, but I think this way it's easier to understand what's going 
on.


Alex

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 38a62ef..9bdb845 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 #endif
 }
 
+static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
+{
+   u32 is_debug = vcpu-arch.shared-msr  MSR_DE;
+
+   /* Force debug to on in guest space when user space wants to debug */
+   if (vcpu-guest_debug)
+   is_debug = MSR_DE;
+
+#ifdef CONFIG_KVM_BOOKE_HV
+   /*
+* Since there is no shadow MSR, sync MSR_DE into the guest
+* visible MSR.
+*/
+   vcpu-arch.shared-msr = ~MSR_DE;
+   vcpu-arch.shared-msr |= is_debug;
+#endif
+
+#ifndef CONFIG_KVM_BOOKE_HV
+   vcpu-arch.shadow_msr = ~MSR_DE;
+   vcpu-arch.shadow_msr |= is_debug;
+#endif
+}
+
 /*
  * Helper function for full MSR writes.  No need to call this if only
  * EE/CE/ME/DE/RI are changing.
@@ -150,6 +173,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
kvmppc_mmu_msr_notify(vcpu, old_msr);
kvmppc_vcpu_sync_spe(vcpu);
kvmppc_vcpu_sync_fpu(vcpu);
+   kvmppc_vcpu_sync_debug(vcpu);
 }
 
 static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,--
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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Scott Wood 

On 01/31/2013 01:20:39 PM, Alexander Graf wrote:


On 31.01.2013, at 20:05, Alexander Graf wrote:


 On 31.01.2013, at 19:54, Scott Wood wrote:

 On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
 On 31.01.2013, at 19:43, Scott Wood wrote:
 On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
 How about something like this? Then both targets at least suck  
as much :).


 I'm not sure that should be the goal...

 Thanks to e500mc's awful hardware design, we don't know who  
sets the MSR_DE bit. Once we forced it onto the guest, we have no  
change to know whether the guest also set it or not. We could only  
guess.


 MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we  
still need to set it in the first place.


 According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to  
let the guest know that the debug resources are not available, and  
that the value of MSR[DE] is not specified and not modifiable.
 So what would the guest do then to tell the hypervisor that it  
actually wants to know about debug events?


 The guest is out of luck, just as if a JTAG were in use.

 Hrm.

 Can we somehow generalize this out of luck behavior?

 Every time we would set or clear an MSR bit in shadow_msr on  
e500v2, we would instead set or clear it in the real MSR. That way  
only e500mc is out of luck, but the code would still be shared.


I don't follow.  e500v2 is just as out-of-luck.  The mechanism simply  
does not support sharing debug resources.


What do you mean by the real MSR?  The real MSR is shadow_msr, and  
MSR_DE must always be set there if the host is debugging the guest.  As  
for reflecting it into the guest MSR, we could, but I don't really see  
the point.  We're never going to actually send a debug exception to the  
guest when the host owns the debug resources.


Speaking of naming issues, guest_debug is very ambiguous...


diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 38a62ef..9bdb845 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu  
*vcpu)

 #endif
 }

+static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
+{
+   u32 is_debug = vcpu-arch.shared-msr  MSR_DE;
+
+	/* Force debug to on in guest space when user space wants to  
debug */

+   if (vcpu-guest_debug)
+   is_debug = MSR_DE;
+
+#ifdef CONFIG_KVM_BOOKE_HV
+   /*
+* Since there is no shadow MSR, sync MSR_DE into the guest
+* visible MSR.
+*/
+   vcpu-arch.shared-msr = ~MSR_DE;
+   vcpu-arch.shared-msr |= is_debug;
+#endif
+
+#ifndef CONFIG_KVM_BOOKE_HV
+   vcpu-arch.shadow_msr = ~MSR_DE;
+   vcpu-arch.shadow_msr |= is_debug;
+#endif
+}


The = ~MSR_DE line is pointless on bookehv, and makes it harder to  
read.  I had to stare at it a while before noticing that you initially  
set is_debug from the guest MSR and that you'd never really clear  
MSR_DE here on bookehv.


-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 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-31 Thread Alexander Graf 

On 31.01.2013, at 23:40, Scott Wood wrote:

 On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
 On 31.01.2013, at 20:05, Alexander Graf wrote:
 
  On 31.01.2013, at 19:54, Scott Wood wrote:
 
  On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
  On 31.01.2013, at 19:43, Scott Wood wrote:
  On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
  How about something like this? Then both targets at least suck as much 
  :).
 
  I'm not sure that should be the goal...
 
  Thanks to e500mc's awful hardware design, we don't know who sets the 
  MSR_DE bit. Once we forced it onto the guest, we have no change to 
  know whether the guest also set it or not. We could only guess.
 
  MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we still 
  need to set it in the first place.
 
  According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to let 
  the guest know that the debug resources are not available, and that 
  the value of MSR[DE] is not specified and not modifiable.
  So what would the guest do then to tell the hypervisor that it actually 
  wants to know about debug events?
 
  The guest is out of luck, just as if a JTAG were in use.
 
  Hrm.
 
  Can we somehow generalize this out of luck behavior?
 
  Every time we would set or clear an MSR bit in shadow_msr on e500v2, we 
  would instead set or clear it in the real MSR. That way only e500mc is out 
  of luck, but the code would still be shared.
 
 I don't follow.  e500v2 is just as out-of-luck.  The mechanism simply does 
 not support sharing debug resources.

For e500v2 we have 2 fields

  * MSR as the guest sees it
  * MSR as we execute when the guest runs

Since we know the MSR when the guest sees it, we can decide what to do when we 
get an unhandled debug interrupt. We can simulate what hardware would do 
depending on the guest's MSR_DE setting.

For e500mc we only have

  * MSR as the guest sees it and as we execute when the guest runs

Because there is only one field, as soon as we OR MSR_DE into there, we can no 
longer distinguish whether the guest wanted to have MSR_DE enabled or not.

 What do you mean by the real MSR?  The real MSR is shadow_msr, and MSR_DE 
 must always be set there if the host is debugging the guest.  As for 
 reflecting it into the guest MSR, we could, but I don't really see the point. 
  We're never going to actually send a debug exception to the guest when the 
 host owns the debug resources.

Why not? That's the whole point of jumping through user space.

  1) guest exits with debug interrupt
  2) QEMU gets a debug exit
  3) QEMU checks in its list whether it belongs to its own debug points
  4) if not, it reinjects the interrupt into the guest

Step 4 is pretty difficult to do when we don't know whether the guest is 
actually capable of handling debug interrupts at that moment.

 Speaking of naming issues, guest_debug is very ambiguous...

I agree.

 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index 38a62ef..9bdb845 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 #endif
 }
 +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
 +{
 +u32 is_debug = vcpu-arch.shared-msr  MSR_DE;
 +
 +/* Force debug to on in guest space when user space wants to debug */
 +if (vcpu-guest_debug)
 +is_debug = MSR_DE;
 +
 +#ifdef CONFIG_KVM_BOOKE_HV
 +/*
 + * Since there is no shadow MSR, sync MSR_DE into the guest
 + * visible MSR.
 + */
 +vcpu-arch.shared-msr = ~MSR_DE;
 +vcpu-arch.shared-msr |= is_debug;
 +#endif
 +
 +#ifndef CONFIG_KVM_BOOKE_HV
 +vcpu-arch.shadow_msr = ~MSR_DE;
 +vcpu-arch.shadow_msr |= is_debug;
 +#endif
 +}
 
 The = ~MSR_DE line is pointless on bookehv, and makes it harder to read.  
 I had to stare at it a while before noticing that you initially set is_debug 
 from the guest MSR and that you'd never really clear MSR_DE here on bookehv.

Well, I'm mostly bouncing ideas here to find a way to express what we're trying 
to say in a way that someone who hasn't read this email thread would still 
understand what's going on :).

How about this version?


diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 38a62ef..9929c41 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 #endif
 }
 
+static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
+{
+#ifndef CONFIG_KVM_BOOKE_HV
+   /* Synchronize guest's desire to get debug interrupts into shadow MSR */
+   vcpu-arch.shadow_msr = ~MSR_DE;
+   vcpu-arch.shadow_msr |= vcpu-arch.shared-msr  MSR_DE;
+#endif
+
+   /* Force enable debug interrupts when user space wants to debug */
+   if (vcpu-guest_debug) {
+#ifdef CONFIG_KVM_BOOKE_HV
+   /*
+* Since there is no shadow MSR, sync MSR_DE into the guest
+* visible

RE: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-30 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On
 Behalf Of Alexander Graf
 Sent: Friday, January 25, 2013 5:44 PM
 To: Bhushan Bharat-R65777
 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Bhushan Bharat-R65777
 Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to 
 guest
 
 
 On 16.01.2013, at 09:24, Bharat Bhushan wrote:
 
  Allow userspace to inject debug interrupt to guest. QEMU can
 
 s/QEMU/user space.
 
  inject the debug interrupt to guest if it is not able to handle the
  debug interrupt.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  arch/powerpc/kvm/booke.c  |   32 +++-
  arch/powerpc/kvm/e500mc.c |   10 +-
  2 files changed, 40 insertions(+), 2 deletions(-)
 
  diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
  faa0a0b..547797f 100644
  --- a/arch/powerpc/kvm/booke.c
  +++ b/arch/powerpc/kvm/booke.c
  @@ -133,6 +133,13 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
  *vcpu) #endif }
 
  +#ifdef CONFIG_KVM_BOOKE_HV
  +static int kvmppc_core_pending_debug(struct kvm_vcpu *vcpu) {
  +   return test_bit(BOOKE_IRQPRIO_DEBUG,
  +vcpu-arch.pending_exceptions); } #endif
  +
  /*
   * Helper function for full MSR writes.  No need to call this if only
   * EE/CE/ME/DE/RI are changing.
  @@ -144,7 +151,11 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
  #ifdef CONFIG_KVM_BOOKE_HV
  new_msr |= MSR_GS;
 
  -   if (vcpu-guest_debug)
  +   /*
  +* Set MSR_DE if the hardware debug resources are owned by user-space
  +* and there is no debug interrupt pending for guest to handle.
 
 Why?

QEMU is using the IAC/DAC registers to set hardware breakpoint/watchpoints via 
debug ioctls. As debug events are enabled/gated by MSR_DE so somehow we need to 
set MSR_DE on hardware MSR when guest is running in this case.

On bookehv this is how I am controlling the MSR_DE in hardware MSR.  

 And why is this whole thing only executed on HV?

On e500v2 we always enable MSR_DE using vcpu-arch.shadow_msr in e500.c
#ifndef CONFIG_KVM_BOOKE_HV
-   vcpu-arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
+   vcpu-arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
vcpu-arch.shadow_pid = 1;
vcpu-arch.shared-msr = 0;
#endif

Thanks
-Bharat

 
 
 Alex
 
  +*/
  +   if (vcpu-guest_debug  !kvmppc_core_pending_debug(vcpu))
  new_msr |= MSR_DE;
  #endif
 
  @@ -234,6 +245,16 @@ static void kvmppc_core_dequeue_watchdog(struct 
  kvm_vcpu
 *vcpu)
  clear_bit(BOOKE_IRQPRIO_WATCHDOG, vcpu-arch.pending_exceptions);
  }
 
  +static void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
  +{
  +   kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
  +}
  +
  +static void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
  +{
  +   clear_bit(BOOKE_IRQPRIO_DEBUG, vcpu-arch.pending_exceptions);
  +}
  +
  static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 
  srr1)
  {
  #ifdef CONFIG_KVM_BOOKE_HV
  @@ -1278,6 +1299,7 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
  sregs-u.e.dec = kvmppc_get_dec(vcpu, tb);
  sregs-u.e.tb = tb;
  sregs-u.e.vrsave = vcpu-arch.vrsave;
  +   sregs-u.e.dbsr = vcpu-arch.dbsr;
  }
 
  static int set_sregs_base(struct kvm_vcpu *vcpu,
  @@ -1310,6 +1332,14 @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
  update_timer_ints(vcpu);
  }
 
  +   if (sregs-u.e.update_special  KVM_SREGS_E_UPDATE_DBSR) {
  +   vcpu-arch.dbsr = sregs-u.e.dbsr;
  +   if (vcpu-arch.dbsr)
  +   kvmppc_core_queue_debug(vcpu);
  +   else
  +   kvmppc_core_dequeue_debug(vcpu);
  +   }
  +
  return 0;
  }
 
  diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
  index 81abe92..7d90622 100644
  --- a/arch/powerpc/kvm/e500mc.c
  +++ b/arch/powerpc/kvm/e500mc.c
  @@ -208,7 +208,7 @@ void kvmppc_core_get_sregs(struct kvm_vcpu *vcpu, struct
 kvm_sregs *sregs)
  struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
 
  sregs-u.e.features |= KVM_SREGS_E_ARCH206_MMU | KVM_SREGS_E_PM |
  -  KVM_SREGS_E_PC;
  +  KVM_SREGS_E_PC | KVM_SREGS_E_ED;
  sregs-u.e.impl_id = KVM_SREGS_E_IMPL_FSL;
 
  sregs-u.e.impl.fsl.features = 0;
  @@ -216,6 +216,9 @@ void kvmppc_core_get_sregs(struct kvm_vcpu *vcpu, struct
 kvm_sregs *sregs)
  sregs-u.e.impl.fsl.hid0 = vcpu_e500-hid0;
  sregs-u.e.impl.fsl.mcar = vcpu_e500-mcar;
 
  +   sregs-u.e.dsrr0 = vcpu-arch.dsrr0;
  +   sregs-u.e.dsrr1 = vcpu-arch.dsrr1;
  +
  kvmppc_get_sregs_e500_tlb(vcpu, sregs);
 
  sregs-u.e.ivor_high[3] =
  @@ -256,6 +259,11 @@ int kvmppc_core_set_sregs(struct kvm_vcpu *vcpu, struct
 kvm_sregs *sregs)
  sregs-u.e.ivor_high[5];
  }
 
  +   if (sregs-u.e.features  KVM_SREGS_E_ED) {
  +   vcpu-arch.dsrr0 = sregs-u.e.dsrr0

Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

2013-01-25 Thread Alexander Graf 

On 16.01.2013, at 09:24, Bharat Bhushan wrote:

 Allow userspace to inject debug interrupt to guest. QEMU can

s/QEMU/user space.

 inject the debug interrupt to guest if it is not able to handle
 the debug interrupt.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 arch/powerpc/kvm/booke.c  |   32 +++-
 arch/powerpc/kvm/e500mc.c |   10 +-
 2 files changed, 40 insertions(+), 2 deletions(-)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index faa0a0b..547797f 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -133,6 +133,13 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 #endif
 }
 
 +#ifdef CONFIG_KVM_BOOKE_HV
 +static int kvmppc_core_pending_debug(struct kvm_vcpu *vcpu)
 +{
 + return test_bit(BOOKE_IRQPRIO_DEBUG, vcpu-arch.pending_exceptions);
 +}
 +#endif
 +
 /*
  * Helper function for full MSR writes.  No need to call this if only
  * EE/CE/ME/DE/RI are changing.
 @@ -144,7 +151,11 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
 #ifdef CONFIG_KVM_BOOKE_HV
   new_msr |= MSR_GS;
 
 - if (vcpu-guest_debug)
 + /*
 +  * Set MSR_DE if the hardware debug resources are owned by user-space
 +  * and there is no debug interrupt pending for guest to handle.

Why? And why is this whole thing only executed on HV?


Alex

 +  */
 + if (vcpu-guest_debug  !kvmppc_core_pending_debug(vcpu))
   new_msr |= MSR_DE;
 #endif
 
 @@ -234,6 +245,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu 
 *vcpu)
   clear_bit(BOOKE_IRQPRIO_WATCHDOG, vcpu-arch.pending_exceptions);
 }
 
 +static void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
 +{
 + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
 +}
 +
 +static void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
 +{
 + clear_bit(BOOKE_IRQPRIO_DEBUG, vcpu-arch.pending_exceptions);
 +}
 +
 static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1)
 {
 #ifdef CONFIG_KVM_BOOKE_HV
 @@ -1278,6 +1299,7 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
   sregs-u.e.dec = kvmppc_get_dec(vcpu, tb);
   sregs-u.e.tb = tb;
   sregs-u.e.vrsave = vcpu-arch.vrsave;
 + sregs-u.e.dbsr = vcpu-arch.dbsr;
 }
 
 static int set_sregs_base(struct kvm_vcpu *vcpu,
 @@ -1310,6 +1332,14 @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
   update_timer_ints(vcpu);
   }
 
 + if (sregs-u.e.update_special  KVM_SREGS_E_UPDATE_DBSR) {
 + vcpu-arch.dbsr = sregs-u.e.dbsr;
 + if (vcpu-arch.dbsr)
 + kvmppc_core_queue_debug(vcpu);
 + else
 + kvmppc_core_dequeue_debug(vcpu);
 + }
 +
   return 0;
 }
 
 diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
 index 81abe92..7d90622 100644
 --- a/arch/powerpc/kvm/e500mc.c
 +++ b/arch/powerpc/kvm/e500mc.c
 @@ -208,7 +208,7 @@ void kvmppc_core_get_sregs(struct kvm_vcpu *vcpu, struct 
 kvm_sregs *sregs)
   struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
 
   sregs-u.e.features |= KVM_SREGS_E_ARCH206_MMU | KVM_SREGS_E_PM |
 -KVM_SREGS_E_PC;
 +KVM_SREGS_E_PC | KVM_SREGS_E_ED;
   sregs-u.e.impl_id = KVM_SREGS_E_IMPL_FSL;
 
   sregs-u.e.impl.fsl.features = 0;
 @@ -216,6 +216,9 @@ void kvmppc_core_get_sregs(struct kvm_vcpu *vcpu, struct 
 kvm_sregs *sregs)
   sregs-u.e.impl.fsl.hid0 = vcpu_e500-hid0;
   sregs-u.e.impl.fsl.mcar = vcpu_e500-mcar;
 
 + sregs-u.e.dsrr0 = vcpu-arch.dsrr0;
 + sregs-u.e.dsrr1 = vcpu-arch.dsrr1;
 +
   kvmppc_get_sregs_e500_tlb(vcpu, sregs);
 
   sregs-u.e.ivor_high[3] =
 @@ -256,6 +259,11 @@ int kvmppc_core_set_sregs(struct kvm_vcpu *vcpu, struct 
 kvm_sregs *sregs)
   sregs-u.e.ivor_high[5];
   }
 
 + if (sregs-u.e.features  KVM_SREGS_E_ED) {
 + vcpu-arch.dsrr0 = sregs-u.e.dsrr0;
 + vcpu-arch.dsrr1 = sregs-u.e.dsrr1;
 + }
 +
   return kvmppc_set_sregs_ivor(vcpu, sregs);
 }
 
 -- 
 1.7.0.4
 
 
 --
 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

--
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