Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Apr 29, 2024 at 9:20 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
[...]
> >> +static void notif_irq_handler(int irq, void *data)
> >> +{
> >> +    const struct arm_smccc_1_2_regs arg = {
> >> +        .a0 = FFA_NOTIFICATION_INFO_GET_64,
> >> +    };
> >> +    struct arm_smccc_1_2_regs resp;
> >> +    unsigned int id_pos;
> >> +    unsigned int list_count;
> >> +    uint64_t ids_count;
> >> +    unsigned int n;
> >> +    int32_t res;
> >> +
> >> +    do {
> >> +        arm_smccc_1_2_smc(&arg, &resp);
> >> +        res = ffa_get_ret_code(&resp);
> >> +        if ( res )
> >> +        {
> >> +            if ( res != FFA_RET_NO_DATA )
> >> +                printk(XENLOG_ERR "ffa: notification info get failed: 
> >> error %d\n",
> >> +                       res);
> >> +            return;
> >> +        }
> >> +
> >> +        ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> >> +        list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> >> +                     FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> >> +
> >> +        id_pos = 0;
> >> +        for ( n = 0; n < list_count; n++ )
> >> +        {
> >> +            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> >> +            struct domain *d;
> >> +
> >> +            d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> >
> > Thinking a bit more about the question from Bertrand about get_domain_id() 
> > vs rcu_lock_domain_by_id(). I am actually not sure whether either are ok 
> > here.
> >
> > If I am not mistaken, d->arch.tee will be freed as part of the domain 
> > teardown process. This means you can have the following scenario:
> >
> > CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
> >
> > CPU1: call domain_kill()
> > CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
> >
> > d->arch.tee is now a dangling pointer
> >
> > CPU0: access d->arch.tee
> >
> > This implies you may need to gain a global lock (I don't have a better idea 
> > so far) to protect the IRQ handler against domains teardown.
> >
> > Did I miss anything?
>
> I think you are right which is why I was thinking to use 
> rcu_lock_live_remote_domain_by_id to only get a reference
> to the domain if it is not dying.
>
> From the comment in sched.h:
> /*
>  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>  * This is the preferred function if the returned domain reference
>  * is short lived,  but it cannot be used if the domain reference needs
>  * to be kept beyond the current scope (e.g., across a softirq).
>  * The returned domain reference must be discarded using rcu_unlock_domain().
>  */
>
> Now the question of short lived should be challenged but I do not think we can
> consider the current code as "long lived".
>
> It would be a good idea to start a mailing list thread discussion with other
> maintainers on the subject to confirm.
> @Jens: as i will be off for some time, would you mind doing it ?

Sure, first I'll send out a new patch set with the current comments
addressed. I'll update to use rcu_lock_live_remote_domain_by_id() both
because it makes more sense than the current code, and also to have a
good base for the discussion.

Thanks,
Jens