public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
@ 2018-06-20 17:23 Andrew Fish
  2018-06-21  3:06 ` Ni, Ruiyu
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Fish @ 2018-06-20 17:23 UTC (permalink / raw)
  To: edk2-devel

Is there some MP safe contract with the PEI Core implementation I don't know about? 

Looks like the APs are calling PeiServicesGetHobList() to figure out "who they are"? How does this work? Or am I missing something?


https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c#L1945

/**
  This return the handle number for the calling processor.  This service may be
  called from the BSP and APs.
  @param[out] ProcessorNumber  Pointer to the handle number of AP.
                               The range is from 0 to the total number of
                               logical processors minus 1. The total number of
                               logical processors can be retrieved by
                               MpInitLibGetNumberOfProcessors().
  @retval EFI_SUCCESS             The current processor handle number was returned
                                  in ProcessorNumber.
  @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
  @retval EFI_NOT_READY           MP Initialize Library is not initialized.
**/
EFI_STATUS
EFIAPI
MpInitLibWhoAmI (
  OUT UINTN                    *ProcessorNumber
  )
{
  CPU_MP_DATA           *CpuMpData;

  if (ProcessorNumber == NULL) {
    return EFI_INVALID_PARAMETER;
  }

  CpuMpData = GetCpuMpData ();

  return GetProcessorNumber (CpuMpData, ProcessorNumber);
}


https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c#L34

/**
  Get pointer to CPU MP Data structure.
  @return  The pointer to CPU MP Data structure.
**/
CPU_MP_DATA *
GetCpuMpData (
  VOID
  )
{
  CPU_MP_DATA      *CpuMpData;

  CpuMpData = GetCpuMpDataFromGuidedHob ();
  ASSERT (CpuMpData != NULL);
  return CpuMpData;
}

https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c#L2302


/**
  Get pointer to CPU MP Data structure from GUIDed HOB.
  @return  The pointer to CPU MP Data structure.
**/
CPU_MP_DATA *
GetCpuMpDataFromGuidedHob (
  VOID
  )
{
  EFI_HOB_GUID_TYPE       *GuidHob;
  VOID                    *DataInHob;
  CPU_MP_DATA             *CpuMpData;

  CpuMpData = NULL;
  GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);
  if (GuidHob != NULL) {
    DataInHob = GET_GUID_HOB_DATA (GuidHob);
    CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);
  }
  return CpuMpData;
}

Thanks,

Andrew Fish


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
  2018-06-20 17:23 Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs? Andrew Fish
@ 2018-06-21  3:06 ` Ni, Ruiyu
  2018-06-21  3:24   ` Andrew Fish
  0 siblings, 1 reply; 7+ messages in thread
From: Ni, Ruiyu @ 2018-06-21  3:06 UTC (permalink / raw)
  To: Andrew Fish, edk2-devel

Andrew,
Good catch! It does break the rule that AP shouldn't call PEI services.
But considering the specific case, it should be safe.
Because:
1. HOB only stores a pointer to the buffer that contains all the MP information.
2. BSP modifies the HOB by only appending data to the end. It may modifies some HOB content. But MpInitLib
implementation itself doesn't modify the pointer value stored in HOB.

In PEI environment where global variable is read-only, it's hard to not rely on HOB.
I guess that's the reason of today's tricky implementation.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Andrew
> Fish
> Sent: Thursday, June 21, 2018 1:23 AM
> To: edk2-devel <edk2-devel@lists.01.org>
> Subject: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling
> GetFirstGuidHob on the APs?
> 
> Is there some MP safe contract with the PEI Core implementation I don't
> know about?
> 
> Looks like the APs are calling PeiServicesGetHobList() to figure out "who they
> are"? How does this work? Or am I missing something?
> 
> 
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
> tLib/MpLib.c#L1945
> 
> /**
>   This return the handle number for the calling processor.  This service may be
>   called from the BSP and APs.
>   @param[out] ProcessorNumber  Pointer to the handle number of AP.
>                                The range is from 0 to the total number of
>                                logical processors minus 1. The total number of
>                                logical processors can be retrieved by
>                                MpInitLibGetNumberOfProcessors().
>   @retval EFI_SUCCESS             The current processor handle number was
> returned
>                                   in ProcessorNumber.
>   @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
>   @retval EFI_NOT_READY           MP Initialize Library is not initialized.
> **/
> EFI_STATUS
> EFIAPI
> MpInitLibWhoAmI (
>   OUT UINTN                    *ProcessorNumber
>   )
> {
>   CPU_MP_DATA           *CpuMpData;
> 
>   if (ProcessorNumber == NULL) {
>     return EFI_INVALID_PARAMETER;
>   }
> 
>   CpuMpData = GetCpuMpData ();
> 
>   return GetProcessorNumber (CpuMpData, ProcessorNumber); }
> 
> 
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
> tLib/PeiMpLib.c#L34
> 
> /**
>   Get pointer to CPU MP Data structure.
>   @return  The pointer to CPU MP Data structure.
> **/
> CPU_MP_DATA *
> GetCpuMpData (
>   VOID
>   )
> {
>   CPU_MP_DATA      *CpuMpData;
> 
>   CpuMpData = GetCpuMpDataFromGuidedHob ();
>   ASSERT (CpuMpData != NULL);
>   return CpuMpData;
> }
> 
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
> tLib/MpLib.c#L2302
> 
> 
> /**
>   Get pointer to CPU MP Data structure from GUIDed HOB.
>   @return  The pointer to CPU MP Data structure.
> **/
> CPU_MP_DATA *
> GetCpuMpDataFromGuidedHob (
>   VOID
>   )
> {
>   EFI_HOB_GUID_TYPE       *GuidHob;
>   VOID                    *DataInHob;
>   CPU_MP_DATA             *CpuMpData;
> 
>   CpuMpData = NULL;
>   GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);
>   if (GuidHob != NULL) {
>     DataInHob = GET_GUID_HOB_DATA (GuidHob);
>     CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);
>   }
>   return CpuMpData;
> }
> 
> Thanks,
> 
> Andrew Fish
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
  2018-06-21  3:06 ` Ni, Ruiyu
