v2: https://lore.kernel.org/lkml/[email protected]/
v2 -> v3: Add comments, improve the commit messages (Christian, David).
Add R-bs.
Patches that need review: [4/6], [6/6].
v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2: Fix three more issues.
Add selftests (Claudio).
Hi,
I tried to compare the behavior of KVM and TCG by diffing instruction
traces, and found five issues in KVM related to stepping into interrupt
handlers.
Best regards,
Ilya
Ilya Leoshkevich (6):
KVM: s390: interrupt: Fix single-stepping into interrupt handlers
KVM: s390: interrupt: Fix single-stepping into program interrupt
handlers
KVM: s390: interrupt: Fix single-stepping kernel-emulated instructions
KVM: s390: interrupt: Fix single-stepping userspace-emulated
instructions
KVM: s390: interrupt: Fix single-stepping keyless mode exits
KVM: s390: selftests: Add selftest for single-stepping
arch/s390/kvm/intercept.c | 40 ++++-
arch/s390/kvm/interrupt.c | 14 ++
arch/s390/kvm/kvm-s390.c | 27 ++-
tools/testing/selftests/kvm/Makefile | 1 +
.../testing/selftests/kvm/s390x/debug_test.c | 160 ++++++++++++++++++
5 files changed, 230 insertions(+), 12 deletions(-)
create mode 100644 tools/testing/selftests/kvm/s390x/debug_test.c
--
2.41.0
Test different variations of single-stepping into interrupts:
- SVC and PGM interrupts;
- Interrupts generated by ISKE;
- Interrupts generated by instructions emulated by KVM;
- Interrupts generated by instructions emulated by userspace.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
tools/testing/selftests/kvm/Makefile | 1 +
.../testing/selftests/kvm/s390x/debug_test.c | 160 ++++++++++++++++++
2 files changed, 161 insertions(+)
create mode 100644 tools/testing/selftests/kvm/s390x/debug_test.c
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index c692cc86e7da..f3306eaa7031 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -166,6 +166,7 @@ TEST_GEN_PROGS_s390x += s390x/resets
TEST_GEN_PROGS_s390x += s390x/sync_regs_test
TEST_GEN_PROGS_s390x += s390x/tprot
TEST_GEN_PROGS_s390x += s390x/cmma_test
+TEST_GEN_PROGS_s390x += s390x/debug_test
TEST_GEN_PROGS_s390x += demand_paging_test
TEST_GEN_PROGS_s390x += dirty_log_test
TEST_GEN_PROGS_s390x += kvm_create_max_vcpus
diff --git a/tools/testing/selftests/kvm/s390x/debug_test.c b/tools/testing/selftests/kvm/s390x/debug_test.c
new file mode 100644
index 000000000000..a8fa9fe93b3c
--- /dev/null
+++ b/tools/testing/selftests/kvm/s390x/debug_test.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Test KVM debugging features. */
+#include "kvm_util.h"
+#include "test_util.h"
+
+#include <linux/kvm.h>
+
+#define __LC_SVC_NEW_PSW 0x1c0
+#define __LC_PGM_NEW_PSW 0x1d0
+#define ICPT_INSTRUCTION 0x04
+#define IPA0_DIAG 0x8300
+#define PGM_SPECIFICATION 0x06
+
+/* Common code for testing single-stepping interruptions. */
+extern char int_handler[];
+asm("int_handler:\n"
+ "j .\n");
+
+static struct kvm_vm *test_step_int_1(struct kvm_vcpu **vcpu, void *guest_code,
+ size_t new_psw_off, uint64_t *new_psw)
+{
+ struct kvm_guest_debug debug = {};
+ struct kvm_regs regs;
+ struct kvm_vm *vm;
+ char *lowcore;
+
+ vm = vm_create_with_one_vcpu(vcpu, guest_code);
+ lowcore = addr_gpa2hva(vm, 0);
+ new_psw[0] = (*vcpu)->run->psw_mask;
+ new_psw[1] = (uint64_t)int_handler;
+ memcpy(lowcore + new_psw_off, new_psw, 16);
+ vcpu_regs_get(*vcpu, ®s);
+ regs.gprs[2] = -1;
+ vcpu_regs_set(*vcpu, ®s);
+ debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+ vcpu_guest_debug_set(*vcpu, &debug);
+ vcpu_run(*vcpu);
+
+ return vm;
+}
+
+static void test_step_int(void *guest_code, size_t new_psw_off)
+{
+ struct kvm_vcpu *vcpu;
+ uint64_t new_psw[2];
+ struct kvm_vm *vm;
+
+ vm = test_step_int_1(&vcpu, guest_code, new_psw_off, new_psw);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);
+ ASSERT_EQ(vcpu->run->psw_mask, new_psw[0]);
+ ASSERT_EQ(vcpu->run->psw_addr, new_psw[1]);
+ kvm_vm_free(vm);
+}
+
+/* Test single-stepping "boring" program interruptions. */
+extern char test_step_pgm_guest_code[];
+asm("test_step_pgm_guest_code:\n"
+ ".insn rr,0x1d00,%r1,%r0 /* dr %r1,%r0 */\n"
+ "j .\n");
+
+static void test_step_pgm(void)
+{
+ test_step_int(test_step_pgm_guest_code, __LC_PGM_NEW_PSW);
+}
+
+/*
+ * Test single-stepping program interruptions caused by DIAG.
+ * Userspace emulation must not interfere with single-stepping.
+ */
+extern char test_step_pgm_diag_guest_code[];
+asm("test_step_pgm_diag_guest_code:\n"
+ "diag %r0,%r0,0\n"
+ "j .\n");
+
+static void test_step_pgm_diag(void)
+{
+ struct kvm_s390_irq irq = {
+ .type = KVM_S390_PROGRAM_INT,
+ .u.pgm.code = PGM_SPECIFICATION,
+ };
+ struct kvm_vcpu *vcpu;
+ uint64_t new_psw[2];
+ struct kvm_vm *vm;
+
+ vm = test_step_int_1(&vcpu, test_step_pgm_diag_guest_code,
+ __LC_PGM_NEW_PSW, new_psw);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_S390_SIEIC);
+ ASSERT_EQ(vcpu->run->s390_sieic.icptcode, ICPT_INSTRUCTION);
+ ASSERT_EQ(vcpu->run->s390_sieic.ipa & 0xff00, IPA0_DIAG);
+ vcpu_ioctl(vcpu, KVM_S390_IRQ, &irq);
+ vcpu_run(vcpu);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);
+ ASSERT_EQ(vcpu->run->psw_mask, new_psw[0]);
+ ASSERT_EQ(vcpu->run->psw_addr, new_psw[1]);
+ kvm_vm_free(vm);
+}
+
+/*
+ * Test single-stepping program interruptions caused by ISKE.
+ * CPUSTAT_KSS handling must not interfere with single-stepping.
+ */
+extern char test_step_pgm_iske_guest_code[];
+asm("test_step_pgm_iske_guest_code:\n"
+ "iske %r2,%r2\n"
+ "j .\n");
+
+static void test_step_pgm_iske(void)
+{
+ test_step_int(test_step_pgm_iske_guest_code, __LC_PGM_NEW_PSW);
+}
+
+/*
+ * Test single-stepping program interruptions caused by LCTL.
+ * KVM emulation must not interfere with single-stepping.
+ */
+extern char test_step_pgm_lctl_guest_code[];
+asm("test_step_pgm_lctl_guest_code:\n"
+ "lctl %c0,%c0,1\n"
+ "j .\n");
+
+static void test_step_pgm_lctl(void)
+{
+ test_step_int(test_step_pgm_lctl_guest_code, __LC_PGM_NEW_PSW);
+}
+
+/* Test single-stepping supervisor-call interruptions. */
+extern char test_step_svc_guest_code[];
+asm("test_step_svc_guest_code:\n"
+ "svc 0\n"
+ "j .\n");
+
+static void test_step_svc(void)
+{
+ test_step_int(test_step_svc_guest_code, __LC_SVC_NEW_PSW);
+}
+
+/* Run all tests above. */
+static struct testdef {
+ const char *name;
+ void (*test)(void);
+} testlist[] = {
+ { "single-step pgm", test_step_pgm },
+ { "single-step pgm caused by diag", test_step_pgm_diag },
+ { "single-step pgm caused by iske", test_step_pgm_iske },
+ { "single-step pgm caused by lctl", test_step_pgm_lctl },
+ { "single-step svc", test_step_svc },
+};
+
+int main(int argc, char *argv[])
+{
+ int idx;
+
+ ksft_print_header();
+ ksft_set_plan(ARRAY_SIZE(testlist));
+ for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
+ testlist[idx].test();
+ ksft_test_result_pass("%s\n", testlist[idx].name);
+ }
+ ksft_finished();
+}
--
2.41.0
Single-stepping a userspace-emulated instruction that generates an
interrupt causes GDB to land on the instruction following it instead of
the respective interrupt handler.
The reason is that after arranging a KVM_EXIT_S390_SIEIC exit,
kvm_handle_sie_intercept() calls kvm_s390_handle_per_ifetch_icpt(),
which sets KVM_GUESTDBG_EXIT_PENDING. This bit, however, is not
processed immediately, but rather persists until the next ioctl(),
causing a spurious single-step exit.
Fix by clearing this bit in ioctl().
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/kvm/kvm-s390.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0c6333b108ba..e6511608280c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5383,6 +5383,7 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
{
struct kvm_vcpu *vcpu = filp->private_data;
void __user *argp = (void __user *)arg;
+ int rc;
switch (ioctl) {
case KVM_S390_IRQ: {
@@ -5390,7 +5391,8 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
if (copy_from_user(&s390irq, argp, sizeof(s390irq)))
return -EFAULT;
- return kvm_s390_inject_vcpu(vcpu, &s390irq);
+ rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
+ break;
}
case KVM_S390_INTERRUPT: {
struct kvm_s390_interrupt s390int;
@@ -5400,10 +5402,25 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
return -EFAULT;
if (s390int_to_s390irq(&s390int, &s390irq))
return -EINVAL;
- return kvm_s390_inject_vcpu(vcpu, &s390irq);
+ rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
+ break;
}
+ default:
+ rc = -ENOIOCTLCMD;
+ break;
}
- return -ENOIOCTLCMD;
+
+ /*
+ * To simplify single stepping of userspace-emulated instructions,
+ * KVM_EXIT_S390_SIEIC exit sets KVM_GUESTDBG_EXIT_PENDING (see
+ * should_handle_per_ifetch()). However, if userspace emulation injects
+ * an interrupt, it needs to be cleared, so that KVM_EXIT_DEBUG happens
+ * after (and not before) the interrupt delivery.
+ */
+ if (!rc)
+ vcpu->guest_debug &= ~KVM_GUESTDBG_EXIT_PENDING;
+
+ return rc;
}
static int kvm_s390_handle_pv_vcpu_dump(struct kvm_vcpu *vcpu,
--
2.41.0
kvm_s390_skey_check_enable() does not emulate any instructions, rather,
it clears CPUSTAT_KSS and arranges the instruction that caused the exit
(e.g., ISKE, SSKE, RRBE or LPSWE with a keyed PSW) to run again.
Therefore, skip the PER check and let the instruction execution happen.
Otherwise, a debugger will see two single-step events on the same
instruction.
Reviewed-by: Christian Borntraeger <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/kvm/intercept.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index d2f7940c5d03..62b3b956c843 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -630,8 +630,8 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
rc = handle_partial_execution(vcpu);
break;
case ICPT_KSS:
- rc = kvm_s390_skey_check_enable(vcpu);
- break;
+ /* Instruction will be redriven, skip the PER check. */
+ return kvm_s390_skey_check_enable(vcpu);
case ICPT_MCHKREQ:
case ICPT_INT_ENABLE:
/*
--
2.41.0
After single-stepping an instruction that generates an interrupt, GDB
ends up on the second instruction of the respective interrupt handler.
The reason is that vcpu_pre_run() manually delivers the interrupt, and
then __vcpu_run() runs the first handler instruction using the
CPUSTAT_P flag. This causes a KVM_SINGLESTEP exit on the second handler
instruction.
Fix by delaying the KVM_SINGLESTEP exit until after the manual
interrupt delivery.
Acked-by: David Hildenbrand <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/kvm/interrupt.c | 14 ++++++++++++++
arch/s390/kvm/kvm-s390.c | 4 ++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 9bd0a873f3b1..85e39f472bb4 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1392,6 +1392,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
{
struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
int rc = 0;
+ bool delivered = false;
unsigned long irq_type;
unsigned long irqs;
@@ -1465,6 +1466,19 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
WARN_ONCE(1, "Unknown pending irq type %ld", irq_type);
clear_bit(irq_type, &li->pending_irqs);
}
+ delivered |= !rc;
+ }
+
+ /*
+ * We delivered at least one interrupt and modified the PC. Force a
+ * singlestep event now.
+ */
+ if (delivered && guestdbg_sstep_enabled(vcpu)) {
+ struct kvm_debug_exit_arch *debug_exit = &vcpu->run->debug.arch;
+
+ debug_exit->addr = vcpu->arch.sie_block->gpsw.addr;
+ debug_exit->type = KVM_SINGLESTEP;
+ vcpu->guest_debug |= KVM_GUESTDBG_EXIT_PENDING;
}
set_intercept_indicators(vcpu);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d1e768bcfe1d..0c6333b108ba 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4611,7 +4611,7 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
if (!kvm_is_ucontrol(vcpu->kvm)) {
rc = kvm_s390_deliver_pending_interrupts(vcpu);
- if (rc)
+ if (rc || guestdbg_exit_pending(vcpu))
return rc;
}
@@ -4738,7 +4738,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
do {
rc = vcpu_pre_run(vcpu);
- if (rc)
+ if (rc || guestdbg_exit_pending(vcpu))
break;
kvm_vcpu_srcu_read_unlock(vcpu);
--
2.41.0
Currently, after single-stepping an instruction that generates a
specification exception, GDB ends up on the instruction immediately
following it.
The reason is that vcpu_post_run() injects the interrupt and sets
KVM_GUESTDBG_EXIT_PENDING, causing a KVM_SINGLESTEP exit. The
interrupt is not delivered, however, therefore userspace sees the
address of the next instruction.
Fix by letting the __vcpu_run() loop go into the next iteration,
where vcpu_pre_run() delivers the interrupt and sets
KVM_GUESTDBG_EXIT_PENDING.
Reviewed-by: David Hildenbrand <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/kvm/intercept.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 954d39adf85c..7cdd927541b0 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -226,7 +226,22 @@ static int handle_itdb(struct kvm_vcpu *vcpu)
return 0;
}
-#define per_event(vcpu) (vcpu->arch.sie_block->iprcc & PGM_PER)
+static bool should_handle_per_event(const struct kvm_vcpu *vcpu)
+{
+ if (!guestdbg_enabled(vcpu))
+ return false;
+ if (!(vcpu->arch.sie_block->iprcc & PGM_PER))
+ return false;
+ if (guestdbg_sstep_enabled(vcpu) &&
+ vcpu->arch.sie_block->iprcc != PGM_PER) {
+ /*
+ * __vcpu_run() will exit after delivering the concurrently
+ * indicated condition.
+ */
+ return false;
+ }
+ return true;
+}
static int handle_prog(struct kvm_vcpu *vcpu)
{
@@ -242,7 +257,7 @@ static int handle_prog(struct kvm_vcpu *vcpu)
if (kvm_s390_pv_cpu_is_protected(vcpu))
return -EOPNOTSUPP;
- if (guestdbg_enabled(vcpu) && per_event(vcpu)) {
+ if (should_handle_per_event(vcpu)) {
rc = kvm_s390_handle_per_event(vcpu);
if (rc)
return rc;
--
2.41.0
Single-stepping a kernel-emulated instruction that generates an
interrupt causes GDB to land on the instruction following it instead of
the respective interrupt handler.
The reason is that kvm_handle_sie_intercept(), after injecting the
interrupt, also processes the PER event and arranges a KVM_SINGLESTEP
exit. The interrupt is not yet delivered, however, so the userspace
sees the next instruction.
Fix by avoiding the KVM_SINGLESTEP exit when there is a pending
interrupt. The next __vcpu_run() loop iteration will arrange a
KVM_SINGLESTEP exit after delivering the interrupt.
Reviewed-by: David Hildenbrand <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/kvm/intercept.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 7cdd927541b0..d2f7940c5d03 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -583,6 +583,19 @@ static int handle_pv_notification(struct kvm_vcpu *vcpu)
return handle_instruction(vcpu);
}
+static bool should_handle_per_ifetch(const struct kvm_vcpu *vcpu, int rc)
+{
+ /* Process PER, also if the instruction is processed in user space. */
+ if (!(vcpu->arch.sie_block->icptstatus & 0x02))
+ return false;
+ if (rc != 0 && rc != -EOPNOTSUPP)
+ return false;
+ if (guestdbg_sstep_enabled(vcpu) && vcpu->arch.local_int.pending_irqs)
+ /* __vcpu_run() will exit after delivering the interrupt. */
+ return false;
+ return true;
+}
+
int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
{
int rc, per_rc = 0;
@@ -645,9 +658,7 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
return -EOPNOTSUPP;
}
- /* process PER, also if the instruction is processed in user space */
- if (vcpu->arch.sie_block->icptstatus & 0x02 &&
- (!rc || rc == -EOPNOTSUPP))
+ if (should_handle_per_ifetch(vcpu, rc))
per_rc = kvm_s390_handle_per_ifetch_icpt(vcpu);
return per_rc ? per_rc : rc;
}
--
2.41.0
On 24.07.23 11:44, Ilya Leoshkevich wrote:
> Single-stepping a userspace-emulated instruction that generates an
> interrupt causes GDB to land on the instruction following it instead of
> the respective interrupt handler.
>
> The reason is that after arranging a KVM_EXIT_S390_SIEIC exit,
> kvm_handle_sie_intercept() calls kvm_s390_handle_per_ifetch_icpt(),
> which sets KVM_GUESTDBG_EXIT_PENDING. This bit, however, is not
> processed immediately, but rather persists until the next ioctl(),
> causing a spurious single-step exit.
>
> Fix by clearing this bit in ioctl().
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
> ---
> arch/s390/kvm/kvm-s390.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0c6333b108ba..e6511608280c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5383,6 +5383,7 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> {
> struct kvm_vcpu *vcpu = filp->private_data;
> void __user *argp = (void __user *)arg;
> + int rc;
>
> switch (ioctl) {
> case KVM_S390_IRQ: {
> @@ -5390,7 +5391,8 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
>
> if (copy_from_user(&s390irq, argp, sizeof(s390irq)))
> return -EFAULT;
> - return kvm_s390_inject_vcpu(vcpu, &s390irq);
> + rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> + break;
> }
> case KVM_S390_INTERRUPT: {
> struct kvm_s390_interrupt s390int;
> @@ -5400,10 +5402,25 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> return -EFAULT;
> if (s390int_to_s390irq(&s390int, &s390irq))
> return -EINVAL;
> - return kvm_s390_inject_vcpu(vcpu, &s390irq);
> + rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> + break;
> }
> + default:
> + rc = -ENOIOCTLCMD;
> + break;
> }
> - return -ENOIOCTLCMD;
> +
> + /*
> + * To simplify single stepping of userspace-emulated instructions,
> + * KVM_EXIT_S390_SIEIC exit sets KVM_GUESTDBG_EXIT_PENDING (see
> + * should_handle_per_ifetch()). However, if userspace emulation injects
> + * an interrupt, it needs to be cleared, so that KVM_EXIT_DEBUG happens
> + * after (and not before) the interrupt delivery.
> + */
> + if (!rc)
> + vcpu->guest_debug &= ~KVM_GUESTDBG_EXIT_PENDING;
> +
> + return rc;
> }
>
> static int kvm_s390_handle_pv_vcpu_dump(struct kvm_vcpu *vcpu,
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On Mon, 24 Jul 2023 11:44:10 +0200
Ilya Leoshkevich <[email protected]> wrote:
> Single-stepping a userspace-emulated instruction that generates an
> interrupt causes GDB to land on the instruction following it instead of
> the respective interrupt handler.
>
> The reason is that after arranging a KVM_EXIT_S390_SIEIC exit,
> kvm_handle_sie_intercept() calls kvm_s390_handle_per_ifetch_icpt(),
> which sets KVM_GUESTDBG_EXIT_PENDING. This bit, however, is not
> processed immediately, but rather persists until the next ioctl(),
> causing a spurious single-step exit.
>
> Fix by clearing this bit in ioctl().
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> arch/s390/kvm/kvm-s390.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0c6333b108ba..e6511608280c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5383,6 +5383,7 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> {
> struct kvm_vcpu *vcpu = filp->private_data;
> void __user *argp = (void __user *)arg;
> + int rc;
>
> switch (ioctl) {
> case KVM_S390_IRQ: {
> @@ -5390,7 +5391,8 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
>
> if (copy_from_user(&s390irq, argp, sizeof(s390irq)))
> return -EFAULT;
> - return kvm_s390_inject_vcpu(vcpu, &s390irq);
> + rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> + break;
> }
> case KVM_S390_INTERRUPT: {
> struct kvm_s390_interrupt s390int;
> @@ -5400,10 +5402,25 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> return -EFAULT;
> if (s390int_to_s390irq(&s390int, &s390irq))
> return -EINVAL;
> - return kvm_s390_inject_vcpu(vcpu, &s390irq);
> + rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> + break;
> }
> + default:
> + rc = -ENOIOCTLCMD;
> + break;
> }
> - return -ENOIOCTLCMD;
> +
> + /*
> + * To simplify single stepping of userspace-emulated instructions,
> + * KVM_EXIT_S390_SIEIC exit sets KVM_GUESTDBG_EXIT_PENDING (see
> + * should_handle_per_ifetch()). However, if userspace emulation injects
> + * an interrupt, it needs to be cleared, so that KVM_EXIT_DEBUG happens
> + * after (and not before) the interrupt delivery.
> + */
> + if (!rc)
> + vcpu->guest_debug &= ~KVM_GUESTDBG_EXIT_PENDING;
> +
> + return rc;
> }
>
> static int kvm_s390_handle_pv_vcpu_dump(struct kvm_vcpu *vcpu,
On Mon, 24 Jul 2023 11:44:12 +0200
Ilya Leoshkevich <[email protected]> wrote:
> Test different variations of single-stepping into interrupts:
>
> - SVC and PGM interrupts;
> - Interrupts generated by ISKE;
> - Interrupts generated by instructions emulated by KVM;
> - Interrupts generated by instructions emulated by userspace.
thank you for writing this selftest!
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> tools/testing/selftests/kvm/Makefile | 1 +
> .../testing/selftests/kvm/s390x/debug_test.c | 160 ++++++++++++++++++
> 2 files changed, 161 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/s390x/debug_test.c
>
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index c692cc86e7da..f3306eaa7031 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -166,6 +166,7 @@ TEST_GEN_PROGS_s390x += s390x/resets
> TEST_GEN_PROGS_s390x += s390x/sync_regs_test
> TEST_GEN_PROGS_s390x += s390x/tprot
> TEST_GEN_PROGS_s390x += s390x/cmma_test
> +TEST_GEN_PROGS_s390x += s390x/debug_test
> TEST_GEN_PROGS_s390x += demand_paging_test
> TEST_GEN_PROGS_s390x += dirty_log_test
> TEST_GEN_PROGS_s390x += kvm_create_max_vcpus
> diff --git a/tools/testing/selftests/kvm/s390x/debug_test.c b/tools/testing/selftests/kvm/s390x/debug_test.c
> new file mode 100644
> index 000000000000..a8fa9fe93b3c
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/s390x/debug_test.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Test KVM debugging features. */
> +#include "kvm_util.h"
> +#include "test_util.h"
> +
> +#include <linux/kvm.h>
> +
> +#define __LC_SVC_NEW_PSW 0x1c0
> +#define __LC_PGM_NEW_PSW 0x1d0
> +#define ICPT_INSTRUCTION 0x04
> +#define IPA0_DIAG 0x8300
> +#define PGM_SPECIFICATION 0x06
> +
> +/* Common code for testing single-stepping interruptions. */
> +extern char int_handler[];
> +asm("int_handler:\n"
> + "j .\n");
> +
> +static struct kvm_vm *test_step_int_1(struct kvm_vcpu **vcpu, void *guest_code,
> + size_t new_psw_off, uint64_t *new_psw)
> +{
> + struct kvm_guest_debug debug = {};
> + struct kvm_regs regs;
> + struct kvm_vm *vm;
> + char *lowcore;
> +
> + vm = vm_create_with_one_vcpu(vcpu, guest_code);
> + lowcore = addr_gpa2hva(vm, 0);
> + new_psw[0] = (*vcpu)->run->psw_mask;
> + new_psw[1] = (uint64_t)int_handler;
> + memcpy(lowcore + new_psw_off, new_psw, 16);
> + vcpu_regs_get(*vcpu, ®s);
> + regs.gprs[2] = -1;
> + vcpu_regs_set(*vcpu, ®s);
> + debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
> + vcpu_guest_debug_set(*vcpu, &debug);
> + vcpu_run(*vcpu);
> +
> + return vm;
> +}
> +
> +static void test_step_int(void *guest_code, size_t new_psw_off)
> +{
> + struct kvm_vcpu *vcpu;
> + uint64_t new_psw[2];
> + struct kvm_vm *vm;
> +
> + vm = test_step_int_1(&vcpu, guest_code, new_psw_off, new_psw);
> + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);
> + ASSERT_EQ(vcpu->run->psw_mask, new_psw[0]);
> + ASSERT_EQ(vcpu->run->psw_addr, new_psw[1]);
> + kvm_vm_free(vm);
> +}
> +
> +/* Test single-stepping "boring" program interruptions. */
> +extern char test_step_pgm_guest_code[];
> +asm("test_step_pgm_guest_code:\n"
> + ".insn rr,0x1d00,%r1,%r0 /* dr %r1,%r0 */\n"
> + "j .\n");
> +
> +static void test_step_pgm(void)
> +{
> + test_step_int(test_step_pgm_guest_code, __LC_PGM_NEW_PSW);
> +}
> +
> +/*
> + * Test single-stepping program interruptions caused by DIAG.
> + * Userspace emulation must not interfere with single-stepping.
> + */
> +extern char test_step_pgm_diag_guest_code[];
> +asm("test_step_pgm_diag_guest_code:\n"
> + "diag %r0,%r0,0\n"
> + "j .\n");
> +
> +static void test_step_pgm_diag(void)
> +{
> + struct kvm_s390_irq irq = {
> + .type = KVM_S390_PROGRAM_INT,
> + .u.pgm.code = PGM_SPECIFICATION,
> + };
> + struct kvm_vcpu *vcpu;
> + uint64_t new_psw[2];
> + struct kvm_vm *vm;
> +
> + vm = test_step_int_1(&vcpu, test_step_pgm_diag_guest_code,
> + __LC_PGM_NEW_PSW, new_psw);
> + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_S390_SIEIC);
> + ASSERT_EQ(vcpu->run->s390_sieic.icptcode, ICPT_INSTRUCTION);
> + ASSERT_EQ(vcpu->run->s390_sieic.ipa & 0xff00, IPA0_DIAG);
> + vcpu_ioctl(vcpu, KVM_S390_IRQ, &irq);
> + vcpu_run(vcpu);
> + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);
> + ASSERT_EQ(vcpu->run->psw_mask, new_psw[0]);
> + ASSERT_EQ(vcpu->run->psw_addr, new_psw[1]);
> + kvm_vm_free(vm);
> +}
> +
> +/*
> + * Test single-stepping program interruptions caused by ISKE.
> + * CPUSTAT_KSS handling must not interfere with single-stepping.
> + */
> +extern char test_step_pgm_iske_guest_code[];
> +asm("test_step_pgm_iske_guest_code:\n"
> + "iske %r2,%r2\n"
> + "j .\n");
> +
> +static void test_step_pgm_iske(void)
> +{
> + test_step_int(test_step_pgm_iske_guest_code, __LC_PGM_NEW_PSW);
> +}
> +
> +/*
> + * Test single-stepping program interruptions caused by LCTL.
> + * KVM emulation must not interfere with single-stepping.
> + */
> +extern char test_step_pgm_lctl_guest_code[];
> +asm("test_step_pgm_lctl_guest_code:\n"
> + "lctl %c0,%c0,1\n"
> + "j .\n");
> +
> +static void test_step_pgm_lctl(void)
> +{
> + test_step_int(test_step_pgm_lctl_guest_code, __LC_PGM_NEW_PSW);
> +}
> +
> +/* Test single-stepping supervisor-call interruptions. */
> +extern char test_step_svc_guest_code[];
> +asm("test_step_svc_guest_code:\n"
> + "svc 0\n"
> + "j .\n");
> +
> +static void test_step_svc(void)
> +{
> + test_step_int(test_step_svc_guest_code, __LC_SVC_NEW_PSW);
> +}
> +
> +/* Run all tests above. */
> +static struct testdef {
> + const char *name;
> + void (*test)(void);
> +} testlist[] = {
> + { "single-step pgm", test_step_pgm },
> + { "single-step pgm caused by diag", test_step_pgm_diag },
> + { "single-step pgm caused by iske", test_step_pgm_iske },
> + { "single-step pgm caused by lctl", test_step_pgm_lctl },
> + { "single-step svc", test_step_svc },
> +};
> +
> +int main(int argc, char *argv[])
> +{
> + int idx;
> +
> + ksft_print_header();
> + ksft_set_plan(ARRAY_SIZE(testlist));
> + for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
> + testlist[idx].test();
> + ksft_test_result_pass("%s\n", testlist[idx].name);
> + }
> + ksft_finished();
> +}
On Mon, 24 Jul 2023 11:44:09 +0200
Ilya Leoshkevich <[email protected]> wrote:
> Single-stepping a kernel-emulated instruction that generates an
> interrupt causes GDB to land on the instruction following it instead of
> the respective interrupt handler.
>
> The reason is that kvm_handle_sie_intercept(), after injecting the
> interrupt, also processes the PER event and arranges a KVM_SINGLESTEP
> exit. The interrupt is not yet delivered, however, so the userspace
> sees the next instruction.
>
> Fix by avoiding the KVM_SINGLESTEP exit when there is a pending
> interrupt. The next __vcpu_run() loop iteration will arrange a
> KVM_SINGLESTEP exit after delivering the interrupt.
>
> Reviewed-by: David Hildenbrand <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
> ---
> arch/s390/kvm/intercept.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 7cdd927541b0..d2f7940c5d03 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -583,6 +583,19 @@ static int handle_pv_notification(struct kvm_vcpu *vcpu)
> return handle_instruction(vcpu);
> }
>
> +static bool should_handle_per_ifetch(const struct kvm_vcpu *vcpu, int rc)
> +{
> + /* Process PER, also if the instruction is processed in user space. */
> + if (!(vcpu->arch.sie_block->icptstatus & 0x02))
> + return false;
> + if (rc != 0 && rc != -EOPNOTSUPP)
> + return false;
> + if (guestdbg_sstep_enabled(vcpu) && vcpu->arch.local_int.pending_irqs)
> + /* __vcpu_run() will exit after delivering the interrupt. */
> + return false;
> + return true;
> +}
> +
> int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
> {
> int rc, per_rc = 0;
> @@ -645,9 +658,7 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
> return -EOPNOTSUPP;
> }
>
> - /* process PER, also if the instruction is processed in user space */
> - if (vcpu->arch.sie_block->icptstatus & 0x02 &&
> - (!rc || rc == -EOPNOTSUPP))
> + if (should_handle_per_ifetch(vcpu, rc))
> per_rc = kvm_s390_handle_per_ifetch_icpt(vcpu);
> return per_rc ? per_rc : rc;
> }
On Mon, 24 Jul 2023 11:44:07 +0200
Ilya Leoshkevich <[email protected]> wrote:
> After single-stepping an instruction that generates an interrupt, GDB
> ends up on the second instruction of the respective interrupt handler.
>
> The reason is that vcpu_pre_run() manually delivers the interrupt, and
> then __vcpu_run() runs the first handler instruction using the
> CPUSTAT_P flag. This causes a KVM_SINGLESTEP exit on the second handler
> instruction.
>
> Fix by delaying the KVM_SINGLESTEP exit until after the manual
> interrupt delivery.
>
> Acked-by: David Hildenbrand <[email protected]>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> arch/s390/kvm/interrupt.c | 14 ++++++++++++++
> arch/s390/kvm/kvm-s390.c | 4 ++--
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 9bd0a873f3b1..85e39f472bb4 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1392,6 +1392,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> {
> struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> int rc = 0;
> + bool delivered = false;
> unsigned long irq_type;
> unsigned long irqs;
>
> @@ -1465,6 +1466,19 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> WARN_ONCE(1, "Unknown pending irq type %ld", irq_type);
> clear_bit(irq_type, &li->pending_irqs);
> }
> + delivered |= !rc;
> + }
> +
> + /*
> + * We delivered at least one interrupt and modified the PC. Force a
> + * singlestep event now.
> + */
> + if (delivered && guestdbg_sstep_enabled(vcpu)) {
> + struct kvm_debug_exit_arch *debug_exit = &vcpu->run->debug.arch;
> +
> + debug_exit->addr = vcpu->arch.sie_block->gpsw.addr;
> + debug_exit->type = KVM_SINGLESTEP;
> + vcpu->guest_debug |= KVM_GUESTDBG_EXIT_PENDING;
> }
>
> set_intercept_indicators(vcpu);
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d1e768bcfe1d..0c6333b108ba 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4611,7 +4611,7 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
>
> if (!kvm_is_ucontrol(vcpu->kvm)) {
> rc = kvm_s390_deliver_pending_interrupts(vcpu);
> - if (rc)
> + if (rc || guestdbg_exit_pending(vcpu))
> return rc;
> }
>
> @@ -4738,7 +4738,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>
> do {
> rc = vcpu_pre_run(vcpu);
> - if (rc)
> + if (rc || guestdbg_exit_pending(vcpu))
> break;
>
> kvm_vcpu_srcu_read_unlock(vcpu);
On Mon, 24 Jul 2023 11:44:08 +0200
Ilya Leoshkevich <[email protected]> wrote:
> Currently, after single-stepping an instruction that generates a
> specification exception, GDB ends up on the instruction immediately
> following it.
>
> The reason is that vcpu_post_run() injects the interrupt and sets
> KVM_GUESTDBG_EXIT_PENDING, causing a KVM_SINGLESTEP exit. The
> interrupt is not delivered, however, therefore userspace sees the
> address of the next instruction.
>
> Fix by letting the __vcpu_run() loop go into the next iteration,
> where vcpu_pre_run() delivers the interrupt and sets
> KVM_GUESTDBG_EXIT_PENDING.
>
> Reviewed-by: David Hildenbrand <[email protected]>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
> ---
> arch/s390/kvm/intercept.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 954d39adf85c..7cdd927541b0 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -226,7 +226,22 @@ static int handle_itdb(struct kvm_vcpu *vcpu)
> return 0;
> }
>
> -#define per_event(vcpu) (vcpu->arch.sie_block->iprcc & PGM_PER)
> +static bool should_handle_per_event(const struct kvm_vcpu *vcpu)
> +{
> + if (!guestdbg_enabled(vcpu))
> + return false;
> + if (!(vcpu->arch.sie_block->iprcc & PGM_PER))
why not if (!per_event(vcpu)) ?
maybe you can even merge it with the previous if:
if (!guestdbg_enabled(vcpu) || !per_event(vcpu))
return false;
this is closer to the old code too
> + return false;
> + if (guestdbg_sstep_enabled(vcpu) &&
> + vcpu->arch.sie_block->iprcc != PGM_PER) {
> + /*
> + * __vcpu_run() will exit after delivering the concurrently
> + * indicated condition.
> + */
> + return false;
> + }
> + return true;
> +}
>
> static int handle_prog(struct kvm_vcpu *vcpu)
> {
> @@ -242,7 +257,7 @@ static int handle_prog(struct kvm_vcpu *vcpu)
> if (kvm_s390_pv_cpu_is_protected(vcpu))
> return -EOPNOTSUPP;
>
> - if (guestdbg_enabled(vcpu) && per_event(vcpu)) {
> + if (should_handle_per_event(vcpu)) {
> rc = kvm_s390_handle_per_event(vcpu);
> if (rc)
> return rc;