[RFC PATCH v5 0/1] s390x: Improvements to SIGP handling [KVM]

[RFC PATCH v5 0/1] s390x: Improvements to SIGP handling [KVM]

[Eric Farman]

Here is an update to the handling of SIGP between kernel and userspace.

As before, I'm looking at problems encountered when a SIGP order that is
processed in the kernel (for example, SIGP SENSE) is run concurrently
with another one is processed in userspace (for example, SIGP STOP).
Being able to provide an honest answer in the SIGP SENSE as to whether
the targeted VCPU is/not stopped is important to provide a consistent
answer while a guest OS is bringing its configuration online.

This version was suggested by David Hildenbrand on v3 [1], where we
examine the target vcpu for a pending STOP/RESTART IRQ while processing
a SIGP order in-kernel, and returning the CC2 if one is in-flight.

Unlike v2-v4 of this RFC, this solution requires no changes to userspace
to exploit a new interface, but a small change is made on the QEMU side
to keep the sequence of events in checks.

Thoughts?

[1] https://lore.kernel.org/r/858e4f2b-d601-a4f1-9e80-8f7838299c9a@redhat.com/

Previous RFCs:
v1: https://lore.kernel.org/r/20211008203112.1979843-1-farman@linux.ibm.com/
v2: https://lore.kernel.org/r/20211102194652.2685098-1-farman@linux.ibm.com/
v3: https://lore.kernel.org/r/20211110203322.1374925-1-farman@linux.ibm.com/
v4: https://lore.kernel.org/r/20211119213707.2363945-1-farman@linux.ibm.com/

Eric Farman (1):
  KVM: s390: Clarify SIGP orders versus STOP/RESTART

 arch/s390/kvm/interrupt.c |  7 +++++++
 arch/s390/kvm/kvm-s390.c  |  9 +++++++--
 arch/s390/kvm/kvm-s390.h  |  1 +
 arch/s390/kvm/sigp.c      | 28 ++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)

-- 
2.32.0

[RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Eric Farman]

With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
and thus are processed in-kernel. The remainder are sent to userspace
with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
(RESTART, STOP, and STOP AND STORE STATUS) have the potential to
inject work back into the kernel, and thus are asynchronous.

Let's look for those pending IRQs when processing one of the in-kernel
SIGP orders, and return BUSY (CC2) if one is in process. This is in
agreement with the Principles of Operation, which states that only one
order can be "active" on a CPU at a time.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c |  7 +++++++
 arch/s390/kvm/kvm-s390.c  |  9 +++++++--
 arch/s390/kvm/kvm-s390.h  |  1 +
 arch/s390/kvm/sigp.c      | 28 ++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 37f47e32d9c4..d339e1c47e4d 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2115,6 +2115,13 @@ int kvm_s390_is_stop_irq_pending(struct kvm_vcpu *vcpu)
 	return test_bit(IRQ_PEND_SIGP_STOP, &li->pending_irqs);
 }
 
