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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Julien,

On Fri, Apr 26, 2024 at 7:58 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Jens,
>
> On 26/04/2024 09:47, Jens Wiklander wrote:
> > +static void notif_irq_enable(void *info)
> > +{
> > +    struct notif_irq_info *irq_info = info;
> > +
> > +    irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> In v2, you were using request_irq(). But now you seem to be open-coding
> it. Can you explain why?

It's because request_irq() does a memory allocation that can't be done
in interrupt context.
>
> > +    if ( irq_info->ret )
> > +        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> > +               irq_info->irq, irq_info->ret);
> > +}
> > +
> > +void ffa_notif_init(void)
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = FFA_FEATURES,
> > +        .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > +    };
> > +    struct notif_irq_info irq_info = { };
> > +    struct arm_smccc_1_2_regs resp;
> > +    unsigned int cpu;
> > +
> > +    arm_smccc_1_2_smc(&arg, &resp);
> > +    if ( resp.a0 != FFA_SUCCESS_32 )
> > +        return;
> > +
> > +    irq_info.irq = resp.a2;
> > +    if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> > +    {
> > +        printk(XENLOG_ERR "ffa: notification initialization failed: 
> > conflicting SGI %u\n",
> > +               irq_info.irq);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> > +     * IPI to call notif_irq_enable() on each CPU including the current
> > +     * CPU. The struct irqaction is preallocated since we can't allocate
> > +     * memory while in interrupt context.
> > +     */
> > +    for_each_online_cpu(cpu)
> Even though we currently don't support CPU hotplug, you want to add a
> CPU Notifier to also register the IRQ when a CPU is onlined
> ffa_notif_init().
>
> For an example, see time.c. We may also want to consider to enable TEE
> in presmp_initcalls() so we don't need to have a for_each_online_cpu().

I was considering that too. I'll update the code.

Thanks,
Jens