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

[Eric Farman <farman@linux.ibm.com>]

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