Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <479f0342-f6f8-9bb5-6ee5-6d788dc25631@linux.ibm.com>
Date:   Wed, 26 Jan 2022 09:24:39 +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 v2 26/30] vfio-pci/zdev: wire up zPCI adapter interrupt
 forwarding support
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,
        borntraeger@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: <20220114203145.242984-1-mjrosato@linux.ibm.com>
 <20220114203145.242984-27-mjrosato@linux.ibm.com>
 <75c74f80-0a74-40dc-6797-473522ef2803@linux.ibm.com>
 <5f3797f7-e127-7de0-dc96-4b04e5ff839a@linux.ibm.com>
From:   Pierre Morel <pmorel@linux.ibm.com>
In-Reply-To: <5f3797f7-e127-7de0-dc96-4b04e5ff839a@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 1/25/22 15:16, Matthew Rosato wrote:
> On 1/25/22 7:36 AM, Pierre Morel wrote:
>>
>>
>> On 1/14/22 21:31, Matthew Rosato wrote:
>>> Introduce support for VFIO_DEVICE_FEATURE_ZPCI_AIF, which is a new
>>> VFIO_DEVICE_FEATURE ioctl.  This interface is used to indicate that an
>>> s390x vfio-pci device wishes to enable/disable zPCI adapter interrupt
>>> forwarding, which allows underlying firmware to deliver interrupts
>>> directly to the associated kvm guest.
>>>
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> ---
>>>   arch/s390/include/asm/kvm_pci.h  |  2 +
>>>   drivers/vfio/pci/vfio_pci_core.c |  2 +
>>>   drivers/vfio/pci/vfio_pci_zdev.c | 98 +++++++++++++++++++++++++++++++-
>>>   include/linux/vfio_pci_core.h    | 10 ++++
>>>   include/uapi/linux/vfio.h        |  7 +++
>>>   include/uapi/linux/vfio_zdev.h   | 20 +++++++
>>>   6 files changed, 138 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_pci.h 
>>> b/arch/s390/include/asm/kvm_pci.h
>>> index dc00c3f27a00..dbab349a4a75 100644
>>> --- a/arch/s390/include/asm/kvm_pci.h
>>> +++ b/arch/s390/include/asm/kvm_pci.h
>>> @@ -36,6 +36,8 @@ struct kvm_zdev {
>>>       struct zpci_fib fib;
>>>       struct notifier_block nb;
>>>       bool interp;
>>> +    bool aif;
>>> +    bool fhost;
>>
>> Can we please have a comment on these booleans? > Can we have explicit 
>> naming to be able to follow their usage more easily?
>> May be aif_float and aif_host to match with the VFIO feature?
> 
> Sure, rename would be fine.
> 
> As for a comment, maybe something like
> 
> bool aif_float; /* Enabled for floating interrupt assist */
> bool aif_host;  /* Require host delivery */

good for me.


> 
> ...
> 
>>> diff --git a/include/uapi/linux/vfio_zdev.h 
>>> b/include/uapi/linux/vfio_zdev.h
>>> index 575f0410dc66..c574e23f9385 100644
>>> --- a/include/uapi/linux/vfio_zdev.h
>>> +++ b/include/uapi/linux/vfio_zdev.h
>>> @@ -90,4 +90,24 @@ struct vfio_device_zpci_interp {
>>>       __u32 fh;        /* Host device function handle */
>>>   };
>>> +/**
>>> + * VFIO_DEVICE_FEATURE_ZPCI_AIF
>>> + *
>>> + * This feature is used for enabling forwarding of adapter 
>>> interrupts directly
>>> + * from firmware to the guest.  When setting this feature, the flags 
>>> indicate
>>> + * whether to enable/disable the feature and the structure defined 
>>> below is
>>> + * used to setup the forwarding structures.  When getting this 
>>> feature, only
>>> + * the flags are used to indicate the current state.
>>> + */
>>> +struct vfio_device_zpci_aif {
>>> +    __u64 flags;
>>> +#define VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT 1
>>> +#define VFIO_DEVICE_ZPCI_FLAG_AIF_HOST 2
>>
>> I think we need more information on these flags.
>> What does AIF_FLOAT and what does AIF_HOST ?
>>
> 
> You actually asked for this already on Jan 19 :), here's a copy of that 
> response inline here:

:) I forgot

> 
> I can add a small line comment for each, like:
> 
>   AIF_FLOAT 1 /* Floating interrupts enabled */
>   AIF_HOST 2  /* Host delivery forced */
> 
> But here's a bit more detail:
> 
> On SET:
> AIF_FLOAT = 1 means enable the interrupt forwarding assist for floating 
> interrupt delivery
> AIF_FLOAT = 0 means to disable it.
> AIF_HOST = 1 means the assist will always deliver the interrupt to the 
> host and let the host inject it
> AIF_HOST = 0 host only gets interrupts when firmware can't deliver
> 
> on GET, we just indicate the current settings from the most recent SET, 
> meaning:
> AIF_FLOAT = 1 interrupt forwarding assist is currently active
> AIF_FLOAT = 0 interrupt forwarding assist is not currently active
> AIF_HOST = 1 interrupt forwarding will always go through host
> AIF_HOST = 0 interrupt forwarding will only go through the host when 
> necessary
> 
> My thought would be add the line comments in this patch and then the 
> additional detail in a follow-on patch that adds vfio zPCI to 
> Documentation/S390
> 

good for me.

-- 
Pierre Morel
IBM Lab Boeblingen