| Message ID | 1462196143-21998-7-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 >
> -----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 > >
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.
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 -
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(-)