On 8/2/2022 1:14 AM, Dmitry Baryshkov wrote:
On 02/08/2022 00:12, Elliot Berman wrote:Gunyah message queues are unidirectional pipelines to communicate between 2 virtual machines, but are typically paired to allow bidirectional communication. The intended use case is for small control messages between 2 VMs, as they support a maximum of 240 bytes. Message queues can be discovered either by resource manager or on the devicetree. To support discovery on the devicetree, client drivers candevicetree and discovery do not quite match to me. The device is delared in the DT, not discovered.>> use gh_msgq_platform_host_attach to allocate the tx and rx messagequeues according to Documentation/devicetree/bindings/gunyah/qcom,hypervisor.yml.-ENOSUCHFILE
Should be Documentaton/devicetree/bindings/firmware/gunyah-hypervisor.yaml
Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx> --- arch/arm64/include/asm/gunyah.h | 4 + drivers/virt/gunyah/Makefile | 2 +- drivers/virt/gunyah/gunyah_private.h | 3 + drivers/virt/gunyah/msgq.c | 223 +++++++++++++++++++++++++++ drivers/virt/gunyah/sysfs.c | 9 ++ include/linux/gunyah.h | 13 ++ 6 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 drivers/virt/gunyah/msgq.cdiff --git a/arch/arm64/include/asm/gunyah.h b/arch/arm64/include/asm/gunyah.hindex 3aee35009910..ba7398bd851b 100644 --- a/arch/arm64/include/asm/gunyah.h +++ b/arch/arm64/include/asm/gunyah.h @@ -27,6 +27,10 @@ | ((fn) & GH_CALL_FUNCTION_NUM_MASK)) #define GH_HYPERCALL_HYP_IDENTIFY GH_HYPERCALL(0x0000) +#define GH_HYPERCALL_MSGQ_SEND GH_HYPERCALL(0x001B) +#define GH_HYPERCALL_MSGQ_RECV GH_HYPERCALL(0x001C) + +#define GH_HYPERCALL_MSGQ_SEND_FLAGS_PUSH BIT(0) #define ___gh_count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile index 3869fb7371df..94dc8e738911 100644 --- a/drivers/virt/gunyah/Makefile +++ b/drivers/virt/gunyah/Makefile @@ -1,4 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only -gunyah-y += sysfs.o device.o +gunyah-y += sysfs.o device.o msgq.o obj-$(CONFIG_GUNYAH) += gunyah.o \ No newline at end of fileNewlinediff --git a/drivers/virt/gunyah/gunyah_private.h b/drivers/virt/gunyah/gunyah_private.hindex 5f3832608020..2ade32bd9bdf 100644 --- a/drivers/virt/gunyah/gunyah_private.h +++ b/drivers/virt/gunyah/gunyah_private.h @@ -9,4 +9,7 @@ int __init gunyah_bus_init(void); void gunyah_bus_exit(void); +int __init gh_msgq_init(void); +void gh_msgq_exit(void); + #endif diff --git a/drivers/virt/gunyah/msgq.c b/drivers/virt/gunyah/msgq.c new file mode 100644 index 000000000000..afc2572d3e7d --- /dev/null +++ b/drivers/virt/gunyah/msgq.c @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.+ */ + +#include <linux/interrupt.h> +#include <linux/gunyah.h> +#include <linux/module.h> +#include <linux/printk.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/wait.h> + +#include "gunyah_private.h" + +struct gh_msgq { + bool ready; + wait_queue_head_t wq; + spinlock_t lock; +}; + +static irqreturn_t gh_msgq_irq_handler(int irq, void *dev) +{ + struct gh_msgq *msgq = dev; + + spin_lock(&msgq->lock); + msgq->ready = true; + spin_unlock(&msgq->lock); + wake_up_interruptible_all(&msgq->wq); + + return IRQ_HANDLED; +} ++static int __gh_msgq_send(struct gunyah_device *ghdev, void *buff, size_t size, u64 tx_flags)+{ + unsigned long flags, gh_error; + struct gh_msgq *msgq = ghdev_get_drvdata(ghdev); + ssize_t ret; + bool ready; + + spin_lock_irqsave(&msgq->lock, flags); + arch_gh_hypercall(GH_HYPERCALL_MSGQ_SEND, 5, + ghdev->capid, size, (uintptr_t)buff, tx_flags, 0, + gh_error, ready); + switch (gh_error) { + case GH_ERROR_OK: + ret = 0; + msgq->ready = ready; + break; + case GH_ERROR_MSGQUEUE_FULL: + ret = -EAGAIN; + msgq->ready = false; + break; + default: + ret = gh_remap_error(gh_error); + break; + } + spin_unlock_irqrestore(&msgq->lock, flags); + + return ret; +} + +/**+ * gh_msgq_send() - Send a message to the client running on a different VM + * @client: The client descriptor that was obtained via gh_msgq_register()+ * @buff: Pointer to the buffer where the received data must be placed + * @buff_size: The size of the buffer space available+ * @flags: Optional flags to pass to receive the data. For the list of flags,+ * see linux/gunyah/gh_msgq.h + *+ * Returns: The number of bytes copied to buff. <0 if there was an error.+ *+ * Note: this function may sleep and should not be called from interrupt context+ */+ssize_t gh_msgq_send(struct gunyah_device *ghdev, void *buff, size_t size,+ const unsigned long flags) +{ + struct gh_msgq *msgq = ghdev_get_drvdata(ghdev); + ssize_t ret; + u64 tx_flags = 0; + + if (flags & GH_MSGQ_TX_PUSH) + tx_flags |= GH_HYPERCALL_MSGQ_SEND_FLAGS_PUSH; + + do { + ret = __gh_msgq_send(ghdev, buff, size, tx_flags); + + if (ret == -EAGAIN) { + if (flags & GH_MSGQ_NONBLOCK) + goto out; + if (wait_event_interruptible(msgq->wq, msgq->ready)) + ret = -ERESTARTSYS; + } + } while (ret == -EAGAIN);Any limit on the amount of retries? Can the driver wait forever here?+ +out: + return ret; +} +EXPORT_SYMBOL_GPL(gh_msgq_send);Both _send and _recv functions are not well designed. Can you call gh_msgq_send() on any gunyah_device? Yes. Will it work? No.
My intention here is to rely on hypervisor to properly complain about it. I thought it better to not have redundant checks, but I can add it in the Linux layer as well.
Could you please check if mailbox API work for you? It seems that it is what you are trying to implement on your own.
My understanding is that mailbox API was designed for queuing single register accesses. The mailbox APIs aren't intended to queue up arbitrary length messages like needed here. rpmsg is similar in the sense that it had variable length messages and doesn't use the mailbox framework for this reason.
++static ssize_t __gh_msgq_recv(struct gunyah_device *ghdev, void *buff, size_t size)+{ + unsigned long flags, gh_error; + size_t recv_size; + struct gh_msgq *msgq = ghdev_get_drvdata(ghdev); + ssize_t ret; + bool ready; + + spin_lock_irqsave(&msgq->lock, flags); + + arch_gh_hypercall(GH_HYPERCALL_MSGQ_RECV, 4, + ghdev->capid, (uintptr_t)buff, size, 0, + gh_error, recv_size, ready); + switch (gh_error) { + case GH_ERROR_OK: + ret = recv_size; + msgq->ready = ready; + break; + case GH_ERROR_MSGQUEUE_EMPTY: + ret = -EAGAIN; + msgq->ready = false; + break; + default: + ret = gh_remap_error(gh_error); + break; + } + spin_unlock_irqrestore(&msgq->lock, flags); + + return ret; +} + +/**+ * gh_msgq_recv() - Receive a message from the client running on a different VM + * @client: The client descriptor that was obtained via gh_msgq_register()+ * @buff: Pointer to the buffer where the received data must be placed + * @buff_size: The size of the buffer space available+ * @flags: Optional flags to pass to receive the data. For the list of flags,+ * see linux/gunyah/gh_msgq.h + *+ * Returns: The number of bytes copied to buff. <0 if there was an error.+ *+ * Note: this function may sleep and should not be called from interrupt context+ */+ssize_t gh_msgq_recv(struct gunyah_device *ghdev, void *buff, size_t size,+ const unsigned long flags) +{ + struct gh_msgq *msgq = ghdev_get_drvdata(ghdev); + ssize_t ret; + + do { + ret = __gh_msgq_recv(ghdev, buff, size); + + if (ret == -EAGAIN) { + if (flags & GH_MSGQ_NONBLOCK) + goto out; + if (wait_event_interruptible(msgq->wq, msgq->ready)) + ret = -ERESTARTSYS; + } + } while (ret == -EAGAIN); + +out: + return ret; +} +EXPORT_SYMBOL_GPL(gh_msgq_recv); + +static int gh_msgq_probe(struct gunyah_device *ghdev) +{ + struct gh_msgq *msgq; + + msgq = devm_kzalloc(&ghdev->dev, sizeof(*msgq), GFP_KERNEL); + if (!msgq) + return -ENOMEM; + ghdev_set_drvdata(ghdev, msgq); ++ msgq->ready = true; /* Assume we can use the message queue right away */+ init_waitqueue_head(&msgq->wq); + spin_lock_init(&msgq->lock); ++ return devm_request_irq(&ghdev->dev, ghdev->irq, gh_msgq_irq_handler, 0,+ dev_name(&ghdev->dev), msgq); +} + +static struct gunyah_driver gh_msgq_tx_driver = { + .driver = { + .name = "gh_msgq_tx", + .owner = THIS_MODULE, + }, + .type = GUNYAH_DEVICE_TYPE_MSGQ_TX, + .probe = gh_msgq_probe, +}; + +static struct gunyah_driver gh_msgq_rx_driver = { + .driver = { + .name = "gh_msgq_rx", + .owner = THIS_MODULE, + }, + .type = GUNYAH_DEVICE_TYPE_MSGQ_RX, + .probe = gh_msgq_probe,If you have to duplicate the whole device structure just to bind to two difference devices, it looks like a bad abstraction. Please check how other busses have solved this issue. They did, believe me.+};MODULE_DEVICE_TABLE() ?+ +int __init gh_msgq_init(void) +{ + int ret; + + ret = gunyah_register_driver(&gh_msgq_tx_driver); + if (ret) + return ret; + + ret = gunyah_register_driver(&gh_msgq_rx_driver); + if (ret) + goto err_rx; + + return ret; +err_rx: + gunyah_unregister_driver(&gh_msgq_tx_driver); + return ret; +} + +void gh_msgq_exit(void) +{ + gunyah_unregister_driver(&gh_msgq_rx_driver); + gunyah_unregister_driver(&gh_msgq_tx_driver); +} diff --git a/drivers/virt/gunyah/sysfs.c b/drivers/virt/gunyah/sysfs.c index 220560cb3b1c..7589689e5e92 100644 --- a/drivers/virt/gunyah/sysfs.c +++ b/drivers/virt/gunyah/sysfs.c@@ -73,6 +73,8 @@ static ssize_t features_show(struct kobject *kobj, struct kobj_attribute *attr,if (GH_IDENTIFY_PARTITION_CSPACE(gunyah_api.flags)) len += sysfs_emit_at(buffer, len, "cspace "); + if (GH_IDENTIFY_MSGQUEUE(gunyah_api.flags)) + len += sysfs_emit_at(buffer, len, "message-queue ");Again, this should go to the sysfs patch.len += sysfs_emit_at(buffer, len, "\n"); return len; @@ -142,7 +144,13 @@ static int __init gunyah_init(void) if (ret) goto err_sysfs; + ret = gh_msgq_init(); + if (ret) + goto err_bus; +Please stop beating everything in a single module. Having a provider (bus) and a consumer (drivers for this bus) in a single module sounds like an overkill. Or, a wrong abstraction.Please remind me, why do you need gunyah bus in the first place? I could not find any other calls to gunyah_device_add in this series. Which devices do you expect to be added in future? Would they require separate drivers?
In a future series, I'll add the support to load other virtual machines. When running other virtual machines, additional gunyah devices are needed for doorbells (e.g. to emulate interrupts for paravirtualized devices) and to represent the vCPUs of that other VM. Other gunyah devices are also possible, but those are the immediate devices coming over the horizon.
I had an offline discussion with Bjorn and he was recommending dropping the bus/device/driver design entirely.
return ret; +err_bus: + gunyah_bus_exit(); err_sysfs: gh_sysfs_unregister(); return ret; @@ -151,6 +159,7 @@ module_init(gunyah_init); static void __exit gunyah_exit(void) { + gh_msgq_exit(); gunyah_bus_exit(); gh_sysfs_unregister(); } diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h index ce35f4491773..099224f9d6d1 100644 --- a/include/linux/gunyah.h +++ b/include/linux/gunyah.h @@ -6,6 +6,7 @@ #ifndef _GUNYAH_H #define _GUNYAH_H +#include <linux/platform_device.h> #include <linux/device.h> #include <linux/types.h> #include <linux/errno.h> @@ -117,4 +118,16 @@ struct gunyah_driver { int gunyah_register_driver(struct gunyah_driver *ghdrv); void gunyah_unregister_driver(struct gunyah_driver *ghdrv); +#define GH_MSGQ_MAX_MSG_SIZE 1024 + +/* Possible flags to pass for Tx or Rx */ +#define GH_MSGQ_TX_PUSH BIT(0) +#define GH_MSGQ_NONBLOCK BIT(32) ++ssize_t gh_msgq_send(struct gunyah_device *ghdev, void *buff, size_t size,+ const unsigned long flags);+ssize_t gh_msgq_recv(struct gunyah_device *ghdev, void *buff, size_t size,+ const unsigned long flags); + + #endif
