From: Alex Bennée <hidden> Date: 2014-11-25 16:35:43
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
Signed-off-by: Alex Bennée <redacted>
From: Andrew Jones <hidden> Date: 2014-11-26 16:41:56
On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
quoted hunk
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
Signed-off-by: Alex Bennée <redacted>
same @run comment as other handler header in previous patch
+ *
+ * See: ARM ARM D2.12 for the details. While the host is routing debug
+ * exceptions to it's handlers we have to suppress the ability of the
+ * guest to trigger exceptions.
+ */
+static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+ WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
I'm not sure about this WARN_ON. Is there some scenario you were
thinking of when you put it here? Is there some scenario where this
could trigger so frequently we kill the log buffer?
From: Alex Bennée <hidden> Date: 2014-11-26 18:00:54
Andrew Jones [off-list ref] writes:
On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
quoted
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
Signed-off-by: Alex Bennée <redacted>
same @run comment as other handler header in previous patch
Yeah I think I'll be merging them all together given the comments about
passing syndrome info directly.
quoted
+ *
+ * See: ARM ARM D2.12 for the details. While the host is routing debug
+ * exceptions to it's handlers we have to suppress the ability of the
+ * guest to trigger exceptions.
+ */
+static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+ WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
I'm not sure about this WARN_ON. Is there some scenario you were
thinking of when you put it here? Is there some scenario where this
could trigger so frequently we kill the log buffer?
The main one I had in mind was not suppressing the guest's attempt to
step while guest debugging was running.
<snip>
From: Peter Maydell <hidden> Date: 2014-11-26 19:27:30
On 25 November 2014 at 16:10, Alex Bennée [off-list ref] wrote:
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
A corner case I don't think this patch handles: if the debugger
tries to single step an insn which is emulated by the
hypervisor (because it's a load/store which is trapped and
handled as emulated mmio in userspace) then we won't
correctly update the single-step state machine (and so we'll end
up incorrectly stopping after the following insn rather than
before, I think).
You should be able to achieve this effect by simply always clearing
the guest's PSTATE.SS when you advance the PC to skip the emulated
instruction (cf the comment in the pseudocode SSAdvance() function).
I think we should also be doing this PC advance on return from
userspace's handling of the mmio rather than before we drop back
to userspace as we do now, but I can't remember why I think that.
Christoffer, I don't suppose you recall, do you? I think it was
you I had this conversation with on IRC a month or so back...
-- PMM
From: Christoffer Dall <hidden> Date: 2014-11-30 10:10:20
On Wed, Nov 26, 2014 at 07:27:06PM +0000, Peter Maydell wrote:
On 25 November 2014 at 16:10, Alex Bennée [off-list ref] wrote:
quoted
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
A corner case I don't think this patch handles: if the debugger
tries to single step an insn which is emulated by the
hypervisor (because it's a load/store which is trapped and
handled as emulated mmio in userspace) then we won't
correctly update the single-step state machine (and so we'll end
up incorrectly stopping after the following insn rather than
before, I think).
You should be able to achieve this effect by simply always clearing
the guest's PSTATE.SS when you advance the PC to skip the emulated
instruction (cf the comment in the pseudocode SSAdvance() function).
I think we should also be doing this PC advance on return from
userspace's handling of the mmio rather than before we drop back
to userspace as we do now, but I can't remember why I think that.
Christoffer, I don't suppose you recall, do you? I think it was
you I had this conversation with on IRC a month or so back...
I don't remember clearly, no. Was it not during lunch at LCU we had
this conversation?
In any case, I think it was related to how userspace observes the state
of the CPU, because when you do the MMIO operation emulation in
userspace, currently if you observe the PC though GET_ONE_REG, you'll
see a PC pointing to the next instruction, not the one you're emulating
which is strange.
Not sure what the relation to a guest single-stepping itself was.
-Christoffer
From: Peter Maydell <hidden> Date: 2014-11-30 10:20:43
On 30 November 2014 at 10:10, Christoffer Dall
[off-list ref] wrote:
In any case, I think it was related to how userspace observes the state
of the CPU, because when you do the MMIO operation emulation in
userspace, currently if you observe the PC though GET_ONE_REG, you'll
see a PC pointing to the next instruction, not the one you're emulating
which is strange.
Also if we ever add support for userspace to say "this MMIO should
fault" then we definitely need the PC-advance to happen afterwards,
not before.
Not sure what the relation to a guest single-stepping itself was.
I think it just came up in the course of that discussion, because
single-step handling also needs to perform an action (clear PSTATE.SS)
as part of the "advance over this insn" operation. But I think that
you're right that doing the advance before dropping out to userspace
is no worse for singlestep than it is for any other case.
-- PMM
From: Christoffer Dall <hidden> Date: 2014-11-30 10:21:02
On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
Admittedly this is a corner case, but wouldn't the only really nasty bit
of this be to emulate the guest debug exception?
"we may want" sounds like we're not sure what we'll be doing here. We
probably want to write something like "If the guest is being debugged we
need to set blah blah blah".
+ mrs x1, spsr_el2
+ mrs x2, mdscr_el1
+
+ // See ARM ARM D2.12.3 The software step state machine
+ // If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
+ orr x1, x1, #DBG_SPSR_SS
+ orr x2, x2, #DBG_MDSCR_SS
+ tbnz x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
+ // If we are not doing Single Step we want to prevent the guest doing so
+ // as otherwise we will have to deal with the re-routed exceptions as we
+ // are doing other guest debug related things
+ eor x1, x1, #DBG_SPSR_SS
+ eor x2, x2, #DBG_MDSCR_SS
this really confuses me: so you're setting the SS bits in both
registers, and then if we're not single-stepping the guest, you clear
both bits again?
Wouldn't it be much simper to mask off the bits with a 'bic' and then
setting the bits when needed?
Alternatively, we could manage all these registers from C code and just
save/restore them off the VCPU struct.
quoted hunk
+1:
+ msr spsr_el2, x1
+ msr mdscr_el1, x2
+2:
// Last bits of the 64bit state
pop x2, x3
pop x0, x1
From: Alex Bennée <hidden> Date: 2014-12-01 11:50:17
Christoffer Dall [off-list ref] writes:
On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
quoted
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
Admittedly this is a corner case, but wouldn't the only really nasty bit
of this be to emulate the guest debug exception?
Well yes - currently this is all squashed by ignoring the guest's wishes
while we are debugging (save for SW breakpoints).
+ * result modification of the guest registers needs to take place
+ * after they have been restored in the hyp.S trampoline code.
I don't understand this??
We can't use GET/SET one reg to manipulate the registers we want as
these are the guest visible versions and subject to modification by
userspace. This is why the debugging code makes it's changes after the
guest state has been restored.
quoted
+ */
int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
struct kvm_guest_debug *dbg)
{
@@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, /* Single Step */ if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {- kvm_info("SS requested, not yet implemented\n");- return -EINVAL;+ kvm_info("SS requested\n");+ route_el2 = true; } /* Software Break Points */
is this something that can actually happen or should it be a BUG_ON() -
which may even go away once you're doing hacking on this?
It shouldn't happen. I was treating more like an assert, failure of
which would indicate something has gone wrong somewhere although
generally not worth bringing the kernel down for.
"we may want" sounds like we're not sure what we'll be doing here. We
probably want to write something like "If the guest is being debugged we
need to set blah blah blah".
+ mrs x1, spsr_el2
+ mrs x2, mdscr_el1
+
+ // See ARM ARM D2.12.3 The software step state machine
+ // If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
+ orr x1, x1, #DBG_SPSR_SS
+ orr x2, x2, #DBG_MDSCR_SS
+ tbnz x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
+ // If we are not doing Single Step we want to prevent the guest doing so
+ // as otherwise we will have to deal with the re-routed exceptions as we
+ // are doing other guest debug related things
+ eor x1, x1, #DBG_SPSR_SS
+ eor x2, x2, #DBG_MDSCR_SS
this really confuses me: so you're setting the SS bits in both
registers, and then if we're not single-stepping the guest, you clear
both bits again?
Wouldn't it be much simper to mask off the bits with a 'bic' and then
setting the bits when needed?
Is there a non-vector BIC #imm? I was being frugal with register usage
at this point. The orr/eor steps where just to avoid having too many
branch cases.
Alternatively, we could manage all these registers from C code and just
save/restore them off the VCPU struct.
Yes but this has to be done as we run into the hyp.S code after all
guest registers are confirmed as the changes are on-top of whatever the
guest view is (for the _el1 regs).
Where would you suggest that goes?
quoted
+1:
+ msr spsr_el2, x1
+ msr mdscr_el1, x2
+2:
// Last bits of the 64bit state
pop x2, x3
pop x0, x1
From: Christoffer Dall <hidden> Date: 2014-12-02 13:17:14
On Mon, Dec 01, 2014 at 11:50:14AM +0000, Alex Bennée wrote:
Christoffer Dall [off-list ref] writes:
quoted
On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
quoted
This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.
Admittedly this is a corner case, but wouldn't the only really nasty bit
of this be to emulate the guest debug exception?
Well yes - currently this is all squashed by ignoring the guest's wishes
while we are debugging (save for SW breakpoints).
+ * result modification of the guest registers needs to take place
+ * after they have been restored in the hyp.S trampoline code.
I don't understand this??
We can't use GET/SET one reg to manipulate the registers we want as
these are the guest visible versions and subject to modification by
userspace. This is why the debugging code makes it's changes after the
guest state has been restored.
eh, once you're in the KVM_RUN ioctl, user space can't fiddle your VCPU
regs because you're holding the vcpu mutex, so doing stuff in some
callout from kvm_arch_vcpu_ioctl_run() seems every bid as valid for this
case as doing it in EL2. In fact, the only reason why we're doing
anything in EL2 is when you're accessing state only accessible in EL2,
when you need to write the whole thing in assembly (like the context
switch of GP registers) etc.
If it doesn't have huge performance costs, we should use C-code in EL1
to the furthest extent possible.
quoted
quoted
+ */
int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
struct kvm_guest_debug *dbg)
{
@@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, /* Single Step */ if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {- kvm_info("SS requested, not yet implemented\n");- return -EINVAL;+ kvm_info("SS requested\n");+ route_el2 = true; } /* Software Break Points */
is this something that can actually happen or should it be a BUG_ON() -
which may even go away once you're doing hacking on this?
It shouldn't happen. I was treating more like an assert, failure of
which would indicate something has gone wrong somewhere although
generally not worth bringing the kernel down for.
huh, I guess that's fair enough, but somewhat unconventional for kernel
code. Typically I read WARN_ON (I could be wrong here) as something
that may happen under extreme circumstances (debugging turned on, crazy
low-memory situations etc.), but not as something that catches a bug.
I've seen the argument before that if something that sholdn't ever
happen in the kernel, indeed does happen in the kernel, then that is a
bug, and then you should panic().
So I do feel that this is either a kvm_info()/kvm_err() situation or a
BUG_ON() situation, or nothing at all.
"we may want" sounds like we're not sure what we'll be doing here. We
probably want to write something like "If the guest is being debugged we
need to set blah blah blah".
+ mrs x1, spsr_el2
+ mrs x2, mdscr_el1
+
+ // See ARM ARM D2.12.3 The software step state machine
+ // If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
+ orr x1, x1, #DBG_SPSR_SS
+ orr x2, x2, #DBG_MDSCR_SS
+ tbnz x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
+ // If we are not doing Single Step we want to prevent the guest doing so
+ // as otherwise we will have to deal with the re-routed exceptions as we
+ // are doing other guest debug related things
+ eor x1, x1, #DBG_SPSR_SS
+ eor x2, x2, #DBG_MDSCR_SS
this really confuses me: so you're setting the SS bits in both
registers, and then if we're not single-stepping the guest, you clear
both bits again?
Wouldn't it be much simper to mask off the bits with a 'bic' and then
setting the bits when needed?
Is there a non-vector BIC #imm? I was being frugal with register usage
at this point. The orr/eor steps where just to avoid having too many
branch cases.
there are "bic x3, x3, #1" and such in this very file, so I would guess,
yes.
quoted
Alternatively, we could manage all these registers from C code and just
save/restore them off the VCPU struct.
Yes but this has to be done as we run into the hyp.S code after all
guest registers are confirmed as the changes are on-top of whatever the
guest view is (for the _el1 regs).
Where would you suggest that goes?
As a call-out from the arch-specific KVM_RUN ioctl, call
kvm_setup_guest_debug() or something like that, just like we do to check
if our vmid is valid and to setup the vgic state and so on. Does that
work?
-Christoffer