Re: [EXTERNAL] Re: enabling same PPI interrupt to all CPU in ARM64 SMP

On Sun, May 14, 2023 at 9:59 AM Souradeep Chakrabarti
<schakrabarti@microsoft.com> wrote:
>
>
>
>
> >-----Original Message-----
> >From: Kyle Evans <kevans@freebsd.org>
> >Sent: Friday, May 12, 2023 10:40 PM
> >To: Souradeep Chakrabarti <schakrabarti@microsoft.com>
> >Cc: Wei Hu <weh@microsoft.com>; freebsd-hackers@FreeBSD.org
> >Subject: [EXTERNAL] Re: enabling same PPI interrupt to all CPU in ARM64 SMP
> >
> >On Fri, May 12, 2023 at 9:51 AM Souradeep Chakrabarti
> ><schakrabarti@microsoft.com> wrote:
> >>
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Souradeep Chakrabarti
> >> >Sent: Monday, May 8, 2023 6:39 PM
> >> >To: Kyle Evans <kevans@freebsd.org>
> >> >Cc: Wei Hu <weh@microsoft.com>; freebsd-hackers@FreeBSD.org
> >> >Subject: enabling same PPI interrupt to all CPU in ARM64 SMP
> >> >
> >> >Hi ,
> >> >
> >> >While using SMP in ARM64 Hyper-V we are getting stuck in boot if
> >> >there is a interrupt for VMBus coming to CPU1 and VMBus interrupt
> >> >handler is not getting that interrupt.
> >> >
> >> >In ARM64 Hyper-V we are using IRQ18 for VMBus and it is a PPI interrupt.
> >> >
> >> >But Hypev-V host sends interrupt to this IRQ 18 for both CPU0 and
> >> >CPU1 in 2CPU system.
> >> >This is based on the corresponding VMBus channel which assigned with the CPU.
> >> >
> >> >Now VMBus ISR is getting the interrupt in CPU0 but not getting from CPU1.
> >> >Any idea, how we can use the same PPI 18 for all the CPU cores?
> >> >
> >> >Any help will be appreciated, as this is blocking the enablement of
> >> >FreeBSD in Azure ARM64.
> >> [Souradeep]
> >> Can someone please help me it.
> >>
> >
> >Looking at least at the GIC implementation, it looks like this is a known limitation:
> >
> > 875         /*
> > 876          * XXX - In case that per CPU interrupt is going to be
> >enabled in time
> > 877          *       when SMP is already started, we need some IPI
> >call which
> > 878          *       enables it on others CPUs. Further, it's more
> >complicated as
> > 879          *       pic_enable_source() and pic_disable_source()
> >should act on
> > 880          *       per CPU basis only. Thus, it should be solved
> >here somehow.
> > 881          */
> > 882         if (isrc->isrc_flags & INTR_ISRCF_PPI)
> > 883                 CPU_SET(PCPU_GET(cpuid), &isrc->isrc_cpu);
> >
> >I think we need something /like/ this:
> >https://people.fr/
> >eebsd.org%2F~kevans%2Fppi.diff&data=05%7C01%7Cschakrabarti%40microsoft.c
> >om%7Cc5c3d254b9d841e9ae9b08db530bb3d2%7C72f988bf86f141af91ab2d7c
> >d011db47%7C1%7C0%7C638195082027744706%7CUnknown%7CTWFpbGZsb3
> >d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> >%7C3000%7C%7C%7C&sdata=SwoR2vHxh6QQhwpOgFcQ9378nDVovhdEKEXoEo
> >gPKsc%3D&reserved=0, though it still has the caveat that PPIs effectively cannot be
> >fully setup before SI_SUB_SMP.
> >So, it's likely almost a NOP for existing platforms (will emit a warning with
> >bootverbose for armv8 timers) but might do the trick for you.
> [Souradeep]
> Thanks for the change but it did not solve the problem. Still the interrupt handler
> vmbus_handle_intr(struct trapframe *trap_frame), is not getting called for the CPU 1.
> It is only getting called for CPU 0 all the time in ARM64 but in x86 it is getting called
> for both CPU1 and CPU0.

Interesting! I do see one problem with the patch (and some cosmetic
issues): we really need to take the gic_mtx in gic_v3_setup_periph()
right up front because CPU_SET() won't necessarily be atomic. That's
not the problem, though for other reasons, but also because...

> From DDB I have collected this data in arm64. irq18 is for vmbus.
> db> show irqs
> ...
> irq18  <gic0,p2>: cpu 03 cnt 411
> ....
>

That would seem to indicate that both CPUs have set it up, but it
occurs to me that enable_intr also needs the same treatment. Let's
wipe gic_v3.c back to a clean slate and try a v2 of the patch:
https://people.freebsd.org/~kevans/ppi-v2.diff

For now we just pretend that we won't be disabling any PPIs as a
proof-of-concept.

Thanks,

Kyle Evans