Re: [RFC PATCH v3 2/2] s390x: Implement the USER_SIGP_BUSY capability

[David Hildenbrand]

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