@ 2018-06-21  3:24   ` Andrew Fish
  2018-06-21  3:34     ` Ni, Ruiyu
  2018-06-22  6:52     ` 答复: " Fan Jeff
  0 siblings, 2 replies; 7+ messages in thread
From: Andrew Fish @ 2018-06-21  3:24 UTC (permalink / raw)
  To: Ni, Ruiyu; +Cc: edk2-devel



> On Jun 20, 2018, at 8:06 PM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> 
> Andrew,
> Good catch! It does break the rule that AP shouldn't call PEI services.
> But considering the specific case, it should be safe.
> Because:
> 1. HOB only stores a pointer to the buffer that contains all the MP information.
> 2. BSP modifies the HOB by only appending data to the end. It may modifies some HOB content. But MpInitLib
> implementation itself doesn't modify the pointer value stored in HOB.
> 

Ray,

I think the should be safe is also making assumptions about the debug and a few other library instances that got tested against. It is not really an architectural guaranty against the library classes that could be linked. We don't use the default 

It would be good to use an IDT entry, or optionally a global variable if memory is present, longer term. 

Thanks,

Andrew Fish

> In PEI environment where global variable is read-only, it's hard to not rely on HOB.
> I guess that's the reason of today's tricky implementation.
> 
> Thanks/Ray
> 
>> -----Original Message-----
>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Andrew
>> Fish
>> Sent: Thursday, June 21, 2018 1:23 AM
>> To: edk2-devel <edk2-devel@lists.01.org>
>> Subject: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling
>> GetFirstGuidHob on the APs?
>> 
>> Is there some MP safe contract with the PEI Core implementation I don't
>> know about?
>> 
>> Looks like the APs are calling PeiServicesGetHobList() to figure out "who they
>> are"? How does this work? Or am I missing something?
>> 
>> 
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>> tLib/MpLib.c#L1945
>> 
>> /**
>>  This return the handle number for the calling processor.  This service may be
>>  called from the BSP and APs.
>>  @param[out] ProcessorNumber  Pointer to the handle number of AP.
>>                               The range is from 0 to the total number of
>>                               logical processors minus 1. The total number of
>>                               logical processors can be retrieved by
>>                               MpInitLibGetNumberOfProcessors().
>>  @retval EFI_SUCCESS             The current processor handle number was
>> returned
>>                                  in ProcessorNumber.
>>  @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
>>  @retval EFI_NOT_READY           MP Initialize Library is not initialized.
>> **/
>> EFI_STATUS
>> EFIAPI
>> MpInitLibWhoAmI (
>>  OUT UINTN                    *ProcessorNumber
>>  )
>> {
>>  CPU_MP_DATA           *CpuMpData;
>> 
>>  if (ProcessorNumber == NULL) {
>>    return EFI_INVALID_PARAMETER;
>>  }
>> 
>>  CpuMpData = GetCpuMpData ();
>> 
>>  return GetProcessorNumber (CpuMpData, ProcessorNumber); }
>> 
>> 
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>> tLib/PeiMpLib.c#L34
>> 
>> /**
>>  Get pointer to CPU MP Data structure.
>>  @return  The pointer to CPU MP Data structure.
>> **/
>> CPU_MP_DATA *
>> GetCpuMpData (
>>  VOID
>>  )
>> {
>>  CPU_MP_DATA      *CpuMpData;
>> 
>>  CpuMpData = GetCpuMpDataFromGuidedHob ();
>>  ASSERT (CpuMpData != NULL);
>>  return CpuMpData;
>> }
>> 
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>> tLib/MpLib.c#L2302
>> 
>> 
>> /**
>>  Get pointer to CPU MP Data structure from GUIDed HOB.
>>  @return  The pointer to CPU MP Data structure.
>> **/
>> CPU_MP_DATA *
>> GetCpuMpDataFromGuidedHob (
>>  VOID
>>  )
>> {
>>  EFI_HOB_GUID_TYPE       *GuidHob;
>>  VOID                    *DataInHob;
>>  CPU_MP_DATA             *CpuMpData;
>> 
>>  CpuMpData = NULL;
>>  GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);
>>  if (GuidHob != NULL) {
>>    DataInHob = GET_GUID_HOB_DATA (GuidHob);
>>    CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);
>>  }
>>  return CpuMpData;
>> }
>> 
>> Thanks,
>> 
>> Andrew Fish
>> _______________________________________________
>> 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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
  2018-06-21  3:24   ` Andrew Fish
