Re: [XEN PATCH v5 7/7] xen/arm: ffa: support notification

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi,

On Wed, Jun 5, 2024 at 12:45 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi,
>
> > On 4 Jun 2024, at 12:24, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 03/06/2024 10:50, Jens Wiklander wrote:
> >> Hi Julien,
> >
> > Hi Jens,
> >
> >
> >> On Mon, Jun 3, 2024 at 11:12 AM Julien Grall <julien@xxxxxxx> wrote:
> >>>
> >>> Hi Jens,
> >>>
> >>> On 03/06/2024 10:01, Jens Wiklander wrote:
> >>>> On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
> >>>> <Bertrand.Marquis@xxxxxxx> wrote:
> >>>>>
> >>>>> Hi Jens,
> >>>>>
> >>>>>> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@xxxxxxxxxx> 
> >>>>>> wrote:
> >>>>>>
> >>>>>> Add support for FF-A notifications, currently limited to an SP (Secure
> >>>>>> Partition) sending an asynchronous notification to a guest.
> >>>>>>
> >>>>>> Guests and Xen itself are made aware of pending notifications with an
> >>>>>> interrupt. The interrupt handler triggers a tasklet to retrieve the
> >>>>>> notifications using the FF-A ABI and deliver them to their 
> >>>>>> destinations.
> >>>>>>
> >>>>>> Update ffa_partinfo_domain_init() to return error code like
> >>>>>> ffa_notif_domain_init().
> >>>>>>
> >>>>>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> v4->v5:
> >>>>>> - Move the freeing of d->arch.tee to the new TEE mediator 
> >>>>>> free_domain_ctx
> >>>>>>   callback to make the context accessible during 
> >>>>>> rcu_lock_domain_by_id()
> >>>>>>   from a tasklet
> >>>>>> - Initialize interrupt handlers for secondary CPUs from the new TEE 
> >>>>>> mediator
> >>>>>>   init_interrupt() callback
> >>>>>> - Restore the ffa_probe() from v3, that is, remove the
> >>>>>>   presmp_initcall(ffa_init) approach and use ffa_probe() as usual now 
> >>>>>> that we
> >>>>>>   have the init_interrupt callback.
> >>>>>> - A tasklet is added to handle the Schedule Receiver interrupt. The 
> >>>>>> tasklet
> >>>>>>   finds each relevant domain with rcu_lock_domain_by_id() which now is 
> >>>>>> enough
> >>>>>>   to guarantee that the FF-A context can be accessed.
> >>>>>> - The notification interrupt handler only schedules the notification
> >>>>>>   tasklet mentioned above
> >>>>>>
> >>>>>> v3->v4:
> >>>>>> - Add another note on FF-A limitations
> >>>>>> - Clear secure_pending in ffa_handle_notification_get() if both SP and 
> >>>>>> SPM
> >>>>>>   bitmaps are retrieved
> >>>>>> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id 
> >>>>>> FF-A
> >>>>>>   uses for Xen itself
> >>>>>> - Replace the get_domain_by_id() call done via 
> >>>>>> ffa_get_domain_by_vm_id() in
> >>>>>>   notif_irq_handler() with a call to 
> >>>>>> rcu_lock_live_remote_domain_by_id() via
> >>>>>>   ffa_rcu_lock_domain_by_vm_id()
> >>>>>> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as 
> >>>>>> needed
> >>>>>>   to access and update the secure_pending field
> >>>>>> - In notif_irq_handler(), look for the first online CPU instead of 
> >>>>>> assuming
> >>>>>>   that the first CPU is online
> >>>>>> - Initialize FF-A via presmp_initcall() before the other CPUs are 
> >>>>>> online,
> >>>>>>   use register_cpu_notifier() to install the interrupt handler
> >>>>>>   notif_irq_handler()
> >>>>>> - Update commit message to reflect recent updates
> >>>>>>
> >>>>>> v2->v3:
> >>>>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> >>>>>>   FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> >>>>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> >>>>>>   setup_irq()
> >>>>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> >>>>>>   doesn't conflict with static SGI handlers
> >>>>>>
> >>>>>> v1->v2:
> >>>>>> - Addressing review comments
> >>>>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> >>>>>>   cpu_user_regs *regs as argument.
> >>>>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to 
> >>>>>> return
> >>>>>>   an error code.
> >>>>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> >>>>>> ---
> >>>>>> xen/arch/arm/tee/Makefile       |   1 +
> >>>>>> xen/arch/arm/tee/ffa.c          |  72 +++++-
> >>>>>> xen/arch/arm/tee/ffa_notif.c    | 409 ++++++++++++++++++++++++++++++++
> >>>>>> xen/arch/arm/tee/ffa_partinfo.c |   9 +-
> >>>>>> xen/arch/arm/tee/ffa_private.h  |  56 ++++-
> >>>>>> xen/arch/arm/tee/tee.c          |   2 +-
> >>>>>> xen/include/public/arch-arm.h   |  14 ++
> >>>>>> 7 files changed, 548 insertions(+), 15 deletions(-)
> >>>>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >>>>>>
> >>>> [...]
> >>>>>>
> >>>>>> @@ -517,8 +567,10 @@ err_rxtx_destroy:
> >>>>>> static const struct tee_mediator_ops ffa_ops =
> >>>>>> {
> >>>>>>      .probe = ffa_probe,
> >>>>>> +    .init_interrupt = ffa_notif_init_interrupt,
> >>>>>
> >>>>> With the previous change on init secondary i would suggest to
> >>>>> have a ffa_init_secondary here actually calling the 
> >>>>> ffa_notif_init_interrupt.
> >>>>
> >>>> Yes, that makes sense. I'll update.
> >>>>
> >>>>>
> >>>>>>      .domain_init = ffa_domain_init,
> >>>>>>      .domain_teardown = ffa_domain_teardown,
> >>>>>> +    .free_domain_ctx = ffa_free_domain_ctx,
> >>>>>>      .relinquish_resources = ffa_relinquish_resources,
> >>>>>>      .handle_call = ffa_handle_call,
> >>>>>> };
> >>>>>> diff --git a/xen/arch/arm/tee/ffa_notif.c 
> >>>>>> b/xen/arch/arm/tee/ffa_notif.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..e8e8b62590b3
> >>>>>> --- /dev/null
> >>>>>> +++ b/xen/arch/arm/tee/ffa_notif.c
> >>>> [...]
> >>>>>> +static void notif_vm_pend_intr(uint16_t vm_id)
> >>>>>> +{
> >>>>>> +    struct ffa_ctx *ctx;
> >>>>>> +    struct domain *d;
> >>>>>> +    struct vcpu *v;
> >>>>>> +
> >>>>>> +    /*
> >>>>>> +     * vm_id == 0 means a notifications pending for Xen itself, but
> >>>>>> +     * we don't support that yet.
> >>>>>> +     */
> >>>>>> +    if ( !vm_id )
> >>>>>> +        return;
> >>>>>> +
> >>>>>> +    d = ffa_rcu_lock_domain_by_vm_id(vm_id);
> >>>>>> +    if ( !d )
> >>>>>> +        return;
> >>>>>> +
> >>>>>> +    ctx = d->arch.tee;
> >>>>>> +    if ( !ctx )
> >>>>>> +        goto out_unlock;
> >>>>>
> >>>>> In both previous cases you are silently ignoring an interrupt
> >>>>> due to an internal error.
> >>>>> Is this something that we should trace ? maybe just debug ?
> >>>>>
> >>>>> Could you add a comment to explain why this could happen
> >>>>> (when possible) or not and the possible side effects ?
> >>>>>
> >>>>> The second one would be a notification for a domain without
> >>>>> FF-A enabled which is ok but i am a bit more wondering on
> >>>>> the first one.
> >>>>
> >>>> The SPMC must be out of sync in both cases. I've been looking for a
> >>>> window where that can happen, but I can't find any. SPMC is called
> >>>> with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
> >>>> SPMC shouldn't try to deliver any notifications after that.
> >>>
> >>> I don't think I agree with the conclusion. I believe, this can also
> >>> happen in normal operation.
> >>>
> >>> For example, the SPMC could have trigger the interrupt before
> >>> FFA_NOTIFICATION_BITMAP_DESTROY but Xen didn't handle the interrupt (or
> >>> run the tasklet) until later.
> >> You're right, there is a window. Delayed handling is OK since
> >> FFA_NOTIFICATION_INFO_GET_64 is invoked from the tasklet, but there is
> >> a window if the tasklet is suspended or another core destroys the
> >> domain before the tasklet has called ffa_rcu_lock_domain_by_vm_id().
> >> So far it's harmless and I guess we can afford a print.
> >
> > I think it would confuse more the user than anything else because this is 
> > an expected race. If we wanted to print a message, then I would argue it 
> > should be in the case where...
> >
> >>>
> >>> This could be at the time where the domain has been fully destroyed or
> >>> even when...
> >>>
> >>>> In the second case, the domain ID might have been reused for a domain
> >>>> without FF-A enabled, but the SPMC should have known that already.
> >>>
> >>> ... a new domain has been created. Although, the latter is rather 
> >>> unlikely.
> >>>
> >>> So what if the new domain has FFA enabled? Is there any potential
> >>> security issue?
> >> In this case, we'll inject an NPI in the guest, but when it invokes
> >> FFA_NOTIFICATION_GET it will get accurate information from the SPMC.
> >> The worst case is a spurious NPI. This shouldn't be a security issue.
> >
> > ... we inject the interrupt to the "wrong" domain. But I also understand 
> > that it would be difficult for Xen to detect it.
> >
> > So I would say no print should be needed. Bertrand, what do you think?
>
> Yes i agree that it could confuse the user.
> I would just put comments to explain the situations where it could occur in 
> the code.

Thanks, I'll add comments for the next version.

Cheers,
Jens