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

[David Hildenbrand]

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 ?

> +{
> +    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?

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.




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

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


-- 
Thanks,

David / dhildenb