| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0c6a4f7b-f43a-8f4c-49bb-db10ca010f1f@linux.ibm.com>
Date: Mon, 16 May 2022 11:35:28 -0400
From: Matthew Rosato <mjrosato@...ux.ibm.com>
To: Thomas Huth <thuth@...hat.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 5/16/22 5:52 AM, Thomas Huth wrote:
> 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?
Yup, VM ioctl, bad copy/paste job...
>
>> +: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?
>
Oh no, this is a leftover from a prior version... Good catch. There
should no longer be a copy_to_user.
>> + __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?
It's a VM ioctl. These are guest kernel physical addresses that are
later pinned in arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() as part of
handling the ioctl.
>
>> + __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?
Sure, I can mention them. And yes, I agree that userspace should
initialize them to 0, I'll update the QEMU series accordingly.
>
>> 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?
copy_to_user should be dropped. Thanks!
>
>> + 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).
Good idea, will do
>
>> + 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). >
As mentioned above, the copy_to_user() should be removed.
>> +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