Re: [RFC PATCH v1 5/6] KVM: s390: Give BUSY to SIGP SENSE during Restart

[David Hildenbrand <david@redhat.com>]

On 08.10.21 22:31, Eric Farman wrote:
> A SIGP RESTART is a special animal, in that it directs the
> destination CPU to perform the restart operation. This is
> basically the loading of the Restart PSW and letting it take
> over, but a stopped CPU must first be made operating for this
> to work correctly.
> 
> As this can take a moment, let's leave a reminder that this
> SIGP is being processed, such that the SIGP SENSE logic
> (which is not handled in userspace) can return CC=2 instead
> of CC=1 (and STOPPED) until the CPU is started.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h |  1 +
>   arch/s390/kvm/kvm-s390.c         |  1 +
>   arch/s390/kvm/sigp.c             | 17 +++++++++++++++++
>   3 files changed, 19 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index a604d51acfc8..536f174c5e81 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -746,6 +746,7 @@ struct kvm_vcpu_arch {
>   	__u64 cputm_start;
>   	bool gs_enabled;
>   	bool skey_enabled;
> +	bool sigp_restart;
>   	struct kvm_s390_pv_vcpu pv;
>   	union diag318_info diag318_info;
>   };
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6a6dd5e1daf6..33d71fa42d68 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4603,6 +4603,7 @@ int kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
>   	}
>   
>   	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_STOPPED);
> +	vcpu->arch.sigp_restart = 0;
>   	/*
>   	 * The real PSW might have changed due to a RESTART interpreted by the
>   	 * ultravisor. We block all interrupts and let the next sie exit
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index c64e37f4347d..5a21354d0265 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -27,6 +27,8 @@ static int __sigp_sense(struct kvm_vcpu *vcpu, struct kvm_vcpu *dst_vcpu,
>   	ext_call_pending = kvm_s390_ext_call_pending(dst_vcpu);
>   	if (!stopped && !ext_call_pending)
>   		rc = SIGP_CC_ORDER_CODE_ACCEPTED;
> +	else if (stopped && dst_vcpu->arch.sigp_restart)
> +		rc = SIGP_CC_BUSY;
>   	else {
>   		*reg &= 0xffffffff00000000UL;
>   		if (ext_call_pending)
> @@ -385,6 +387,18 @@ static int handle_sigp_order_in_user_space(struct kvm_vcpu *vcpu, u8 order_code,
>   	return 1;
>   }
>   
> +static void handle_sigp_restart(struct kvm_vcpu *vcpu, u16 cpu_addr)
> +{
> +	struct kvm_vcpu *dst_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, cpu_addr);
> +
> +	/* Ignore SIGP Restart to non-existent CPUs */
> +	if (!dst_vcpu)
> +		return;
> +
> +	if (is_vcpu_stopped(dst_vcpu))
> +		dst_vcpu->arch.sigp_restart = 1;
> +}
> +
>   static int handle_sigp_order_is_blocked(struct kvm_vcpu *vcpu, u8 order_code,
>   					u16 cpu_addr)
>   {
> @@ -443,6 +457,9 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
>   	if (handle_sigp_order_is_blocked(vcpu, order_code, cpu_addr))
>   		return 0;
>   
> +	if (order_code == SIGP_RESTART)
> +		handle_sigp_restart(vcpu, cpu_addr);
> +
>   	if (handle_sigp_order_in_user_space(vcpu, order_code, cpu_addr))
>   		return -EOPNOTSUPP;
>   
> 

Okay, staring at this, I think we really might just let user space 
indicate SIGP as busy/!busy. Will take a lot of magic out of this code. 
My 2 cents.

-- 
Thanks,

David / dhildenb