| Message ID | 20211207205743.150299-8-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: enable zPCI for interpretive execution | expand |
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. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > 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); > > #endif > diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c > index 28d863aaafea..d1a8bd43ce26 100644 > --- a/arch/s390/pci/pci_insn.c > +++ b/arch/s390/pci/pci_insn.c > @@ -97,7 +97,7 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range) > } > > /* Set Interruption Controls */ > -int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) > +int zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) > { > if (!test_facility(72)) > return -EIO; > @@ -108,6 +108,7 @@ int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) > > return 0; > } > +EXPORT_SYMBOL_GPL(zpci_set_irq_ctrl); > > /* PCI Load */ > static inline int ____pcilg(u64 *data, u64 req, u64 offset, u8 *status) > diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c > index dfd4f3276a6d..6b29e39496d1 100644 > --- a/arch/s390/pci/pci_irq.c > +++ b/arch/s390/pci/pci_irq.c > @@ -15,13 +15,6 @@ > > static enum {FLOATING, DIRECTED} irq_delivery; > > -#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 > - > /* > * summary bit vector > * FLOATING - summary bit per function > @@ -145,6 +138,13 @@ static int zpci_set_irq_affinity(struct irq_data *data, const struct cpumask *de > return IRQ_SET_MASK_OK; > } > > +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); > +} > + > static struct irq_chip zpci_irq_chip = { > .name = "PCI-MSI", > .irq_unmask = pci_msi_unmask_irq, > @@ -165,7 +165,7 @@ static void zpci_handle_cpu_local_irq(bool rescan) > /* End of second scan with interrupts on. */ > break; > /* First scan complete, reenable interrupts. */ > - if (zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC)) > + if (__zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC)) > break; > bit = 0; > continue; > @@ -203,7 +203,7 @@ static void zpci_handle_fallback_irq(void) > /* End of second scan with interrupts on. */ > break; > /* First scan complete, reenable interrupts. */ > - if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) > + if (__zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) > break; > cpu = 0; > continue; > @@ -247,7 +247,7 @@ static void zpci_floating_irq_handler(struct airq_struct *airq, > /* End of second scan with interrupts on. */ > break; > /* First scan complete, reenable interrupts. */ > - if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) > + if (__zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) > break; > si = 0; > continue; > @@ -412,8 +412,8 @@ static void __init cpu_enable_directed_irq(void *unused) > > iib.cdiib.dibv_addr = (u64) zpci_ibv[smp_processor_id()]->vector; > > - __zpci_set_irq_ctrl(SIC_IRQ_MODE_SET_CPU, 0, &iib); > - zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC); > + zpci_set_irq_ctrl(SIC_IRQ_MODE_SET_CPU, 0, &iib); > + __zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC); > } > > static int __init zpci_directed_irq_init(void) > @@ -428,7 +428,7 @@ static int __init zpci_directed_irq_init(void) > iib.diib.isc = PCI_ISC; > iib.diib.nr_cpus = num_possible_cpus(); > iib.diib.disb_addr = (u64) zpci_sbv->vector; > - __zpci_set_irq_ctrl(SIC_IRQ_MODE_DIRECT, 0, &iib); > + zpci_set_irq_ctrl(SIC_IRQ_MODE_DIRECT, 0, &iib); > > zpci_ibv = kcalloc(num_possible_cpus(), sizeof(*zpci_ibv), > GFP_KERNEL); > @@ -504,7 +504,7 @@ int __init zpci_irq_init(void) > * Enable floating IRQs (with suppression after one IRQ). When using > * directed IRQs this enables the fallback path. > */ > - zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC); > + __zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC); > > return 0; > out_airq: >
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@linux.ibm.com> > > --- > > 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. > > > > #endif > > diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c > > index 28d863aaafea..d1a8bd43ce26 100644 > > --- a/arch/s390/pci/pci_insn.c > > +++ b/arch/s390/pci/pci_insn.c > > @@ -97,7 +97,7 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range) > > } > > > > /* Set Interruption Controls */ > > -int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) > > +int zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) > > { > > if (!test_facility(72)) > > return -EIO; > > @@ -108,6 +108,7 @@ int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) > > > > return 0; > > } > > +EXPORT_SYMBOL_GPL(zpci_set_irq_ctrl); > > > > /* PCI Load */ > > static inline int ____pcilg(u64 *data, u64 req, u64 offset, u8 *status) > > diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c > > index dfd4f3276a6d..6b29e39496d1 100644 > > --- a/arch/s390/pci/pci_irq.c > > +++ b/arch/s390/pci/pci_irq.c > > @@ -15,13 +15,6 @@ > > > > static enum {FLOATING, DIRECTED} irq_delivery; > > > > -#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 > > - > > /* > > * summary bit vector > > * FLOATING - summary bit per function > > @@ -145,6 +138,13 @@ static int zpci_set_irq_affinity(struct irq_data *data, const struct cpumask *de > > return IRQ_SET_MASK_OK; > > } > > > > +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); > > +} > > + I would be totally fine and slighlt prefer to have the 0 iib repeated at those 3 call sites that don't need it. On first glance that should come out to pretty much the same number of lines of code and it removes the potential confusion of swapping the __ prefixed and non-prefixed variants. What do you think? > > static struct irq_chip zpci_irq_chip = { > > .name = "PCI-MSI", > > .irq_unmask = pci_msi_unmask_irq, > > @@ -165,7 +165,7 @@ static void zpci_handle_cpu_local_irq(bool rescan) > > ---8<---
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@linux.ibm.com> >>> --- >>> 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, it was added via commit 48070c73058be6de9c0d754d441ed7092dfc8f12 Author: Christian Borntraeger <borntraeger@de.ibm.com> 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. So for this patch, the sane thing to do is probably just keep the test_facility() in place and move to header, inline. Maybe there's a subsequent optimization to be made (setup a static key like have_mio vs doing test_facility all the time?) > >>> >>> #endif >>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c >>> index 28d863aaafea..d1a8bd43ce26 100644 >>> --- a/arch/s390/pci/pci_insn.c >>> +++ b/arch/s390/pci/pci_insn.c >>> @@ -97,7 +97,7 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range) >>> } >>> >>> /* Set Interruption Controls */ >>> -int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) >>> +int zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) >>> { >>> if (!test_facility(72)) >>> return -EIO; >>> @@ -108,6 +108,7 @@ int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) >>> >>> return 0; >>> } >>> +EXPORT_SYMBOL_GPL(zpci_set_irq_ctrl); >>> >>> /* PCI Load */ >>> static inline int ____pcilg(u64 *data, u64 req, u64 offset, u8 *status) >>> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c >>> index dfd4f3276a6d..6b29e39496d1 100644 >>> --- a/arch/s390/pci/pci_irq.c >>> +++ b/arch/s390/pci/pci_irq.c >>> @@ -15,13 +15,6 @@ >>> >>> static enum {FLOATING, DIRECTED} irq_delivery; >>> >>> -#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 >>> - >>> /* >>> * summary bit vector >>> * FLOATING - summary bit per function >>> @@ -145,6 +138,13 @@ static int zpci_set_irq_affinity(struct irq_data *data, const struct cpumask *de >>> return IRQ_SET_MASK_OK; >>> } >>> >>> +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); >>> +} >>> + > > I would be totally fine and slighlt prefer to have the 0 iib repeated > at those 3 call sites that don't need it. On first glance that should > come out to pretty much the same number of lines of code and it removes > the potential confusion of swapping the __ prefixed and non-prefixed > variants. What do you think? Sure, I can do that. > >>> static struct irq_chip zpci_irq_chip = { >>> .name = "PCI-MSI", >>> .irq_unmask = pci_msi_unmask_irq, >>> @@ -165,7 +165,7 @@ static void zpci_handle_cpu_local_irq(bool rescan) >>> > ---8<--- >
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@linux.ibm.com> > > > > --- > > > > 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@de.ibm.com> > 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<---
On 12/8/21 10:59 AM, Niklas Schnelle wrote: > 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@linux.ibm.com> >>>>> --- >>>>> 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@de.ibm.com> >> 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? AIS was already being set as a KVM facility / allowed as QEMU capability before this series, however there was a period of time where QEMU was disabling it (disabled in QEMU 3f2d07b3b01e, enabled again in QEMU a5c8617af691) which I suspect was the impetus for this kernel change; this means that there are older machines that won't have it, but moving forward we should be OK in the standard case. Of course the kernel should still be able to tolerate the case where AIS is unavailable (old machine, intentionally forced off, etc), so maybe the unlikely indeed makes the most sense. As far as a comment for the unlikely I could add something like 'some virtualized environments may have disabled the AIS facility'?
On Wed, 2021-12-08 at 11:20 -0500, Matthew Rosato wrote: > On 12/8/21 10:59 AM, Niklas Schnelle wrote: > > 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@linux.ibm.com> > > > > > > --- > > > > > > 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@de.ibm.com> > > > 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? > AIS was already being set as a KVM facility / allowed as QEMU capability > before this series, however there was a period of time where QEMU was > disabling it (disabled in QEMU 3f2d07b3b01e, enabled again in QEMU > a5c8617af691) which I suspect was the impetus for this kernel change; > this means that there are older machines that won't have it, but moving > forward we should be OK in the standard case. Of course the kernel > should still be able to tolerate the case where AIS is unavailable (old > machine, intentionally forced off, etc), so maybe the unlikely indeed > makes the most sense. Thanks for the background! > > As far as a comment for the unlikely I could add something like 'some > virtualized environments may have disabled the AIS facility'? I think we should add the unlikely() and comment in a separate patch such that this one really doesn't change behavior only the call signature and export.
On Wed, 2021-12-08 at 17:41 +0100, Niklas Schnelle wrote: > On Wed, 2021-12-08 at 11:20 -0500, Matthew Rosato wrote: > > On 12/8/21 10:59 AM, Niklas Schnelle wrote: > > > 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@linux.ibm.com> > > > > > > > --- > > > > > > > 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@de.ibm.com> > > > > 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. As discussed out of band, slight change of plan. Let's keep the implementation in pci_insn.c for now but remove the __* prefix and the iib 0 wrapper. This way we get rid of potential confusion of swapping what each variant does and we also don't need to export a __* prefixed function. I tried it out locally and having the iib 0 at the callsites indeed doesn't look worse.
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); #endif diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c index 28d863aaafea..d1a8bd43ce26 100644 --- a/arch/s390/pci/pci_insn.c +++ b/arch/s390/pci/pci_insn.c @@ -97,7 +97,7 @@ int zpci_refresh_trans(u64 fn, u64 addr, u64 range) } /* Set Interruption Controls */ -int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) +int zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) { if (!test_facility(72)) return -EIO; @@ -108,6 +108,7 @@ int __zpci_set_irq_ctrl(u16 ctl, u8 isc, union zpci_sic_iib *iib) return 0; } +EXPORT_SYMBOL_GPL(zpci_set_irq_ctrl); /* PCI Load */ static inline int ____pcilg(u64 *data, u64 req, u64 offset, u8 *status) diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c index dfd4f3276a6d..6b29e39496d1 100644 --- a/arch/s390/pci/pci_irq.c +++ b/arch/s390/pci/pci_irq.c @@ -15,13 +15,6 @@ static enum {FLOATING, DIRECTED} irq_delivery; -#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 - /* * summary bit vector * FLOATING - summary bit per function @@ -145,6 +138,13 @@ static int zpci_set_irq_affinity(struct irq_data *data, const struct cpumask *de return IRQ_SET_MASK_OK; } +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); +} + static struct irq_chip zpci_irq_chip = { .name = "PCI-MSI", .irq_unmask = pci_msi_unmask_irq, @@ -165,7 +165,7 @@ static void zpci_handle_cpu_local_irq(bool rescan) /* End of second scan with interrupts on. */ break; /* First scan complete, reenable interrupts. */ - if (zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC)) + if (__zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC)) break; bit = 0; continue; @@ -203,7 +203,7 @@ static void zpci_handle_fallback_irq(void) /* End of second scan with interrupts on. */ break; /* First scan complete, reenable interrupts. */ - if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) + if (__zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) break; cpu = 0; continue; @@ -247,7 +247,7 @@ static void zpci_floating_irq_handler(struct airq_struct *airq, /* End of second scan with interrupts on. */ break; /* First scan complete, reenable interrupts. */ - if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) + if (__zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC)) break; si = 0; continue; @@ -412,8 +412,8 @@ static void __init cpu_enable_directed_irq(void *unused) iib.cdiib.dibv_addr = (u64) zpci_ibv[smp_processor_id()]->vector; - __zpci_set_irq_ctrl(SIC_IRQ_MODE_SET_CPU, 0, &iib); - zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC); + zpci_set_irq_ctrl(SIC_IRQ_MODE_SET_CPU, 0, &iib); + __zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC); } static int __init zpci_directed_irq_init(void) @@ -428,7 +428,7 @@ static int __init zpci_directed_irq_init(void) iib.diib.isc = PCI_ISC; iib.diib.nr_cpus = num_possible_cpus(); iib.diib.disb_addr = (u64) zpci_sbv->vector; - __zpci_set_irq_ctrl(SIC_IRQ_MODE_DIRECT, 0, &iib); + zpci_set_irq_ctrl(SIC_IRQ_MODE_DIRECT, 0, &iib); zpci_ibv = kcalloc(num_possible_cpus(), sizeof(*zpci_ibv), GFP_KERNEL); @@ -504,7 +504,7 @@ int __init zpci_irq_init(void) * Enable floating IRQs (with suppression after one IRQ). When using * directed IRQs this enables the fallback path. */ - zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC); + __zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, PCI_ISC); return 0; out_airq:
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. Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- 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(-)