On Thu, Feb 17, 2022 at 06:37:15AM -0500, Steffen Eiden wrote:
> This patch adds a new miscdevice to expose some Ultravisor functions
> to userspace. Userspace can send IOCTLis to the uvdevice that will then
> emit a corresponding Ultravisor Call and hands the result over to
> userspace. The uvdevice is available if the Ultravisor Call facility is
> present.
>
> Userspace is now able to call the Query Ultravisor Information
> Ultravisor Command through the uvdevice.
>
> Signed-off-by: Steffen Eiden <[email protected]>
> ---
> MAINTAINERS | 2 +
> arch/s390/include/uapi/asm/uvdevice.h | 27 +++++
> drivers/s390/char/Kconfig | 9 ++
> drivers/s390/char/Makefile | 1 +
> drivers/s390/char/uvdevice.c | 162 ++++++++++++++++++++++++++
> 5 files changed, 201 insertions(+)
> create mode 100644 arch/s390/include/uapi/asm/uvdevice.h
> create mode 100644 drivers/s390/char/uvdevice.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5250298d2817..c7d8d0fe48cf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10457,9 +10457,11 @@ F: Documentation/virt/kvm/s390*
> F: arch/s390/include/asm/gmap.h
> F: arch/s390/include/asm/kvm*
> F: arch/s390/include/uapi/asm/kvm*
> +F: arch/s390/include/uapi/asm/uvdevice.h
> F: arch/s390/kernel/uv.c
> F: arch/s390/kvm/
> F: arch/s390/mm/gmap.c
> +F: drivers/s390/char/uvdevice.c
> F: tools/testing/selftests/kvm/*/s390x/
> F: tools/testing/selftests/kvm/s390x/
>
> diff --git a/arch/s390/include/uapi/asm/uvdevice.h b/arch/s390/include/uapi/asm/uvdevice.h
> new file mode 100644
> index 000000000000..f2e4984a6e2e
> --- /dev/null
> +++ b/arch/s390/include/uapi/asm/uvdevice.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright IBM Corp. 2022
> + * Author(s): Steffen Eiden <[email protected]>
> + */
> +#ifndef __S390X_ASM_UVDEVICE_H
> +#define __S390X_ASM_UVDEVICE_H
> +
> +#include <linux/types.h>
> +
> +struct uvio_ioctl_cb {
> + __u32 flags; /* Currently no flags defined, must be zero */
> + __u16 uv_rc; /* UV header rc value */
> + __u16 uv_rrc; /* UV header rrc value */
> + __u64 argument_addr; /* Userspace address of uvio argument */
> + __u32 argument_len;
> + __u8 reserved14[0x40 - 0x14]; /* must be zero */
> +};
> +
> +#define UVIO_QUI_MAX_LEN 0x8000
> +
> +#define UVIO_DEVICE_NAME "uv"
> +#define UVIO_TYPE_UVC 'u'
> +
> +#define UVIO_IOCTL_QUI _IOWR(UVIO_TYPE_UVC, 0x01, struct uvio_ioctl_cb)
> +
> +#endif /* __S390X_ASM_UVDEVICE_H */
> diff --git a/drivers/s390/char/Kconfig b/drivers/s390/char/Kconfig
> index 6cc4b19acf85..933c0d0062d6 100644
> --- a/drivers/s390/char/Kconfig
> +++ b/drivers/s390/char/Kconfig
> @@ -184,3 +184,12 @@ config S390_VMUR
> depends on S390
> help
> Character device driver for z/VM reader, puncher and printer.
> +
> +config UV_UAPI
> + def_tristate m
> + prompt "Ultravisor userspace API"
> + depends on PROTECTED_VIRTUALIZATION_GUEST
> + help
> + Selecting exposes parts of the UV interface to userspace
> + by providing a misc character device. Using IOCTLs one
> + can interact with the UV.
> diff --git a/drivers/s390/char/Makefile b/drivers/s390/char/Makefile
> index c6fdb81a068a..b5c83092210e 100644
> --- a/drivers/s390/char/Makefile
> +++ b/drivers/s390/char/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_MONREADER) += monreader.o
> obj-$(CONFIG_MONWRITER) += monwriter.o
> obj-$(CONFIG_S390_VMUR) += vmur.o
> obj-$(CONFIG_CRASH_DUMP) += sclp_sdias.o zcore.o
> +obj-$(CONFIG_UV_UAPI) += uvdevice.o
>
> hmcdrv-objs := hmcdrv_mod.o hmcdrv_dev.o hmcdrv_ftp.o hmcdrv_cache.o diag_ftp.o sclp_ftp.o
> obj-$(CONFIG_HMC_DRV) += hmcdrv.o
> diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
> new file mode 100644
> index 000000000000..e8efcbf0e7ab
> --- /dev/null
> +++ b/drivers/s390/char/uvdevice.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright IBM Corp. 2022
> + * Author(s): Steffen Eiden <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt ".\n"
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/types.h>
> +#include <linux/stddef.h>
> +#include <linux/vmalloc.h>
> +#include <linux/slab.h>
> +
> +#include <asm/uvdevice.h>
> +#include <asm/uv.h>
> +
> +/**
> + * uvio_qui() - Perform a Query Ultravisor Information UVC.
> + *
> + * uv_ioctl: ioctl control block
> + *
> + * uvio_qui() does a Query Ultravisor Information (QUI) Ultravisor Call.
> + * It creates the uvc qui request and sends it to the Ultravisor. After that
> + * it copies the response to userspace and fills the rc and rrc of uv_ioctl
> + * uv_call with the response values of the Ultravisor.
> + *
> + * Create the UVC structure, send the UVC to UV and write the response in the ioctl struct.
> + *
> + * Return: 0 on success or a negative error code on error.
> + */
> +static int uvio_qui(struct uvio_ioctl_cb *uv_ioctl)
> +{
> + u8 __user *user_buf_addr = (__user u8 *)uv_ioctl->argument_addr;
> + size_t user_buf_len = uv_ioctl->argument_len;
> + struct uv_cb_header *uvcb_qui = NULL;
> + int ret;
> +
> + /*
> + * Do not check for a too small buffer. If userspace provides a buffer
> + * that is too small the Ultravisor will complain.
> + */
> + ret = -EINVAL;
> + if (!user_buf_len || user_buf_len > UVIO_QUI_MAX_LEN)
> + goto out;
> + ret = -ENOMEM;
> + uvcb_qui = kvzalloc(user_buf_len, GFP_KERNEL);
> + if (!uvcb_qui)
> + goto out;
> + uvcb_qui->len = user_buf_len;
> + uvcb_qui->cmd = UVC_CMD_QUI;
> +
> + uv_call(0, (u64)uvcb_qui);
> +
> + ret = -EFAULT;
> + if (copy_to_user(user_buf_addr, uvcb_qui, uvcb_qui->len))
> + goto out;
> + uv_ioctl->uv_rc = uvcb_qui->rc;
> + uv_ioctl->uv_rrc = uvcb_qui->rrc;
> +
> + ret = 0;
> +out:
> + kvfree(uvcb_qui);
> + return ret;
> +}
> +
> +static int uvio_copy_and_check_ioctl(struct uvio_ioctl_cb *ioctl, void __user *argp)
> +{
> + u64 sum = 0;
> + u64 i;
> +
> + if (copy_from_user(ioctl, argp, sizeof(*ioctl)))
> + return -EFAULT;
> + if (ioctl->flags != 0)
> + return -EINVAL;
> + for (i = 0; i < ARRAY_SIZE(ioctl->reserved14); i++)
> + sum += ioctl->reserved14[i];
> + if (sum)
> + return -EINVAL;
So you can have -1, 1, -1, 1, and so on and cause this to be an
incorrect check. Just test for 0 and bail out early please.
> +
> + return 0;
> +}
> +
> +static int uvio_dev_open(struct inode *inode, struct file *filp)
> +{
> + return 0;
> +}
> +
> +static int uvio_dev_close(struct inode *inodep, struct file *filp)
> +{
> + return 0;
> +}
If open/close do nothing, no need to provide it at all, just drop them.
> +
> +/*
> + * IOCTL entry point for the Ultravisor device.
> + */
> +static long uvio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + void __user *argp = (void __user *)arg;
> + struct uvio_ioctl_cb *uv_ioctl;
> + long ret;
> +
> + ret = -ENOMEM;
> + uv_ioctl = vzalloc(sizeof(*uv_ioctl));
> + if (!uv_ioctl)
> + goto out;
> +
> + switch (cmd) {
> + case UVIO_IOCTL_QUI:
> + ret = uvio_copy_and_check_ioctl(uv_ioctl, argp);
> + if (ret)
> + goto out;
> + ret = uvio_qui(uv_ioctl);
> + break;
> + default:
> + ret = -EINVAL;
Wrong error value :(
> + break;
> + }
> + if (ret)
> + goto out;
> +
> + if (copy_to_user(argp, uv_ioctl, sizeof(*uv_ioctl)))
> + ret = -EFAULT;
> +
> + out:
> + vfree(uv_ioctl);
> + return ret;
> +}
> +
> +static const struct file_operations uvio_dev_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = uvio_ioctl,
> + .open = uvio_dev_open,
> + .release = uvio_dev_close,
> + .llseek = no_llseek,
> +};
> +
> +static struct miscdevice uvio_dev_miscdev = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = UVIO_DEVICE_NAME,
> + .fops = &uvio_dev_fops,
> +};
> +
> +static void __exit uvio_dev_exit(void)
> +{
> + misc_deregister(&uvio_dev_miscdev);
> +}
> +
> +static int __init uvio_dev_init(void)
> +{
> + if (!test_facility(158))
> + return -ENXIO;
> + return misc_register(&uvio_dev_miscdev);
> +}
> +
> +module_init(uvio_dev_init);
> +module_exit(uvio_dev_exit);
> +
> +MODULE_AUTHOR("IBM Corporation");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Ultravisor UAPI driver");
Nothing to cause this to automatically be loaded when the "hardware" is
present?
thanks,
greg k-h
Hey Greg,
thanks for your review.
On 2/17/22 13:33, Greg KH wrote:
> On Thu, Feb 17, 2022 at 06:37:15AM -0500, Steffen Eiden wrote:
>> This patch adds a new miscdevice to expose some Ultravisor functions
>> to userspace. Userspace can send IOCTLis to the uvdevice that will then
>> emit a corresponding Ultravisor Call and hands the result over to
>> userspace. The uvdevice is available if the Ultravisor Call facility is
>> present.
>>
>> Userspace is now able to call the Query Ultravisor Information
>> Ultravisor Command through the uvdevice.
>>
>> Signed-off-by: Steffen Eiden <[email protected]>
>> ---
>> MAINTAINERS | 2 +
>> arch/s390/include/uapi/asm/uvdevice.h | 27 +++++
>> drivers/s390/char/Kconfig | 9 ++
>> drivers/s390/char/Makefile | 1 +
>> drivers/s390/char/uvdevice.c | 162 ++++++++++++++++++++++++++
>> 5 files changed, 201 insertions(+)
>> create mode 100644 arch/s390/include/uapi/asm/uvdevice.h
>> create mode 100644 drivers/s390/char/uvdevice.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5250298d2817..c7d8d0fe48cf 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10457,9 +10457,11 @@ F: Documentation/virt/kvm/s390*
>> F: arch/s390/include/asm/gmap.h
>> F: arch/s390/include/asm/kvm*
>> F: arch/s390/include/uapi/asm/kvm*
>> +F: arch/s390/include/uapi/asm/uvdevice.h
>> F: arch/s390/kernel/uv.c
>> F: arch/s390/kvm/
>> F: arch/s390/mm/gmap.c
>> +F: drivers/s390/char/uvdevice.c
>> F: tools/testing/selftests/kvm/*/s390x/
>> F: tools/testing/selftests/kvm/s390x/
>>
>> diff --git a/arch/s390/include/uapi/asm/uvdevice.h b/arch/s390/include/uapi/asm/uvdevice.h
>> new file mode 100644
>> index 000000000000..f2e4984a6e2e
>> --- /dev/null
>> +++ b/arch/s390/include/uapi/asm/uvdevice.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Steffen Eiden <[email protected]>
>> + */
>> +#ifndef __S390X_ASM_UVDEVICE_H
>> +#define __S390X_ASM_UVDEVICE_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct uvio_ioctl_cb {
>> + __u32 flags; /* Currently no flags defined, must be zero */
>> + __u16 uv_rc; /* UV header rc value */
>> + __u16 uv_rrc; /* UV header rrc value */
>> + __u64 argument_addr; /* Userspace address of uvio argument */
>> + __u32 argument_len;
>> + __u8 reserved14[0x40 - 0x14]; /* must be zero */
>> +};
>> +
>> +#define UVIO_QUI_MAX_LEN 0x8000
>> +
>> +#define UVIO_DEVICE_NAME "uv"
>> +#define UVIO_TYPE_UVC 'u'
>> +
>> +#define UVIO_IOCTL_QUI _IOWR(UVIO_TYPE_UVC, 0x01, struct uvio_ioctl_cb)
>> +
>> +#endif /* __S390X_ASM_UVDEVICE_H */
>> diff --git a/drivers/s390/char/Kconfig b/drivers/s390/char/Kconfig
>> index 6cc4b19acf85..933c0d0062d6 100644
>> --- a/drivers/s390/char/Kconfig
>> +++ b/drivers/s390/char/Kconfig
>> @@ -184,3 +184,12 @@ config S390_VMUR
>> depends on S390
>> help
>> Character device driver for z/VM reader, puncher and printer.
>> +
>> +config UV_UAPI
>> + def_tristate m
>> + prompt "Ultravisor userspace API"
>> + depends on PROTECTED_VIRTUALIZATION_GUEST
>> + help
>> + Selecting exposes parts of the UV interface to userspace
>> + by providing a misc character device. Using IOCTLs one
>> + can interact with the UV.
>> diff --git a/drivers/s390/char/Makefile b/drivers/s390/char/Makefile
>> index c6fdb81a068a..b5c83092210e 100644
>> --- a/drivers/s390/char/Makefile
>> +++ b/drivers/s390/char/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MONREADER) += monreader.o
>> obj-$(CONFIG_MONWRITER) += monwriter.o
>> obj-$(CONFIG_S390_VMUR) += vmur.o
>> obj-$(CONFIG_CRASH_DUMP) += sclp_sdias.o zcore.o
>> +obj-$(CONFIG_UV_UAPI) += uvdevice.o
>>
>> hmcdrv-objs := hmcdrv_mod.o hmcdrv_dev.o hmcdrv_ftp.o hmcdrv_cache.o diag_ftp.o sclp_ftp.o
>> obj-$(CONFIG_HMC_DRV) += hmcdrv.o
>> diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
>> new file mode 100644
>> index 000000000000..e8efcbf0e7ab
>> --- /dev/null
>> +++ b/drivers/s390/char/uvdevice.c
>> @@ -0,0 +1,162 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Steffen Eiden <[email protected]>
>> + */
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt ".\n"
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/types.h>
>> +#include <linux/stddef.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/slab.h>
>> +
>> +#include <asm/uvdevice.h>
>> +#include <asm/uv.h>
>> +
>> +/**
>> + * uvio_qui() - Perform a Query Ultravisor Information UVC.
>> + *
>> + * uv_ioctl: ioctl control block
>> + *
>> + * uvio_qui() does a Query Ultravisor Information (QUI) Ultravisor Call.
>> + * It creates the uvc qui request and sends it to the Ultravisor. After that
>> + * it copies the response to userspace and fills the rc and rrc of uv_ioctl
>> + * uv_call with the response values of the Ultravisor.
>> + *
>> + * Create the UVC structure, send the UVC to UV and write the response in the ioctl struct.
>> + *
>> + * Return: 0 on success or a negative error code on error.
>> + */
>> +static int uvio_qui(struct uvio_ioctl_cb *uv_ioctl)
>> +{
>> + u8 __user *user_buf_addr = (__user u8 *)uv_ioctl->argument_addr;
>> + size_t user_buf_len = uv_ioctl->argument_len;
>> + struct uv_cb_header *uvcb_qui = NULL;
>> + int ret;
>> +
>> + /*
>> + * Do not check for a too small buffer. If userspace provides a buffer
>> + * that is too small the Ultravisor will complain.
>> + */
>> + ret = -EINVAL;
>> + if (!user_buf_len || user_buf_len > UVIO_QUI_MAX_LEN)
>> + goto out;
>> + ret = -ENOMEM;
>> + uvcb_qui = kvzalloc(user_buf_len, GFP_KERNEL);
>> + if (!uvcb_qui)
>> + goto out;
>> + uvcb_qui->len = user_buf_len;
>> + uvcb_qui->cmd = UVC_CMD_QUI;
>> +
>> + uv_call(0, (u64)uvcb_qui);
>> +
>> + ret = -EFAULT;
>> + if (copy_to_user(user_buf_addr, uvcb_qui, uvcb_qui->len))
>> + goto out;
>> + uv_ioctl->uv_rc = uvcb_qui->rc;
>> + uv_ioctl->uv_rrc = uvcb_qui->rrc;
>> +
>> + ret = 0;
>> +out:
>> + kvfree(uvcb_qui);
>> + return ret;
>> +}
>> +
>> +static int uvio_copy_and_check_ioctl(struct uvio_ioctl_cb *ioctl, void __user *argp)
>> +{
>> + u64 sum = 0;
>> + u64 i;
>> +
>> + if (copy_from_user(ioctl, argp, sizeof(*ioctl)))
>> + return -EFAULT;
>> + if (ioctl->flags != 0)
>> + return -EINVAL;
>> + for (i = 0; i < ARRAY_SIZE(ioctl->reserved14); i++)
>> + sum += ioctl->reserved14[i];
>> + if (sum)
>> + return -EINVAL;
>
> So you can have -1, 1, -1, 1, and so on and cause this to be an
> incorrect check. Just test for 0 and bail out early please.
These ints are unsigned, your szenario cannot happen. However, I changed
it to bailout early anyway.
>
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int uvio_dev_open(struct inode *inode, struct file *filp)
>> +{
>> + return 0;
>> +}
>> +
>> +static int uvio_dev_close(struct inode *inodep, struct file *filp)
>> +{
>> + return 0;
>> +}
>
> If open/close do nothing, no need to provide it at all, just drop them.
Makes sense.
>
>> +
>> +/*
>> + * IOCTL entry point for the Ultravisor device.
>> + */
>> +static long uvio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> + void __user *argp = (void __user *)arg;
>> + struct uvio_ioctl_cb *uv_ioctl;
>> + long ret;
>> +
>> + ret = -ENOMEM;
>> + uv_ioctl = vzalloc(sizeof(*uv_ioctl));
>> + if (!uv_ioctl)
>> + goto out;
>> +
>> + switch (cmd) {
>> + case UVIO_IOCTL_QUI:
>> + ret = uvio_copy_and_check_ioctl(uv_ioctl, argp);
>> + if (ret)
>> + goto out;
>> + ret = uvio_qui(uv_ioctl);
>> + break;
>> + default:
>> + ret = -EINVAL;
>
> Wrong error value :(
changed
>
>> + break;
>> + }
>> + if (ret)
>> + goto out;
>> +
>> + if (copy_to_user(argp, uv_ioctl, sizeof(*uv_ioctl)))
>> + ret = -EFAULT;
>> +
>> + out:
>> + vfree(uv_ioctl);
>> + return ret;
>> +}
>> +
>> +static const struct file_operations uvio_dev_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = uvio_ioctl,
>> + .open = uvio_dev_open,
>> + .release = uvio_dev_close,
>> + .llseek = no_llseek,
>> +};
>> +
>> +static struct miscdevice uvio_dev_miscdev = {
>> + .minor = MISC_DYNAMIC_MINOR,
>> + .name = UVIO_DEVICE_NAME,
>> + .fops = &uvio_dev_fops,
>> +};
>> +
>> +static void __exit uvio_dev_exit(void)
>> +{
>> + misc_deregister(&uvio_dev_miscdev);
>> +}
>> +
>> +static int __init uvio_dev_init(void)
>> +{
>> + if (!test_facility(158))
>> + return -ENXIO;
>> + return misc_register(&uvio_dev_miscdev);
>> +}
>> +
>> +module_init(uvio_dev_init);
>> +module_exit(uvio_dev_exit);
>> +
>> +MODULE_AUTHOR("IBM Corporation");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Ultravisor UAPI driver");
>
> Nothing to cause this to automatically be loaded when the "hardware" is
> present?
We do not have anything like a s390 facility bus which could trigger
such automatic loads. Developing such a bus would be an overkill.
However we could do the approach e.g. kvm-s390 takes. Define
MODULE_ALIAS(devname:uv) that will trigger an automatic module load if
someone calls open on /dev/uv the first time.
IIRC we need to define a fixed misc minor number with this approach.
something like that:
--- a/drivers/s390/char/uvdevice.c
+++ b/drivers/s390/char/uvdevice.c
@@ -309,3 +309,5
MODULE_AUTHOR("IBM Corporation");
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Ultravisor UAPI driver");
+MODULE_ALIAS_MISCDEV(SOME_FIXED_MISC_MINOR);
+MODULE_ALIAS("devname:uv");
We then maybe need to discuss if 'uv' is unique enough.
>
> thanks,
>
> greg k-h
>
Steffen