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

[Niklas Schnelle]

On Thu, 2021-12-09 at 15:20 -0500, Matthew Rosato wrote:
> 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
> > > 
---8<---
> > >   int kvm_s390_pci_dev_open(struct zpci_dev *zdev)
> > >   {
> > > @@ -55,3 +162,9 @@ int kvm_s390_pci_attach_kvm(struct zpci_dev *zdev, 
> > > struct kvm *kvm)
> > >       return 0;
> > >   }
> > >   EXPORT_SYMBOL_GPL(kvm_s390_pci_attach_kvm);
> > > +
> > > +void kvm_s390_pci_init(void)
> > > +{
> > > +    spin_lock_init(&aift.gait_lock);
> > > +    mutex_init(&aift.lock);
> > > +}
> > 
> > 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

Hmm, if we do change zpci_get_mdd() which I'm generally fine with I
feel like the initializer would be weird mix of truly static lock
initialization and a function that actually does a CLP.
I'm also a little worried about initialization order if kvm is built-
in. The CLP should work even with PCI not initialized but what if for
example the facility isn't even there?

Also if you do change zpci_get-mdd() I'd prefer a pr_err() instead of a
WARN_ON(), no reason to crash the system for this if it runs with
panic-on-warn. So I think overall keeping it as is makes more sense.