@ 2018-06-21  3:34     ` Ni, Ruiyu
  2018-06-21  5:57       ` Dong, Eric
  2018-06-22  6:52     ` 答复: " Fan Jeff
  1 sibling, 1 reply; 7+ messages in thread
From: Ni, Ruiyu @ 2018-06-21  3:34 UTC (permalink / raw)
  To: afish@apple.com; +Cc: edk2-devel, Fan Jeff, Dong, Eric, Gao, Liming

Andrew,
Today's MpInitLib does depend on physical memory (the waking up vector sits below 1MB).
So the MpPei module can use RegisterForShadow() to make sure it is loaded in memory before running.
Then the module global variable can be used.
But when system is in S3 boot path, it seems PeiCore doesn't follow RegisterForShadow() to load the PEIM
in memory.

Using IDT entry as the storage of global variable is a good idea. We will evaluate.

Thanks/Ray

> -----Original Message-----
> From: afish@apple.com <afish@apple.com>
> Sent: Thursday, June 21, 2018 11:25 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: edk2-devel <edk2-devel@lists.01.org>
> Subject: Re: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so
> calling GetFirstGuidHob on the APs?
> 
> 
> 
> > On Jun 20, 2018, at 8:06 PM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >
> > Andrew,
> > Good catch! It does break the rule that AP shouldn't call PEI services.
> > But considering the specific case, it should be safe.
> > Because:
> > 1. HOB only stores a pointer to the buffer that contains all the MP
> information.
> > 2. BSP modifies the HOB by only appending data to the end. It may
> > modifies some HOB content. But MpInitLib implementation itself doesn't
> modify the pointer value stored in HOB.
> >
> 
> Ray,
> 
> I think the should be safe is also making assumptions about the debug and a
> few other library instances that got tested against. It is not really an
> architectural guaranty against the library classes that could be linked. We
> don't use the default
> 
> It would be good to use an IDT entry, or optionally a global variable if memory
> is present, longer term.
> 
> Thanks,
> 
> Andrew Fish
> 
> > In PEI environment where global variable is read-only, it's hard to not rely
> on HOB.
> > I guess that's the reason of today's tricky implementation.
> >
> > Thanks/Ray
> >
> >> -----Original Message-----
> >> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> >> Andrew Fish
> >> Sent: Thursday, June 21, 2018 1:23 AM
> >> To: edk2-devel <edk2-devel@lists.01.org>
> >> Subject: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so
> >> calling GetFirstGuidHob on the APs?
> >>
> >> Is there some MP safe contract with the PEI Core implementation I
> >> don't know about?
> >>
> >> Looks like the APs are calling PeiServicesGetHobList() to figure out
> >> "who they are"? How does this work? Or am I missing something?
> >>
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIn
> >> i
> >> tLib/MpLib.c#L1945
> >>
> >> /**
> >>  This return the handle number for the calling processor.  This
> >> service may be  called from the BSP and APs.
> >>  @param[out] ProcessorNumber  Pointer to the handle number of AP.
> >>                               The range is from 0 to the total number of
> >>                               logical processors minus 1. The total number of
> >>                               logical processors can be retrieved by
> >>                               MpInitLibGetNumberOfProcessors().
> >>  @retval EFI_SUCCESS             The current processor handle number was
> >> returned
> >>                                  in ProcessorNumber.
> >>  @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
> >>  @retval EFI_NOT_READY           MP Initialize Library is not initialized.
> >> **/
> >> EFI_STATUS
> >> EFIAPI
> >> MpInitLibWhoAmI (
> >>  OUT UINTN                    *ProcessorNumber
> >>  )
> >> {
> >>  CPU_MP_DATA           *CpuMpData;
> >>
> >>  if (ProcessorNumber == NULL) {
> >>    return EFI_INVALID_PARAMETER;
> >>  }
> >>
> >>  CpuMpData = GetCpuMpData ();
> >>
> >>  return GetProcessorNumber (CpuMpData, ProcessorNumber); }
> >>
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIn
> >> i
> >> tLib/PeiMpLib.c#L34
> >>
> >> /**
> >>  Get pointer to CPU MP Data structure.
> >>  @return  The pointer to CPU MP Data structure.
> >> **/
> >> CPU_MP_DATA *
> >> GetCpuMpData (
> >>  VOID
> >>  )
> >> {
> >>  CPU_MP_DATA      *CpuMpData;
> >>
> >>  CpuMpData = GetCpuMpDataFromGuidedHob ();  ASSERT
> (CpuMpData !=
> >> NULL);  return CpuMpData; }
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIn
> >> i
> >> tLib/MpLib.c#L2302
> >>
> >>
> >> /**
> >>  Get pointer to CPU MP Data structure from GUIDed HOB.
> >>  @return  The pointer to CPU MP Data structure.
> >> **/
> >> CPU_MP_DATA *
> >> GetCpuMpDataFromGuidedHob (
> >>  VOID
> >>  )
> >> {
> >>  EFI_HOB_GUID_TYPE       *GuidHob;
> >>  VOID                    *DataInHob;
> >>  CPU_MP_DATA             *CpuMpData;
> >>
> >>  CpuMpData = NULL;
> >>  GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);  if (GuidHob !=
> >> NULL) {
> >>    DataInHob = GET_GUID_HOB_DATA (GuidHob);
> >>    CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);  }  return
> >> CpuMpData; }
> >>
> >> Thanks,
> >>
> >> Andrew Fish
> >> _______________________________________________
> >> 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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
  2018-06-21  3:34     ` Ni, Ruiyu
