Received-SPF: pass (google.com: domain of linux-kernel+bounces-96768-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 8 Mar 2024 10:44:40 +0100
From: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: Jens Wiklander <jens.wiklander@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	Marc Bonnici <marc.bonnici@arm.com>,
	Olivier Deprez <Olivier.Deprez@arm.com>
Subject: Re: [PATCH] firmware: arm_ffa: support running as a guest in a vm
Message-ID: <ZereCD7kJxP+vzHN@lpieralisi>
References: <20240307092132.943881-1-jens.wiklander@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240307092132.943881-1-jens.wiklander@linaro.org>

On Thu, Mar 07, 2024 at 10:21:32AM +0100, Jens Wiklander wrote:
> Add support for running the driver in a guest to a hypervisor. The main
> difference is that the notification interrupt is retrieved
> with FFA_FEAT_NOTIFICATION_PENDING_INT and that
> FFA_NOTIFICATION_BITMAP_CREATE doesn't need to be called.

I have a couple of questions about these changes, comments below.

> FFA_FEAT_NOTIFICATION_PENDING_INT gives the interrupt the hypervisor has
> chosen to notify its guest of pending notifications.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/firmware/arm_ffa/driver.c | 45 ++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index f2556a8e9401..c183c7d39c0f 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1306,17 +1306,28 @@ static void ffa_sched_recv_irq_work_fn(struct work_struct *work)
>  	ffa_notification_info_get();
>  }
>  
> +static int ffa_get_notif_intid(int *intid)
> +{
> +	int ret;
> +
> +	ret = ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, intid, NULL);
> +	if (!ret)
> +		return 0;
> +	ret = ffa_features(FFA_FEAT_NOTIFICATION_PENDING_INT, 0, intid, NULL);
> +	if (!ret)
> +		return 0;

I think that both interrupts should be probed in eg a host and the
actions their handlers should take are different.

> +
> +	pr_err("Failed to retrieve one of scheduler Rx or notif pending interrupts\n");
> +	return ret;
> +}
> +
>  static int ffa_sched_recv_irq_map(void)
>  {
> -	int ret, irq, sr_intid;
> +	int ret, irq, intid;
>  
> -	/* The returned sr_intid is assumed to be SGI donated to NS world */
> -	ret = ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, &sr_intid, NULL);
> -	if (ret < 0) {
> -		if (ret != -EOPNOTSUPP)
> -			pr_err("Failed to retrieve scheduler Rx interrupt\n");
> +	ret = ffa_get_notif_intid(&intid);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	if (acpi_disabled) {
>  		struct of_phandle_args oirq = {};
> @@ -1329,12 +1340,12 @@ static int ffa_sched_recv_irq_map(void)
>  
>  		oirq.np = gic;
>  		oirq.args_count = 1;
> -		oirq.args[0] = sr_intid;
> +		oirq.args[0] = intid;
>  		irq = irq_create_of_mapping(&oirq);
>  		of_node_put(gic);
>  #ifdef CONFIG_ACPI
>  	} else {
> -		irq = acpi_register_gsi(NULL, sr_intid, ACPI_EDGE_SENSITIVE,
> +		irq = acpi_register_gsi(NULL, intid, ACPI_EDGE_SENSITIVE,
>  					ACPI_ACTIVE_HIGH);
>  #endif

This means that for both schedule receiver interrupt and notification
pending interrupt we would end up calling FFA_NOTIFICATION_INFO_GET (?),
which is not correct AFAIK, for many reasons.

If there is a pending notification for a VM, a scheduler receiver
interrupt is triggered in the host. This would end up calling
FFA_NOTIFICATION_INFO_GET(), that is destructive (calling it again in
the notified *guest* - in the interrupt handler triggered by the
hypervisor - would not return the pending notifications for it).

Therefore, the action for the pending notification interrupt should
be different and should just call FFA_NOTIFICATION_GET.

Please let me know if that matches your understanding, this
hunk is unclear to me.

>  	}
> @@ -1442,17 +1453,15 @@ static void ffa_notifications_setup(void)
>  	int ret, irq;
>  
>  	ret = ffa_features(FFA_NOTIFICATION_BITMAP_CREATE, 0, NULL, NULL);
> -	if (ret) {
> -		pr_info("Notifications not supported, continuing with it ..\n");
> -		return;
> -	}
> +	if (!ret) {
>  
> -	ret = ffa_notification_bitmap_create();
> -	if (ret) {
> -		pr_info("Notification bitmap create error %d\n", ret);
> -		return;
> +		ret = ffa_notification_bitmap_create();
> +		if (ret) {
> +			pr_err("notification_bitmap_create error %d\n", ret);
> +			return;
> +		}
> +		drv_info->bitmap_created = true;
>  	}
> -	drv_info->bitmap_created = true;

This boils down to saying that FFA_NOTIFICATION_BITMAP_CREATE is not
implemented for a VM (because the hypervisor should take care of issuing
that call before the VM is created), so if the feature is not present
that does not mean that notifications aren't supported.

It is just about removing a spurious log.

Is that correct ?

Thanks,
Lorenzo

>  
>  	irq = ffa_sched_recv_irq_map();
>  	if (irq <= 0) {
> -- 
> 2.34.1
>