Re: [edk2-devel] [PATCH v4 0/3] ArmPkg,MdeModulePkg: Implement EFI_MP_SERVICES_PROTOCOL for AArch64 and add an MpServicesTest application to exercise it

["Rebecca Cran" <quic_rcran@quicinc.com>]

The patches aren't attached to the cover letter, but are separate 
emails. You should be able to find them by looking for "PATCH v4 1/3", 
"PATCH v4 2/3" etc.

I've made several changes to the WaitEvent handling in the latest patch: 
could you check if the problem has been fixed please?

-- 
Rebecca Cran

On 1/5/23 22:11, Kun Qin wrote:
> Hi Ard/Rebecca,
> 
> Thanks for bringing this to the mailing list. But somehow I cannot find 
> the patches sent along with this
> v4 cover letter. Could you please point me to them?
> 
> I have been running the previous version of this patch and noticed a 
> minor issue when the wait event is
> specified but the timeout is 0. In this case the CheckApStatus function 
> could fall into the timeout scenario
> regardless and signal the event timeout incorrectly. I attempted a 
> simple fix here:
> https://github.com/kuqin12/mu_silicon_arm_tiano/commit/591462cc32e621c1da470a8319f7deda723e8509
> 
> Please let me know if I misunderstood anything for the above case. 
> Thanks in advance!
> 
> Regards,
> Kun
> 
> On 1/5/2023 9:59 AM, Ard Biesheuvel wrote:
>> On Thu, 5 Jan 2023 at 18:39, Ard Biesheuvel <ardb@kernel.org> wrote:
>>> On Wed, 4 Jan 2023 at 16:37, Rebecca Cran <rebecca@quicinc.com> wrote:
>>>> Implement EFI_MP_SERVICES_PROTOCOL based on PSCI calls for AArch64, and
>>>> add an MpServicesTest application to exercise it.
>>>>
>>>> Changes from v3:
>>>>
>>>> Removed Ard's 'Reviewed-by' line from the commits since the code has 
>>>> changed
>>>> sufficiently that it's no longer valid.
>>>>
>>>> Rebecca Cran (3):
>>>>    ArmPkg: Add GET_MPIDR_AFFINITY_BITS and MPIDR_MT_BIT to ArmLib.h
>>>>    ArmPkg: implement EFI_MP_SERVICES_PROTOCOL based on PSCI calls
>>>>    MdeModulePkg: Add new Application/MpServicesTest application
>>>>
>>>>   ArmPkg/ArmPkg.dsc                                            |    1 +
>>>>   MdeModulePkg/MdeModulePkg.dsc                                |    2 +
>>>>   ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.inf |   56 +
>>>>   MdeModulePkg/Application/MpServicesTest/MpServicesTest.inf   |   40 +
>>>>   ArmPkg/Drivers/ArmPsciMpServicesDxe/MpServicesInternal.h     |  
>>>> 344 ++++
>>>>   ArmPkg/Include/Library/ArmLib.h                              |   
>>>> 16 +-
>>>>   MdeModulePkg/Application/MpServicesTest/Options.h            |   39 +
>>>>   ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.c   | 
>>>> 1847 ++++++++++++++++++++
>>>>   MdeModulePkg/Application/MpServicesTest/MpServicesTest.c     |  
>>>> 560 ++++++
>>>>   MdeModulePkg/Application/MpServicesTest/Options.c            |  
>>>> 164 ++
>>>>   ArmPkg/Drivers/ArmPsciMpServicesDxe/MpFuncs.S                |   57 +
>>>>   11 files changed, 3119 insertions(+), 7 deletions(-)
>>> Hello Rebecca,
>>>
>>> Thanks for reposting this.
>>>
>>> Running the secondaries with MMU and caches on is a huge improvement.
>>> I would suggest, though, that we enable the MMU first thing out of
>>> reset, and do so from asm code so we don't have to reason about the
>>> stack (pushing something with the MMU off and popping it with the MMU
>>> on requires cache maintenance as well, and there is no way this can be
>>> done from the code itself without help from the compiler)
>>>
>>> So I propose adding the diff below - note that only the variables
>>> holding TCR, MAIR and TTBR0 need cache maintenance now (in addition to
>>> the executable image) - everything else will be accessed by the
>>> secondaries with the MMU enabled.
>>>
>>> https://paste.debian.net/1266242/
>>>
>>> WIth a tweak to the RPI4 platform that I sent out separately, this all
>>> works fine for me (both with and without the diff above applied)
>>>
>> Actually, maybe better to retain the hunk below. I saw some weird
>> hangs without them
>>
>> diff --git a/ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.c
>> b/ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.c
>> index ab439bffd722..eb634a25b33a 100644
>> --- a/ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.c
>> +++ b/ArmPkg/Drivers/ArmPsciMpServicesDxe/ArmPsciMpServicesDxe.c
>> @@ -1345,18 +1345,6 @@ MpServicesInitialize (
>>
>>     gProcessorIDs[Index] = MAX_UINT64;
>>
>> -  //
>> -  // The global pointer variables as well as the gProcessorIDs array 
>> contents
>> -  // are accessed by the other cores so we must clean them to the PoC
>> -  //
>> -  WriteBackDataCacheRange (&gProcessorIDs, sizeof (UINT64 *));
>> -  WriteBackDataCacheRange (&gApStacksBase, sizeof (UINT64 *));
>> -
>> -  WriteBackDataCacheRange (
>> -    gProcessorIDs,
>> -    (mCpuMpData.NumberOfProcessors + 1) * sizeof (UINT64)
>> -    );
>> -
>>     mNonBlockingModeAllowed = TRUE;
>>
>>     Status = EfiCreateEventReadyToBootEx (
>>
>>
>> 
>>
>>