Re: [PATCH v2 26/30] vfio-pci/zdev: wire up zPCI adapter interrupt forwarding support

[Matthew Rosato]

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@xxxxxxxxxxxxx>
> > ---
> >   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 */

...

> > 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 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