Re: [PATCH 07/32] s390/pci: externalize the SIC operation controls and routine

[Niklas Schnelle]

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.

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