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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall