Messages in this thread | | | Date | Mon, 16 May 2022 11:52:00 +0200 | From | Thomas Huth <> | Subject | Re: [PATCH v7 20/22] KVM: s390: add KVM_S390_ZPCI_OP to manage guest zPCI devices |
| |
On 13/05/2022 21.15, Matthew Rosato wrote: > The KVM_S390_ZPCI_OP ioctl provides a mechanism for managing > hardware-assisted virtualization features for s390X zPCI passthrough.
s/s390X/s390x/
> Add the first 2 operations, which can be used to enable/disable > the specified device for Adapter Event Notification interpretation. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > Documentation/virt/kvm/api.rst | 45 +++++++++++++++++++ > arch/s390/kvm/kvm-s390.c | 23 ++++++++++ > arch/s390/kvm/pci.c | 81 ++++++++++++++++++++++++++++++++++ > arch/s390/kvm/pci.h | 2 + > include/uapi/linux/kvm.h | 31 +++++++++++++ > 5 files changed, 182 insertions(+) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 4a900cdbc62e..a7cd5ebce031 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -5645,6 +5645,51 @@ enabled with ``arch_prctl()``, but this may change in the future. > The offsets of the state save areas in struct kvm_xsave follow the contents > of CPUID leaf 0xD on the host. > > +4.135 KVM_S390_ZPCI_OP > +-------------------- > + > +:Capability: KVM_CAP_S390_ZPCI_OP > +:Architectures: s390 > +:Type: vcpu ioctl
vcpu? ... you're wiring it up in kvm_arch_vm_ioctl() later, so I assume it's rather a VM ioctl?
> +:Parameters: struct kvm_s390_zpci_op (in) > +:Returns: 0 on success, <0 on error > + > +Used to manage hardware-assisted virtualization features for zPCI devices. > + > +Parameters are specified via the following structure:: > + > + struct kvm_s390_zpci_op { > + /* in */
If all is "in", why is there a copy_to_user() in the code later?
> + __u32 fh; /* target device */ > + __u8 op; /* operation to perform */ > + __u8 pad[3]; > + union { > + /* for KVM_S390_ZPCIOP_REG_AEN */ > + struct { > + __u64 ibv; /* Guest addr of interrupt bit vector */ > + __u64 sb; /* Guest addr of summary bit */
If this is really a vcpu ioctl, what kind of addresses are you talking about here? virtual addresses? real addresses? absolute addresses?
> + __u32 flags; > + __u32 noi; /* Number of interrupts */ > + __u8 isc; /* Guest interrupt subclass */ > + __u8 sbo; /* Offset of guest summary bit vector */ > + __u16 pad; > + } reg_aen; > + __u64 reserved[8]; > + } u; > + }; > + > +The type of operation is specified in the "op" field. > +KVM_S390_ZPCIOP_REG_AEN is used to register the VM for adapter event > +notification interpretation, which will allow firmware delivery of adapter > +events directly to the vm, with KVM providing a backup delivery mechanism; > +KVM_S390_ZPCIOP_DEREG_AEN is used to subsequently disable interpretation of > +adapter event notifications. > + > +The target zPCI function must also be specified via the "fh" field. For the > +KVM_S390_ZPCIOP_REG_AEN operation, additional information to establish firmware > +delivery must be provided via the "reg_aen" struct. > + > +The "reserved" field is meant for future extensions.
Maybe also mention the "pad" fields? And add should these also be initialized to 0 by the calling userspace program?
> 5. The kvm_run structure > ======================== > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index b95b25490018..1af7cea9d579 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -618,6 +618,12 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_S390_PROTECTED: > r = is_prot_virt_host(); > break; > + case KVM_CAP_S390_ZPCI_OP: > + if (kvm_s390_pci_interp_allowed()) > + r = 1; > + else > + r = 0;
Could be shortened to:
r = kvm_s390_pci_interp_allowed();
> + break; > default: > r = 0; > } > @@ -2633,6 +2639,23 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = -EFAULT; > break; > } > + case KVM_S390_ZPCI_OP: { > + struct kvm_s390_zpci_op args; > + > + r = -EINVAL; > + if (!IS_ENABLED(CONFIG_VFIO_PCI_ZDEV_KVM)) > + break; > + if (copy_from_user(&args, argp, sizeof(args))) { > + r = -EFAULT; > + break; > + } > + r = kvm_s390_pci_zpci_op(kvm, &args); > + if (r) > + break; > + if (copy_to_user(argp, &args, sizeof(args))) > + r = -EFAULT;
So this copy_to_user() indicates that information is returned to userspace ... but below, the ioctl is declared with _IOW only ... this does not match. Should it be declared with _IOWR or should the copy_to_user() be dropped?
> + break; > + } > default: > r = -ENOTTY; > } > diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c > index 1393a1604494..6e6254016be4 100644 > --- a/arch/s390/kvm/pci.c > +++ b/arch/s390/kvm/pci.c > @@ -585,6 +585,87 @@ void kvm_s390_pci_clear_list(struct kvm *kvm) > spin_unlock(&kvm->arch.kzdev_list_lock); > } > > +static struct zpci_dev *get_zdev_from_kvm_by_fh(struct kvm *kvm, u32 fh) > +{ > + struct zpci_dev *zdev = NULL; > + struct kvm_zdev *kzdev; > + > + spin_lock(&kvm->arch.kzdev_list_lock); > + list_for_each_entry(kzdev, &kvm->arch.kzdev_list, entry) { > + if (kzdev->zdev->fh == fh) { > + zdev = kzdev->zdev; > + break; > + } > + } > + spin_unlock(&kvm->arch.kzdev_list_lock); > + > + return zdev; > +} > + > +static int kvm_s390_pci_zpci_reg_aen(struct zpci_dev *zdev, > + struct kvm_s390_zpci_op *args) > +{ > + struct zpci_fib fib = {}; > + > + fib.fmt0.aibv = args->u.reg_aen.ibv; > + fib.fmt0.isc = args->u.reg_aen.isc; > + fib.fmt0.noi = args->u.reg_aen.noi; > + if (args->u.reg_aen.sb != 0) { > + fib.fmt0.aisb = args->u.reg_aen.sb; > + fib.fmt0.aisbo = args->u.reg_aen.sbo; > + fib.fmt0.sum = 1; > + } else { > + fib.fmt0.aisb = 0; > + fib.fmt0.aisbo = 0; > + fib.fmt0.sum = 0; > + } > + > + if (args->u.reg_aen.flags & KVM_S390_ZPCIOP_REGAEN_HOST) > + return kvm_s390_pci_aif_enable(zdev, &fib, true); > + else > + return kvm_s390_pci_aif_enable(zdev, &fib, false);
Alternatively (just a matter of taste):
bool hostflag; ... hostflag = (args->u.reg_aen.flags & KVM_S390_ZPCIOP_REGAEN_HOST); return kvm_s390_pci_aif_enable(zdev, &fib, hostflag);
> +} > + > +int kvm_s390_pci_zpci_op(struct kvm *kvm, struct kvm_s390_zpci_op *args) > +{ > + struct kvm_zdev *kzdev; > + struct zpci_dev *zdev; > + int r; > + > + zdev = get_zdev_from_kvm_by_fh(kvm, args->fh); > + if (!zdev) > + return -ENODEV; > + > + mutex_lock(&zdev->kzdev_lock); > + mutex_lock(&kvm->lock); > + > + kzdev = zdev->kzdev; > + if (!kzdev) { > + r = -ENODEV; > + goto out; > + } > + if (kzdev->kvm != kvm) { > + r = -EPERM; > + goto out; > + } > + > + switch (args->op) { > + case KVM_S390_ZPCIOP_REG_AEN:
Please also check here that args->u.reg_aen.flags does not have any bits set that we don't support here (otherwise, this could cause some trouble when introducing additional flags later).
> + r = kvm_s390_pci_zpci_reg_aen(zdev, args); > + break; > + case KVM_S390_ZPCIOP_DEREG_AEN: > + r = kvm_s390_pci_aif_disable(zdev, false); > + break; > + default: > + r = -EINVAL; > + } > + > +out: > + mutex_unlock(&kvm->lock); > + mutex_unlock(&zdev->kzdev_lock); > + return r; > +} > + > int kvm_s390_pci_init(void) > { > aift = kzalloc(sizeof(struct zpci_aift), GFP_KERNEL); > diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h > index fb2b91b76e0c..0351382e990f 100644 > --- a/arch/s390/kvm/pci.h > +++ b/arch/s390/kvm/pci.h > @@ -59,6 +59,8 @@ void kvm_s390_pci_aen_exit(void); > void kvm_s390_pci_init_list(struct kvm *kvm); > void kvm_s390_pci_clear_list(struct kvm *kvm); > > +int kvm_s390_pci_zpci_op(struct kvm *kvm, struct kvm_s390_zpci_op *args); > + > int kvm_s390_pci_init(void); > void kvm_s390_pci_exit(void); > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 6a184d260c7f..1d3d41523d10 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1152,6 +1152,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_DISABLE_QUIRKS2 213 > /* #define KVM_CAP_VM_TSC_CONTROL 214 */ > #define KVM_CAP_SYSTEM_EVENT_DATA 215 > +#define KVM_CAP_S390_ZPCI_OP 216 > > #ifdef KVM_CAP_IRQ_ROUTING > > @@ -2068,4 +2069,34 @@ struct kvm_stats_desc { > /* Available with KVM_CAP_XSAVE2 */ > #define KVM_GET_XSAVE2 _IOR(KVMIO, 0xcf, struct kvm_xsave) > > +/* Available with KVM_CAP_S390_ZPCI_OP */ > +#define KVM_S390_ZPCI_OP _IOW(KVMIO, 0xd0, struct kvm_s390_zpci_op)
Please double-check whether this should be _IOWR instead (see above).
> +struct kvm_s390_zpci_op { > + /* in */ > + __u32 fh; /* target device */ > + __u8 op; /* operation to perform */ > + __u8 pad[3]; > + union { > + /* for KVM_S390_ZPCIOP_REG_AEN */ > + struct { > + __u64 ibv; /* Guest addr of interrupt bit vector */ > + __u64 sb; /* Guest addr of summary bit */ > + __u32 flags; > + __u32 noi; /* Number of interrupts */ > + __u8 isc; /* Guest interrupt subclass */ > + __u8 sbo; /* Offset of guest summary bit vector */ > + __u16 pad; > + } reg_aen; > + __u64 reserved[8]; > + } u; > +}; > + > +/* types for kvm_s390_zpci_op->op */ > +#define KVM_S390_ZPCIOP_REG_AEN 0 > +#define KVM_S390_ZPCIOP_DEREG_AEN 1 > + > +/* flags for kvm_s390_zpci_op->u.reg_aen.flags */ > +#define KVM_S390_ZPCIOP_REGAEN_HOST (1 << 0) > + > #endif /* __LINUX_KVM_H */
Thomas
| |