On 02.11.21 21:11, Eric Farman wrote: > With the USER_SIGP capability, the kernel will pass most (but not all) > SIGP orders to userspace for processing. But that means that the kernel > is unable to determine if/when the order has been completed by userspace, > and could potentially return an incorrect answer (CC1 with status bits > versus CC2 indicating BUSY) for one of the remaining in-kernel orders. > > With a new USER_SIGP_BUSY capability, the kernel will automatically > flag a vcpu as busy for a SIGP order, and block further orders directed > to the same vcpu until userspace resets it to indicate the order has > been fully processed. > > Let's use the new capability in QEMU. > > Signed-off-by: Eric Farman <far...@linux.ibm.com>
[...] > +void kvm_s390_vcpu_reset_busy(S390CPU *cpu) kvm_s390_vcpu_reset_sigp_busy ? > +{ > + CPUState *cs = CPU(cpu); > + > + /* Don't care about the response from this */ > + kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET_SIGP_BUSY); > +} > + > bool kvm_arch_cpu_check_are_resettable(void) > { > return true; [...] > static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si) > @@ -338,12 +367,14 @@ static void sigp_sense_running(S390CPU *dst_cpu, > SigpInfo *si) > if (!tcg_enabled()) { > /* handled in KVM */ > set_sigp_status(si, SIGP_STAT_INVALID_ORDER); > + s390_cpu_reset_sigp_busy(dst_cpu); > return; > } > > /* sensing without locks is racy, but it's the same for real hw */ > if (!s390_has_feat(S390_FEAT_SENSE_RUNNING_STATUS)) { > set_sigp_status(si, SIGP_STAT_INVALID_ORDER); > + s390_cpu_reset_sigp_busy(dst_cpu); > return; > } > > @@ -353,6 +384,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo > *si) > } else { > si->cc = SIGP_CC_ORDER_CODE_ACCEPTED; > } > + s390_cpu_reset_sigp_busy(dst_cpu); > } Can't we call s390_cpu_reset_sigp_busy() directly from handle_sigp_single_dst(), after the handle_sigp_single_dst() call? IIRC we could clear it in case cpu->env.sigp_order wasn't set. Otherwise, we'll have to clear it once we clear cpu->env.sigp_order -- e.g., in do_stop_interrupt(), but also during s390_cpu_reset(). We could have a helper function that sets cpu->env.sigp_order = 0 and clears the busy indication. > > static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu, uint8_t > order, > @@ -420,6 +452,7 @@ static int handle_sigp_single_dst(S390CPU *cpu, S390CPU > *dst_cpu, uint8_t order, > break; > default: > set_sigp_status(&si, SIGP_STAT_INVALID_ORDER); > + s390_cpu_reset_sigp_busy(dst_cpu); > } > > return si.cc; > @@ -444,6 +477,12 @@ int handle_sigp(CPUS390XState *env, uint8_t order, > uint64_t r1, uint64_t r3) > int ret; > Maybe rather lookup the dst once: if (order != SIGP_SET_ARCH) { /* all other sigp orders target a single vcpu */ dst_cpu = s390_cpu_addr2state(env->regs[r3]); } if (qemu_mutex_trylock(&qemu_sigp_mutex)) { if (dst_cpu) { s390_cpu_reset_sigp_busy(dst_cpu); } ret = SIGP_CC_BUSY; goto out; } switch (order) { case SIGP_SET_ARCH: ret = sigp_set_architecture(cpu, param, status_reg); break; default: ret = handle_sigp_single_dst(cpu, dst_cpu, order, param, status_reg); } BUT, I wonder if this is fully correct. Can't it happen that another CPU is currently processing an order for that very same dst_cpu and you would clear SIGP busy of that cpu prematurely? > if (qemu_mutex_trylock(&qemu_sigp_mutex)) { > + if (order != SIGP_SET_ARCH) { > + dst_cpu = s390_cpu_addr2state(env->regs[r3]); > + if (dst_cpu) { > + s390_cpu_reset_sigp_busy(dst_cpu); > + } > + } > ret = SIGP_CC_BUSY; > goto out; > } > @@ -487,6 +526,7 @@ void do_stop_interrupt(CPUS390XState *env) > } > env->sigp_order = 0; > env->pending_int &= ~INTERRUPT_STOP; > + s390_cpu_reset_sigp_busy(cpu); > } > > void s390_init_sigp(void) > -- Thanks, David / dhildenb