On Thu, 2021-11-04 at 09:23 +0100, David Hildenbrand wrote: > 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 ?
Agreed. > > > +{ > > + 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? Can I? Most of the orders in that routine are invoked via "run_on_cpu(CPU(dst_cpu), ..." dispatching them to other vcpus, so I presumed that was a stacked task. But of course, that doesn't make a lot of sense, since it's holding that sigp lock across the duration, so it must be a vcpu switch instead. Not sure what I was thinking at the time, so I'll give this a try. > > 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. > Agreed, this was some of the refactoring that I had in mind looking at this mindboggling patch. I would love it if sigp_order weren't limited to the STOP and STOP AND STORE STATUS orders, but I hesitate to mess with that too much, for fear of ripples across the system. > > > > > > > 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? Ah, yes... I got caught up in the "this is a global lock so another vcpu must be doing a sigp" side of things, and relying on the kernel to make the determination that "vcpuN is already processing an order, don't send it another one." > > > 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) > > > >