Received-SPF: pass (google.com: domain of linux-kernel+bounces-107913-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240307092132.943881-1-jens.wiklander@linaro.org>
 <ZereCD7kJxP+vzHN@lpieralisi> <CAHUa44Har+TLBXOgz4EqekEu7fKWgFeCuOa0i8UiNLfen4tJiw@mail.gmail.com>
 <ZfmfIN_ODYq9MvOq@bogus>
In-Reply-To: <ZfmfIN_ODYq9MvOq@bogus>
From: Jens Wiklander <jens.wiklander@linaro.org>
Date: Tue, 19 Mar 2024 18:26:57 +0100
Message-ID: <CAHUa44FVmK6im92cKTUshBG936xHAwS6nASfyBa9D2Yb44ansg@mail.gmail.com>
Subject: Re: [PATCH] firmware: arm_ffa: support running as a guest in a vm
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 19, 2024 at 3:20=E2=80=AFPM 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=E2=80=AFAM Lorenzo Pieralisi <lpieralisi@k=
ernel.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 hyperviso=
r 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/a=
rm_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(stru=
ct work_struct *work)
> > > >       ffa_notification_info_get();
> > > >  }
> > > >
> > > > +static int ffa_get_notif_intid(int *intid)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret =3D ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, intid=
, NULL);
> > > > +     if (!ret)
> > > > +             return 0;
> > > > +     ret =3D ffa_features(FFA_FEAT_NOTIFICATION_PENDING_INT, 0, in=
tid, 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 pendi=
ng 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 w=
orld */
> > > > -     ret =3D ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, &sr_i=
ntid, NULL);
> > > > -     if (ret < 0) {
> > > > -             if (ret !=3D -EOPNOTSUPP)
> > > > -                     pr_err("Failed to retrieve scheduler Rx inter=
rupt\n");
> > > > +     ret =3D ffa_get_notif_intid(&intid);
> > > > +     if (ret)
> > > >               return ret;
> > > > -     }
> > > >
> > > >       if (acpi_disabled) {
> > > >               struct of_phandle_args oirq =3D {};
> > > > @@ -1329,12 +1340,12 @@ static int ffa_sched_recv_irq_map(void)
> > > >
> > > >               oirq.np =3D gic;
> > > >               oirq.args_count =3D 1;
> > > > -             oirq.args[0] =3D sr_intid;
> > > > +             oirq.args[0] =3D intid;
> > > >               irq =3D irq_create_of_mapping(&oirq);
> > > >               of_node_put(gic);
> > > >  #ifdef CONFIG_ACPI
> > > >       } else {
> > > > -             irq =3D acpi_register_gsi(NULL, sr_intid, ACPI_EDGE_S=
ENSITIVE,
> > > > +             irq =3D acpi_register_gsi(NULL, intid, ACPI_EDGE_SENS=
ITIVE,
> > > >                                       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 a=
nd
> VMs that have pending notifications. The Hypervisor returns the list of t=
hose
> 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 versio=
ns
> 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 =3D ffa_features(FFA_NOTIFICATION_BITMAP_CREATE, 0, NULL,=
 NULL);
> > > > -     if (ret) {
> > > > -             pr_info("Notifications not supported, continuing with=
 it ..\n");
> > > > -             return;
> > > > -     }
> > > > +     if (!ret) {
> > > >
> > > > -     ret =3D ffa_notification_bitmap_create();
> > > > -     if (ret) {
> > > > -             pr_info("Notification bitmap create error %d\n", ret)=
;
> > > > -             return;
> > > > +             ret =3D ffa_notification_bitmap_create();
> > > > +             if (ret) {
> > > > +                     pr_err("notification_bitmap_create error %d\n=
", ret);
> > > > +                     return;
> > > > +             }
> > > > +             drv_info->bitmap_created =3D true;
> > > >       }
> > > > -     drv_info->bitmap_created =3D true;
> > >
> > > This boils down to saying that FFA_NOTIFICATION_BITMAP_CREATE is not
> > > implemented for a VM (because the hypervisor should take care of issu=
ing
> > > 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