On Thu, 2021-11-11 at 10:01 +0100, David Hildenbrand wrote: > On 10.11.21 21:45, 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, userspace can tell the kernel > > when > > it is started processing a SIGP order and when it has finished, > > such that > > the in-kernel orders can be returned with the BUSY condition > > between the > > two IOCTLs. > > > > Let's use the new capability in QEMU. > > This looks much better. A suggestion on how to make it even simpler > below. > > > } > > si->cc = SIGP_CC_ORDER_CODE_ACCEPTED; > > @@ -375,6 +378,10 @@ static int handle_sigp_single_dst(S390CPU > > *cpu, S390CPU *dst_cpu, uint8_t order, > > return SIGP_CC_BUSY; > > } > > > > + if (s390_cpu_set_sigp_busy(dst_cpu) == -EBUSY) { > > + return SIGP_CC_BUSY; > > + } > > I'd assume we want something like this instead:
Hi David, My apologies, this suggestion got lost and I only noticed it while updating the cover-letter for v4. I do like these ideas and need to spend some time trying them, so am making a note to revisit once the interface settles down. Cheers, Eric > > --- a/target/s390x/sigp.c > +++ b/target/s390x/sigp.c > @@ -355,6 +355,12 @@ static void sigp_sense_running(S390CPU *dst_cpu, > SigpInfo *si) > } > } > > +static bool sigp_dst_is_busy(S390CPU *dst_cpu) > +{ > + return dst_cpu->env.sigp_order != 0 || > + s390_cpu_set_sigp_busy(dst_cpu) == -EBUSY; > +} > + > static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu, > uint8_t order, > uint64_t param, uint64_t > *status_reg) > { > @@ -369,7 +375,7 @@ static int handle_sigp_single_dst(S390CPU *cpu, > S390CPU *dst_cpu, uint8_t order, > } > > /* only resets can break pending orders */ > - if (dst_cpu->env.sigp_order != 0 && > + if (sigp_dst_is_busy(dst_cpu) && > order != SIGP_CPU_RESET && > order != SIGP_INITIAL_CPU_RESET) { > return SIGP_CC_BUSY; > > > > > But as raised, it might be good enough (and simpler) to > special-case SIGP STOP * only, because pending SIGP STOP that could > take > forever and is processed asynchronously is AFAIU the only real case > that's > problematic. We'll also have to handle the migration case with SIGP > STOP, > so below would be my take (excluding the KVM parts from your patch) > > > > diff --git a/slirp b/slirp > --- a/slirp > +++ b/slirp > @@ -1 +1 @@ > -Subproject commit a88d9ace234a24ce1c17189642ef9104799425e0 > +Subproject commit a88d9ace234a24ce1c17189642ef9104799425e0-dirty > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index ccdbaf84d5..6ead62d1fd 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -114,7 +114,7 @@ static void s390_cpu_reset(CPUState *s, > cpu_reset_type type) > DeviceState *dev = DEVICE(s); > > scc->parent_reset(dev); > - cpu->env.sigp_order = 0; > + s390_cpu_set_sigp_busy(cpu, 0); > s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); > > switch (type) { > diff --git a/target/s390x/machine.c b/target/s390x/machine.c > index 37a076858c..d4ad2534a5 100644 > --- a/target/s390x/machine.c > +++ b/target/s390x/machine.c > @@ -41,6 +41,14 @@ static int cpu_post_load(void *opaque, int > version_id) > tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL); > } > > + /* > + * Make sure KVM is aware of the BUSY SIGP order, reset it the > official > + * way. > + */ > + if (cpu->env.sigp_order) { > + s390_cpu_set_sigp_busy(cpu, cpu->env.sigp_order); > + } > + > return 0; > } > > diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x- > internal.h > index 1a178aed41..690cadef77 100644 > --- a/target/s390x/s390x-internal.h > +++ b/target/s390x/s390x-internal.h > @@ -402,6 +402,7 @@ void s390x_translate_init(void); > > > /* sigp.c */ > +void s390_cpu_set_sigp_busy(S390CPU *cpu, uint8_t sigp_order); > int handle_sigp(CPUS390XState *env, uint8_t order, uint64_t r1, > uint64_t r3); > void do_stop_interrupt(CPUS390XState *env); > > diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c > index 51c727834c..9640267124 100644 > --- a/target/s390x/sigp.c > +++ b/target/s390x/sigp.c > @@ -27,6 +27,33 @@ typedef struct SigpInfo { > uint64_t *status_reg; > } SigpInfo; > > +void s390_cpu_set_sigp_busy(S390CPU *cpu, uint8_t sigp_order) > +{ > + /* > + * For now we only expect one of these orders that are processed > + * asynchronously, or clearing the busy order. > + */ > + g_assert(sigp_order == SIGP_STOP || sigp_order == > SIGP_STOP_STORE_STATUS || > + !sigp_order); > + if (kvm_enabled()) { > + /* > + * Note: We're the only ones setting/resetting a CPU in KVM > busy, and > + * we always update the state in KVM when updating the state > + * (cpu->env.sigp_order) in QEMU. > + */ > + if (sigp_order) > + kvm_s390_vcpu_set_sigp_busy(cpu); > + else > + kvm_s390_vcpu_reset_sigp_busy(cpu); > + } > + cpu->env.sigp_order = sigp_order; > +} > + > +static bool s390x_cpu_is_sigp_busy(S390CPU *cpu) > +{ > + return cpu->env.sigp_order != 0; > +} > + > static void set_sigp_status(SigpInfo *si, uint64_t status) > { > *si->status_reg &= 0xffffffff00000000ULL; > @@ -119,7 +146,7 @@ static void sigp_stop(CPUState *cs, > run_on_cpu_data arg) > s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); > } else { > /* execute the stop function */ > - cpu->env.sigp_order = SIGP_STOP; > + s390_cpu_set_sigp_busy(cpu, SIGP_STOP); > cpu_inject_stop(cpu); > } > si->cc = SIGP_CC_ORDER_CODE_ACCEPTED; > @@ -137,7 +164,7 @@ static void sigp_stop_and_store_status(CPUState > *cs, run_on_cpu_data arg) > > switch (s390_cpu_get_state(cpu)) { > case S390_CPU_STATE_OPERATING: > - cpu->env.sigp_order = SIGP_STOP_STORE_STATUS; > + s390_cpu_set_sigp_busy(cpu, SIGP_STOP_STORE_STATUS); > cpu_inject_stop(cpu); > /* store will be performed in do_stop_interrup() */ > break; > @@ -369,7 +396,7 @@ static int handle_sigp_single_dst(S390CPU *cpu, > S390CPU *dst_cpu, uint8_t order, > } > > /* only resets can break pending orders */ > - if (dst_cpu->env.sigp_order != 0 && > + if (s390x_cpu_is_sigp_busy(dst_cpu) && > order != SIGP_CPU_RESET && > order != SIGP_INITIAL_CPU_RESET) { > return SIGP_CC_BUSY; > @@ -485,7 +512,7 @@ void do_stop_interrupt(CPUS390XState *env) > if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) { > s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true); > } > - env->sigp_order = 0; > + s390_cpu_set_sigp_busy(cpu, 0); > env->pending_int &= ~INTERRUPT_STOP; > } > > > > We can optimize in s390_cpu_set_sigp_busy() to not trigger an ioctl > if not necessary based on the old value of env->sigp_order. Only the > migration path needs some tweaking then. >