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.
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
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 <[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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
> ---
> 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:
> /*
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Cheers,
David / dhildenb
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 <[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;
> }
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
> + 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