+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);
+}
+
 void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu)
 {
 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 5f52e7eec02f..bfdf610bfecb 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4641,10 +4641,15 @@ int kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
+	/*
+	 * Set the VCPU to STOPPED and THEN clear the interrupt flag,
+	 * now that the SIGP STOP and SIGP STOP AND STORE STATUS orders
+	 * have been fully processed. This will ensure that the VCPU
+	 * is kept BUSY if another VCPU is inquiring with SIGP SENSE.
+	 */
+	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
 	kvm_s390_clear_stop_irq(vcpu);
 
-	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
 	__disable_ibs_on_vcpu(vcpu);
 
 	for (i = 0; i < online_vcpus; i++) {
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index c07a050d757d..1876ab0c293f 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -427,6 +427,7 @@ 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_stop_irq_pending(struct kvm_vcpu *vcpu);
+int kvm_s390_is_restart_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,
 			   void __user *buf, int len);
diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 5ad3fb4619f1..c4884de0858b 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -276,6 +276,34 @@ static int handle_sigp_dst(struct kvm_vcpu *vcpu, u8 order_code,
 	if (!dst_vcpu)
 		return SIGP_CC_NOT_OPERATIONAL;
 
+	/*
+	 * SIGP RESTART, SIGP STOP, and SIGP STOP AND STORE STATUS orders
+	 * are processed asynchronously. Until the affected VCPU finishes
+	 * its work and calls back into KVM to clear the (RESTART or STOP)
+	 * interrupt, we need to return any new non-reset orders "busy".
+	 *
+	 * This is important because a single VCPU could issue:
+	 *  1) SIGP STOP $DESTINATION
+	 *  2) SIGP SENSE $DESTINATION
+	 *
+	 * If the SIGP SENSE would not be rejected as "busy", it could
+	 * return an incorrect answer as to whether the VCPU is STOPPED
+	 * or OPERATING.
+	 */
+	if (order_code != SIGP_INITIAL_CPU_RESET &&
+	    order_code != SIGP_CPU_RESET) {
+		/*
+		 * Lockless check. Both SIGP STOP and SIGP (RE)START
+		 * properly synchronize everything while processing
+		 * their orders, while the guest cannot observe a
+		 * difference when issuing other orders from two
+		 * different VCPUs.
+		 */
+		if (kvm_s390_is_stop_irq_pending(dst_vcpu) ||
+		    kvm_s390_is_restart_irq_pending(dst_vcpu))
+			return SIGP_CC_BUSY;
+	}
+
 	switch (order_code) {
 	case SIGP_SENSE:
 		vcpu->stat.instruction_sigp_sense++;
-- 
2.32.0

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Christian Borntraeger]

Am 13.12.21 um 22:05 schrieb Eric Farman:
> With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
> orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
> SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
> and thus are processed in-kernel. The remainder are sent to userspace
> with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
> (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
> inject work back into the kernel, and thus are asynchronous.
> 
> Let's look for those pending IRQs when processing one of the in-kernel
> SIGP orders, and return BUSY (CC2) if one is in process. This is in
> agreement with the Principles of Operation, which states that only one
> order can be "active" on a CPU at a time.

As far as I understand this fixes a real bug with some test tools. Correct?
Then a stable tag might be appropriate.
(Still have to review this)

How hard would it be to also build a kvm-unit test testcase?

> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c |  7 +++++++
>   arch/s390/kvm/kvm-s390.c  |  9 +++++++--
>   arch/s390/kvm/kvm-s390.h  |  1 +
>   arch/s390/kvm/sigp.c      | 28 ++++++++++++++++++++++++++++
>   4 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 37f47e32d9c4..d339e1c47e4d 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2115,6 +2115,13 @@ int kvm_s390_is_stop_irq_pending(struct kvm_vcpu *vcpu)
>   	return test_bit(IRQ_PEND_SIGP_STOP, &li->pending_irqs);
>   }
>   
> +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);
> +}
> +
>   void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu)
>   {
>   	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5f52e7eec02f..bfdf610bfecb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4641,10 +4641,15 @@ int kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>   		}
>   	}
>   
> -	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
> +	/*
> +	 * Set the VCPU to STOPPED and THEN clear the interrupt flag,
> +	 * now that the SIGP STOP and SIGP STOP AND STORE STATUS orders
> +	 * have been fully processed. This will ensure that the VCPU
> +	 * is kept BUSY if another VCPU is inquiring with SIGP SENSE.
> +	 */
> +	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>   	kvm_s390_clear_stop_irq(vcpu);
>   
> -	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>   	__disable_ibs_on_vcpu(vcpu);
>   
>   	for (i = 0; i < online_vcpus; i++) {
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index c07a050d757d..1876ab0c293f 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -427,6 +427,7 @@ 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_stop_irq_pending(struct kvm_vcpu *vcpu);
> +int kvm_s390_is_restart_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,
>   			   void __user *buf, int len);
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index 5ad3fb4619f1..c4884de0858b 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -276,6 +276,34 @@ static int handle_sigp_dst(struct kvm_vcpu *vcpu, u8 order_code,
>   	if (!dst_vcpu)
>   		return SIGP_CC_NOT_OPERATIONAL;
>   
> +	/*
> +	 * SIGP RESTART, SIGP STOP, and SIGP STOP AND STORE STATUS orders
> +	 * are processed asynchronously. Until the affected VCPU finishes
> +	 * its work and calls back into KVM to clear the (RESTART or STOP)
> +	 * interrupt, we need to return any new non-reset orders "busy".
> +	 *
> +	 * This is important because a single VCPU could issue:
> +	 *  1) SIGP STOP $DESTINATION
> +	 *  2) SIGP SENSE $DESTINATION
> +	 *
> +	 * If the SIGP SENSE would not be rejected as "busy", it could
> +	 * return an incorrect answer as to whether the VCPU is STOPPED
> +	 * or OPERATING.
> +	 */
> +	if (order_code != SIGP_INITIAL_CPU_RESET &&
> +	    order_code != SIGP_CPU_RESET) {
> +		/*
> +		 * Lockless check. Both SIGP STOP and SIGP (RE)START
> +		 * properly synchronize everything while processing
> +		 * their orders, while the guest cannot observe a
> +		 * difference when issuing other orders from two
> +		 * different VCPUs.
> +		 */
> +		if (kvm_s390_is_stop_irq_pending(dst_vcpu) ||
> +		    kvm_s390_is_restart_irq_pending(dst_vcpu))
> +			return SIGP_CC_BUSY;
> +	}
> +
>   	switch (order_code) {
>   	case SIGP_SENSE:
>   		vcpu->stat.instruction_sigp_sense++;
> 

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[David Hildenbrand]

On 13.12.21 22:05, Eric Farman wrote:
> With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
> orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
> SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
> and thus are processed in-kernel. The remainder are sent to userspace
> with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
> (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
> inject work back into the kernel, and thus are asynchronous.
> 
> Let's look for those pending IRQs when processing one of the in-kernel
> SIGP orders, and return BUSY (CC2) if one is in process. This is in
> agreement with the Principles of Operation, which states that only one
> order can be "active" on a CPU at a time.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---

In general, LGTM. As raised, with SIGP RESTART there are other cases we
could fix in the kernel, but they are of very low priority IMHO.

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Christian Borntraeger]

Am 15.12.21 um 14:24 schrieb David Hildenbrand:
> On 13.12.21 22:05, Eric Farman wrote:
>> With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
>> orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
>> SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
>> and thus are processed in-kernel. The remainder are sent to userspace
>> with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
>> (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
>> inject work back into the kernel, and thus are asynchronous.
>>
>> Let's look for those pending IRQs when processing one of the in-kernel
>> SIGP orders, and return BUSY (CC2) if one is in process. This is in
>> agreement with the Principles of Operation, which states that only one
>> order can be "active" on a CPU at a time.
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
> 
> In general, LGTM. As raised, with SIGP RESTART there are other cases we
> could fix in the kernel, but they are of very low priority IMHO.

Does that qualify as an RB, assuming that we can fix the other cases later on?

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[David Hildenbrand]

On 15.12.21 14:57, Christian Borntraeger wrote:
> 
> 
> Am 15.12.21 um 14:24 schrieb David Hildenbrand:
>> On 13.12.21 22:05, Eric Farman wrote:
>>> With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
>>> orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
>>> SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
>>> and thus are processed in-kernel. The remainder are sent to userspace
>>> with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
>>> (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
>>> inject work back into the kernel, and thus are asynchronous.
>>>
>>> Let's look for those pending IRQs when processing one of the in-kernel
>>> SIGP orders, and return BUSY (CC2) if one is in process. This is in
>>> agreement with the Principles of Operation, which states that only one
>>> order can be "active" on a CPU at a time.
>>>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>
>> In general, LGTM. As raised, with SIGP RESTART there are other cases we
>> could fix in the kernel, but they are of very low priority IMHO.
> 
> Does that qualify as an RB, assuming that we can fix the other cases later on?
> 

Certainly an Acked-by: David Hildenbrand <david@redhat.com> , to fully
review I need some more time (maybe tomorrow)

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Eric Farman]

On Wed, 2021-12-15 at 14:07 +0100, Christian Borntraeger wrote:
> 
> Am 13.12.21 um 22:05 schrieb Eric Farman:
> > With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
> > orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL
> > CALL,
> > SENSE, and SENSE RUNNING STATUS) which are intended for frequent
> > use
> > and thus are processed in-kernel. The remainder are sent to
> > userspace
> > with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
> > (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
> > inject work back into the kernel, and thus are asynchronous.
> > 
> > Let's look for those pending IRQs when processing one of the in-
> > kernel
> > SIGP orders, and return BUSY (CC2) if one is in process. This is in
> > agreement with the Principles of Operation, which states that only
> > one
> > order can be "active" on a CPU at a time.
> 
> As far as I understand this fixes a real bug with some test tools.
> Correct?

Correct.

> Then a stable tag might be appropriate.

Agreed.

> (Still have to review this)
> 
> How hard would it be to also build a kvm-unit test testcase?

I don't think it's too hard, and something I'd like to see done rather
than the setup I'm using. It's on my list for after the holidays.

> 
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   arch/s390/kvm/interrupt.c |  7 +++++++
> >   arch/s390/kvm/kvm-s390.c  |  9 +++++++--
> >   arch/s390/kvm/kvm-s390.h  |  1 +
> >   arch/s390/kvm/sigp.c      | 28 ++++++++++++++++++++++++++++
> >   4 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > index 37f47e32d9c4..d339e1c47e4d 100644
> > --- a/arch/s390/kvm/interrupt.c
> > +++ b/arch/s390/kvm/interrupt.c
> > @@ -2115,6 +2115,13 @@ int kvm_s390_is_stop_irq_pending(struct
> > kvm_vcpu *vcpu)
> >   	return test_bit(IRQ_PEND_SIGP_STOP, &li->pending_irqs);
> >   }
> >   
> > +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);
> > +}
> > +
> >   void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu)
> >   {
> >   	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 5f52e7eec02f..bfdf610bfecb 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -4641,10 +4641,15 @@ int kvm_s390_vcpu_stop(struct kvm_vcpu
> > *vcpu)
> >   		}
> >   	}
> >   
> > -	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully
> > processed */
> > +	/*
> > +	 * Set the VCPU to STOPPED and THEN clear the interrupt flag,
> > +	 * now that the SIGP STOP and SIGP STOP AND STORE STATUS orders
> > +	 * have been fully processed. This will ensure that the VCPU
> > +	 * is kept BUSY if another VCPU is inquiring with SIGP SENSE.
> > +	 */
> > +	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
> >   	kvm_s390_clear_stop_irq(vcpu);
> >   
> > -	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
> >   	__disable_ibs_on_vcpu(vcpu);
> >   
> >   	for (i = 0; i < online_vcpus; i++) {
> > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> > index c07a050d757d..1876ab0c293f 100644
> > --- a/arch/s390/kvm/kvm-s390.h
> > +++ b/arch/s390/kvm/kvm-s390.h
> > @@ -427,6 +427,7 @@ 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_stop_irq_pending(struct kvm_vcpu *vcpu);
> > +int kvm_s390_is_restart_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,
> >   			   void __user *buf, int len);
> > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> > index 5ad3fb4619f1..c4884de0858b 100644
> > --- a/arch/s390/kvm/sigp.c
> > +++ b/arch/s390/kvm/sigp.c
> > @@ -276,6 +276,34 @@ static int handle_sigp_dst(struct kvm_vcpu
> > *vcpu, u8 order_code,
> >   	if (!dst_vcpu)
> >   		return SIGP_CC_NOT_OPERATIONAL;
> >   
> > +	/*
> > +	 * SIGP RESTART, SIGP STOP, and SIGP STOP AND STORE STATUS
> > orders
> > +	 * are processed asynchronously. Until the affected VCPU
> > finishes
> > +	 * its work and calls back into KVM to clear the (RESTART or
> > STOP)
> > +	 * interrupt, we need to return any new non-reset orders
> > "busy".
> > +	 *
> > +	 * This is important because a single VCPU could issue:
> > +	 *  1) SIGP STOP $DESTINATION
> > +	 *  2) SIGP SENSE $DESTINATION
> > +	 *
> > +	 * If the SIGP SENSE would not be rejected as "busy", it could
> > +	 * return an incorrect answer as to whether the VCPU is STOPPED
> > +	 * or OPERATING.
> > +	 */
> > +	if (order_code != SIGP_INITIAL_CPU_RESET &&
> > +	    order_code != SIGP_CPU_RESET) {
> > +		/*
> > +		 * Lockless check. Both SIGP STOP and SIGP (RE)START
> > +		 * properly synchronize everything while processing
> > +		 * their orders, while the guest cannot observe a
> > +		 * difference when issuing other orders from two
> > +		 * different VCPUs.
> > +		 */
> > +		if (kvm_s390_is_stop_irq_pending(dst_vcpu) ||
> > +		    kvm_s390_is_restart_irq_pending(dst_vcpu))
> > +			return SIGP_CC_BUSY;
> > +	}
> > +
> >   	switch (order_code) {
> >   	case SIGP_SENSE:
> >   		vcpu->stat.instruction_sigp_sense++;
> > 

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Eric Farman]

On Wed, 2021-12-15 at 15:02 +0100, David Hildenbrand wrote:
> On 15.12.21 14:57, Christian Borntraeger wrote:
> > 
> > Am 15.12.21 um 14:24 schrieb David Hildenbrand:
> > > On 13.12.21 22:05, Eric Farman wrote:
> > > > With KVM_CAP_S390_USER_SIGP, there are only five Signal
> > > > Processor
> > > > orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL,
> > > > EXTERNAL CALL,
> > > > SENSE, and SENSE RUNNING STATUS) which are intended for
> > > > frequent use
> > > > and thus are processed in-kernel. The remainder are sent to
> > > > userspace
> > > > with the KVM_CAP_S390_USER_SIGP capability. Of those, three
> > > > orders
> > > > (RESTART, STOP, and STOP AND STORE STATUS) have the potential
> > > > to
> > > > inject work back into the kernel, and thus are asynchronous.
> > > > 
> > > > Let's look for those pending IRQs when processing one of the
> > > > in-kernel
> > > > SIGP orders, and return BUSY (CC2) if one is in process. This
> > > > is in
> > > > agreement with the Principles of Operation, which states that
> > > > only one
> > > > order can be "active" on a CPU at a time.
> > > > 
> > > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > ---
> > > 
> > > In general, LGTM. As raised, with SIGP RESTART there are other
> > > cases we
> > > could fix in the kernel, but they are of very low priority IMHO.
> > 
> > Does that qualify as an RB, assuming that we can fix the other
> > cases later on?
> > 
> 
> Certainly an Acked-by: David Hildenbrand <david@redhat.com> , to
> fully
> review I need some more time (maybe tomorrow)
> 

