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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d08ddd8008468dddb02876c38313c17e57be89af.camel@linux.ibm.com>
Date:   Fri, 10 Dec 2021 09:36:29 +0100
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Matthew Rosato <mjrosato@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        linux-s390@...r.kernel.org
Cc:     alex.williamson@...hat.com, cohuck@...hat.com,
        farman@...ux.ibm.com, pmorel@...ux.ibm.com, hca@...ux.ibm.com,
        gor@...ux.ibm.com, gerald.schaefer@...ux.ibm.com,
        agordeev@...ux.ibm.com, frankja@...ux.ibm.com, david@...hat.com,
        imbrenda@...ux.ibm.com, vneethv@...ux.ibm.com,
        oberpar@...ux.ibm.com, freude@...ux.ibm.com, thuth@...hat.com,
        pasic@...ux.ibm.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 14/32] KVM: s390: pci: do initial setup for AEN
 interpretation

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@...ux.ibm.com>
> > > ---
> > >   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.