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

[Christian Borntraeger <borntraeger@linux.ibm.com>]

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;