[PATCH v2 0/6] KVM: s390: interrupt: Fix stepping into interrupt handlers

[PATCH v2 0/6] KVM: s390: interrupt: Fix stepping into interrupt handlers

[Ilya Leoshkevich]

v1: https://lore.kernel.org/lkml/20230629083452.183274-1-iii@linux.ibm.com/
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.

I'm not very familiar with the KVM code base, so please let me know if
the fixes can be improved or if these problems need to be handled
completely differently.

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 ISKE
  KVM: s390: selftests: Add selftest for single-stepping

 arch/s390/kvm/intercept.c                     |  39 ++++-
 arch/s390/kvm/interrupt.c                     |  10 ++
 arch/s390/kvm/kvm-s390.c                      |  20 ++-
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/s390x/debug_test.c  | 160 ++++++++++++++++++
 5 files changed, 218 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/s390x/debug_test.c

-- 
2.41.0

[PATCH v2 1/6] KVM: s390: interrupt: Fix single-stepping into interrupt handlers

[Ilya Leoshkevich]

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.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 10 ++++++++++
 arch/s390/kvm/kvm-s390.c  |  4 ++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 9bd0a873f3b1..2cebe4227b8e 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,15 @@ 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;
+	}
+
+	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

[PATCH v2 2/6] KVM: s390: interrupt: Fix single-stepping into program interrupt handlers

[Ilya Leoshkevich]

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.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 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

[PATCH v2 3/6] KVM: s390: interrupt: Fix single-stepping kernel-emulated instructions

[Ilya Leoshkevich]

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.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 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

[PATCH v2 4/6] KVM: s390: interrupt: Fix single-stepping userspace-emulated instructions

[Ilya Leoshkevich]

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 before KVM_EXIT_S390_SIEIC.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0c6333b108ba..b717fb8cffed 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,18 @@ 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;
+
+	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

[PATCH v2 5/6] KVM: s390: interrupt: Fix single-stepping ISKE

[Ilya Leoshkevich]

kvm_s390_skey_check_enable() does not emulate any instructions, rather,
it clears CPUSTAT_KSS and arranges for ISKE to run again. Therefore,
skip the PER check and let ISKE run happen. Otherwise a debugger will
see two single-step events on the same ISKE.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/kvm/intercept.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index d2f7940c5d03..8793cec066a6 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -630,8 +630,7 @@ 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;
+		return kvm_s390_skey_check_enable(vcpu);
 	case ICPT_MCHKREQ:
 	case ICPT_INT_ENABLE:
 		/*
-- 
2.41.0

[PATCH v2 6/6] KVM: s390: selftests: Add selftest for single-stepping

[Ilya Leoshkevich]

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 <iii@linux.ibm.com>
---
 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, &regs);
+	regs.gprs[2] = -1;
+	vcpu_regs_set(*vcpu, &regs);
+	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

Re: [PATCH v2 5/6] KVM: s390: interrupt: Fix single-stepping ISKE

[Christian Borntraeger]

Am 21.07.23 um 13:57 schrieb Ilya Leoshkevich:
> kvm_s390_skey_check_enable() does not emulate any instructions, rather,
> it clears CPUSTAT_KSS and arranges for ISKE to run again. Therefore,
> skip the PER check and let ISKE run happen. Otherwise a debugger will
> see two single-step events on the same ISKE.

The same would be true for all instruction triggering a keyless mode exit,
like SSKE, RRBE but also LPSWE with a keyed PSW, no?
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> ---
>   arch/s390/kvm/intercept.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index d2f7940c5d03..8793cec066a6 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -630,8 +630,7 @@ 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;

maybe add a comment here: /* Instruction will be redriven, skip the PER check */
> +		return kvm_s390_skey_check_enable(vcpu);

