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

[Thomas Huth <thuth@redhat.com>]

On 08/10/2021 22.31, Eric Farman 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>
> ---
>   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;
>   }

I was initially a little bit torn by this modification, since, as you 
already mentioned, it could theoretically be possible that a userspace (like 
an older version of QEMU) does not use CZAM bit yet. But then I read an 
older version of the PoP which does not feature CZAM yet, and it reads:

"The set-architecture order is completed as follows:
• If the code in the parameter register is not 0, 1, or
   2, or if the CPU is already in the architectural
   mode specified by the code, the order is not
   accepted. Instead, bit 55 (invalid parameter) of
   the general register designated by the R 1 field of
   the SIGNAL PROCESSOR instruction is set to
   one, and condition code 1 is set.
• If it is not true that all other CPUs in the configu-
   ration are in the stopped or check-stop state, the
   order is not accepted. Instead, bit 54 (incorrect
   state) of the general register designated by the
   R 1 field of the SIGNAL PROCESSOR instruction
   is set to one, and condition code 1 is set.
• The architectural mode of all CPUs in the config-
   uration is set as specified by the code.
   ..."

So to me this sounds like "invalid parameter" has a higher priority than 
"incorrect state" anyway, so we likely never
should have reported here "incorrect state"...?

Thus, I think it's the right way to go now:

Reviewed-by: Thomas Huth <thuth@redhat.com>