[v10,07/26] mailbox: Add Gunyah message queue mailbox

  Gunyah message queues are a unidirectional inter-VM pipe for messages up
to 1024 bytes. This driver supports pairing a receiver message queue and
a transmitter message queue to expose a single mailbox channel.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 Documentation/virt/gunyah/message-queue.rst |   8 +
 drivers/mailbox/Makefile                    |   2 +
 drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
 include/linux/gunyah.h                      |  56 +++++
 4 files changed, 280 insertions(+)
 create mode 100644 drivers/mailbox/gunyah-msgq.c
  

Hi Elliot,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 3ebb0ac55efaf1d0fb1b106f852c114e5021f7eb]

url:    https://github.com/intel-lab-lkp/linux/commits/Elliot-Berman/docs-gunyah-Introduce-Gunyah-Hypervisor/20230215-055721
base:   3ebb0ac55efaf1d0fb1b106f852c114e5021f7eb
patch link:    https://lore.kernel.org/r/20230214212316.3309053-1-quic_eberman%40quicinc.com
patch subject: [PATCH v10 07/26] mailbox: Add Gunyah message queue mailbox
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20230216/202302161137.mfophRBY-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b427188cd418632da7b26f283f5d2c668038186f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Elliot-Berman/docs-gunyah-Introduce-Gunyah-Hypervisor/20230215-055721
        git checkout b427188cd418632da7b26f283f5d2c668038186f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302161137.mfophRBY-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/mailbox/gunyah-msgq.c: In function 'gh_msgq_init':
