| Message ID | 20211008203112.1979843-4-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improvements to SIGP handling [KVM] | expand |
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?
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? Thomas
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. Eric > > Thomas >
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. And we should do the same with the __prepare_sigp_cpu_reset() function (in a separate patch). Just my 0.02 € of course. Thomas
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 >
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,
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(-)