Re: [PATCH] firmware: arm_ffa: support running as a guest in a vm

From: Jens Wiklander <jens.wiklander@linaro.org>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>,
	linux-kernel@vger.kernel.org,
	 linux-arm-kernel@lists.infradead.org,
	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
Date: Tue, 19 Mar 2024 18:26:57 +0100	[thread overview]
Message-ID: <CAHUa44FVmK6im92cKTUshBG936xHAwS6nASfyBa9D2Yb44ansg@mail.gmail.com> (raw)
In-Reply-To: <ZfmfIN_ODYq9MvOq@bogus>

On Tue, Mar 19, 2024 at 3:20 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Mar 08, 2024 at 01:35:05PM +0100, Jens Wiklander wrote:
> > On Fri, Mar 8, 2024 at 10:44 AM Lorenzo Pieralisi <lpieralisi@kernel.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@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.
> > >
>
> +1, I have the same opinion.
>
> > > > +
> > > > +     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.
>
> As you can expect, the above matches my understanding too.
>
> >
> > 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.
> >
>
> OK and I assume this aligns with the below excerpts from the spec about
> FFA_NOTIFICATION_INFO_GET:
> "
> This ABI is invoked by a VM at the Non-secure virtual FF-A instance with the
> SMC or HVC conduits to request the Hypervisor to return the list of SPs and
> VMs that have pending notifications. The Hypervisor returns the list of those
> endpoints whose schedulers are implemented in the calling VM.
> "
>
> But if OPTEE driver in the VM/guest is the scheduler for the OPTEE SP,
> then I would expect the FF-A driver to just register for SRI. It can't be
> NPI as that contradicts with above.
>
> > If the FF-A driver is supposed to be a host driver instead, then I
> > wonder where we should have the guest driver.
> >
>
> At least so far we haven't found a strong reason to have different versions
> for each.
>
> > 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.
> >
>
> OK, thanks for the setup info.
>
> > 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.
> >
>
> Agreed.
>
> > >
> > > >       }
> > > > @@ -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.
>
> Understood.
>
> > With this patch, if ffa_get_notif_intid() fails, then we don't have
> > support for notifications.
> >
>
> But I still don't understand the mixup of SRI and NPI in your usecase
> model. It should be just SRI. NPI handler must just use NOTIFICATION_GET
> and not INFO_GET as Lorenzo has explained above.

Sorry for my confusion, I get your point now. I'll make a v2 where I
add an NPI handler and keep the SRI handler as you described.

Thanks,
Jens

>
> --
> Regards,
> Sudeep