All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v3 0/2] s390x: Improvements to SIGP handling [QEMU]
@ 2021-11-10 20:45 Eric Farman
  2021-11-10 20:45 ` [RFC PATCH v3 1/2] Temporary linux-headers update Eric Farman
  2021-11-10 20:45 ` [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability Eric Farman
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Farman @ 2021-11-10 20:45 UTC (permalink / raw)
  To: Halil Pasic, Cornelia Huck, Christian Borntraeger, Thomas Huth,
	David Hildenbrand
  Cc: Eric Farman, qemu-s390x, Richard Henderson, qemu-devel

Here is an update to the SIGP handling series, to correspond to
version 3 of the KVM series [1].

The main changes here from v2 are the simplified use of
s390_cpu_reset_sigp_busy() throughout the SIGP handlers,
a new invocation of it from s390_cpu_reset(), and the
implementation of the "set sigp busy" IOCTL.

[1] https://lore.kernel.org/r/20211110203322.1374925-1-farman@linux.ibm.com/

Previous RFCs:
v1: https://lore.kernel.org/r/20211008203811.1980478-1-farman@linux.ibm.com/
v2: https://lore.kernel.org/r/20211102201122.3188108-1-farman@linux.ibm.com/

Eric Farman (2):
  Temporary linux-headers update
  s390x: Implement the USER_SIGP_BUSY capability

 linux-headers/linux/kvm.h    |  5 +++++
 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 ++++++++++++++++++-
 7 files changed, 65 insertions(+), 1 deletion(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RFC PATCH v3 1/2] Temporary linux-headers update
  2021-11-10 20:45 [RFC PATCH v3 0/2] s390x: Improvements to SIGP handling [QEMU] Eric Farman
@ 2021-11-10 20:45 ` Eric Farman
  2021-11-10 20:45 ` [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability Eric Farman
  1 sibling, 0 replies; 5+ messages in thread
From: Eric Farman @ 2021-11-10 20:45 UTC (permalink / raw)
  To: Halil Pasic, Cornelia Huck, Christian Borntraeger, Thomas Huth,
	David Hildenbrand
  Cc: Eric Farman, qemu-s390x, Richard Henderson, qemu-devel

This should be replaced with ./scripts/update-linux-headers.sh

But this is enough for the RFC.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 linux-headers/linux/kvm.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index bcaf66cc4d..997296e07a 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_BINARY_STATS_FD 203
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
+#define KVM_CAP_S390_USER_SIGP_BUSY 206
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -2004,4 +2005,8 @@ struct kvm_stats_desc {
 
 #define KVM_GET_STATS_FD  _IO(KVMIO,  0xce)
 
+/* Available with KVM_CAP_S390_USER_SIGP_BUSY */
+#define KVM_S390_VCPU_RESET_SIGP_BUSY       _IO(KVMIO, 0xcf)
+#define KVM_S390_VCPU_SET_SIGP_BUSY         _IO(KVMIO, 0xd0)
+
 #endif /* __LINUX_KVM_H */
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability
  2021-11-10 20:45 [RFC PATCH v3 0/2] s390x: Improvements to SIGP handling [QEMU] Eric Farman
  2021-11-10 20:45 ` [RFC PATCH v3 1/2] Temporary linux-headers update Eric Farman
@ 2021-11-10 20:45 ` Eric Farman
  2021-11-11  9:01   ` David Hildenbrand
  1 sibling, 1 reply; 5+ messages in thread
From: Eric Farman @ 2021-11-10 20:45 UTC (permalink / raw)
  To: Halil Pasic, Cornelia Huck, Christian Borntraeger, Thomas Huth,
	David Hildenbrand
  Cc: Eric Farman, qemu-s390x, Richard Henderson, qemu-devel

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(-)

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)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability
  2021-11-10 20:45 ` [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability Eric Farman
@ 2021-11-11  9:01   ` David Hildenbrand
  2021-11-19 21:12     ` Eric Farman
  0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2021-11-11  9:01 UTC (permalink / raw)
  To: Eric Farman, Halil Pasic, Cornelia Huck, Christian Borntraeger,
	Thomas Huth
  Cc: qemu-s390x, Richard Henderson, qemu-devel

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.

-- 
Thanks,

David / dhildenb



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability
  2021-11-11  9:01   ` David Hildenbrand
@ 2021-11-19 21:12     ` Eric Farman
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Farman @ 2021-11-19 21:12 UTC (permalink / raw)
  To: David Hildenbrand, Halil Pasic, Cornelia Huck,
	Christian Borntraeger, Thomas Huth
  Cc: qemu-s390x, Richard Henderson, qemu-devel

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.
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-11-19 21:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 20:45 [RFC PATCH v3 0/2] s390x: Improvements to SIGP handling [QEMU] Eric Farman
2021-11-10 20:45 ` [RFC PATCH v3 1/2] Temporary linux-headers update Eric Farman
2021-11-10 20:45 ` [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability Eric Farman
2021-11-11  9:01   ` David Hildenbrand
2021-11-19 21:12     ` Eric Farman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.