>   	case ICPT_MCHKREQ:
>   	case ICPT_INT_ENABLE:
>   		/*

Re: [PATCH v2 1/6] KVM: s390: interrupt: Fix single-stepping into interrupt handlers

[David Hildenbrand]

On 21.07.23 13:57, Ilya Leoshkevich 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.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c | 10 ++++++++++
>   arch/s390/kvm/kvm-s390.c  |  4 ++--
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 9bd0a873f3b1..2cebe4227b8e 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,15 @@ 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;
> +	}
> +


Can we add a comment like

/*
  * 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;
>   	}

I do wonder if we, instead, want to do this whenever we modify the PSW.

That way we could catch any PC changes and only have to add checks for 
guestdbg_exit_pending().


But this is simpler and should work as well.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 2/6] KVM: s390: interrupt: Fix single-stepping into program interrupt handlers

[David Hildenbrand]

On 21.07.23 13:57, Ilya Leoshkevich 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.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   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;

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/6] KVM: s390: interrupt: Fix single-stepping kernel-emulated instructions

[David Hildenbrand]

On 21.07.23 13:57, Ilya Leoshkevich 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.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   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;
>   }

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/6] KVM: s390: interrupt: Fix single-stepping userspace-emulated instructions

[David Hildenbrand]

> +		rc = -ENOIOCTLCMD;
> +		break;
>   	}
> -	return -ENOIOCTLCMD;
> +

This really needs a comment. :)

> +	if (!rc)
> +		vcpu->guest_debug &= ~KVM_GUESTDBG_EXIT_PENDING;
> +
> +	return rc;
>   }
>   
>   static int kvm_s390_handle_pv_vcpu_dump(struct kvm_vcpu *vcpu,

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 5/6] KVM: s390: interrupt: Fix single-stepping ISKE

[David Hildenbrand]

On 21.07.23 13:57, Ilya Leoshkevich wrote:
> kvm_s390_skey_check_enable() does not emulate any instructions, rather,
> it clears CPUSTAT_KSS and arranges for ISKE to run again. Therefore,
> skip the PER check and let ISKE run happen. Otherwise a debugger will
> see two single-step events on the same ISKE.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   arch/s390/kvm/intercept.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index d2f7940c5d03..8793cec066a6 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -630,8 +630,7 @@ 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;
> +		return kvm_s390_skey_check_enable(vcpu);
>   	case ICPT_MCHKREQ:
>   	case ICPT_INT_ENABLE:
>   		/*

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/6] KVM: s390: interrupt: Fix single-stepping into interrupt handlers

[Ilya Leoshkevich]

On Mon, 2023-07-24 at 10:22 +0200, David Hildenbrand wrote:
> On 21.07.23 13:57, Ilya Leoshkevich 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.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   arch/s390/kvm/interrupt.c | 10 ++++++++++
> >   arch/s390/kvm/kvm-s390.c  |  4 ++--
> >   2 files changed, 12 insertions(+), 2 deletions(-)

[...]
> 

> Can we add a comment like
> 
> /*
>   * We delivered at least one interrupt and modified the PC. Force a
>   * singlestep event now.
>   */

Ok, will do.

> > +       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;
> >         }
> 
> I do wonder if we, instead, want to do this whenever we modify the
> PSW.
> 
> That way we could catch any PC changes and only have to add checks
> for 
> guestdbg_exit_pending().

Wouldn't this break a corner case where the first instruction of the
interrupt handler causes the same interrupt?

> But this is simpler and should work as well.
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Re: [PATCH v2 1/6] KVM: s390: interrupt: Fix single-stepping into interrupt handlers

[David Hildenbrand]

On 24.07.23 10:42, Ilya Leoshkevich wrote:
> On Mon, 2023-07-24 at 10:22 +0200, David Hildenbrand wrote:
>> On 21.07.23 13:57, Ilya Leoshkevich 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.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    arch/s390/kvm/interrupt.c | 10 ++++++++++
>>>    arch/s390/kvm/kvm-s390.c  |  4 ++--
>>>    2 files changed, 12 insertions(+), 2 deletions(-)
> 
> [...]
>>
> 
>> Can we add a comment like
>>
>> /*
>>    * We delivered at least one interrupt and modified the PC. Force a
>>    * singlestep event now.
>>    */
> 
> Ok, will do.
> 
>>> +       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;
>>>          }
>>
>> I do wonder if we, instead, want to do this whenever we modify the
>> PSW.
>>
>> That way we could catch any PC changes and only have to add checks
>> for
>> guestdbg_exit_pending().
> 
> Wouldn't this break a corner case where the first instruction of the
> interrupt handler causes the same interrupt?

Could be, there are many possible corner cases (PGM interrupt at the 
first instruction of PGM interrupt handler -- our PSW address might not 
even change)

-- 
Cheers,

David / dhildenb