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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Julien,

On Fri, May 3, 2024 at 4:25 PM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Jens,
>
> On 03/05/2024 14:54, Jens Wiklander wrote:
> >>> +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.
>
> Ah I didn't notice that notify_cpu_starting() is called with IRQ
> disabled. I assumed they would be enabled.
>
> Then I would consider to do:
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 6efed876782e..db322672e508 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -389,6 +389,7 @@ void asmlinkage start_secondary(void)
>        */
>       init_maintenance_interrupt();
>       init_timer_interrupt();
> +    init_tee_interrupt();
>
>       local_abort_enable();
>
> And plumb through the TEE subsystem.

I'll use that in the next version, it should remove a lot of complex code.

Thanks,
Jens