@ 2018-06-21  5:57       ` Dong, Eric
  0 siblings, 0 replies; 7+ messages in thread
From: Dong, Eric @ 2018-06-21  5:57 UTC (permalink / raw)
  To: Ni, Ruiyu, afish@apple.com; +Cc: edk2-devel, Fan Jeff, Gao, Liming

Hi Ruiyu & Andrew,

I have submit https://bugzilla.tianocore.org/show_bug.cgi?id=993 for this issue and will follow up to fix it.

Thanks,
Eric

-----Original Message-----
From: Ni, Ruiyu 
Sent: Thursday, June 21, 2018 11:35 AM
To: afish@apple.com
Cc: edk2-devel <edk2-devel@lists.01.org>; Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?

Andrew,
Today's MpInitLib does depend on physical memory (the waking up vector sits below 1MB).
So the MpPei module can use RegisterForShadow() to make sure it is loaded in memory before running.
Then the module global variable can be used.
But when system is in S3 boot path, it seems PeiCore doesn't follow RegisterForShadow() to load the PEIM in memory.

Using IDT entry as the storage of global variable is a good idea. We will evaluate.

Thanks/Ray

> -----Original Message-----
> From: afish@apple.com <afish@apple.com>
> Sent: Thursday, June 21, 2018 11:25 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: edk2-devel <edk2-devel@lists.01.org>
> Subject: Re: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think 
> so calling GetFirstGuidHob on the APs?
> 
> 
> 
> > On Jun 20, 2018, at 8:06 PM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >
> > Andrew,
> > Good catch! It does break the rule that AP shouldn't call PEI services.
> > But considering the specific case, it should be safe.
> > Because:
> > 1. HOB only stores a pointer to the buffer that contains all the MP
> information.
> > 2. BSP modifies the HOB by only appending data to the end. It may 
> > modifies some HOB content. But MpInitLib implementation itself 
> > doesn't
> modify the pointer value stored in HOB.
> >
> 
> Ray,
> 
> I think the should be safe is also making assumptions about the debug 
> and a few other library instances that got tested against. It is not 
> really an architectural guaranty against the library classes that 
> could be linked. We don't use the default
> 
> It would be good to use an IDT entry, or optionally a global variable 
> if memory is present, longer term.
> 
> Thanks,
> 
> Andrew Fish
> 
> > In PEI environment where global variable is read-only, it's hard to 
> > not rely
> on HOB.
> > I guess that's the reason of today's tricky implementation.
> >
> > Thanks/Ray
> >
> >> -----Original Message-----
> >> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of 
> >> Andrew Fish
> >> Sent: Thursday, June 21, 2018 1:23 AM
> >> To: edk2-devel <edk2-devel@lists.01.org>
> >> Subject: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think 
> >> so calling GetFirstGuidHob on the APs?
> >>
> >> Is there some MP safe contract with the PEI Core implementation I 
> >> don't know about?
> >>
> >> Looks like the APs are calling PeiServicesGetHobList() to figure 
> >> out "who they are"? How does this work? Or am I missing something?
> >>
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIn
> >> i
> >> tLib/MpLib.c#L1945
> >>
> >> /**
> >>  This return the handle number for the calling processor.  This 
> >> service may be  called from the BSP and APs.
> >>  @param[out] ProcessorNumber  Pointer to the handle number of AP.
> >>                               The range is from 0 to the total number of
> >>                               logical processors minus 1. The total number of
> >>                               logical processors can be retrieved by
> >>                               MpInitLibGetNumberOfProcessors().
> >>  @retval EFI_SUCCESS             The current processor handle number was
> >> returned
> >>                                  in ProcessorNumber.
> >>  @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
> >>  @retval EFI_NOT_READY           MP Initialize Library is not initialized.
> >> **/
> >> EFI_STATUS
> >> EFIAPI
> >> MpInitLibWhoAmI (
> >>  OUT UINTN                    *ProcessorNumber
> >>  )
> >> {
> >>  CPU_MP_DATA           *CpuMpData;
> >>
> >>  if (ProcessorNumber == NULL) {
> >>    return EFI_INVALID_PARAMETER;
> >>  }
> >>
> >>  CpuMpData = GetCpuMpData ();
> >>
> >>  return GetProcessorNumber (CpuMpData, ProcessorNumber); }
> >>
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIn
> >> i
> >> tLib/PeiMpLib.c#L34
> >>
> >> /**
> >>  Get pointer to CPU MP Data structure.
> >>  @return  The pointer to CPU MP Data structure.
> >> **/
> >> CPU_MP_DATA *
> >> GetCpuMpData (
> >>  VOID
> >>  )
> >> {
> >>  CPU_MP_DATA      *CpuMpData;
> >>
> >>  CpuMpData = GetCpuMpDataFromGuidedHob ();  ASSERT
> (CpuMpData !=
> >> NULL);  return CpuMpData; }
> >>
> >>
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIn
> >> i
> >> tLib/MpLib.c#L2302
> >>
> >>
> >> /**
> >>  Get pointer to CPU MP Data structure from GUIDed HOB.
> >>  @return  The pointer to CPU MP Data structure.
> >> **/
> >> CPU_MP_DATA *
> >> GetCpuMpDataFromGuidedHob (
> >>  VOID
> >>  )
> >> {
> >>  EFI_HOB_GUID_TYPE       *GuidHob;
> >>  VOID                    *DataInHob;
> >>  CPU_MP_DATA             *CpuMpData;
> >>
> >>  CpuMpData = NULL;
> >>  GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);  if (GuidHob !=
> >> NULL) {
> >>    DataInHob = GET_GUID_HOB_DATA (GuidHob);
> >>    CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);  }  return 
> >> CpuMpData; }
> >>
> >> Thanks,
> >>
> >> Andrew Fish
> >> _______________________________________________
> >> 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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* 答复: Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
  2018-06-21  3:24   ` Andrew Fish
  2018-06-21  3:34     ` Ni, Ruiyu
@ 2018-06-22  6:52     ` Fan Jeff
  2018-06-22 17:32       ` Andrew Fish
  1 sibling, 1 reply; 7+ messages in thread
From: Fan Jeff @ 2018-06-22  6:52 UTC (permalink / raw)
  To: Andrew Fish, Ni, Ruiyu; +Cc: edk2-devel

Andrew,

Not all services are permitted for AP routines. For example, MP->WhoAmI() could be invoked by AP by specified

Current implementation should be safe. I also agree with IDT entry solution, as we did in DebugAgentLib.

Thanks!
Jeff

发件人: Andrew Fish<mailto:afish@apple.com>
发送时间: 2018年6月21日 11:25
收件人: Ni, Ruiyu<mailto:ruiyu.ni@intel.com>
抄送: edk2-devel<mailto:edk2-devel@lists.01.org>
主题: Re: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?



> On Jun 20, 2018, at 8:06 PM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>
> Andrew,
> Good catch! It does break the rule that AP shouldn't call PEI services.
> But considering the specific case, it should be safe.
> Because:
> 1. HOB only stores a pointer to the buffer that contains all the MP information.
> 2. BSP modifies the HOB by only appending data to the end. It may modifies some HOB content. But MpInitLib
> implementation itself doesn't modify the pointer value stored in HOB.
>

Ray,

I think the should be safe is also making assumptions about the debug and a few other library instances that got tested against. It is not really an architectural guaranty against the library classes that could be linked. We don't use the default

It would be good to use an IDT entry, or optionally a global variable if memory is present, longer term.

Thanks,

Andrew Fish

> In PEI environment where global variable is read-only, it's hard to not rely on HOB.
> I guess that's the reason of today's tricky implementation.
>
> Thanks/Ray
>
>> -----Original Message-----
>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Andrew
>> Fish
>> Sent: Thursday, June 21, 2018 1:23 AM
>> To: edk2-devel <edk2-devel@lists.01.org>
>> Subject: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling
>> GetFirstGuidHob on the APs?
>>
>> Is there some MP safe contract with the PEI Core implementation I don't
>> know about?
>>
>> Looks like the APs are calling PeiServicesGetHobList() to figure out "who they
>> are"? How does this work? Or am I missing something?
>>
>>
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>> tLib/MpLib.c#L1945
>>
>> /**
>>  This return the handle number for the calling processor.  This service may be
>>  called from the BSP and APs.
>>  @param[out] ProcessorNumber  Pointer to the handle number of AP.
>>                               The range is from 0 to the total number of
>>                               logical processors minus 1. The total number of
>>                               logical processors can be retrieved by
>>                               MpInitLibGetNumberOfProcessors().
>>  @retval EFI_SUCCESS             The current processor handle number was
>> returned
>>                                  in ProcessorNumber.
>>  @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
>>  @retval EFI_NOT_READY           MP Initialize Library is not initialized.
>> **/
>> EFI_STATUS
>> EFIAPI
>> MpInitLibWhoAmI (
>>  OUT UINTN                    *ProcessorNumber
>>  )
>> {
>>  CPU_MP_DATA           *CpuMpData;
>>
>>  if (ProcessorNumber == NULL) {
>>    return EFI_INVALID_PARAMETER;
>>  }
>>
>>  CpuMpData = GetCpuMpData ();
>>
>>  return GetProcessorNumber (CpuMpData, ProcessorNumber); }
>>
>>
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>> tLib/PeiMpLib.c#L34
>>
>> /**
>>  Get pointer to CPU MP Data structure.
>>  @return  The pointer to CPU MP Data structure.
>> **/
>> CPU_MP_DATA *
>> GetCpuMpData (
>>  VOID
>>  )
>> {
>>  CPU_MP_DATA      *CpuMpData;
>>
>>  CpuMpData = GetCpuMpDataFromGuidedHob ();
>>  ASSERT (CpuMpData != NULL);
>>  return CpuMpData;
>> }
>>
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>> tLib/MpLib.c#L2302
>>
>>
>> /**
>>  Get pointer to CPU MP Data structure from GUIDed HOB.
>>  @return  The pointer to CPU MP Data structure.
>> **/
>> CPU_MP_DATA *
>> GetCpuMpDataFromGuidedHob (
>>  VOID
>>  )
>> {
>>  EFI_HOB_GUID_TYPE       *GuidHob;
>>  VOID                    *DataInHob;
>>  CPU_MP_DATA             *CpuMpData;
>>
>>  CpuMpData = NULL;
>>  GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);
>>  if (GuidHob != NULL) {
>>    DataInHob = GET_GUID_HOB_DATA (GuidHob);
>>    CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);
>>  }
>>  return CpuMpData;
>> }
>>
>> Thanks,
>>
>> Andrew Fish
>> _______________________________________________
>> 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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: 答复: Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
  2018-06-22  6:52     ` 答复: " Fan Jeff
