Re: [RFC PATCH v1 1/6] KVM: s390: Simplify SIGP Set Arch handling

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri,  8 Oct 2021 22:31:07 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> The Principles of Operations describe the various reasons that
> each individual SIGP orders might be rejected, and the status
> bit that are set for each condition.
> 
> For example, for the Set Architecture order, it states:
> 
>   "If it is not true that all other CPUs in the configu-
>    ration are in the stopped or check-stop state, ...
>    bit 54 (incorrect state) ... is set to one."
> 
> However, it also states:
> 
>   "... if the CZAM facility is installed, ...
>    bit 55 (invalid parameter) ... is set to one."
> 
> Since the Configuration-z/Architecture-Architectural Mode (CZAM)
> facility is unconditionally presented, there is no need to examine
> each VCPU to determine if it is started/stopped. It can simply be
> rejected outright with the Invalid Parameter bit.
> 
> Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

I like removing and simplifying code while staying architecturally
correct :)

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/kvm/sigp.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index 683036c1c92a..cf4de80bd541 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu,
>  static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter,
>  			   u64 *status_reg)
>  {
> -	unsigned int i;
> -	struct kvm_vcpu *v;
> -	bool all_stopped = true;
> -
> -	kvm_for_each_vcpu(i, v, vcpu->kvm) {
> -		if (v == vcpu)
> -			continue;
> -		if (!is_vcpu_stopped(v))
> -			all_stopped = false;
> -	}
> -
>  	*status_reg &= 0xffffffff00000000UL;
>  
>  	/* Reject set arch order, with czam we're always in z/Arch mode. */
> -	*status_reg |= (all_stopped ? SIGP_STATUS_INVALID_PARAMETER :
> -					SIGP_STATUS_INCORRECT_STATE);
> +	*status_reg |= SIGP_STATUS_INVALID_PARAMETER;
>  	return SIGP_CC_STATUS_STORED;
>  }
>