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@xxxxxxxxxx>
> > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> > ---
> > 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++;
> >
