[Added Paolo, including QEMU userspace patch]

Julien Thierry [off-list ref] writes:

On 03/10/17 18:26, Alex Benn?e wrote:

quoted

Julien Thierry [off-list ref] writes:

quoted

On 03/10/17 17:30, Alex Benn?e wrote:

quoted

Julien Thierry [off-list ref] writes:

quoted

On 03/10/17 15:57, Alex Benn?e wrote:

quoted

Julien Thierry [off-list ref] writes:

quoted

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Does that sound like what you had in mind? Or does it seem better than
the
current patch?

I was thinking to change the skip_instruction function to return an
int, and then call kvm_handle_debug_ss() from skip_instruction, which
would update the kvm_run structure and exit here and then.

Setting up the debug exception from within kvm_skip_instruction seem to
change a bit too much its semantic from arm to arm64. I would find this
easily confusing.

quoted

However, I'm now thinking that this doesn't really work either,
because we could have to emulate a trapped MMIO instruction in user
space, and then it's not clear how to exit with a debug exception at
the same time.

A debug exception at guest exit point is (IIRC) just having the
appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
to exit for MMIO emulation (i.e. the instruction has not run yet) you
shouldn't do that. Exit, emulate and return. We could handle the ioctl
to clear SS in userspace but I guess that gets just as messy.

quoted

quoted

quoted

quoted

So perhaps we should stick with your original approach.

I had not realized that was possible. This makes things more complicated for
avoiding a back and forth with the guest for trapped exceptions. Out of
luck, having the debug flag does look like single stepping would work as
expected for userland MMIOs.

<snip>

This is my currently untested but otherwise simpler solution:

  From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <redacted>
Date: Tue, 3 Oct 2017 17:17:15 +0100
Subject: [PATCH] KVM: arm64: handle single-stepping trapped instructions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If we are using guest debug to single-step the guest we need to ensure
we exit after emulating the instruction. This only affects
instructions emulated by the kernel. If we exit to userspace anyway we
leave it to userspace to work out what to do.

Signed-off-by: Alex Benn?e <redacted>
---
   arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
   1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 7debb74843a0..b197ffb10e96 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c

@@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
   	return arm_exit_handlers[hsr_ec];
   }

+/*
+ * When handling traps we need to ensure exit the guest if we
+ * successfully emulated the instruction while single-stepping. If we
+ * have to exit anyway for userspace emulation then it's up to
+ * userspace to handle the "while SSing case".
+ */
+

I have not tested the code but if it work we also need to do something
similar for MMIOs that are handled by the kernel (without returning to
userland). But it should be pretty similar.

<snip>

Which path do they take to the mmio emulation?

Nevermind, I was mistaken, mmio code will get called by exit_handler
and "handled" will be true if the mmio was handled by KVM. So your
patch already handles this.

There is also the case of GIC CPU inteface accesses being trapped
(which shouldn't be the default behaviour). If the vgic access fails,
we will skip the instruction (in __kvm_vcpu_run in
arch/arm64/kvm/hyp/switch.c) and if we were single stepping we will
single step 2 instructions. But this seems to be a corner case of a
corner case (GIC CPUIF trapped + access failing + single stepping), so
I don't know if we want to take that into account right now?

Yeah looking at the hyp code I did wonder if it warranted the complexity
of adding handling there.

I'm still a bit concerned about the change of semantics for
KVM_EXIT_MMIO with regards to userland (cf. my previous mail). But if
this is deemed to be a reasonable change, the patch seems fine to me.

Have we changed the semantics? A normal un-handled by the kernel IO/MMIO
exit needs to be processed before the single step takes effect. I can't
speak for other userspace but I think for QEMU it is as simple as the
patch bellow. That said it wasn't quite clear where the PC gets updated
in this path - I think the kernel updates the PC before the
KVM_EXIT_MMIO in the same path as the internal handling.

I'd like to test these patches on some real life examples. I tried
tracing over the pl011_write code in a kernel boot but I ran into the
problem of el1_irq's occurring as I tried to step through the guest
kernel. Is this something you've come across? What MMIO accesses have
you been using in your testing?

QEMU Patch bellow:

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <redacted>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after
emulating the access. Otherwise single-stepping across emulated IO
accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode
should be handled there.

Signed-off-by: Alex Benn?e <redacted>
---
 accel/kvm/kvm-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 90c88b517d..85bcb2b0d4 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c

@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                           run->io.direction,
                           run->io.size,
                           run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");

@@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu)
                              run->mmio.data,
                              run->mmio.len,
                              run->mmio.is_write);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
             DPRINTF("irq_window_open\n");
--

2.14.1

Thanks,

--
Alex Benn?e