[RFC PATCH v1 0/6] Improvements to SIGP handling [KVM]

[RFC PATCH v1 0/6] Improvements to SIGP handling [KVM]

[Eric Farman]

I'm cleaning up some of the SIGP code in KVM and QEMU,
and would like to propose the following changes.

Patch 1 is interesting and could use some discussion, in that
CZAM cannot be disabled with QEMU (it is present in the earliest
CPU models) but the CPU model interface _could_ allow userspace
to leave it out. On the other (other?) hand, since we are always
in z/Architecture, that wouldn't make much sense as there would
probably be some other interesting side effects.

Patch 6 isn't required, but as I was looking at the intersection
of KVM capabilities S390_USER_SIGP and MP_STATE for this,
I thought some footprints could be useful.

There is no dependency on QEMU code, however another series of
patches for QEMU will follow.

Eric Farman (6):
  KVM: s390: Simplify SIGP Set Arch handling
  KVM: s390: Reject SIGP when destination CPU is busy
  KVM: s390: Simplify SIGP Restart
  KVM: s390: Restart IRQ should also block SIGP
  KVM: s390: Give BUSY to SIGP SENSE during Restart
  KVM: s390: Add a routine for setting userspace CPU state

 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/interrupt.c        |  7 +++
 arch/s390/kvm/kvm-s390.c         |  7 +--
 arch/s390/kvm/kvm-s390.h         | 10 ++++
 arch/s390/kvm/sigp.c             | 86 +++++++++++++++++++++++---------
 5 files changed, 85 insertions(+), 26 deletions(-)

-- 
2.25.1

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

[Eric Farman]

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;
 }
 
-- 
2.25.1

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

[Eric Farman]

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;
 
-- 
2.25.1

[RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Eric Farman]

Now that we check for the STOP IRQ injection at the top of the SIGP
handler (before the userspace/kernelspace check), we don't need to do
it down here for the Restart order.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 arch/s390/kvm/sigp.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 6ca01bbc72cf..0c08927ca7c9 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -240,17 +240,8 @@ static int __sigp_sense_running(struct kvm_vcpu *vcpu,
 static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
 				   struct kvm_vcpu *dst_vcpu, u8 order_code)
 {
-	struct kvm_s390_local_interrupt *li = &dst_vcpu->arch.local_int;
 	/* handle (RE)START in user space */
-	int rc = -EOPNOTSUPP;
-
-	/* make sure we don't race with STOP irq injection */
-	spin_lock(&li->lock);
-	if (kvm_s390_is_stop_irq_pending(dst_vcpu))
-		rc = SIGP_CC_BUSY;
-	spin_unlock(&li->lock);
-
-	return rc;
+	return -EOPNOTSUPP;
 }
 
 static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
-- 
2.25.1

[RFC PATCH v1 4/6] KVM: s390: Restart IRQ should also block SIGP

[Eric Farman]

When userspace handles a SIGP Restart, it first looks at the
destination CPU state to determine its next course of action:

  if (cpu is online)
     inject restart IRQ
  else
     set cpu online
     load restart PSW

Since we already have logic for dealing with an in-flight
STOP IRQ when a new SIGP comes in, let's include the RESTART
IRQ in the same logic, so we don't race with that work.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 7 +++++++
 arch/s390/kvm/kvm-s390.h  | 1 +
 arch/s390/kvm/sigp.c      | 5 +++--
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 10722455fd02..77c5d73ff0e2 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2108,6 +2108,13 @@ int s390int_to_s390irq(struct kvm_s390_interrupt *s390int,
 	return 0;
 }
 
+int kvm_s390_is_restart_irq_pending(struct kvm_vcpu *vcpu)
+{
+	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
+
+	return test_bit(IRQ_PEND_RESTART, &li->pending_irqs);
+}
+
 int kvm_s390_is_stop_irq_pending(struct kvm_vcpu *vcpu)
 {
 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 52bc8fbaa60a..57c5e9369d65 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -417,6 +417,7 @@ int psw_extint_disabled(struct kvm_vcpu *vcpu);
 void kvm_s390_destroy_adapters(struct kvm *kvm);
 int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu);
 extern struct kvm_device_ops kvm_flic_ops;
