diff mbox series

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

Message ID 20240411-ffa_npi_support-v2-1-927a670254e6@arm.com (mailing list archive)
State New, archived
Headers show
Series firmware: arm_ffa: Add support to run inside a guest/VM under a hypervisor | expand

Commit Message

Sudeep Holla April 11, 2024, 12:57 p.m. UTC 
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
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.

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(-)

Comments

Sebastian Ene April 12, 2024, 10:42 a.m. UTC | #1 
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
Sudeep Holla April 12, 2024, 11:31 a.m. UTC | #2 
On Fri, Apr 12, 2024 at 10:42:07AM +0000, Sebastian Ene wrote:
> 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).

The hypervisor has to IIUC from the spec as it is responsible for creating
the bitmaps on its behalf. It can trap and handle those calls if it prefers
to fail for FFA_FEATURES(FFA_NOTIFICATION_BITMAP_CREATE).

> > 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.
>

Sure, in such case pKVM can always trap and fail for FFA_VERSION call so
that no further FFA transactions happen from that guest or just fail for
notification APIs if the notifications alone are not needed.

Hope that clarifies.

> > 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(..) 
>

Sure I can fixup when I apply.
diff mbox series

Patch

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);
+			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) {