@ 2018-06-22 17:32       ` Andrew Fish
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Fish @ 2018-06-22 17:32 UTC (permalink / raw)
  To: Fan Jeff; +Cc: Ni, Ruiyu, edk2-devel

Jeff,

I forgot to mention I found this issue by inspection, but I I'm chasing 2 MP issues in our internal code base. I'm not sure if it is the UefiCpuPkg code, the consumers code, or something else that is causing the issues. 
1) I'm seeing an intermittent issue in PEI and it looks like one of the APs is getting disabled, and a required configuration step is being skipped. 
2) I turned on the mwait AP Idle (I also add BSP mwait idle) and one of our very complicated DXE MP functions started failing. 

I'm hoping to get an ITP (JTAG debugger) setup next week and get more info on what is happening. 

Thanks,

Andrew Fish

> On Jun 21, 2018, at 11:52 PM, Fan Jeff <vanjeff_919@hotmail.com> wrote:
> 
> Andrew,
> 
> Not all services are permitted for AP routines. For example, MP->WhoAmI() could be invoked by AP by specified
> 
> Current implementation should be safe. I also agree with IDT entry solution, as we did in DebugAgentLib.
> 
> Thanks!
> Jeff
> 
> 发件人: Andrew Fish<mailto:afish@apple.com>
> 发送时间: 2018年6月21日 11:25
> 收件人: Ni, Ruiyu<mailto:ruiyu.ni@intel.com>
> 抄送: edk2-devel<mailto:edk2-devel@lists.01.org>
> 主题: Re: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs?
> 
> 
> 
>> On Jun 20, 2018, at 8:06 PM, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>> 
>> Andrew,
>> Good catch! It does break the rule that AP shouldn't call PEI services.
>> But considering the specific case, it should be safe.
>> Because:
>> 1. HOB only stores a pointer to the buffer that contains all the MP information.
>> 2. BSP modifies the HOB by only appending data to the end. It may modifies some HOB content. But MpInitLib
>> implementation itself doesn't modify the pointer value stored in HOB.
>> 
> 
> Ray,
> 
> I think the should be safe is also making assumptions about the debug and a few other library instances that got tested against. It is not really an architectural guaranty against the library classes that could be linked. We don't use the default
> 
> It would be good to use an IDT entry, or optionally a global variable if memory is present, longer term.
> 
> Thanks,
> 
> Andrew Fish
> 
>> In PEI environment where global variable is read-only, it's hard to not rely on HOB.
>> I guess that's the reason of today's tricky implementation.
>> 
>> Thanks/Ray
>> 
>>> -----Original Message-----
>>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Andrew
>>> Fish
>>> Sent: Thursday, June 21, 2018 1:23 AM
>>> To: edk2-devel <edk2-devel@lists.01.org>
>>> Subject: [edk2] Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling
>>> GetFirstGuidHob on the APs?
>>> 
>>> Is there some MP safe contract with the PEI Core implementation I don't
>>> know about?
>>> 
>>> Looks like the APs are calling PeiServicesGetHobList() to figure out "who they
>>> are"? How does this work? Or am I missing something?
>>> 
>>> 
>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>>> tLib/MpLib.c#L1945
>>> 
>>> /**
>>> This return the handle number for the calling processor.  This service may be
>>> called from the BSP and APs.
>>> @param[out] ProcessorNumber  Pointer to the handle number of AP.
>>>                              The range is from 0 to the total number of
>>>                              logical processors minus 1. The total number of
>>>                              logical processors can be retrieved by
>>>                              MpInitLibGetNumberOfProcessors().
>>> @retval EFI_SUCCESS             The current processor handle number was
>>> returned
>>>                                 in ProcessorNumber.
>>> @retval EFI_INVALID_PARAMETER   ProcessorNumber is NULL.
>>> @retval EFI_NOT_READY           MP Initialize Library is not initialized.
>>> **/
>>> EFI_STATUS
>>> EFIAPI
>>> MpInitLibWhoAmI (
>>> OUT UINTN                    *ProcessorNumber
>>> )
>>> {
>>> CPU_MP_DATA           *CpuMpData;
>>> 
>>> if (ProcessorNumber == NULL) {
>>>   return EFI_INVALID_PARAMETER;
>>> }
>>> 
>>> CpuMpData = GetCpuMpData ();
>>> 
>>> return GetProcessorNumber (CpuMpData, ProcessorNumber); }
>>> 
>>> 
>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>>> tLib/PeiMpLib.c#L34
>>> 
>>> /**
>>> Get pointer to CPU MP Data structure.
>>> @return  The pointer to CPU MP Data structure.
>>> **/
>>> CPU_MP_DATA *
>>> GetCpuMpData (
>>> VOID
>>> )
>>> {
>>> CPU_MP_DATA      *CpuMpData;
>>> 
>>> CpuMpData = GetCpuMpDataFromGuidedHob ();
>>> ASSERT (CpuMpData != NULL);
>>> return CpuMpData;
>>> }
>>> 
>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni
>>> tLib/MpLib.c#L2302
>>> 
>>> 
>>> /**
>>> Get pointer to CPU MP Data structure from GUIDed HOB.
>>> @return  The pointer to CPU MP Data structure.
>>> **/
>>> CPU_MP_DATA *
>>> GetCpuMpDataFromGuidedHob (
>>> VOID
>>> )
>>> {
>>> EFI_HOB_GUID_TYPE       *GuidHob;
>>> VOID                    *DataInHob;
>>> CPU_MP_DATA             *CpuMpData;
>>> 
>>> CpuMpData = NULL;
>>> GuidHob = GetFirstGuidHob (&mCpuInitMpLibHobGuid);
>>> if (GuidHob != NULL) {
>>>   DataInHob = GET_GUID_HOB_DATA (GuidHob);
>>>   CpuMpData = (CPU_MP_DATA *) (*(UINTN *) DataInHob);
>>> }
>>> return CpuMpData;
>>> }
>>> 
>>> Thanks,
>>> 
>>> Andrew Fish
>>> _______________________________________________
>>> 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
> 
> _______________________________________________
> 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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-06-22 17:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-20 17:23 Is the PEI Core MP Safe? UefiCpuPkg seems to think so calling GetFirstGuidHob on the APs? Andrew Fish
2018-06-21  3:06 ` Ni, Ruiyu
2018-06-21  3:24   ` Andrew Fish
2018-06-21  3:34     ` Ni, Ruiyu
2018-06-21  5:57       ` Dong, Eric
2018-06-22  6:52     ` 答复: " Fan Jeff
2018-06-22 17:32       ` Andrew Fish

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox