Re: [RFC PATCH v1 2/6] KVM: s390: Reject SIGP when destination CPU is busy

[Thomas Huth <thuth@redhat.com>]

On 08/10/2021 22.31, Eric Farman wrote:
> With KVM_CAP_USER_SIGP enabled, most orders are handled by userspace.
> However, some orders (such as STOP or STOP AND STORE STATUS) end up
> injecting work back into the kernel. Userspace itself should (and QEMU
> does) look for this conflict, and reject additional (non-reset) orders
> until this work completes.
> 
> But there's no need to delay that. If the kernel knows about the STOP
> IRQ that is in process, the newly-requested SIGP order can be rejected
> with a BUSY condition right up front.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>   arch/s390/kvm/sigp.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index cf4de80bd541..6ca01bbc72cf 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -394,6 +394,45 @@ static int handle_sigp_order_in_user_space(struct kvm_vcpu *vcpu, u8 order_code,
>   	return 1;
>   }
>   
> +static int handle_sigp_order_is_blocked(struct kvm_vcpu *vcpu, u8 order_code,
> +					u16 cpu_addr)
> +{
> +	struct kvm_vcpu *dst_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, cpu_addr);
> +	int rc = 0;
> +
> +	/*
> +	 * SIGP orders directed at invalid vcpus are not blocking,
> +	 * and should not return busy here. The code that handles
> +	 * the actual SIGP order will generate the "not operational"
> +	 * response for such a vcpu.
> +	 */
> +	if (!dst_vcpu)
> +		return 0;
> +
> +	/*
> +	 * SIGP orders that process a flavor of reset would not be
> +	 * blocked through another SIGP on the destination CPU.
> +	 */
> +	if (order_code == SIGP_CPU_RESET ||
> +	    order_code == SIGP_INITIAL_CPU_RESET)
> +		return 0;
> +
> +	/*
> +	 * Any other SIGP order could race with an existing SIGP order
> +	 * on the destination CPU, and thus encounter a busy condition
> +	 * on the CPU processing the SIGP order. Reject the order at
> +	 * this point, rather than racing with the STOP IRQ injection.
> +	 */
> +	spin_lock(&dst_vcpu->arch.local_int.lock);
> +	if (kvm_s390_is_stop_irq_pending(dst_vcpu)) {
> +		kvm_s390_set_psw_cc(vcpu, SIGP_CC_BUSY);
> +		rc = 1;
> +	}
> +	spin_unlock(&dst_vcpu->arch.local_int.lock);
> +
> +	return rc;
> +}
> +
>   int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
>   {
>   	int r1 = (vcpu->arch.sie_block->ipa & 0x00f0) >> 4;
> @@ -408,6 +447,10 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
>   		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>   
>   	order_code = kvm_s390_get_base_disp_rs(vcpu, NULL);
> +
> +	if (handle_sigp_order_is_blocked(vcpu, order_code, cpu_addr))
> +		return 0;
> +
>   	if (handle_sigp_order_in_user_space(vcpu, order_code, cpu_addr))
>   		return -EOPNOTSUPP;

We've been bitten quite a bit of times in the past already by doing too much 
control logic in the kernel instead of doing it in QEMU, where we should 
have a more complete view of the state ... so I'm feeling quite a bit uneasy 
of adding this in front of the "return -EOPNOTSUPP" here ... Did you see any 
performance issues that would justify this change?

  Thomas