Re: [RFC PATCH v1 3/6] KVM: s390: Simplify SIGP Restart

[Eric Farman <farman@linux.ibm.com>]

On Wed, 2021-10-13 at 07:54 +0200, Thomas Huth wrote:
> On 12/10/2021 17.31, Eric Farman wrote:
> > On Tue, 2021-10-12 at 17:23 +0200, Thomas Huth wrote:
> > > On 11/10/2021 09.45, Christian Borntraeger wrote:
> > > > Am 08.10.21 um 22:31 schrieb Eric Farman:
> > > > > Now that we check for the STOP IRQ injection at the top of
> > > > > the
> > > > > SIGP
> > > > > handler (before the userspace/kernelspace check), we don't
> > > > > need
> > > > > to do
> > > > > it down here for the Restart order.
> > > > > 
> > > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > > ---
> > > > >    arch/s390/kvm/sigp.c | 11 +----------
> > > > >    1 file changed, 1 insertion(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> > > > > index 6ca01bbc72cf..0c08927ca7c9 100644
> > > > > --- a/arch/s390/kvm/sigp.c
> > > > > +++ b/arch/s390/kvm/sigp.c
> > > > > @@ -240,17 +240,8 @@ static int __sigp_sense_running(struct
> > > > > kvm_vcpu *vcpu,
> > > > >    static int __prepare_sigp_re_start(struct kvm_vcpu *vcpu,
> > > > >                       struct kvm_vcpu *dst_vcpu, u8
> > > > > order_code)
> > > > >    {
> > > > > -    struct kvm_s390_local_interrupt *li = &dst_vcpu-
> > > > > > arch.local_int;
> > > > >        /* handle (RE)START in user space */
> > > > > -    int rc = -EOPNOTSUPP;
> > > > > -
> > > > > -    /* make sure we don't race with STOP irq injection */
> > > > > -    spin_lock(&li->lock);
> > > > > -    if (kvm_s390_is_stop_irq_pending(dst_vcpu))
> > > > > -        rc = SIGP_CC_BUSY;
> > > > > -    spin_unlock(&li->lock);
> > > > > -
> > > > > -    return rc;
> > > > > +    return -EOPNOTSUPP;
> > > > >    }
> > > > >    static int __prepare_sigp_cpu_reset(struct kvm_vcpu *vcpu,
> > > > > 
> > > > 
> > > > @thuth?
> > > > Question is, does it make sense to merge patch 2 and 3 to make
> > > > things more
> > > > obvious?
> > > 
> > > Maybe.
> > > 
> > > Anyway: Would it make sense to remove __prepare_sigp_re_start()
> > > completely
> > > now and let __prepare_sigp_unknown() set the return code in the
> > > "default:" case?
> > 
> > We could, but that would affect the SIGP START case which also uses
> > the
> > re_start routine. And if we're going down that path, we could
> > remove
> > (INITIAL) CPU RESET handled in __prepare_sigp_cpu_reset, which does
> > the
> > same thing (nothing). Not sure it buys us much, other than losing
> > the
> > details in the different counters of which SIGP orders are
> > processed.
> 
> Ok, we likely shouldn't change the way of counting the SIGPs here...
> So what about removing the almost empty function and simply do the
> "rc = 
> -EOPNOTSUPP" right in the handle_sigp_dst() function? That's still
> the 
> easiest way to read the code, I think. 

Hrm, that might be better. I've almost got the IOCTL stuff in a
reasonable place for a discussion, will see about such cleanups at the
end of that (new) series.

> And we should do the same with the 
> __prepare_sigp_cpu_reset() function (in a separate patch). Just my
> 0.02 € of 
> course.

I appreciate it. Though I still don't have an easy way to use the €
coins I have in a drawer over here. ;-)

Eric

> 
>   Thomas
>