Received-SPF: pass (google.com: domain of linux-kernel+bounces-95871-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 7 Mar 2024 08:41:14 -0800
From: Elliot Berman <quic_eberman@quicinc.com>
To: Srivatsa Vaddagiri <quic_svaddagi@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: Re: [PATCH v17 07/35] gunyah: rsc_mgr: Add resource manager RPC
 core
Message-ID: <20240307083827092-0800.eberman@hu-eberman-lv.qualcomm.com>
Mail-Followup-To: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>, 
	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
References: <20240222-gunyah-v17-0-1e9da6763d38@quicinc.com>
 <20240222-gunyah-v17-7-1e9da6763d38@quicinc.com>
 <20240307153843.GC1695516@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240307153843.GC1695516@quicinc.com>

On Thu, Mar 07, 2024 at 09:08:43PM +0530, Srivatsa Vaddagiri wrote:
> * 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?
> 

There might be more messages to read, which we would not otherwise read.
Since those messages might be parseable, I'd rather try to recover than
break communication with RM.

As you mention, we should never encounter this condition. The guard is
to avoid reading garbage values.

> > +		}
> > +
> > +		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;
> > +}