[PATCH] firmware: arm_ffa: support running as a guest in a vm
Jens Wiklander
jens.wiklander at linaro.org
Tue Mar 19 10:26:57 PDT 2024
On Tue, Mar 19, 2024 at 3:20 PM Sudeep Holla <sudeep.holla at 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 at 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 at 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
More information about the linux-arm-kernel
mailing list