| Message ID | 20220801211240.597859-8-quic_eberman@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
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@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
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 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 > Should be Documentaton/devicetree/bindings/firmware/gunyah-hypervisor.yaml >> >> Signed-off-by: Elliot Berman <quic_eberman@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. > 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 > >
On 8/9/2022 4:29 AM, Marc Zyngier wrote: > On Mon, 08 Aug 2022 23:22:48 +0100, > Elliot Berman <quic_eberman@quicinc.com> wrote: >> >> 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. > > Can you elaborate on this "doorbell" aspect? If you signal interrupts > to guests, they should be signalled as actual interrupts, not as some > hypervisor-specific events, as we rely on the interrupt semantics for > most things. > > Or are you talking about injecting an interrupt from a guest into > another, the doorbell representing an interrupt source? > Doorbells can operate either of these modes: 1. As simple interrupt sources. The doorbell sender makes a hypercall and an interrupt is raised on the receiver. The hypervisor can be configured to raise a specific SPI on the receiver VM and simply acknowledging the SPI is enough to clear the interrupt assert. No hypervisor-specific code is needed on the receiver to handle these interrupts. This is the mode one would expect to use for paravirtualized devices. 2. As hypervisor-specific events which must be acknowledged using hypercalls. We aren't currently using this advanced use-case and no plans currently to post these. However, I can try to briefly explain: These doorbells can operate on a bitfield and the sender can assert flags on the bitmask; the receiver can decide which bits should trigger the interrupt and which SPI the doorbell "runs" on. The "user story" for this doorbell is to support multiple sender using the same doorbell object. Each sender has a few designated bits they should set. The receiver can choose which events it wants an interrupt to be raised for and then can process all the pending events. To re-iterate, we don't have an interesting use-case for this yet, so don't plan on post patches for this second mode of doorbell. > Thanks, > > M. >
On 09/08/2022 19:50, Elliot Berman wrote: > > > On 8/9/2022 4:29 AM, Marc Zyngier wrote: >> On Mon, 08 Aug 2022 23:22:48 +0100, >> Elliot Berman <quic_eberman@quicinc.com> wrote: >>> >>> 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. >> >> Can you elaborate on this "doorbell" aspect? If you signal interrupts >> to guests, they should be signalled as actual interrupts, not as some >> hypervisor-specific events, as we rely on the interrupt semantics for >> most things. >> >> Or are you talking about injecting an interrupt from a guest into >> another, the doorbell representing an interrupt source? >> > > Doorbells can operate either of these modes: > 1. As simple interrupt sources. The doorbell sender makes a hypercall > and an interrupt is raised on the receiver. The hypervisor can be > configured to raise a specific SPI on the receiver VM and simply > acknowledging the SPI is enough to clear the interrupt assert. No > hypervisor-specific code is needed on the receiver to handle these > interrupts. This is the mode one would expect to use for > paravirtualized devices. This sounds good. > 2. As hypervisor-specific events which must be acknowledged using > hypercalls. We aren't currently using this advanced use-case and no > plans currently to post these. However, I can try to briefly > explain: These doorbells can operate on a bitfield and the sender > can assert flags on the bitmask; the receiver can decide which bits > should trigger the interrupt and which SPI the doorbell "runs" on. > The "user story" for this doorbell is to support multiple sender > using the same doorbell object. Each sender has a few designated > bits they should set. The receiver can choose which events it wants > an interrupt to be raised for and then can process all the pending > events. To re-iterate, we don't have an interesting use-case for > this yet, so don't plan on post patches for this second mode of > doorbell. Well. For me this sounds like 'we have such capability, no real usecase, but we want to support it anyway' kind of story. As history has shown multiple times, the order should be the opposite one. First you have the use case, then you create the API for it. Otherwise it is very easy to end up with the abstraction that looks good on the API side, but is very hard to fit into the actual user code. I would suggest to drop the second bullet for now and focus on getting the simple doorbells done and accepted into mainline.
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 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); + +out: + return ret; +} +EXPORT_SYMBOL_GPL(gh_msgq_send); + +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, +}; + +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 "); 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; + 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
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 use gh_msgq_platform_host_attach to allocate the tx and rx message queues according to Documentation/devicetree/bindings/gunyah/qcom,hypervisor.yml. Signed-off-by: Elliot Berman <quic_eberman@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