Received-SPF: pass (google.com: domain of linux-kernel+bounces-97008-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240307092132.943881-1-jens.wiklander@linaro.org> <ZereCD7kJxP+vzHN@lpieralisi>
In-Reply-To: <ZereCD7kJxP+vzHN@lpieralisi>
From: Jens Wiklander <jens.wiklander@linaro.org>
Date: Fri, 8 Mar 2024 13:35:05 +0100
Message-ID: <CAHUa44Har+TLBXOgz4EqekEu7fKWgFeCuOa0i8UiNLfen4tJiw@mail.gmail.com>
Subject: Re: [PATCH] firmware: arm_ffa: support running as a guest in a vm
To: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	Sudeep Holla <sudeep.holla@arm.com>, 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 Fri, Mar 8, 2024 at 10:44=E2=80=AFAM Lorenzo Pieralisi <lpieralisi@kerne=
l.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 ha=
s
> > 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_f=
fa/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 w=
ork_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, NU=
LL);
> > +     if (!ret)
> > +             return 0;
> > +     ret =3D 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 i=
nterrupts\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 =3D ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, &sr_intid=
, NULL);
> > -     if (ret < 0) {
> > -             if (ret !=3D -EOPNOTSUPP)
> > -                     pr_err("Failed to retrieve scheduler Rx interrupt=
\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_SENSI=
TIVE,
> > +             irq =3D acpi_register_gsi(NULL, intid, ACPI_EDGE_SENSITIV=
E,
> >                                       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 =3D ffa_features(FFA_NOTIFICATION_BITMAP_CREATE, 0, NULL, NUL=
L);
> > -     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", r=
et);
> > +                     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 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 =3D ffa_sched_recv_irq_map();
> >       if (irq <=3D 0) {
> > --
> > 2.34.1
> >