| Message ID | 20211110204528.1378524-3-farman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Improvements to SIGP handling [QEMU] | expand |
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: --- 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.
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. >
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c index 5471e01ee8..60dff5bcd9 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -254,6 +254,21 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu) return s390_count_running_cpus(); } +int s390_cpu_set_sigp_busy(S390CPU *cpu) +{ + if (kvm_enabled()) { + return kvm_s390_vcpu_set_sigp_busy(cpu); + } + return 0; +} + +void s390_cpu_reset_sigp_busy(S390CPU *cpu) +{ + if (kvm_enabled()) { + kvm_s390_vcpu_reset_sigp_busy(cpu); + } +} + int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit) { if (kvm_enabled()) { diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 7b7b05f1d3..b5fdca05cf 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -115,6 +115,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) scc->parent_reset(dev); cpu->env.sigp_order = 0; + s390_cpu_reset_sigp_busy(cpu); s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); switch (type) { diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index ca3845d023..ef3d3a5b10 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -780,11 +780,19 @@ int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id, int vq, bool assign); #ifndef CONFIG_USER_ONLY unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu); +int s390_cpu_set_sigp_busy(S390CPU *cpu); +void s390_cpu_reset_sigp_busy(S390CPU *cpu); #else static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu) { return 0; } +static inline int s390_cpu_set_sigp_busy(S390CPU *cpu) +{ +} +static inline void s390_cpu_reset_sigp_busy(S390CPU *cpu) +{ +} #endif /* CONFIG_USER_ONLY */ static inline uint8_t s390_cpu_get_state(S390CPU *cpu) { diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index 5b1fdb55c4..533747de34 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -360,6 +360,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) cap_protected = kvm_check_extension(s, KVM_CAP_S390_PROTECTED); kvm_vm_enable_cap(s, KVM_CAP_S390_USER_SIGP, 0); + kvm_vm_enable_cap(s, KVM_CAP_S390_USER_SIGP_BUSY, 0); kvm_vm_enable_cap(s, KVM_CAP_S390_VECTOR_REGISTERS, 0); kvm_vm_enable_cap(s, KVM_CAP_S390_USER_STSI, 0); if (ri_allowed()) { @@ -2558,6 +2559,21 @@ void kvm_s390_stop_interrupt(S390CPU *cpu) kvm_s390_vcpu_interrupt(cpu, &irq); } +int kvm_s390_vcpu_set_sigp_busy(S390CPU *cpu) +{ + CPUState *cs = CPU(cpu); + + return kvm_vcpu_ioctl(cs, KVM_S390_VCPU_SET_SIGP_BUSY); +} + +void kvm_s390_vcpu_reset_sigp_busy(S390CPU *cpu) +{ + 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; diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h index 05a5e1e6f4..de148b68c4 100644 --- a/target/s390x/kvm/kvm_s390x.h +++ b/target/s390x/kvm/kvm_s390x.h @@ -45,5 +45,7 @@ void kvm_s390_crypto_reset(void); void kvm_s390_restart_interrupt(S390CPU *cpu); void kvm_s390_stop_interrupt(S390CPU *cpu); void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info); +int kvm_s390_vcpu_set_sigp_busy(S390CPU *cpu); +void kvm_s390_vcpu_reset_sigp_busy(S390CPU *cpu); #endif /* KVM_S390X_H */ diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c index 51c727834c..8f191df42a 100644 --- a/target/s390x/sigp.c +++ b/target/s390x/sigp.c @@ -111,12 +111,14 @@ static void sigp_stop(CPUState *cs, run_on_cpu_data arg) if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) { si->cc = SIGP_CC_ORDER_CODE_ACCEPTED; + s390_cpu_reset_sigp_busy(cpu); return; } /* disabled wait - sleeping in user space */ if (cs->halted) { s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); + s390_cpu_reset_sigp_busy(cpu); } else { /* execute the stop function */ cpu->env.sigp_order = SIGP_STOP; @@ -139,12 +141,13 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg) case S390_CPU_STATE_OPERATING: cpu->env.sigp_order = SIGP_STOP_STORE_STATUS; cpu_inject_stop(cpu); - /* store will be performed in do_stop_interrup() */ + /* store will be performed in do_stop_interrupt() */ break; case S390_CPU_STATE_STOPPED: /* already stopped, just store the status */ cpu_synchronize_state(cs); s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true); + s390_cpu_reset_sigp_busy(cpu); break; } 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; + } + switch (order) { case SIGP_SENSE: sigp_sense(dst_cpu, &si); @@ -422,6 +429,15 @@ static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu, uint8_t order, set_sigp_status(&si, SIGP_STAT_INVALID_ORDER); } + switch (order) { + case SIGP_STOP: + case SIGP_STOP_STORE_STATUS: + /* These orders will clean up the indicator when they are finished */ + break; + default: + s390_cpu_reset_sigp_busy(dst_cpu); + } + return si.cc; } @@ -487,6 +503,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)
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. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- target/s390x/cpu-sysemu.c | 15 +++++++++++++++ target/s390x/cpu.c | 1 + target/s390x/cpu.h | 8 ++++++++ target/s390x/kvm/kvm.c | 16 ++++++++++++++++ target/s390x/kvm/kvm_s390x.h | 2 ++ target/s390x/sigp.c | 19 ++++++++++++++++++- 6 files changed, 60 insertions(+), 1 deletion(-)