Re: [RFC PATCH] hw/arm: use correct base address when setting CBAR

["Alex Bennée" <alex.bennee@linaro.org>]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> On 18/6/26 11:36, Alex Bennée wrote:
>> This should point at the base of the Cortex-A9 MPCore private address
>> space. If it doesn't we will confuse the Linux kernel as it probes the
>> SCU registers and erroneously assumes the system is a buggy Aegis SOC
>> and nerf the emission of SEV instructions, deadlocking any WFE's in
>> the kernel (or QEMU smpboot code).
>
> I'm confused...
>
>   The Configuration Base Address Register holds the physical base
>   address of the memory-mapped GIC CPU interface registers.

For which CPU? These are cortex-a9's not a57's and the behaviour is
IMPDEF.

See:

  https://developer.arm.com/documentation/ddi0407/i/introduction/private-memory-region

>
> NPCM7XX_GIC_CPU_IF_ADDR is the correct address of "the memory-mapped
> GIC CPU interface", NPCM7XX_CPUP_BA is the A9MPCORE SCU MMIO address.
> AFAIU the modelling looks correct.
>
> Per your description, the Linux guest tries to access GIC_CPU_IF_ADDR
> but ends up accessing SCU_ADDR? Maybe the issue is with the
> arm_boot_info::gic_cpu_if_addr address we pass to the guest? Maybe
> the secondary reset code is corrupted?
>
> static const ARMInsnFixup smpboot[] = {
>     { 0xe59f2028 }, /* ldr r2, gic_cpu_if */
>     { 0xe59f0028 }, /* ldr r0, bootreg_addr */
>     { 0xe3a01001 }, /* mov r1, #1 */
>     { 0xe5821000 }, /* str r1, [r2] - set GICC_CTLR.Enable */
>     { 0xe3a010ff }, /* mov r1, #0xff */
>     { 0xe5821004 }, /* str r1, [r2, 4] - set GIC_PMR.Priority to 0xff */
>     { 0, FIXUP_DSB },   /* dsb */
>     { 0xe320f003 }, /* wfi */
>     { 0xe5901000 }, /* ldr     r1, [r0] */
>     { 0xe1110001 }, /* tst     r1, r1 */
>     { 0x0afffffb }, /* beq     <wfi> */
>     { 0xe12fff11 }, /* bx      r1 */
>     { 0, FIXUP_GIC_CPU_IF }, /* gic_cpu_if: .word 0x.... */
>
> Can you check ^^^ this value?

I don't think we use this for npcm7xx:

  static void npcm7xx_write_secondary_boot(ARMCPU *cpu,
                                           const struct arm_boot_info *info)
  {
      /*
       * The default smpboot stub halts the secondary CPU with a 'wfi'
       * instruction, but the arch/arm/mach-npcm/platsmp.c in the Linux kernel
       * does not send an IPI to wake it up, so the second CPU fails to boot. So
       * we need to provide our own smpboot stub that can not use 'wfi', it has
       * to spin the secondary CPU until the first CPU writes to the SCRPAD reg.
       */
      uint32_t smpboot[] = {
          0xe59f2018,     /* ldr r2, bootreg_addr */
          0xe3a00000,     /* mov r0, #0 */
          0xe5820000,     /* str r0, [r2] */
          0xe320f002,     /* wfe */
          0xe5921000,     /* ldr r1, [r2] */
          0xe1110001,     /* tst r1, r1 */
          0x0afffffb,     /* beq <wfe> */
          0xe12fff11,     /* bx r1 */
          NPCM7XX_SMP_BOOTREG_ADDR,
      };
      int i;

      for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
          smpboot[i] = tswap32(smpboot[i]);
      }

      rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
                         NPCM7XX_SMP_LOADER_START);
  }


>
> Maybe this code has to be updated for your WFE use case?

In earlier attempts I did drop the wfe in favour of a NOP until I
figured out why the SEV's were not flowing.

>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Suggested-by: Arnd Bergmann <arnd@linaro.org>
>> ---
>>   hw/arm/npcm7xx.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
>> index c2bbcd89dbc..c27f149c04a 100644
>> --- a/hw/arm/npcm7xx.c
>> +++ b/hw/arm/npcm7xx.c
>> @@ -492,7 +492,7 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
>>       /* CPUs */
>>       for (i = 0; i < nc->num_cpus; i++) {
>>           object_property_set_int(OBJECT(&s->cpu[i]), "reset-cbar",
>> -                                NPCM7XX_GIC_CPU_IF_ADDR, &error_abort);
>> +                                NPCM7XX_CPUP_BA, &error_abort);
>>           object_property_set_bool(OBJECT(&s->cpu[i]), "reset-hivecs", true,
>>                                    &error_abort);
>>   

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro