| Message ID | 20220114203145.242984-27-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: enable zPCI for interpretive execution | expand |
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; > }; > > int kvm_s390_pci_dev_open(struct zpci_dev *zdev); > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index 2b2d64a2190c..01658de660bd 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -1174,6 +1174,8 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, > return 0; > case VFIO_DEVICE_FEATURE_ZPCI_INTERP: > return vfio_pci_zdev_feat_interp(vdev, feature, arg); > + case VFIO_DEVICE_FEATURE_ZPCI_AIF: > + return vfio_pci_zdev_feat_aif(vdev, feature, arg); > default: > return -ENOTTY; > } > diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c > index 4339f48b98bc..891cfa016d63 100644 > --- a/drivers/vfio/pci/vfio_pci_zdev.c > +++ b/drivers/vfio/pci/vfio_pci_zdev.c > @@ -13,6 +13,7 @@ > #include <linux/vfio_zdev.h> > #include <asm/pci_clp.h> > #include <asm/pci_io.h> > +#include <asm/pci_insn.h> > #include <asm/kvm_pci.h> > > #include <linux/vfio_pci_core.h> > @@ -208,6 +209,99 @@ int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, > return rc; > } > > +int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, > + struct vfio_device_feature feature, > + unsigned long arg) > +{ > + struct zpci_dev *zdev = to_zpci(vdev->pdev); > + struct vfio_device_zpci_aif *data; > + struct vfio_device_feature *feat; > + unsigned long minsz; > + int size, rc = 0; > + > + if (!zdev || !zdev->kzdev) > + return -EINVAL; > + > + /* If PROBE specified, return probe results immediately */ > + if (feature.flags & VFIO_DEVICE_FEATURE_PROBE) > + return kvm_s390_pci_aif_probe(zdev); > + > + /* GET and SET are mutually exclusive */ > + if ((feature.flags & VFIO_DEVICE_FEATURE_GET) && > + (feature.flags & VFIO_DEVICE_FEATURE_SET)) > + return -EINVAL; > + > + size = sizeof(*feat) + sizeof(*data); > + feat = kzalloc(size, GFP_KERNEL); > + if (!feat) > + return -ENOMEM; > + > + data = (struct vfio_device_zpci_aif *)&feat->data; > + minsz = offsetofend(struct vfio_device_feature, flags); > + > + if (feature.argsz < minsz + sizeof(*data)) > + return -EINVAL; > + > + /* Get the rest of the payload for GET/SET */ > + rc = copy_from_user(data, (void __user *)(arg + minsz), > + sizeof(*data)); > + if (rc) > + rc = -EINVAL; > + > + if (feature.flags & VFIO_DEVICE_FEATURE_GET) { > + if (zdev->kzdev->aif) > + data->flags = VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT; > + if (zdev->kzdev->fhost) > + data->flags |= VFIO_DEVICE_ZPCI_FLAG_AIF_HOST; > + > + if (copy_to_user((void __user *)arg, feat, size)) > + rc = -EFAULT; > + } else if (feature.flags & VFIO_DEVICE_FEATURE_SET) { > + if (data->flags & VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT) { > + /* create a guest fib */ > + struct zpci_fib fib; > + > + fib.fmt0.aibv = data->ibv; > + fib.fmt0.isc = data->isc; > + fib.fmt0.noi = data->noi; > + if (data->sb != 0) { > + fib.fmt0.aisb = data->sb; > + fib.fmt0.aisbo = data->sbo; > + fib.fmt0.sum = 1; > + } else { > + fib.fmt0.aisb = 0; > + fib.fmt0.aisbo = 0; > + fib.fmt0.sum = 0; > + } > + if (data->flags & VFIO_DEVICE_ZPCI_FLAG_AIF_HOST) { > + rc = kvm_s390_pci_aif_enable(zdev, &fib, false); > + if (!rc) { > + zdev->kzdev->aif = true; > + zdev->kzdev->fhost = true; > + } > + } else { > + rc = kvm_s390_pci_aif_enable(zdev, &fib, true); > + if (!rc) > + zdev->kzdev->aif = true; > + } > + } else if (data->flags == 0) { > + rc = kvm_s390_pci_aif_disable(zdev); > + if (!rc) { > + zdev->kzdev->aif = false; > + zdev->kzdev->fhost = false; > + } > + } else { > + rc = -EINVAL; > + } > + } else { > + /* Neither GET nor SET were specified */ > + rc = -EINVAL; > + } > + > + kfree(feat); > + return rc; > +} > + > static int vfio_pci_zdev_group_notifier(struct notifier_block *nb, > unsigned long action, void *data) > { > @@ -255,8 +349,10 @@ void vfio_pci_zdev_release(struct vfio_pci_core_device *vdev) > * If the device was using interpretation, don't trust that userspace > * did the appropriate cleanup > */ > - if (zdev->gd != 0) > + if (zdev->gd != 0) { > + kvm_s390_pci_aif_disable(zdev); > kvm_s390_pci_interp_disable(zdev); > + } > > kvm_s390_pci_dev_release(zdev); > } > diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h > index 0db2b1051931..7ec5e82e7933 100644 > --- a/include/linux/vfio_pci_core.h > +++ b/include/linux/vfio_pci_core.h > @@ -201,6 +201,9 @@ extern int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev, > int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, > struct vfio_device_feature feature, > unsigned long arg); > +int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, > + struct vfio_device_feature feature, > + unsigned long arg); > void vfio_pci_zdev_open(struct vfio_pci_core_device *vdev); > void vfio_pci_zdev_release(struct vfio_pci_core_device *vdev); > #else > @@ -217,6 +220,13 @@ static inline int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, > return -ENOTTY; > } > > +static inline int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, > + struct vfio_device_feature feature, > + unsigned long arg) > +{ > + return -ENOTTY; > +} > + > static inline void vfio_pci_zdev_open(struct vfio_pci_core_device *vdev) > { > } > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index b9a75485b8e7..fe3bfd99bf50 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -1009,6 +1009,13 @@ struct vfio_device_feature { > */ > #define VFIO_DEVICE_FEATURE_ZPCI_INTERP (1) > > +/* > + * Provide support for enbaling adapter interruption forwarding for zPCI > + * devices. This feature is only valid for s390x PCI devices. Data provided > + * when setting and getting this feature is further described in vfio_zdev.h > + */ > +#define VFIO_DEVICE_FEATURE_ZPCI_AIF (2) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /** > 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 Generaly it looks good to me but I miss some explanation on these flags. Which makes me realize that a more complete documentation under Documentation/S390 for VFIO zPCI as we have for VFIO AP and VFIO CCW would be of great interest. > + __u64 ibv; /* Address of guest interrupt bit vector */ > + __u64 sb; /* Address of guest summary bit */ > + __u32 noi; /* Number of interrupts */ > + __u8 isc; /* Guest interrupt subclass */ > + __u8 sbo; /* Offset of guest summary bit vector */ > +}; > + > #endif >
On 1/19/22 12:10 PM, Pierre Morel wrote: > > > On 1/14/22 21:31, Matthew Rosato wrote: ... >> 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 > > Generaly it looks good to me but I miss some explanation on these flags. 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 > > Which makes me realize that a more complete documentation under > Documentation/S390 for VFIO zPCI as we have for VFIO AP and VFIO CCW > would be of great interest. You're not wrong -- a similar comment came up for QEMU. I will add this to my todo list as a follow-on.
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? > }; > > int kvm_s390_pci_dev_open(struct zpci_dev *zdev); > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index 2b2d64a2190c..01658de660bd 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -1174,6 +1174,8 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, > return 0; > case VFIO_DEVICE_FEATURE_ZPCI_INTERP: > return vfio_pci_zdev_feat_interp(vdev, feature, arg); > + case VFIO_DEVICE_FEATURE_ZPCI_AIF: > + return vfio_pci_zdev_feat_aif(vdev, feature, arg); > default: > return -ENOTTY; > } > diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c > index 4339f48b98bc..891cfa016d63 100644 > --- a/drivers/vfio/pci/vfio_pci_zdev.c > +++ b/drivers/vfio/pci/vfio_pci_zdev.c > @@ -13,6 +13,7 @@ > #include <linux/vfio_zdev.h> > #include <asm/pci_clp.h> > #include <asm/pci_io.h> > +#include <asm/pci_insn.h> > #include <asm/kvm_pci.h> > > #include <linux/vfio_pci_core.h> > @@ -208,6 +209,99 @@ int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, > return rc; > } > > +int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, > + struct vfio_device_feature feature, > + unsigned long arg) > +{ > + struct zpci_dev *zdev = to_zpci(vdev->pdev); > + struct vfio_device_zpci_aif *data; > + struct vfio_device_feature *feat; > + unsigned long minsz; > + int size, rc = 0; > + > + if (!zdev || !zdev->kzdev) > + return -EINVAL; > + > + /* If PROBE specified, return probe results immediately */ > + if (feature.flags & VFIO_DEVICE_FEATURE_PROBE) > + return kvm_s390_pci_aif_probe(zdev); > + > + /* GET and SET are mutually exclusive */ > + if ((feature.flags & VFIO_DEVICE_FEATURE_GET) && > + (feature.flags & VFIO_DEVICE_FEATURE_SET)) > + return -EINVAL; > + > + size = sizeof(*feat) + sizeof(*data); > + feat = kzalloc(size, GFP_KERNEL); > + if (!feat) > + return -ENOMEM; > + > + data = (struct vfio_device_zpci_aif *)&feat->data; > + minsz = offsetofend(struct vfio_device_feature, flags); > + > + if (feature.argsz < minsz + sizeof(*data)) > + return -EINVAL; > + > + /* Get the rest of the payload for GET/SET */ > + rc = copy_from_user(data, (void __user *)(arg + minsz), > + sizeof(*data)); > + if (rc) > + rc = -EINVAL; > + > + if (feature.flags & VFIO_DEVICE_FEATURE_GET) { > + if (zdev->kzdev->aif) > + data->flags = VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT; > + if (zdev->kzdev->fhost) > + data->flags |= VFIO_DEVICE_ZPCI_FLAG_AIF_HOST; > + > + if (copy_to_user((void __user *)arg, feat, size)) > + rc = -EFAULT; > + } else if (feature.flags & VFIO_DEVICE_FEATURE_SET) { > + if (data->flags & VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT) { > + /* create a guest fib */ > + struct zpci_fib fib; > + > + fib.fmt0.aibv = data->ibv; > + fib.fmt0.isc = data->isc; > + fib.fmt0.noi = data->noi; > + if (data->sb != 0) { > + fib.fmt0.aisb = data->sb; > + fib.fmt0.aisbo = data->sbo; > + fib.fmt0.sum = 1; > + } else { > + fib.fmt0.aisb = 0; > + fib.fmt0.aisbo = 0; > + fib.fmt0.sum = 0; > + } > + if (data->flags & VFIO_DEVICE_ZPCI_FLAG_AIF_HOST) { > + rc = kvm_s390_pci_aif_enable(zdev, &fib, false); > + if (!rc) { > + zdev->kzdev->aif = true; > + zdev->kzdev->fhost = true; > + } > + } else { > + rc = kvm_s390_pci_aif_enable(zdev, &fib, true); > + if (!rc) > + zdev->kzdev->aif = true; > + } > + } else if (data->flags == 0) { > + rc = kvm_s390_pci_aif_disable(zdev); > + if (!rc) { > + zdev->kzdev->aif = false; > + zdev->kzdev->fhost = false; > + } > + } else { > + rc = -EINVAL; > + } > + } else { > + /* Neither GET nor SET were specified */ > + rc = -EINVAL; > + } > + > + kfree(feat); > + return rc; > +} > + > static int vfio_pci_zdev_group_notifier(struct notifier_block *nb, > unsigned long action, void *data) > { > @@ -255,8 +349,10 @@ void vfio_pci_zdev_release(struct vfio_pci_core_device *vdev) > * If the device was using interpretation, don't trust that userspace > * did the appropriate cleanup > */ > - if (zdev->gd != 0) > + if (zdev->gd != 0) { > + kvm_s390_pci_aif_disable(zdev); > kvm_s390_pci_interp_disable(zdev); > + } > > kvm_s390_pci_dev_release(zdev); > } > diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h > index 0db2b1051931..7ec5e82e7933 100644 > --- a/include/linux/vfio_pci_core.h > +++ b/include/linux/vfio_pci_core.h > @@ -201,6 +201,9 @@ extern int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev, > int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, > struct vfio_device_feature feature, > unsigned long arg); > +int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, > + struct vfio_device_feature feature, > + unsigned long arg); > void vfio_pci_zdev_open(struct vfio_pci_core_device *vdev); > void vfio_pci_zdev_release(struct vfio_pci_core_device *vdev); > #else > @@ -217,6 +220,13 @@ static inline int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, > return -ENOTTY; > } > > +static inline int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, > + struct vfio_device_feature feature, > + unsigned long arg) > +{ > + return -ENOTTY; > +} > + > static inline void vfio_pci_zdev_open(struct vfio_pci_core_device *vdev) > { > } > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index b9a75485b8e7..fe3bfd99bf50 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -1009,6 +1009,13 @@ struct vfio_device_feature { > */ > #define VFIO_DEVICE_FEATURE_ZPCI_INTERP (1) > > +/* > + * Provide support for enbaling adapter interruption forwarding for zPCI > + * devices. This feature is only valid for s390x PCI devices. Data provided > + * when setting and getting this feature is further described in vfio_zdev.h > + */ > +#define VFIO_DEVICE_FEATURE_ZPCI_AIF (2) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /** > 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 ? > + __u64 ibv; /* Address of guest interrupt bit vector */ > + __u64 sb; /* Address of guest summary bit */ > + __u32 noi; /* Number of interrupts */ > + __u8 isc; /* Guest interrupt subclass */ > + __u8 sbo; /* Offset of guest summary bit vector */ > +}; > + > #endif >
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 */ ... >> 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
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.
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; }; int kvm_s390_pci_dev_open(struct zpci_dev *zdev); diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 2b2d64a2190c..01658de660bd 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1174,6 +1174,8 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, return 0; case VFIO_DEVICE_FEATURE_ZPCI_INTERP: return vfio_pci_zdev_feat_interp(vdev, feature, arg); + case VFIO_DEVICE_FEATURE_ZPCI_AIF: + return vfio_pci_zdev_feat_aif(vdev, feature, arg); default: return -ENOTTY; } diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c index 4339f48b98bc..891cfa016d63 100644 --- a/drivers/vfio/pci/vfio_pci_zdev.c +++ b/drivers/vfio/pci/vfio_pci_zdev.c @@ -13,6 +13,7 @@ #include <linux/vfio_zdev.h> #include <asm/pci_clp.h> #include <asm/pci_io.h> +#include <asm/pci_insn.h> #include <asm/kvm_pci.h> #include <linux/vfio_pci_core.h> @@ -208,6 +209,99 @@ int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, return rc; } +int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, + struct vfio_device_feature feature, + unsigned long arg) +{ + struct zpci_dev *zdev = to_zpci(vdev->pdev); + struct vfio_device_zpci_aif *data; + struct vfio_device_feature *feat; + unsigned long minsz; + int size, rc = 0; + + if (!zdev || !zdev->kzdev) + return -EINVAL; + + /* If PROBE specified, return probe results immediately */ + if (feature.flags & VFIO_DEVICE_FEATURE_PROBE) + return kvm_s390_pci_aif_probe(zdev); + + /* GET and SET are mutually exclusive */ + if ((feature.flags & VFIO_DEVICE_FEATURE_GET) && + (feature.flags & VFIO_DEVICE_FEATURE_SET)) + return -EINVAL; + + size = sizeof(*feat) + sizeof(*data); + feat = kzalloc(size, GFP_KERNEL); + if (!feat) + return -ENOMEM; + + data = (struct vfio_device_zpci_aif *)&feat->data; + minsz = offsetofend(struct vfio_device_feature, flags); + + if (feature.argsz < minsz + sizeof(*data)) + return -EINVAL; + + /* Get the rest of the payload for GET/SET */ + rc = copy_from_user(data, (void __user *)(arg + minsz), + sizeof(*data)); + if (rc) + rc = -EINVAL; + + if (feature.flags & VFIO_DEVICE_FEATURE_GET) { + if (zdev->kzdev->aif) + data->flags = VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT; + if (zdev->kzdev->fhost) + data->flags |= VFIO_DEVICE_ZPCI_FLAG_AIF_HOST; + + if (copy_to_user((void __user *)arg, feat, size)) + rc = -EFAULT; + } else if (feature.flags & VFIO_DEVICE_FEATURE_SET) { + if (data->flags & VFIO_DEVICE_ZPCI_FLAG_AIF_FLOAT) { + /* create a guest fib */ + struct zpci_fib fib; + + fib.fmt0.aibv = data->ibv; + fib.fmt0.isc = data->isc; + fib.fmt0.noi = data->noi; + if (data->sb != 0) { + fib.fmt0.aisb = data->sb; + fib.fmt0.aisbo = data->sbo; + fib.fmt0.sum = 1; + } else { + fib.fmt0.aisb = 0; + fib.fmt0.aisbo = 0; + fib.fmt0.sum = 0; + } + if (data->flags & VFIO_DEVICE_ZPCI_FLAG_AIF_HOST) { + rc = kvm_s390_pci_aif_enable(zdev, &fib, false); + if (!rc) { + zdev->kzdev->aif = true; + zdev->kzdev->fhost = true; + } + } else { + rc = kvm_s390_pci_aif_enable(zdev, &fib, true); + if (!rc) + zdev->kzdev->aif = true; + } + } else if (data->flags == 0) { + rc = kvm_s390_pci_aif_disable(zdev); + if (!rc) { + zdev->kzdev->aif = false; + zdev->kzdev->fhost = false; + } + } else { + rc = -EINVAL; + } + } else { + /* Neither GET nor SET were specified */ + rc = -EINVAL; + } + + kfree(feat); + return rc; +} + static int vfio_pci_zdev_group_notifier(struct notifier_block *nb, unsigned long action, void *data) { @@ -255,8 +349,10 @@ void vfio_pci_zdev_release(struct vfio_pci_core_device *vdev) * If the device was using interpretation, don't trust that userspace * did the appropriate cleanup */ - if (zdev->gd != 0) + if (zdev->gd != 0) { + kvm_s390_pci_aif_disable(zdev); kvm_s390_pci_interp_disable(zdev); + } kvm_s390_pci_dev_release(zdev); } diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index 0db2b1051931..7ec5e82e7933 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -201,6 +201,9 @@ extern int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev, int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, struct vfio_device_feature feature, unsigned long arg); +int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, + struct vfio_device_feature feature, + unsigned long arg); void vfio_pci_zdev_open(struct vfio_pci_core_device *vdev); void vfio_pci_zdev_release(struct vfio_pci_core_device *vdev); #else @@ -217,6 +220,13 @@ static inline int vfio_pci_zdev_feat_interp(struct vfio_pci_core_device *vdev, return -ENOTTY; } +static inline int vfio_pci_zdev_feat_aif(struct vfio_pci_core_device *vdev, + struct vfio_device_feature feature, + unsigned long arg) +{ + return -ENOTTY; +} + static inline void vfio_pci_zdev_open(struct vfio_pci_core_device *vdev) { } diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index b9a75485b8e7..fe3bfd99bf50 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -1009,6 +1009,13 @@ struct vfio_device_feature { */ #define VFIO_DEVICE_FEATURE_ZPCI_INTERP (1) +/* + * Provide support for enbaling adapter interruption forwarding for zPCI + * devices. This feature is only valid for s390x PCI devices. Data provided + * when setting and getting this feature is further described in vfio_zdev.h + */ +#define VFIO_DEVICE_FEATURE_ZPCI_AIF (2) + /* -------- API for Type1 VFIO IOMMU -------- */ /** 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 + __u64 ibv; /* Address of guest interrupt bit vector */ + __u64 sb; /* Address of guest summary bit */ + __u32 noi; /* Number of interrupts */ + __u8 isc; /* Guest interrupt subclass */ + __u8 sbo; /* Offset of guest summary bit vector */ +}; + #endif
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(-)