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

[Eric Farman]

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