Message-ID: <CAHUa44Har+TLBXOgz4EqekEu7fKWgFeCuOa0i8UiNLfen4tJiw@mail.gmail.com>
Date: Fri, 8 Mar 2024 13:35:05 +0100
From: Jens Wiklander <jens.wiklander@...aro.org>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	Sudeep Holla <sudeep.holla@....com>, Marc Bonnici <marc.bonnici@....com>, 
	Olivier Deprez <Olivier.Deprez@....com>
Subject: Re: [PATCH] firmware: arm_ffa: support running as a guest in a vm

On Fri, Mar 8, 2024 at 10:44 AM Lorenzo Pieralisi <lpieralisi@...nel.org> wrote:
>
> 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@...aro.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.

This patch was made from the assumption that this FF-A driver is a
guest driver, that is, FFA_NOTIFICATION_INFO_GET lands in the
Hypervisor at EL2. The FFA_NOTIFICATION_INFO_GET call is needed to
know which FFA_NOTIFICATION_GET calls should be dispatched in this VM,
to retrieve global notifications and per vCPU notifications.

If the FF-A driver is supposed to be a host driver instead, then I
wonder where we should have the guest driver.

For clarification, my setup has Xen as hypervisor at EL2 (doing the
host processing), TF-A as SPMD at EL3, and OP-TEE as SPMC at S-EL1.
I'm testing this on QEMU. I'm going to post the Xen patches relating
to this quite soon.

I believe that until now the FF-A driver has worked under the
assumption that it's a non-secure physical FF-A instance. With
hypervisor at EL2 it becomes a virtual FF-A instance.

>
> >       }
> > @@ -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 ?

No, this is about not aborting notification setup just because we have
a hypervisor that handles the FFA_NOTIFICATION_BITMAP_CREATE call.
With this patch, if ffa_get_notif_intid() fails, then we don't have
support for notifications.

Cheers,
Jens

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

Powered by blists - more mailing lists