[PATCH v2 07/11] gunyah: msgq: Add Gunyah message queues
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Aug 2 01:14:53 PDT 2022
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 can
devicetree 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 message
> queues according to
> Documentation/devicetree/bindings/gunyah/qcom,hypervisor.yml.
-ENOSUCHFILE
>
> Signed-off-by: Elliot Berman <quic_eberman at quicinc.com>
> ---
> 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.c
>
> diff --git a/arch/arm64/include/asm/gunyah.h b/arch/arm64/include/asm/gunyah.h
> index 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 file
Newline
> diff --git a/drivers/virt/gunyah/gunyah_private.h b/drivers/virt/gunyah/gunyah_private.h
> index 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.
Could you please check if mailbox API work for you? It seems that it is
what you are trying to implement on your own.
> +
> +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?
> 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
--
With best wishes
Dmitry
More information about the linux-arm-kernel
mailing list