+int kvm_s390_is_restart_irq_pending(struct kvm_vcpu *vcpu);
 int kvm_s390_is_stop_irq_pending(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu);
 int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 0c08927ca7c9..c64e37f4347d 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -412,10 +412,11 @@ static int handle_sigp_order_is_blocked(struct kvm_vcpu *vcpu, u8 order_code,
 	 * 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.
+	 * this point, rather than racing with any IRQ injection.
 	 */
 	spin_lock(&dst_vcpu->arch.local_int.lock);
-	if (kvm_s390_is_stop_irq_pending(dst_vcpu)) {
+	if (kvm_s390_is_stop_irq_pending(dst_vcpu) ||
+	    kvm_s390_is_restart_irq_pending(dst_vcpu)) {
 		kvm_s390_set_psw_cc(vcpu, SIGP_CC_BUSY);
 		rc = 1;
 	}
-- 
2.25.1

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

[Eric Farman]

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;
 
-- 
2.25.1

[RFC PATCH v1 6/6] KVM: s390: Add a routine for setting userspace CPU state

[Eric Farman]

This capability exists, but we don't record anything when userspace
enables it. Let's refactor that code so that a note can be made in
the debug logs that it was enabled.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 6 +++---
 arch/s390/kvm/kvm-s390.h | 9 +++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 33d71fa42d68..48ac0bd05bee 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2487,8 +2487,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	case KVM_S390_PV_COMMAND: {
 		struct kvm_pv_cmd args;
 
-		/* protvirt means user sigp */
-		kvm->arch.user_cpu_state_ctrl = 1;
+		/* protvirt means user cpu state */
+		kvm_s390_set_user_cpu_state_ctrl(kvm);
 		r = 0;
 		if (!is_prot_virt_host()) {
 			r = -EINVAL;
@@ -3801,7 +3801,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 	vcpu_load(vcpu);
 
 	/* user space knows about this interface - let it control the state */
-	vcpu->kvm->arch.user_cpu_state_ctrl = 1;
+	kvm_s390_set_user_cpu_state_ctrl(vcpu->kvm);
 
 	switch (mp_state->mp_state) {
 	case KVM_MP_STATE_STOPPED:
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 57c5e9369d65..36f4d585513c 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -208,6 +208,15 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
 	return kvm->arch.user_cpu_state_ctrl != 0;
 }
 
+static inline void kvm_s390_set_user_cpu_state_ctrl(struct kvm *kvm)
+{
+	if (kvm->arch.user_cpu_state_ctrl)
+		return;
+
+	VM_EVENT(kvm, 3, "%s", "ENABLE: Userspace CPU state control");
+	kvm->arch.user_cpu_state_ctrl = 1;
+}
+
 /* implemented in pv.c */
 int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
 int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
-- 
2.25.1

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

[Thomas Huth]

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>

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

[Christian Borntraeger]

Am 11.10.21 um 08:29 schrieb Thomas Huth:
> 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:

I had the same concern, if we should cope in the kernel for ancient userspace, but your explanation below makes this actually even better.
And by definition in KVM we ARE always in z/Arch mode.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> "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>
> 

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

[Thomas Huth]

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

Re: [RFC PATCH v1 6/6] KVM: s390: Add a routine for setting userspace CPU state

[Thomas Huth]

On 08/10/2021 22.31, Eric Farman wrote:
> This capability exists, but we don't record anything when userspace
> enables it. Let's refactor that code so that a note can be made in
> the debug logs that it was enabled.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 6 +++---
>   arch/s390/kvm/kvm-s390.h | 9 +++++++++
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 33d71fa42d68..48ac0bd05bee 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2487,8 +2487,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   	case KVM_S390_PV_COMMAND: {
>   		struct kvm_pv_cmd args;
>   
> -		/* protvirt means user sigp */
> -		kvm->arch.user_cpu_state_ctrl = 1;
> +		/* protvirt means user cpu state */
> +		kvm_s390_set_user_cpu_state_ctrl(kvm);
>   		r = 0;
>   		if (!is_prot_virt_host()) {
>   			r = -EINVAL;
> @@ -3801,7 +3801,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>   	vcpu_load(vcpu);
>   
>   	/* user space knows about this interface - let it control the state */
> -	vcpu->kvm->arch.user_cpu_state_ctrl = 1;
> +	kvm_s390_set_user_cpu_state_ctrl(vcpu->kvm);
>   
>   	switch (mp_state->mp_state) {
>   	case KVM_MP_STATE_STOPPED:
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 57c5e9369d65..36f4d585513c 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -208,6 +208,15 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
>   	return kvm->arch.user_cpu_state_ctrl != 0;
>   }
>   
> +static inline void kvm_s390_set_user_cpu_state_ctrl(struct kvm *kvm)
> +{
> +	if (kvm->arch.user_cpu_state_ctrl)
> +		return;
> +
> +	VM_EVENT(kvm, 3, "%s", "ENABLE: Userspace CPU state control");
> +	kvm->arch.user_cpu_state_ctrl = 1;
> +}
> +
>   /* implemented in pv.c */
>   int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
>   int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> 

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

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

[Christian Borntraeger]

Am 11.10.21 um 09:27 schrieb Thomas Huth:
> 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?

It does at least handle this case for simple userspaces without KVM_CAP_S390_USER_SIGP .

Re: [RFC PATCH v1 6/6] KVM: s390: Add a routine for setting userspace CPU state

[David Hildenbrand]

On 08.10.21 22:31, Eric Farman wrote:
> This capability exists, but we don't record anything when userspace
> enables it. Let's refactor that code so that a note can be made in
> the debug logs that it was enabled.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 6 +++---
>   arch/s390/kvm/kvm-s390.h | 9 +++++++++
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 33d71fa42d68..48ac0bd05bee 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2487,8 +2487,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   	case KVM_S390_PV_COMMAND: {
>   		struct kvm_pv_cmd args;
>   
> -		/* protvirt means user sigp */
> -		kvm->arch.user_cpu_state_ctrl = 1;
> +		/* protvirt means user cpu state */
> +		kvm_s390_set_user_cpu_state_ctrl(kvm);
>   		r = 0;
>   		if (!is_prot_virt_host()) {
>   			r = -EINVAL;
> @@ -3801,7 +3801,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>   	vcpu_load(vcpu);
>   
>   	/* user space knows about this interface - let it control the state */
> -	vcpu->kvm->arch.user_cpu_state_ctrl = 1;
> +	kvm_s390_set_user_cpu_state_ctrl(vcpu->kvm);
>   
>   	switch (mp_state->mp_state) {
>   	case KVM_MP_STATE_STOPPED:
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 57c5e9369d65..36f4d585513c 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -208,6 +208,15 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
>   	return kvm->arch.user_cpu_state_ctrl != 0;
>   }
>   
> +static inline void kvm_s390_set_user_cpu_state_ctrl(struct kvm *kvm)
> +{
> +	if (kvm->arch.user_cpu_state_ctrl)
> +		return;
> +
> +	VM_EVENT(kvm, 3, "%s", "ENABLE: Userspace CPU state control");
> +	kvm->arch.user_cpu_state_ctrl = 1;
> +}
> +
>   /* implemented in pv.c */
>   int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
>   int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Christian Borntraeger]

Am 08.10.21 um 22:31 schrieb Eric Farman:
> Now that we check for the STOP IRQ injection at the top of the SIGP
> handler (before the userspace/kernelspace check), we don't need to do
> it down here for the Restart order.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/kvm/sigp.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index 6ca01bbc72cf..0c08927ca7c9 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -240,17 +240,8 @@ static int __sigp_sense_running(struct kvm_vcpu *vcpu,
>   static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
>   				   struct kvm_vcpu *dst_vcpu, u8 order_code)
>   {
> -	struct kvm_s390_local_interrupt *li = &dst_vcpu->arch.local_int;
>   	/* handle (RE)START in user space */
> -	int rc = -EOPNOTSUPP;
> -
> -	/* make sure we don't race with STOP irq injection */
> -	spin_lock(&li->lock);
> -	if (kvm_s390_is_stop_irq_pending(dst_vcpu))
> -		rc = SIGP_CC_BUSY;
> -	spin_unlock(&li->lock);
> -
> -	return rc;
> +	return -EOPNOTSUPP;
>   }
>   
>   static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
> 

@thuth?
Question is, does it make sense to merge patch 2 and 3 to make things more obvious?

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

[Thomas Huth]

On 11/10/2021 09.43, Christian Borntraeger wrote:
> Am 11.10.21 um 09:27 schrieb Thomas Huth:
>> 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?
> 
> It does at least handle this case for simple userspaces without 
> KVM_CAP_S390_USER_SIGP .

For that case, I'd prefer to swap the order here by doing the "if 
handle_sigp_order_in_user_space return -EOPNOTSUPP" first, and doing the "if 
handle_sigp_order_is_blocked return 0" afterwards.

... unless we feel really, really sure that it always ok to do it like in 
this patch ... but as I said, we've been bitten by such things a couple of 
times already, so I'd suggest to better play safe...

  Thomas

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

[David Hildenbrand]

On 08.10.21 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;
>   }
>   
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 11.10.21 09:52, Thomas Huth wrote:
> On 11/10/2021 09.43, Christian Borntraeger wrote:
>> Am 11.10.21 um 09:27 schrieb Thomas Huth:
>>> 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?
>>
>> It does at least handle this case for simple userspaces without
>> KVM_CAP_S390_USER_SIGP .
> 
> For that case, I'd prefer to swap the order here by doing the "if
> handle_sigp_order_in_user_space return -EOPNOTSUPP" first, and doing the "if
> handle_sigp_order_is_blocked return 0" afterwards.
> 
> ... unless we feel really, really sure that it always ok to do it like in
> this patch ... but as I said, we've been bitten by such things a couple of
> times already, so I'd suggest to better play safe...

As raised in the QEMU series, I wonder if it's cleaner for user space to 
set the target CPU as busy/!busy for SIGP while processing an order. 
We'll need a new VCPU IOCTL, but it conceptually sounds cleaner to me ...
-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

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

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

[Eric Farman]

On Mon, 2021-10-11 at 19:58 +0200, David Hildenbrand wrote:
> On 11.10.21 09:52, Thomas Huth wrote:
> > On 11/10/2021 09.43, Christian Borntraeger wrote:
> > > Am 11.10.21 um 09:27 schrieb Thomas Huth:
> > > > 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 ... 

Fair enough. It's an unfortunate side effect that "USER_SIGP" means
"all SIGP orders except for these ones that are handled totally within
the kernel."


> > > > 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?
> > > 
> > > It does at least handle this case for simple userspaces without
> > > KVM_CAP_S390_USER_SIGP .
> > 
> > For that case, I'd prefer to swap the order here by doing the "if
> > handle_sigp_order_in_user_space return -EOPNOTSUPP" first, and
> > doing the "if
> > handle_sigp_order_is_blocked return 0" afterwards.

Well, that would be fine. But of course part of my worry is when
userspace has CAP_S390_USER_SIGP, and we have a race between the kernel
handling SENSE and userspace handling things like STOP/RESTART.

> > 
> > ... unless we feel really, really sure that it always ok to do it
> > like in
> > this patch ... but as I said, we've been bitten by such things a
> > couple of
> > times already, so I'd suggest to better play safe...
> 
> As raised in the QEMU series, I wonder if it's cleaner for user space
> to 
> set the target CPU as busy/!busy for SIGP while processing an order. 
> We'll need a new VCPU IOCTL, but it conceptually sounds cleaner to me
> ...

Hrm... Well I guess I'd hoped to address this within the boundaries of
what exists today. Since there already is a "userspace sets cpu state"
interface, but those states do not contain a "busy" (just stopped or
operating, according to POPS), I'd tried to stay away from going down
that path to avoid confusion. I'll take a swag at it, though.

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

[Claudio Imbrenda]

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;
>  }
>  

Re: [RFC PATCH v1 6/6] KVM: s390: Add a routine for setting userspace CPU state

[Claudio Imbrenda]

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

> This capability exists, but we don't record anything when userspace
> enables it. Let's refactor that code so that a note can be made in
> the debug logs that it was enabled.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

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

> ---
>  arch/s390/kvm/kvm-s390.c | 6 +++---
>  arch/s390/kvm/kvm-s390.h | 9 +++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 33d71fa42d68..48ac0bd05bee 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2487,8 +2487,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  	case KVM_S390_PV_COMMAND: {
>  		struct kvm_pv_cmd args;
>  
> -		/* protvirt means user sigp */
> -		kvm->arch.user_cpu_state_ctrl = 1;
> +		/* protvirt means user cpu state */
> +		kvm_s390_set_user_cpu_state_ctrl(kvm);
>  		r = 0;
>  		if (!is_prot_virt_host()) {
>  			r = -EINVAL;
> @@ -3801,7 +3801,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  	vcpu_load(vcpu);
>  
>  	/* user space knows about this interface - let it control the state */
> -	vcpu->kvm->arch.user_cpu_state_ctrl = 1;
> +	kvm_s390_set_user_cpu_state_ctrl(vcpu->kvm);
>  
>  	switch (mp_state->mp_state) {
>  	case KVM_MP_STATE_STOPPED:
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 57c5e9369d65..36f4d585513c 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -208,6 +208,15 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
>  	return kvm->arch.user_cpu_state_ctrl != 0;
>  }
>  
> +static inline void kvm_s390_set_user_cpu_state_ctrl(struct kvm *kvm)
> +{
> +	if (kvm->arch.user_cpu_state_ctrl)
> +		return;
> +
> +	VM_EVENT(kvm, 3, "%s", "ENABLE: Userspace CPU state control");
> +	kvm->arch.user_cpu_state_ctrl = 1;
> +}
> +
>  /* implemented in pv.c */
>  int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
>  int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);

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

[Christian Borntraeger]

Am 08.10.21 um 22:31 schrieb Eric Farman:
> 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 applied this one to reduce the number of patches :-). Thanks

Re: [RFC PATCH v1 6/6] KVM: s390: Add a routine for setting userspace CPU state

[Christian Borntraeger]

Am 08.10.21 um 22:31 schrieb Eric Farman:
> This capability exists, but we don't record anything when userspace
> enables it. Let's refactor that code so that a note can be made in
> the debug logs that it was enabled.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

I also applied this one. Thanks.
> ---
>   arch/s390/kvm/kvm-s390.c | 6 +++---
>   arch/s390/kvm/kvm-s390.h | 9 +++++++++
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 33d71fa42d68..48ac0bd05bee 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2487,8 +2487,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   	case KVM_S390_PV_COMMAND: {
>   		struct kvm_pv_cmd args;
>   
> -		/* protvirt means user sigp */
> -		kvm->arch.user_cpu_state_ctrl = 1;
> +		/* protvirt means user cpu state */
> +		kvm_s390_set_user_cpu_state_ctrl(kvm);
>   		r = 0;
>   		if (!is_prot_virt_host()) {
>   			r = -EINVAL;
> @@ -3801,7 +3801,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>   	vcpu_load(vcpu);
>   
>   	/* user space knows about this interface - let it control the state */
> -	vcpu->kvm->arch.user_cpu_state_ctrl = 1;
> +	kvm_s390_set_user_cpu_state_ctrl(vcpu->kvm);
>   
>   	switch (mp_state->mp_state) {
>   	case KVM_MP_STATE_STOPPED:
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 57c5e9369d65..36f4d585513c 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -208,6 +208,15 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm)
>   	return kvm->arch.user_cpu_state_ctrl != 0;
>   }
>   
> +static inline void kvm_s390_set_user_cpu_state_ctrl(struct kvm *kvm)
> +{
> +	if (kvm->arch.user_cpu_state_ctrl)
> +		return;
> +
> +	VM_EVENT(kvm, 3, "%s", "ENABLE: Userspace CPU state control");
> +	kvm->arch.user_cpu_state_ctrl = 1;
> +}
> +
>   /* implemented in pv.c */
>   int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
>   int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> 

Re: [RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Thomas Huth]

On 11/10/2021 09.45, Christian Borntraeger wrote:
> 
> 
> Am 08.10.21 um 22:31 schrieb Eric Farman:
>> Now that we check for the STOP IRQ injection at the top of the SIGP
>> handler (before the userspace/kernelspace check), we don't need to do
>> it down here for the Restart order.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   arch/s390/kvm/sigp.c | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
>> index 6ca01bbc72cf..0c08927ca7c9 100644
>> --- a/arch/s390/kvm/sigp.c
>> +++ b/arch/s390/kvm/sigp.c
>> @@ -240,17 +240,8 @@ static int __sigp_sense_running(struct kvm_vcpu *vcpu,
>>   static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
>>                      struct kvm_vcpu *dst_vcpu, u8 order_code)
>>   {
>> -    struct kvm_s390_local_interrupt *li = &dst_vcpu->arch.local_int;
>>       /* handle (RE)START in user space */
>> -    int rc = -EOPNOTSUPP;
>> -
>> -    /* make sure we don't race with STOP irq injection */
>> -    spin_lock(&li->lock);
>> -    if (kvm_s390_is_stop_irq_pending(dst_vcpu))
>> -        rc = SIGP_CC_BUSY;
>> -    spin_unlock(&li->lock);
>> -
>> -    return rc;
>> +    return -EOPNOTSUPP;
>>   }
>>   static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
>>
> 
> @thuth?
> Question is, does it make sense to merge patch 2 and 3 to make things more 
> obvious?

Maybe.

Anyway: Would it make sense to remove __prepare_sigp_re_start() completely 
now and let __prepare_sigp_unknown() set the return code in the "default:" case?

  Thomas

Re: [RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Eric Farman]

On Tue, 2021-10-12 at 17:23 +0200, Thomas Huth wrote:
> On 11/10/2021 09.45, Christian Borntraeger wrote:
> > 
> > Am 08.10.21 um 22:31 schrieb Eric Farman:
> > > Now that we check for the STOP IRQ injection at the top of the
> > > SIGP
> > > handler (before the userspace/kernelspace check), we don't need
> > > to do
> > > it down here for the Restart order.
> > > 
> > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > ---
> > >   arch/s390/kvm/sigp.c | 11 +----------
> > >   1 file changed, 1 insertion(+), 10 deletions(-)
> > > 
> > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> > > index 6ca01bbc72cf..0c08927ca7c9 100644
> > > --- a/arch/s390/kvm/sigp.c
> > > +++ b/arch/s390/kvm/sigp.c
> > > @@ -240,17 +240,8 @@ static int __sigp_sense_running(struct
> > > kvm_vcpu *vcpu,
> > >   static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
> > >                      struct kvm_vcpu *dst_vcpu, u8 order_code)
> > >   {
> > > -    struct kvm_s390_local_interrupt *li = &dst_vcpu-
> > > >arch.local_int;
> > >       /* handle (RE)START in user space */
> > > -    int rc = -EOPNOTSUPP;
> > > -
> > > -    /* make sure we don't race with STOP irq injection */
> > > -    spin_lock(&li->lock);
> > > -    if (kvm_s390_is_stop_irq_pending(dst_vcpu))
> > > -        rc = SIGP_CC_BUSY;
> > > -    spin_unlock(&li->lock);
> > > -
> > > -    return rc;
> > > +    return -EOPNOTSUPP;
> > >   }
> > >   static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
> > > 
> > 
> > @thuth?
> > Question is, does it make sense to merge patch 2 and 3 to make
> > things more 
> > obvious?
> 
> Maybe.
> 
> Anyway: Would it make sense to remove __prepare_sigp_re_start()
> completely 
> now and let __prepare_sigp_unknown() set the return code in the
> "default:" case?

We could, but that would affect the SIGP START case which also uses the
re_start routine. And if we're going down that path, we could remove
(INITIAL) CPU RESET handled in __prepare_sigp_cpu_reset, which does the
same thing (nothing). Not sure it buys us much, other than losing the
details in the different counters of which SIGP orders are processed.

Eric

> 
>   Thomas
> 

Re: [RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Thomas Huth]

On 12/10/2021 17.31, Eric Farman wrote:
> On Tue, 2021-10-12 at 17:23 +0200, Thomas Huth wrote:
>> On 11/10/2021 09.45, Christian Borntraeger wrote:
>>>
>>> Am 08.10.21 um 22:31 schrieb Eric Farman:
>>>> Now that we check for the STOP IRQ injection at the top of the
>>>> SIGP
>>>> handler (before the userspace/kernelspace check), we don't need
>>>> to do
>>>> it down here for the Restart order.
>>>>
>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>> ---
>>>>    arch/s390/kvm/sigp.c | 11 +----------
>>>>    1 file changed, 1 insertion(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
>>>> index 6ca01bbc72cf..0c08927ca7c9 100644
>>>> --- a/arch/s390/kvm/sigp.c
>>>> +++ b/arch/s390/kvm/sigp.c
>>>> @@ -240,17 +240,8 @@ static int __sigp_sense_running(struct
>>>> kvm_vcpu *vcpu,
>>>>    static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
>>>>                       struct kvm_vcpu *dst_vcpu, u8 order_code)
>>>>    {
>>>> -    struct kvm_s390_local_interrupt *li = &dst_vcpu-
>>>>> arch.local_int;
>>>>        /* handle (RE)START in user space */
>>>> -    int rc = -EOPNOTSUPP;
>>>> -
>>>> -    /* make sure we don't race with STOP irq injection */
>>>> -    spin_lock(&li->lock);
>>>> -    if (kvm_s390_is_stop_irq_pending(dst_vcpu))
>>>> -        rc = SIGP_CC_BUSY;
>>>> -    spin_unlock(&li->lock);
>>>> -
>>>> -    return rc;
>>>> +    return -EOPNOTSUPP;
>>>>    }
>>>>    static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
>>>>
>>>
>>> @thuth?
>>> Question is, does it make sense to merge patch 2 and 3 to make
>>> things more
>>> obvious?
>>
>> Maybe.
>>
>> Anyway: Would it make sense to remove __prepare_sigp_re_start()
>> completely
>> now and let __prepare_sigp_unknown() set the return code in the
>> "default:" case?
> 
> We could, but that would affect the SIGP START case which also uses the
> re_start routine. And if we're going down that path, we could remove
> (INITIAL) CPU RESET handled in __prepare_sigp_cpu_reset, which does the
> same thing (nothing). Not sure it buys us much, other than losing the
> details in the different counters of which SIGP orders are processed.

Ok, we likely shouldn't change the way of counting the SIGPs here...
So what about removing the almost empty function and simply do the "rc = 
-EOPNOTSUPP" right in the handle_sigp_dst() function? That's still the 
easiest way to read the code, I think. And we should do the same with the 
__prepare_sigp_cpu_reset() function (in a separate patch). Just my 0.02 € of 
course.

  Thomas

Re: [RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Eric Farman]

On Wed, 2021-10-13 at 07:54 +0200, Thomas Huth wrote:
> On 12/10/2021 17.31, Eric Farman wrote:
> > On Tue, 2021-10-12 at 17:23 +0200, Thomas Huth wrote:
> > > On 11/10/2021 09.45, Christian Borntraeger wrote:
> > > > Am 08.10.21 um 22:31 schrieb Eric Farman:
> > > > > Now that we check for the STOP IRQ injection at the top of
> > > > > the
> > > > > SIGP
> > > > > handler (before the userspace/kernelspace check), we don't
> > > > > need
> > > > > to do
> > > > > it down here for the Restart order.
> > > > > 
> > > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > > ---
> > > > >    arch/s390/kvm/sigp.c | 11 +----------
> > > > >    1 file changed, 1 insertion(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> > > > > index 6ca01bbc72cf..0c08927ca7c9 100644
> > > > > --- a/arch/s390/kvm/sigp.c
> > > > > +++ b/arch/s390/kvm/sigp.c
> > > > > @@ -240,17 +240,8 @@ static int __sigp_sense_running(struct
> > > > > kvm_vcpu *vcpu,
> > > > >    static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
> > > > >                       struct kvm_vcpu *dst_vcpu, u8
> > > > > order_code)
> > > > >    {
> > > > > -    struct kvm_s390_local_interrupt *li = &dst_vcpu-
> > > > > > arch.local_int;
> > > > >        /* handle (RE)START in user space */
> > > > > -    int rc = -EOPNOTSUPP;
> > > > > -
> > > > > -    /* make sure we don't race with STOP irq injection */
> > > > > -    spin_lock(&li->lock);
> > > > > -    if (kvm_s390_is_stop_irq_pending(dst_vcpu))
> > > > > -        rc = SIGP_CC_BUSY;
> > > > > -    spin_unlock(&li->lock);
> > > > > -
> > > > > -    return rc;
> > > > > +    return -EOPNOTSUPP;
> > > > >    }
> > > > >    static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
> > > > > 
> > > > 
> > > > @thuth?
> > > > Question is, does it make sense to merge patch 2 and 3 to make
> > > > things more
> > > > obvious?
> > > 
> > > Maybe.
> > > 
> > > Anyway: Would it make sense to remove __prepare_sigp_re_start()
> > > completely
> > > now and let __prepare_sigp_unknown() set the return code in the
> > > "default:" case?
> > 
> > We could, but that would affect the SIGP START case which also uses
> > the
> > re_start routine. And if we're going down that path, we could
> > remove
> > (INITIAL) CPU RESET handled in __prepare_sigp_cpu_reset, which does
> > the
> > same thing (nothing). Not sure it buys us much, other than losing
> > the
> > details in the different counters of which SIGP orders are
> > processed.
> 
> Ok, we likely shouldn't change the way of counting the SIGPs here...
> So what about removing the almost empty function and simply do the
> "rc = 
> -EOPNOTSUPP" right in the handle_sigp_dst() function? That's still
> the 
> easiest way to read the code, I think. 

Hrm, that might be better. I've almost got the IOCTL stuff in a
reasonable place for a discussion, will see about such cleanups at the
end of that (new) series.

> And we should do the same with the 
> __prepare_sigp_cpu_reset() function (in a separate patch). Just my
> 0.02 € of 
> course.

I appreciate it. Though I still don't have an easy way to use the €
coins I have in a drawer over here. ;-)

Eric

> 
>   Thomas
>