Re: [PATCH v17 07/35] gunyah: rsc_mgr: Add resource manager RPC core

From: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
To: Elliot Berman <quic_eberman@quicinc.com>
Cc: Alex Elder <elder@linaro.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Murali Nalajal <quic_mnalajal@quicinc.com>,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Carl van Schaik <quic_cvanscha@quicinc.com>,
	Philip Derrin <quic_pderrin@quicinc.com>,
	Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	"Fuad Tabba" <tabba@google.com>,
	Sean Christopherson <seanjc@google.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	<linux-arm-msm@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linux-mm@kvack.org>
Subject: Re: [PATCH v17 07/35] gunyah: rsc_mgr: Add resource manager RPC core
Date: Thu, 7 Mar 2024 21:08:43 +0530	[thread overview]
Message-ID: <20240307153843.GC1695516@quicinc.com> (raw)
In-Reply-To: <20240222-gunyah-v17-7-1e9da6763d38@quicinc.com>

* Elliot Berman <quic_eberman@quicinc.com> [2024-02-22 15:16:30]:

> 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.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>

Left a minor comment below. LGTM otherwise.

Reviewed-by: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>

> +static irqreturn_t gunyah_rm_rx(int irq, void *data)
> +{
> +	enum gunyah_error gunyah_error;
> +	struct gunyah_rm_rpc_hdr *hdr;
> +	struct gunyah_rm *rm = data;
> +	void *msg = &rm->recv_msg[0];
> +	size_t len;
> +	bool ready;
> +
> +	do {
> +		gunyah_error = gunyah_hypercall_msgq_recv(rm->rx_ghrsc.capid,
> +							  msg,
> +							  sizeof(rm->recv_msg),
> +							  &len, &ready);
> +		if (gunyah_error != GUNYAH_ERROR_OK) {
> +			if (gunyah_error != GUNYAH_ERROR_MSGQUEUE_EMPTY)
> +				dev_warn(rm->dev,
> +					 "Failed to receive data: %d\n",
> +					 gunyah_error);
> +			return IRQ_HANDLED;
> +		}
> +
> +		if (len < sizeof(*hdr)) {
> +			dev_err_ratelimited(
> +				rm->dev,
> +				"Too small message received. size=%ld\n", len);
> +			continue;

In practice we should never hit this condition, in case we do encounter, do you
see a reason why continue is preferred over simply breaking the loop?

> +		}
> +
> +		hdr = msg;
> +		if (hdr->api != RM_RPC_API) {
> +			dev_err(rm->dev, "Unknown RM RPC API version: %x\n",
> +				hdr->api);
> +			return IRQ_HANDLED;
> +		}
> +
> +		switch (FIELD_GET(RM_RPC_TYPE_MASK, hdr->type)) {
> +		case RM_RPC_TYPE_NOTIF:
> +			gunyah_rm_process_notif(rm, msg, len);
> +			break;
> +		case RM_RPC_TYPE_REPLY:
> +			gunyah_rm_process_reply(rm, msg, len);
> +			break;
> +		case RM_RPC_TYPE_CONTINUATION:
> +			gunyah_rm_process_cont(rm, rm->active_rx_message, msg,
> +					       len);
> +			break;
> +		default:
> +			dev_err(rm->dev,
> +				"Invalid message type (%lu) received\n",
> +				FIELD_GET(RM_RPC_TYPE_MASK, hdr->type));
> +			return IRQ_HANDLED;
> +		}
> +	} while (ready);
> +
> +	return IRQ_HANDLED;
> +}