>> drivers/mailbox/gunyah-msgq.c:180:15: error: implicit declaration of function 'mbox_bind_client' [-Werror=implicit-function-declaration]
     180 |         ret = mbox_bind_client(gh_msgq_chan(msgq), cl);
         |               ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/mbox_bind_client +180 drivers/mailbox/gunyah-msgq.c

   110	
   111	/**
   112	 * gh_msgq_init() - Initialize a Gunyah message queue with an mbox_client
   113	 * @parent: optional, device parent used for the mailbox controller
   114	 * @msgq: Pointer to the gh_msgq to initialize
   115	 * @cl: A mailbox client to bind to the mailbox channel that the message queue creates
   116	 * @tx_ghrsc: optional, the transmission side of the message queue
   117	 * @rx_ghrsc: optional, the receiving side of the message queue
   118	 *
   119	 * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most message queue use cases come with
   120	 * a pair of message queues to facilitate bidirectional communication. When tx_ghrsc is set,
   121	 * the client can send messages with mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
   122	 * is set, the mbox_client should register an .rx_callback() and the message queue driver will
   123	 * push all available messages upon receiving the RX ready interrupt. The messages should be
   124	 * consumed or copied by the client right away as the gh_msgq_rx_data will be replaced/destroyed
   125	 * after the callback.
   126	 *
   127	 * Returns - 0 on success, negative otherwise
   128	 */
   129	int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct mbox_client *cl,
   130			     struct gunyah_resource *tx_ghrsc, struct gunyah_resource *rx_ghrsc)
   131	{
   132		int ret;
   133	
   134		/* Must have at least a tx_ghrsc or rx_ghrsc and that they are the right device types */
   135		if ((!tx_ghrsc && !rx_ghrsc) ||
   136		    (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
   137		    (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
   138			return -EINVAL;
   139	
   140		if (gh_api_version() != GUNYAH_API_V1) {
   141			pr_err("Unrecognized gunyah version: %u. Currently supported: %d\n",
   142				gh_api_version(), GUNYAH_API_V1);
   143			return -EOPNOTSUPP;
   144		}
   145	
   146		if (!gh_api_has_feature(GH_API_FEATURE_MSGQUEUE))
   147			return -EOPNOTSUPP;
   148	
   149		msgq->tx_ghrsc = tx_ghrsc;
   150		msgq->rx_ghrsc = rx_ghrsc;
   151	
   152		msgq->mbox.dev = parent;
   153		msgq->mbox.ops = &gh_msgq_ops;
   154		msgq->mbox.num_chans = 1;
   155		msgq->mbox.txdone_irq = true;
   156		msgq->mbox.chans = kcalloc(msgq->mbox.num_chans, sizeof(*msgq->mbox.chans), GFP_KERNEL);
   157		if (!msgq->mbox.chans)
   158			return -ENOMEM;
   159	
   160		if (msgq->tx_ghrsc) {
   161			ret = request_irq(msgq->tx_ghrsc->irq, gh_msgq_tx_irq_handler, 0, "gh_msgq_tx",
   162					msgq);
   163			if (ret)
   164				goto err_chans;
   165		}
   166	
   167		if (msgq->rx_ghrsc) {
   168			ret = request_threaded_irq(msgq->rx_ghrsc->irq, NULL, gh_msgq_rx_irq_handler,
   169							IRQF_ONESHOT, "gh_msgq_rx", msgq);
   170			if (ret)
   171				goto err_tx_irq;
   172		}
   173	
   174		tasklet_setup(&msgq->txdone_tasklet, gh_msgq_txdone_tasklet);
   175	
   176		ret = mbox_controller_register(&msgq->mbox);
   177		if (ret)
   178			goto err_rx_irq;
   179	
 > 180		ret = mbox_bind_client(gh_msgq_chan(msgq), cl);
   181		if (ret)
   182			goto err_mbox;
   183	
   184		return 0;
   185	err_mbox:
   186		mbox_controller_unregister(&msgq->mbox);
   187	err_rx_irq:
   188		if (msgq->rx_ghrsc)
   189			free_irq(msgq->rx_ghrsc->irq, msgq);
   190	err_tx_irq:
   191		if (msgq->tx_ghrsc)
   192			free_irq(msgq->tx_ghrsc->irq, msgq);
   193	err_chans:
   194		kfree(msgq->mbox.chans);
   195		return ret;
   196	}
   197	EXPORT_SYMBOL_GPL(gh_msgq_init);
   198
  

On 14/02/2023 21:23, Elliot Berman wrote:
> Gunyah message queues are a unidirectional inter-VM pipe for messages up
> to 1024 bytes. This driver supports pairing a receiver message queue and
> a transmitter message queue to expose a single mailbox channel.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   Documentation/virt/gunyah/message-queue.rst |   8 +
>   drivers/mailbox/Makefile                    |   2 +
>   drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
>   include/linux/gunyah.h                      |  56 +++++
>   4 files changed, 280 insertions(+)
>   create mode 100644 drivers/mailbox/gunyah-msgq.c
> 
> diff --git a/Documentation/virt/gunyah/message-queue.rst b/Documentation/virt/gunyah/message-queue.rst
> index 0667b3eb1ff9..082085e981e0 100644
> --- a/Documentation/virt/gunyah/message-queue.rst
> +++ b/Documentation/virt/gunyah/message-queue.rst
> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs (and two capability IDs).
>         |               |         |                 |         |               |
>         |               |         |                 |         |               |
>         +---------------+         +-----------------+         +---------------+
> +
> +Gunyah message queues are exposed as mailboxes. To create the mailbox, create
> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY interrupt,
> +all messages in the RX message queue are read and pushed via the `rx_callback`
> +of the registered mbox_client.
> +
> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
> +   :identifiers: gh_msgq_init
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index fc9376117111..5f929bb55e9a 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
>   
>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
>   
> +obj-$(CONFIG_GUNYAH)		+= gunyah-msgq.o

Why are we reusing CONFIG_GUNYAH Kconfig symbol for mailbox, why not 
CONFIG_GUNYAH_MBOX?

> +
>   obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
>   
>   obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
> diff --git a/drivers/mailbox/gunyah-msgq.c b/drivers/mailbox/gunyah-msgq.c
> new file mode 100644
> index 000000000000..03ffaa30ce9b
> --- /dev/null
> +++ b/drivers/mailbox/gunyah-msgq.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/gunyah.h>
> +#include <linux/printk.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>

...

> +/* Fired when message queue transitions from "full" to "space available" to send messages */
> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
> +{
> +	struct gh_msgq *msgq = data;
> +
> +	mbox_chan_txdone(gh_msgq_chan(msgq), 0);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Fired after sending message and hypercall told us there was more space available. */
> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)

Tasklets have been long deprecated, consider using workqueues in this 
particular case.


> +{
> +	struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, txdone_tasklet);
> +
> +	mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
> +}
> +
> +static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
> +{
..

> +	tasklet_schedule(&msgq->txdone_tasklet);
> +
> +	return 0;
> +}
> +
> +static struct mbox_chan_ops gh_msgq_ops = {
> +	.send_data = gh_msgq_send_data,
> +};
> +
> +/**
> + * gh_msgq_init() - Initialize a Gunyah message queue with an mbox_client
> + * @parent: optional, device parent used for the mailbox controller
> + * @msgq: Pointer to the gh_msgq to initialize
> + * @cl: A mailbox client to bind to the mailbox channel that the message queue creates
> + * @tx_ghrsc: optional, the transmission side of the message queue
> + * @rx_ghrsc: optional, the receiving side of the message queue
> + *
> + * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most message queue use cases come with
> + * a pair of message queues to facilitate bidirectional communication. When tx_ghrsc is set,
> + * the client can send messages with mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
> + * is set, the mbox_client should register an .rx_callback() and the message queue driver will
> + * push all available messages upon receiving the RX ready interrupt. The messages should be
> + * consumed or copied by the client right away as the gh_msgq_rx_data will be replaced/destroyed
> + * after the callback.
> + *
> + * Returns - 0 on success, negative otherwise
> + */
> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct mbox_client *cl,
> +		     struct gunyah_resource *tx_ghrsc, struct gunyah_resource *rx_ghrsc)
> +{
> +	int ret;
> +
> +	/* Must have at least a tx_ghrsc or rx_ghrsc and that they are the right device types */
> +	if ((!tx_ghrsc && !rx_ghrsc) ||
> +	    (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
> +	    (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
> +		return -EINVAL;
> +
> +	if (gh_api_version() != GUNYAH_API_V1) {
> +		pr_err("Unrecognized gunyah version: %u. Currently supported: %d\n",
dev_err(parent

would make this more useful

> +			gh_api_version(), GUNYAH_API_V1);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!gh_api_has_feature(GH_API_FEATURE_MSGQUEUE))
> +		return -EOPNOTSUPP;
> +
> +	msgq->tx_ghrsc = tx_ghrsc;
> +	msgq->rx_ghrsc = rx_ghrsc;
> +
> +	msgq->mbox.dev = parent;
> +	msgq->mbox.ops = &gh_msgq_ops;
> +	msgq->mbox.num_chans = 1;
> +	msgq->mbox.txdone_irq = true;
> +	msgq->mbox.chans = kcalloc(msgq->mbox.num_chans, sizeof(*msgq->mbox.chans), GFP_KERNEL);
> +	if (!msgq->mbox.chans)
> +		return -ENOMEM;
> +
> +	if (msgq->tx_ghrsc) {
> +		ret = request_irq(msgq->tx_ghrsc->irq, gh_msgq_tx_irq_handler, 0, "gh_msgq_tx",
> +				msgq);
> +		if (ret)
> +			goto err_chans;
> +	}
> +
> +	if (msgq->rx_ghrsc) {
> +		ret = request_threaded_irq(msgq->rx_ghrsc->irq, NULL, gh_msgq_rx_irq_handler,
> +						IRQF_ONESHOT, "gh_msgq_rx", msgq);
> +		if (ret)
> +			goto err_tx_irq;
> +	}
> +
> +	tasklet_setup(&msgq->txdone_tasklet, gh_msgq_txdone_tasklet);
> +
> +	ret = mbox_controller_register(&msgq->mbox);
> +	if (ret)
> +		goto err_rx_irq;
> +
> +	ret = mbox_bind_client(gh_msgq_chan(msgq), cl);
> +	if (ret)
> +		goto err_mbox;
> +
> +	return 0;
> +err_mbox:
> +	mbox_controller_unregister(&msgq->mbox);
> +err_rx_irq:
> +	if (msgq->rx_ghrsc)
> +		free_irq(msgq->rx_ghrsc->irq, msgq);
> +err_tx_irq:
> +	if (msgq->tx_ghrsc)
> +		free_irq(msgq->tx_ghrsc->irq, msgq);
> +err_chans:
> +	kfree(msgq->mbox.chans);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gh_msgq_init);
> +
> +void gh_msgq_remove(struct gh_msgq *msgq)
> +{
> +	mbox_controller_unregister(&msgq->mbox);
> +
> +	if (msgq->rx_ghrsc)
> +		free_irq(msgq->rx_ghrsc->irq, msgq);
> +
> +	if (msgq->tx_ghrsc)
> +		free_irq(msgq->tx_ghrsc->irq, msgq);
> +
> +	kfree(msgq->mbox.chans);
> +}
> +EXPORT_SYMBOL_GPL(gh_msgq_remove);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Gunyah Message Queue Driver");
  

On 2/20/2023 5:59 AM, Srinivas Kandagatla wrote:
> 
> 
> On 14/02/2023 21:23, Elliot Berman wrote:
>> Gunyah message queues are a unidirectional inter-VM pipe for messages up
>> to 1024 bytes. This driver supports pairing a receiver message queue and
>> a transmitter message queue to expose a single mailbox channel.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   Documentation/virt/gunyah/message-queue.rst |   8 +
>>   drivers/mailbox/Makefile                   |   2 +
>>   drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
>>   include/linux/gunyah.h                      |  56 +++++
>>   4 files changed, 280 insertions(+)
>>   create mode 100644 drivers/mailbox/gunyah-msgq.c
>>
>> diff --git a/Documentation/virt/gunyah/message-queue.rst 
>> b/Documentation/virt/gunyah/message-queue.rst
>> index 0667b3eb1ff9..082085e981e0 100644
>> --- a/Documentation/virt/gunyah/message-queue.rst
>> +++ b/Documentation/virt/gunyah/message-queue.rst
>> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs 
>> (and two capability IDs).
>>         |               |         |                 |         
>> |               |
>>         |               |         |                 |         
>> |               |
>>         +---------------+         +-----------------+         
>> +---------------+
>> +
>> +Gunyah message queues are exposed as mailboxes. To create the 
>> mailbox, create
>> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY 
>> interrupt,
>> +all messages in the RX message queue are read and pushed via the 
>> `rx_callback`
>> +of the registered mbox_client.
>> +
>> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
>> +   :identifiers: gh_msgq_init
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index fc9376117111..5f929bb55e9a 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
>>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)    += zynqmp-ipi-mailbox.o
>> +obj-$(CONFIG_GUNYAH)        += gunyah-msgq.o
> 
> Why are we reusing CONFIG_GUNYAH Kconfig symbol for mailbox, why not 
> CONFIG_GUNYAH_MBOX?
> 

There was some previous discussion about this:

https://lore.kernel.org/all/2a7bb5f2-1286-b661-659a-a5037150eae8@quicinc.com/

>> +
>>   obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
>>   obj-$(CONFIG_SPRD_MBOX)       += sprd-mailbox.o
>> diff --git a/drivers/mailbox/gunyah-msgq.c 
>> b/drivers/mailbox/gunyah-msgq.c
>> new file mode 100644
>> index 000000000000..03ffaa30ce9b
>> --- /dev/null
>> +++ b/drivers/mailbox/gunyah-msgq.c
>> @@ -0,0 +1,214 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>> rights reserved.
>> + */
>> +
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/gunyah.h>
>> +#include <linux/printk.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/wait.h>
> 
> ...
> 
>> +/* Fired when message queue transitions from "full" to "space 
>> available" to send messages */
>> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
>> +{
>> +    struct gh_msgq *msgq = data;
>> +
>> +    mbox_chan_txdone(gh_msgq_chan(msgq), 0);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +/* Fired after sending message and hypercall told us there was more 
>> space available. */
>> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
> 
> Tasklets have been long deprecated, consider using workqueues in this 
> particular case.
> 

Workqueues have higher latency and tasklets came as recommendation from 
Jassi. drivers/mailbox/imx-mailbox.c uses tasklets in the same way.

I did some quick unscientific measurements of ~1000x samples. The median 
latency for resource manager went from 25.5 us (tasklet) to 26 us 
(workqueue) (2% slower). The mean went from 28.7 us to 32.5 us (13% 
slower). Obviously, the outliers for workqueues were much more extreme.

> 
>> +{
>> +    struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, 
>> txdone_tasklet);
>> +
>> +    mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
>> +}
>> +
>> +static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
>> +{
> ..
> 
>> +    tasklet_schedule(&msgq->txdone_tasklet);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct mbox_chan_ops gh_msgq_ops = {
>> +    .send_data = gh_msgq_send_data,
>> +};
>> +
>> +/**
>> + * gh_msgq_init() - Initialize a Gunyah message queue with an 
>> mbox_client
>> + * @parent: optional, device parent used for the mailbox controller
>> + * @msgq: Pointer to the gh_msgq to initialize
>> + * @cl: A mailbox client to bind to the mailbox channel that the 
>> message queue creates
>> + * @tx_ghrsc: optional, the transmission side of the message queue
>> + * @rx_ghrsc: optional, the receiving side of the message queue
>> + *
>> + * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most 
>> message queue use cases come with
>> + * a pair of message queues to facilitate bidirectional 
>> communication. When tx_ghrsc is set,
>> + * the client can send messages with 
>> mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
>> + * is set, the mbox_client should register an .rx_callback() and the 
>> message queue driver will
>> + * push all available messages upon receiving the RX ready interrupt. 
>> The messages should be
>> + * consumed or copied by the client right away as the gh_msgq_rx_data 
>> will be replaced/destroyed
>> + * after the callback.
>> + *
>> + * Returns - 0 on success, negative otherwise
>> + */
>> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct 
>> mbox_client *cl,
>> +             struct gunyah_resource *tx_ghrsc, struct gunyah_resource 
>> *rx_ghrsc)
>> +{
>> +    int ret;
>> +
>> +    /* Must have at least a tx_ghrsc or rx_ghrsc and that they are 
>> the right device types */
>> +    if ((!tx_ghrsc && !rx_ghrsc) ||
>> +        (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
>> +        (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
>> +        return -EINVAL;
>> +
>> +    if (gh_api_version() != GUNYAH_API_V1) {
>> +        pr_err("Unrecognized gunyah version: %u. Currently supported: 
>> %d\n",
> dev_err(parent
> 
> would make this more useful
> 

Done.

- Elliot
  

On 23/02/2023 00:15, Elliot Berman wrote:
> 
> 
> On 2/20/2023 5:59 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 14/02/2023 21:23, Elliot Berman wrote:
>>> Gunyah message queues are a unidirectional inter-VM pipe for messages up
>>> to 1024 bytes. This driver supports pairing a receiver message queue and
>>> a transmitter message queue to expose a single mailbox channel.
>>>
>>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>>> ---
>>>   Documentation/virt/gunyah/message-queue.rst |   8 +
>>>   drivers/mailbox/Makefile                   |   2 +
>>>   drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
>>>   include/linux/gunyah.h                      |  56 +++++
>>>   4 files changed, 280 insertions(+)
>>>   create mode 100644 drivers/mailbox/gunyah-msgq.c
>>>
>>> diff --git a/Documentation/virt/gunyah/message-queue.rst 
>>> b/Documentation/virt/gunyah/message-queue.rst
>>> index 0667b3eb1ff9..082085e981e0 100644
>>> --- a/Documentation/virt/gunyah/message-queue.rst
>>> +++ b/Documentation/virt/gunyah/message-queue.rst
>>> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs 
>>> (and two capability IDs).
>>>         |               |         |                 | |               |
>>>         |               |         |                 | |               |
>>>         +---------------+         +-----------------+ +---------------+
>>> +
>>> +Gunyah message queues are exposed as mailboxes. To create the 
>>> mailbox, create
>>> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY 
>>> interrupt,
>>> +all messages in the RX message queue are read and pushed via the 
>>> `rx_callback`
>>> +of the registered mbox_client.
>>> +
>>> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
>>> +   :identifiers: gh_msgq_init
>>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>>> index fc9376117111..5f929bb55e9a 100644
>>> --- a/drivers/mailbox/Makefile
>>> +++ b/drivers/mailbox/Makefile
>>> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
>>>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)    += zynqmp-ipi-mailbox.o
>>> +obj-$(CONFIG_GUNYAH)        += gunyah-msgq.o
>>
>> Why are we reusing CONFIG_GUNYAH Kconfig symbol for mailbox, why not 
>> CONFIG_GUNYAH_MBOX?
>>
> 
> There was some previous discussion about this:
> 
> https://lore.kernel.org/all/2a7bb5f2-1286-b661-659a-a5037150eae8@quicinc.com/
> 
>>> +
>>>   obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
>>>   obj-$(CONFIG_SPRD_MBOX)       += sprd-mailbox.o
>>> diff --git a/drivers/mailbox/gunyah-msgq.c 
>>> b/drivers/mailbox/gunyah-msgq.c
>>> new file mode 100644
>>> index 000000000000..03ffaa30ce9b
>>> --- /dev/null
>>> +++ b/drivers/mailbox/gunyah-msgq.c
>>> @@ -0,0 +1,214 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>>> rights reserved.
>>> + */
>>> +
>>> +#include <linux/mailbox_controller.h>
>>> +#include <linux/module.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/gunyah.h>
>>> +#include <linux/printk.h>
>>> +#include <linux/init.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/wait.h>
>>
>> ...
>>
>>> +/* Fired when message queue transitions from "full" to "space 
>>> available" to send messages */
>>> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
>>> +{
>>> +    struct gh_msgq *msgq = data;
>>> +
>>> +    mbox_chan_txdone(gh_msgq_chan(msgq), 0);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* Fired after sending message and hypercall told us there was more 
>>> space available. */
>>> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
>>
>> Tasklets have been long deprecated, consider using workqueues in this 
>> particular case.
>>
> 
> Workqueues have higher latency and tasklets came as recommendation from 
> Jassi. drivers/mailbox/imx-mailbox.c uses tasklets in the same way.
> 
> I did some quick unscientific measurements of ~1000x samples. The median 
> latency for resource manager went from 25.5 us (tasklet) to 26 us 
> (workqueue) (2% slower). The mean went from 28.7 us to 32.5 us (13% 
> slower). Obviously, the outliers for workqueues were much more extreme.

TBH, this is expected because we are only testing resource manager, Note 
  the advantage that you will see shifting from tasket to workqueues is 
on overall system latencies and some drivers performance that need to 
react to events.

please take some time to read this nice article about this 
https://lwn.net/Articles/830964/


--srini
> 
>>
>>> +{
>>> +    struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, 
>>> txdone_tasklet);
>>> +
>>> +    mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
>>> +}
>>> +
>>> +static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
>>> +{
>> ..
>>
>>> +    tasklet_schedule(&msgq->txdone_tasklet);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct mbox_chan_ops gh_msgq_ops = {
>>> +    .send_data = gh_msgq_send_data,
>>> +};
>>> +
>>> +/**
>>> + * gh_msgq_init() - Initialize a Gunyah message queue with an 
>>> mbox_client
>>> + * @parent: optional, device parent used for the mailbox controller
>>> + * @msgq: Pointer to the gh_msgq to initialize
>>> + * @cl: A mailbox client to bind to the mailbox channel that the 
>>> message queue creates
>>> + * @tx_ghrsc: optional, the transmission side of the message queue
>>> + * @rx_ghrsc: optional, the receiving side of the message queue
>>> + *
>>> + * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most 
>>> message queue use cases come with
>>> + * a pair of message queues to facilitate bidirectional 
>>> communication. When tx_ghrsc is set,
>>> + * the client can send messages with 
>>> mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
>>> + * is set, the mbox_client should register an .rx_callback() and the 
>>> message queue driver will
>>> + * push all available messages upon receiving the RX ready 
>>> interrupt. The messages should be
>>> + * consumed or copied by the client right away as the 
>>> gh_msgq_rx_data will be replaced/destroyed
>>> + * after the callback.
>>> + *
>>> + * Returns - 0 on success, negative otherwise
>>> + */
>>> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct 
>>> mbox_client *cl,
>>> +             struct gunyah_resource *tx_ghrsc, struct 
>>> gunyah_resource *rx_ghrsc)
>>> +{
>>> +    int ret;
>>> +
>>> +    /* Must have at least a tx_ghrsc or rx_ghrsc and that they are 
>>> the right device types */
>>> +    if ((!tx_ghrsc && !rx_ghrsc) ||
>>> +        (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
>>> +        (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
>>> +        return -EINVAL;
>>> +
>>> +    if (gh_api_version() != GUNYAH_API_V1) {
>>> +        pr_err("Unrecognized gunyah version: %u. Currently 
>>> supported: %d\n",
>> dev_err(parent
>>
>> would make this more useful
>>
> 
> Done.
> 
> - Elliot
  

On 2/14/23 3:23 PM, Elliot Berman wrote:
> Gunyah message queues are a unidirectional inter-VM pipe for messages up
> to 1024 bytes. This driver supports pairing a receiver message queue and
> a transmitter message queue to expose a single mailbox channel.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>

I have a general comment for "include/linux/gunyah.h".

You sometimes use "gunyah" in exported names (for example,
enum gunyah_resource_type, or struct gunyah_resource).  In
many cases, though, you use "gh_ or "GH_" (such as in
struct gh_msgq, or GH_DBL_NONBLOCK).  Is there a reason
that you don't pick one and use it everywhere?

I think it would be best--certainly for exported symbols
like these--to use a single symbol prefix for all cases.

Sometimes there might be a reason to distinguish two names
(maybe "gunyah_" symbols are truly public, while "gh_"
symbols are helpers meant generally to be private).  But
I don't think that's the case here.

It seems that "gh" is your most frequent prefix, so that
might be easier to implement.  But "gunyah" is more expressive
and is only 4 characters wider.

					-Alex

> ---
>   Documentation/virt/gunyah/message-queue.rst |   8 +
>   drivers/mailbox/Makefile                    |   2 +
>   drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
>   include/linux/gunyah.h                      |  56 +++++
>   4 files changed, 280 insertions(+)
>   create mode 100644 drivers/mailbox/gunyah-msgq.c
> 
> diff --git a/Documentation/virt/gunyah/message-queue.rst b/Documentation/virt/gunyah/message-queue.rst
> index 0667b3eb1ff9..082085e981e0 100644
> --- a/Documentation/virt/gunyah/message-queue.rst
> +++ b/Documentation/virt/gunyah/message-queue.rst
> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs (and two capability IDs).
>         |               |         |                 |         |               |
>         |               |         |                 |         |               |
>         +---------------+         +-----------------+         +---------------+
> +
> +Gunyah message queues are exposed as mailboxes. To create the mailbox, create
> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY interrupt,
> +all messages in the RX message queue are read and pushed via the `rx_callback`
> +of the registered mbox_client.
> +
> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
> +   :identifiers: gh_msgq_init
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index fc9376117111..5f929bb55e9a 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
>   
>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
>   
> +obj-$(CONFIG_GUNYAH)		+= gunyah-msgq.o
> +
>   obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
>   
>   obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
> diff --git a/drivers/mailbox/gunyah-msgq.c b/drivers/mailbox/gunyah-msgq.c
> new file mode 100644
> index 000000000000..03ffaa30ce9b
> --- /dev/null
> +++ b/drivers/mailbox/gunyah-msgq.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/gunyah.h>
> +#include <linux/printk.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +
> +#define mbox_chan_to_msgq(chan) (container_of(chan->mbox, struct gh_msgq, mbox))
> +
> +static irqreturn_t gh_msgq_rx_irq_handler(int irq, void *data)
> +{
> +	struct gh_msgq *msgq = data;
> +	struct gh_msgq_rx_data rx_data;
> +	enum gh_error err;
> +	bool ready = true;
> +
> +	while (ready) {
> +		err = gh_hypercall_msgq_recv(msgq->rx_ghrsc->capid,
> +				(uintptr_t)&rx_data.data, sizeof(rx_data.data),
> +				&rx_data.length, &ready);
> +		if (err != GH_ERROR_OK) {
> +			if (err != GH_ERROR_MSGQUEUE_EMPTY)
> +				pr_warn("Failed to receive data from msgq for %s: %d\n",
> +					msgq->mbox.dev ? dev_name(msgq->mbox.dev) : "", err);
> +			break;
> +		}
> +		mbox_chan_received_data(gh_msgq_chan(msgq), &rx_data);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Fired when message queue transitions from "full" to "space available" to send messages */
> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
> +{
> +	struct gh_msgq *msgq = data;
> +
> +	mbox_chan_txdone(gh_msgq_chan(msgq), 0);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Fired after sending message and hypercall told us there was more space available. */
> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
> +{
> +	struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, txdone_tasklet);
> +
> +	mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
> +}
> +
> +static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct gh_msgq *msgq = mbox_chan_to_msgq(chan);
> +	struct gh_msgq_tx_data *msgq_data = data;
> +	u64 tx_flags = 0;
> +	enum gh_error gh_error;
> +	bool ready;
> +
> +	if (msgq_data->push)
> +		tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH;
> +
> +	gh_error = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, msgq_data->length,
> +					(uintptr_t)msgq_data->data, tx_flags, &ready);
> +
> +	/**
> +	 * unlikely because Linux tracks state of msgq and should not try to
> +	 * send message when msgq is full.
> +	 */
> +	if (unlikely(gh_error == GH_ERROR_MSGQUEUE_FULL))
> +		return -EAGAIN;
> +
> +	/**
> +	 * Propagate all other errors to client. If we return error to mailbox
> +	 * framework, then no other messages can be sent and nobody will know
> +	 * to retry this message.
> +	 */
> +	msgq->last_ret = gh_remap_error(gh_error);
> +
> +	/**
> +	 * This message was successfully sent, but message queue isn't ready to
> +	 * receive more messages because it's now full. Mailbox framework
> +	 * requires that we only report that message was transmitted when
> +	 * we're ready to transmit another message. We'll get that in the form
> +	 * of tx IRQ once the other side starts to drain the msgq.
> +	 */
> +	if (gh_error == GH_ERROR_OK && !ready)
> +		return 0;
> +
> +	/**
> +	 * We can send more messages. Mailbox framework requires that tx done
> +	 * happens asynchronously to sending the message. Gunyah message queues
> +	 * tell us right away on the hypercall return whether we can send more
> +	 * messages. To work around this, defer the txdone to a tasklet.
> +	 */
> +	tasklet_schedule(&msgq->txdone_tasklet);
> +
> +	return 0;
> +}
> +
> +static struct mbox_chan_ops gh_msgq_ops = {
> +	.send_data = gh_msgq_send_data,
> +};
> +
> +/**
> + * gh_msgq_init() - Initialize a Gunyah message queue with an mbox_client
> + * @parent: optional, device parent used for the mailbox controller
> + * @msgq: Pointer to the gh_msgq to initialize
> + * @cl: A mailbox client to bind to the mailbox channel that the message queue creates
> + * @tx_ghrsc: optional, the transmission side of the message queue
> + * @rx_ghrsc: optional, the receiving side of the message queue
> + *
> + * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most message queue use cases come with
> + * a pair of message queues to facilitate bidirectional communication. When tx_ghrsc is set,
> + * the client can send messages with mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
> + * is set, the mbox_client should register an .rx_callback() and the message queue driver will
> + * push all available messages upon receiving the RX ready interrupt. The messages should be
> + * consumed or copied by the client right away as the gh_msgq_rx_data will be replaced/destroyed
> + * after the callback.
> + *
> + * Returns - 0 on success, negative otherwise
> + */
> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct mbox_client *cl,
> +		     struct gunyah_resource *tx_ghrsc, struct gunyah_resource *rx_ghrsc)
> +{
> +	int ret;
> +
> +	/* Must have at least a tx_ghrsc or rx_ghrsc and that they are the right device types */
> +	if ((!tx_ghrsc && !rx_ghrsc) ||
> +	    (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
> +	    (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
> +		return -EINVAL;
> +
> +	if (gh_api_version() != GUNYAH_API_V1) {
> +		pr_err("Unrecognized gunyah version: %u. Currently supported: %d\n",
> +			gh_api_version(), GUNYAH_API_V1);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!gh_api_has_feature(GH_API_FEATURE_MSGQUEUE))
> +		return -EOPNOTSUPP;
> +
> +	msgq->tx_ghrsc = tx_ghrsc;
> +	msgq->rx_ghrsc = rx_ghrsc;
> +
> +	msgq->mbox.dev = parent;
> +	msgq->mbox.ops = &gh_msgq_ops;
> +	msgq->mbox.num_chans = 1;
> +	msgq->mbox.txdone_irq = true;
> +	msgq->mbox.chans = kcalloc(msgq->mbox.num_chans, sizeof(*msgq->mbox.chans), GFP_KERNEL);
> +	if (!msgq->mbox.chans)
> +		return -ENOMEM;
> +
> +	if (msgq->tx_ghrsc) {
> +		ret = request_irq(msgq->tx_ghrsc->irq, gh_msgq_tx_irq_handler, 0, "gh_msgq_tx",
> +				msgq);
> +		if (ret)
> +			goto err_chans;
> +	}
> +
> +	if (msgq->rx_ghrsc) {
> +		ret = request_threaded_irq(msgq->rx_ghrsc->irq, NULL, gh_msgq_rx_irq_handler,
> +						IRQF_ONESHOT, "gh_msgq_rx", msgq);
> +		if (ret)
> +			goto err_tx_irq;
> +	}
> +
> +	tasklet_setup(&msgq->txdone_tasklet, gh_msgq_txdone_tasklet);
> +
> +	ret = mbox_controller_register(&msgq->mbox);
> +	if (ret)
> +		goto err_rx_irq;
> +
> +	ret = mbox_bind_client(gh_msgq_chan(msgq), cl);
> +	if (ret)
> +		goto err_mbox;
> +
> +	return 0;
> +err_mbox:
> +	mbox_controller_unregister(&msgq->mbox);
> +err_rx_irq:
> +	if (msgq->rx_ghrsc)
> +		free_irq(msgq->rx_ghrsc->irq, msgq);
> +err_tx_irq:
> +	if (msgq->tx_ghrsc)
> +		free_irq(msgq->tx_ghrsc->irq, msgq);
> +err_chans:
> +	kfree(msgq->mbox.chans);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gh_msgq_init);
> +
> +void gh_msgq_remove(struct gh_msgq *msgq)
> +{
> +	mbox_controller_unregister(&msgq->mbox);
> +
> +	if (msgq->rx_ghrsc)
> +		free_irq(msgq->rx_ghrsc->irq, msgq);
> +
> +	if (msgq->tx_ghrsc)
> +		free_irq(msgq->tx_ghrsc->irq, msgq);
> +
> +	kfree(msgq->mbox.chans);
> +}
> +EXPORT_SYMBOL_GPL(gh_msgq_remove);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Gunyah Message Queue Driver");
> diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h
> index cb6df4eec5c2..2e13669c6363 100644
> --- a/include/linux/gunyah.h
> +++ b/include/linux/gunyah.h
> @@ -8,11 +8,67 @@
>   
>   #include <linux/bitfield.h>
>   #include <linux/errno.h>
> +#include <linux/interrupt.h>
>   #include <linux/limits.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox_client.h>
>   #include <linux/types.h>
>   
> +/* Follows resource manager's resource types for VM_GET_HYP_RESOURCES */
> +enum gunyah_resource_type {
> +	GUNYAH_RESOURCE_TYPE_BELL_TX	= 0,
> +	GUNYAH_RESOURCE_TYPE_BELL_RX	= 1,
> +	GUNYAH_RESOURCE_TYPE_MSGQ_TX	= 2,
> +	GUNYAH_RESOURCE_TYPE_MSGQ_RX	= 3,
> +	GUNYAH_RESOURCE_TYPE_VCPU	= 4,
> +};
> +
> +struct gunyah_resource {
> +	enum gunyah_resource_type type;
> +	u64 capid;
> +	int irq;
> +};
> +
> +/**
> + * Gunyah Message Queues
> + */
> +
> +#define GH_MSGQ_MAX_MSG_SIZE	240
> +
> +struct gh_msgq_tx_data {
> +	size_t length;
> +	bool push;
> +	char data[];
> +};
> +
> +struct gh_msgq_rx_data {
> +	size_t length;
> +	char data[GH_MSGQ_MAX_MSG_SIZE];
> +};
> +
> +struct gh_msgq {
> +	struct gunyah_resource *tx_ghrsc;
> +	struct gunyah_resource *rx_ghrsc;
> +
> +	/* msgq private */
> +	int last_ret; /* Linux error, not GH_STATUS_* */
> +	struct mbox_controller mbox;
> +	struct tasklet_struct txdone_tasklet;
> +};
> +
> +
> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct mbox_client *cl,
> +		     struct gunyah_resource *tx_ghrsc, struct gunyah_resource *rx_ghrsc);
> +void gh_msgq_remove(struct gh_msgq *msgq);
> +
> +static inline struct mbox_chan *gh_msgq_chan(struct gh_msgq *msgq)
> +{
> +	return &msgq->mbox.chans[0];
> +}
> +
>   /******************************************************************************/
>   /* Common arch-independent definitions for Gunyah hypercalls                  */
> +
>   #define GH_CAPID_INVAL	U64_MAX
>   #define GH_VMID_ROOT_VM	0xff
>
  

On 2/14/23 3:23 PM, Elliot Berman wrote:
> Gunyah message queues are a unidirectional inter-VM pipe for messages up
> to 1024 bytes. This driver supports pairing a receiver message queue and
> a transmitter message queue to expose a single mailbox channel.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   Documentation/virt/gunyah/message-queue.rst |   8 +
>   drivers/mailbox/Makefile                    |   2 +
>   drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
>   include/linux/gunyah.h                      |  56 +++++
>   4 files changed, 280 insertions(+)
>   create mode 100644 drivers/mailbox/gunyah-msgq.c
> 
> diff --git a/Documentation/virt/gunyah/message-queue.rst b/Documentation/virt/gunyah/message-queue.rst
> index 0667b3eb1ff9..082085e981e0 100644
> --- a/Documentation/virt/gunyah/message-queue.rst
> +++ b/Documentation/virt/gunyah/message-queue.rst
> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs (and two capability IDs).
>         |               |         |                 |         |               |
>         |               |         |                 |         |               |
>         +---------------+         +-----------------+         +---------------+
> +
> +Gunyah message queues are exposed as mailboxes. To create the mailbox, create
> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY interrupt,
> +all messages in the RX message queue are read and pushed via the `rx_callback`
> +of the registered mbox_client.
> +
> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
> +   :identifiers: gh_msgq_init
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index fc9376117111..5f929bb55e9a 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
>   
>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
>   
> +obj-$(CONFIG_GUNYAH)		+= gunyah-msgq.o
> +
>   obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
>   
>   obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
> diff --git a/drivers/mailbox/gunyah-msgq.c b/drivers/mailbox/gunyah-msgq.c
> new file mode 100644
> index 000000000000..03ffaa30ce9b
> --- /dev/null
> +++ b/drivers/mailbox/gunyah-msgq.c

You use a dash in this source file name, but an underscore
everywhere else.  Unless there's a good reason to do this,
please be consistent (use "gunyah_msgq.c").

> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/gunyah.h>
> +#include <linux/printk.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +
> +#define mbox_chan_to_msgq(chan) (container_of(chan->mbox, struct gh_msgq, mbox))
> +
> +static irqreturn_t gh_msgq_rx_irq_handler(int irq, void *data)
> +{
> +	struct gh_msgq *msgq = data;
> +	struct gh_msgq_rx_data rx_data;
> +	enum gh_error err;
> +	bool ready = true;
> +
> +	while (ready) {
> +		err = gh_hypercall_msgq_recv(msgq->rx_ghrsc->capid,
> +				(uintptr_t)&rx_data.data, sizeof(rx_data.data),
> +				&rx_data.length, &ready);
> +		if (err != GH_ERROR_OK) {
> +			if (err != GH_ERROR_MSGQUEUE_EMPTY)

Srini mentioned something about this too.  In many
(all?) cases, there is a device pointer available,
so you should use dev_*() functions rather than pr_*().

In this particular case, I'm not sure why/when the
mbox.dev pointer would be null.  Also, dev_*() handles
the case of a null device pointer, and it reports the
device name (just as you do here).

> +				pr_warn("Failed to receive data from msgq for %s: %d\n",
> +					msgq->mbox.dev ? dev_name(msgq->mbox.dev) : "", err);
> +			break;
> +		}
> +		mbox_chan_received_data(gh_msgq_chan(msgq), &rx_data);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Fired when message queue transitions from "full" to "space available" to send messages */
> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
> +{
> +	struct gh_msgq *msgq = data;
> +
> +	mbox_chan_txdone(gh_msgq_chan(msgq), 0);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Fired after sending message and hypercall told us there was more space available. */
> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
> +{
> +	struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, txdone_tasklet);
> +
> +	mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
> +}
> +
> +static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct gh_msgq *msgq = mbox_chan_to_msgq(chan);
> +	struct gh_msgq_tx_data *msgq_data = data;
> +	u64 tx_flags = 0;
> +	enum gh_error gh_error;

Above you named the variable "err".  It helps readability
if you use a very consistent naming convention for variables
of a certain type when they are used a lot.

> +	bool ready;
> +
> +	if (msgq_data->push)
> +		tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH;
> +
> +	gh_error = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, msgq_data->length,
> +					(uintptr_t)msgq_data->data, tx_flags, &ready);
> +
> +	/**
> +	 * unlikely because Linux tracks state of msgq and should not try to
> +	 * send message when msgq is full.
> +	 */
> +	if (unlikely(gh_error == GH_ERROR_MSGQUEUE_FULL))
> +		return -EAGAIN;
> +
> +	/**
> +	 * Propagate all other errors to client. If we return error to mailbox
> +	 * framework, then no other messages can be sent and nobody will know
> +	 * to retry this message.
> +	 */
> +	msgq->last_ret = gh_remap_error(gh_error);
> +
> +	/**
> +	 * This message was successfully sent, but message queue isn't ready to
> +	 * receive more messages because it's now full. Mailbox framework

Maybe:  s/receive/accept/

> +	 * requires that we only report that message was transmitted when
> +	 * we're ready to transmit another message. We'll get that in the form
> +	 * of tx IRQ once the other side starts to drain the msgq.
> +	 */
> +	if (gh_error == GH_ERROR_OK && !ready)
> +		return 0;
> +
> +	/**
> +	 * We can send more messages. Mailbox framework requires that tx done
> +	 * happens asynchronously to sending the message. Gunyah message queues
> +	 * tell us right away on the hypercall return whether we can send more
> +	 * messages. To work around this, defer the txdone to a tasklet.
> +	 */
> +	tasklet_schedule(&msgq->txdone_tasklet);
> +
> +	return 0;
> +}
> +
> +static struct mbox_chan_ops gh_msgq_ops = {
> +	.send_data = gh_msgq_send_data,
> +};
> +
> +/**
> + * gh_msgq_init() - Initialize a Gunyah message queue with an mbox_client
> + * @parent: optional, device parent used for the mailbox controller
> + * @msgq: Pointer to the gh_msgq to initialize
> + * @cl: A mailbox client to bind to the mailbox channel that the message queue creates
> + * @tx_ghrsc: optional, the transmission side of the message queue
> + * @rx_ghrsc: optional, the receiving side of the message queue
> + *
> + * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most message queue use cases come with

s/should be/must be/

> + * a pair of message queues to facilitate bidirectional communication. When tx_ghrsc is set,
> + * the client can send messages with mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
> + * is set, the mbox_client should register an .rx_callback() and the message queue driver will

s/should register/must register/

A general comment on this code is that you sort of half define
a Gunyah message queue API.  You define an initialization
function and an exit function, but you also expose the fact
that you use the mailbox framework in implementation.  This
despite avoiding defining it as an mbox in the DTS file.

It might be hard to avoid that I guess.  But to me it would be
nice if there were a more distinct Gunyah message queue API,
which would provide a send_message() function, for example.
And in that case, perhaps you would pass in the tx_done and/or
rx_data callbacks to this function (since they're required).

All that said, this is (currently?) only used by the resource
manager, so making a beautiful API might not be that important.
Do you envision this being used to communicate with other VMs
in the future?

> + * push all available messages upon receiving the RX ready interrupt. The messages should be

Maybe: s/push/deliver/

> + * consumed or copied by the client right away as the gh_msgq_rx_data will be replaced/destroyed
> + * after the callback.
> + *
> + * Returns - 0 on success, negative otherwise
> + */
> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct mbox_client *cl,
> +		     struct gunyah_resource *tx_ghrsc, struct gunyah_resource *rx_ghrsc)
> +{
> +	int ret;
> +
> +	/* Must have at least a tx_ghrsc or rx_ghrsc and that they are the right device types */
> +	if ((!tx_ghrsc && !rx_ghrsc) ||
> +	    (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
> +	    (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
> +		return -EINVAL;
> +
> +	if (gh_api_version() != GUNYAH_API_V1) {
> +		pr_err("Unrecognized gunyah version: %u. Currently supported: %d\n",
> +			gh_api_version(), GUNYAH_API_V1);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!gh_api_has_feature(GH_API_FEATURE_MSGQUEUE))
> +		return -EOPNOTSUPP;

Can Gunyah even function if it doesn't have the MSGQUEUE feature?
Will there ever be a Gunyah implementation that does not support
it?  Perhaps this test could be done in gunyah_init() instead.

For that matter, you could verify the result of gh_api_version()
at that time also.

> +
> +	msgq->tx_ghrsc = tx_ghrsc;
> +	msgq->rx_ghrsc = rx_ghrsc;
> +
> +	msgq->mbox.dev = parent;
> +	msgq->mbox.ops = &gh_msgq_ops;
> +	msgq->mbox.num_chans = 1;
> +	msgq->mbox.txdone_irq = true;
> +	msgq->mbox.chans = kcalloc(msgq->mbox.num_chans, sizeof(*msgq->mbox.chans), GFP_KERNEL);

 From what I can tell, you will always use exactly one mailbox channel.
So you could just do kzalloc(sizeof()...).

> +	if (!msgq->mbox.chans)
> +		return -ENOMEM;
> +
> +	if (msgq->tx_ghrsc) {

	if (tx_ghrsc) {

The irq field is assumed to be valid.  Are there any
sanity checks you could perform?  Again this is only
used for the resource manager right now, so maybe
it's OK.

> +		ret = request_irq(msgq->tx_ghrsc->irq, gh_msgq_tx_irq_handler, 0, "gh_msgq_tx",

		ret = request_irq(tx_ghrsc->irq, ...


> +				msgq);
> +		if (ret)
> +			goto err_chans;
> +	}
> +
> +	if (msgq->rx_ghrsc) {
> +		ret = request_threaded_irq(msgq->rx_ghrsc->irq, NULL, gh_msgq_rx_irq_handler,
> +						IRQF_ONESHOT, "gh_msgq_rx", msgq);
> +		if (ret)
> +			goto err_tx_irq;
> +	}
> +
> +	tasklet_setup(&msgq->txdone_tasklet, gh_msgq_txdone_tasklet);
> +
> +	ret = mbox_controller_register(&msgq->mbox);
> +	if (ret)
> +		goto err_rx_irq;
> +
> +	ret = mbox_bind_client(gh_msgq_chan(msgq), cl);


> +	if (ret)
> +		goto err_mbox;
> +
> +	return 0;
> +err_mbox:
> +	mbox_controller_unregister(&msgq->mbox);
> +err_rx_irq:
> +	if (msgq->rx_ghrsc)
> +		free_irq(msgq->rx_ghrsc->irq, msgq);
> +err_tx_irq:
> +	if (msgq->tx_ghrsc)
> +		free_irq(msgq->tx_ghrsc->irq, msgq);
> +err_chans:
> +	kfree(msgq->mbox.chans);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gh_msgq_init);
> +
> +void gh_msgq_remove(struct gh_msgq *msgq)
> +{

Is there any need to un-bind the client?

> +	mbox_controller_unregister(&msgq->mbox);
> +
> +	if (msgq->rx_ghrsc)
> +		free_irq(msgq->rx_ghrsc->irq, msgq);
> +
> +	if (msgq->tx_ghrsc)
> +		free_irq(msgq->tx_ghrsc->irq, msgq);
> +
> +	kfree(msgq->mbox.chans);
> +}
> +EXPORT_SYMBOL_GPL(gh_msgq_remove);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Gunyah Message Queue Driver");
> diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h
> index cb6df4eec5c2..2e13669c6363 100644
> --- a/include/linux/gunyah.h
> +++ b/include/linux/gunyah.h
> @@ -8,11 +8,67 @@
>   
>   #include <linux/bitfield.h>
>   #include <linux/errno.h>
> +#include <linux/interrupt.h>
>   #include <linux/limits.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox_client.h>
>   #include <linux/types.h>
>   
> +/* Follows resource manager's resource types for VM_GET_HYP_RESOURCES */
> +enum gunyah_resource_type {
> +	GUNYAH_RESOURCE_TYPE_BELL_TX	= 0,
> +	GUNYAH_RESOURCE_TYPE_BELL_RX	= 1,
> +	GUNYAH_RESOURCE_TYPE_MSGQ_TX	= 2,
> +	GUNYAH_RESOURCE_TYPE_MSGQ_RX	= 3,
> +	GUNYAH_RESOURCE_TYPE_VCPU	= 4,

The maximum value here must fit in 8 bits.  I guess
there's no risk right now of using that up, but you
use negative values in some cases elsewhere.

> +};
> +
> +struct gunyah_resource {
> +	enum gunyah_resource_type type;
> +	u64 capid;
> +	int irq;

request_irq() defines the IRQ value to be an unsigned int.

> +};
> +
> +/**
> + * Gunyah Message Queues
> + */
> +
> +#define GH_MSGQ_MAX_MSG_SIZE	240
> +
> +struct gh_msgq_tx_data {
> +	size_t length;
> +	bool push;
> +	char data[];
> +};
> +
> +struct gh_msgq_rx_data {
> +	size_t length;
> +	char data[GH_MSGQ_MAX_MSG_SIZE];
> +};
> +
> +struct gh_msgq {
> +	struct gunyah_resource *tx_ghrsc;
> +	struct gunyah_resource *rx_ghrsc;
> +
> +	/* msgq private */
> +	int last_ret; /* Linux error, not GH_STATUS_* */
> +	struct mbox_controller mbox;
> +	struct tasklet_struct txdone_tasklet;

Can the msgq_client be embedded here too?  (I don't really
know whether msgq and msgq_client are one-to one.)

> +};
> +
> +
> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct mbox_client *cl,
> +		     struct gunyah_resource *tx_ghrsc, struct gunyah_resource *rx_ghrsc);
> +void gh_msgq_remove(struct gh_msgq *msgq);

I suggested:

int gh_msgq_send(struct gh_msgq, struct gh_msgq_tx_data *data);

					-Alex

> +
> +static inline struct mbox_chan *gh_msgq_chan(struct gh_msgq *msgq)
> +{
> +	return &msgq->mbox.chans[0];
> +}
> +
>   /******************************************************************************/
>   /* Common arch-independent definitions for Gunyah hypercalls                  */
> +
>   #define GH_CAPID_INVAL	U64_MAX
>   #define GH_VMID_ROOT_VM	0xff
>
  

On 2/23/2023 2:25 AM, Srinivas Kandagatla wrote:
> 
> 
> On 23/02/2023 00:15, Elliot Berman wrote:
>>
>>
>> On 2/20/2023 5:59 AM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 14/02/2023 21:23, Elliot Berman wrote:
>>>> Gunyah message queues are a unidirectional inter-VM pipe for 
>>>> messages up
>>>> to 1024 bytes. This driver supports pairing a receiver message queue 
>>>> and
>>>> a transmitter message queue to expose a single mailbox channel.
>>>>
>>>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>>>> ---
>>>>   Documentation/virt/gunyah/message-queue.rst |   8 +
>>>>   drivers/mailbox/Makefile                   |   2 +
>>>>   drivers/mailbox/gunyah-msgq.c               | 214 
>>>> ++++++++++++++++++++
>>>>   include/linux/gunyah.h                      |  56 +++++
>>>>   4 files changed, 280 insertions(+)
>>>>   create mode 100644 drivers/mailbox/gunyah-msgq.c
>>>>
>>>> diff --git a/Documentation/virt/gunyah/message-queue.rst 
>>>> b/Documentation/virt/gunyah/message-queue.rst
>>>> index 0667b3eb1ff9..082085e981e0 100644
>>>> --- a/Documentation/virt/gunyah/message-queue.rst
>>>> +++ b/Documentation/virt/gunyah/message-queue.rst
>>>> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs 
>>>> (and two capability IDs).
>>>>         |               |         |                 | |               |
>>>>         |               |         |                 | |               |
>>>>         +---------------+         +-----------------+ +---------------+
>>>> +
>>>> +Gunyah message queues are exposed as mailboxes. To create the 
>>>> mailbox, create
>>>> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY 
>>>> interrupt,
>>>> +all messages in the RX message queue are read and pushed via the 
>>>> `rx_callback`
>>>> +of the registered mbox_client.
>>>> +
>>>> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
>>>> +   :identifiers: gh_msgq_init
>>>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>>>> index fc9376117111..5f929bb55e9a 100644
>>>> --- a/drivers/mailbox/Makefile
>>>> +++ b/drivers/mailbox/Makefile
>>>> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
>>>>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)    += zynqmp-ipi-mailbox.o
>>>> +obj-$(CONFIG_GUNYAH)        += gunyah-msgq.o
>>>
>>> Why are we reusing CONFIG_GUNYAH Kconfig symbol for mailbox, why not 
>>> CONFIG_GUNYAH_MBOX?
>>>
>>
>> There was some previous discussion about this:
>>
>> https://lore.kernel.org/all/2a7bb5f2-1286-b661-659a-a5037150eae8@quicinc.com/
>>
>>>> +
>>>>   obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
>>>>   obj-$(CONFIG_SPRD_MBOX)       += sprd-mailbox.o
>>>> diff --git a/drivers/mailbox/gunyah-msgq.c 
>>>> b/drivers/mailbox/gunyah-msgq.c
>>>> new file mode 100644
>>>> index 000000000000..03ffaa30ce9b
>>>> --- /dev/null
>>>> +++ b/drivers/mailbox/gunyah-msgq.c
>>>> @@ -0,0 +1,214 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>>>> rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/mailbox_controller.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/gunyah.h>
>>>> +#include <linux/printk.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/wait.h>
>>>
>>> ...
>>>
>>>> +/* Fired when message queue transitions from "full" to "space 
>>>> available" to send messages */
>>>> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
>>>> +{
>>>> +    struct gh_msgq *msgq = data;
>>>> +
>>>> +    mbox_chan_txdone(gh_msgq_chan(msgq), 0);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +/* Fired after sending message and hypercall told us there was more 
>>>> space available. */
>>>> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
>>>
>>> Tasklets have been long deprecated, consider using workqueues in this 
>>> particular case.
>>>
>>
>> Workqueues have higher latency and tasklets came as recommendation 
>> from Jassi. drivers/mailbox/imx-mailbox.c uses tasklets in the same way.
>>
>> I did some quick unscientific measurements of ~1000x samples. The 
>> median latency for resource manager went from 25.5 us (tasklet) to 26 
>> us (workqueue) (2% slower). The mean went from 28.7 us to 32.5 us (13% 
>> slower). Obviously, the outliers for workqueues were much more extreme.
> 
> TBH, this is expected because we are only testing resource manager, Note 
>   the advantage that you will see shifting from tasket to workqueues is 
> on overall system latencies and some drivers performance that need to 
> react to events.
> 
> please take some time to read this nice article about this 
> https://lwn.net/Articles/830964/
> 

Hmm, this article is from 2020 and there was another effort in 2007. 
Neither seems to have succeeded. I'd like to stick to same mechanisms as 
other mailbox controllers.

Jassi, do you have any preferences?

Thanks,
Elliot
  

On 23/02/2023 23:15, Elliot Berman wrote:
> 
> 
> On 2/23/2023 2:25 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 23/02/2023 00:15, Elliot Berman wrote:
>>>
>>>
>>> On 2/20/2023 5:59 AM, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 14/02/2023 21:23, Elliot Berman wrote:
>>>>> Gunyah message queues are a unidirectional inter-VM pipe for 
>>>>> messages up
>>>>> to 1024 bytes. This driver supports pairing a receiver message 
>>>>> queue and
>>>>> a transmitter message queue to expose a single mailbox channel.
>>>>>
>>>>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>>>>> ---
>>>>>   Documentation/virt/gunyah/message-queue.rst |   8 +
>>>>>   drivers/mailbox/Makefile                   |   2 +
>>>>>   drivers/mailbox/gunyah-msgq.c               | 214 
>>>>> ++++++++++++++++++++
>>>>>   include/linux/gunyah.h                      |  56 +++++
>>>>>   4 files changed, 280 insertions(+)
>>>>>   create mode 100644 drivers/mailbox/gunyah-msgq.c
>>>>>
>>>>> diff --git a/Documentation/virt/gunyah/message-queue.rst 
>>>>> b/Documentation/virt/gunyah/message-queue.rst
>>>>> index 0667b3eb1ff9..082085e981e0 100644
>>>>> --- a/Documentation/virt/gunyah/message-queue.rst
>>>>> +++ b/Documentation/virt/gunyah/message-queue.rst
>>>>> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs 
>>>>> (and two capability IDs).
>>>>>         |               |         |                 | 
>>>>> |               |
>>>>>         |               |         |                 | 
>>>>> |               |
>>>>>         +---------------+         +-----------------+ 
>>>>> +---------------+
>>>>> +
>>>>> +Gunyah message queues are exposed as mailboxes. To create the 
>>>>> mailbox, create
>>>>> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY 
>>>>> interrupt,
>>>>> +all messages in the RX message queue are read and pushed via the 
>>>>> `rx_callback`
>>>>> +of the registered mbox_client.
>>>>> +
>>>>> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
>>>>> +   :identifiers: gh_msgq_init
>>>>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>>>>> index fc9376117111..5f929bb55e9a 100644
>>>>> --- a/drivers/mailbox/Makefile
>>>>> +++ b/drivers/mailbox/Makefile
>>>>> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
>>>>>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)    += zynqmp-ipi-mailbox.o
>>>>> +obj-$(CONFIG_GUNYAH)        += gunyah-msgq.o
>>>>
>>>> Why are we reusing CONFIG_GUNYAH Kconfig symbol for mailbox, why not 
>>>> CONFIG_GUNYAH_MBOX?
>>>>
>>>
>>> There was some previous discussion about this:
>>>
>>> https://lore.kernel.org/all/2a7bb5f2-1286-b661-659a-a5037150eae8@quicinc.com/
>>>
>>>>> +
>>>>>   obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
>>>>>   obj-$(CONFIG_SPRD_MBOX)       += sprd-mailbox.o
>>>>> diff --git a/drivers/mailbox/gunyah-msgq.c 
>>>>> b/drivers/mailbox/gunyah-msgq.c
>>>>> new file mode 100644
>>>>> index 000000000000..03ffaa30ce9b
>>>>> --- /dev/null
>>>>> +++ b/drivers/mailbox/gunyah-msgq.c
>>>>> @@ -0,0 +1,214 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +/*
>>>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>>>>> rights reserved.
>>>>> + */
>>>>> +
>>>>> +#include <linux/mailbox_controller.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/interrupt.h>
>>>>> +#include <linux/gunyah.h>
>>>>> +#include <linux/printk.h>
>>>>> +#include <linux/init.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/wait.h>
>>>>
>>>> ...
>>>>
>>>>> +/* Fired when message queue transitions from "full" to "space 
>>>>> available" to send messages */
>>>>> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
>>>>> +{
>>>>> +    struct gh_msgq *msgq = data;
>>>>> +
>>>>> +    mbox_chan_txdone(gh_msgq_chan(msgq), 0);
>>>>> +
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +/* Fired after sending message and hypercall told us there was 
>>>>> more space available. */
>>>>> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
>>>>
>>>> Tasklets have been long deprecated, consider using workqueues in 
>>>> this particular case.
>>>>
>>>
>>> Workqueues have higher latency and tasklets came as recommendation 
>>> from Jassi. drivers/mailbox/imx-mailbox.c uses tasklets in the same way.
>>>
>>> I did some quick unscientific measurements of ~1000x samples. The 
>>> median latency for resource manager went from 25.5 us (tasklet) to 26 
>>> us (workqueue) (2% slower). The mean went from 28.7 us to 32.5 us 
>>> (13% slower). Obviously, the outliers for workqueues were much more 
>>> extreme.
>>
>> TBH, this is expected because we are only testing resource manager, 
>> Note   the advantage that you will see shifting from tasket to 
>> workqueues is on overall system latencies and some drivers performance 
>> that need to react to events.
>>
>> please take some time to read this nice article about this 
>> https://lwn.net/Articles/830964/
>>
> 
> Hmm, this article is from 2020 and there was another effort in 2007. 
> Neither seems to have succeeded. I'd like to stick to same mechanisms as 
> other mailbox controllers.

I don't want to block this series because of this. We will have more 
opportunity to improve this once some system wide profiling is done.

AFAIU, In this system we will have atleast 2 tasklets between VM and RM 
and 2 per inter-vm, so if the number of tasklets increase in the system 
will be potentially spending more time in soft irq handling it.

At somepoint in time its good to get some profiling done using 
bcc/softirqs to see how much time is spent on softirqs.


--srini

> 
> Jassi, do you have any preferences?
> 
> Thanks,
> Elliot
> 
>
  

On 2/23/2023 1:11 PM, Alex Elder wrote:
> On 2/14/23 3:23 PM, Elliot Berman wrote:
>> Gunyah message queues are a unidirectional inter-VM pipe for messages up
>> to 1024 bytes. This driver supports pairing a receiver message queue and
>> a transmitter message queue to expose a single mailbox channel.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   Documentation/virt/gunyah/message-queue.rst |   8 +
>>   drivers/mailbox/Makefile                    |   2 +
>>   drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
>>   include/linux/gunyah.h                      |  56 +++++
>>   4 files changed, 280 insertions(+)
>>   create mode 100644 drivers/mailbox/gunyah-msgq.c
>>
>> diff --git a/Documentation/virt/gunyah/message-queue.rst 
>> b/Documentation/virt/gunyah/message-queue.rst
>> index 0667b3eb1ff9..082085e981e0 100644
>> --- a/Documentation/virt/gunyah/message-queue.rst
>> +++ b/Documentation/virt/gunyah/message-queue.rst
>> @@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs 
>> (and two capability IDs).
>>         |               |         |                 |         
>> |               |
>>         |               |         |                 |         
>> |               |
>>         +---------------+         +-----------------+         
>> +---------------+
>> +
>> +Gunyah message queues are exposed as mailboxes. To create the 
>> mailbox, create
>> +a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY 
>> interrupt,
>> +all messages in the RX message queue are read and pushed via the 
>> `rx_callback`
>> +of the registered mbox_client.
>> +
>> +.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
>> +   :identifiers: gh_msgq_init
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index fc9376117111..5f929bb55e9a 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
>>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)    += zynqmp-ipi-mailbox.o
>> +obj-$(CONFIG_GUNYAH)        += gunyah-msgq.o
>> +
>>   obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
>>   obj-$(CONFIG_SPRD_MBOX)        += sprd-mailbox.o
>> diff --git a/drivers/mailbox/gunyah-msgq.c 
>> b/drivers/mailbox/gunyah-msgq.c
>> new file mode 100644
>> index 000000000000..03ffaa30ce9b
>> --- /dev/null
>> +++ b/drivers/mailbox/gunyah-msgq.c
> 
> You use a dash in this source file name, but an underscore
> everywhere else.  Unless there's a good reason to do this,
> please be consistent (use "gunyah_msgq.c").
> 
>> @@ -0,0 +1,214 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
>> rights reserved.
>> + */
>> +
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/gunyah.h>
>> +#include <linux/printk.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/wait.h>
>> +
>> +#define mbox_chan_to_msgq(chan) (container_of(chan->mbox, struct 
>> gh_msgq, mbox))
>> +
>> +static irqreturn_t gh_msgq_rx_irq_handler(int irq, void *data)
>> +{
>> +    struct gh_msgq *msgq = data;
>> +    struct gh_msgq_rx_data rx_data;
>> +    enum gh_error err;
>> +    bool ready = true;
>> +
>> +    while (ready) {
>> +        err = gh_hypercall_msgq_recv(msgq->rx_ghrsc->capid,
>> +                (uintptr_t)&rx_data.data, sizeof(rx_data.data),
>> +                &rx_data.length, &ready);
>> +        if (err != GH_ERROR_OK) {
>> +            if (err != GH_ERROR_MSGQUEUE_EMPTY)
> 
> Srini mentioned something about this too.  In many
> (all?) cases, there is a device pointer available,
> so you should use dev_*() functions rather than pr_*().
> 
> In this particular case, I'm not sure why/when the
> mbox.dev pointer would be null.  Also, dev_*() handles
> the case of a null device pointer, and it reports the
> device name (just as you do here).
> 
>> +                pr_warn("Failed to receive data from msgq for %s: %d\n",
>> +                    msgq->mbox.dev ? dev_name(msgq->mbox.dev) : "", 
>> err);
>> +            break;
>> +        }
>> +        mbox_chan_received_data(gh_msgq_chan(msgq), &rx_data);
>> +    }
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +/* Fired when message queue transitions from "full" to "space 
>> available" to send messages */
>> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
>> +{
>> +    struct gh_msgq *msgq = data;
>> +
>> +    mbox_chan_txdone(gh_msgq_chan(msgq), 0);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +/* Fired after sending message and hypercall told us there was more 
>> space available. */
>> +static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)
>> +{
>> +    struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, 
>> txdone_tasklet);
>> +
>> +    mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
>> +}
>> +
>> +static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +    struct gh_msgq *msgq = mbox_chan_to_msgq(chan);
>> +    struct gh_msgq_tx_data *msgq_data = data;
>> +    u64 tx_flags = 0;
>> +    enum gh_error gh_error;
> 
> Above you named the variable "err".  It helps readability
> if you use a very consistent naming convention for variables
> of a certain type when they are used a lot.
> 
>> +    bool ready;
>> +
>> +    if (msgq_data->push)
>> +        tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH;
>> +
>> +    gh_error = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, 
>> msgq_data->length,
>> +                    (uintptr_t)msgq_data->data, tx_flags, &ready);
>> +
>> +    /**
>> +     * unlikely because Linux tracks state of msgq and should not try to
>> +     * send message when msgq is full.
>> +     */
>> +    if (unlikely(gh_error == GH_ERROR_MSGQUEUE_FULL))
>> +        return -EAGAIN;
>> +
>> +    /**
>> +     * Propagate all other errors to client. If we return error to 
>> mailbox
>> +     * framework, then no other messages can be sent and nobody will 
>> know
>> +     * to retry this message.
>> +     */
>> +    msgq->last_ret = gh_remap_error(gh_error);
>> +
>> +    /**
>> +     * This message was successfully sent, but message queue isn't 
>> ready to
>> +     * receive more messages because it's now full. Mailbox framework
> 
> Maybe:  s/receive/accept/
> 
>> +     * requires that we only report that message was transmitted when
>> +     * we're ready to transmit another message. We'll get that in the 
>> form
>> +     * of tx IRQ once the other side starts to drain the msgq.
>> +     */
>> +    if (gh_error == GH_ERROR_OK && !ready)
>> +        return 0;
>> +
>> +    /**
>> +     * We can send more messages. Mailbox framework requires that tx 
>> done
>> +     * happens asynchronously to sending the message. Gunyah message 
>> queues
>> +     * tell us right away on the hypercall return whether we can send 
>> more
>> +     * messages. To work around this, defer the txdone to a tasklet.
>> +     */
>> +    tasklet_schedule(&msgq->txdone_tasklet);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct mbox_chan_ops gh_msgq_ops = {
>> +    .send_data = gh_msgq_send_data,
>> +};
>> +
>> +/**
>> + * gh_msgq_init() - Initialize a Gunyah message queue with an 
>> mbox_client
>> + * @parent: optional, device parent used for the mailbox controller
>> + * @msgq: Pointer to the gh_msgq to initialize
>> + * @cl: A mailbox client to bind to the mailbox channel that the 
>> message queue creates
>> + * @tx_ghrsc: optional, the transmission side of the message queue
>> + * @rx_ghrsc: optional, the receiving side of the message queue
>> + *
>> + * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most 
>> message queue use cases come with
> 
> s/should be/must be/
> 
>> + * a pair of message queues to facilitate bidirectional 
>> communication. When tx_ghrsc is set,
>> + * the client can send messages with 
>> mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
>> + * is set, the mbox_client should register an .rx_callback() and the 
>> message queue driver will
> 
> s/should register/must register/
> 
> A general comment on this code is that you sort of half define
> a Gunyah message queue API.  You define an initialization
> function and an exit function, but you also expose the fact
> that you use the mailbox framework in implementation.  This
> despite avoiding defining it as an mbox in the DTS file.
> 
> It might be hard to avoid that I guess.  But to me it would be
> nice if there were a more distinct Gunyah message queue API,
> which would provide a send_message() function, for example.
> And in that case, perhaps you would pass in the tx_done and/or
> rx_data callbacks to this function (since they're required).

I can write a wrapper for send_message, but I think it limits the code 
re-use of mailbox framework.

> 
> All that said, this is (currently?) only used by the resource
> manager, so making a beautiful API might not be that important.
> Do you envision this being used to communicate with other VMs
> in the future?
> 
>> + * push all available messages upon receiving the RX ready interrupt. 
>> The messages should be
> 
> Maybe: s/push/deliver/
> 
>> + * consumed or copied by the client right away as the gh_msgq_rx_data 
>> will be replaced/destroyed
>> + * after the callback.
>> + *
>> + * Returns - 0 on success, negative otherwise
>> + */
>> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct 
>> mbox_client *cl,
>> +             struct gunyah_resource *tx_ghrsc, struct gunyah_resource 
>> *rx_ghrsc)
>> +{
>> +    int ret;
>> +
>> +    /* Must have at least a tx_ghrsc or rx_ghrsc and that they are 
>> the right device types */
>> +    if ((!tx_ghrsc && !rx_ghrsc) ||
>> +        (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
>> +        (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
>> +        return -EINVAL;
>> +
>> +    if (gh_api_version() != GUNYAH_API_V1) {
>> +        pr_err("Unrecognized gunyah version: %u. Currently supported: 
>> %d\n",
>> +            gh_api_version(), GUNYAH_API_V1);
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    if (!gh_api_has_feature(GH_API_FEATURE_MSGQUEUE))
>> +        return -EOPNOTSUPP;
> 
> Can Gunyah even function if it doesn't have the MSGQUEUE feature?
> Will there ever be a Gunyah implementation that does not support
> it?  Perhaps this test could be done in gunyah_init() instead.

I don't think we will ever have a Gunyah implementation that doesn't 
support message queues. Perhaps some long distant Gunyah will use IPC 
mechanism X instead of message queues and the message queue support is 
dropped.

> 
> For that matter, you could verify the result of gh_api_version()
> at that time also.
> 

Moved the gh_api_version() check to gunyah_init()

>> +
>> +    msgq->tx_ghrsc = tx_ghrsc;
>> +    msgq->rx_ghrsc = rx_ghrsc;
>> +
>> +    msgq->mbox.dev = parent;
>> +    msgq->mbox.ops = &gh_msgq_ops;
>> +    msgq->mbox.num_chans = 1;
>> +    msgq->mbox.txdone_irq = true;
>> +    msgq->mbox.chans = kcalloc(msgq->mbox.num_chans, 
>> sizeof(*msgq->mbox.chans), GFP_KERNEL);
> 
>  From what I can tell, you will always use exactly one mailbox channel.
> So you could just do kzalloc(sizeof()...).
> 

If it's all the same, I'd like to keep it as kcalloc because chans is 
expected to be an array with num_chans size. It seems more correct to 
use kcalloc.

>> +    if (!msgq->mbox.chans)
>> +        return -ENOMEM;
>> +
>> +    if (msgq->tx_ghrsc) {
> 
>      if (tx_ghrsc) {
> 
> The irq field is assumed to be valid.  Are there any
> sanity checks you could perform?  Again this is only
> used for the resource manager right now, so maybe
> it's OK.
> 

We should safely assume irq field is valid. If we need to be skeptical 
of irq, we'd also need to be skeptical of capid and there's not validity 
check to perform there. struct gunyah_resource's are either filled from 
DT (in this case) or would be created by resource manager which does 
validity checks.

>> +        ret = request_irq(msgq->tx_ghrsc->irq, 
>> gh_msgq_tx_irq_handler, 0, "gh_msgq_tx",
> 
>          ret = request_irq(tx_ghrsc->irq, ...
> 
> 
>> +                msgq);
>> +        if (ret)
>> +            goto err_chans;
>> +    }
>> +
>> +    if (msgq->rx_ghrsc) {
>> +        ret = request_threaded_irq(msgq->rx_ghrsc->irq, NULL, 
>> gh_msgq_rx_irq_handler,
>> +                        IRQF_ONESHOT, "gh_msgq_rx", msgq);
>> +        if (ret)
>> +            goto err_tx_irq;
>> +    }
>> +
>> +    tasklet_setup(&msgq->txdone_tasklet, gh_msgq_txdone_tasklet);
>> +
>> +    ret = mbox_controller_register(&msgq->mbox);
>> +    if (ret)
>> +        goto err_rx_irq;
>> +
>> +    ret = mbox_bind_client(gh_msgq_chan(msgq), cl);
> 
> 
>> +    if (ret)
>> +        goto err_mbox;
>> +
>> +    return 0;
>> +err_mbox:
>> +    mbox_controller_unregister(&msgq->mbox);
>> +err_rx_irq:
>> +    if (msgq->rx_ghrsc)
>> +        free_irq(msgq->rx_ghrsc->irq, msgq);
>> +err_tx_irq:
>> +    if (msgq->tx_ghrsc)
>> +        free_irq(msgq->tx_ghrsc->irq, msgq);
>> +err_chans:
>> +    kfree(msgq->mbox.chans);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(gh_msgq_init);
>> +
>> +void gh_msgq_remove(struct gh_msgq *msgq)
>> +{
> 
> Is there any need to un-bind the client?
> 

I was leaving un-binding the client to the client (RM).

>> +    mbox_controller_unregister(&msgq->mbox);
>> +
>> +    if (msgq->rx_ghrsc)
>> +        free_irq(msgq->rx_ghrsc->irq, msgq);
>> +
>> +    if (msgq->tx_ghrsc)
>> +        free_irq(msgq->tx_ghrsc->irq, msgq);
>> +
>> +    kfree(msgq->mbox.chans);
>> +}
>> +EXPORT_SYMBOL_GPL(gh_msgq_remove);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Gunyah Message Queue Driver");
>> diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h
>> index cb6df4eec5c2..2e13669c6363 100644
>> --- a/include/linux/gunyah.h
>> +++ b/include/linux/gunyah.h
>> @@ -8,11 +8,67 @@
>>   #include <linux/bitfield.h>
>>   #include <linux/errno.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/limits.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/mailbox_client.h>
>>   #include <linux/types.h>
>> +/* Follows resource manager's resource types for VM_GET_HYP_RESOURCES */
>> +enum gunyah_resource_type {
>> +    GUNYAH_RESOURCE_TYPE_BELL_TX    = 0,
>> +    GUNYAH_RESOURCE_TYPE_BELL_RX    = 1,
>> +    GUNYAH_RESOURCE_TYPE_MSGQ_TX    = 2,
>> +    GUNYAH_RESOURCE_TYPE_MSGQ_RX    = 3,
>> +    GUNYAH_RESOURCE_TYPE_VCPU    = 4,
> 
> The maximum value here must fit in 8 bits.  I guess
> there's no risk right now of using that up, but you
> use negative values in some cases elsewhere.
> 
>> +};
>> +
>> +struct gunyah_resource {
>> +    enum gunyah_resource_type type;
>> +    u64 capid;
>> +    int irq;
> 
> request_irq() defines the IRQ value to be an unsigned int.
> 

Done.

>> +};
>> +
>> +/**
>> + * Gunyah Message Queues
>> + */
>> +
>> +#define GH_MSGQ_MAX_MSG_SIZE    240
>> +
>> +struct gh_msgq_tx_data {
>> +    size_t length;
>> +    bool push;
>> +    char data[];
>> +};
>> +
>> +struct gh_msgq_rx_data {
>> +    size_t length;
>> +    char data[GH_MSGQ_MAX_MSG_SIZE];
>> +};
>> +
>> +struct gh_msgq {
>> +    struct gunyah_resource *tx_ghrsc;
>> +    struct gunyah_resource *rx_ghrsc;
>> +
>> +    /* msgq private */
>> +    int last_ret; /* Linux error, not GH_STATUS_* */
>> +    struct mbox_controller mbox;
>> +    struct tasklet_struct txdone_tasklet;
> 
> Can the msgq_client be embedded here too?  (I don't really
> know whether msgq and msgq_client are one-to one.)
> 

They are one-to-one. I can embed the struct in the struct gh_msgq and 
drop the kcalloc.

Thanks,
Elliot

>> +};
>> +
>> +
>> +int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct 
>> mbox_client *cl,
>> +             struct gunyah_resource *tx_ghrsc, struct gunyah_resource 
>> *rx_ghrsc);
>> +void gh_msgq_remove(struct gh_msgq *msgq);
> 
> I suggested:
> 
> int gh_msgq_send(struct gh_msgq, struct gh_msgq_tx_data *data);
> 
>                      -Alex
> 
>> +
>> +static inline struct mbox_chan *gh_msgq_chan(struct gh_msgq *msgq)
>> +{
>> +    return &msgq->mbox.chans[0];
>> +}
>> +
>>   
>> /******************************************************************************/
>>   /* Common arch-independent definitions for Gunyah 
>> hypercalls                  */
>> +
>>   #define GH_CAPID_INVAL    U64_MAX
>>   #define GH_VMID_ROOT_VM    0xff
>