From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DB77CC76196 for ; Fri, 31 Mar 2023 14:29:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:References:Cc:To:Subject:From: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Yjr60RcW2ktczdV3yWOaIvSMoxwXLtRHtsDWI2XfmFY=; b=11R2E4y5SHFw8G 5KFsw+lPHN+AsSCO5Hg4yMm4eyfZi1ywEn6KtkjNozET3InRUmKD3JnyW/7fpRZVTbdvDLPBZEHCJ 9v8beT9ZO9gJuj1VuqTjy5/Ki8rn1qEGW5tqL4p7Ubas/81ApqpPLVtDnF5j/UUtXo73aU8ScTFje MrHiisp8T0T72i8nVn9dzt84qFXyzgfPLopkyOLbvzIMIqZO5+IVGdIkavUqrdKRNV9g1u5T8Uajz 9+QibPkaJ/8Jd7x2RLgrJni0DbsVUdTXRb5Whqr+JZX8B1m6e0m4T0RVyCUIfjb+teHlh9zeUk6Ms qlrlY6ZvMALOF2aHcRYw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1piFjU-007hFC-39; Fri, 31 Mar 2023 14:28:01 +0000 Received: from mail-il1-x132.google.com ([2607:f8b0:4864:20::132]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1piFh6-007gHa-2z for linux-arm-kernel@lists.infradead.org; Fri, 31 Mar 2023 14:25:35 +0000 Received: by mail-il1-x132.google.com with SMTP id n1so10849969ili.10 for ; Fri, 31 Mar 2023 07:25:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1680272732; x=1682864732; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=Qj34PtqNG7Rnq3eSaDacYkRFKU4FhTPAc7mbt6SsSe4=; b=FOfS4QWQqm0/TyrdUMHKahiUjW63wa6X4L4MPcKI/iv71zBrToel9pj+7W+7HAla9Z hjd/2HCUAmOKmQXtmWE5mIQlSvTJg8TNRR6uJD/187Pk6Q9T20AfbB4OwhuWuGE8O+UL 1qnGmBsz2IPY8X8LArrr1cnsjuWNZByDptMNVlzxgTIyKsUajlZ0zWXaihvbOTblGOS/ IguLdCtKqQkJBIMDpgh4NQ3oE+HVor2ZkJF8lYYWy8AYsDjxHFnilHiWzH/teyQLvFYd MuRyVol0jYl6OsuySGpF8axCX34bOtTytvUeUSXIsEjDA9UYLzNTRDUvoVZBbghXPuU3 9eaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680272732; x=1682864732; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Qj34PtqNG7Rnq3eSaDacYkRFKU4FhTPAc7mbt6SsSe4=; b=ae5VDvS9tw24m4ZyPMgvLmmJu9weuuUBX2pEQ6LG7DQCHI5rv0xlWC1Ge2GgFsDs/U 3sIX19xOEXvYBCM4UQky73DXHUVWf39psU0rEsQXudHGSPOgsTeNbc/i7IcwCvsPfVxI uDzYDxFNqBZiLUpj5gwWl/3AhR4QM8ax/u1PPBIAgaFFAS2Du34FJKyWi0v7F53kvQOy llw+T0UgkF+u+Pm+J1xtKWz+6vTTFI6pSBddsCd+cXPteYCmBlr4cO8N/yBZ9xCg42/G tuplZ6bTP5bUJ6usZcu6HmpK37RAj2+4R5Gi9DZ9i7ttRYGcz05I4aqne+UGBuP9lTEQ S0EQ== X-Gm-Message-State: AAQBX9enJt/mH/U6BK8hKRQmHQwezdDq8Ry8hGgxIkRfPfXBO6QrdreX alEXSX1lAoeR7Q7pTGTQ6umLOw== X-Google-Smtp-Source: AKy350Z8Vx20bXEGM5KbNCs7dvf4pmdF3e5OyS2cQdNspxn8c8ZNkOGmtwRJEI4xx6+qQgszQfGQQg== X-Received: by 2002:a92:cc06:0:b0:325:bb3d:4f7 with SMTP id s6-20020a92cc06000000b00325bb3d04f7mr18256639ilp.1.1680272732136; Fri, 31 Mar 2023 07:25:32 -0700 (PDT) Received: from [172.22.22.4] ([98.61.227.136]) by smtp.googlemail.com with ESMTPSA id y13-20020a927d0d000000b00313f1b861b7sm433413ilc.51.2023.03.31.07.25.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 31 Mar 2023 07:25:31 -0700 (PDT) Message-ID: <02ab8928-a34a-f034-d123-b0319edf1c6f@linaro.org> Date: Fri, 31 Mar 2023 09:25:29 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 From: Alex Elder Subject: Re: [PATCH v11 08/26] gunyah: rsc_mgr: Add resource manager RPC core To: Elliot Berman , Srinivas Kandagatla , Prakruthi Deepak Heragu Cc: Murali Nalajala , Trilok Soni , Srivatsa Vaddagiri , Carl van Schaik , Dmitry Baryshkov , Bjorn Andersson , Konrad Dybcio , Arnd Bergmann , Greg Kroah-Hartman , Rob Herring , Krzysztof Kozlowski , Jonathan Corbet , Bagas Sanjaya , Will Deacon , Andy Gross , Catalin Marinas , Jassi Brar , linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <20230304010632.2127470-1-quic_eberman@quicinc.com> <20230304010632.2127470-9-quic_eberman@quicinc.com> Content-Language: en-US In-Reply-To: <20230304010632.2127470-9-quic_eberman@quicinc.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230331_072533_023046_5E7BB622 X-CRM114-Status: GOOD ( 42.68 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 3/3/23 7:06 PM, Elliot Berman wrote: > The resource manager is a special virtual machine which is always > running on a Gunyah system. It provides APIs for creating and destroying > VMs, secure memory management, sharing/lending of memory between VMs, > and setup of inter-VM communication. Calls to the resource manager are > made via message queues. > > This patch implements the basic probing and RPC mechanism to make those > API calls. Request/response calls can be made with gh_rm_call. > Drivers can also register to notifications pushed by RM via > gh_rm_register_notifier > > Specific API calls that resource manager supports will be implemented in > subsequent patches. Mostly very simple issues noted here. -Alex > Signed-off-by: Elliot Berman > --- > drivers/virt/gunyah/Makefile | 3 + > drivers/virt/gunyah/rsc_mgr.c | 688 +++++++++++++++++++++++++++++++++ > drivers/virt/gunyah/rsc_mgr.h | 16 + > include/linux/gunyah_rsc_mgr.h | 21 + > 4 files changed, 728 insertions(+) > create mode 100644 drivers/virt/gunyah/rsc_mgr.c > create mode 100644 drivers/virt/gunyah/rsc_mgr.h > create mode 100644 include/linux/gunyah_rsc_mgr.h > > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile > index 34f32110faf9..cc864ff5abbb 100644 > --- a/drivers/virt/gunyah/Makefile > +++ b/drivers/virt/gunyah/Makefile > @@ -1,3 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_GUNYAH) += gunyah.o > + > +gunyah_rsc_mgr-y += rsc_mgr.o > +obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o > diff --git a/drivers/virt/gunyah/rsc_mgr.c b/drivers/virt/gunyah/rsc_mgr.c > new file mode 100644 > index 000000000000..67813c9a52db > --- /dev/null > +++ b/drivers/virt/gunyah/rsc_mgr.c > @@ -0,0 +1,688 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + . . . > +static void gh_rm_try_complete_connection(struct gh_rm *rm) > +{ > + struct gh_rm_connection *connection = rm->active_rx_connection; > + > + if (!connection || connection->fragments_received != connection->num_fragments) > + return; > + > + switch (connection->type) { > + case RM_RPC_TYPE_REPLY: > + complete(&connection->reply.seq_done); > + break; > + case RM_RPC_TYPE_NOTIF: > + schedule_work(&connection->notification.work); > + break; > + default: > + dev_err_ratelimited(rm->dev, "Invalid message type (%d) received\n", s/%d/%u/ > + connection->type); > + gh_rm_abort_connection(rm); > + break; > + } > + > + rm->active_rx_connection = NULL; > +} > + > +static void gh_rm_msgq_rx_data(struct mbox_client *cl, void *mssg) > +{ > + struct gh_rm *rm = container_of(cl, struct gh_rm, msgq_client); > + struct gh_msgq_rx_data *rx_data = mssg; > + size_t msg_size = rx_data->length; > + void *msg = rx_data->data; > + struct gh_rm_rpc_hdr *hdr; > + > + if (msg_size < sizeof(*hdr) || msg_size > GH_MSGQ_MAX_MSG_SIZE) > + return; > + > + hdr = msg; > + if (hdr->api != RM_RPC_API) { > + dev_err(rm->dev, "Unknown RM RPC API version: %x\n", hdr->api); > + return; > + } > + > + switch (FIELD_GET(RM_RPC_TYPE_MASK, hdr->type)) { > + case RM_RPC_TYPE_NOTIF: > + gh_rm_process_notif(rm, msg, msg_size); > + break; > + case RM_RPC_TYPE_REPLY: > + gh_rm_process_rply(rm, msg, msg_size); > + break; > + case RM_RPC_TYPE_CONTINUATION: > + gh_rm_process_cont(rm, rm->active_rx_connection, msg, msg_size); > + break; > + default: > + dev_err(rm->dev, "Invalid message type (%lu) received\n", > + FIELD_GET(RM_RPC_TYPE_MASK, hdr->type)); > + return; > + } > + > + gh_rm_try_complete_connection(rm); > +} > + > +static void gh_rm_msgq_tx_done(struct mbox_client *cl, void *mssg, int r) > +{ > + struct gh_rm *rm = container_of(cl, struct gh_rm, msgq_client); > + > + kmem_cache_free(rm->cache, mssg); > + rm->last_tx_ret = r; > +} > + > +static int gh_rm_send_request(struct gh_rm *rm, u32 message_id, > + const void *req_buff, size_t req_buf_size, > + struct gh_rm_connection *connection) > +{ > + size_t buf_size_remaining = req_buf_size; > + const void *req_buf_curr = req_buff; > + struct gh_msgq_tx_data *msg; > + struct gh_rm_rpc_hdr *hdr, hdr_template; > + u32 cont_fragments = 0; > + size_t payload_size; > + void *payload; > + int ret; > + > + if (req_buf_size > GH_RM_MAX_NUM_FRAGMENTS * GH_RM_MAX_MSG_SIZE) { > + dev_warn(rm->dev, "Limit exceeded for the number of fragments: %u\n", > + cont_fragments); You are printing the value of cont_fragments here when it's just zero. > + dump_stack(); > + return -E2BIG; > + } > + Move the computation of cont_fragments prior to the block above. You could use a ?: statement to assign it. > + if (req_buf_size) > + cont_fragments = (req_buf_size - 1) / GH_RM_MAX_MSG_SIZE; > + > + hdr_template.api = RM_RPC_API; > + hdr_template.type = FIELD_PREP(RM_RPC_TYPE_MASK, RM_RPC_TYPE_REQUEST) | > + FIELD_PREP(RM_RPC_FRAGMENTS_MASK, cont_fragments); The line above should be indented further. > + hdr_template.seq = cpu_to_le16(connection->reply.seq); > + hdr_template.msg_id = cpu_to_le32(message_id); > + > + ret = mutex_lock_interruptible(&rm->send_lock); > + if (ret) > + return ret; > + > + /* Consider also the 'request' packet for the loop count */ I don't think the comment above is helpful. > + do { > + msg = kmem_cache_zalloc(rm->cache, GFP_KERNEL); > + if (!msg) { > + ret = -ENOMEM; > + goto out; > + } > + > + /* Fill header */ > + hdr = (struct gh_rm_rpc_hdr *)msg->data; I personally would prefer &msg->data[0] in this case. > + *hdr = hdr_template; > + > + /* Copy payload */ > + payload = hdr + 1; I think I might have suggested using "hdr + 1" here. Elsewhere you use something like: payload = (char *)hdr + sizeof(hdr); or something similar. I suggest you choose one approach and use it consistently througout the driver. Either is fine, but I have a slight preference for the "hdr + 1" way. > + payload_size = min(buf_size_remaining, GH_RM_MAX_MSG_SIZE); > + memcpy(payload, req_buf_curr, payload_size); > + req_buf_curr += payload_size; > + buf_size_remaining -= payload_size; > + > + /* Force the last fragment to immediately alert the receiver */ > + msg->push = !buf_size_remaining; > + msg->length = sizeof(*hdr) + payload_size; > + > + ret = mbox_send_message(gh_msgq_chan(&rm->msgq), msg); > + if (ret < 0) { > + kmem_cache_free(rm->cache, msg); > + break; > + } > + > + if (rm->last_tx_ret) { > + ret = rm->last_tx_ret; > + break; > + } > + > + hdr_template.type = FIELD_PREP(RM_RPC_TYPE_MASK, RM_RPC_TYPE_CONTINUATION) | > + FIELD_PREP(RM_RPC_FRAGMENTS_MASK, cont_fragments); > + } while (buf_size_remaining); > + > +out: > + mutex_unlock(&rm->send_lock); > + return ret < 0 ? ret : 0; > +} > + > +/** > + * gh_rm_call: Achieve request-response type communication with RPC > + * @rm: Pointer to Gunyah resource manager internal data > + * @message_id: The RM RPC message-id > + * @req_buff: Request buffer that contains the payload > + * @req_buf_size: Total size of the payload > + * @resp_buf: Pointer to a response buffer > + * @resp_buf_size: Size of the response buffer > + * > + * Make a request to the RM-VM and wait for reply back. For a successful I think you could just say "to the RM and wait"... Overall I suggest using "RM" or "RM VM" consistently when you talk about the Resource Manager. This is the only place I see "RM-VM". > + * response, the function returns the payload. The size of the payload is set in > + * resp_buf_size. The resp_buf should be freed by the caller when 0 is returned s/should/must/ > + * and resp_buf_size != 0. > + * > + * req_buff should be not NULL for req_buf_size >0. If req_buf_size == 0, > + * req_buff *can* be NULL and no additional payload is sent. I'd say use "buf" or "buff" but not both in your naming convention. > + * > + * Context: Process context. Will sleep waiting for reply. > + * Return: 0 on success. <0 if error. > + */ > +int gh_rm_call(struct gh_rm *rm, u32 message_id, void *req_buff, size_t req_buf_size, > + void **resp_buf, size_t *resp_buf_size) I suspect you could define the request buffer as a pointer to const; can you? > +{ > + struct gh_rm_connection *connection; > + u32 seq_id; > + int ret; > + > + /* message_id 0 is reserved. req_buf_size implies req_buf is not NULL */ > + if (!message_id || (!req_buff && req_buf_size) || !rm) If you're going to check for a null RM pointer, I'd check it first. > + return -EINVAL; > + > + > + connection = kzalloc(sizeof(*connection), GFP_KERNEL); > + if (!connection) > + return -ENOMEM; > + > + connection->type = RM_RPC_TYPE_REPLY; > + connection->msg_id = cpu_to_le32(message_id); > + > + init_completion(&connection->reply.seq_done); . . . > diff --git a/include/linux/gunyah_rsc_mgr.h b/include/linux/gunyah_rsc_mgr.h > new file mode 100644 > index 000000000000..deca9b3da541 > --- /dev/null > +++ b/include/linux/gunyah_rsc_mgr.h > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _GUNYAH_RSC_MGR_H > +#define _GUNYAH_RSC_MGR_H > + > +#include > +#include > +#include > + > +#define GH_VMID_INVAL U16_MAX Add a tab before U16_MAX; it will line up more nicely when you define GH_MEM_HANDLE_INVAL later. > + > +struct gh_rm; > +int gh_rm_notifier_register(struct gh_rm *rm, struct notifier_block *nb); > +int gh_rm_notifier_unregister(struct gh_rm *rm, struct notifier_block *nb); > +struct device *gh_rm_get(struct gh_rm *rm); > +void gh_rm_put(struct gh_rm *rm); > + > +#endif _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel