| Message ID | 20230304010632.2127470-1-quic_eberman@quicinc.com |
|---|---|
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
On 04/03/2023 01:06, Elliot Berman wrote: > Add hypercalls to identify when Linux is running a virtual machine under > Gunyah. > > There are two calls to help identify Gunyah: > > 1. gh_hypercall_get_uid() returns a UID when running under a Gunyah > hypervisor. > 2. gh_hypercall_hyp_identify() returns build information and a set of > feature flags that are supported by Gunyah. > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > arch/arm64/Kbuild | 1 + > arch/arm64/gunyah/Makefile | 3 ++ > arch/arm64/gunyah/gunyah_hypercall.c | 64 ++++++++++++++++++++++++++++ > drivers/virt/Kconfig | 2 + > drivers/virt/gunyah/Kconfig | 13 ++++++ > include/linux/gunyah.h | 28 ++++++++++++ > 6 files changed, 111 insertions(+) > create mode 100644 arch/arm64/gunyah/Makefile > create mode 100644 arch/arm64/gunyah/gunyah_hypercall.c > create mode 100644 drivers/virt/gunyah/Kconfig > > diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild > index 5bfbf7d79c99..e4847ba0e3c9 100644 > --- a/arch/arm64/Kbuild > +++ b/arch/arm64/Kbuild > @@ -3,6 +3,7 @@ obj-y += kernel/ mm/ net/ > obj-$(CONFIG_KVM) += kvm/ > obj-$(CONFIG_XEN) += xen/ > obj-$(subst m,y,$(CONFIG_HYPERV)) += hyperv/ > +obj-$(CONFIG_GUNYAH) += gunyah/ > obj-$(CONFIG_CRYPTO) += crypto/ > > # for cleaning > diff --git a/arch/arm64/gunyah/Makefile b/arch/arm64/gunyah/Makefile > new file mode 100644 > index 000000000000..84f1e38cafb1 > --- /dev/null > +++ b/arch/arm64/gunyah/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +obj-$(CONFIG_GUNYAH) += gunyah_hypercall.o > diff --git a/arch/arm64/gunyah/gunyah_hypercall.c b/arch/arm64/gunyah/gunyah_hypercall.c > new file mode 100644 > index 000000000000..0d14e767e2c8 > --- /dev/null > +++ b/arch/arm64/gunyah/gunyah_hypercall.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/arm-smccc.h> > +#include <linux/module.h> > +#include <linux/gunyah.h> > +#include <linux/uuid.h> > + > +static const uuid_t gh_known_uuids[] = { > + /* Qualcomm's version of Gunyah {19bd54bd-0b37-571b-946f-609b54539de6} */ > + UUID_INIT(0x19bd54bd, 0x0b37, 0x571b, 0x94, 0x6f, 0x60, 0x9b, 0x54, 0x53, 0x9d, 0xe6), > + /* Standard version of Gunyah {c1d58fcd-a453-5fdb-9265-ce36673d5f14} */ > + UUID_INIT(0xc1d58fcd, 0xa453, 0x5fdb, 0x92, 0x65, 0xce, 0x36, 0x67, 0x3d, 0x5f, 0x14), > +}; > + > +bool arch_is_gh_guest(void) > +{ > + struct arm_smccc_res res; > + uuid_t uuid; > + int i; > + > + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); > + > + ((u32 *)&uuid.b[0])[0] = lower_32_bits(res.a0); > + ((u32 *)&uuid.b[0])[1] = lower_32_bits(res.a1); > + ((u32 *)&uuid.b[0])[2] = lower_32_bits(res.a2); > + ((u32 *)&uuid.b[0])[3] = lower_32_bits(res.a3); > + > + for (i = 0; i < ARRAY_SIZE(gh_known_uuids); i++) > + if (uuid_equal(&uuid, &gh_known_uuids[i])) > + return true; > + > + return false; > +} > +EXPORT_SYMBOL_GPL(arch_is_gh_guest); > + > +#define GH_HYPERCALL(fn) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64, \ > + ARM_SMCCC_OWNER_VENDOR_HYP, \ > + fn) > + > +#define GH_HYPERCALL_HYP_IDENTIFY GH_HYPERCALL(0x8000) > + > +/** > + * gh_hypercall_hyp_identify() - Returns build information and feature flags > + * supported by Gunyah. > + * @hyp_identity: filled by the hypercall with the API info and feature flags. > + */ > +void gh_hypercall_hyp_identify(struct gh_hypercall_hyp_identify_resp *hyp_identity) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_1_1_hvc(GH_HYPERCALL_HYP_IDENTIFY, &res); > + > + hyp_identity->api_info = res.a0; > + hyp_identity->flags[0] = res.a1; > + hyp_identity->flags[1] = res.a2; > + hyp_identity->flags[2] = res.a3; > +} > +EXPORT_SYMBOL_GPL(gh_hypercall_hyp_identify); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Gunyah Hypervisor Hypercalls"); > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig > index f79ab13a5c28..85bd6626ffc9 100644 > --- a/drivers/virt/Kconfig > +++ b/drivers/virt/Kconfig > @@ -54,4 +54,6 @@ source "drivers/virt/coco/sev-guest/Kconfig" > > source "drivers/virt/coco/tdx-guest/Kconfig" > > +source "drivers/virt/gunyah/Kconfig" > + > endif > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig > new file mode 100644 > index 000000000000..1a737694c333 > --- /dev/null > +++ b/drivers/virt/gunyah/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +config GUNYAH > + tristate "Gunyah Virtualization drivers" > + depends on ARM64 > + depends on MAILBOX > + help > + The Gunyah drivers are the helper interfaces that run in a guest VM > + such as basic inter-VM IPC and signaling mechanisms, and higher level > + services such as memory/device sharing, IRQ sharing, and so on. > + > + Say Y/M here to enable the drivers needed to interact in a Gunyah > + virtual environment. > diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h > index 54b4be71caf7..bd080e3a6fc9 100644 > --- a/include/linux/gunyah.h > +++ b/include/linux/gunyah.h > @@ -6,8 +6,10 @@ > #ifndef _LINUX_GUNYAH_H > #define _LINUX_GUNYAH_H > > +#include <linux/bitfield.h> > #include <linux/errno.h> > #include <linux/limits.h> > +#include <linux/types.h> > > /******************************************************************************/ > /* Common arch-independent definitions for Gunyah hypercalls */ > @@ -80,4 +82,30 @@ static inline int gh_remap_error(enum gh_error gh_error) > } > } > > +enum gh_api_feature { > + GH_FEATURE_DOORBELL = 1, > + GH_FEATURE_MSGQUEUE = 2, > + GH_FEATURE_VCPU = 5, > + GH_FEATURE_MEMEXTENT = 6, > +}; > + > +bool arch_is_gh_guest(void); > + > +u16 gh_api_version(void); > +bool gh_api_has_feature(enum gh_api_feature feature); > + > +#define GH_API_V1 1 > + > +#define GH_API_INFO_API_VERSION_MASK GENMASK_ULL(13, 0) > +#define GH_API_INFO_BIG_ENDIAN BIT_ULL(14) > +#define GH_API_INFO_IS_64BIT BIT_ULL(15) > +#define GH_API_INFO_VARIANT_MASK GENMASK_ULL(63, 56) > + > +struct gh_hypercall_hyp_identify_resp { > + u64 api_info; > + u64 flags[3]; > +}; > + > +void gh_hypercall_hyp_identify(struct gh_hypercall_hyp_identify_resp *hyp_identity); > + > #endif
On 04/03/2023 01:06, Elliot Berman wrote: > Gunyah VM manager is a kernel moduel which exposes an interface to > Gunyah userspace to load, run, and interact with other Gunyah virtual > machines. The interface is a character device at /dev/gunyah. > > Add a basic VM manager driver. Upcoming patches will add more ioctls > into this driver. > > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > .../userspace-api/ioctl/ioctl-number.rst | 1 + > drivers/virt/gunyah/Makefile | 2 +- > drivers/virt/gunyah/rsc_mgr.c | 38 +++++- > drivers/virt/gunyah/vm_mgr.c | 116 ++++++++++++++++++ > drivers/virt/gunyah/vm_mgr.h | 23 ++++ > include/uapi/linux/gunyah.h | 23 ++++ > 6 files changed, 201 insertions(+), 2 deletions(-) > create mode 100644 drivers/virt/gunyah/vm_mgr.c > create mode 100644 drivers/virt/gunyah/vm_mgr.h > create mode 100644 include/uapi/linux/gunyah.h > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst > index 0a1882e296ae..2513324ae7be 100644 > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > @@ -137,6 +137,7 @@ Code Seq# Include File Comments > 'F' DD video/sstfb.h conflict! > 'G' 00-3F drivers/misc/sgi-gru/grulib.h conflict! > 'G' 00-0F xen/gntalloc.h, xen/gntdev.h conflict! > +'G' 00-0f linux/gunyah.h conflict! > 'H' 00-7F linux/hiddev.h conflict! > 'H' 00-0F linux/hidraw.h conflict! > 'H' 01 linux/mei.h conflict! > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile > index de29769f2f3f..03951cf82023 100644 > --- a/drivers/virt/gunyah/Makefile > +++ b/drivers/virt/gunyah/Makefile > @@ -2,5 +2,5 @@ > > obj-$(CONFIG_GUNYAH) += gunyah.o > > -gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o > +gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o > obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o > diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c > index 67813c9a52db..d7ce692d0067 100644 > --- a/drivers/virt/gunyah/rsc_mgr.c > +++ b/drivers/virt/gunyah/rsc_mgr.c > @@ -15,8 +15,10 @@ > #include <linux/completion.h> > #include <linux/gunyah_rsc_mgr.h> > #include <linux/platform_device.h> > +#include <linux/miscdevice.h> > > #include "rsc_mgr.h" > +#include "vm_mgr.h" > > #define RM_RPC_API_VERSION_MASK GENMASK(3, 0) > #define RM_RPC_HEADER_WORDS_MASK GENMASK(7, 4) > @@ -129,6 +131,7 @@ struct gh_rm_connection { > * @cache: cache for allocating Tx messages > * @send_lock: synchronization to allow only one request to be sent at a time > * @nh: notifier chain for clients interested in RM notification messages > + * @miscdev: /dev/gunyah > */ > struct gh_rm { > struct device *dev; > @@ -145,6 +148,8 @@ struct gh_rm { > struct kmem_cache *cache; > struct mutex send_lock; > struct blocking_notifier_head nh; > + > + struct miscdevice miscdev; > }; > > /** > @@ -593,6 +598,21 @@ void gh_rm_put(struct gh_rm *rm) > } > EXPORT_SYMBOL_GPL(gh_rm_put); > > +static long gh_dev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + struct miscdevice *miscdev = filp->private_data; > + struct gh_rm *rm = container_of(miscdev, struct gh_rm, miscdev); > + > + return gh_dev_vm_mgr_ioctl(rm, cmd, arg); > +} > + > +static const struct file_operations gh_dev_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = gh_dev_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > + .llseek = noop_llseek, > +}; > + > static int gh_msgq_platform_probe_direction(struct platform_device *pdev, bool tx, > struct gh_resource *ghrsc) > { > @@ -651,7 +671,22 @@ static int gh_rm_drv_probe(struct platform_device *pdev) > rm->msgq_client.rx_callback = gh_rm_msgq_rx_data; > rm->msgq_client.tx_done = gh_rm_msgq_tx_done; > > - return gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, &rm->tx_ghrsc, &rm->rx_ghrsc); > + ret = gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, &rm->tx_ghrsc, &rm->rx_ghrsc); > + if (ret) > + goto err_cache; > + > + rm->miscdev.name = "gunyah"; > + rm->miscdev.minor = MISC_DYNAMIC_MINOR; > + rm->miscdev.fops = &gh_dev_fops; > + > + ret = misc_register(&rm->miscdev); > + if (ret) > + goto err_msgq; > + > + return 0; > +err_msgq: > + mbox_free_channel(gh_msgq_chan(&rm->msgq)); > + gh_msgq_remove(&rm->msgq); > err_cache: > kmem_cache_destroy(rm->cache); > return ret; > @@ -661,6 +696,7 @@ static int gh_rm_drv_remove(struct platform_device *pdev) > { > struct gh_rm *rm = platform_get_drvdata(pdev); > > + misc_deregister(&rm->miscdev); > mbox_free_channel(gh_msgq_chan(&rm->msgq)); > gh_msgq_remove(&rm->msgq); > kmem_cache_destroy(rm->cache); > diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c > new file mode 100644 > index 000000000000..dbacf36af72d > --- /dev/null > +++ b/drivers/virt/gunyah/vm_mgr.c > @@ -0,0 +1,116 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) "gh_vm_mgr: " fmt > + > +#include <linux/anon_inodes.h> > +#include <linux/file.h> > +#include <linux/gunyah_rsc_mgr.h> > +#include <linux/miscdevice.h> > +#include <linux/module.h> > + > +#include <uapi/linux/gunyah.h> > + > +#include "vm_mgr.h" > + > +static void gh_vm_free(struct work_struct *work) > +{ > + struct gh_vm *ghvm = container_of(work, struct gh_vm, free_work); > + int ret; > + > + ret = gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid); > + if (ret) > + pr_warn("Failed to deallocate vmid: %d\n", ret); > + > + put_gh_rm(ghvm->rm); > + kfree(ghvm); > +} > + > +static __must_check struct gh_vm *gh_vm_alloc(struct gh_rm *rm) > +{ > + struct gh_vm *ghvm; > + int vmid; > + > + vmid = gh_rm_alloc_vmid(rm, 0); > + if (vmid < 0) > + return ERR_PTR(vmid); > + > + ghvm = kzalloc(sizeof(*ghvm), GFP_KERNEL); > + if (!ghvm) { > + gh_rm_dealloc_vmid(rm, vmid); > + return ERR_PTR(-ENOMEM); > + } > + > + ghvm->parent = gh_rm_get(rm); > + ghvm->vmid = vmid; > + ghvm->rm = rm; > + > + INIT_WORK(&ghvm->free_work, gh_vm_free); > + > + return ghvm; > +} > + > +static int gh_vm_release(struct inode *inode, struct file *filp) > +{ > + struct gh_vm *ghvm = filp->private_data; > + > + /* VM will be reset and make RM calls which can interruptible sleep. > + * Defer to a work so this thread can receive signal. > + */ > + schedule_work(&ghvm->free_work); > + return 0; > +} > + > +static const struct file_operations gh_vm_fops = { > + .release = gh_vm_release, > + .llseek = noop_llseek, > +}; > + > +static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg) > +{ > + struct gh_vm *ghvm; > + struct file *file; > + int fd, err; > + > + /* arg reserved for future use. */ > + if (arg) > + return -EINVAL; > + > + ghvm = gh_vm_alloc(rm); > + if (IS_ERR(ghvm)) > + return PTR_ERR(ghvm); > + > + fd = get_unused_fd_flags(O_CLOEXEC); > + if (fd < 0) { > + err = fd; > + goto err_destroy_vm; > + } > + > + file = anon_inode_getfile("gunyah-vm", &gh_vm_fops, ghvm, O_RDWR); > + if (IS_ERR(file)) { > + err = PTR_ERR(file); > + goto err_put_fd; > + } > + > + fd_install(fd, file); > + > + return fd; > + > +err_put_fd: > + put_unused_fd(fd); > +err_destroy_vm: > + gh_vm_free(&ghvm->free_work); > + return err; > +} > + > +long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd, unsigned long arg) > +{ > + switch (cmd) { > + case GH_CREATE_VM: > + return gh_dev_ioctl_create_vm(rm, arg); > + default: > + return -ENOIOCTLCMD; > + } > +} > diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h > new file mode 100644 > index 000000000000..4b22fbcac91c > --- /dev/null > +++ b/drivers/virt/gunyah/vm_mgr.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _GH_PRIV_VM_MGR_H > +#define _GH_PRIV_VM_MGR_H > + > +#include <linux/gunyah_rsc_mgr.h> > + > +#include <uapi/linux/gunyah.h> > + > +long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd, unsigned long arg); > + > +struct gh_vm { > + u16 vmid; > + struct gh_rm *rm; > + struct device *parent; > + > + struct work_struct free_work; > +}; > + > +#endif > diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h > new file mode 100644 > index 000000000000..10ba32d2b0a6 > --- /dev/null > +++ b/include/uapi/linux/gunyah.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _UAPI_LINUX_GUNYAH > +#define _UAPI_LINUX_GUNYAH > + > +/* > + * Userspace interface for /dev/gunyah - gunyah based virtual machine > + */ > + > +#include <linux/types.h> > +#include <linux/ioctl.h> > + > +#define GH_IOCTL_TYPE 'G' > + > +/* > + * ioctls for /dev/gunyah fds: > + */ > +#define GH_CREATE_VM _IO(GH_IOCTL_TYPE, 0x0) /* Returns a Gunyah VM fd */ > + > +#endif
Hi Elliot, On 04/03/2023 01:06, Elliot Berman wrote: > Qualcomm platforms have a firmware entity which performs access control > to physical pages. Dynamically started Gunyah virtual machines use the > QCOM_SCM_RM_MANAGED_VMID for access. Linux thus needs to assign access > to the memory used by guest VMs. Gunyah doesn't do this operation for us > since it is the current VM (typically VMID_HLOS) delegating the access > and not Gunyah itself. Use the Gunyah platform ops to achieve this so > that only Qualcomm platforms attempt to make the needed SCM calls. > > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/firmware/Kconfig | 2 + > drivers/firmware/qcom_scm.c | 100 +++++++++++++++++++++++++++++++++ > include/linux/gunyah_rsc_mgr.h | 2 +- > 3 files changed, 103 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index b59e3041fd62..b888068ff6f2 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -214,6 +214,8 @@ config MTK_ADSP_IPC > > config QCOM_SCM > tristate > + select VIRT_DRIVERS > + select GUNYAH_PLATFORM_HOOKS > I still have concerns with this selects in Kconfig on older Qualcomm platforms that use SCM and do not have GUNYAH. In our last discussing you mentioned the requirement for "CONFIG_GUNYAH=y and CONFIG_QCOM_SCM=m" I think that should be doable and remove selecting if you can make a separate GUNYAH_QCOM_PLATFORM_HOOKS driver Does this work? >----------------------->cut<------------------------------- From 1fb7995aecf17caefd09ffb516579bc4ac9ac301 Mon Sep 17 00:00:00 2001 From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Date: Tue, 21 Mar 2023 13:34:02 +0000 Subject: [PATCH] virt: gunyah: add qcom platform hooks Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/firmware/Kconfig | 2 -- drivers/firmware/qcom_scm.c | 14 +++----- drivers/virt/gunyah/Kconfig | 5 +++ drivers/virt/gunyah/Makefile | 1 + .../virt/gunyah/gunyah_qcom_platform_hooks.c | 35 +++++++++++++++++++ include/linux/firmware/qcom/qcom_scm.h | 3 ++ 6 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 drivers/virt/gunyah/gunyah_qcom_platform_hooks.c diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index b888068ff6f2..b59e3041fd62 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -214,8 +214,6 @@ config MTK_ADSP_IPC config QCOM_SCM tristate - select VIRT_DRIVERS - select GUNYAH_PLATFORM_HOOKS config QCOM_SCM_DOWNLOAD_MODE_DEFAULT bool "Qualcomm download mode enabled by default" diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index 5273cf64ee2a..194ea2bc9a1d 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -1301,7 +1301,7 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, } EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); -static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel) +int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel *mem_parcel) { struct qcom_scm_vmperm *new_perms; u64 src, src_cpy; @@ -1359,8 +1359,9 @@ static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct gh_rm_mem_parce kfree(new_perms); return ret; } +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_pre_mem_share); -static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel) +int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel *mem_parcel) { struct qcom_scm_vmperm new_perms; u64 src = 0, src_cpy; @@ -1388,11 +1389,7 @@ static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_pa return ret; } - -static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = { - .pre_mem_share = qcom_scm_gh_rm_pre_mem_share, - .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim, -}; +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_post_mem_reclaim); static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) { @@ -1597,9 +1594,6 @@ static int qcom_scm_probe(struct platform_device *pdev) if (download_mode) qcom_scm_set_download_mode(true); - if (devm_gh_rm_register_platform_ops(&pdev->dev, &qcom_scm_gh_rm_platform_ops)) - dev_warn(__scm->dev, "Gunyah RM platform ops were already registered\n"); - return 0; } diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig index bd8e31184962..a9c48d6518f7 100644 --- a/drivers/virt/gunyah/Kconfig +++ b/drivers/virt/gunyah/Kconfig @@ -16,6 +16,11 @@ config GUNYAH config GUNYAH_PLATFORM_HOOKS tristate +config GUNYAH_QCOM_PLATFORM_HOOKS + tristate "Gunyah Platform hooks for Qualcomm" + depends on ARCH_QCOM && QCOM_SCM + depends on GUNYAH + config GUNYAH_VCPU tristate "Runnable Gunyah vCPUs" depends on GUNYAH diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile index 7347b1470491..c33f701bb5c8 100644 --- a/drivers/virt/gunyah/Makefile +++ b/drivers/virt/gunyah/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_GUNYAH) += gunyah.o obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o +obj-$(CONFIG_GUNYAH_QCOM_PLATFORM_HOOKS) += gunyah_qcom_platform_hooks.o gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o diff --git a/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c new file mode 100644 index 000000000000..3332f84134d3 --- /dev/null +++ b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c @@ -0,0 +1,35 @@ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/firmware/qcom/qcom_scm.h> +#include <linux/gunyah_rsc_mgr.h> + +static int qcom_gh_rm_pre_mem_share(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel) +{ + return qcom_scm_gh_rm_pre_mem_share(mem_parcel); +} + +static int qcom_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel) +{ + return qcom_scm_gh_rm_post_mem_reclaim(mem_parcel); +} + +static struct gh_rm_platform_ops qcom_gh_platform_hooks_ops = { + .pre_mem_share = qcom_gh_rm_pre_mem_share, + .post_mem_reclaim = qcom_gh_rm_post_mem_reclaim, +}; + +static int __init qcom_gh_platform_hooks_register(void) +{ + return gh_rm_register_platform_ops(&qcom_gh_platform_hooks_ops); +} + +static void __exit qcom_gh_platform_hooks_unregister(void) +{ + gh_rm_unregister_platform_ops(&qcom_gh_platform_hooks_ops); +} + +module_init(qcom_gh_platform_hooks_register); +module_exit(qcom_gh_platform_hooks_unregister); + +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Gunyah Platform Hooks driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h index 1e449a5d7f5c..9b0d33db803d 100644 --- a/include/linux/firmware/qcom/qcom_scm.h +++ b/include/linux/firmware/qcom/qcom_scm.h @@ -121,5 +121,8 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, u64 limit_node, u32 node_id, u64 version); extern int qcom_scm_lmh_profile_change(u32 profile_id); extern bool qcom_scm_lmh_dcvsh_available(void); +struct gh_rm_mem_parcel; +extern int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel *mem_parcel); +extern int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel *mem_parcel); #endif --------------------------->cut<----------------------- > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > bool "Qualcomm download mode enabled by default" > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index b95616b35bff..89a261a9e021 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -20,6 +20,7 @@ > #include <linux/clk.h> > #include <linux/reset-controller.h> > #include <linux/arm-smccc.h> > +#include <linux/gunyah_rsc_mgr.h> > > #include "qcom_scm.h" > > @@ -30,6 +31,9 @@ module_param(download_mode, bool, 0); > #define SCM_HAS_IFACE_CLK BIT(1) > #define SCM_HAS_BUS_CLK BIT(2) > > +#define QCOM_SCM_RM_MANAGED_VMID 0x3A > +#define QCOM_SCM_MAX_MANAGED_VMID 0x3F > + > struct qcom_scm { > struct device *dev; > struct clk *core_clk; > @@ -1299,6 +1303,99 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > } > EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); > > +static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel) > +{ > + struct qcom_scm_vmperm *new_perms; > + u64 src, src_cpy; > + int ret = 0, i, n; > + u16 vmid; > + > + new_perms = kcalloc(mem_parcel->n_acl_entries, sizeof(*new_perms), GFP_KERNEL); > + if (!new_perms) > + return -ENOMEM; > + > + for (n = 0; n < mem_parcel->n_acl_entries; n++) { > + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); > + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) > + new_perms[n].vmid = vmid; > + else > + new_perms[n].vmid = QCOM_SCM_RM_MANAGED_VMID; > + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_X) > + new_perms[n].perm |= QCOM_SCM_PERM_EXEC; > + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_W) > + new_perms[n].perm |= QCOM_SCM_PERM_WRITE; > + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_R) > + new_perms[n].perm |= QCOM_SCM_PERM_READ; > + } > + > + src = (1ull << QCOM_SCM_VMID_HLOS); > + > + for (i = 0; i < mem_parcel->n_mem_entries; i++) { > + src_cpy = src; > + ret = qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), > + le64_to_cpu(mem_parcel->mem_entries[i].size), > + &src_cpy, new_perms, mem_parcel->n_acl_entries); > + if (ret) { > + src = 0; > + for (n = 0; n < mem_parcel->n_acl_entries; n++) { > + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); > + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) > + src |= (1ull << vmid); > + else > + src |= (1ull << QCOM_SCM_RM_MANAGED_VMID); > + } > + > + new_perms[0].vmid = QCOM_SCM_VMID_HLOS; > + > + for (i--; i >= 0; i--) { > + src_cpy = src; > + WARN_ON_ONCE(qcom_scm_assign_mem( > + le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), > + le64_to_cpu(mem_parcel->mem_entries[i].size), > + &src_cpy, new_perms, 1)); > + } > + break; > + } > + } > + > + kfree(new_perms); > + return ret; > +} > + > +static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel) > +{ > + struct qcom_scm_vmperm new_perms; > + u64 src = 0, src_cpy; > + int ret = 0, i, n; > + u16 vmid; > + > + new_perms.vmid = QCOM_SCM_VMID_HLOS; > + new_perms.perm = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ; > + > + for (n = 0; n < mem_parcel->n_acl_entries; n++) { > + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); > + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) > + src |= (1ull << vmid); > + else > + src |= (1ull << QCOM_SCM_RM_MANAGED_VMID); > + } > + > + for (i = 0; i < mem_parcel->n_mem_entries; i++) { > + src_cpy = src; > + ret = qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), > + le64_to_cpu(mem_parcel->mem_entries[i].size), > + &src_cpy, &new_perms, 1); > + WARN_ON_ONCE(ret); > + } > + > + return ret; > +} > + > +static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = { > + .pre_mem_share = qcom_scm_gh_rm_pre_mem_share, > + .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim, > +}; > + > static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > { > struct device_node *tcsr; > @@ -1502,6 +1599,9 @@ static int qcom_scm_probe(struct platform_device *pdev) > if (download_mode) > qcom_scm_set_download_mode(true); > > + if (devm_gh_rm_register_platform_ops(&pdev->dev, &qcom_scm_gh_rm_platform_ops)) > + dev_warn(__scm->dev, "Gunyah RM platform ops were already registered\n"); > + > return 0; > } > > diff --git a/include/linux/gunyah_rsc_mgr.h b/include/linux/gunyah_rsc_mgr.h > index 515087931a2b..acf8c1545a6c 100644 > --- a/include/linux/gunyah_rsc_mgr.h > +++ b/include/linux/gunyah_rsc_mgr.h > @@ -145,7 +145,7 @@ int gh_rm_get_hyp_resources(struct gh_rm *rm, u16 vmid, > struct gh_rm_hyp_resources **resources); > int gh_rm_get_vmid(struct gh_rm *rm, u16 *vmid); > > -struct gunyah_rm_platform_ops { > +struct gh_rm_platform_ops { > int (*pre_mem_share)(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel); > int (*post_mem_reclaim)(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel); > };
On 3/21/2023 7:24 AM, Srinivas Kandagatla wrote: > Hi Elliot, > > On 04/03/2023 01:06, Elliot Berman wrote: >> Qualcomm platforms have a firmware entity which performs access control >> to physical pages. Dynamically started Gunyah virtual machines use the >> QCOM_SCM_RM_MANAGED_VMID for access. Linux thus needs to assign access >> to the memory used by guest VMs. Gunyah doesn't do this operation for us >> since it is the current VM (typically VMID_HLOS) delegating the access >> and not Gunyah itself. Use the Gunyah platform ops to achieve this so >> that only Qualcomm platforms attempt to make the needed SCM calls. >> >> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> >> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> drivers/firmware/Kconfig | 2 + >> drivers/firmware/qcom_scm.c | 100 +++++++++++++++++++++++++++++++++ >> include/linux/gunyah_rsc_mgr.h | 2 +- >> 3 files changed, 103 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >> index b59e3041fd62..b888068ff6f2 100644 >> --- a/drivers/firmware/Kconfig >> +++ b/drivers/firmware/Kconfig >> @@ -214,6 +214,8 @@ config MTK_ADSP_IPC >> config QCOM_SCM >> tristate >> + select VIRT_DRIVERS >> + select GUNYAH_PLATFORM_HOOKS >> > > I still have concerns with this selects in Kconfig on older Qualcomm > platforms that use SCM and do not have GUNYAH. > > In our last discussing you mentioned the requirement for > "CONFIG_GUNYAH=y and CONFIG_QCOM_SCM=m" > > I think that should be doable and remove selecting if you can make a > separate GUNYAH_QCOM_PLATFORM_HOOKS driver > > Does this work? This works for Android and all the Qualcomm vendor (downstream) platforms where we can explicitly load modules. I don't think this module would be implicitly loaded by any kernel mechanism. > >----------------------->cut<------------------------------- > From 1fb7995aecf17caefd09ffb516579bc4ac9ac301 Mon Sep 17 00:00:00 2001 > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Date: Tue, 21 Mar 2023 13:34:02 +0000 > Subject: [PATCH] virt: gunyah: add qcom platform hooks > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/firmware/Kconfig | 2 -- > drivers/firmware/qcom_scm.c | 14 +++----- > drivers/virt/gunyah/Kconfig | 5 +++ > drivers/virt/gunyah/Makefile | 1 + > .../virt/gunyah/gunyah_qcom_platform_hooks.c | 35 +++++++++++++++++++ > include/linux/firmware/qcom/qcom_scm.h | 3 ++ > 6 files changed, 48 insertions(+), 12 deletions(-) > create mode 100644 drivers/virt/gunyah/gunyah_qcom_platform_hooks.c > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index b888068ff6f2..b59e3041fd62 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -214,8 +214,6 @@ config MTK_ADSP_IPC > > config QCOM_SCM > tristate > - select VIRT_DRIVERS > - select GUNYAH_PLATFORM_HOOKS > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > bool "Qualcomm download mode enabled by default" > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 5273cf64ee2a..194ea2bc9a1d 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -1301,7 +1301,7 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 > payload_reg, u32 payload_val, > } > EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); > > -static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct > gh_rm_mem_parcel *mem_parcel) > +int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel *mem_parcel) > { > struct qcom_scm_vmperm *new_perms; > u64 src, src_cpy; > @@ -1359,8 +1359,9 @@ static int qcom_scm_gh_rm_pre_mem_share(struct > gh_rm *rm, struct gh_rm_mem_parce > kfree(new_perms); > return ret; > } > +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_pre_mem_share); > > -static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct > gh_rm_mem_parcel *mem_parcel) > +int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel *mem_parcel) > { > struct qcom_scm_vmperm new_perms; > u64 src = 0, src_cpy; > @@ -1388,11 +1389,7 @@ static int qcom_scm_gh_rm_post_mem_reclaim(struct > gh_rm *rm, struct gh_rm_mem_pa > > return ret; > } > - > -static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = { > - .pre_mem_share = qcom_scm_gh_rm_pre_mem_share, > - .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim, > -}; > +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_post_mem_reclaim); > > static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > { > @@ -1597,9 +1594,6 @@ static int qcom_scm_probe(struct platform_device > *pdev) > if (download_mode) > qcom_scm_set_download_mode(true); > > - if (devm_gh_rm_register_platform_ops(&pdev->dev, > &qcom_scm_gh_rm_platform_ops)) > - dev_warn(__scm->dev, "Gunyah RM platform ops were already > registered\n"); > - > return 0; > } > > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig > index bd8e31184962..a9c48d6518f7 100644 > --- a/drivers/virt/gunyah/Kconfig > +++ b/drivers/virt/gunyah/Kconfig > @@ -16,6 +16,11 @@ config GUNYAH > config GUNYAH_PLATFORM_HOOKS > tristate > > +config GUNYAH_QCOM_PLATFORM_HOOKS > + tristate "Gunyah Platform hooks for Qualcomm" > + depends on ARCH_QCOM && QCOM_SCM > + depends on GUNYAH > + > config GUNYAH_VCPU > tristate "Runnable Gunyah vCPUs" > depends on GUNYAH > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile > index 7347b1470491..c33f701bb5c8 100644 > --- a/drivers/virt/gunyah/Makefile > +++ b/drivers/virt/gunyah/Makefile > @@ -2,6 +2,7 @@ > > obj-$(CONFIG_GUNYAH) += gunyah.o > obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o > +obj-$(CONFIG_GUNYAH_QCOM_PLATFORM_HOOKS) += gunyah_qcom_platform_hooks.o > > gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o > obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o > diff --git a/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c > b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c > new file mode 100644 > index 000000000000..3332f84134d3 > --- /dev/null > +++ b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c > @@ -0,0 +1,35 @@ > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/firmware/qcom/qcom_scm.h> > +#include <linux/gunyah_rsc_mgr.h> > + > +static int qcom_gh_rm_pre_mem_share(struct gh_rm *rm, struct > gh_rm_mem_parcel *mem_parcel) > +{ > + return qcom_scm_gh_rm_pre_mem_share(mem_parcel); > +} > + > +static int qcom_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct > gh_rm_mem_parcel *mem_parcel) > +{ > + return qcom_scm_gh_rm_post_mem_reclaim(mem_parcel); > +} > + > +static struct gh_rm_platform_ops qcom_gh_platform_hooks_ops = { > + .pre_mem_share = qcom_gh_rm_pre_mem_share, > + .post_mem_reclaim = qcom_gh_rm_post_mem_reclaim, > +}; > + > +static int __init qcom_gh_platform_hooks_register(void) > +{ > + return gh_rm_register_platform_ops(&qcom_gh_platform_hooks_ops); > +} > + > +static void __exit qcom_gh_platform_hooks_unregister(void) > +{ > + gh_rm_unregister_platform_ops(&qcom_gh_platform_hooks_ops); > +} > + > +module_init(qcom_gh_platform_hooks_register); > +module_exit(qcom_gh_platform_hooks_unregister); > + > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Gunyah Platform Hooks > driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/firmware/qcom/qcom_scm.h > b/include/linux/firmware/qcom/qcom_scm.h > index 1e449a5d7f5c..9b0d33db803d 100644 > --- a/include/linux/firmware/qcom/qcom_scm.h > +++ b/include/linux/firmware/qcom/qcom_scm.h > @@ -121,5 +121,8 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 > payload_reg, u32 payload_val, > u64 limit_node, u32 node_id, u64 version); > extern int qcom_scm_lmh_profile_change(u32 profile_id); > extern bool qcom_scm_lmh_dcvsh_available(void); > +struct gh_rm_mem_parcel; > +extern int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel > *mem_parcel); > +extern int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel > *mem_parcel); > > #endif > --------------------------->cut<----------------------- > >> config QCOM_SCM_DOWNLOAD_MODE_DEFAULT >> bool "Qualcomm download mode enabled by default" >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index b95616b35bff..89a261a9e021 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -20,6 +20,7 @@ >> #include <linux/clk.h> >> #include <linux/reset-controller.h> >> #include <linux/arm-smccc.h> >> +#include <linux/gunyah_rsc_mgr.h> >> #include "qcom_scm.h" >> @@ -30,6 +31,9 @@ module_param(download_mode, bool, 0); >> #define SCM_HAS_IFACE_CLK BIT(1) >> #define SCM_HAS_BUS_CLK BIT(2) >> +#define QCOM_SCM_RM_MANAGED_VMID 0x3A >> +#define QCOM_SCM_MAX_MANAGED_VMID 0x3F >> + >> struct qcom_scm { >> struct device *dev; >> struct clk *core_clk; >> @@ -1299,6 +1303,99 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 >> payload_reg, u32 payload_val, >> } >> EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); >> +static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel) >> +{ >> + struct qcom_scm_vmperm *new_perms; >> + u64 src, src_cpy; >> + int ret = 0, i, n; >> + u16 vmid; >> + >> + new_perms = kcalloc(mem_parcel->n_acl_entries, >> sizeof(*new_perms), GFP_KERNEL); >> + if (!new_perms) >> + return -ENOMEM; >> + >> + for (n = 0; n < mem_parcel->n_acl_entries; n++) { >> + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); >> + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) >> + new_perms[n].vmid = vmid; >> + else >> + new_perms[n].vmid = QCOM_SCM_RM_MANAGED_VMID; >> + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_X) >> + new_perms[n].perm |= QCOM_SCM_PERM_EXEC; >> + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_W) >> + new_perms[n].perm |= QCOM_SCM_PERM_WRITE; >> + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_R) >> + new_perms[n].perm |= QCOM_SCM_PERM_READ; >> + } >> + >> + src = (1ull << QCOM_SCM_VMID_HLOS); >> + >> + for (i = 0; i < mem_parcel->n_mem_entries; i++) { >> + src_cpy = src; >> + ret = >> qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), >> + le64_to_cpu(mem_parcel->mem_entries[i].size), >> + &src_cpy, new_perms, mem_parcel->n_acl_entries); >> + if (ret) { >> + src = 0; >> + for (n = 0; n < mem_parcel->n_acl_entries; n++) { >> + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); >> + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) >> + src |= (1ull << vmid); >> + else >> + src |= (1ull << QCOM_SCM_RM_MANAGED_VMID); >> + } >> + >> + new_perms[0].vmid = QCOM_SCM_VMID_HLOS; >> + >> + for (i--; i >= 0; i--) { >> + src_cpy = src; >> + WARN_ON_ONCE(qcom_scm_assign_mem( >> + >> le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), >> + le64_to_cpu(mem_parcel->mem_entries[i].size), >> + &src_cpy, new_perms, 1)); >> + } >> + break; >> + } >> + } >> + >> + kfree(new_perms); >> + return ret; >> +} >> + >> +static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel) >> +{ >> + struct qcom_scm_vmperm new_perms; >> + u64 src = 0, src_cpy; >> + int ret = 0, i, n; >> + u16 vmid; >> + >> + new_perms.vmid = QCOM_SCM_VMID_HLOS; >> + new_perms.perm = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | >> QCOM_SCM_PERM_READ; >> + >> + for (n = 0; n < mem_parcel->n_acl_entries; n++) { >> + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); >> + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) >> + src |= (1ull << vmid); >> + else >> + src |= (1ull << QCOM_SCM_RM_MANAGED_VMID); >> + } >> + >> + for (i = 0; i < mem_parcel->n_mem_entries; i++) { >> + src_cpy = src; >> + ret = >> qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), >> + le64_to_cpu(mem_parcel->mem_entries[i].size), >> + &src_cpy, &new_perms, 1); >> + WARN_ON_ONCE(ret); >> + } >> + >> + return ret; >> +} >> + >> +static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = { >> + .pre_mem_share = qcom_scm_gh_rm_pre_mem_share, >> + .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim, >> +}; >> + >> static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) >> { >> struct device_node *tcsr; >> @@ -1502,6 +1599,9 @@ static int qcom_scm_probe(struct platform_device >> *pdev) >> if (download_mode) >> qcom_scm_set_download_mode(true); >> + if (devm_gh_rm_register_platform_ops(&pdev->dev, >> &qcom_scm_gh_rm_platform_ops)) >> + dev_warn(__scm->dev, "Gunyah RM platform ops were already >> registered\n"); >> + >> return 0; >> } >> diff --git a/include/linux/gunyah_rsc_mgr.h >> b/include/linux/gunyah_rsc_mgr.h >> index 515087931a2b..acf8c1545a6c 100644 >> --- a/include/linux/gunyah_rsc_mgr.h >> +++ b/include/linux/gunyah_rsc_mgr.h >> @@ -145,7 +145,7 @@ int gh_rm_get_hyp_resources(struct gh_rm *rm, u16 >> vmid, >> struct gh_rm_hyp_resources **resources); >> int gh_rm_get_vmid(struct gh_rm *rm, u16 *vmid); >> -struct gunyah_rm_platform_ops { >> +struct gh_rm_platform_ops { >> int (*pre_mem_share)(struct gh_rm *rm, struct gh_rm_mem_parcel >> *mem_parcel); >> int (*post_mem_reclaim)(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel); >> };
On 21/03/2023 18:40, Elliot Berman wrote: > > > On 3/21/2023 7:24 AM, Srinivas Kandagatla wrote: >> Hi Elliot, >> >> On 04/03/2023 01:06, Elliot Berman wrote: >>> Qualcomm platforms have a firmware entity which performs access control >>> to physical pages. Dynamically started Gunyah virtual machines use the >>> QCOM_SCM_RM_MANAGED_VMID for access. Linux thus needs to assign access >>> to the memory used by guest VMs. Gunyah doesn't do this operation for us >>> since it is the current VM (typically VMID_HLOS) delegating the access >>> and not Gunyah itself. Use the Gunyah platform ops to achieve this so >>> that only Qualcomm platforms attempt to make the needed SCM calls. >>> >>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> >>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> >>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >>> --- >>> drivers/firmware/Kconfig | 2 + >>> drivers/firmware/qcom_scm.c | 100 +++++++++++++++++++++++++++++++++ >>> include/linux/gunyah_rsc_mgr.h | 2 +- >>> 3 files changed, 103 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >>> index b59e3041fd62..b888068ff6f2 100644 >>> --- a/drivers/firmware/Kconfig >>> +++ b/drivers/firmware/Kconfig >>> @@ -214,6 +214,8 @@ config MTK_ADSP_IPC >>> config QCOM_SCM >>> tristate >>> + select VIRT_DRIVERS >>> + select GUNYAH_PLATFORM_HOOKS >>> >> >> I still have concerns with this selects in Kconfig on older Qualcomm >> platforms that use SCM and do not have GUNYAH. >> >> In our last discussing you mentioned the requirement for >> "CONFIG_GUNYAH=y and CONFIG_QCOM_SCM=m" >> >> I think that should be doable and remove selecting if you can make a >> separate GUNYAH_QCOM_PLATFORM_HOOKS driver >> >> Does this work? > > This works for Android and all the Qualcomm vendor (downstream) > platforms where we can explicitly load modules. I don't think this > module would be implicitly loaded by any kernel mechanism. We could also load this module based on UUID match at the gunyah core level too, if that helps. --srini > >> >----------------------->cut<------------------------------- >> From 1fb7995aecf17caefd09ffb516579bc4ac9ac301 Mon Sep 17 00:00:00 2001 >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> Date: Tue, 21 Mar 2023 13:34:02 +0000 >> Subject: [PATCH] virt: gunyah: add qcom platform hooks >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/firmware/Kconfig | 2 -- >> drivers/firmware/qcom_scm.c | 14 +++----- >> drivers/virt/gunyah/Kconfig | 5 +++ >> drivers/virt/gunyah/Makefile | 1 + >> .../virt/gunyah/gunyah_qcom_platform_hooks.c | 35 +++++++++++++++++++ >> include/linux/firmware/qcom/qcom_scm.h | 3 ++ >> 6 files changed, 48 insertions(+), 12 deletions(-) >> create mode 100644 drivers/virt/gunyah/gunyah_qcom_platform_hooks.c >> >> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >> index b888068ff6f2..b59e3041fd62 100644 >> --- a/drivers/firmware/Kconfig >> +++ b/drivers/firmware/Kconfig >> @@ -214,8 +214,6 @@ config MTK_ADSP_IPC >> >> config QCOM_SCM >> tristate >> - select VIRT_DRIVERS >> - select GUNYAH_PLATFORM_HOOKS >> >> config QCOM_SCM_DOWNLOAD_MODE_DEFAULT >> bool "Qualcomm download mode enabled by default" >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 5273cf64ee2a..194ea2bc9a1d 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -1301,7 +1301,7 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 >> payload_reg, u32 payload_val, >> } >> EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); >> >> -static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel) >> +int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel *mem_parcel) >> { >> struct qcom_scm_vmperm *new_perms; >> u64 src, src_cpy; >> @@ -1359,8 +1359,9 @@ static int qcom_scm_gh_rm_pre_mem_share(struct >> gh_rm *rm, struct gh_rm_mem_parce >> kfree(new_perms); >> return ret; >> } >> +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_pre_mem_share); >> >> -static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel) >> +int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel *mem_parcel) >> { >> struct qcom_scm_vmperm new_perms; >> u64 src = 0, src_cpy; >> @@ -1388,11 +1389,7 @@ static int >> qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_pa >> >> return ret; >> } >> - >> -static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = { >> - .pre_mem_share = qcom_scm_gh_rm_pre_mem_share, >> - .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim, >> -}; >> +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_post_mem_reclaim); >> >> static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) >> { >> @@ -1597,9 +1594,6 @@ static int qcom_scm_probe(struct platform_device >> *pdev) >> if (download_mode) >> qcom_scm_set_download_mode(true); >> >> - if (devm_gh_rm_register_platform_ops(&pdev->dev, >> &qcom_scm_gh_rm_platform_ops)) >> - dev_warn(__scm->dev, "Gunyah RM platform ops were already >> registered\n"); >> - >> return 0; >> } >> >> diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig >> index bd8e31184962..a9c48d6518f7 100644 >> --- a/drivers/virt/gunyah/Kconfig >> +++ b/drivers/virt/gunyah/Kconfig >> @@ -16,6 +16,11 @@ config GUNYAH >> config GUNYAH_PLATFORM_HOOKS >> tristate >> >> +config GUNYAH_QCOM_PLATFORM_HOOKS >> + tristate "Gunyah Platform hooks for Qualcomm" >> + depends on ARCH_QCOM && QCOM_SCM >> + depends on GUNYAH >> + >> config GUNYAH_VCPU >> tristate "Runnable Gunyah vCPUs" >> depends on GUNYAH >> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile >> index 7347b1470491..c33f701bb5c8 100644 >> --- a/drivers/virt/gunyah/Makefile >> +++ b/drivers/virt/gunyah/Makefile >> @@ -2,6 +2,7 @@ >> >> obj-$(CONFIG_GUNYAH) += gunyah.o >> obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o >> +obj-$(CONFIG_GUNYAH_QCOM_PLATFORM_HOOKS) += gunyah_qcom_platform_hooks.o >> >> gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o >> obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o >> diff --git a/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c >> b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c >> new file mode 100644 >> index 000000000000..3332f84134d3 >> --- /dev/null >> +++ b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c >> @@ -0,0 +1,35 @@ >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/firmware/qcom/qcom_scm.h> >> +#include <linux/gunyah_rsc_mgr.h> >> + >> +static int qcom_gh_rm_pre_mem_share(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel) >> +{ >> + return qcom_scm_gh_rm_pre_mem_share(mem_parcel); >> +} >> + >> +static int qcom_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct >> gh_rm_mem_parcel *mem_parcel) >> +{ >> + return qcom_scm_gh_rm_post_mem_reclaim(mem_parcel); >> +} >> + >> +static struct gh_rm_platform_ops qcom_gh_platform_hooks_ops = { >> + .pre_mem_share = qcom_gh_rm_pre_mem_share, >> + .post_mem_reclaim = qcom_gh_rm_post_mem_reclaim, >> +}; >> + >> +static int __init qcom_gh_platform_hooks_register(void) >> +{ >> + return gh_rm_register_platform_ops(&qcom_gh_platform_hooks_ops); >> +} >> + >> +static void __exit qcom_gh_platform_hooks_unregister(void) >> +{ >> + gh_rm_unregister_platform_ops(&qcom_gh_platform_hooks_ops); >> +} >> + >> +module_init(qcom_gh_platform_hooks_register); >> +module_exit(qcom_gh_platform_hooks_unregister); >> + >> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Gunyah Platform Hooks >> driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/firmware/qcom/qcom_scm.h >> b/include/linux/firmware/qcom/qcom_scm.h >> index 1e449a5d7f5c..9b0d33db803d 100644 >> --- a/include/linux/firmware/qcom/qcom_scm.h >> +++ b/include/linux/firmware/qcom/qcom_scm.h >> @@ -121,5 +121,8 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 >> payload_reg, u32 payload_val, >> u64 limit_node, u32 node_id, u64 version); >> extern int qcom_scm_lmh_profile_change(u32 profile_id); >> extern bool qcom_scm_lmh_dcvsh_available(void); >> +struct gh_rm_mem_parcel; >> +extern int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel >> *mem_parcel); >> +extern int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel >> *mem_parcel); >> >> #endif >> --------------------------->cut<----------------------- >> >>> config QCOM_SCM_DOWNLOAD_MODE_DEFAULT >>> bool "Qualcomm download mode enabled by default" >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index b95616b35bff..89a261a9e021 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -20,6 +20,7 @@ >>> #include <linux/clk.h> >>> #include <linux/reset-controller.h> >>> #include <linux/arm-smccc.h> >>> +#include <linux/gunyah_rsc_mgr.h> >>> #include "qcom_scm.h" >>> @@ -30,6 +31,9 @@ module_param(download_mode, bool, 0); >>> #define SCM_HAS_IFACE_CLK BIT(1) >>> #define SCM_HAS_BUS_CLK BIT(2) >>> +#define QCOM_SCM_RM_MANAGED_VMID 0x3A >>> +#define QCOM_SCM_MAX_MANAGED_VMID 0x3F >>> + >>> struct qcom_scm { >>> struct device *dev; >>> struct clk *core_clk; >>> @@ -1299,6 +1303,99 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 >>> payload_reg, u32 payload_val, >>> } >>> EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); >>> +static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct >>> gh_rm_mem_parcel *mem_parcel) >>> +{ >>> + struct qcom_scm_vmperm *new_perms; >>> + u64 src, src_cpy; >>> + int ret = 0, i, n; >>> + u16 vmid; >>> + >>> + new_perms = kcalloc(mem_parcel->n_acl_entries, >>> sizeof(*new_perms), GFP_KERNEL); >>> + if (!new_perms) >>> + return -ENOMEM; >>> + >>> + for (n = 0; n < mem_parcel->n_acl_entries; n++) { >>> + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); >>> + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) >>> + new_perms[n].vmid = vmid; >>> + else >>> + new_perms[n].vmid = QCOM_SCM_RM_MANAGED_VMID; >>> + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_X) >>> + new_perms[n].perm |= QCOM_SCM_PERM_EXEC; >>> + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_W) >>> + new_perms[n].perm |= QCOM_SCM_PERM_WRITE; >>> + if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_R) >>> + new_perms[n].perm |= QCOM_SCM_PERM_READ; >>> + } >>> + >>> + src = (1ull << QCOM_SCM_VMID_HLOS); >>> + >>> + for (i = 0; i < mem_parcel->n_mem_entries; i++) { >>> + src_cpy = src; >>> + ret = >>> qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), >>> + le64_to_cpu(mem_parcel->mem_entries[i].size), >>> + &src_cpy, new_perms, >>> mem_parcel->n_acl_entries); >>> + if (ret) { >>> + src = 0; >>> + for (n = 0; n < mem_parcel->n_acl_entries; n++) { >>> + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); >>> + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) >>> + src |= (1ull << vmid); >>> + else >>> + src |= (1ull << QCOM_SCM_RM_MANAGED_VMID); >>> + } >>> + >>> + new_perms[0].vmid = QCOM_SCM_VMID_HLOS; >>> + >>> + for (i--; i >= 0; i--) { >>> + src_cpy = src; >>> + WARN_ON_ONCE(qcom_scm_assign_mem( >>> + le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), >>> + le64_to_cpu(mem_parcel->mem_entries[i].size), >>> + &src_cpy, new_perms, 1)); >>> + } >>> + break; >>> + } >>> + } >>> + >>> + kfree(new_perms); >>> + return ret; >>> +} >>> + >>> +static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct >>> gh_rm_mem_parcel *mem_parcel) >>> +{ >>> + struct qcom_scm_vmperm new_perms; >>> + u64 src = 0, src_cpy; >>> + int ret = 0, i, n; >>> + u16 vmid; >>> + >>> + new_perms.vmid = QCOM_SCM_VMID_HLOS; >>> + new_perms.perm = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | >>> QCOM_SCM_PERM_READ; >>> + >>> + for (n = 0; n < mem_parcel->n_acl_entries; n++) { >>> + vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid); >>> + if (vmid <= QCOM_SCM_MAX_MANAGED_VMID) >>> + src |= (1ull << vmid); >>> + else >>> + src |= (1ull << QCOM_SCM_RM_MANAGED_VMID); >>> + } >>> + >>> + for (i = 0; i < mem_parcel->n_mem_entries; i++) { >>> + src_cpy = src; >>> + ret = >>> qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base), >>> + le64_to_cpu(mem_parcel->mem_entries[i].size), >>> + &src_cpy, &new_perms, 1); >>> + WARN_ON_ONCE(ret); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = { >>> + .pre_mem_share = qcom_scm_gh_rm_pre_mem_share, >>> + .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim, >>> +}; >>> + >>> static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) >>> { >>> struct device_node *tcsr; >>> @@ -1502,6 +1599,9 @@ static int qcom_scm_probe(struct >>> platform_device *pdev) >>> if (download_mode) >>> qcom_scm_set_download_mode(true); >>> + if (devm_gh_rm_register_platform_ops(&pdev->dev, >>> &qcom_scm_gh_rm_platform_ops)) >>> + dev_warn(__scm->dev, "Gunyah RM platform ops were already >>> registered\n"); >>> + >>> return 0; >>> } >>> diff --git a/include/linux/gunyah_rsc_mgr.h >>> b/include/linux/gunyah_rsc_mgr.h >>> index 515087931a2b..acf8c1545a6c 100644 >>> --- a/include/linux/gunyah_rsc_mgr.h >>> +++ b/include/linux/gunyah_rsc_mgr.h >>> @@ -145,7 +145,7 @@ int gh_rm_get_hyp_resources(struct gh_rm *rm, u16 >>> vmid, >>> struct gh_rm_hyp_resources **resources); >>> int gh_rm_get_vmid(struct gh_rm *rm, u16 *vmid); >>> -struct gunyah_rm_platform_ops { >>> +struct gh_rm_platform_ops { >>> int (*pre_mem_share)(struct gh_rm *rm, struct gh_rm_mem_parcel >>> *mem_parcel); >>> int (*post_mem_reclaim)(struct gh_rm *rm, struct >>> gh_rm_mem_parcel *mem_parcel); >>> };
On 3/3/23 7:06 PM, Elliot Berman wrote: > Gunyah is a Type-1 hypervisor independent of any > high-level OS kernel, and runs in a higher CPU privilege level. It does > not depend on any lower-privileged OS kernel/code for its core > functionality. This increases its security and can support a much smaller > trusted computing base than a Type-2 hypervisor. > > Gunyah is an open source hypervisor. The source repo is available at > https://github.com/quic/gunyah-hypervisor. I've done a pretty detailed review again, and got further along than I did last time. Things are definitely looking better, but I have found some bugs that need to be addressed. I also make a lot of comments about grouping certain sets of definitions into enumerated types. Also I tend to notice when things aren't done consistently, and I mention that a lot. There are silly suggestions all over about alignment of things--these are mainly to make the code look prettier, though that's a matter of opinion. I still prefer having lines generally closer to 80 columns wide, but I've already mentioned that... I really focused on the code, and not the documentation. In fact I didn't even pay much attention to your patch headers either. I did not review the SCM calls yet. So in summary I have not reviewed patches 1, 2, 16, 17, and 26. I try to look at everything in my next review, which I hope will be final (or very close). -Alex > The diagram below shows the architecture. > > :: > > VM A VM B > +-----+ +-----+ | +-----+ +-----+ +-----+ > | | | | | | | | | | | > EL0 | APP | | APP | | | APP | | APP | | APP | > | | | | | | | | | | | > +-----+ +-----+ | +-----+ +-----+ +-----+ > ---------------------|------------------------- > +--------------+ | +----------------------+ > | | | | | > EL1 | Linux Kernel | | |Linux kernel/Other OS | ... > | | | | | > +--------------+ | +----------------------+ > --------hvc/smc------|------hvc/smc------------ > +----------------------------------------+ > | | > EL2 | Gunyah Hypervisor | > | | > +----------------------------------------+ > > Gunyah provides these following features. > . . .
On 3/3/23 7:06 PM, Elliot Berman wrote: > Add hypercalls to identify when Linux is running a virtual machine under > Gunyah. > > There are two calls to help identify Gunyah: > > 1. gh_hypercall_get_uid() returns a UID when running under a Gunyah > hypervisor. > 2. gh_hypercall_hyp_identify() returns build information and a set of > feature flags that are supported by Gunyah. > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> Two very minor comments below. -Alex > --- > arch/arm64/Kbuild | 1 + > arch/arm64/gunyah/Makefile | 3 ++ > arch/arm64/gunyah/gunyah_hypercall.c | 64 ++++++++++++++++++++++++++++ > drivers/virt/Kconfig | 2 + > drivers/virt/gunyah/Kconfig | 13 ++++++ > include/linux/gunyah.h | 28 ++++++++++++ > 6 files changed, 111 insertions(+) > create mode 100644 arch/arm64/gunyah/Makefile > create mode 100644 arch/arm64/gunyah/gunyah_hypercall.c > create mode 100644 drivers/virt/gunyah/Kconfig > > diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild > index 5bfbf7d79c99..e4847ba0e3c9 100644 > --- a/arch/arm64/Kbuild > +++ b/arch/arm64/Kbuild > @@ -3,6 +3,7 @@ obj-y += kernel/ mm/ net/ > obj-$(CONFIG_KVM) += kvm/ > obj-$(CONFIG_XEN) += xen/ > obj-$(subst m,y,$(CONFIG_HYPERV)) += hyperv/ > +obj-$(CONFIG_GUNYAH) += gunyah/ > obj-$(CONFIG_CRYPTO) += crypto/ > > # for cleaning > diff --git a/arch/arm64/gunyah/Makefile b/arch/arm64/gunyah/Makefile > new file mode 100644 > index 000000000000..84f1e38cafb1 > --- /dev/null > +++ b/arch/arm64/gunyah/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +obj-$(CONFIG_GUNYAH) += gunyah_hypercall.o > diff --git a/arch/arm64/gunyah/gunyah_hypercall.c b/arch/arm64/gunyah/gunyah_hypercall.c > new file mode 100644 > index 000000000000..0d14e767e2c8 > --- /dev/null > +++ b/arch/arm64/gunyah/gunyah_hypercall.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/arm-smccc.h> > +#include <linux/module.h> > +#include <linux/gunyah.h> > +#include <linux/uuid.h> > + > +static const uuid_t gh_known_uuids[] = { > + /* Qualcomm's version of Gunyah {19bd54bd-0b37-571b-946f-609b54539de6} */ > + UUID_INIT(0x19bd54bd, 0x0b37, 0x571b, 0x94, 0x6f, 0x60, 0x9b, 0x54, 0x53, 0x9d, 0xe6), > + /* Standard version of Gunyah {c1d58fcd-a453-5fdb-9265-ce36673d5f14} */ > + UUID_INIT(0xc1d58fcd, 0xa453, 0x5fdb, 0x92, 0x65, 0xce, 0x36, 0x67, 0x3d, 0x5f, 0x14), > +}; > + > +bool arch_is_gh_guest(void) > +{ > + struct arm_smccc_res res; > + uuid_t uuid; > + int i; > + > + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); > + > + ((u32 *)&uuid.b[0])[0] = lower_32_bits(res.a0); > + ((u32 *)&uuid.b[0])[1] = lower_32_bits(res.a1); > + ((u32 *)&uuid.b[0])[2] = lower_32_bits(res.a2); > + ((u32 *)&uuid.b[0])[3] = lower_32_bits(res.a3); > + > + for (i = 0; i < ARRAY_SIZE(gh_known_uuids); i++) > + if (uuid_equal(&uuid, &gh_known_uuids[i])) > + return true; > + > + return false; > +} > +EXPORT_SYMBOL_GPL(arch_is_gh_guest); > + > +#define GH_HYPERCALL(fn) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64, \ > + ARM_SMCCC_OWNER_VENDOR_HYP, \ > + fn) > + > +#define GH_HYPERCALL_HYP_IDENTIFY GH_HYPERCALL(0x8000) > + > +/** > + * gh_hypercall_hyp_identify() - Returns build information and feature flags > + * supported by Gunyah. > + * @hyp_identity: filled by the hypercall with the API info and feature flags. > + */ > +void gh_hypercall_hyp_identify(struct gh_hypercall_hyp_identify_resp *hyp_identity) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_1_1_hvc(GH_HYPERCALL_HYP_IDENTIFY, &res); > + > + hyp_identity->api_info = res.a0; > + hyp_identity->flags[0] = res.a1; > + hyp_identity->flags[1] = res.a2; > + hyp_identity->flags[2] = res.a3; > +} > +EXPORT_SYMBOL_GPL(gh_hypercall_hyp_identify); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Gunyah Hypervisor Hypercalls"); > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig > index f79ab13a5c28..85bd6626ffc9 100644 > --- a/drivers/virt/Kconfig > +++ b/drivers/virt/Kconfig > @@ -54,4 +54,6 @@ source "drivers/virt/coco/sev-guest/Kconfig" > > source "drivers/virt/coco/tdx-guest/Kconfig" > > +source "drivers/virt/gunyah/Kconfig" > + > endif > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig > new file mode 100644 > index 000000000000..1a737694c333 > --- /dev/null > +++ b/drivers/virt/gunyah/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +config GUNYAH > + tristate "Gunyah Virtualization drivers" > + depends on ARM64 > + depends on MAILBOX > + help > + The Gunyah drivers are the helper interfaces that run in a guest VM > + such as basic inter-VM IPC and signaling mechanisms, and higher level > + services such as memory/device sharing, IRQ sharing, and so on. > + > + Say Y/M here to enable the drivers needed to interact in a Gunyah > + virtual environment. > diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h > index 54b4be71caf7..bd080e3a6fc9 100644 > --- a/include/linux/gunyah.h > +++ b/include/linux/gunyah.h > @@ -6,8 +6,10 @@ > #ifndef _LINUX_GUNYAH_H > #define _LINUX_GUNYAH_H > > +#include <linux/bitfield.h> > #include <linux/errno.h> > #include <linux/limits.h> > +#include <linux/types.h> > > /******************************************************************************/ > /* Common arch-independent definitions for Gunyah hypercalls */ > @@ -80,4 +82,30 @@ static inline int gh_remap_error(enum gh_error gh_error) > } > } > > +enum gh_api_feature { > + GH_FEATURE_DOORBELL = 1, > + GH_FEATURE_MSGQUEUE = 2, > + GH_FEATURE_VCPU = 5, Fix alignment in the line above. > + GH_FEATURE_MEMEXTENT = 6, > +}; > + > +bool arch_is_gh_guest(void); > + > +u16 gh_api_version(void); > +bool gh_api_has_feature(enum gh_api_feature feature); > + > +#define GH_API_V1 1 > + > +#define GH_API_INFO_API_VERSION_MASK GENMASK_ULL(13, 0) > +#define GH_API_INFO_BIG_ENDIAN BIT_ULL(14) > +#define GH_API_INFO_IS_64BIT BIT_ULL(15) Maybe a comment saying "bits 16-55 are reserved"? > +#define GH_API_INFO_VARIANT_MASK GENMASK_ULL(63, 56) > + > +struct gh_hypercall_hyp_identify_resp { > + u64 api_info; > + u64 flags[3]; > +}; > + > +void gh_hypercall_hyp_identify(struct gh_hypercall_hyp_identify_resp *hyp_identity); > + > #endif
On 3/3/23 7:06 PM, Elliot Berman wrote: > Gunyah VM manager is a kernel moduel which exposes an interface to > Gunyah userspace to load, run, and interact with other Gunyah virtual > machines. The interface is a character device at /dev/gunyah. > > Add a basic VM manager driver. Upcoming patches will add more ioctls > into this driver. > > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> One suggestion to move some code here. And a few other minor things. -Alex > --- > .../userspace-api/ioctl/ioctl-number.rst | 1 + > drivers/virt/gunyah/Makefile | 2 +- > drivers/virt/gunyah/rsc_mgr.c | 38 +++++- > drivers/virt/gunyah/vm_mgr.c | 116 ++++++++++++++++++ > drivers/virt/gunyah/vm_mgr.h | 23 ++++ > include/uapi/linux/gunyah.h | 23 ++++ > 6 files changed, 201 insertions(+), 2 deletions(-) > create mode 100644 drivers/virt/gunyah/vm_mgr.c > create mode 100644 drivers/virt/gunyah/vm_mgr.h > create mode 100644 include/uapi/linux/gunyah.h > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst > index 0a1882e296ae..2513324ae7be 100644 > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > @@ -137,6 +137,7 @@ Code Seq# Include File Comments > 'F' DD video/sstfb.h conflict! > 'G' 00-3F drivers/misc/sgi-gru/grulib.h conflict! > 'G' 00-0F xen/gntalloc.h, xen/gntdev.h conflict! > +'G' 00-0f linux/gunyah.h conflict! > 'H' 00-7F linux/hiddev.h conflict! > 'H' 00-0F linux/hidraw.h conflict! > 'H' 01 linux/mei.h conflict! > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile > index de29769f2f3f..03951cf82023 100644 > --- a/drivers/virt/gunyah/Makefile > +++ b/drivers/virt/gunyah/Makefile > @@ -2,5 +2,5 @@ > > obj-$(CONFIG_GUNYAH) += gunyah.o > > -gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o > +gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o > obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o > diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c > index 67813c9a52db..d7ce692d0067 100644 > --- a/drivers/virt/gunyah/rsc_mgr.c > +++ b/drivers/virt/gunyah/rsc_mgr.c > @@ -15,8 +15,10 @@ > #include <linux/completion.h> > #include <linux/gunyah_rsc_mgr.h> > #include <linux/platform_device.h> > +#include <linux/miscdevice.h> > > #include "rsc_mgr.h" > +#include "vm_mgr.h" > > #define RM_RPC_API_VERSION_MASK GENMASK(3, 0) > #define RM_RPC_HEADER_WORDS_MASK GENMASK(7, 4) > @@ -129,6 +131,7 @@ struct gh_rm_connection { > * @cache: cache for allocating Tx messages > * @send_lock: synchronization to allow only one request to be sent at a time > * @nh: notifier chain for clients interested in RM notification messages > + * @miscdev: /dev/gunyah > */ > struct gh_rm { > struct device *dev; > @@ -145,6 +148,8 @@ struct gh_rm { > struct kmem_cache *cache; > struct mutex send_lock; > struct blocking_notifier_head nh; > + > + struct miscdevice miscdev; > }; > > /** > @@ -593,6 +598,21 @@ void gh_rm_put(struct gh_rm *rm) > } > EXPORT_SYMBOL_GPL(gh_rm_put); > I feel like /dev/gunyah code would more appropriately be found in "vm_mgr.c". All gh_dev_ioctl() does is call the function defined there, and it's therefore a VM-oriented rather than resource-oriented device. > +static long gh_dev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + struct miscdevice *miscdev = filp->private_data; > + struct gh_rm *rm = container_of(miscdev, struct gh_rm, miscdev); > + > + return gh_dev_vm_mgr_ioctl(rm, cmd, arg); > +} > + > +static const struct file_operations gh_dev_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = gh_dev_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > + .llseek = noop_llseek, > +}; > + > static int gh_msgq_platform_probe_direction(struct platform_device *pdev, bool tx, > struct gh_resource *ghrsc) > { > @@ -651,7 +671,22 @@ static int gh_rm_drv_probe(struct platform_device *pdev) > rm->msgq_client.rx_callback = gh_rm_msgq_rx_data; > rm->msgq_client.tx_done = gh_rm_msgq_tx_done; > > - return gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, &rm->tx_ghrsc, &rm->rx_ghrsc); > + ret = gh_msgq_init(&pdev->dev, &rm->msgq, &rm->msgq_client, &rm->tx_ghrsc, &rm->rx_ghrsc); > + if (ret) > + goto err_cache; > + > + rm->miscdev.name = "gunyah"; > + rm->miscdev.minor = MISC_DYNAMIC_MINOR; > + rm->miscdev.fops = &gh_dev_fops; > + > + ret = misc_register(&rm->miscdev); > + if (ret) > + goto err_msgq; > + > + return 0; > +err_msgq: > + mbox_free_channel(gh_msgq_chan(&rm->msgq)); > + gh_msgq_remove(&rm->msgq); > err_cache: > kmem_cache_destroy(rm->cache); > return ret; > @@ -661,6 +696,7 @@ static int gh_rm_drv_remove(struct platform_device *pdev) > { > struct gh_rm *rm = platform_get_drvdata(pdev); > > + misc_deregister(&rm->miscdev); > mbox_free_channel(gh_msgq_chan(&rm->msgq)); > gh_msgq_remove(&rm->msgq); > kmem_cache_destroy(rm->cache); > diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c > new file mode 100644 > index 000000000000..dbacf36af72d > --- /dev/null > +++ b/drivers/virt/gunyah/vm_mgr.c > @@ -0,0 +1,116 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) "gh_vm_mgr: " fmt > + > +#include <linux/anon_inodes.h> > +#include <linux/file.h> > +#include <linux/gunyah_rsc_mgr.h> > +#include <linux/miscdevice.h> > +#include <linux/module.h> > + > +#include <uapi/linux/gunyah.h> > + > +#include "vm_mgr.h" > + > +static void gh_vm_free(struct work_struct *work) > +{ > + struct gh_vm *ghvm = container_of(work, struct gh_vm, free_work); > + int ret; > + > + ret = gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid); > + if (ret) > + pr_warn("Failed to deallocate vmid: %d\n", ret); > + > + put_gh_rm(ghvm->rm); > + kfree(ghvm); > +} > + > +static __must_check struct gh_vm *gh_vm_alloc(struct gh_rm *rm) > +{ > + struct gh_vm *ghvm; > + int vmid; > + > + vmid = gh_rm_alloc_vmid(rm, 0); > + if (vmid < 0) > + return ERR_PTR(vmid); > + > + ghvm = kzalloc(sizeof(*ghvm), GFP_KERNEL); > + if (!ghvm) { > + gh_rm_dealloc_vmid(rm, vmid); > + return ERR_PTR(-ENOMEM); > + } > + > + ghvm->parent = gh_rm_get(rm); > + ghvm->vmid = vmid; > + ghvm->rm = rm; > + > + INIT_WORK(&ghvm->free_work, gh_vm_free); > + > + return ghvm; > +} > + > +static int gh_vm_release(struct inode *inode, struct file *filp) > +{ > + struct gh_vm *ghvm = filp->private_data; > + > + /* VM will be reset and make RM calls which can interruptible sleep. > + * Defer to a work so this thread can receive signal. > + */ > + schedule_work(&ghvm->free_work); > + return 0; > +} > + > +static const struct file_operations gh_vm_fops = { > + .release = gh_vm_release, > + .llseek = noop_llseek, > +}; > + > +static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg) > +{ > + struct gh_vm *ghvm; > + struct file *file; > + int fd, err; > + > + /* arg reserved for future use. */ Do you have a clear idea of how this might be used in the future? I was thinking you could silently ignore the argument value, but I suppose if it *does* get used in the future, you want the caller to know it's being ignored. (Is that right?) > + if (arg) > + return -EINVAL; > + > + ghvm = gh_vm_alloc(rm); > + if (IS_ERR(ghvm)) > + return PTR_ERR(ghvm); > + > + fd = get_unused_fd_flags(O_CLOEXEC); > + if (fd < 0) { > + err = fd; > + goto err_destroy_vm; > + } > + > + file = anon_inode_getfile("gunyah-vm", &gh_vm_fops, ghvm, O_RDWR); > + if (IS_ERR(file)) { > + err = PTR_ERR(file); > + goto err_put_fd; > + } > + > + fd_install(fd, file); > + > + return fd; > + > +err_put_fd: > + put_unused_fd(fd); > +err_destroy_vm: > + gh_vm_free(&ghvm->free_work); > + return err; > +} > + > +long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd, unsigned long arg) > +{ > + switch (cmd) { > + case GH_CREATE_VM: > + return gh_dev_ioctl_create_vm(rm, arg); > + default: > + return -ENOIOCTLCMD; > + } > +} > diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h > new file mode 100644 > index 000000000000..4b22fbcac91c > --- /dev/null > +++ b/drivers/virt/gunyah/vm_mgr.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _GH_PRIV_VM_MGR_H > +#define _GH_PRIV_VM_MGR_H Maybe _GH_VM_MGR_H? > + > +#include <linux/gunyah_rsc_mgr.h> > + > +#include <uapi/linux/gunyah.h> > + > +long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd, unsigned long arg); > + > +struct gh_vm { > + u16 vmid; > + struct gh_rm *rm; > + struct device *parent; > + > + struct work_struct free_work; > +}; > + > +#endif > diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h > new file mode 100644 > index 000000000000..10ba32d2b0a6 > --- /dev/null > +++ b/include/uapi/linux/gunyah.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _UAPI_LINUX_GUNYAH > +#define _UAPI_LINUX_GUNYAH Use _UAPI_LINUX_GUNYAH_H > + > +/* > + * Userspace interface for /dev/gunyah - gunyah based virtual machine > + */ > + > +#include <linux/types.h> > +#include <linux/ioctl.h> > + > +#define GH_IOCTL_TYPE 'G' > + > +/* > + * ioctls for /dev/gunyah fds: > + */ > +#define GH_CREATE_VM _IO(GH_IOCTL_TYPE, 0x0) /* Returns a Gunyah VM fd */ > + > +#endif
On 3/3/23 7:06 PM, Elliot Berman wrote: > When booting a Gunyah virtual machine, the host VM may gain capabilities > to interact with resources for the guest virtual machine. Examples of > such resources are vCPUs or message queues. To use those resources, we > need to translate the RM response into a gunyah_resource structure which > are useful to Linux drivers. Presently, Linux drivers need only to know > the type of resource, the capability ID, and an interrupt. > > On ARM64 systems, the interrupt reported by Gunyah is the GIC interrupt > ID number and always a SPI. > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> Several comments here, nothing major. -Alex > --- > arch/arm64/include/asm/gunyah.h | 23 +++++ > drivers/virt/gunyah/rsc_mgr.c | 163 +++++++++++++++++++++++++++++++- > include/linux/gunyah.h | 4 + > include/linux/gunyah_rsc_mgr.h | 3 + > 4 files changed, 192 insertions(+), 1 deletion(-) > create mode 100644 arch/arm64/include/asm/gunyah.h > > diff --git a/arch/arm64/include/asm/gunyah.h b/arch/arm64/include/asm/gunyah.h > new file mode 100644 > index 000000000000..64cfb964efee > --- /dev/null > +++ b/arch/arm64/include/asm/gunyah.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > +#ifndef __ASM_GUNYAH_H_ > +#define __ASM_GUNYAH_H_ Maybe just one _ at the beginning and none at the end? Follow the same convention across all your header files. (Maybe you're looking at other files in the same directory as this one, but that's not consistent.) > + > +#include <linux/irq.h> > +#include <dt-bindings/interrupt-controller/arm-gic.h> > + > +static inline int arch_gh_fill_irq_fwspec_params(u32 virq, struct irq_fwspec *fwspec) > +{ > + if (virq < 32 || virq > 1019) > + return -EINVAL; What is special about VIRQs greater than 1019 (minus 32)? It's probably documented somewhere but it's worth adding a comment here to explain the check. You would know better than I, but could/should the caller be responsible for this check? (Not a big deal.) > + > + fwspec->param_count = 3; > + fwspec->param[0] = GIC_SPI; > + fwspec->param[1] = virq - 32; And why is 32 subtracted? > + fwspec->param[2] = IRQ_TYPE_EDGE_RISING; > + return 0; > +} > + > +#endif > diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c > index d7ce692d0067..383be5ac0f44 100644 > --- a/drivers/virt/gunyah/rsc_mgr.c > +++ b/drivers/virt/gunyah/rsc_mgr.c > @@ -17,6 +17,8 @@ > #include <linux/platform_device.h> > #include <linux/miscdevice.h> > > +#include <asm/gunyah.h> > + > #include "rsc_mgr.h" > #include "vm_mgr.h" > > @@ -132,6 +134,7 @@ struct gh_rm_connection { > * @send_lock: synchronization to allow only one request to be sent at a time > * @nh: notifier chain for clients interested in RM notification messages > * @miscdev: /dev/gunyah > + * @irq_domain: Domain to translate Gunyah hwirqs to Linux irqs > */ > struct gh_rm { > struct device *dev; > @@ -150,6 +153,7 @@ struct gh_rm { > struct blocking_notifier_head nh; > > struct miscdevice miscdev; > + struct irq_domain *irq_domain; > }; > > /** > @@ -190,6 +194,134 @@ static inline int gh_rm_remap_error(enum gh_rm_error rm_error) > } > } > > +struct gh_irq_chip_data { > + u32 gh_virq; > +}; > + > +static struct irq_chip gh_rm_irq_chip = { > + .name = "Gunyah", > + .irq_enable = irq_chip_enable_parent, > + .irq_disable = irq_chip_disable_parent, > + .irq_ack = irq_chip_ack_parent, > + .irq_mask = irq_chip_mask_parent, > + .irq_mask_ack = irq_chip_mask_ack_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = irq_chip_set_affinity_parent, > + .irq_set_type = irq_chip_set_type_parent, > + .irq_set_wake = irq_chip_set_wake_parent, > + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, > + .irq_retrigger = irq_chip_retrigger_hierarchy, > + .irq_get_irqchip_state = irq_chip_get_parent_state, > + .irq_set_irqchip_state = irq_chip_set_parent_state, > + .flags = IRQCHIP_SET_TYPE_MASKED | > + IRQCHIP_SKIP_SET_WAKE | > + IRQCHIP_MASK_ON_SUSPEND, > +}; > + > +static int gh_rm_irq_domain_alloc(struct irq_domain *d, unsigned int virq, unsigned int nr_irqs, > + void *arg) > +{ > + struct gh_irq_chip_data *chip_data, *spec = arg; > + struct irq_fwspec parent_fwspec; > + struct gh_rm *rm = d->host_data; > + u32 gh_virq = spec->gh_virq; > + int ret; > + > + if (nr_irqs != 1 || gh_virq == U32_MAX) Does U32_MAX have special meaning? Why are you checking for it? Whatever it is, you should explain why this is invalid here. > + return -EINVAL; > + > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); > + if (!chip_data) > + return -ENOMEM; > + > + chip_data->gh_virq = gh_virq; > + > + ret = irq_domain_set_hwirq_and_chip(d, virq, chip_data->gh_virq, &gh_rm_irq_chip, > + chip_data); > + if (ret) > + goto err_free_irq_data; > + > + parent_fwspec.fwnode = d->parent->fwnode; > + ret = arch_gh_fill_irq_fwspec_params(chip_data->gh_virq, &parent_fwspec); > + if (ret) { > + dev_err(rm->dev, "virq translation failed %u: %d\n", chip_data->gh_virq, ret); > + goto err_free_irq_data; > + } > + > + ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec); > + if (ret) > + goto err_free_irq_data; > + > + return ret; > +err_free_irq_data: > + kfree(chip_data); > + return ret; > +} > + > +static void gh_rm_irq_domain_free_single(struct irq_domain *d, unsigned int virq) > +{ > + struct gh_irq_chip_data *chip_data; No need to define chip_data. > + struct irq_data *irq_data; > + > + irq_data = irq_domain_get_irq_data(d, virq); > + if (!irq_data) > + return; > + > + chip_data = irq_data->chip_data; > + > + kfree(chip_data); Just call kfree(irq_data->chip_data); > + irq_data->chip_data = NULL; > +} > + > +static void gh_rm_irq_domain_free(struct irq_domain *d, unsigned int virq, unsigned int nr_irqs) > +{ > + unsigned int i; > + > + for (i = 0; i < nr_irqs; i++) > + gh_rm_irq_domain_free_single(d, virq); > +} > + > +static const struct irq_domain_ops gh_rm_irq_domain_ops = { > + .alloc = gh_rm_irq_domain_alloc, > + .free = gh_rm_irq_domain_free, > +}; > + > +struct gh_resource *gh_rm_alloc_resource(struct gh_rm *rm, struct gh_rm_hyp_resource *hyp_resource) > +{ > + struct gh_resource *ghrsc; > + > + ghrsc = kzalloc(sizeof(*ghrsc), GFP_KERNEL); > + if (!ghrsc) > + return NULL; > + > + ghrsc->type = hyp_resource->type; > + ghrsc->capid = le64_to_cpu(hyp_resource->cap_id); > + ghrsc->irq = IRQ_NOTCONNECTED; > + ghrsc->rm_label = le32_to_cpu(hyp_resource->resource_label); > + if (hyp_resource->virq && le32_to_cpu(hyp_resource->virq) != U32_MAX) { Again, does U32_MAX have a particular meaning here? > + struct gh_irq_chip_data irq_data = { > + .gh_virq = le32_to_cpu(hyp_resource->virq), > + }; > + > + ghrsc->irq = irq_domain_alloc_irqs(rm->irq_domain, 1, NUMA_NO_NODE, &irq_data); > + if (ghrsc->irq < 0) { > + dev_err(rm->dev, > + "Failed to allocate interrupt for resource %d label: %d: %d\n", > + ghrsc->type, ghrsc->rm_label, ghrsc->irq); > + ghrsc->irq = IRQ_NOTCONNECTED; ghrsc->irq already had that value. You could use a local variable irq to hold the value, and then assign ghrsc->irq after you know it's good. > + } > + } > + > + return ghrsc; > +} > + > +void gh_rm_free_resource(struct gh_resource *ghrsc) > +{ > + irq_dispose_mapping(ghrsc->irq); > + kfree(ghrsc); > +} > + > static int gh_rm_init_connection_payload(struct gh_rm_connection *connection, void *msg, > size_t hdr_size, size_t msg_size) > { > @@ -639,6 +771,8 @@ static int gh_msgq_platform_probe_direction(struct platform_device *pdev, bool t > > static int gh_rm_drv_probe(struct platform_device *pdev) > { > + struct irq_domain *parent_irq_domain; > + struct device_node *parent_irq_node; > struct gh_msgq_tx_data *msg; > struct gh_rm *rm; > int ret; > @@ -675,15 +809,41 @@ static int gh_rm_drv_probe(struct platform_device *pdev) > if (ret) > goto err_cache; > > + parent_irq_node = of_irq_find_parent(pdev->dev.of_node); > + if (!parent_irq_node) { > + dev_err(&pdev->dev, "Failed to find interrupt parent of resource manager\n"); > + ret = -ENODEV; > + goto err_msgq; > + } > + > + parent_irq_domain = irq_find_host(parent_irq_node); > + if (!parent_irq_domain) { > + dev_err(&pdev->dev, "Failed to find interrupt parent domain of resource manager\n"); > + ret = -ENODEV; > + goto err_msgq; > + } > + > + rm->irq_domain = irq_domain_add_hierarchy(parent_irq_domain, 0, 0, pdev->dev.of_node, > + &gh_rm_irq_domain_ops, NULL); > + if (!rm->irq_domain) { > + dev_err(&pdev->dev, "Failed to add irq domain\n"); > + ret = -ENODEV; > + goto err_msgq; > + } > + rm->irq_domain->host_data = rm; > + > + rm->miscdev.parent = &pdev->dev; > rm->miscdev.name = "gunyah"; > rm->miscdev.minor = MISC_DYNAMIC_MINOR; > rm->miscdev.fops = &gh_dev_fops; > > ret = misc_register(&rm->miscdev); > if (ret) > - goto err_msgq; > + goto err_irq_domain; > > return 0; > +err_irq_domain: > + irq_domain_remove(rm->irq_domain); > err_msgq: > mbox_free_channel(gh_msgq_chan(&rm->msgq)); > gh_msgq_remove(&rm->msgq); > @@ -697,6 +857,7 @@ static int gh_rm_drv_remove(struct platform_device *pdev) > struct gh_rm *rm = platform_get_drvdata(pdev); > > misc_deregister(&rm->miscdev); > + irq_domain_remove(rm->irq_domain); > mbox_free_channel(gh_msgq_chan(&rm->msgq)); > gh_msgq_remove(&rm->msgq); > kmem_cache_destroy(rm->cache); > diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h > index 378bec0f2ce1..3e706b59d2c0 100644 > --- a/include/linux/gunyah.h > +++ b/include/linux/gunyah.h > @@ -27,6 +27,10 @@ struct gh_resource { > enum gh_resource_type type; > u64 capid; > unsigned int irq; > + > + /* To help allocator in vm manager */ I don't find the above comment helpful. > + struct list_head list; > + u32 rm_label; > }; > > /** > diff --git a/include/linux/gunyah_rsc_mgr.h b/include/linux/gunyah_rsc_mgr.h > index acf8c1545a6c..58693c27cf1a 100644 > --- a/include/linux/gunyah_rsc_mgr.h > +++ b/include/linux/gunyah_rsc_mgr.h > @@ -145,6 +145,9 @@ int gh_rm_get_hyp_resources(struct gh_rm *rm, u16 vmid, > struct gh_rm_hyp_resources **resources); > int gh_rm_get_vmid(struct gh_rm *rm, u16 *vmid); > > +struct gh_resource *gh_rm_alloc_resource(struct gh_rm *rm, struct gh_rm_hyp_resource *hyp_resource); > +void gh_rm_free_resource(struct gh_resource *ghrsc); > + > struct gh_rm_platform_ops { > int (*pre_mem_share)(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel); > int (*post_mem_reclaim)(struct gh_rm *rm, struct gh_rm_mem_parcel *mem_parcel);
On 3/3/23 7:06 PM, Elliot Berman wrote: > Gunyah allows host virtual machines to schedule guest virtual machines > and handle their MMIO accesses. vCPUs are presented to the host as a > Gunyah resource and represented to userspace as a Gunyah VM function. > > Creating the vcpu VM function will create a file descriptor that: > - can run an ioctl: GH_VCPU_RUN to schedule the guest vCPU until the > next interrupt occurs on the host or when the guest vCPU can no > longer be run. > - can be mmap'd to share a gh_vcpu_run structure which can look up the > reason why GH_VCPU_RUN returned and provide return values for MMIO > access. > > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> I suggest reorganizing and renaming a few things here, but I don't think there's anything major. -Alex > --- > Documentation/virt/gunyah/vm-manager.rst | 46 ++- > arch/arm64/gunyah/gunyah_hypercall.c | 28 ++ > drivers/virt/gunyah/Kconfig | 11 + > drivers/virt/gunyah/Makefile | 2 + > drivers/virt/gunyah/gunyah_vcpu.c | 465 +++++++++++++++++++++++ > drivers/virt/gunyah/vm_mgr.c | 4 + > drivers/virt/gunyah/vm_mgr.h | 1 + > include/linux/gunyah.h | 8 + > include/uapi/linux/gunyah.h | 108 ++++++ > 9 files changed, 671 insertions(+), 2 deletions(-) > create mode 100644 drivers/virt/gunyah/gunyah_vcpu.c > > diff --git a/Documentation/virt/gunyah/vm-manager.rst b/Documentation/virt/gunyah/vm-manager.rst > index af8ad88a88ab..83d326b0d11f 100644 > --- a/Documentation/virt/gunyah/vm-manager.rst > +++ b/Documentation/virt/gunyah/vm-manager.rst > @@ -5,8 +5,7 @@ Virtual Machine Manager > ======================= > > The Gunyah Virtual Machine Manager is a Linux driver to support launching > -virtual machines using Gunyah. It presently supports launching non-proxy > -scheduled Linux-like virtual machines. > +virtual machines using Gunyah. > > Except for some basic information about the location of initial binaries, > most of the configuration about a Gunyah virtual machine is described in the > @@ -107,3 +106,46 @@ GH_VM_START > ~~~~~~~~~~~ > > This ioctl starts the VM. > + > +GH_VM_ADD_FUNCTION > +~~~~~~~~~~~~~~~~~~ > + > +This ioctl registers a Gunyah VM function with the VM manager. The VM function > +is described with a `type` string and some arguments for that type. Typically, > +the function is added before the VM starts, but the function doesn't "operate" > +until the VM starts with GH_VM_START: e.g. vCPU ioclts will all return an error > +until the VM starts because the vCPUs don't exist until the VM is started. This > +allows the VMM to set up all the kernel functionality needed for the VM *before* > +the VM starts. > + > +.. kernel-doc:: include/uapi/linux/gunyah.h > + :identifiers: gh_fn_desc > + > +The possible types are documented below: > + > +.. kernel-doc:: include/uapi/linux/gunyah.h > + :identifiers: GH_FN_VCPU gh_fn_vcpu_arg > + > +Gunyah VCPU API Descriptions > +---------------------------- > + > +A vCPU file descriptor is created after calling `GH_VM_ADD_FUNCTION` with the type `GH_FN_VCPU`. > + > +GH_VCPU_RUN > +~~~~~~~~~~~ > + > +This ioctl is used to run a guest virtual cpu. While there are no > +explicit parameters, there is an implicit parameter block that can be > +obtained by mmap()ing the vcpu fd at offset 0, with the size given by > +GH_VCPU_MMAP_SIZE. The parameter block is formatted as a 'struct > +gh_vcpu_run' (see below). > + > +GH_VCPU_MMAP_SIZE > +~~~~~~~~~~~~~~~~~ > + > +The GH_VCPU_RUN ioctl communicates with userspace via a shared > +memory region. This ioctl returns the size of that region. See the > +GH_VCPU_RUN documentation for details. > + > +.. kernel-doc:: include/uapi/linux/gunyah.h > + :identifiers: gh_vcpu_run gh_vm_exit_info > diff --git a/arch/arm64/gunyah/gunyah_hypercall.c b/arch/arm64/gunyah/gunyah_hypercall.c > index 3420d8f286a9..f01f5cec4d23 100644 > --- a/arch/arm64/gunyah/gunyah_hypercall.c > +++ b/arch/arm64/gunyah/gunyah_hypercall.c > @@ -43,6 +43,7 @@ EXPORT_SYMBOL_GPL(arch_is_gh_guest); > #define GH_HYPERCALL_HYP_IDENTIFY GH_HYPERCALL(0x8000) > #define GH_HYPERCALL_MSGQ_SEND GH_HYPERCALL(0x801B) > #define GH_HYPERCALL_MSGQ_RECV GH_HYPERCALL(0x801C) > +#define GH_HYPERCALL_VCPU_RUN GH_HYPERCALL(0x8065) > > /** > * gh_hypercall_hyp_identify() - Returns build information and feature flags > @@ -91,5 +92,32 @@ enum gh_error gh_hypercall_msgq_recv(u64 capid, void *buff, size_t size, size_t > } > EXPORT_SYMBOL_GPL(gh_hypercall_msgq_recv); > > +enum gh_error gh_hypercall_vcpu_run(u64 capid, u64 *resume_data, > + struct gh_hypercall_vcpu_run_resp *resp) > +{ > + struct arm_smccc_1_2_regs args = { > + .a0 = GH_HYPERCALL_VCPU_RUN, > + .a1 = capid, > + .a2 = resume_data[0], > + .a3 = resume_data[1], > + .a4 = resume_data[2], > + /* C language says this will be implictly zero. Gunyah requires 0, so be explicit */ > + .a5 = 0, > + }; > + struct arm_smccc_1_2_regs res; > + > + arm_smccc_1_2_hvc(&args, &res); > + > + if (res.a0 == GH_ERROR_OK) { > + resp->state = res.a1; > + resp->state_data[0] = res.a2; > + resp->state_data[1] = res.a3; > + resp->state_data[2] = res.a4; > + } > + > + return res.a0; > +} > +EXPORT_SYMBOL_GPL(gh_hypercall_vcpu_run); > + > MODULE_LICENSE("GPL"); > MODULE_DESCRIPTION("Gunyah Hypervisor Hypercalls"); > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig > index de815189dab6..4c1c6110b50e 100644 > --- a/drivers/virt/gunyah/Kconfig > +++ b/drivers/virt/gunyah/Kconfig > @@ -15,3 +15,14 @@ config GUNYAH > > config GUNYAH_PLATFORM_HOOKS > tristate > + > +config GUNYAH_VCPU > + tristate "Runnable Gunyah vCPUs" > + depends on GUNYAH > + help > + Enable kernel support for host-scheduled vCPUs running under Gunyah. > + When selecting this option, userspace virtual machine managers (VMM) > + can schedule the guest VM's vCPUs instead of using Gunyah's scheduler. > + VMMs can also handle stage 2 faults of the vCPUs. > + > + Say Y/M here if unsure and you want to support Gunyah VMMs. > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile > index 6b8f84dbfe0d..2d1b604a7b03 100644 > --- a/drivers/virt/gunyah/Makefile > +++ b/drivers/virt/gunyah/Makefile > @@ -5,3 +5,5 @@ obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o > > gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o > obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o > + > +obj-$(CONFIG_GUNYAH_VCPU) += gunyah_vcpu.o > diff --git a/drivers/virt/gunyah/gunyah_vcpu.c b/drivers/virt/gunyah/gunyah_vcpu.c > new file mode 100644 > index 000000000000..870e471a11df > --- /dev/null > +++ b/drivers/virt/gunyah/gunyah_vcpu.c > @@ -0,0 +1,465 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/anon_inodes.h> > +#include <linux/file.h> > +#include <linux/gunyah.h> > +#include <linux/gunyah_vm_mgr.h> > +#include <linux/interrupt.h> > +#include <linux/kref.h> > +#include <linux/mm.h> > +#include <linux/module.h> > +#include <linux/types.h> > +#include <linux/wait.h> > + > +#include "vm_mgr.h" > + > +#include <uapi/linux/gunyah.h> > + > +#define MAX_VCPU_NAME 20 /* gh-vcpu:u32_max+NUL */ > + > +struct gh_vcpu { > + struct gh_vm_function_instance *f; > + struct gh_resource *rsc; > + struct mutex run_lock; > + /* Track why vcpu_run left last time around. */ > + enum { > + GH_VCPU_UNKNOWN = 0, > + GH_VCPU_READY, > + GH_VCPU_MMIO_READ, > + GH_VCPU_SYSTEM_DOWN, > + } state; > + u8 mmio_read_len; > + struct gh_vcpu_run *vcpu_run; > + struct completion ready; > + struct gh_vm *ghvm; > + > + struct notifier_block nb; > + struct gh_vm_resource_ticket ticket; > + struct kref kref; > +}; > + Here again, I suggest defining the states using an enumerated type. Then add kernel-doc comments to describe them, rather than these one-line comments. I like enums because it gives you a way to refer to the group of values by name. > +/* VCPU is ready to run */ > +#define GH_VCPU_STATE_READY 0 > +/* VCPU is sleeping until an interrupt arrives */ > +#define GH_VCPU_STATE_EXPECTS_WAKEUP 1 > +/* VCPU is powered off */ > +#define GH_VCPU_STATE_POWERED_OFF 2 > +/* VCPU is blocked in EL2 for unspecified reason */ > +#define GH_VCPU_STATE_BLOCKED 3 > +/* VCPU has returned for MMIO READ */ > +#define GH_VCPU_ADDRSPACE_VMMIO_READ 4 > +/* VCPU has returned for MMIO WRITE */ > +#define GH_VCPU_ADDRSPACE_VMMIO_WRITE 5 The states above are used as values held in the state field of the gh_hypercall_vcpu_run_res structure. I find it confusing that you define them here right below the gh_vcpu structure (which also has a "state" field--even though its values are listed above). Perhaps these values should be defined in <linux/gunyah.h> instead, where th_hypercall_vcpu_run_resp is defined. I realize that even if you did that, they'd only be used in this file. > + > +static void vcpu_release(struct kref *kref) > +{ > + struct gh_vcpu *vcpu = container_of(kref, struct gh_vcpu, kref); > + > + free_page((unsigned long)vcpu->vcpu_run); > + kfree(vcpu); > +} > + > +/* > + * When hypervisor allows us to schedule vCPU again, it gives us an interrupt > + */ > +static irqreturn_t gh_vcpu_irq_handler(int irq, void *data) > +{ > + struct gh_vcpu *vcpu = data; > + > + complete(&vcpu->ready); > + return IRQ_HANDLED; > +} > + > +static bool gh_handle_mmio(struct gh_vcpu *vcpu, > + struct gh_hypercall_vcpu_run_resp *vcpu_run_resp) > +{ > + int ret = 0; > + u64 addr = vcpu_run_resp->state_data[0], > + len = vcpu_run_resp->state_data[1], > + data = vcpu_run_resp->state_data[2]; > + > + if (vcpu_run_resp->state == GH_VCPU_ADDRSPACE_VMMIO_READ) { > + vcpu->vcpu_run->mmio.is_write = 0; > + /* Record that we need to give vCPU user's supplied value next gh_vcpu_run() */ > + vcpu->state = GH_VCPU_MMIO_READ; > + vcpu->mmio_read_len = len; > + } else { /* GH_VCPU_ADDRSPACE_VMMIO_WRITE */ > + /* Try internal handlers first */ > + ret = gh_vm_mmio_write(vcpu->f->ghvm, addr, len, data); > + if (!ret) > + return true; > + > + /* Give userspace the info */ > + vcpu->vcpu_run->mmio.is_write = 1; > + memcpy(vcpu->vcpu_run->mmio.data, &data, len); > + } > + > + vcpu->vcpu_run->mmio.phys_addr = addr; > + vcpu->vcpu_run->mmio.len = len; > + vcpu->vcpu_run->exit_reason = GH_VCPU_EXIT_MMIO; > + > + return false; > +} > + > +static int gh_vcpu_rm_notification(struct notifier_block *nb, unsigned long action, void *data) > +{ > + struct gh_vcpu *vcpu = container_of(nb, struct gh_vcpu, nb); > + struct gh_rm_vm_exited_payload *exit_payload = data; > + > + if (action == GH_RM_NOTIFICATION_VM_EXITED && > + le16_to_cpu(exit_payload->vmid) == vcpu->ghvm->vmid) > + complete(&vcpu->ready); > + > + return NOTIFY_OK; > +} > + > +static inline enum gh_vm_status remap_vm_status(enum gh_rm_vm_status rm_status) > +{ > + switch (rm_status) { > + case GH_RM_VM_STATUS_INIT_FAILED: > + return GH_VM_STATUS_LOAD_FAILED; > + case GH_RM_VM_STATUS_EXITED: > + return GH_VM_STATUS_EXITED; > + default: > + return GH_VM_STATUS_CRASHED; > + } > +} > + > +/** > + * gh_vcpu_check_system() - Check whether VM as a whole is running > + * @vcpu: Pointer to gh_vcpu > + * > + * Returns true if the VM is alive. > + * Returns false if the vCPU is the VM is not alive (can only be that VM is shutting down). > + */ > +static bool gh_vcpu_check_system(struct gh_vcpu *vcpu) > + __must_hold(&vcpu->run_lock) > +{ > + bool ret = true; > + > + down_read(&vcpu->ghvm->status_lock); > + if (likely(vcpu->ghvm->vm_status == GH_RM_VM_STATUS_RUNNING)) > + goto out; > + > + vcpu->vcpu_run->status.status = remap_vm_status(vcpu->ghvm->vm_status); > + vcpu->vcpu_run->status.exit_info = vcpu->ghvm->exit_info; > + vcpu->vcpu_run->exit_reason = GH_VCPU_EXIT_STATUS; > + vcpu->state = GH_VCPU_SYSTEM_DOWN; > + ret = false; > +out: > + up_read(&vcpu->ghvm->status_lock); > + return ret; > +} > + > +/** > + * gh_vcpu_run() - Request Gunyah to begin scheduling this vCPU. > + * @vcpu: The client descriptor that was obtained via gh_vcpu_alloc() > + */ > +static int gh_vcpu_run(struct gh_vcpu *vcpu) > +{ > + struct gh_hypercall_vcpu_run_resp vcpu_run_resp; > + u64 state_data[3] = { 0 }; > + enum gh_error gh_error; > + int ret = 0; > + > + if (!vcpu->f) > + return -ENODEV; > + > + if (mutex_lock_interruptible(&vcpu->run_lock)) > + return -ERESTARTSYS; > + > + if (!vcpu->rsc) { > + ret = -ENODEV; > + goto out; > + } > + > + switch (vcpu->state) { > + case GH_VCPU_UNKNOWN: > + if (vcpu->ghvm->vm_status != GH_RM_VM_STATUS_RUNNING) { > + /* Check if VM is up. If VM is starting, will block until VM is fully up > + * since that thread does down_write. > + */ > + if (!gh_vcpu_check_system(vcpu)) > + goto out; > + } > + vcpu->state = GH_VCPU_READY; > + break; > + case GH_VCPU_MMIO_READ: I think you should verify that vcpu->mmio_read_len is <= 8 bytes (or sizeof(state_data[0]). It is set in gh_handle_mmio(), and *should* be correct, but in isolation here it would be defensive. > + memcpy(&state_data[0], vcpu->vcpu_run->mmio.data, vcpu->mmio_read_len); > + vcpu->state = GH_VCPU_READY; > + break; > + case GH_VCPU_SYSTEM_DOWN: > + goto out; > + default: > + break; > + } > + > + while (!ret && !signal_pending(current)) { > + if (vcpu->vcpu_run->immediate_exit) { > + ret = -EINTR; > + goto out; > + } > + > + gh_error = gh_hypercall_vcpu_run(vcpu->rsc->capid, state_data, &vcpu_run_resp); > + if (gh_error == GH_ERROR_OK) { > + ret = 0; > + switch (vcpu_run_resp.state) { > + case GH_VCPU_STATE_READY: > + if (need_resched()) > + schedule(); > + break; > + case GH_VCPU_STATE_POWERED_OFF: > + /* vcpu might be off because the VM is shut down. > + * If so, it won't ever run again: exit back to user > + */ > + if (!gh_vcpu_check_system(vcpu)) > + goto out; > + /* Otherwise, another vcpu will turn it on (e.g. by PSCI) > + * and hyp sends an interrupt to wake Linux up. > + */ > + fallthrough; > + case GH_VCPU_STATE_EXPECTS_WAKEUP: > + ret = wait_for_completion_interruptible(&vcpu->ready); > + /* reinitialize completion before next hypercall. If we reinitialize > + * after the hypercall, interrupt may have already come before > + * re-initializing the completion and then end up waiting for > + * event that already happened. > + */ > + reinit_completion(&vcpu->ready); > + /* Check system status again. Completion might've > + * come from gh_vcpu_rm_notification > + */ > + if (!ret && !gh_vcpu_check_system(vcpu)) > + goto out; > + break; > + case GH_VCPU_STATE_BLOCKED: > + schedule(); > + break; > + case GH_VCPU_ADDRSPACE_VMMIO_READ: > + case GH_VCPU_ADDRSPACE_VMMIO_WRITE: > + if (!gh_handle_mmio(vcpu, &vcpu_run_resp)) > + goto out; > + break; > + default: > + pr_warn_ratelimited("Unknown vCPU state: %llx\n", > + vcpu_run_resp.state); > + schedule(); > + break; > + } > + } else if (gh_error == GH_ERROR_RETRY) { > + schedule(); > + ret = 0; I don't think assigning ret here is necessary. Add curly braces for the block below. > + } else > + ret = gh_remap_error(gh_error); > + } > + > +out: > + mutex_unlock(&vcpu->run_lock); > + > + if (signal_pending(current)) > + return -ERESTARTSYS; > + > + return ret; > +} > + > +static long gh_vcpu_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + struct gh_vcpu *vcpu = filp->private_data; > + long ret = -EINVAL; > + > + switch (cmd) { > + case GH_VCPU_RUN: > + ret = gh_vcpu_run(vcpu); > + break; > + case GH_VCPU_MMAP_SIZE: > + ret = PAGE_SIZE; > + break; > + default: > + break; > + } > + return ret; > +} > + > +static int gh_vcpu_release(struct inode *inode, struct file *filp) > +{ > + struct gh_vcpu *vcpu = filp->private_data; > + > + gh_vm_put(vcpu->ghvm); > + kref_put(&vcpu->kref, vcpu_release); > + return 0; > +} > + > +static vm_fault_t gh_vcpu_fault(struct vm_fault *vmf) > +{ > + struct gh_vcpu *vcpu = vmf->vma->vm_file->private_data; > + struct page *page = NULL; > + > + if (vmf->pgoff == 0) > + page = virt_to_page(vcpu->vcpu_run); > + > + get_page(page); > + vmf->page = page; > + return 0; > +} > + > +static const struct vm_operations_struct gh_vcpu_ops = { > + .fault = gh_vcpu_fault, > +}; > + > +static int gh_vcpu_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + vma->vm_ops = &gh_vcpu_ops; > + return 0; > +} > + > +static const struct file_operations gh_vcpu_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = gh_vcpu_ioctl, > + .release = gh_vcpu_release, > + .llseek = noop_llseek, > + .mmap = gh_vcpu_mmap, > +}; > + > +static int gh_vcpu_populate(struct gh_vm_resource_ticket *ticket, struct gh_resource *ghrsc) > +{ > + struct gh_vcpu *vcpu = container_of(ticket, struct gh_vcpu, ticket); > + int ret; > + > + mutex_lock(&vcpu->run_lock); > + if (vcpu->rsc) { > + ret = -1; I think this should be -EBUSY, or (as I mention elsewhere) this function could return Boolean. > + goto out; > + } > + > + vcpu->rsc = ghrsc; > + init_completion(&vcpu->ready); > + > + ret = request_irq(vcpu->rsc->irq, gh_vcpu_irq_handler, IRQF_TRIGGER_RISING, "gh_vcpu", > + vcpu); > + if (ret) > + pr_warn("Failed to request vcpu irq %d: %d", vcpu->rsc->irq, ret); > + > +out: > + mutex_unlock(&vcpu->run_lock); > + return ret; > +} > + > +static void gh_vcpu_unpopulate(struct gh_vm_resource_ticket *ticket, > + struct gh_resource *ghrsc) > +{ > + struct gh_vcpu *vcpu = container_of(ticket, struct gh_vcpu, ticket); > + > + vcpu->vcpu_run->immediate_exit = true; > + complete_all(&vcpu->ready); > + mutex_lock(&vcpu->run_lock); > + free_irq(vcpu->rsc->irq, vcpu); > + vcpu->rsc = NULL; > + mutex_unlock(&vcpu->run_lock); > +} > + > +static long gh_vcpu_bind(struct gh_vm_function_instance *f) > +{ > + struct gh_fn_vcpu_arg *arg = f->argp; > + struct gh_vcpu *vcpu; > + char name[MAX_VCPU_NAME]; > + struct file *file; > + struct page *page; > + int fd; > + long r; > + > + if (!gh_api_has_feature(GH_FEATURE_VCPU)) > + return -EOPNOTSUPP; > + > + if (f->arg_size != sizeof(*arg)) > + return -EINVAL; > + > + vcpu = kzalloc(sizeof(*vcpu), GFP_KERNEL); > + if (!vcpu) > + return -ENOMEM; > + > + vcpu->f = f; > + f->data = vcpu; > + mutex_init(&vcpu->run_lock); > + kref_init(&vcpu->kref); > + > + page = alloc_page(GFP_KERNEL | __GFP_ZERO); > + if (!page) { > + r = -ENOMEM; > + goto err_destroy_vcpu; > + } > + vcpu->vcpu_run = page_address(page); > + > + vcpu->ticket.resource_type = GH_RESOURCE_TYPE_VCPU; > + vcpu->ticket.label = arg->id; > + vcpu->ticket.owner = THIS_MODULE; > + vcpu->ticket.populate = gh_vcpu_populate; > + vcpu->ticket.unpopulate = gh_vcpu_unpopulate; > + > + r = gh_vm_add_resource_ticket(f->ghvm, &vcpu->ticket); > + if (r) > + goto err_destroy_page; > + > + fd = get_unused_fd_flags(O_CLOEXEC); > + if (fd < 0) { > + r = fd; > + goto err_remove_vcpu; > + } > + > + if (!gh_vm_get(f->ghvm)) { > + r = -ENODEV; > + goto err_put_fd; > + } > + vcpu->ghvm = f->ghvm; > + > + vcpu->nb.notifier_call = gh_vcpu_rm_notification; > + /* Ensure we run after the vm_mgr handles the notification and does > + * any necessary state changes. We wake up to check the new state. > + */ > + vcpu->nb.priority = -1; > + r = gh_rm_notifier_register(f->rm, &vcpu->nb); > + if (r) > + goto err_put_gh_vm; > + > + kref_get(&vcpu->kref); > + snprintf(name, sizeof(name), "gh-vcpu:%d", vcpu->ticket.label); s/%d/%u/ > + file = anon_inode_getfile(name, &gh_vcpu_fops, vcpu, O_RDWR); > + if (IS_ERR(file)) { > + r = PTR_ERR(file); > + goto err_notifier; > + } Maybe group getting the anonymous file with getting an unused file descriptor. > + > + fd_install(fd, file); > + > + return fd; > +err_notifier: > + gh_rm_notifier_unregister(f->rm, &vcpu->nb); > +err_put_gh_vm: > + gh_vm_put(vcpu->ghvm); > +err_put_fd: > + put_unused_fd(fd); > +err_remove_vcpu: > + gh_vm_remove_resource_ticket(f->ghvm, &vcpu->ticket); > +err_destroy_page: > + free_page((unsigned long)vcpu->vcpu_run); > +err_destroy_vcpu: > + kfree(vcpu); > + return r; > +} > + > +static void gh_vcpu_unbind(struct gh_vm_function_instance *f) > +{ > + struct gh_vcpu *vcpu = f->data; > + > + gh_rm_notifier_unregister(f->rm, &vcpu->nb); > + gh_vm_remove_resource_ticket(vcpu->f->ghvm, &vcpu->ticket); > + vcpu->f = NULL; > + > + kref_put(&vcpu->kref, vcpu_release); > +} > + > +DECLARE_GH_VM_FUNCTION_INIT(vcpu, GH_FN_VCPU, gh_vcpu_bind, gh_vcpu_unbind); > +MODULE_DESCRIPTION("Gunyah vCPU Driver"); Maybe "Gunyah vCPU VM function(s)"? And if you use "Driver" (or "driver") here, consider using it for the irqfd and ioeventfd module descriptions as well. > +MODULE_LICENSE("GPL"); > diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c > index b31fac15ff45..d453d902847e 100644 > --- a/drivers/virt/gunyah/vm_mgr.c > +++ b/drivers/virt/gunyah/vm_mgr.c > @@ -354,6 +354,10 @@ static int gh_vm_rm_notification_exited(struct gh_vm *ghvm, void *data) > > down_write(&ghvm->status_lock); > ghvm->vm_status = GH_RM_VM_STATUS_EXITED; > + ghvm->exit_info.type = le16_to_cpu(payload->exit_type); > + ghvm->exit_info.reason_size = le32_to_cpu(payload->exit_reason_size); > + memcpy(&ghvm->exit_info.reason, payload->exit_reason, > + min(GH_VM_MAX_EXIT_REASON_SIZE, ghvm->exit_info.reason_size)); > up_write(&ghvm->status_lock); > > return NOTIFY_DONE; > diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h > index 9c1046af80ed..df78756639b6 100644 > --- a/drivers/virt/gunyah/vm_mgr.h > +++ b/drivers/virt/gunyah/vm_mgr.h > @@ -45,6 +45,7 @@ struct gh_vm { > enum gh_rm_vm_status vm_status; > wait_queue_head_t vm_status_wait; > struct rw_semaphore status_lock; > + struct gh_vm_exit_info exit_info; > > struct work_struct free_work; > struct kref kref; > diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h > index 3e706b59d2c0..37f1e2c822ce 100644 > --- a/include/linux/gunyah.h > +++ b/include/linux/gunyah.h > @@ -175,4 +175,12 @@ enum gh_error gh_hypercall_msgq_send(u64 capid, size_t size, void *buff, int tx_ > enum gh_error gh_hypercall_msgq_recv(u64 capid, void *buff, size_t size, size_t *recv_size, > bool *ready); > > +struct gh_hypercall_vcpu_run_resp { > + u64 state; > + u64 state_data[3]; > +}; > + > +enum gh_error gh_hypercall_vcpu_run(u64 capid, u64 *resume_data, > + struct gh_hypercall_vcpu_run_resp *resp); > + > #endif > diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h > index caeb3b3a3e9a..e52265fa5715 100644 > --- a/include/uapi/linux/gunyah.h > +++ b/include/uapi/linux/gunyah.h > @@ -62,8 +62,32 @@ struct gh_vm_dtb_config { > > #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3) > > +/** > + * GH_FN_VCPU - create a vCPU instance to control a vCPU > + * > + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg > + * > + * The vcpu type will register with the VM Manager to expect to control > + * vCPU number `vcpu_id`. It returns a file descriptor allowing interaction with > + * the vCPU. See the Gunyah vCPU API description sections for interacting with > + * the Gunyah vCPU file descriptors. > + * > + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* ioctls > + */ > +#define GH_FN_VCPU 1 I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD in an enumerated type. Each has a type associated with it, and you can add the explanation for the function in the kernel-doc comments above thosse type definitions. > + > #define GH_FN_MAX_ARG_SIZE 256 > > +/** > + * struct gh_fn_vcpu_arg - Arguments to create a vCPU > + * @id: vcpu id > + */ > +struct gh_fn_vcpu_arg { > + __u32 id; I realize this is the "CPU ID" but the other two function types name this field "label" and it gets used as the label for the ticket when it gets bound. So I suggest naming this "label". > +}; > + > +#define GH_IRQFD_LEVEL (1UL << 0) > + > /** > * struct gh_fn_desc - Arguments to create a VM function > * @type: Type of the function. See GH_FN_* macro for supported types If you do what I suggest, the above comment should instead refer to the enumerated type name. > @@ -79,4 +103,88 @@ struct gh_fn_desc { > #define GH_VM_ADD_FUNCTION _IOW(GH_IOCTL_TYPE, 0x4, struct gh_fn_desc) > #define GH_VM_REMOVE_FUNCTION _IOW(GH_IOCTL_TYPE, 0x7, struct gh_fn_desc) > > +enum gh_vm_status { > + GH_VM_STATUS_LOAD_FAILED = 1, > +#define GH_VM_STATUS_LOAD_FAILED GH_VM_STATUS_LOAD_FAILED > + GH_VM_STATUS_EXITED = 2, > +#define GH_VM_STATUS_EXITED GH_VM_STATUS_EXITED > + GH_VM_STATUS_CRASHED = 3, > +#define GH_VM_STATUS_CRASHED GH_VM_STATUS_CRASHED > +}; > + > +/* > + * Gunyah presently sends max 4 bytes of exit_reason. > + * If that changes, this macro can be safely increased without breaking > + * userspace so long as struct gh_vcpu_run < PAGE_SIZE. Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you expect this driver to work properly if the page size were configured to be 16384 bytes? In other words, is this a Gunyah constant, or is it *really* the page size configured for Linux? > + */ > +#define GH_VM_MAX_EXIT_REASON_SIZE 8u > + > +/** > + * struct gh_vm_exit_info - Reason for VM exit as reported by Gunyah > + * See Gunyah documentation for values. > + * @type: Describes how VM exited > + * @padding: padding bytes > + * @reason_size: Number of bytes valid for `reason` > + * @reason: See Gunyah documentation for interpretation. Note: these values are > + * not interpreted by Linux and need to be converted from little-endian > + * as applicable. > + */ > +struct gh_vm_exit_info { > + __u16 type; > + __u16 padding; > + __u32 reason_size; > + __u8 reason[GH_VM_MAX_EXIT_REASON_SIZE]; > +}; > + Define this group of values in an enumerated type. Are these the possible "exit_reason" values? If so, maybe name them GH_VCPU_EXIT_REASON_*. > +#define GH_VCPU_EXIT_UNKNOWN 0 > +#define GH_VCPU_EXIT_MMIO 1 > +#define GH_VCPU_EXIT_STATUS 2 > + > +/** > + * struct gh_vcpu_run - Application code obtains a pointer to the gh_vcpu_run > + * structure by mmap()ing a vcpu fd. > + * @immediate_exit: polled when scheduling the vcpu. If set, immediately returns -EINTR. > + * @padding: padding bytes > + * @exit_reason: Set when GH_VCPU_RUN returns successfully and gives reason why > + * GH_VCPU_RUN has stopped running the vCPU. > + * @mmio: Used when exit_reason == GH_VCPU_EXIT_MMIO > + * The guest has faulted on an memory-mapped I/O instruction that > + * couldn't be satisfied by gunyah. > + * @mmio.phys_addr: Address guest tried to access > + * @mmio.data: the value that was written if `is_write == 1`. Filled by > + * user for reads (`is_wite == 0`). > + * @mmio.len: Length of write. Only the first `len` bytes of `data` > + * are considered by Gunyah. > + * @mmio.is_write: 1 if VM tried to perform a write, 0 for a read > + * @status: Used when exit_reason == GH_VCPU_EXIT_STATUS. > + * The guest VM is no longer runnable. This struct informs why. > + * @status.status: See `enum gh_vm_status` for possible values > + * @status.exit_info: Used when status == GH_VM_STATUS_EXITED > + */ > +struct gh_vcpu_run { > + /* in */ > + __u8 immediate_exit; > + __u8 padding[7]; > + > + /* out */ > + __u32 exit_reason; > + > + union { > + struct { > + __u64 phys_addr; > + __u8 data[8]; > + __u32 len; > + __u8 is_write; > + } mmio; > + > + struct { > + enum gh_vm_status status; > + struct gh_vm_exit_info exit_info; > + } status; > + }; > +}; > + > +#define GH_VCPU_RUN _IO(GH_IOCTL_TYPE, 0x5) > +#define GH_VCPU_MMAP_SIZE _IO(GH_IOCTL_TYPE, 0x6) > + > #endif
On 3/31/2023 7:25 AM, Alex Elder wrote: > On 3/3/23 7:06 PM, Elliot Berman wrote: >> @@ -129,6 +131,7 @@ struct gh_rm_connection { >> * @cache: cache for allocating Tx messages >> * @send_lock: synchronization to allow only one request to be sent >> at a time >> * @nh: notifier chain for clients interested in RM notification >> messages >> + * @miscdev: /dev/gunyah >> */ >> struct gh_rm { >> struct device *dev; >> @@ -145,6 +148,8 @@ struct gh_rm { >> struct kmem_cache *cache; >> struct mutex send_lock; >> struct blocking_notifier_head nh; >> + >> + struct miscdevice miscdev; >> }; >> /** >> @@ -593,6 +598,21 @@ void gh_rm_put(struct gh_rm *rm) >> } >> EXPORT_SYMBOL_GPL(gh_rm_put); > > I feel like /dev/gunyah code would more appropriately be found > in "vm_mgr.c". All gh_dev_ioctl() does is call the function > defined there, and it's therefore a VM-oriented rather than > resource-oriented device. I'd like to keep the gh_dev_ioctl where it is because it keeps the struct gh_rm explicitly private to rsc_mgr.c and thinking this helps keep the design cleaner long term by preventing new members from sneaking into struct gh_rm. >> + >> +static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg) >> +{ >> + struct gh_vm *ghvm; >> + struct file *file; >> + int fd, err; >> + >> + /* arg reserved for future use. */ > > Do you have a clear idea of how this might be used in the future? Not yet. I have some vague ideas to use it as a enumeration of "special" VM types. We might have special number for VMs which use "protected VM firmware" for the Android boot flow, another number for the "Trusted UI VM", another for "OEM VM", etc. Passing 0 would always be the unauthenticated VM which we are creating today. We're considering bumping the info to a separate ioctl since additional info needs to be passed from userspace to configure the VM. Userspace would do GH_CREATE_VM(). Another ioctl like GH_VM_SET_PVMFW_ADDRESS() would imply that the VM uses the protected VM firmware for the Android boot flow. Another ioctl call would be used to imply the "Trusted UI VM". In any case, we're still in early design phase. > > I was thinking you could silently ignore the argument value, but > I suppose if it *does* get used in the future, you want the caller > to know it's being ignored. (Is that right?) > That's right. Thanks, Elliot
On 3/31/2023 7:27 AM, Alex Elder wrote: > On 3/3/23 7:06 PM, Elliot Berman wrote: [snip] >> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h >> index caeb3b3a3e9a..e52265fa5715 100644 >> --- a/include/uapi/linux/gunyah.h >> +++ b/include/uapi/linux/gunyah.h >> @@ -62,8 +62,32 @@ struct gh_vm_dtb_config { >> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3) >> +/** >> + * GH_FN_VCPU - create a vCPU instance to control a vCPU >> + * >> + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg >> + * >> + * The vcpu type will register with the VM Manager to expect to control >> + * vCPU number `vcpu_id`. It returns a file descriptor allowing >> interaction with >> + * the vCPU. See the Gunyah vCPU API description sections for >> interacting with >> + * the Gunyah vCPU file descriptors. >> + * >> + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* ioctls >> + */ >> +#define GH_FN_VCPU 1 > > I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD > in an enumerated type. Each has a type associated with it, and you can > add the explanation for the function in the kernel-doc comments above > thosse type definitions. > I'd like to enumify the GH_FN_* macros, but one challenge I'm facing is that it breaks the module alias implementation in patch 19. MODULE_ALIAS("ghfunc:"__stringify(_type)) When the GH_FN_* are regular preprocessor macros backed by an integer, the preprocessor will make the module alias ghfunc:0 (or ghfunc:1, etc). This works well because I can do request_module("ghfunc:%d", type); If the function hasn't been registered and then gunyah_vcpu.ko gets loaded automatically. With enum, compiler knows the value of GH_FN_VCPU and preprocessor will make the module alias like ghfunc:GH_FN_VCPU. [snip] >> + >> +/* >> + * Gunyah presently sends max 4 bytes of exit_reason. >> + * If that changes, this macro can be safely increased without breaking >> + * userspace so long as struct gh_vcpu_run < PAGE_SIZE. > > Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you > expect this driver to work properly if the page size were configured > to be 16384 bytes? In other words, is this a Gunyah constant, or > is it *really* the page size configured for Linux? > Our implementations are only doing 4096 bytes. I expect the driver to work properly when using 16k pages. This really is a Linux page. It's a reflection of the alloc_page in gh_vcpu_bind(). The exit reason is copied from hypervisor into field accessible by userspace directly. Gunyah makes the exit reason size dynamic -- there's no architectural limitation preventing the exit reason from being a string or some lengthy data. As I was writing this response, I realized that I should be able to make this a zero-length array and ensure that reason[] doesn't overflow PAGE_SIZE... The comment was trying to explain that Linux itself imposes a limitation on the maximum exit reason size. If we need to support longer exit reason, we're OK to do so long as the total size doesn't overrun PAGE_SIZE. There aren't any plans to need longer exit reasons than the 8 bytes mentioned today. Thanks, Elliot
On 3/31/2023 7:26 AM, Alex Elder wrote: > On 3/3/23 7:06 PM, Elliot Berman wrote: >> When booting a Gunyah virtual machine, the host VM may gain capabilities >> to interact with resources for the guest virtual machine. Examples of >> such resources are vCPUs or message queues. To use those resources, we >> need to translate the RM response into a gunyah_resource structure which >> are useful to Linux drivers. Presently, Linux drivers need only to know >> the type of resource, the capability ID, and an interrupt. >> >> On ARM64 systems, the interrupt reported by Gunyah is the GIC interrupt >> ID number and always a SPI. >> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > Several comments here, nothing major. -Alex > >> --- >> arch/arm64/include/asm/gunyah.h | 23 +++++ >> drivers/virt/gunyah/rsc_mgr.c | 163 +++++++++++++++++++++++++++++++- >> include/linux/gunyah.h | 4 + >> include/linux/gunyah_rsc_mgr.h | 3 + >> 4 files changed, 192 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm64/include/asm/gunyah.h >> >> diff --git a/arch/arm64/include/asm/gunyah.h >> b/arch/arm64/include/asm/gunyah.h >> new file mode 100644 >> index 000000000000..64cfb964efee >> --- /dev/null >> +++ b/arch/arm64/include/asm/gunyah.h >> @@ -0,0 +1,23 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights >> reserved. >> + */ >> +#ifndef __ASM_GUNYAH_H_ >> +#define __ASM_GUNYAH_H_ > > Maybe just one _ at the beginning and none at the end? > Follow the same convention across all your header files. > (Maybe you're looking at other files in the same directory > as this one, but that's not consistent.) > >> + >> +#include <linux/irq.h> >> +#include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> +static inline int arch_gh_fill_irq_fwspec_params(u32 virq, struct >> irq_fwspec *fwspec) >> +{ >> + if (virq < 32 || virq > 1019) >> + return -EINVAL; > > What is special about VIRQs greater than 1019 (minus 32)? > > It's probably documented somewhere but it's worth adding a > comment here to explain the check. > > You would know better than I, but could/should the caller > be responsible for this check? (Not a big deal.) > I think definitely not the caller should be responsible for this check. On arm systems, the IRQ # that is returned is the hwirq # for the GIC. Presently, Gunyah only gives us SPI interrupts [32,1019] so I've written a translation to a SPI fwspec. >> + >> + fwspec->param_count = 3; >> + fwspec->param[0] = GIC_SPI; >> + fwspec->param[1] = virq - 32; > > And why is 32 subtracted? > GIC driver expects the SPI #, not the hwirq #. >> + fwspec->param[2] = IRQ_TYPE_EDGE_RISING; >> + return 0; >> +} >> + >> +#endif >> diff --git a/drivers/virt/gunyah/rsc_mgr.c >> b/drivers/virt/gunyah/rsc_mgr.c >> index d7ce692d0067..383be5ac0f44 100644 >> --- a/drivers/virt/gunyah/rsc_mgr.c >> +++ b/drivers/virt/gunyah/rsc_mgr.c >> @@ -17,6 +17,8 @@ >> #include <linux/platform_device.h> >> #include <linux/miscdevice.h> >> +#include <asm/gunyah.h> >> + >> #include "rsc_mgr.h" >> #include "vm_mgr.h" >> @@ -132,6 +134,7 @@ struct gh_rm_connection { >> * @send_lock: synchronization to allow only one request to be sent >> at a time >> * @nh: notifier chain for clients interested in RM notification >> messages >> * @miscdev: /dev/gunyah >> + * @irq_domain: Domain to translate Gunyah hwirqs to Linux irqs >> */ >> struct gh_rm { >> struct device *dev; >> @@ -150,6 +153,7 @@ struct gh_rm { >> struct blocking_notifier_head nh; >> struct miscdevice miscdev; >> + struct irq_domain *irq_domain; >> }; >> /** >> @@ -190,6 +194,134 @@ static inline int gh_rm_remap_error(enum >> gh_rm_error rm_error) >> } >> } >> +struct gh_irq_chip_data { >> + u32 gh_virq; >> +}; >> + >> +static struct irq_chip gh_rm_irq_chip = { >> + .name = "Gunyah", >> + .irq_enable = irq_chip_enable_parent, >> + .irq_disable = irq_chip_disable_parent, >> + .irq_ack = irq_chip_ack_parent, >> + .irq_mask = irq_chip_mask_parent, >> + .irq_mask_ack = irq_chip_mask_ack_parent, >> + .irq_unmask = irq_chip_unmask_parent, >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> + .irq_set_type = irq_chip_set_type_parent, >> + .irq_set_wake = irq_chip_set_wake_parent, >> + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, >> + .irq_retrigger = irq_chip_retrigger_hierarchy, >> + .irq_get_irqchip_state = irq_chip_get_parent_state, >> + .irq_set_irqchip_state = irq_chip_set_parent_state, >> + .flags = IRQCHIP_SET_TYPE_MASKED | >> + IRQCHIP_SKIP_SET_WAKE | >> + IRQCHIP_MASK_ON_SUSPEND, >> +}; >> + >> +static int gh_rm_irq_domain_alloc(struct irq_domain *d, unsigned int >> virq, unsigned int nr_irqs, >> + void *arg) >> +{ >> + struct gh_irq_chip_data *chip_data, *spec = arg; >> + struct irq_fwspec parent_fwspec; >> + struct gh_rm *rm = d->host_data; >> + u32 gh_virq = spec->gh_virq; >> + int ret; >> + >> + if (nr_irqs != 1 || gh_virq == U32_MAX) > > Does U32_MAX have special meaning? Why are you checking for it? > Whatever it is, you should explain why this is invalid here. > This was holdover from deprecated Gunyah code. Since there are new features/version checks it's not possible for Linux to encounter these values. I've dropped it in v12. Thanks, Elliot
On 4/17/23 5:41 PM, Elliot Berman wrote: > > > On 3/31/2023 7:27 AM, Alex Elder wrote: >> On 3/3/23 7:06 PM, Elliot Berman wrote: > > [snip] > >>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h >>> index caeb3b3a3e9a..e52265fa5715 100644 >>> --- a/include/uapi/linux/gunyah.h >>> +++ b/include/uapi/linux/gunyah.h >>> @@ -62,8 +62,32 @@ struct gh_vm_dtb_config { >>> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3) >>> +/** >>> + * GH_FN_VCPU - create a vCPU instance to control a vCPU >>> + * >>> + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg >>> + * >>> + * The vcpu type will register with the VM Manager to expect to control >>> + * vCPU number `vcpu_id`. It returns a file descriptor allowing >>> interaction with >>> + * the vCPU. See the Gunyah vCPU API description sections for >>> interacting with >>> + * the Gunyah vCPU file descriptors. >>> + * >>> + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* ioctls >>> + */ >>> +#define GH_FN_VCPU 1 >> >> I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD >> in an enumerated type. Each has a type associated with it, and you can >> add the explanation for the function in the kernel-doc comments above >> thosse type definitions. >> > > I'd like to enumify the GH_FN_* macros, but one challenge I'm facing is > that it breaks the module alias implementation in patch 19. > > MODULE_ALIAS("ghfunc:"__stringify(_type)) > > When the GH_FN_* are regular preprocessor macros backed by an integer, > the preprocessor will make the module alias ghfunc:0 (or ghfunc:1, etc). > This works well because I can do > > request_module("ghfunc:%d", type); > > If the function hasn't been registered and then gunyah_vcpu.ko gets > loaded automatically. > > With enum, compiler knows the value of GH_FN_VCPU and preprocessor will > make the module alias like ghfunc:GH_FN_VCPU. > > [snip] > >>> + >>> +/* >>> + * Gunyah presently sends max 4 bytes of exit_reason. >>> + * If that changes, this macro can be safely increased without breaking >>> + * userspace so long as struct gh_vcpu_run < PAGE_SIZE. >> >> Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you >> expect this driver to work properly if the page size were configured >> to be 16384 bytes? In other words, is this a Gunyah constant, or >> is it *really* the page size configured for Linux? >> > > Our implementations are only doing 4096 bytes. I expect the driver to > work properly when using 16k pages. This really is a Linux page. It's a > reflection of the alloc_page in gh_vcpu_bind(). OK. I guess I'd be on the lookout for anything that uses 4096 when PAGE_SIZE is what's actually meant. I have no idea what's involved with the hypervisor if you wanted to try something else, but if you haven't tested that, you could maybe do an early check in your probe function: BUILD_BUG_ON(PAGE_SIZE != 4096); > The exit reason is copied from hypervisor into field accessible by > userspace directly. Gunyah makes the exit reason size dynamic -- there's > no architectural limitation preventing the exit reason from being a > string or some lengthy data. Sounds good. I like having statements like this tested, and maybe you have. I.e., test with the exit_reason size something like 16 bytes and ensure that works. Testing this is not technically needed, but your comment suggests it can be done. > As I was writing this response, I realized that I should be able to make > this a zero-length array and ensure that reason[] doesn't overflow > PAGE_SIZE... Maybe some good came out of it? > The comment was trying to explain that Linux itself imposes a limitation > on the maximum exit reason size. If we need to support longer exit Your comment isn't clear that Linux is what limits the size. This is all kind of picky though. My main point was about the PAGE_SIZE assumption. -Alex > reason, we're OK to do so long as the total size doesn't overrun > PAGE_SIZE. There aren't any plans to need longer exit reasons than the 8 > bytes mentioned today. > > Thanks, > Elliot
On 4/17/2023 3:41 PM, Elliot Berman wrote: > > > On 3/31/2023 7:27 AM, Alex Elder wrote: >> On 3/3/23 7:06 PM, Elliot Berman wrote: > > [snip] > >>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h >>> index caeb3b3a3e9a..e52265fa5715 100644 >>> --- a/include/uapi/linux/gunyah.h >>> +++ b/include/uapi/linux/gunyah.h >>> @@ -62,8 +62,32 @@ struct gh_vm_dtb_config { >>> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3) >>> +/** >>> + * GH_FN_VCPU - create a vCPU instance to control a vCPU >>> + * >>> + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg >>> + * >>> + * The vcpu type will register with the VM Manager to expect to control >>> + * vCPU number `vcpu_id`. It returns a file descriptor allowing >>> interaction with >>> + * the vCPU. See the Gunyah vCPU API description sections for >>> interacting with >>> + * the Gunyah vCPU file descriptors. >>> + * >>> + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* ioctls >>> + */ >>> +#define GH_FN_VCPU 1 >> >> I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD >> in an enumerated type. Each has a type associated with it, and you can >> add the explanation for the function in the kernel-doc comments above >> thosse type definitions. >> > > I'd like to enumify the GH_FN_* macros, but one challenge I'm facing is > that it breaks the module alias implementation in patch 19. > > MODULE_ALIAS("ghfunc:"__stringify(_type)) > > When the GH_FN_* are regular preprocessor macros backed by an integer, > the preprocessor will make the module alias ghfunc:0 (or ghfunc:1, etc). > This works well because I can do > > request_module("ghfunc:%d", type); > > If the function hasn't been registered and then gunyah_vcpu.ko gets > loaded automatically. > > With enum, compiler knows the value of GH_FN_VCPU and preprocessor will > make the module alias like ghfunc:GH_FN_VCPU. > I still like the idea of having enum for documentation and clarity. I noticed that nfnetlink.h saw the same problem for NFNL_SUBSYS_*. Is this compromise terrible and I should give up on the enum? enum gh_fn_type { /* _GH_FN_* macro required for MODULE_ALIAS, otherwise __stringify() trick * won't work anymore */ #define _GH_FN_VCPU 1 GH_FN_VCPU = _GH_FN_VCPU, #define _GH_FN_IRQFD 2 GH_FN_IRQFD = _GH_FN_IRQFD, #define _GH_FN_IOEVENTFD 3 GH_FN_IOEVENTFD = _GH_FN_IOEVENTFD, }; > [snip] > >>> + >>> +/* >>> + * Gunyah presently sends max 4 bytes of exit_reason. >>> + * If that changes, this macro can be safely increased without breaking >>> + * userspace so long as struct gh_vcpu_run < PAGE_SIZE. >> >> Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you >> expect this driver to work properly if the page size were configured >> to be 16384 bytes? In other words, is this a Gunyah constant, or >> is it *really* the page size configured for Linux? >> > > Our implementations are only doing 4096 bytes. I expect the driver to > work properly when using 16k pages. This really is a Linux page. It's a > reflection of the alloc_page in gh_vcpu_bind(). > > The exit reason is copied from hypervisor into field accessible by > userspace directly. Gunyah makes the exit reason size dynamic -- there's > no architectural limitation preventing the exit reason from being a > string or some lengthy data. > > As I was writing this response, I realized that I should be able to make > this a zero-length array and ensure that reason[] doesn't overflow > PAGE_SIZE... > > The comment was trying to explain that Linux itself imposes a limitation > on the maximum exit reason size. If we need to support longer exit > reason, we're OK to do so long as the total size doesn't overrun > PAGE_SIZE. There aren't any plans to need longer exit reasons than the 8 > bytes mentioned today. > > Thanks, > Elliot
On 4/18/23 12:18 PM, Elliot Berman wrote: > > > On 4/17/2023 3:41 PM, Elliot Berman wrote: >> >> >> On 3/31/2023 7:27 AM, Alex Elder wrote: >>> On 3/3/23 7:06 PM, Elliot Berman wrote: >> >> [snip] >> >>>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h >>>> index caeb3b3a3e9a..e52265fa5715 100644 >>>> --- a/include/uapi/linux/gunyah.h >>>> +++ b/include/uapi/linux/gunyah.h >>>> @@ -62,8 +62,32 @@ struct gh_vm_dtb_config { >>>> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3) >>>> +/** >>>> + * GH_FN_VCPU - create a vCPU instance to control a vCPU >>>> + * >>>> + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg >>>> + * >>>> + * The vcpu type will register with the VM Manager to expect to >>>> control >>>> + * vCPU number `vcpu_id`. It returns a file descriptor allowing >>>> interaction with >>>> + * the vCPU. See the Gunyah vCPU API description sections for >>>> interacting with >>>> + * the Gunyah vCPU file descriptors. >>>> + * >>>> + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* >>>> ioctls >>>> + */ >>>> +#define GH_FN_VCPU 1 >>> >>> I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD >>> in an enumerated type. Each has a type associated with it, and you can >>> add the explanation for the function in the kernel-doc comments above >>> thosse type definitions. >>> >> >> I'd like to enumify the GH_FN_* macros, but one challenge I'm facing >> is that it breaks the module alias implementation in patch 19. >> >> MODULE_ALIAS("ghfunc:"__stringify(_type)) >> >> When the GH_FN_* are regular preprocessor macros backed by an integer, >> the preprocessor will make the module alias ghfunc:0 (or ghfunc:1, >> etc). This works well because I can do >> >> request_module("ghfunc:%d", type); >> >> If the function hasn't been registered and then gunyah_vcpu.ko gets >> loaded automatically. >> >> With enum, compiler knows the value of GH_FN_VCPU and preprocessor >> will make the module alias like ghfunc:GH_FN_VCPU. >> > > I still like the idea of having enum for documentation and clarity. I > noticed that nfnetlink.h saw the same problem for NFNL_SUBSYS_*. > > Is this compromise terrible and I should give up on the enum? You know, I've seen this pattern in the kernel and never thought too much about why it was done. Maybe this is exactly the reason. It sure *seems* like there might be some macro magic that might cause the enum symbol's numeric value to be used but I think the problem is that enums are C tokens, which are not evaluated at preprocessor time. You could probably skip the leading underscore, and do this as it's done for nfnetlink_groups in that same header file. Maybe somebody else can confirm, or has a better suggestion. -Alex > enum gh_fn_type { > /* _GH_FN_* macro required for MODULE_ALIAS, otherwise __stringify() trick > * won't work anymore */ > #define _GH_FN_VCPU 1 > GH_FN_VCPU = _GH_FN_VCPU, > #define _GH_FN_IRQFD 2 > GH_FN_IRQFD = _GH_FN_IRQFD, > #define _GH_FN_IOEVENTFD 3 > GH_FN_IOEVENTFD = _GH_FN_IOEVENTFD, > }; > >> [snip] >> >>>> + >>>> +/* >>>> + * Gunyah presently sends max 4 bytes of exit_reason. >>>> + * If that changes, this macro can be safely increased without >>>> breaking >>>> + * userspace so long as struct gh_vcpu_run < PAGE_SIZE. >>> >>> Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you >>> expect this driver to work properly if the page size were configured >>> to be 16384 bytes? In other words, is this a Gunyah constant, or >>> is it *really* the page size configured for Linux? >>> >> >> Our implementations are only doing 4096 bytes. I expect the driver to >> work properly when using 16k pages. This really is a Linux page. It's >> a reflection of the alloc_page in gh_vcpu_bind(). >> >> The exit reason is copied from hypervisor into field accessible by >> userspace directly. Gunyah makes the exit reason size dynamic -- >> there's no architectural limitation preventing the exit reason from >> being a string or some lengthy data. >> >> As I was writing this response, I realized that I should be able to >> make this a zero-length array and ensure that reason[] doesn't >> overflow PAGE_SIZE... >> >> The comment was trying to explain that Linux itself imposes a >> limitation on the maximum exit reason size. If we need to support >> longer exit reason, we're OK to do so long as the total size doesn't >> overrun PAGE_SIZE. There aren't any plans to need longer exit reasons >> than the 8 bytes mentioned today. >> >> Thanks, >> Elliot
On 4/18/2023 10:31 AM, Alex Elder wrote: > On 4/18/23 12:18 PM, Elliot Berman wrote: >> >> >> On 4/17/2023 3:41 PM, Elliot Berman wrote: >>> >>> >>> On 3/31/2023 7:27 AM, Alex Elder wrote: >>>> On 3/3/23 7:06 PM, Elliot Berman wrote: >>> >>> [snip] >>> >>>>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h >>>>> index caeb3b3a3e9a..e52265fa5715 100644 >>>>> --- a/include/uapi/linux/gunyah.h >>>>> +++ b/include/uapi/linux/gunyah.h >>>>> @@ -62,8 +62,32 @@ struct gh_vm_dtb_config { >>>>> #define GH_VM_START _IO(GH_IOCTL_TYPE, 0x3) >>>>> +/** >>>>> + * GH_FN_VCPU - create a vCPU instance to control a vCPU >>>>> + * >>>>> + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg >>>>> + * >>>>> + * The vcpu type will register with the VM Manager to expect to >>>>> control >>>>> + * vCPU number `vcpu_id`. It returns a file descriptor allowing >>>>> interaction with >>>>> + * the vCPU. See the Gunyah vCPU API description sections for >>>>> interacting with >>>>> + * the Gunyah vCPU file descriptors. >>>>> + * >>>>> + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* >>>>> ioctls >>>>> + */ >>>>> +#define GH_FN_VCPU 1 >>>> >>>> I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD >>>> in an enumerated type. Each has a type associated with it, and you can >>>> add the explanation for the function in the kernel-doc comments above >>>> thosse type definitions. >>>> >>> >>> I'd like to enumify the GH_FN_* macros, but one challenge I'm facing >>> is that it breaks the module alias implementation in patch 19. >>> >>> MODULE_ALIAS("ghfunc:"__stringify(_type)) >>> >>> When the GH_FN_* are regular preprocessor macros backed by an >>> integer, the preprocessor will make the module alias ghfunc:0 (or >>> ghfunc:1, etc). This works well because I can do >>> >>> request_module("ghfunc:%d", type); >>> >>> If the function hasn't been registered and then gunyah_vcpu.ko gets >>> loaded automatically. >>> >>> With enum, compiler knows the value of GH_FN_VCPU and preprocessor >>> will make the module alias like ghfunc:GH_FN_VCPU. >>> >> >> I still like the idea of having enum for documentation and clarity. I >> noticed that nfnetlink.h saw the same problem for NFNL_SUBSYS_*. >> >> Is this compromise terrible and I should give up on the enum? > > You know, I've seen this pattern in the kernel and never thought > too much about why it was done. Maybe this is exactly the reason. > > It sure *seems* like there might be some macro magic that might > cause the enum symbol's numeric value to be used but I think the > problem is that enums are C tokens, which are not evaluated at > preprocessor time. > > You could probably skip the leading underscore, and do this as > it's done for nfnetlink_groups in that same header file. > > Maybe somebody else can confirm, or has a better suggestion. > In the preprocessor macro case, the preprocessor macro GH_FN_VCPU expands to the GH_FN_VCPU enum value and stuck back as if I didn't have the preprocessor macro in first place. I'm not sure why the preprocessor macros are done for nfnetlink_groups. I saw one case where enum kvm_device_type does the same, but that might be done because it was converting preprocessor macro to enum. Just a guess -- maybe the preprocessor macro was preserved to support userspace code doing this? #ifdef KVM_DEV_TYPE_FSL_MPIC_20 ... #endif > -Alex > > >> enum gh_fn_type { >> /* _GH_FN_* macro required for MODULE_ALIAS, otherwise __stringify() >> trick >> * won't work anymore */ >> #define _GH_FN_VCPU 1 >> GH_FN_VCPU = _GH_FN_VCPU, >> #define _GH_FN_IRQFD 2 >> GH_FN_IRQFD = _GH_FN_IRQFD, >> #define _GH_FN_IOEVENTFD 3 >> GH_FN_IOEVENTFD = _GH_FN_IOEVENTFD, >> }; >> >>> [snip] >>> >>>>> + >>>>> +/* >>>>> + * Gunyah presently sends max 4 bytes of exit_reason. >>>>> + * If that changes, this macro can be safely increased without >>>>> breaking >>>>> + * userspace so long as struct gh_vcpu_run < PAGE_SIZE. >>>> >>>> Is PAGE_SIZE allowed to be anything other than 4096 bytes? Do you >>>> expect this driver to work properly if the page size were configured >>>> to be 16384 bytes? In other words, is this a Gunyah constant, or >>>> is it *really* the page size configured for Linux? >>>> >>> >>> Our implementations are only doing 4096 bytes. I expect the driver to >>> work properly when using 16k pages. This really is a Linux page. It's >>> a reflection of the alloc_page in gh_vcpu_bind(). >>> >>> The exit reason is copied from hypervisor into field accessible by >>> userspace directly. Gunyah makes the exit reason size dynamic -- >>> there's no architectural limitation preventing the exit reason from >>> being a string or some lengthy data. >>> >>> As I was writing this response, I realized that I should be able to >>> make this a zero-length array and ensure that reason[] doesn't >>> overflow PAGE_SIZE... >>> >>> The comment was trying to explain that Linux itself imposes a >>> limitation on the maximum exit reason size. If we need to support >>> longer exit reason, we're OK to do so long as the total size doesn't >>> overrun PAGE_SIZE. There aren't any plans to need longer exit reasons >>> than the 8 bytes mentioned today. >>> >>> Thanks, >>> Elliot >
Gunyah is a Type-1 hypervisor independent of any high-level OS kernel, and runs in a higher CPU privilege level. It does not depend on any lower-privileged OS kernel/code for its core functionality. This increases its security and can support a much smaller trusted computing base than a Type-2 hypervisor. Gunyah is an open source hypervisor. The source repo is available at https://github.com/quic/gunyah-hypervisor. The diagram below shows the architecture. :: VM A VM B +-----+ +-----+ | +-----+ +-----+ +-----+ | | | | | | | | | | | EL0 | APP | | APP | | | APP | | APP | | APP | | | | | | | | | | | | +-----+ +-----+ | +-----+ +-----+ +-----+ ---------------------|------------------------- +--------------+ | +----------------------+ | | | | | EL1 | Linux Kernel | | |Linux kernel/Other OS | ... | | | | | +--------------+ | +----------------------+ --------hvc/smc------|------hvc/smc------------ +----------------------------------------+ | | EL2 | Gunyah Hypervisor | | | +----------------------------------------+ Gunyah provides these following features. - Threads and Scheduling: The scheduler schedules virtual CPUs (VCPUs) on physical CPUs and enables time-sharing of the CPUs. - Memory Management: Gunyah tracks memory ownership and use of all memory under its control. Memory partitioning between VMs is a fundamental security feature. - Interrupt Virtualization: All interrupts are handled in the hypervisor and routed to the assigned VM. - Inter-VM Communication: There are several different mechanisms provided for communicating between VMs. - Device Virtualization: Para-virtualization of devices is supported using inter-VM communication. Low level system features and devices such as interrupt controllers are supported with emulation where required. This series adds the basic framework for detecting that Linux is running under Gunyah as a virtual machine, communication with the Gunyah Resource Manager, and a virtual machine manager capable of launching virtual machines. The series relies on two other patches posted separately: - https://lore.kernel.org/all/20230213181832.3489174-1-quic_eberman@quicinc.com/ - https://lore.kernel.org/all/20230213232537.2040976-2-quic_eberman@quicinc.com/ Changes in v11: - Rename struct gh_vm_dtb_config:gpa -> guest_phys_addr & overflow checks for this - More docstrings throughout - Make resp_buf and resp_buf_size optional - Replace deprecated idr with xarray - Refconting on misc device instead of RM's platform device - Renaming variables, structs, etc. from gunyah_ -> gh_ - Drop removal of user mem regions - Drop mem_lend functionality; to converge with restricted_memfd later Changes in v10: https://lore.kernel.org/all/20230214211229.3239350-1-quic_eberman@quicinc.com/ - Fix bisectability (end result of series is same, --fixups applied to wrong commits) - Convert GH_ERROR_* and GH_RM_ERROR_* to enums - Correct race condition between allocating/freeing user memory - Replace offsetof with struct_size - Series-wide renaming of functions to be more consistent - VM shutdown & restart support added in vCPU and VM Manager patches - Convert VM function name (string) to type (number) - Convert VM function argument to value (which could be a pointer) to remove memory wastage for arguments - Remove defensive checks of hypervisor correctness - Clean ups to ioeventfd as suggested by Srivatsa Changes in v9: https://lore.kernel.org/all/20230120224627.4053418-1-quic_eberman@quicinc.com/ - Refactor Gunyah API flags to be exposed as feature flags at kernel level - Move mbox client cleanup into gunyah_msgq_remove() - Simplify gh_rm_call return value and response payload - Missing clean-up/error handling/little endian fixes as suggested by Srivatsa and Alex in v8 series Changes in v8: https://lore.kernel.org/all/20221219225850.2397345-1-quic_eberman@quicinc.com/ - Treat VM manager as a library of RM - Add patches 21-28 as RFC to support proxy-scheduled vCPUs and necessary bits to support virtio from Gunyah userspace Changes in v7: https://lore.kernel.org/all/20221121140009.2353512-1-quic_eberman@quicinc.com/ - Refactor to remove gunyah RM bus - Refactor allow multiple RM device instances - Bump UAPI to start at 0x0 - Refactor QCOM SCM's platform hooks to allow CONFIG_QCOM_SCM=Y/CONFIG_GUNYAH=M combinations Changes in v6: https://lore.kernel.org/all/20221026185846.3983888-1-quic_eberman@quicinc.com/ - *Replace gunyah-console with gunyah VM Manager* - Move include/asm-generic/gunyah.h into include/linux/gunyah.h - s/gunyah_msgq/gh_msgq/ - Minor tweaks and documentation tidying based on comments from Jiri, Greg, Arnd, Dmitry, and Bagas. Changes in v5: https://lore.kernel.org/all/20221011000840.289033-1-quic_eberman@quicinc.com/ - Dropped sysfs nodes - Switch from aux bus to Gunyah RM bus for the subdevices - Cleaning up RM console Changes in v4: https://lore.kernel.org/all/20220928195633.2348848-1-quic_eberman@quicinc.com/ - Tidied up documentation throughout based on questions/feedback received - Switched message queue implementation to use mailboxes - Renamed "gunyah_device" as "gunyah_resource" Changes in v3: https://lore.kernel.org/all/20220811214107.1074343-1-quic_eberman@quicinc.com/ - /Maintained/Supported/ in MAINTAINERS - Tidied up documentation throughout based on questions/feedback received - Moved hypercalls into arch/arm64/gunyah/; following hyper-v's implementation - Drop opaque typedefs - Move sysfs nodes under /sys/hypervisor/gunyah/ - Moved Gunyah console driver to drivers/tty/ - Reworked gh_device design to drop the Gunyah bus. Changes in v2: https://lore.kernel.org/all/20220801211240.597859-1-quic_eberman@quicinc.com/ - DT bindings clean up - Switch hypercalls to follow SMCCC v1: https://lore.kernel.org/all/20220223233729.1571114-1-quic_eberman@quicinc.com/ Elliot Berman (26): docs: gunyah: Introduce Gunyah Hypervisor dt-bindings: Add binding for gunyah hypervisor gunyah: Common types and error codes for Gunyah hypercalls virt: gunyah: Add hypercalls to identify Gunyah virt: gunyah: Identify hypervisor version virt: gunyah: msgq: Add hypercalls to send and receive messages mailbox: Add Gunyah message queue mailbox gunyah: rsc_mgr: Add resource manager RPC core gunyah: rsc_mgr: Add VM lifecycle RPC gunyah: vm_mgr: Introduce basic VM Manager gunyah: rsc_mgr: Add RPC for sharing memory gunyah: vm_mgr: Add/remove user memory regions gunyah: vm_mgr: Add ioctls to support basic non-proxy VM boot samples: Add sample userspace Gunyah VM Manager gunyah: rsc_mgr: Add platform ops on mem_lend/mem_reclaim firmware: qcom_scm: Register Gunyah platform ops docs: gunyah: Document Gunyah VM Manager virt: gunyah: Translate gh_rm_hyp_resource into gunyah_resource gunyah: vm_mgr: Add framework to add VM Functions virt: gunyah: Add resource tickets virt: gunyah: Add IO handlers virt: gunyah: Add proxy-scheduled vCPUs virt: gunyah: Add hypercalls for sending doorbell virt: gunyah: Add irqfd interface virt: gunyah: Add ioeventfd MAINTAINERS: Add Gunyah hypervisor drivers section .../bindings/firmware/gunyah-hypervisor.yaml | 82 ++ .../userspace-api/ioctl/ioctl-number.rst | 1 + Documentation/virt/gunyah/index.rst | 114 +++ Documentation/virt/gunyah/message-queue.rst | 71 ++ Documentation/virt/gunyah/vm-manager.rst | 151 +++ Documentation/virt/index.rst | 1 + MAINTAINERS | 13 + arch/arm64/Kbuild | 1 + arch/arm64/gunyah/Makefile | 3 + arch/arm64/gunyah/gunyah_hypercall.c | 148 +++ arch/arm64/include/asm/gunyah.h | 23 + drivers/firmware/Kconfig | 2 + drivers/firmware/qcom_scm.c | 100 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/gunyah-msgq.c | 209 +++++ drivers/virt/Kconfig | 2 + drivers/virt/Makefile | 1 + drivers/virt/gunyah/Kconfig | 46 + drivers/virt/gunyah/Makefile | 11 + drivers/virt/gunyah/gunyah.c | 57 ++ drivers/virt/gunyah/gunyah_ioeventfd.c | 117 +++ drivers/virt/gunyah/gunyah_irqfd.c | 164 ++++ drivers/virt/gunyah/gunyah_platform_hooks.c | 80 ++ drivers/virt/gunyah/gunyah_vcpu.c | 465 +++++++++ drivers/virt/gunyah/rsc_mgr.c | 885 ++++++++++++++++++ drivers/virt/gunyah/rsc_mgr.h | 19 + drivers/virt/gunyah/rsc_mgr_rpc.c | 497 ++++++++++ drivers/virt/gunyah/vm_mgr.c | 785 ++++++++++++++++ drivers/virt/gunyah/vm_mgr.h | 71 ++ drivers/virt/gunyah/vm_mgr_mm.c | 252 +++++ include/linux/gunyah.h | 194 ++++ include/linux/gunyah_rsc_mgr.h | 168 ++++ include/linux/gunyah_vm_mgr.h | 112 +++ include/uapi/linux/gunyah.h | 257 +++++ samples/Kconfig | 10 + samples/Makefile | 1 + samples/gunyah/.gitignore | 2 + samples/gunyah/Makefile | 6 + samples/gunyah/gunyah_vmm.c | 270 ++++++ samples/gunyah/sample_vm.dts | 68 ++ 40 files changed, 5461 insertions(+) create mode 100644 Documentation/devicetree/bindings/firmware/gunyah-hypervisor.yaml create mode 100644 Documentation/virt/gunyah/index.rst create mode 100644 Documentation/virt/gunyah/message-queue.rst create mode 100644 Documentation/virt/gunyah/vm-manager.rst create mode 100644 arch/arm64/gunyah/Makefile create mode 100644 arch/arm64/gunyah/gunyah_hypercall.c create mode 100644 arch/arm64/include/asm/gunyah.h create mode 100644 drivers/mailbox/gunyah-msgq.c create mode 100644 drivers/virt/gunyah/Kconfig create mode 100644 drivers/virt/gunyah/Makefile create mode 100644 drivers/virt/gunyah/gunyah.c create mode 100644 drivers/virt/gunyah/gunyah_ioeventfd.c create mode 100644 drivers/virt/gunyah/gunyah_irqfd.c create mode 100644 drivers/virt/gunyah/gunyah_platform_hooks.c create mode 100644 drivers/virt/gunyah/gunyah_vcpu.c create mode 100644 drivers/virt/gunyah/rsc_mgr.c create mode 100644 drivers/virt/gunyah/rsc_mgr.h create mode 100644 drivers/virt/gunyah/rsc_mgr_rpc.c create mode 100644 drivers/virt/gunyah/vm_mgr.c create mode 100644 drivers/virt/gunyah/vm_mgr.h create mode 100644 drivers/virt/gunyah/vm_mgr_mm.c create mode 100644 include/linux/gunyah.h create mode 100644 include/linux/gunyah_rsc_mgr.h create mode 100644 include/linux/gunyah_vm_mgr.h create mode 100644 include/uapi/linux/gunyah.h create mode 100644 samples/gunyah/.gitignore create mode 100644 samples/gunyah/Makefile create mode 100644 samples/gunyah/gunyah_vmm.c create mode 100644 samples/gunyah/sample_vm.dts base-commit: 2eb29d59ddf02e39774abfb60b2030b0b7e27c1f prerequisite-patch-id: 25a39c504532b2fcdf51baff6dc55f7885db2375 prerequisite-patch-id: b48c45acdec06adf37e09fe35e6a9412c5784800