Message-ID: <c65a9055-a472-5f89-bfd2-a80ed4973ad9@linux.ibm.com>
Date:   Thu, 9 Dec 2021 21:23:57 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH 14/32] KVM: s390: pci: do initial setup for AEN
 interpretation
Content-Language: en-US
To:     Matthew Rosato <mjrosato@linux.ibm.com>, linux-s390@vger.kernel.org
Cc:     alex.williamson@redhat.com, cohuck@redhat.com,
        schnelle@linux.ibm.com, farman@linux.ibm.com, pmorel@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        frankja@linux.ibm.com, david@redhat.com, imbrenda@linux.ibm.com,
        vneethv@linux.ibm.com, oberpar@linux.ibm.com, freude@linux.ibm.com,
        thuth@redhat.com, pasic@linux.ibm.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211207205743.150299-1-mjrosato@linux.ibm.com>
 <20211207205743.150299-15-mjrosato@linux.ibm.com>
 <596857e3-ab13-7513-eeda-ed407fe22732@linux.ibm.com>
 <31980a07-e2e8-cef3-f0b4-370dad4cb14c@linux.ibm.com>
From:   Christian Borntraeger <borntraeger@linux.ibm.com>
In-Reply-To: <31980a07-e2e8-cef3-f0b4-370dad4cb14c@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@linux.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
>>>
>>> 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?
>