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

[Eric Farman]

On Thu, 2021-11-04 at 09:23 +0100, David Hildenbrand wrote:
> On 02.11.21 21:11, 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, the kernel will automatically
> > flag a vcpu as busy for a SIGP order, and block further orders
> > directed
> > to the same vcpu until userspace resets it to indicate the order
> > has
> > been fully processed.
> > 
> > Let's use the new capability in QEMU.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> [...]
> 
> > +void kvm_s390_vcpu_reset_busy(S390CPU *cpu)
> 
> kvm_s390_vcpu_reset_sigp_busy ?

Agreed.

> 
> > +{
> > +    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;
> 
> [...]
> 
> >  static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si)
> > @@ -338,12 +367,14 @@ static void sigp_sense_running(S390CPU
> > *dst_cpu, SigpInfo *si)
> >      if (!tcg_enabled()) {
> >          /* handled in KVM */
> >          set_sigp_status(si, SIGP_STAT_INVALID_ORDER);
> > +        s390_cpu_reset_sigp_busy(dst_cpu);
> >          return;
> >      }
> >  
> >      /* sensing without locks is racy, but it's the same for real
> > hw */
> >      if (!s390_has_feat(S390_FEAT_SENSE_RUNNING_STATUS)) {
> >          set_sigp_status(si, SIGP_STAT_INVALID_ORDER);
> > +        s390_cpu_reset_sigp_busy(dst_cpu);
> >          return;
> >      }
> >  
> > @@ -353,6 +384,7 @@ static void sigp_sense_running(S390CPU
> > *dst_cpu, SigpInfo *si)
> >      } else {
> >          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
> >      }
> > +    s390_cpu_reset_sigp_busy(dst_cpu);
> >  }
> 
> Can't we call s390_cpu_reset_sigp_busy() directly from
> handle_sigp_single_dst(),
> after the handle_sigp_single_dst() call?

Can I? Most of the orders in that routine are invoked via
"run_on_cpu(CPU(dst_cpu), ..." dispatching them to other vcpus, so I
presumed that was a stacked task. But of course, that doesn't make a
lot of sense, since it's holding that sigp lock across the duration, so
it must be a vcpu switch instead. Not sure what I was thinking at the
time, so I'll give this a try.

> 
> IIRC we could clear it in case cpu->env.sigp_order wasn't set.
> Otherwise,
> we'll have to clear it once we clear cpu->env.sigp_order -- e.g., in
> do_stop_interrupt(), but
> also during s390_cpu_reset().
> 
> We could have a helper function that sets cpu->env.sigp_order = 0 and
> clears the busy indication.
> 

Agreed, this was some of the refactoring that I had in mind looking at
this mindboggling patch.

I would love it if sigp_order weren't limited to the STOP and STOP AND
STORE STATUS orders, but I hesitate to mess with that too much, for
fear of ripples across the system.

> 
> 
> 
> >  
> >  static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu,
> > uint8_t order,
> > @@ -420,6 +452,7 @@ static int handle_sigp_single_dst(S390CPU *cpu,
> > S390CPU *dst_cpu, uint8_t order,
> >          break;
> >      default:
> >          set_sigp_status(&si, SIGP_STAT_INVALID_ORDER);
> > +        s390_cpu_reset_sigp_busy(dst_cpu);
> >      }
> >  
> >      return si.cc;
> > @@ -444,6 +477,12 @@ int handle_sigp(CPUS390XState *env, uint8_t
> > order, uint64_t r1, uint64_t r3)
> >      int ret;
> >  
> 
> Maybe rather lookup the dst once:
> 
> if (order != SIGP_SET_ARCH) {
>     /* all other sigp orders target a single vcpu */
>     dst_cpu = s390_cpu_addr2state(env->regs[r3]);
> }
> 
> if (qemu_mutex_trylock(&qemu_sigp_mutex)) {
>     if (dst_cpu) {
>         s390_cpu_reset_sigp_busy(dst_cpu);
>     }
>     ret = SIGP_CC_BUSY;
>     goto out;
> }
> 
> switch (order) {
> case SIGP_SET_ARCH:
>     ret = sigp_set_architecture(cpu, param, status_reg);
>     break;
> default:
>     ret = handle_sigp_single_dst(cpu, dst_cpu, order, param,
> status_reg);
> }
> 
> 
> BUT, I wonder if this is fully correct.
> 
> Can't it happen that another CPU is currently processing an order for
> that very same dst_cpu and you would clear SIGP busy of that cpu
> prematurely?

Ah, yes...  I got caught up in the "this is a global lock so another
vcpu must be doing a sigp" side of things, and relying on the kernel to
make the determination that "vcpuN is already processing an order,
don't send it another one."

> 
> >      if (qemu_mutex_trylock(&qemu_sigp_mutex)) {
> > +        if (order != SIGP_SET_ARCH) {
> > +            dst_cpu = s390_cpu_addr2state(env->regs[r3]);
> > +            if (dst_cpu) {
> > +                s390_cpu_reset_sigp_busy(dst_cpu);
> > +            }
> > +        }
> >          ret = SIGP_CC_BUSY;
> >          goto out;
> >      }
> > @@ -487,6 +526,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)
> > 
> 
>