Re: [PATCH v2 1/3] firmware: arm_ffa: Skip creation of the notification bitmaps

From: Sebastian Ene <sebastianene@google.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Marc Bonnici <marc.bonnici@arm.com>,
	Olivier Deprez <Olivier.Deprez@arm.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Jens Wiklander <jens.wiklander@linaro.org>
Subject: Re: [PATCH v2 1/3] firmware: arm_ffa: Skip creation of the notification bitmaps
Date: Fri, 12 Apr 2024 10:42:07 +0000	[thread overview]
Message-ID: <ZhkP__aSXl0wbazy@google.com> (raw)
In-Reply-To: <20240411-ffa_npi_support-v2-1-927a670254e6@arm.com>

On Thu, Apr 11, 2024 at 01:57:32PM +0100, Sudeep Holla wrote:
> From: Jens Wiklander <jens.wiklander@linaro.org>
> 
> When the FF-A driver is running inside a guest VM under an hypervisor,
> the driver/guest VM doesn't have the permission/capability to request
> the creation of notification bitmaps. For a VM, the hypervisor reserves

Who restrains them from having this capability ? (to use the
FFA_NOTIFICATION_BITMAP_CREATE API).

> memory for its VM and hypervisor framework notification bitmaps and the
> SPMC reserves memory for its SP and SPMC framework notification bitmaps
> before the hypervisor initializes it.
> 
> The hypervisor does not initialize a VM if memory cannot be reserved
> for all its notification bitmaps. So the creation of all the necessary
> bitmaps are already done when the driver initialises and hence it can be
> skipped. We rely on FFA_FEATURES(FFA_NOTIFICATION_BITMAP_CREATE) to fail
> when running in the guest to handle this in the driver.
> 

This implies that the hypervisor reserves memory in advance to keep the
notification bitmaps even though the guest might decide not to use them.
For pKVM this assumption does not work because we might have guests that will not be
allowed to talk to TZ. 

> Signed-off-by:Jens Wiklander <jens.wiklander@linaro.org>
> [sudeep.holla: Updated the commit message]
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index f2556a8e9401..4a576af7b8fd 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1442,17 +1442,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);

Not a big deal but shouldn't this be a pr_err(..) 

Thanks,
Seb

> +			return;
> +		}
>  
> -	ret = ffa_notification_bitmap_create();
> -	if (ret) {
> -		pr_info("Notification bitmap create error %d\n", ret);
> -		return;
> +		drv_info->bitmap_created = true;
>  	}
> -	drv_info->bitmap_created = true;
>  
>  	irq = ffa_sched_recv_irq_map();
>  	if (irq <= 0) {
> 
> -- 
> 2.43.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel