[Linaro-uefi,RFC,6/7] Platforms/Styx/FdtDxe: boot secondaries straight into OS pen

Message ID 1462196143-21998-7-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel May 2, 2016, 1:35 p.m.
Instead of using the UEFI specific ArmMpCore protocol to boot the cores
into UEFI first before moving the into the OS pen, defer the secondary
boot until we can move them there straight away. Since Styx does not
execute in place, and has always boots the secondaries explicitly, either
via the ISCP interface or via PSCI, this is the safest approach.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c | 113 ++++++++++++++++++--
 Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf               |   8 +-
 2 files changed, 107 insertions(+), 14 deletions(-)

Comments

Duran, Leo May 2, 2016, 2:41 p.m. | #1
Ard,
Wow... I do like the way this is going!
Q: Have you booted all core using DO_PSCI=0?

All,
BTW, I'd like to propose that we require PcdTrustedFWSupport = TRUE (for EL3 handling)
This way, we could then can drop the ISCP code that brings cores out of reset into UEFI.

Leo

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, May 02, 2016 8:36 AM
> To: linaro-uefi@lists.linaro.org
> Cc: leif.lindholm@linaro.org; ricardo.salveti@linaro.org; Duran, Leo
> <leo.duran@amd.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [RFC PATCH 6/7] Platforms/Styx/FdtDxe: boot secondaries straight
> into OS pen
> 
> Instead of using the UEFI specific ArmMpCore protocol to boot the cores into
> UEFI first before moving the into the OS pen, defer the secondary boot until
> we can move them there straight away. Since Styx does not execute in place,
> and has always boots the secondaries explicitly, either via the ISCP interface
> or via PSCI, this is the safest approach.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c | 113
> ++++++++++++++++++--
>  Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf               |   8 +-
>  2 files changed, 107 insertions(+), 14 deletions(-)
> 
> diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
> b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
> index 0fb2f4e47dd2..3b88a11bb771 100644
> --- a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
> +++ b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
> @@ -18,14 +18,19 @@
> 
>  **/
> 
> +#include <Library/ArmSmcLib.h>
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/PcdLib.h>
>  #include <Base.h>
>  #include <BdsLib/BdsInternal.h>
> -#include <Library/ArmGicLib.h>
> -#include <Library/IoLib.h>
> 
> +#include <Common/CoreState.h>
> +#include <IndustryStandard/ArmStdSmc.h> #include
> +<Protocol/AmdIscpDxeProtocol.h>
>  #include <AmdStyxHelperLib.h>
> 
> +#define PLATINIT_CONTEXT_ID       ( 0x0 )   // used on SMC calls
> +
>  /* These externs are used to relocate some ASM code into Linux memory.
> */  extern VOID  *SecondariesPenStart;  extern VOID  *SecondariesPenEnd;
> @@ -34,6 +39,85 @@ extern UINTN *AsmMailboxBase;
> 
>  extern  EFI_BOOT_SERVICES       *gBS;
> 
> +AMD_ISCP_DXE_PROTOCOL *mIscpDxeProtocol;
> +
> +STATIC
> +VOID
> +EFIAPI
> +AmdStyxBringupSecondary (
> +  ARM_CORE_INFO             *ArmCoreInfoTable,
> +  UINTN                     Index,
> +  EFI_PHYSICAL_ADDRESS      SecondaryEntry
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  ISCP_CPU_RESET_INFO         CpuResetInfo;
> +  BOOLEAN                     fCoreTransitionDone;
> +  ARM_SMC_ARGS                SmcRegs;
> +
> +  if (FixedPcdGetBool (PcdTrustedFWSupport)) {
> +    SmcRegs.Arg0 = ARM_SMC_ID_PSCI_CPU_ON_AARCH64;
> +    SmcRegs.Arg1 = GET_MPID (ArmCoreInfoTable[Index].ClusterId,
> +                             ArmCoreInfoTable[Index].CoreId);
> +    SmcRegs.Arg2 = SecondaryEntry;
> +    SmcRegs.Arg3 = PLATINIT_CONTEXT_ID;
> +    ArmCallSmc (&SmcRegs);
> +
> +    if (SmcRegs.Arg0 == ARM_SMC_PSCI_RET_SUCCESS ||
> +        SmcRegs.Arg0 == ARM_SMC_PSCI_RET_ALREADY_ON) {
> +      DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));
> +    } else {
> +      DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d] to RUN
> state.\n", Index));
> +    }
> +  } else if (FixedPcdGetBool (PcdIscpSupport)) {
> +    CpuResetInfo.CoreNum = Index;
> +    Status = mIscpDxeProtocol->AmdExecuteCpuRetrieveId
> (mIscpDxeProtocol,
> +                                                        &CpuResetInfo);
> +    ASSERT_EFI_ERROR (Status);
> +
> +    // Transition core to the RUN state
> +    fCoreTransitionDone = FALSE;
> +    do {
> +      switch (CpuResetInfo.CoreStatus.Status) {
> +      case CPU_CORE_POWERDOWN:
> +        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERDOWN -> POWERUP\n",
> Index));
> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_POWERUP;
> +        break;
> +
> +      case CPU_CORE_POWERUP:
> +      case CPU_CORE_SLEEP:
> +        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERUP -> RESET\n", Index));
> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_RESET;
> +        CpuResetInfo.CoreStatus.ResetVector = SecondaryEntry;
> +        break;
> +
> +      case CPU_CORE_RESET:
> +        DEBUG ((EFI_D_ERROR, "Core[%d]: RESET -> RUN\n", Index));
> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_RUN;
> +        break;
> +
> +      default:
> +         if (CpuResetInfo.CoreStatus.Status == CPU_CORE_RUN) {
> +           DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));
> +         } else {
> +           DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d] to
> RUN state.\n", Index));
> +         }
> +         fCoreTransitionDone = TRUE;
> +        break;
> +      }
> +
> +      // Transition core to next state
> +      if (!fCoreTransitionDone) {
> +        Status = mIscpDxeProtocol->AmdExecuteCpuCoreReset
> (mIscpDxeProtocol,
> +                                                           &CpuResetInfo);
> +        ASSERT_EFI_ERROR (Status);
> +      }
> +    } while (!fCoreTransitionDone);
> +  } else {
> +    ASSERT (FALSE);
> +  }
> +}
> +
>  VOID
>  EFIAPI
>  AmdStyxMoveParkedCores(
> @@ -52,6 +136,19 @@ AmdStyxMoveParkedCores(
>    UINTN                    CoreMailbox;
>    UINTN                    CoreParking;
> 
> +  if (FixedPcdGetBool (PcdIscpSupport)) {
> +    Status = gBS->LocateProtocol (
> +                    &gAmdIscpDxeProtocolGuid,
> +                    NULL,
> +                    (VOID **)&mIscpDxeProtocol
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      mIscpDxeProtocol = NULL;
> +      DEBUG ((EFI_D_ERROR, "Failed to Locate ISCP DXE Protocol"));
> +      return;
> +    }
> +  }
> +
>    // Get core information
>    ArmCoreInfoTable = AmdStyxGetArmCoreInfoTable (&ArmCoreCount);
>    ASSERT (ArmCoreInfoTable != NULL);
> @@ -103,17 +200,15 @@ AmdStyxMoveParkedCores(
>      *((UINTN*)(CoreParking + sizeof (UINT64))) = 0x0;
>      *((UINTN*)(CoreParking + SIZE_2KB)) = CoreNum;
> 
> -    // Move secondary core to our new Pen
> -    MmioWrite64(ArmCoreInfoTable[CoreNum].MailboxSetAddress,
> (UINTN)PenBase);
> -
>      // Update table entry to be consumed by FDT parser.
>      ArmCoreInfoTable[CoreNum].MailboxSetAddress = CoreMailbox;
>    }
> 
> -  // Flush caches to make sure our pen gets to memory before we release
> secondary cores.
> -  ArmCleanDataCache();
> +  WriteBackDataCacheRange ((VOID *)MpParkingBase, MpParkingSize);
> 
> -  // Send msg to secondary cores to jump to our new Pen.
> -  ArmGicSendSgiTo (PcdGet32(PcdGicDistributorBase),
> ARM_GIC_ICDSGIR_FILTER_EVERYONEELSE, 0x0E, PcdGet32
> (PcdGicSgiIntId));
> +  for (CoreNum = 0; CoreNum < ArmCoreCount; CoreNum++) {
> +    // Move secondary core to our new Pen
> +    AmdStyxBringupSecondary (ArmCoreInfoTable, CoreNum, PenBase);  }
>  }
> 
> diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
> b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
> index 5ac210ba04e4..c660ccd04810 100644
> --- a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
> +++ b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
> @@ -50,6 +50,7 @@
>    FdtLib
>    DevicePathLib
>    AmdStyxHelperLib
> +  ArmSmcLib
> 
>  [LibraryClasses.AARCH64]
>    ArmGicLib
> @@ -63,6 +64,7 @@
> 
>  [Protocols]
>    gEfiFirmwareVolume2ProtocolGuid    ##CONSUMED
> +  gAmdIscpDxeProtocolGuid            ## SOMETIMES_CONSUMES
> 
>  [Pcd]
>    gAmdStyxTokenSpaceGuid.PcdStyxFdt
> @@ -80,11 +82,7 @@
>    gAmdStyxTokenSpaceGuid.PcdTrustedFWSupport
>    gAmdStyxTokenSpaceGuid.PcdParkingProtocolBase
>    gAmdStyxTokenSpaceGuid.PcdParkingProtocolSize
> +  gAmdStyxTokenSpaceGuid.PcdIscpSupport
> 
> -[Pcd.AARCH64]
> -  gArmTokenSpaceGuid.PcdGicDistributorBase
> -  gArmTokenSpaceGuid.PcdGicSgiIntId
> -
>  [Depex]
>    TRUE
> -
> --
> 2.7.4
Ard Biesheuvel May 2, 2016, 2:45 p.m. | #2
On 2 May 2016 at 16:41, Duran, Leo <leo.duran@amd.com> wrote:
> Ard,
> Wow... I do like the way this is going!
> Q: Have you booted all core using DO_PSCI=0?
>

Into the pen, yes, but not all the way into the OS. I was getting an
SError which turned out to be unrelated to these changes.

> All,
> BTW, I'd like to propose that we require PcdTrustedFWSupport = TRUE (for EL3 handling)
> This way, we could then can drop the ISCP code that brings cores out of reset into UEFI.
>

Yes, I think that is reasonable. That would also allow us to move this
to generic code, i.e., a generic DXE driver that implements ACPI
parking protocol/spin-table on top of PSCI. For now, I would like to
see it working first, especially since the MpCore stuff is somewhat
broken if you run your XIP PEI code from DRAM




>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, May 02, 2016 8:36 AM
>> To: linaro-uefi@lists.linaro.org
>> Cc: leif.lindholm@linaro.org; ricardo.salveti@linaro.org; Duran, Leo
>> <leo.duran@amd.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: [RFC PATCH 6/7] Platforms/Styx/FdtDxe: boot secondaries straight
>> into OS pen
>>
>> Instead of using the UEFI specific ArmMpCore protocol to boot the cores into
>> UEFI first before moving the into the OS pen, defer the secondary boot until
>> we can move them there straight away. Since Styx does not execute in place,
>> and has always boots the secondaries explicitly, either via the ISCP interface
>> or via PSCI, this is the safest approach.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c | 113
>> ++++++++++++++++++--
>>  Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf               |   8 +-
>>  2 files changed, 107 insertions(+), 14 deletions(-)
>>
>> diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
>> b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
>> index 0fb2f4e47dd2..3b88a11bb771 100644
>> --- a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
>> +++ b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
>> @@ -18,14 +18,19 @@
>>
>>  **/
>>
>> +#include <Library/ArmSmcLib.h>
>> +#include <Library/CacheMaintenanceLib.h>
>>  #include <Library/PcdLib.h>
>>  #include <Base.h>
>>  #include <BdsLib/BdsInternal.h>
>> -#include <Library/ArmGicLib.h>
>> -#include <Library/IoLib.h>
>>
>> +#include <Common/CoreState.h>
>> +#include <IndustryStandard/ArmStdSmc.h> #include
>> +<Protocol/AmdIscpDxeProtocol.h>
>>  #include <AmdStyxHelperLib.h>
>>
>> +#define PLATINIT_CONTEXT_ID       ( 0x0 )   // used on SMC calls
>> +
>>  /* These externs are used to relocate some ASM code into Linux memory.
>> */  extern VOID  *SecondariesPenStart;  extern VOID  *SecondariesPenEnd;
>> @@ -34,6 +39,85 @@ extern UINTN *AsmMailboxBase;
>>
>>  extern  EFI_BOOT_SERVICES       *gBS;
>>
>> +AMD_ISCP_DXE_PROTOCOL *mIscpDxeProtocol;
>> +
>> +STATIC
>> +VOID
>> +EFIAPI
>> +AmdStyxBringupSecondary (
>> +  ARM_CORE_INFO             *ArmCoreInfoTable,
>> +  UINTN                     Index,
>> +  EFI_PHYSICAL_ADDRESS      SecondaryEntry
>> +  )
>> +{
>> +  EFI_STATUS                  Status;
>> +  ISCP_CPU_RESET_INFO         CpuResetInfo;
>> +  BOOLEAN                     fCoreTransitionDone;
>> +  ARM_SMC_ARGS                SmcRegs;
>> +
>> +  if (FixedPcdGetBool (PcdTrustedFWSupport)) {
>> +    SmcRegs.Arg0 = ARM_SMC_ID_PSCI_CPU_ON_AARCH64;
>> +    SmcRegs.Arg1 = GET_MPID (ArmCoreInfoTable[Index].ClusterId,
>> +                             ArmCoreInfoTable[Index].CoreId);
>> +    SmcRegs.Arg2 = SecondaryEntry;
>> +    SmcRegs.Arg3 = PLATINIT_CONTEXT_ID;
>> +    ArmCallSmc (&SmcRegs);
>> +
>> +    if (SmcRegs.Arg0 == ARM_SMC_PSCI_RET_SUCCESS ||
>> +        SmcRegs.Arg0 == ARM_SMC_PSCI_RET_ALREADY_ON) {
>> +      DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));
>> +    } else {
>> +      DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d] to RUN
>> state.\n", Index));
>> +    }
>> +  } else if (FixedPcdGetBool (PcdIscpSupport)) {
>> +    CpuResetInfo.CoreNum = Index;
>> +    Status = mIscpDxeProtocol->AmdExecuteCpuRetrieveId
>> (mIscpDxeProtocol,
>> +                                                        &CpuResetInfo);
>> +    ASSERT_EFI_ERROR (Status);
>> +
>> +    // Transition core to the RUN state
>> +    fCoreTransitionDone = FALSE;
>> +    do {
>> +      switch (CpuResetInfo.CoreStatus.Status) {
>> +      case CPU_CORE_POWERDOWN:
>> +        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERDOWN -> POWERUP\n",
>> Index));
>> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_POWERUP;
>> +        break;
>> +
>> +      case CPU_CORE_POWERUP:
>> +      case CPU_CORE_SLEEP:
>> +        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERUP -> RESET\n", Index));
>> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_RESET;
>> +        CpuResetInfo.CoreStatus.ResetVector = SecondaryEntry;
>> +        break;
>> +
>> +      case CPU_CORE_RESET:
>> +        DEBUG ((EFI_D_ERROR, "Core[%d]: RESET -> RUN\n", Index));
>> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_RUN;
>> +        break;
>> +
>> +      default:
>> +         if (CpuResetInfo.CoreStatus.Status == CPU_CORE_RUN) {
>> +           DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));
>> +         } else {
>> +           DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d] to
>> RUN state.\n", Index));
>> +         }
>> +         fCoreTransitionDone = TRUE;
>> +        break;
>> +      }
>> +
>> +      // Transition core to next state
>> +      if (!fCoreTransitionDone) {
>> +        Status = mIscpDxeProtocol->AmdExecuteCpuCoreReset
>> (mIscpDxeProtocol,
>> +                                                           &CpuResetInfo);
>> +        ASSERT_EFI_ERROR (Status);
>> +      }
>> +    } while (!fCoreTransitionDone);
>> +  } else {
>> +    ASSERT (FALSE);
>> +  }
>> +}
>> +
>>  VOID
>>  EFIAPI
>>  AmdStyxMoveParkedCores(
>> @@ -52,6 +136,19 @@ AmdStyxMoveParkedCores(
>>    UINTN                    CoreMailbox;
>>    UINTN                    CoreParking;
>>
>> +  if (FixedPcdGetBool (PcdIscpSupport)) {
>> +    Status = gBS->LocateProtocol (
>> +                    &gAmdIscpDxeProtocolGuid,
>> +                    NULL,
>> +                    (VOID **)&mIscpDxeProtocol
>> +                    );
>> +    if (EFI_ERROR (Status)) {
>> +      mIscpDxeProtocol = NULL;
>> +      DEBUG ((EFI_D_ERROR, "Failed to Locate ISCP DXE Protocol"));
>> +      return;
>> +    }
>> +  }
>> +
>>    // Get core information
>>    ArmCoreInfoTable = AmdStyxGetArmCoreInfoTable (&ArmCoreCount);
>>    ASSERT (ArmCoreInfoTable != NULL);
>> @@ -103,17 +200,15 @@ AmdStyxMoveParkedCores(
>>      *((UINTN*)(CoreParking + sizeof (UINT64))) = 0x0;
>>      *((UINTN*)(CoreParking + SIZE_2KB)) = CoreNum;
>>
>> -    // Move secondary core to our new Pen
>> -    MmioWrite64(ArmCoreInfoTable[CoreNum].MailboxSetAddress,
>> (UINTN)PenBase);
>> -
>>      // Update table entry to be consumed by FDT parser.
>>      ArmCoreInfoTable[CoreNum].MailboxSetAddress = CoreMailbox;
>>    }
>>
>> -  // Flush caches to make sure our pen gets to memory before we release
>> secondary cores.
>> -  ArmCleanDataCache();
>> +  WriteBackDataCacheRange ((VOID *)MpParkingBase, MpParkingSize);
>>
>> -  // Send msg to secondary cores to jump to our new Pen.
>> -  ArmGicSendSgiTo (PcdGet32(PcdGicDistributorBase),
>> ARM_GIC_ICDSGIR_FILTER_EVERYONEELSE, 0x0E, PcdGet32
>> (PcdGicSgiIntId));
>> +  for (CoreNum = 0; CoreNum < ArmCoreCount; CoreNum++) {
>> +    // Move secondary core to our new Pen
>> +    AmdStyxBringupSecondary (ArmCoreInfoTable, CoreNum, PenBase);  }
>>  }
>>
>> diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
>> b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
>> index 5ac210ba04e4..c660ccd04810 100644
>> --- a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
>> +++ b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
>> @@ -50,6 +50,7 @@
>>    FdtLib
>>    DevicePathLib
>>    AmdStyxHelperLib
>> +  ArmSmcLib
>>
>>  [LibraryClasses.AARCH64]
>>    ArmGicLib
>> @@ -63,6 +64,7 @@
>>
>>  [Protocols]
>>    gEfiFirmwareVolume2ProtocolGuid    ##CONSUMED
>> +  gAmdIscpDxeProtocolGuid            ## SOMETIMES_CONSUMES
>>
>>  [Pcd]
>>    gAmdStyxTokenSpaceGuid.PcdStyxFdt
>> @@ -80,11 +82,7 @@
>>    gAmdStyxTokenSpaceGuid.PcdTrustedFWSupport
>>    gAmdStyxTokenSpaceGuid.PcdParkingProtocolBase
>>    gAmdStyxTokenSpaceGuid.PcdParkingProtocolSize
>> +  gAmdStyxTokenSpaceGuid.PcdIscpSupport
>>
>> -[Pcd.AARCH64]
>> -  gArmTokenSpaceGuid.PcdGicDistributorBase
>> -  gArmTokenSpaceGuid.PcdGicSgiIntId
>> -
>>  [Depex]
>>    TRUE
>> -
>> --
>> 2.7.4
>
Duran, Leo May 2, 2016, 2:58 p.m. | #3
> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Monday, May 02, 2016 9:45 AM

> To: Duran, Leo <leo.duran@amd.com>

> Cc: linaro-uefi@lists.linaro.org; leif.lindholm@linaro.org;

> ricardo.salveti@linaro.org

> Subject: Re: [RFC PATCH 6/7] Platforms/Styx/FdtDxe: boot secondaries

> straight into OS pen

> 

> On 2 May 2016 at 16:41, Duran, Leo <leo.duran@amd.com> wrote:

> > Ard,

> > Wow... I do like the way this is going!

> > Q: Have you booted all core using DO_PSCI=0?

> >

> 

> Into the pen, yes, but not all the way into the OS. I was getting an SError

> which turned out to be unrelated to these changes.

> 

> > All,

> > BTW, I'd like to propose that we require PcdTrustedFWSupport = TRUE

> > (for EL3 handling) This way, we could then can drop the ISCP code that

> brings cores out of reset into UEFI.

> >

> 

> Yes, I think that is reasonable. That would also allow us to move this to

> generic code, i.e., a generic DXE driver that implements ACPI parking

> protocol/spin-table on top of PSCI. For now, I would like to see it working

> first, especially since the MpCore stuff is somewhat broken if you run your

> XIP PEI code from DRAM

> 

> 


Ummh... the baseline code (prior this patch series) used to be able to boot secondary cores, using DO_PSCI=0 and the patched MainMPCore.c.
However, the secondary boot path now gets a "Synchronous Exception" when it invokes GetMpCoreInfo().

I have not tried exercising the (DO_PSCI=0) code path in quite a while, so I don't know when it got broken.
(I suspect due to  change in EDK2, because the Styx code has not changed)

Leo.

> 

> 

> >> -----Original Message-----

> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> >> Sent: Monday, May 02, 2016 8:36 AM

> >> To: linaro-uefi@lists.linaro.org

> >> Cc: leif.lindholm@linaro.org; ricardo.salveti@linaro.org; Duran, Leo

> >> <leo.duran@amd.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> Subject: [RFC PATCH 6/7] Platforms/Styx/FdtDxe: boot secondaries

> >> straight into OS pen

> >>

> >> Instead of using the UEFI specific ArmMpCore protocol to boot the

> >> cores into UEFI first before moving the into the OS pen, defer the

> >> secondary boot until we can move them there straight away. Since Styx

> >> does not execute in place, and has always boots the secondaries

> >> explicitly, either via the ISCP interface or via PSCI, this is the safest

> approach.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> ---

> >>  Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c | 113

> >> ++++++++++++++++++--

> >>  Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf               |   8 +-

> >>  2 files changed, 107 insertions(+), 14 deletions(-)

> >>

> >> diff --git

> >> a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c

> >> b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c

> >> index 0fb2f4e47dd2..3b88a11bb771 100644

> >> --- a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c

> >> +++ b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c

> >> @@ -18,14 +18,19 @@

> >>

> >>  **/

> >>

> >> +#include <Library/ArmSmcLib.h>

> >> +#include <Library/CacheMaintenanceLib.h>

> >>  #include <Library/PcdLib.h>

> >>  #include <Base.h>

> >>  #include <BdsLib/BdsInternal.h>

> >> -#include <Library/ArmGicLib.h>

> >> -#include <Library/IoLib.h>

> >>

> >> +#include <Common/CoreState.h>

> >> +#include <IndustryStandard/ArmStdSmc.h> #include

> >> +<Protocol/AmdIscpDxeProtocol.h>

> >>  #include <AmdStyxHelperLib.h>

> >>

> >> +#define PLATINIT_CONTEXT_ID       ( 0x0 )   // used on SMC calls

> >> +

> >>  /* These externs are used to relocate some ASM code into Linux

> memory.

> >> */  extern VOID  *SecondariesPenStart;  extern VOID

> >> *SecondariesPenEnd; @@ -34,6 +39,85 @@ extern UINTN

> *AsmMailboxBase;

> >>

> >>  extern  EFI_BOOT_SERVICES       *gBS;

> >>

> >> +AMD_ISCP_DXE_PROTOCOL *mIscpDxeProtocol;

> >> +

> >> +STATIC

> >> +VOID

> >> +EFIAPI

> >> +AmdStyxBringupSecondary (

> >> +  ARM_CORE_INFO             *ArmCoreInfoTable,

> >> +  UINTN                     Index,

> >> +  EFI_PHYSICAL_ADDRESS      SecondaryEntry

> >> +  )

> >> +{

> >> +  EFI_STATUS                  Status;

> >> +  ISCP_CPU_RESET_INFO         CpuResetInfo;

> >> +  BOOLEAN                     fCoreTransitionDone;

> >> +  ARM_SMC_ARGS                SmcRegs;

> >> +

> >> +  if (FixedPcdGetBool (PcdTrustedFWSupport)) {

> >> +    SmcRegs.Arg0 = ARM_SMC_ID_PSCI_CPU_ON_AARCH64;

> >> +    SmcRegs.Arg1 = GET_MPID (ArmCoreInfoTable[Index].ClusterId,

> >> +                             ArmCoreInfoTable[Index].CoreId);

> >> +    SmcRegs.Arg2 = SecondaryEntry;

> >> +    SmcRegs.Arg3 = PLATINIT_CONTEXT_ID;

> >> +    ArmCallSmc (&SmcRegs);

> >> +

> >> +    if (SmcRegs.Arg0 == ARM_SMC_PSCI_RET_SUCCESS ||

> >> +        SmcRegs.Arg0 == ARM_SMC_PSCI_RET_ALREADY_ON) {

> >> +      DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));

> >> +    } else {

> >> +      DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d]

> >> + to RUN

> >> state.\n", Index));

> >> +    }

> >> +  } else if (FixedPcdGetBool (PcdIscpSupport)) {

> >> +    CpuResetInfo.CoreNum = Index;

> >> +    Status = mIscpDxeProtocol->AmdExecuteCpuRetrieveId

> >> (mIscpDxeProtocol,

> >> +                                                        &CpuResetInfo);

> >> +    ASSERT_EFI_ERROR (Status);

> >> +

> >> +    // Transition core to the RUN state

> >> +    fCoreTransitionDone = FALSE;

> >> +    do {

> >> +      switch (CpuResetInfo.CoreStatus.Status) {

> >> +      case CPU_CORE_POWERDOWN:

> >> +        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERDOWN -> POWERUP\n",

> >> Index));

> >> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_POWERUP;

> >> +        break;

> >> +

> >> +      case CPU_CORE_POWERUP:

> >> +      case CPU_CORE_SLEEP:

> >> +        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERUP -> RESET\n", Index));

> >> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_RESET;

> >> +        CpuResetInfo.CoreStatus.ResetVector = SecondaryEntry;

> >> +        break;

> >> +

> >> +      case CPU_CORE_RESET:

> >> +        DEBUG ((EFI_D_ERROR, "Core[%d]: RESET -> RUN\n", Index));

> >> +        CpuResetInfo.CoreStatus.Status = CPU_CORE_RUN;

> >> +        break;

> >> +

> >> +      default:

> >> +         if (CpuResetInfo.CoreStatus.Status == CPU_CORE_RUN) {

> >> +           DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));

> >> +         } else {

> >> +           DEBUG ((EFI_D_ERROR, "Warning: Could not transition

> >> + Core[%d] to

> >> RUN state.\n", Index));

> >> +         }

> >> +         fCoreTransitionDone = TRUE;

> >> +        break;

> >> +      }

> >> +

> >> +      // Transition core to next state

> >> +      if (!fCoreTransitionDone) {

> >> +        Status = mIscpDxeProtocol->AmdExecuteCpuCoreReset

> >> (mIscpDxeProtocol,

> >> +                                                           &CpuResetInfo);

> >> +        ASSERT_EFI_ERROR (Status);

> >> +      }

> >> +    } while (!fCoreTransitionDone);

> >> +  } else {

> >> +    ASSERT (FALSE);

> >> +  }

> >> +}

> >> +

> >>  VOID

> >>  EFIAPI

> >>  AmdStyxMoveParkedCores(

> >> @@ -52,6 +136,19 @@ AmdStyxMoveParkedCores(

> >>    UINTN                    CoreMailbox;

> >>    UINTN                    CoreParking;

> >>

> >> +  if (FixedPcdGetBool (PcdIscpSupport)) {

> >> +    Status = gBS->LocateProtocol (

> >> +                    &gAmdIscpDxeProtocolGuid,

> >> +                    NULL,

> >> +                    (VOID **)&mIscpDxeProtocol

> >> +                    );

> >> +    if (EFI_ERROR (Status)) {

> >> +      mIscpDxeProtocol = NULL;

> >> +      DEBUG ((EFI_D_ERROR, "Failed to Locate ISCP DXE Protocol"));

> >> +      return;

> >> +    }

> >> +  }

> >> +

> >>    // Get core information

> >>    ArmCoreInfoTable = AmdStyxGetArmCoreInfoTable (&ArmCoreCount);

> >>    ASSERT (ArmCoreInfoTable != NULL); @@ -103,17 +200,15 @@

> >> AmdStyxMoveParkedCores(

> >>      *((UINTN*)(CoreParking + sizeof (UINT64))) = 0x0;

> >>      *((UINTN*)(CoreParking + SIZE_2KB)) = CoreNum;

> >>

> >> -    // Move secondary core to our new Pen

> >> -    MmioWrite64(ArmCoreInfoTable[CoreNum].MailboxSetAddress,

> >> (UINTN)PenBase);

> >> -

> >>      // Update table entry to be consumed by FDT parser.

> >>      ArmCoreInfoTable[CoreNum].MailboxSetAddress = CoreMailbox;

> >>    }

> >>

> >> -  // Flush caches to make sure our pen gets to memory before we

> >> release secondary cores.

> >> -  ArmCleanDataCache();

> >> +  WriteBackDataCacheRange ((VOID *)MpParkingBase, MpParkingSize);

> >>

> >> -  // Send msg to secondary cores to jump to our new Pen.

> >> -  ArmGicSendSgiTo (PcdGet32(PcdGicDistributorBase),

> >> ARM_GIC_ICDSGIR_FILTER_EVERYONEELSE, 0x0E, PcdGet32

> >> (PcdGicSgiIntId));

> >> +  for (CoreNum = 0; CoreNum < ArmCoreCount; CoreNum++) {

> >> +    // Move secondary core to our new Pen

> >> +    AmdStyxBringupSecondary (ArmCoreInfoTable, CoreNum, PenBase);

> }

> >>  }

> >>

> >> diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf

> >> b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf

> >> index 5ac210ba04e4..c660ccd04810 100644

> >> --- a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf

> >> +++ b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf

> >> @@ -50,6 +50,7 @@

> >>    FdtLib

> >>    DevicePathLib

> >>    AmdStyxHelperLib

> >> +  ArmSmcLib

> >>

> >>  [LibraryClasses.AARCH64]

> >>    ArmGicLib

> >> @@ -63,6 +64,7 @@

> >>

> >>  [Protocols]

> >>    gEfiFirmwareVolume2ProtocolGuid    ##CONSUMED

> >> +  gAmdIscpDxeProtocolGuid            ## SOMETIMES_CONSUMES

> >>

> >>  [Pcd]

> >>    gAmdStyxTokenSpaceGuid.PcdStyxFdt

> >> @@ -80,11 +82,7 @@

> >>    gAmdStyxTokenSpaceGuid.PcdTrustedFWSupport

> >>    gAmdStyxTokenSpaceGuid.PcdParkingProtocolBase

> >>    gAmdStyxTokenSpaceGuid.PcdParkingProtocolSize

> >> +  gAmdStyxTokenSpaceGuid.PcdIscpSupport

> >>

> >> -[Pcd.AARCH64]

> >> -  gArmTokenSpaceGuid.PcdGicDistributorBase

> >> -  gArmTokenSpaceGuid.PcdGicSgiIntId

> >> -

> >>  [Depex]

> >>    TRUE

> >> -

> >> --

> >> 2.7.4

> >
Ard Biesheuvel May 2, 2016, 6:24 p.m. | #4
On 2 May 2016 at 16:58, Duran, Leo <leo.duran@amd.com> wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, May 02, 2016 9:45 AM
>> To: Duran, Leo <leo.duran@amd.com>
>> Cc: linaro-uefi@lists.linaro.org; leif.lindholm@linaro.org;
>> ricardo.salveti@linaro.org
>> Subject: Re: [RFC PATCH 6/7] Platforms/Styx/FdtDxe: boot secondaries
>> straight into OS pen
>>
>> On 2 May 2016 at 16:41, Duran, Leo <leo.duran@amd.com> wrote:
>> > Ard,
>> > Wow... I do like the way this is going!
>> > Q: Have you booted all core using DO_PSCI=0?
>> >
>>
>> Into the pen, yes, but not all the way into the OS. I was getting an SError
>> which turned out to be unrelated to these changes.
>>
>> > All,
>> > BTW, I'd like to propose that we require PcdTrustedFWSupport = TRUE
>> > (for EL3 handling) This way, we could then can drop the ISCP code that
>> brings cores out of reset into UEFI.
>> >
>>
>> Yes, I think that is reasonable. That would also allow us to move this to
>> generic code, i.e., a generic DXE driver that implements ACPI parking
>> protocol/spin-table on top of PSCI. For now, I would like to see it working
>> first, especially since the MpCore stuff is somewhat broken if you run your
>> XIP PEI code from DRAM
>>
>>
>
> Ummh... the baseline code (prior this patch series) used to be able to boot secondary cores, using DO_PSCI=0 and the patched MainMPCore.c.
> However, the secondary boot path now gets a "Synchronous Exception" when it invokes GetMpCoreInfo().
>
> I have not tried exercising the (DO_PSCI=0) code path in quite a while, so I don't know when it got broken.
> (I suspect due to  change in EDK2, because the Styx code has not changed)
>

That's a shame. I don't have the bandwidth currently to dig deeper
into this than I already have. In any case, using MpCore with SEC and
PEI executing from RAM is a configuration we should stop using asap,
so I think you should consider DO_PSCI=0 broken for now, even without
the synchronous exception you reported.

Patch

diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
index 0fb2f4e47dd2..3b88a11bb771 100644
--- a/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
+++ b/Platforms/AMD/Styx/Drivers/FdtDxe/AArch64/BdsLinuxLoader.c
@@ -18,14 +18,19 @@ 
 
 **/
 
+#include <Library/ArmSmcLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/PcdLib.h>
 #include <Base.h>
 #include <BdsLib/BdsInternal.h>
-#include <Library/ArmGicLib.h>
-#include <Library/IoLib.h>
 
+#include <Common/CoreState.h>
+#include <IndustryStandard/ArmStdSmc.h>
+#include <Protocol/AmdIscpDxeProtocol.h>
 #include <AmdStyxHelperLib.h>
 
+#define PLATINIT_CONTEXT_ID       ( 0x0 )   // used on SMC calls
+
 /* These externs are used to relocate some ASM code into Linux memory. */
 extern VOID  *SecondariesPenStart;
 extern VOID  *SecondariesPenEnd;
@@ -34,6 +39,85 @@  extern UINTN *AsmMailboxBase;
 
 extern  EFI_BOOT_SERVICES       *gBS;
 
+AMD_ISCP_DXE_PROTOCOL *mIscpDxeProtocol;
+
+STATIC
+VOID
+EFIAPI
+AmdStyxBringupSecondary (
+  ARM_CORE_INFO             *ArmCoreInfoTable,
+  UINTN                     Index,
+  EFI_PHYSICAL_ADDRESS      SecondaryEntry
+  )
+{
+  EFI_STATUS                  Status;
+  ISCP_CPU_RESET_INFO         CpuResetInfo;
+  BOOLEAN                     fCoreTransitionDone;
+  ARM_SMC_ARGS                SmcRegs;
+
+  if (FixedPcdGetBool (PcdTrustedFWSupport)) {
+    SmcRegs.Arg0 = ARM_SMC_ID_PSCI_CPU_ON_AARCH64;
+    SmcRegs.Arg1 = GET_MPID (ArmCoreInfoTable[Index].ClusterId,
+                             ArmCoreInfoTable[Index].CoreId);
+    SmcRegs.Arg2 = SecondaryEntry;
+    SmcRegs.Arg3 = PLATINIT_CONTEXT_ID;
+    ArmCallSmc (&SmcRegs);
+
+    if (SmcRegs.Arg0 == ARM_SMC_PSCI_RET_SUCCESS ||
+        SmcRegs.Arg0 == ARM_SMC_PSCI_RET_ALREADY_ON) {
+      DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));
+    } else {
+      DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d] to RUN state.\n", Index));
+    }
+  } else if (FixedPcdGetBool (PcdIscpSupport)) {
+    CpuResetInfo.CoreNum = Index;
+    Status = mIscpDxeProtocol->AmdExecuteCpuRetrieveId (mIscpDxeProtocol,
+                                                        &CpuResetInfo);
+    ASSERT_EFI_ERROR (Status);
+
+    // Transition core to the RUN state
+    fCoreTransitionDone = FALSE;
+    do {
+      switch (CpuResetInfo.CoreStatus.Status) {
+      case CPU_CORE_POWERDOWN:
+        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERDOWN -> POWERUP\n", Index));
+        CpuResetInfo.CoreStatus.Status = CPU_CORE_POWERUP;
+        break;
+
+      case CPU_CORE_POWERUP:
+      case CPU_CORE_SLEEP:
+        DEBUG ((EFI_D_ERROR, "Core[%d]: POWERUP -> RESET\n", Index));
+        CpuResetInfo.CoreStatus.Status = CPU_CORE_RESET;
+        CpuResetInfo.CoreStatus.ResetVector = SecondaryEntry;
+        break;
+
+      case CPU_CORE_RESET:
+        DEBUG ((EFI_D_ERROR, "Core[%d]: RESET -> RUN\n", Index));
+        CpuResetInfo.CoreStatus.Status = CPU_CORE_RUN;
+        break;
+
+      default:
+         if (CpuResetInfo.CoreStatus.Status == CPU_CORE_RUN) {
+           DEBUG ((EFI_D_ERROR, "Core[%d] at RUN state.\n", Index));
+         } else {
+           DEBUG ((EFI_D_ERROR, "Warning: Could not transition Core[%d] to RUN state.\n", Index));
+         }
+         fCoreTransitionDone = TRUE;
+        break;
+      }
+
+      // Transition core to next state
+      if (!fCoreTransitionDone) {
+        Status = mIscpDxeProtocol->AmdExecuteCpuCoreReset (mIscpDxeProtocol,
+                                                           &CpuResetInfo);
+        ASSERT_EFI_ERROR (Status);
+      }
+    } while (!fCoreTransitionDone);
+  } else {
+    ASSERT (FALSE);
+  }
+}
+
 VOID
 EFIAPI
 AmdStyxMoveParkedCores(
@@ -52,6 +136,19 @@  AmdStyxMoveParkedCores(
   UINTN                    CoreMailbox;
   UINTN                    CoreParking;
 
+  if (FixedPcdGetBool (PcdIscpSupport)) {
+    Status = gBS->LocateProtocol (
+                    &gAmdIscpDxeProtocolGuid,
+                    NULL,
+                    (VOID **)&mIscpDxeProtocol
+                    );
+    if (EFI_ERROR (Status)) {
+      mIscpDxeProtocol = NULL;
+      DEBUG ((EFI_D_ERROR, "Failed to Locate ISCP DXE Protocol"));
+      return;
+    }
+  }
+
   // Get core information
   ArmCoreInfoTable = AmdStyxGetArmCoreInfoTable (&ArmCoreCount);
   ASSERT (ArmCoreInfoTable != NULL);
@@ -103,17 +200,15 @@  AmdStyxMoveParkedCores(
     *((UINTN*)(CoreParking + sizeof (UINT64))) = 0x0;
     *((UINTN*)(CoreParking + SIZE_2KB)) = CoreNum;
 
-    // Move secondary core to our new Pen
-    MmioWrite64(ArmCoreInfoTable[CoreNum].MailboxSetAddress, (UINTN)PenBase);
-
     // Update table entry to be consumed by FDT parser.
     ArmCoreInfoTable[CoreNum].MailboxSetAddress = CoreMailbox;
   }
 
-  // Flush caches to make sure our pen gets to memory before we release secondary cores.
-  ArmCleanDataCache();
+  WriteBackDataCacheRange ((VOID *)MpParkingBase, MpParkingSize);
 
-  // Send msg to secondary cores to jump to our new Pen.
-  ArmGicSendSgiTo (PcdGet32(PcdGicDistributorBase), ARM_GIC_ICDSGIR_FILTER_EVERYONEELSE, 0x0E, PcdGet32 (PcdGicSgiIntId));
+  for (CoreNum = 0; CoreNum < ArmCoreCount; CoreNum++) {
+    // Move secondary core to our new Pen
+    AmdStyxBringupSecondary (ArmCoreInfoTable, CoreNum, PenBase);
+  }
 }
 
diff --git a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
index 5ac210ba04e4..c660ccd04810 100644
--- a/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
+++ b/Platforms/AMD/Styx/Drivers/FdtDxe/FdtDxe.inf
@@ -50,6 +50,7 @@ 
   FdtLib
   DevicePathLib
   AmdStyxHelperLib
+  ArmSmcLib
 
 [LibraryClasses.AARCH64]
   ArmGicLib
@@ -63,6 +64,7 @@ 
 
 [Protocols]  
   gEfiFirmwareVolume2ProtocolGuid    ##CONSUMED
+  gAmdIscpDxeProtocolGuid            ## SOMETIMES_CONSUMES
  
 [Pcd]
   gAmdStyxTokenSpaceGuid.PcdStyxFdt
@@ -80,11 +82,7 @@ 
   gAmdStyxTokenSpaceGuid.PcdTrustedFWSupport
   gAmdStyxTokenSpaceGuid.PcdParkingProtocolBase
   gAmdStyxTokenSpaceGuid.PcdParkingProtocolSize
+  gAmdStyxTokenSpaceGuid.PcdIscpSupport
 
-[Pcd.AARCH64]
-  gArmTokenSpaceGuid.PcdGicDistributorBase
-  gArmTokenSpaceGuid.PcdGicSgiIntId
- 
 [Depex]
   TRUE
-