[PATCH] bsps/arm: fix off-by-1 in gicv3 processor count

[Gedare Bloom]

On Wed, Aug 4, 2021 at 11:50 AM Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
>
> On 04/08/2021 19:45, Sebastian Huber wrote:
> > On 04/08/2021 19:18, Gedare Bloom wrote:
> >> Hi Sebastian,
> >>
> >>
> >> On Wed, Aug 4, 2021 at 11:15 AM Gedare Bloom<gedare at rtems.org>  wrote:
> >>> ---
> >>>   bsps/shared/dev/irq/arm-gicv3.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/bsps/shared/dev/irq/arm-gicv3.c
> >>> b/bsps/shared/dev/irq/arm-gicv3.c
> >>> index ea123d325e..95021f6ddf 100644
> >>> --- a/bsps/shared/dev/irq/arm-gicv3.c
> >>> +++ b/bsps/shared/dev/irq/arm-gicv3.c
> >>> @@ -551,11 +551,11 @@ uint32_t arm_gic_irq_processor_count(void)
> >>>       for (i = 0; i < CPU_MAXIMUM_PROCESSORS; ++i) {
> >>>         volatile gic_redist *redist = gicv3_get_redist(i);
> >>>
> >>> +      ++cpu_count;
> >>> +
> >>>         if ((redist->icrtyper & GIC_REDIST_ICRTYPER_LAST) != 0) {
> >>>           break;
> >>>         }
> >>> -
> >>> -      ++cpu_count;
> >>>       }
> >> Can you check my thinking here? I'm trying to implement SMP support
> >> for the versal on aarch64. The call to arm_gic_irq_processor_count()
> >> returns 1 through this code path. Since cpu_count is initialized to 0,
> >> I think we need to increment it each time we find an active processor,
> >> until we find the last active processor, which we also need to include
> >> in the count, because the cpu_count should be NUM_PROC + 1?
> >>
> >> After I make this change, I encounter other problems with the
> >> secondary processor initialization, but at least it tries to
> >> initialize/wait for core#1 then.
> >
> > The current logic is based on this text in the Cortex-R52 TRM for
> > GICR_TYPER[Last]:
> >
> > "In a processor configured with an interrupt export port, this bit is
> > set for the Redistributor associated with
> > the export port. Otherwise, in a system with n cores, this bit is set
> > for the Redistributor associated with
> > core n-1."
> >
> > I don't know how you can figure out if a processor has an interrupt
> > export port. Maybe there are better ways to figure out the processor
> > count. I would have a look at the Linux or U-Boot sources.
> >
>From what I could tell, they use the MPIDR to determine cpu count and
boot vs secondary cpu:
U-Boot:
  https://github.com/u-boot/u-boot/blob/master/arch/arm/cpu/armv8/start.S#L179
  https://github.com/u-boot/u-boot/blob/master/arch/arm/include/asm/macro.h#L146

Linux:
  https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/smp.c#L559

I may try something like that out.

> > Independent of this, I had problems with the software generated
> > interrupts on Qemu and AArch64. They worked only once. This cases the
> > new interrupt manager tests to fail with a timeout.
>
> I have to check this on the FVP simulator. Maybe this simulator has two
> interrupt export ports or we have an off by one error.
>
> --
> embedded brains GmbH
> Herr Sebastian HUBER
> Dornierstr. 4
> 82178 Puchheim
> Germany
> email: sebastian.huber at embedded-brains.de
> phone: +49-89-18 94 741 - 16
> fax:   +49-89-18 94 741 - 08
>
> Registergericht: Amtsgericht München
> Registernummer: HRB 157899
> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> Unsere Datenschutzerklärung finden Sie hier:
> https://embedded-brains.de/datenschutzerklaerung/