Re: [PATCH 14/32] KVM: s390: pci: do initial setup for AEN interpretation

[Christian Borntraeger]

Am 09.12.21 um 21:20 schrieb Matthew Rosato:
> On 12/9/21 2:54 PM, Christian Borntraeger wrote:
> > Am 07.12.21 um 21:57 schrieb Matthew Rosato:
> > > Initial setup for Adapter Event Notification Interpretation for zPCI
> > > passthrough devices.  Specifically, allocate a structure for forwarding of
> > > adapter events and pass the address of this structure to firmware.
> > >
> > > Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > > ---
> > >   arch/s390/include/asm/pci_insn.h |  12 ++++
> > >   arch/s390/kvm/interrupt.c        |  17 +++++
> > >   arch/s390/kvm/kvm-s390.c         |   3 +
> > >   arch/s390/kvm/pci.c              | 113 +++++++++++++++++++++++++++++++
> > >   arch/s390/kvm/pci.h              |  42 ++++++++++++
> > >   5 files changed, 187 insertions(+)
> > >   create mode 100644 arch/s390/kvm/pci.h
> > >
> > > diff --git a/arch/s390/include/asm/pci_insn.h b/arch/s390/include/asm/pci_insn.h
> > > index 5331082fa516..e5f57cfe1d45 100644
> > > --- a/arch/s390/include/asm/pci_insn.h
> > > +++ b/arch/s390/include/asm/pci_insn.h
> > > @@ -101,6 +101,7 @@ struct zpci_fib {
> > >   /* Set Interruption Controls Operation Controls  */
> > >   #define    SIC_IRQ_MODE_ALL        0
> > >   #define    SIC_IRQ_MODE_SINGLE        1
> > > +#define    SIC_SET_AENI_CONTROLS        2
> > >   #define    SIC_IRQ_MODE_DIRECT        4
> > >   #define    SIC_IRQ_MODE_D_ALL        16
> > >   #define    SIC_IRQ_MODE_D_SINGLE        17
> > > @@ -127,9 +128,20 @@ struct zpci_cdiib {
> > >       u64 : 64;
> > >   } __packed __aligned(8);
> > > +/* adapter interruption parameters block */
> > > +struct zpci_aipb {
> > > +    u64 faisb;
> > > +    u64 gait;
> > > +    u16 : 13;
> > > +    u16 afi : 3;
> > > +    u32 : 32;
> > > +    u16 faal;
> > > +} __packed __aligned(8);
> > > +
> > >   union zpci_sic_iib {
> > >       struct zpci_diib diib;
> > >       struct zpci_cdiib cdiib;
> > > +    struct zpci_aipb aipb;
> > >   };
> > >   DECLARE_STATIC_KEY_FALSE(have_mio);
> > > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > > index f9b872e358c6..4efe0e95a40f 100644
> > > --- a/arch/s390/kvm/interrupt.c
> > > +++ b/arch/s390/kvm/interrupt.c
> > > @@ -32,6 +32,7 @@
> > >   #include "kvm-s390.h"
> > >   #include "gaccess.h"
> > >   #include "trace-s390.h"
> > > +#include "pci.h"
> > >   #define PFAULT_INIT 0x0600
> > >   #define PFAULT_DONE 0x0680
> > > @@ -3276,8 +3277,16 @@ static struct airq_struct gib_alert_irq = {
> > >   void kvm_s390_gib_destroy(void)
> > >   {
> > > +    struct zpci_aift *aift;
> > > +
> > >       if (!gib)
> > >           return;
> > > +    aift = kvm_s390_pci_get_aift();
> > > +    if (aift) {
> > > +        mutex_lock(&aift->lock)
> >
> > aift is a static variable and later patches seem to access that directly without the wrapper.
> > Can we get rid of kvm_s390_pci_get_aift?
>
> kvm/interrupt.c must also access it when handling AEN forwarding (next patch)

So maybe just make it non-static and declare it in the header file?
[...]

> > Can we maybe use designated initializer for the static definition of aift, e.g. something
> > like
> > static struct zpci_aift aift = {
> >      .gait_lock = __SPIN_LOCK_UNLOCKED(aift.gait_lock),
> >      .lock    = __MUTEX_INITIALIZER(aift.lock),
> > }
> > and get rid of the init function? >
>
> Maybe -- I can certainly do the above, but I do add a call to zpci_get_mdd() in the init function (patch 23), so if I want to in patch 23 instead add .mdd = zpci_get_mdd() to this designated initializer I'd have to re-work zpci_get_mdd (patch 12) to return the mdd rather than the CLP LIST PCI return code.  We want at least a warning if we're setting a 0 for mdd because the CLP failed for some bizarre reason.
>
> I guess one option would be to move the WARN_ON into the zpci_get_mdd() function itself and then now we can do

So maybe leave this as is.
> u32 zpci_get_mdd(void);
>
> Niklas, what do you think?