| Message ID | 1458851413-26577-3-git-send-email-leo.duran@amd.com |
|---|---|
| State | New |
| Headers | show |
On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Leo Duran <leo.duran@amd.com> > --- > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- > ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- > 2 files changed, 16 insertions(+), 4 deletions(-) > ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has the following defines #define ARM_VE_SYS_FLAGS_SET_REG (ARM_VE_BOARD_PERIPH_BASE + 0x00030) #define ARM_VE_SYS_FLAGS_CLR_REG (ARM_VE_BOARD_PERIPH_BASE + 0x00034) #define ARM_VE_SYS_FLAGS_NV_REG (ARM_VE_BOARD_PERIPH_BASE + 0x00038) and is used on Juno as well as the older 32-bit archs. I don't know if this is dead code now that PSCI is implemented by ARM trusted firmware, and so we never bring up the secondaries in UEFI. But I would like Leif's take on this when he gets back, since writing 64-bit values is clearly not correct either in this particular case. > diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > index fa544c7..8309f62 100644 > --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c > +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > @@ -80,13 +80,19 @@ SecondaryMain ( > ASSERT (Index != ArmCoreCount); > > // Clear Secondary cores MailBox > - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > + if (sizeof(UINTN) == sizeof(UINT64)) > + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > + else > + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > do { > ArmCallWFI (); > > // Read the Mailbox > - SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > + if (sizeof(UINTN) == sizeof(UINT64)) > + SecondaryEntryAddr = MmioRead64 (ArmCoreInfoTable[Index].MailboxGetAddress); > + else > + SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > > // Acknowledge the interrupt and send End of Interrupt signal. > AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 (PcdGicInterruptInterfaceBase), &InterruptId); > diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c > index 603f4bb..d7e2352 100644 > --- a/ArmPlatformPkg/PrePi/MainMPCore.c > +++ b/ArmPlatformPkg/PrePi/MainMPCore.c > @@ -79,13 +79,19 @@ SecondaryMain ( > ASSERT (Index != ArmCoreCount); > > // Clear Secondary cores MailBox > - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > + if (sizeof(UINTN) == sizeof(UINT64)) > + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > + else > + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > do { > ArmCallWFI (); > > // Read the Mailbox > - SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > + if (sizeof(UINTN) == sizeof(UINT64)) > + SecondaryEntryAddr = MmioRead64 (ArmCoreInfoTable[Index].MailboxGetAddress); > + else > + SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > > // Acknowledge the interrupt and send End of Interrupt signal. > AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 (PcdGicInterruptInterfaceBase), &InterruptId); > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 24 March 2016 at 23:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Leo Duran <leo.duran@amd.com> >> --- >> ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- >> ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- >> 2 files changed, 16 insertions(+), 4 deletions(-) >> > > > ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has the > following defines > > #define ARM_VE_SYS_FLAGS_SET_REG > (ARM_VE_BOARD_PERIPH_BASE + 0x00030) > #define ARM_VE_SYS_FLAGS_CLR_REG > (ARM_VE_BOARD_PERIPH_BASE + 0x00034) > #define ARM_VE_SYS_FLAGS_NV_REG > (ARM_VE_BOARD_PERIPH_BASE + 0x00038) > > and is used on Juno as well as the older 32-bit archs. I don't know if > this is dead code now that PSCI is implemented by ARM trusted > firmware, and so we never bring up the secondaries in UEFI. But I > would like Leif's take on this when he gets back, since writing 64-bit > values is clearly not correct either in this particular case. > Hi Leif, It would be good if you could shed some light on this one. It is not entirely clear to me how these mailboxes are backed on Vexpress, and whether we could support 64-bit addresses here. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 8 April 2016 at 13:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 24 March 2016 at 23:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Leo Duran <leo.duran@amd.com> >>> --- >>> ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- >>> ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- >>> 2 files changed, 16 insertions(+), 4 deletions(-) >>> >> >> >> ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has the >> following defines >> >> #define ARM_VE_SYS_FLAGS_SET_REG >> (ARM_VE_BOARD_PERIPH_BASE + 0x00030) >> #define ARM_VE_SYS_FLAGS_CLR_REG >> (ARM_VE_BOARD_PERIPH_BASE + 0x00034) >> #define ARM_VE_SYS_FLAGS_NV_REG >> (ARM_VE_BOARD_PERIPH_BASE + 0x00038) >> >> and is used on Juno as well as the older 32-bit archs. I don't know if >> this is dead code now that PSCI is implemented by ARM trusted >> firmware, and so we never bring up the secondaries in UEFI. But I >> would like Leif's take on this when he gets back, since writing 64-bit >> values is clearly not correct either in this particular case. >> > > Hi Leif, > > It would be good if you could shed some light on this one. It is not > entirely clear to me how these mailboxes are backed on Vexpress, and > whether we could support 64-bit addresses here. Ping? _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Mar 24, 2016 at 11:33:22PM +0100, Ard Biesheuvel wrote: > On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Leo Duran <leo.duran@amd.com> > > --- > > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- > > ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- > > 2 files changed, 16 insertions(+), 4 deletions(-) > > > > > ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has the > following defines > > #define ARM_VE_SYS_FLAGS_SET_REG > (ARM_VE_BOARD_PERIPH_BASE + 0x00030) > #define ARM_VE_SYS_FLAGS_CLR_REG > (ARM_VE_BOARD_PERIPH_BASE + 0x00034) > #define ARM_VE_SYS_FLAGS_NV_REG > (ARM_VE_BOARD_PERIPH_BASE + 0x00038) > > and is used on Juno as well as the older 32-bit archs. I don't know if > this is dead code now that PSCI is implemented by ARM trusted > firmware, and so we never bring up the secondaries in UEFI. But I > would like Leif's take on this when he gets back, since writing 64-bit > values is clearly not correct either in this particular case. Yeah, these remain 32-bit also on Juno. Moreover, only user I see of this module in public code is FVP (OpenPlatformPkg), which should probably be investigated. Leo: do you actually need PrePeiCoreMPCore in the current version of your platform code? / Leif > > diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > index fa544c7..8309f62 100644 > > --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > @@ -80,13 +80,19 @@ SecondaryMain ( > > ASSERT (Index != ArmCoreCount); > > > > // Clear Secondary cores MailBox > > - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > + if (sizeof(UINTN) == sizeof(UINT64)) > > + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > + else > > + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > > > do { > > ArmCallWFI (); > > > > // Read the Mailbox > > - SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > > + if (sizeof(UINTN) == sizeof(UINT64)) > > + SecondaryEntryAddr = MmioRead64 (ArmCoreInfoTable[Index].MailboxGetAddress); > > + else > > + SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > > > > // Acknowledge the interrupt and send End of Interrupt signal. > > AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 (PcdGicInterruptInterfaceBase), &InterruptId); > > diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c > > index 603f4bb..d7e2352 100644 > > --- a/ArmPlatformPkg/PrePi/MainMPCore.c > > +++ b/ArmPlatformPkg/PrePi/MainMPCore.c > > @@ -79,13 +79,19 @@ SecondaryMain ( > > ASSERT (Index != ArmCoreCount); > > > > // Clear Secondary cores MailBox > > - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > + if (sizeof(UINTN) == sizeof(UINT64)) > > + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > + else > > + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); > > > > do { > > ArmCallWFI (); > > > > // Read the Mailbox > > - SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > > + if (sizeof(UINTN) == sizeof(UINT64)) > > + SecondaryEntryAddr = MmioRead64 (ArmCoreInfoTable[Index].MailboxGetAddress); > > + else > > + SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); > > > > // Acknowledge the interrupt and send End of Interrupt signal. > > AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 (PcdGicInterruptInterfaceBase), &InterruptId); > > -- > > 1.9.1 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 18 April 2016 at 12:39, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Thu, Mar 24, 2016 at 11:33:22PM +0100, Ard Biesheuvel wrote: >> On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: >> > Contributed-under: TianoCore Contribution Agreement 1.0 >> > Signed-off-by: Leo Duran <leo.duran@amd.com> >> > --- >> > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- >> > ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- >> > 2 files changed, 16 insertions(+), 4 deletions(-) >> > >> >> >> ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has the >> following defines >> >> #define ARM_VE_SYS_FLAGS_SET_REG >> (ARM_VE_BOARD_PERIPH_BASE + 0x00030) >> #define ARM_VE_SYS_FLAGS_CLR_REG >> (ARM_VE_BOARD_PERIPH_BASE + 0x00034) >> #define ARM_VE_SYS_FLAGS_NV_REG >> (ARM_VE_BOARD_PERIPH_BASE + 0x00038) >> >> and is used on Juno as well as the older 32-bit archs. I don't know if >> this is dead code now that PSCI is implemented by ARM trusted >> firmware, and so we never bring up the secondaries in UEFI. But I >> would like Leif's take on this when he gets back, since writing 64-bit >> values is clearly not correct either in this particular case. > > Yeah, these remain 32-bit also on Juno. > > Moreover, only user I see of this module in public code is FVP > (OpenPlatformPkg), which should probably be investigated. > > Leo: do you actually need PrePeiCoreMPCore in the current version of > your platform code? > I suppose the question here is whether all cores enter UEFI or only the boot CPU. If the EL3 firmware is doing PSCI, then only the boot core enters UEFI, and this code could be dropped or simplified. Note that there are some checks in the code still that prevent you from running the unicore version on a SMP system, so that should still be addressed afair _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, Apr 18, 2016 at 10:58:26AM +0000, Bhupesh Sharma wrote: > We also should consider ARM SOCs which have Internal ROM code > running even before the ATF starts running in EL3. Such a internal > ROM code might be configured to make sure that only the primary core > runs the ATF and UEFI code and the secondaries enter the ATF code > first time only when Linux does a secondary CPU_ON call. Sure. But this code lives in ArmPlatformPkg, making it (theoretically) only relevant to ARM ltd. platforms. I know this line has historically been pretty blurred, but really, any code from here being used in other platforms should be looking to migrate to a different package. Regards, Leif > > Regards, > Bhupesh > > ________________________________________ > From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Monday, April 18, 2016 4:12:15 PM > To: Leif Lindholm > Cc: edk2-devel@lists.01.org; Leo Duran > Subject: Re: [edk2] [PATCH] ArmPlatformPkg: fixups for 64-bit mailbox pointers > > On 18 April 2016 at 12:39, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > On Thu, Mar 24, 2016 at 11:33:22PM +0100, Ard Biesheuvel wrote: > >> On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: > >> > Contributed-under: TianoCore Contribution Agreement 1.0 > >> > Signed-off-by: Leo Duran <leo.duran@amd.com> > >> > --- > >> > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- > >> > ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- > >> > 2 files changed, 16 insertions(+), 4 deletions(-) > >> > > >> > >> > >> ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has the > >> following defines > >> > >> #define ARM_VE_SYS_FLAGS_SET_REG > >> (ARM_VE_BOARD_PERIPH_BASE + 0x00030) > >> #define ARM_VE_SYS_FLAGS_CLR_REG > >> (ARM_VE_BOARD_PERIPH_BASE + 0x00034) > >> #define ARM_VE_SYS_FLAGS_NV_REG > >> (ARM_VE_BOARD_PERIPH_BASE + 0x00038) > >> > >> and is used on Juno as well as the older 32-bit archs. I don't know if > >> this is dead code now that PSCI is implemented by ARM trusted > >> firmware, and so we never bring up the secondaries in UEFI. But I > >> would like Leif's take on this when he gets back, since writing 64-bit > >> values is clearly not correct either in this particular case. > > > > Yeah, these remain 32-bit also on Juno. > > > > Moreover, only user I see of this module in public code is FVP > > (OpenPlatformPkg), which should probably be investigated. > > > > Leo: do you actually need PrePeiCoreMPCore in the current version of > > your platform code? > > > > I suppose the question here is whether all cores enter UEFI or only > the boot CPU. If the EL3 firmware is doing PSCI, then only the boot > core enters UEFI, and this code could be dropped or simplified. Note > that there are some checks in the code still that prevent you from > running the unicore version on a SMP system, so that should still be > addressed afair > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Leif, Please see my reply below. Leo. > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Monday, April 18, 2016 5:39 AM > To: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org > Subject: Re: [PATCH] ArmPlatformPkg: fixups for 64-bit mailbox pointers > > On Thu, Mar 24, 2016 at 11:33:22PM +0100, Ard Biesheuvel wrote: > > On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > Signed-off-by: Leo Duran <leo.duran@amd.com> > > > --- > > > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- > > > ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- > > > 2 files changed, 16 insertions(+), 4 deletions(-) > > > > > > > > > ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has > the > > following defines > > > > #define ARM_VE_SYS_FLAGS_SET_REG > > (ARM_VE_BOARD_PERIPH_BASE + 0x00030) > > #define ARM_VE_SYS_FLAGS_CLR_REG > > (ARM_VE_BOARD_PERIPH_BASE + 0x00034) > > #define ARM_VE_SYS_FLAGS_NV_REG > > (ARM_VE_BOARD_PERIPH_BASE + 0x00038) > > > > and is used on Juno as well as the older 32-bit archs. I don't know if > > this is dead code now that PSCI is implemented by ARM trusted > > firmware, and so we never bring up the secondaries in UEFI. But I > > would like Leif's take on this when he gets back, since writing 64-bit > > values is clearly not correct either in this particular case. > > Yeah, these remain 32-bit also on Juno. > > Moreover, only user I see of this module in public code is FVP > (OpenPlatformPkg), which should probably be investigated. > > Leo: do you actually need PrePeiCoreMPCore in the current version of your > platform code? > Leif, The "mailbox" signaling is used to support MP-boot in scenarios where PSCI may not be enabled. (E.g., boot core uses mailbox to park AP cores in a "pen", and from there the OS claims them via FDT's "spin-table" or ACPI's "Parked Address".) Note that the ARM_CORE_INFO structure defines Mailbox[Set/Get/Clear]Address as EFI_PHYSICAL_ADDRESS, which of course can be 64-bit. Leo. > / > Leif > > > > diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > > b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > > index fa544c7..8309f62 100644 > > > --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > > +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > > > @@ -80,13 +80,19 @@ SecondaryMain ( > > > ASSERT (Index != ArmCoreCount); > > > > > > // Clear Secondary cores MailBox > > > - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, > > > ArmCoreInfoTable[Index].MailboxClearValue); > > > + if (sizeof(UINTN) == sizeof(UINT64)) > > > + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, > > > + ArmCoreInfoTable[Index].MailboxClearValue); > > > + else > > > + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, > > > + ArmCoreInfoTable[Index].MailboxClearValue); > > > > > > do { > > > ArmCallWFI (); > > > > > > // Read the Mailbox > > > - SecondaryEntryAddr = MmioRead32 > (ArmCoreInfoTable[Index].MailboxGetAddress); > > > + if (sizeof(UINTN) == sizeof(UINT64)) > > > + SecondaryEntryAddr = MmioRead64 > (ArmCoreInfoTable[Index].MailboxGetAddress); > > > + else > > > + SecondaryEntryAddr = MmioRead32 > > > + (ArmCoreInfoTable[Index].MailboxGetAddress); > > > > > > // Acknowledge the interrupt and send End of Interrupt signal. > > > AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 > > > (PcdGicInterruptInterfaceBase), &InterruptId); diff --git > > > a/ArmPlatformPkg/PrePi/MainMPCore.c > > > b/ArmPlatformPkg/PrePi/MainMPCore.c > > > index 603f4bb..d7e2352 100644 > > > --- a/ArmPlatformPkg/PrePi/MainMPCore.c > > > +++ b/ArmPlatformPkg/PrePi/MainMPCore.c > > > @@ -79,13 +79,19 @@ SecondaryMain ( > > > ASSERT (Index != ArmCoreCount); > > > > > > // Clear Secondary cores MailBox > > > - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, > > > ArmCoreInfoTable[Index].MailboxClearValue); > > > + if (sizeof(UINTN) == sizeof(UINT64)) > > > + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, > > > + ArmCoreInfoTable[Index].MailboxClearValue); > > > + else > > > + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, > > > + ArmCoreInfoTable[Index].MailboxClearValue); > > > > > > do { > > > ArmCallWFI (); > > > > > > // Read the Mailbox > > > - SecondaryEntryAddr = MmioRead32 > (ArmCoreInfoTable[Index].MailboxGetAddress); > > > + if (sizeof(UINTN) == sizeof(UINT64)) > > > + SecondaryEntryAddr = MmioRead64 > (ArmCoreInfoTable[Index].MailboxGetAddress); > > > + else > > > + SecondaryEntryAddr = MmioRead32 > > > + (ArmCoreInfoTable[Index].MailboxGetAddress); > > > > > > // Acknowledge the interrupt and send End of Interrupt signal. > > > AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 > > > (PcdGicInterruptInterfaceBase), &InterruptId); > > > -- > > > 1.9.1 > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 19 April 2016 at 20:36, Duran, Leo <leo.duran@amd.com> wrote: > Leif, > Please see my reply below. > Leo. > >> -----Original Message----- >> From: Leif Lindholm [mailto:leif.lindholm@linaro.org] >> Sent: Monday, April 18, 2016 5:39 AM >> To: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org >> Subject: Re: [PATCH] ArmPlatformPkg: fixups for 64-bit mailbox pointers >> >> On Thu, Mar 24, 2016 at 11:33:22PM +0100, Ard Biesheuvel wrote: >> > On 24 March 2016 at 21:30, Leo Duran <leo.duran@amd.com> wrote: >> > > Contributed-under: TianoCore Contribution Agreement 1.0 >> > > Signed-off-by: Leo Duran <leo.duran@amd.com> >> > > --- >> > > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- >> > > ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- >> > > 2 files changed, 16 insertions(+), 4 deletions(-) >> > > >> > >> > >> > ArmPlatformPkg/ArmVExpressPkg/Include/VExpressMotherBoard.h has >> the >> > following defines >> > >> > #define ARM_VE_SYS_FLAGS_SET_REG >> > (ARM_VE_BOARD_PERIPH_BASE + 0x00030) >> > #define ARM_VE_SYS_FLAGS_CLR_REG >> > (ARM_VE_BOARD_PERIPH_BASE + 0x00034) >> > #define ARM_VE_SYS_FLAGS_NV_REG >> > (ARM_VE_BOARD_PERIPH_BASE + 0x00038) >> > >> > and is used on Juno as well as the older 32-bit archs. I don't know if >> > this is dead code now that PSCI is implemented by ARM trusted >> > firmware, and so we never bring up the secondaries in UEFI. But I >> > would like Leif's take on this when he gets back, since writing 64-bit >> > values is clearly not correct either in this particular case. >> >> Yeah, these remain 32-bit also on Juno. >> >> Moreover, only user I see of this module in public code is FVP >> (OpenPlatformPkg), which should probably be investigated. >> >> Leo: do you actually need PrePeiCoreMPCore in the current version of your >> platform code? >> > > Leif, > The "mailbox" signaling is used to support MP-boot in scenarios where PSCI may not be enabled. > (E.g., boot core uses mailbox to park AP cores in a "pen", and from there the OS claims them via FDT's "spin-table" or ACPI's "Parked Address".) > So in absence of PSCI support in EL3, the firmware residing there will boot all cores into UEFI? > Note that the ARM_CORE_INFO structure defines Mailbox[Set/Get/Clear]Address as EFI_PHYSICAL_ADDRESS, which of course can be 64-bit. > Leo. > So the question is why the mailbox itself needs to be inside some magic peripheral, whereas the pen itself is simply in RAM. In fact, the PrePi version looks fairly broken in this regard, since the RAM it executes from doesn't look like it has any kind of protection from being clobbered by the kernel as soon as it loads if you are not using the linux loader. I guess the bottom line is that the ARM MPcore stuff in Tianocore is badly broken, and we should really keep the secondaries spinning in EL3 until the kernel is ready to release them. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c index fa544c7..8309f62 100644 --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c @@ -80,13 +80,19 @@ SecondaryMain ( ASSERT (Index != ArmCoreCount); // Clear Secondary cores MailBox - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); + if (sizeof(UINTN) == sizeof(UINT64)) + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); + else + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); do { ArmCallWFI (); // Read the Mailbox - SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); + if (sizeof(UINTN) == sizeof(UINT64)) + SecondaryEntryAddr = MmioRead64 (ArmCoreInfoTable[Index].MailboxGetAddress); + else + SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); // Acknowledge the interrupt and send End of Interrupt signal. AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 (PcdGicInterruptInterfaceBase), &InterruptId); diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c index 603f4bb..d7e2352 100644 --- a/ArmPlatformPkg/PrePi/MainMPCore.c +++ b/ArmPlatformPkg/PrePi/MainMPCore.c @@ -79,13 +79,19 @@ SecondaryMain ( ASSERT (Index != ArmCoreCount); // Clear Secondary cores MailBox - MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); + if (sizeof(UINTN) == sizeof(UINT64)) + MmioWrite64 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); + else + MmioWrite32 (ArmCoreInfoTable[Index].MailboxClearAddress, ArmCoreInfoTable[Index].MailboxClearValue); do { ArmCallWFI (); // Read the Mailbox - SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); + if (sizeof(UINTN) == sizeof(UINT64)) + SecondaryEntryAddr = MmioRead64 (ArmCoreInfoTable[Index].MailboxGetAddress); + else + SecondaryEntryAddr = MmioRead32 (ArmCoreInfoTable[Index].MailboxGetAddress); // Acknowledge the interrupt and send End of Interrupt signal. AcknowledgeInterrupt = ArmGicAcknowledgeInterrupt (PcdGet32 (PcdGicInterruptInterfaceBase), &InterruptId);
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Leo Duran <leo.duran@amd.com> --- ArmPlatformPkg/PrePeiCore/MainMPCore.c | 10 ++++++++-- ArmPlatformPkg/PrePi/MainMPCore.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel