Date:   Mon, 16 May 2022 11:52:00 +0200
From:   Thomas Huth <thuth@...hat.com>
To:     Matthew Rosato <mjrosato@...ux.ibm.com>, linux-s390@...r.kernel.org
Cc:     alex.williamson@...hat.com, cohuck@...hat.com,
        schnelle@...ux.ibm.com, farman@...ux.ibm.com, pmorel@...ux.ibm.com,
        borntraeger@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, agordeev@...ux.ibm.com,
        svens@...ux.ibm.com, frankja@...ux.ibm.com, david@...hat.com,
        imbrenda@...ux.ibm.com, vneethv@...ux.ibm.com,
        oberpar@...ux.ibm.com, freude@...ux.ibm.com, pasic@...ux.ibm.com,
        pbonzini@...hat.com, corbet@....net, jgg@...dia.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org
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@...ux.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

Powered by blists - more mailing lists