On Wed, 2021-12-08 at 10:33 -0500, Matthew Rosato wrote:
> On 12/8/21 8:53 AM, Niklas Schnelle wrote:
> > On Wed, 2021-12-08 at 14:09 +0100, Christian Borntraeger wrote:
> > > Am 07.12.21 um 21:57 schrieb Matthew Rosato:
> > > > A subsequent patch will be issuing SIC from KVM -- export the necessary
> > > > routine and make the operation control definitions available from a header.
> > > > Because the routine will now be exported, let's swap the purpose of
> > > > zpci_set_irq_ctrl and __zpci_set_irq_ctrl, leaving the latter as a static
> > > > within pci_irq.c only for SIC calls that don't specify an iib.
> > >
> > > Maybe it would be simpler to export the __ version instead of renaming everything.
> > > Whatever Niklas prefers.
> >
> > See below I think it's just not worth it having both variants at all.
> >
> > > > Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > > > ---
> > > > arch/s390/include/asm/pci_insn.h | 17 +++++++++--------
> > > > arch/s390/pci/pci_insn.c | 3 ++-
> > > > arch/s390/pci/pci_irq.c | 28 ++++++++++++++--------------
> > > > 3 files changed, 25 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/arch/s390/include/asm/pci_insn.h b/arch/s390/include/asm/pci_insn.h
> > > > index 61cf9531f68f..5331082fa516 100644
> > > > --- a/arch/s390/include/asm/pci_insn.h
> > > > +++ b/arch/s390/include/asm/pci_insn.h
> > > > @@ -98,6 +98,14 @@ struct zpci_fib {
> > > > u32 gd;
> > > > } __packed __aligned(8);
> > > >
> > > > +/* Set Interruption Controls Operation Controls */
> > > > +#define SIC_IRQ_MODE_ALL 0
> > > > +#define SIC_IRQ_MODE_SINGLE 1
> > > > +#define SIC_IRQ_MODE_DIRECT 4
> > > > +#define SIC_IRQ_MODE_D_ALL 16
> > > > +#define SIC_IRQ_MODE_D_SINGLE 17
> > > > +#define SIC_IRQ_MODE_SET_CPU 18
> > > > +
> > > > /* directed interruption information block */
> > > > struct zpci_diib {
> > > > u32 : 1;
> > > > @@ -134,13 +142,6 @@ int __zpci_store(u64 data, u64 req, u64 offset);
> > > > int zpci_store(const volatile void __iomem *addr, u64 data, unsigned long len);
> > > > int __zpci_store_block(const u64 *data, u64 req, u64 offset);
> > > > void zpci_barrier(void);
> > > > -int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib);
> > > > -
> > > > -static inline int zpci_set_irq_ctrl(u16 ctl, u8 isc)
> > > > -{
> > > > - union zpci_sic_iib iib = {{0}};
> > > > -
> > > > - return __zpci_set_irq_ctrl(ctl, isc, &iib);
> > > > -}
> > > > +int zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib);
> >
> > Since the __zpci_set_irq_ctrl() was already non static/inline the above
> > inline to non-inline change shouldn't make a performance difference.
> >
> > Looking at this makes me wonder though. Wouldn't it make sense to just
> > have the zpci_set_irq_ctrl() function inline in the header. Its body is
> > a single instruction inline asm plus a test_facility(). The latter by
> > the way I think also looks rather out of place there considering we
> > call zpci_set_irq_ctrl() in the interrupt handler and facilities can't
> > go away so it's pretty silly to check for it on every single
> > interrupt.. unless I'm totally missing something.
>
> This test_facility isn't new to this patch
Yeah I got that part, your patch just made me look.
> , it was added via
>
> commit 48070c73058be6de9c0d754d441ed7092dfc8f12
> Author: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> Date: Mon Oct 30 14:38:58 2017 +0100
>
> s390/pci: do not require AIS facility
>
> It looks like in the past, we would not even initialize zpci at all if
> AIS wasn't available. With this, we initialize PCI but only do the SIC
> when we have AIS, which makes sense.
Ah yes I guess that is the something I was missing. I was wondering why
that wasn't just tested for during init.
>
> So for this patch, the sane thing to do is probably just keep the
> test_facility() in place and move to header, inline.
Yes sounds good.
>
> Maybe there's a subsequent optimization to be made (setup a static key
> like have_mio vs doing test_facility all the time?)
Yeah, looking again more closely at test_facilities() it's probably not
that expensive either I'll do some tests. Maybe we can also just add a
comment and a normal unlikely() macro since with this series KVM would
also support AIS, correct?
>
---8<---
