Re: [PATCH v3 1/1] KVM: s390: pv: fix external interruption loop not always detected

Christian Borntraeger
2023-02-13T09:06:33Z

Am 13.02.23 um 09:55 schrieb Nico Boehr:
> To determine whether the guest has caused an external interruption loop
> upon code 20 (external interrupt) intercepts, the ext_new_psw needs to
> be inspected to see whether external interrupts are enabled.
>
> Under non-PV, ext_new_psw can simply be taken from guest lowcore. Under
> PV, KVM can only access the encrypted guest lowcore and hence the
> ext_new_psw must not be taken from guest lowcore.
>
> handle_external_interrupt() incorrectly did that and hence was not able
> to reliably tell whether an external interruption loop is happening or
> not. False negatives cause spurious failures of my kvm-unit-test
> for extint loops[1] under PV.
>
> Since code 20 is only caused under PV if and only if the guest's
> ext_new_psw is enabled for external interrupts, false positive detection
> of a external interruption loop can not happen.
>
> Fix this issue by instead looking at the guest PSW in the state
> description. Since the PSW swap for external interrupt is done by the
> ultravisor before the intercept is caused, this reliably tells whether
> the guest is enabled for external interrupts in the ext_new_psw.
>
> Also update the comments to explain better what is happening.
>
> [1] https://lore.kernel.org/kvm/20220812062151.1980937-4-nrb@linux.ibm.com/
>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
> arch/s390/kvm/intercept.c | 32 ++++++++++++++++++++++++--------
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 0ee02dae14b2..2cda8d9d7c6e 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -271,10 +271,18 @@ static int handle_prog(struct kvm_vcpu *vcpu)
> * handle_external_interrupt - used for external interruption interceptions
> * @vcpu: virtual cpu
> *
> - * This interception only occurs if the CPUSTAT_EXT_INT bit was set, or if
> - * the new PSW does not have external interrupts disabled. In the first case,
> - * we've got to deliver the interrupt manually, and in the second case, we
> - * drop to userspace to handle the situation there.
> + * This interception occurs if:
> + * - the CPUSTAT_EXT_INT bit was already set when the external interrupt
> + * occurred. In this case, the interrupt needs to be injected manually to
> + * preserve interrupt priority.
> + * - the external new PSW has external interrupts enabled, which will cause an
> + * interruption loop. We drop to userspace in this case.
> + *
> + * The latter case can be detected by inspecting the external mask bit in the
> + * external new psw.
> + *
> + * Under PV, only the latter case can occur, since interrupt priorities are
> + * handled in the ultravisor.
> */
> static int handle_external_interrupt(struct kvm_vcpu *vcpu)
> {
> @@ -285,10 +293,18 @@ static int handle_external_interrupt(struct kvm_vcpu *vcpu)
>
> vcpu->stat.exit_external_interrupt++;
>
> - rc = read_guest_lc(vcpu, __LC_EXT_NEW_PSW, &newpsw, sizeof(psw_t));
> - if (rc)
> - return rc;
> - /* We can not handle clock comparator or timer interrupt with bad PSW */
> + if (kvm_s390_pv_cpu_is_protected(vcpu)) {
> + newpsw = vcpu->arch.sie_block->gpsw;
> + } else {
> + rc = read_guest_lc(vcpu, __LC_EXT_NEW_PSW, &newpsw, sizeof(psw_t));
> + if (rc)
> + return rc;
> + }
> +
> + /*
> + * Clock comparator or timer interrupt with external interrupt enabled
> + * will cause interrupt loop. Drop to userspace.
> + */
> if ((eic == EXT_IRQ_CLK_COMP || eic == EXT_IRQ_CPU_TIMER) &&
> (newpsw.mask & PSW_MASK_EXT))
> return -EOPNOTSUPP;