[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Julien,

On Fri, May 3, 2024 at 11:59 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Jens,
>
> On 02/05/2024 15:56, Jens Wiklander wrote:
> > -static bool ffa_probe(void)
> > +static int __init ffa_init(void)
> >   {
> >       uint32_t vers;
> >       unsigned int major_vers;
> > @@ -460,16 +511,16 @@ static bool ffa_probe(void)
> >           printk(XENLOG_ERR
> >                  "ffa: unsupported SMCCC version %#x (need at least %#x)\n",
> >                  smccc_ver, ARM_SMCCC_VERSION_1_2);
> > -        return false;
> > +        return 0;
> >       }
> >
> >       if ( !ffa_get_version(&vers) )
> > -        return false;
> > +        return 0;
> >
> >       if ( vers < FFA_MIN_SPMC_VERSION || vers > FFA_MY_VERSION )
> >       {
> >           printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers);
> > -        return false;
> > +        return 0;
> >       }
> >
> >       major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) & 
> > FFA_VERSION_MAJOR_MASK;
> > @@ -492,26 +543,33 @@ static bool ffa_probe(void)
> >            !check_mandatory_feature(FFA_MEM_SHARE_32) ||
> >            !check_mandatory_feature(FFA_MEM_RECLAIM) ||
> >            !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> > -        return false;
> > +        return 0;
> >
> >       if ( !ffa_rxtx_init() )
> > -        return false;
> > +        return 0;
> >
> >       ffa_version = vers;
> >
> >       if ( !ffa_partinfo_init() )
> >           goto err_rxtx_destroy;
> >
> > +    ffa_notif_init();
> >       INIT_LIST_HEAD(&ffa_teardown_head);
> >       init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> >
> > -    return true;
> > +    return 0;
> >
> >   err_rxtx_destroy:
> >       ffa_rxtx_destroy();
> >       ffa_version = 0;
> >
> > -    return false;
> > +    return 0;
> > +}
> > +presmp_initcall(ffa_init);
> I would rather prefer if tee_init() is called from presmp_initcall().
> This would avoid FFA to have to try to initialize itself before all the
> others.

OK, I'll update.

>
> This is what I had in mind with my original request, but I guess I
> wasn't clear enough. Sorry for that.

No worries.

>
> [...]
>
> > +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;
> > +            uint16_t vm_id = get_id_from_resp(&resp, id_pos);
> > +            struct domain *d;
> > +
> > +            /*
> > +             * vm_id == 0 means a notifications pending for Xen itself, but
> > +             * we don't support that yet.
> > +             */
> > +            if (vm_id)
> > +                d = ffa_rcu_lock_domain_by_vm_id(vm_id);
>
> I am still unconvinced that this is sufficient. This will prevent
> "struct domain *" to be freed. But ...
>
> > +            else
> > +                d = NULL;
> > +
> > +            if ( d )
> > +            {
> > +                struct ffa_ctx *ctx = d->arch.tee;
>
> ... I don't think it will protect d->arch.tee to be freed as this is
> happening during teardorwn. You mostly need some other sort of locking
> to avoid d->arch.tee been freed behind your back.

OK, I'll need to work more on this. The outcome of the [1] thread may
affect this too.

[1] https://lists.xenproject.org/archives/html/xen-devel/2024-05/msg00156.html

>
> > +                struct vcpu *v;
> > +
> > +                ACCESS_ONCE(ctx->notif.secure_pending) = true;
> > +
> > +                /*
> > +                 * Since we're only delivering global notification, always
> > +                 * deliver to the first online vCPU. It doesn't matter
> > +                 * which we chose, as long as it's available.
> > +                 */
> > +                for_each_vcpu(d, v)
> > +                {
> > +                    if ( is_vcpu_online(v) )
> > +                    {
> > +                        vgic_inject_irq(d, v, GUEST_FFA_NOTIF_PEND_INTR_ID,
> > +                                        true);
> > +                        break;
> > +                    }
> > +                }
> > +                if ( !v )
> > +                    printk(XENLOG_ERR "ffa: can't inject NPI, all vCPUs 
> > offline\n");
> > +
> > +                rcu_unlock_domain(d);
> > +            }
> > +
> > +            id_pos += count;
> > +        }
> > +
> > +    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> > +}
>
> [...]
>
> > +static int ffa_setup_irq_callback(struct notifier_block *nfb,
> > +                                  unsigned long action, void *hcpu)
> > +{
> > +    unsigned int cpu = (unsigned long)hcpu;
> > +    struct notif_irq_info irq_info = { };
> > +
> > +    switch ( action )
> > +    {
> > +    case CPU_ONLINE:
>
> Can't you execute the notifier in CPU_STARTING? This will be called on
> the CPU directly, so you should be able to use request_irq(...).

I tried that first but it failed with the ASSERT_ALLOC_CONTEXT() in _xmalloc().

I've also tested a three-step solution with CPU_UP_PREPARE,
CPU_STARTING, and CPU_UP_CANCELED.
My approach here is more direct, but it still suffers from a weakness
in error handling even if it seems quite unlikely to run out of heap
or for setup_irq() to fail at this stage.

Thanks,
Jens

>
> > +        /*
> > +         * The notifier call is done on the primary or initiating CPU when
> > +         * the target CPU have come online, but the SGI must be setup on
> > +         * the target CPU.
> > +         *
> > +         * We make an IPI call to the target CPU to setup the SGI. The call
> > +         * is executed in interrupt context on the target CPU, so we can't
> > +         * call request_irq() directly since it allocates memory.
> > +         *
> > +         * We preallocate the needed irqaction here and pass it via the
> > +         * temporary struct notif_irq_info. The call is synchronous in the
> > +         * sense that when on_selected_cpus() returns the callback
> > +         * notif_irq_enable() has done the same on the target CPU.
> > +         *
> > +         * We deal with two errors, one where notif_irq_enable() hasn't
> > +         * been called for some reason, that error is logged below. The
> > +         * other where setup_irq() fails is logged in the callback. We must
> > +         * free the irqaction in both cases since it has failed to become
> > +         * registered.
> > +         *
> > +         * Failures leads to a problem notifications, the CPUs with failure
> > +         * will trigger on the SGI indicating that there are notifications
> > +         * pending, while the SPMC in the secure world will not notice that
> > +         * the interrupt was lost.
> > +         */
> > +        irq_info.action = xmalloc(struct irqaction);
> > +        if ( !irq_info.action )
> > +        {
> > +            printk(XENLOG_ERR "ffa: setup irq %u failed, out of memory\n",
> > +                   notif_sri_irq);
> > +            return -ENOMEM;
> > +        }
> > +
> > +        *irq_info.action = (struct irqaction){
> > +            .handler = notif_irq_handler,
> > +            .name = "FF-A notif",
> > +            .free_on_release = 1,
> > +        };
> > +
> > +        on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> > +        if (!irq_info.called)
> > +            printk(XENLOG_ERR "ffa: on_selected_cpus(cpumask_of(%u)) 
> > failed\n",
> > +                   cpu);
> > +        /*
> > +         * The irqaction is unused and must be freed if irq_info.action is
> > +         * non-NULL at this stage.
> > +         */
> > +        XFREE(irq_info.action);
> > +        break;
> > +    default:
> > +        break;
> > +    }
> > +
> > +    return 0;
> > +}
>
> Cheers,
>
> --
> Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.