I have the list you'd provided for the other orders on my todo list;
wasn't having a lot of success building a testcase that would
prove/disprove the situation we were in, besides the orders described
here.

(I had previously mentioned the CPU RESET orders, but I'm worried that
that was an error in my setup based on debugging I have been doing
lately.)

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Christian Borntraeger]

Am 13.12.21 um 22:05 schrieb Eric Farman:
> With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
> orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
> SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
> and thus are processed in-kernel. The remainder are sent to userspace
> with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
> (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
> inject work back into the kernel, and thus are asynchronous.
> 
> Let's look for those pending IRQs when processing one of the in-kernel
> SIGP orders, and return BUSY (CC2) if one is in process. This is in
> agreement with the Principles of Operation, which states that only one
> order can be "active" on a CPU at a time.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/kvm/interrupt.c |  7 +++++++
>   arch/s390/kvm/kvm-s390.c  |  9 +++++++--
>   arch/s390/kvm/kvm-s390.h  |  1 +
>   arch/s390/kvm/sigp.c      | 28 ++++++++++++++++++++++++++++
>   4 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 37f47e32d9c4..d339e1c47e4d 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2115,6 +2115,13 @@ int kvm_s390_is_stop_irq_pending(struct kvm_vcpu *vcpu)
>   	return test_bit(IRQ_PEND_SIGP_STOP, &li->pending_irqs);
>   }
>   
> +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);
> +}
> +
>   void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu)
>   {
>   	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5f52e7eec02f..bfdf610bfecb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4641,10 +4641,15 @@ int kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>   		}
>   	}
>   
> -	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
> +	/*
> +	 * Set the VCPU to STOPPED and THEN clear the interrupt flag,
> +	 * now that the SIGP STOP and SIGP STOP AND STORE STATUS orders
> +	 * have been fully processed. This will ensure that the VCPU
> +	 * is kept BUSY if another VCPU is inquiring with SIGP SENSE.
> +	 */
> +	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>   	kvm_s390_clear_stop_irq(vcpu);
>   
> -	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>   	__disable_ibs_on_vcpu(vcpu);
>   
>   	for (i = 0; i < online_vcpus; i++) {
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index c07a050d757d..1876ab0c293f 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -427,6 +427,7 @@ 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_stop_irq_pending(struct kvm_vcpu *vcpu);
> +int kvm_s390_is_restart_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,
>   			   void __user *buf, int len);
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index 5ad3fb4619f1..c4884de0858b 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -276,6 +276,34 @@ static int handle_sigp_dst(struct kvm_vcpu *vcpu, u8 order_code,
>   	if (!dst_vcpu)
>   		return SIGP_CC_NOT_OPERATIONAL;
>   
> +	/*
> +	 * SIGP RESTART, SIGP STOP, and SIGP STOP AND STORE STATUS orders
> +	 * are processed asynchronously. Until the affected VCPU finishes
> +	 * its work and calls back into KVM to clear the (RESTART or STOP)
> +	 * interrupt, we need to return any new non-reset orders "busy".
> +	 *
> +	 * This is important because a single VCPU could issue:
> +	 *  1) SIGP STOP $DESTINATION
> +	 *  2) SIGP SENSE $DESTINATION
> +	 *
> +	 * If the SIGP SENSE would not be rejected as "busy", it could
> +	 * return an incorrect answer as to whether the VCPU is STOPPED
> +	 * or OPERATING.
> +	 */
> +	if (order_code != SIGP_INITIAL_CPU_RESET &&
> +	    order_code != SIGP_CPU_RESET) {
> +		/*
> +		 * Lockless check. Both SIGP STOP and SIGP (RE)START
> +		 * properly synchronize everything while processing
> +		 * their orders, while the guest cannot observe a
> +		 * difference when issuing other orders from two
> +		 * different VCPUs.
> +		 */
> +		if (kvm_s390_is_stop_irq_pending(dst_vcpu) ||
> +		    kvm_s390_is_restart_irq_pending(dst_vcpu))
> +			return SIGP_CC_BUSY;
> +	}
> +

Kind of similar to v1 but much simpler
took me a while to review, but I think you are right, no need for taking the lock.

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>


>   	switch (order_code) {
>   	case SIGP_SENSE:
>   		vcpu->stat.instruction_sigp_sense++;
> 

Re: [RFC PATCH v5 1/1] KVM: s390: Clarify SIGP orders versus STOP/RESTART

[Christian Borntraeger]

Am 13.12.21 um 22:05 schrieb Eric Farman:
> With KVM_CAP_S390_USER_SIGP, there are only five Signal Processor
> orders (CONDITIONAL EMERGENCY SIGNAL, EMERGENCY SIGNAL, EXTERNAL CALL,
> SENSE, and SENSE RUNNING STATUS) which are intended for frequent use
> and thus are processed in-kernel. The remainder are sent to userspace
> with the KVM_CAP_S390_USER_SIGP capability. Of those, three orders
> (RESTART, STOP, and STOP AND STORE STATUS) have the potential to
> inject work back into the kernel, and thus are asynchronous.
> 
> Let's look for those pending IRQs when processing one of the in-kernel
> SIGP orders, and return BUSY (CC2) if one is in process. This is in
> agreement with the Principles of Operation, which states that only one
> order can be "active" on a CPU at a time.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

I have applied this and will queue for next (but with cc stable).

> ---
>   arch/s390/kvm/interrupt.c |  7 +++++++
>   arch/s390/kvm/kvm-s390.c  |  9 +++++++--
>   arch/s390/kvm/kvm-s390.h  |  1 +
>   arch/s390/kvm/sigp.c      | 28 ++++++++++++++++++++++++++++
>   4 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 37f47e32d9c4..d339e1c47e4d 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2115,6 +2115,13 @@ int kvm_s390_is_stop_irq_pending(struct kvm_vcpu *vcpu)
>   	return test_bit(IRQ_PEND_SIGP_STOP, &li->pending_irqs);
>   }
>   
> +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);
> +}
> +
>   void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu)
>   {
>   	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5f52e7eec02f..bfdf610bfecb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4641,10 +4641,15 @@ int kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>   		}
>   	}
>   
> -	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
> +	/*
> +	 * Set the VCPU to STOPPED and THEN clear the interrupt flag,
> +	 * now that the SIGP STOP and SIGP STOP AND STORE STATUS orders
> +	 * have been fully processed. This will ensure that the VCPU
> +	 * is kept BUSY if another VCPU is inquiring with SIGP SENSE.
> +	 */
> +	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>   	kvm_s390_clear_stop_irq(vcpu);
>   
> -	kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>   	__disable_ibs_on_vcpu(vcpu);
>   
>   	for (i = 0; i < online_vcpus; i++) {
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index c07a050d757d..1876ab0c293f 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -427,6 +427,7 @@ 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_stop_irq_pending(struct kvm_vcpu *vcpu);
> +int kvm_s390_is_restart_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,
>   			   void __user *buf, int len);
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index 5ad3fb4619f1..c4884de0858b 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -276,6 +276,34 @@ static int handle_sigp_dst(struct kvm_vcpu *vcpu, u8 order_code,
>   	if (!dst_vcpu)
>   		return SIGP_CC_NOT_OPERATIONAL;
>   
> +	/*
> +	 * SIGP RESTART, SIGP STOP, and SIGP STOP AND STORE STATUS orders
> +	 * are processed asynchronously. Until the affected VCPU finishes
> +	 * its work and calls back into KVM to clear the (RESTART or STOP)
> +	 * interrupt, we need to return any new non-reset orders "busy".
> +	 *
> +	 * This is important because a single VCPU could issue:
> +	 *  1) SIGP STOP $DESTINATION
> +	 *  2) SIGP SENSE $DESTINATION
> +	 *
> +	 * If the SIGP SENSE would not be rejected as "busy", it could
> +	 * return an incorrect answer as to whether the VCPU is STOPPED
> +	 * or OPERATING.
> +	 */
> +	if (order_code != SIGP_INITIAL_CPU_RESET &&
> +	    order_code != SIGP_CPU_RESET) {
> +		/*
> +		 * Lockless check. Both SIGP STOP and SIGP (RE)START
> +		 * properly synchronize everything while processing
> +		 * their orders, while the guest cannot observe a
> +		 * difference when issuing other orders from two
> +		 * different VCPUs.
> +		 */
> +		if (kvm_s390_is_stop_irq_pending(dst_vcpu) ||
> +		    kvm_s390_is_restart_irq_pending(dst_vcpu))
> +			return SIGP_CC_BUSY;
> +	}
> +
>   	switch (order_code) {
>   	case SIGP_SENSE:
>   		vcpu->stat.instruction_sigp_sense++;
>