Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-03-06 Thread Laszlo Ersek 
On 03/06/18 01:45, Brian J. Johnson wrote:
> On 03/05/2018 12:22 PM, Laszlo Ersek wrote:
>> PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
>> RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
>> like RAM) for stack & heap; whatever HOBs they produce are stored in
>> "temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
>> (basically it programs the memory controller and publishes the RAM
>> ranges). In turn the PEI core "migrates" PEIMs from temporary to
>> permanent RAM, including the HOB list.
>>
>> Before the temporary RAM migration (when still executing in place from
>> flash), PEIMs cannot have writeable global variables. For example,
>> dynamic PCDs are also maintained in a HOB (the PCD HOB).
>>
>> A PEIM normally cannot (and shouldn't) tell whether it is dispatched
>> before or after permanent RAM is published. If needed, a PEIM can
>> advertise that it depends on permanent RAM (for example because it needs
>> a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
>> its DEPEX.
>>
>> Finally, it seems like a PEIM can also express, "I'm fine with being
>> dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
>> tell me whichever it is". Apparently, if the PEIM is dispatched from
>> flash (before permanent RAM is available), its call to
>> RegisterForShadow() returns EFI_SUCCESS (and then its entry point
>> function will be invoked for a 2nd time, after the temp RAM migration).
>> And when a PEIM is dispatched from RAM (either for the very first time,
>> or for the second time, after being dispatched from flash first), the
>> same call returns EFI_ALREADY_STARTED.
>>
>> Honestly, I'm unsure what this is good for (both in general, and
>> specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
>> doing the measurements (which makes sense); I just wonder what exactly
>> it does so that it cannot simply specify
>> "gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.
> 
> I haven't looked at this particular PEIM.  But one case where
> registering for shadowing is useful is for improving performance when
> running from permanent RAM, where writable global variables are
> available.  For instance, when running from flash, a ReportStatusCode
> PEIM may need to go through a slow process to locate an output hardware
> device on every PPI call.  This may involve traversing the HOB list,
> consulting other PPIs, even probing hardware addresses.  But once it's
> shadowed to RAM, it can locate the device once, then cache its address
> in a global.  Not to mention that the code itself is far, far faster
> when run from RAM vs. flash.  (That's probably a key difference between
> a real machine and a VM.)

Ah, this sounds like a great example. Status code reporting is useful /
important for debugging even before permanent RAM is installed, so the
PEIM providing that PPI should not have a depex on
gEfiPeiMemoryDiscoveredPpiGuid. However, once the permanent RAM is
published, it makes sense to speed up the implementation. Thanks for
this example!

> Also, I've personally written a PEIM which saves a bunch of internal
> state in a HOB, since that's the main writable storage when running from
> flash.  That state includes pointers to other data (in flash.)  Once the
> data is all shadowed to RAM, it updates the HOB to point to the data in
> RAM.  That way it's a lot faster to access the data.

Another good example. (I think I've been generally blind to this perf
difference between flash and RAM, specifically concerning execution -- I
must have been spoiled by virt, as you say :) )

> I also have other PEIMs which are constrained (via DEPEX) to run *only*
> from RAM, since they have larger memory requirements than can be
> satisfied from temporary cache-as-RAM.  That's certainly a valid
> technique as well.

Right, that's quite known to OVMF too, due to CpuMpPei being "hungry"
like this.

> RegisterForShadow() is a useful tool for making the most of the
> restricted PEI environment.  And having it standardized like this is, as
> Andrew said, a lot better than the hacks people had to use beforehand.

I agree. I'm happy to have learned about this service!

Thanks all,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-03-05 Thread Gao, Liming 
Laszlo:
  I also suggest to check the generated ProcessLibraryConstructorList () 
function. It is in the driver build output AutoGen.c code. You can check what 
library function be called in this function. Then, further add debug message in 
the library function. I suspect some function does the wrong operation and 
corrupt the memory. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Tuesday, March 6, 2018 2:23 AM
> To: Marc-André Lureau <marcandre.lur...@gmail.com>; Andrew Fish 
> <af...@apple.com>
> Cc: edk2-devel@lists.01.org; Peter Jones <pjo...@redhat.com>; Yao, Jiewen 
> <jiewen@intel.com>; QEMU
> <qemu-de...@nongnu.org>; Javier Martinez Canillas <javi...@redhat.com>
> Subject: Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop
> 
> On 03/05/18 15:05, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish <af...@apple.com> wrote:
> >>
> >>
> >>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
> >>>
> >>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >>>
> >>> Without this hack, GetNextHob() loops infinitely with the next
> >>> patch. I don't understand the reason.
> >>>
> >>> The loop is triggered by the GetFirstGuidHob ()
> >>> call.
> >>>
> >>> CC: Laszlo Ersek <ler...@redhat.com>
> >>> CC: Stefan Berger <stef...@linux.vnet.ibm.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> >>> ---
> >>> MdePkg/Library/PeiHobLib/HobLib.c | 4 
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
> >>> b/MdePkg/Library/PeiHobLib/HobLib.c
> >>> index 5c0eeb992f..ed3c5fbd6d 100644
> >>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> >>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> >>> @@ -89,6 +89,10 @@ GetNextHob (
> >>> if (Hob.Header->HobType == Type) {
> >>>   return Hob.Raw;
> >>> }
> >>> +if (GET_HOB_LENGTH (HobStart) == 0) {
> >>
> >> As Laszlo points out this error condition is likely memory
> >> corruption. Thus it would be better to check for all know illegal
> >> values?
> >>
> >> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
> >>
> >
> > Thanks, I have adjusted the check.
> >
> > With manual calls and printf (I don't know  a better way to debug ovmf
> > ;),
> 
> Well that's how I generally debug it too :)
> 
> > I try to locate the issue. It's somehow related to
> > RegisterForShadow(). The "corruption" seems to happen during the
> > second call. After the
> > PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
> > calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
> > function, it fails (with the same arguments). Right after it succeeds
> > again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
> > suppose there is some kind of wrapping code, but I fail to find where.
> > Any idea?
> 
> This sounds helpful. I don't know what the problem is, but I can
> elaborate on your details a bit; perhaps someone else will have more
> ideas.
> 
> Apparently there is a PEI service called RegisterForShadow().
> ("Apparently", because I've never seen, let alone written, a PEIM
> calling this service.) The service is specified in the PI spec, which is
> quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:
> 
> > /**
> >   Register a PEIM so that it will be shadowed and called again.
> >
> >   This service registers a file handle so that after memory is
> >   available, the PEIM will be re-loaded into permanent memory and
> >   re-initialized. The PEIM registered this way will always be
> >   initialized twice. The first time, this function call will
> >   return EFI_SUCCESS. The second time, this function call will
> >   return EFI_ALREADY_STARTED. Depending on the order in which
> >   PEIMs are dispatched, the PEIM making this call may be
> >   initialized after permanent memory is installed, even the first
> >   time.
> >
> >   @param  FileHandlePEIM's file handle. Must be the currently
> > executing PEIM.
> >
> >   @retval EFI_SUC

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-03-05 Thread Brian J. Johnson 

On 03/05/2018 12:22 PM, Laszlo Ersek wrote:

PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
like RAM) for stack & heap; whatever HOBs they produce are stored in
"temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
(basically it programs the memory controller and publishes the RAM
ranges). In turn the PEI core "migrates" PEIMs from temporary to
permanent RAM, including the HOB list.

Before the temporary RAM migration (when still executing in place from
flash), PEIMs cannot have writeable global variables. For example,
dynamic PCDs are also maintained in a HOB (the PCD HOB).

A PEIM normally cannot (and shouldn't) tell whether it is dispatched
before or after permanent RAM is published. If needed, a PEIM can
advertise that it depends on permanent RAM (for example because it needs
a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
its DEPEX.

Finally, it seems like a PEIM can also express, "I'm fine with being
dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
tell me whichever it is". Apparently, if the PEIM is dispatched from
flash (before permanent RAM is available), its call to
RegisterForShadow() returns EFI_SUCCESS (and then its entry point
function will be invoked for a 2nd time, after the temp RAM migration).
And when a PEIM is dispatched from RAM (either for the very first time,
or for the second time, after being dispatched from flash first), the
same call returns EFI_ALREADY_STARTED.

Honestly, I'm unsure what this is good for (both in general, and
specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
doing the measurements (which makes sense); I just wonder what exactly
it does so that it cannot simply specify
"gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.


I haven't looked at this particular PEIM.  But one case where 
registering for shadowing is useful is for improving performance when 
running from permanent RAM, where writable global variables are 
available.  For instance, when running from flash, a ReportStatusCode 
PEIM may need to go through a slow process to locate an output hardware 
device on every PPI call.  This may involve traversing the HOB list, 
consulting other PPIs, even probing hardware addresses.  But once it's 
shadowed to RAM, it can locate the device once, then cache its address 
in a global.  Not to mention that the code itself is far, far faster 
when run from RAM vs. flash.  (That's probably a key difference between 
a real machine and a VM.)


Also, I've personally written a PEIM which saves a bunch of internal 
state in a HOB, since that's the main writable storage when running from 
flash.  That state includes pointers to other data (in flash.)  Once the 
data is all shadowed to RAM, it updates the HOB to point to the data in 
RAM.  That way it's a lot faster to access the data.


I also have other PEIMs which are constrained (via DEPEX) to run *only* 
from RAM, since they have larger memory requirements than can be 
satisfied from temporary cache-as-RAM.  That's certainly a valid 
technique as well.


RegisterForShadow() is a useful tool for making the most of the 
restricted PEI environment.  And having it standardized like this is, as 
Andrew said, a lot better than the hacks people had to use beforehand.


Thanks,
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

brian.john...@hpe.com

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

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-03-05 Thread Andrew Fish 


> On Mar 5, 2018, at 10:22 AM, Laszlo Ersek  wrote:
> 
> On 03/05/18 15:05, Marc-André Lureau wrote:
>> Hi
>> 
>> On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
>>> 
>>> 
 On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
 
 From: Marc-André Lureau 
 
 Without this hack, GetNextHob() loops infinitely with the next
 patch. I don't understand the reason.
 
 The loop is triggered by the GetFirstGuidHob ()
 call.
 
 CC: Laszlo Ersek 
 CC: Stefan Berger 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Marc-André Lureau 
 ---
 MdePkg/Library/PeiHobLib/HobLib.c | 4 
 1 file changed, 4 insertions(+)
 
 diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
 b/MdePkg/Library/PeiHobLib/HobLib.c
 index 5c0eeb992f..ed3c5fbd6d 100644
 --- a/MdePkg/Library/PeiHobLib/HobLib.c
 +++ b/MdePkg/Library/PeiHobLib/HobLib.c
 @@ -89,6 +89,10 @@ GetNextHob (
if (Hob.Header->HobType == Type) {
  return Hob.Raw;
}
 +if (GET_HOB_LENGTH (HobStart) == 0) {
>>> 
>>> As Laszlo points out this error condition is likely memory
>>> corruption. Thus it would be better to check for all know illegal
>>> values?
>>> 
>>> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>>> 
>> 
>> Thanks, I have adjusted the check.
>> 
>> With manual calls and printf (I don't know  a better way to debug ovmf
>> ;),
> 
> Well that's how I generally debug it too :)
> 
>> I try to locate the issue. It's somehow related to
>> RegisterForShadow(). The "corruption" seems to happen during the
>> second call. After the
>> PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
>> calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
>> function, it fails (with the same arguments). Right after it succeeds
>> again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
>> suppose there is some kind of wrapping code, but I fail to find where.
>> Any idea?
> 
> This sounds helpful. I don't know what the problem is, but I can
> elaborate on your details a bit; perhaps someone else will have more
> ideas.
> 
> Apparently there is a PEI service called RegisterForShadow().
> ("Apparently", because I've never seen, let alone written, a PEIM
> calling this service.) The service is specified in the PI spec, which is
> quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:
> 

In the "olden days" the PEI Core did not shadow PEIMs. There were a few PEIMs 
that had some hacky code to shadow themselves into memory. We thought of making 
it a library, but there was not a clean way to detect if the PEIM was shadowed. 
So we ended up adding the PEI Service. You could also use RegisterForShadow 
with a DEPEX of gEfiPeiMemoryDiscoveredPpiGuid to cause your PEIM to get 
shadowed even if the PEI Core did not support automatically shadowing PEIMs 
after memory showed up. 

Anyway the RegisterForShadow can cause the entry point for the PEIM to get 
called again, and if there is a bug handling that I imagine bad things can 
happen. 

There are plenty examples of RegisterForShadow usage in the edk2. Maybe looking 
at how other PEIMs are using it would be helpful. 

(master)>git grep ".RegisterForShadow"
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c:852:Status = 
(**PeiServices).RegisterForShadow(FileHandle);
SecurityPkg/Tcg/TcgPei/TcgPei.c:776:Status = 
(**PeiServices).RegisterForShadow(FileHandle);
SecurityPkg/Tcg/TrEEPei/TrEEPei.c:616:Status = 
(**PeiServices).RegisterForShadow(FileHandle);
(master)>git grep "PeiServicesRegisterForShadow"
EmulatorPkg/Library/SecPeiServicesLib/PeiServicesLib.c:423:PeiServicesRegisterForShadow
 (
FatPkg/FatPei/FatLiteApi.c:258:  Status = PeiServicesRegisterForShadow 
(FileHandle);
IntelFrameworkModulePkg/Bus/Isa/IsaFloppyPei/FloppyPeim.c:1725:  Status = 
PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.c:1199:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c:44:  Status = 
PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Bus/Pci/SdMmcPciHcPei/SdMmcPciHcPei.c:103:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/UfsPciHcPei/UfsPciHcPei.c:92:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.c:121:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.c:1462:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcBlockIoPei.c:693:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Sd/SdBlockIoPei/SdBlockIoPei.c:540:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-03-05 Thread Laszlo Ersek 
On 03/05/18 15:05, Marc-André Lureau wrote:
> Hi
>
> On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
>>
>>
>>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
>>>
>>> From: Marc-André Lureau 
>>>
>>> Without this hack, GetNextHob() loops infinitely with the next
>>> patch. I don't understand the reason.
>>>
>>> The loop is triggered by the GetFirstGuidHob ()
>>> call.
>>>
>>> CC: Laszlo Ersek 
>>> CC: Stefan Berger 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marc-André Lureau 
>>> ---
>>> MdePkg/Library/PeiHobLib/HobLib.c | 4 
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
>>> b/MdePkg/Library/PeiHobLib/HobLib.c
>>> index 5c0eeb992f..ed3c5fbd6d 100644
>>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>>> @@ -89,6 +89,10 @@ GetNextHob (
>>> if (Hob.Header->HobType == Type) {
>>>   return Hob.Raw;
>>> }
>>> +if (GET_HOB_LENGTH (HobStart) == 0) {
>>
>> As Laszlo points out this error condition is likely memory
>> corruption. Thus it would be better to check for all know illegal
>> values?
>>
>> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>>
>
> Thanks, I have adjusted the check.
>
> With manual calls and printf (I don't know  a better way to debug ovmf
> ;),

Well that's how I generally debug it too :)

> I try to locate the issue. It's somehow related to
> RegisterForShadow(). The "corruption" seems to happen during the
> second call. After the
> PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
> calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
> function, it fails (with the same arguments). Right after it succeeds
> again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
> suppose there is some kind of wrapping code, but I fail to find where.
> Any idea?

This sounds helpful. I don't know what the problem is, but I can
elaborate on your details a bit; perhaps someone else will have more
ideas.

Apparently there is a PEI service called RegisterForShadow().
("Apparently", because I've never seen, let alone written, a PEIM
calling this service.) The service is specified in the PI spec, which is
quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:

> /**
>   Register a PEIM so that it will be shadowed and called again.
>
>   This service registers a file handle so that after memory is
>   available, the PEIM will be re-loaded into permanent memory and
>   re-initialized. The PEIM registered this way will always be
>   initialized twice. The first time, this function call will
>   return EFI_SUCCESS. The second time, this function call will
>   return EFI_ALREADY_STARTED. Depending on the order in which
>   PEIMs are dispatched, the PEIM making this call may be
>   initialized after permanent memory is installed, even the first
>   time.
>
>   @param  FileHandlePEIM's file handle. Must be the currently
> executing PEIM.
>
>   @retval EFI_SUCCESS   The PEIM was successfully registered for
> shadowing.
>   @retval EFI_ALREADY_STARTED   The PEIM was previously
> registered for shadowing.
>   @retval EFI_NOT_FOUND The FileHandle does not refer to a
> valid file handle.
>
> **/
> typedef
> EFI_STATUS
> (EFIAPI *EFI_PEI_REGISTER_FOR_SHADOW)(
>   IN  EFI_PEI_FILE_HANDLE FileHandle
>   );

PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
like RAM) for stack & heap; whatever HOBs they produce are stored in
"temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
(basically it programs the memory controller and publishes the RAM
ranges). In turn the PEI core "migrates" PEIMs from temporary to
permanent RAM, including the HOB list.

Before the temporary RAM migration (when still executing in place from
flash), PEIMs cannot have writeable global variables. For example,
dynamic PCDs are also maintained in a HOB (the PCD HOB).

A PEIM normally cannot (and shouldn't) tell whether it is dispatched
before or after permanent RAM is published. If needed, a PEIM can
advertise that it depends on permanent RAM (for example because it needs
a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
its DEPEX.

Finally, it seems like a PEIM can also express, "I'm fine with being
dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
tell me whichever it is". Apparently, if the PEIM is dispatched from
flash (before permanent RAM is available), its call to
RegisterForShadow() returns EFI_SUCCESS (and then its entry point
function will be invoked for a 2nd time, after the temp RAM migration).
And when a PEIM

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-03-05 Thread Marc-André Lureau 
Hi

On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
>
>
>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
>>
>> From: Marc-André Lureau 
>>
>> Without this hack, GetNextHob() loops infinitely with the next patch.
>> I don't understand the reason.
>>
>> The loop is triggered by the GetFirstGuidHob () call.
>>
>> CC: Laszlo Ersek 
>> CC: Stefan Berger 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau 
>> ---
>> MdePkg/Library/PeiHobLib/HobLib.c | 4 
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
>> b/MdePkg/Library/PeiHobLib/HobLib.c
>> index 5c0eeb992f..ed3c5fbd6d 100644
>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>> @@ -89,6 +89,10 @@ GetNextHob (
>> if (Hob.Header->HobType == Type) {
>>   return Hob.Raw;
>> }
>> +if (GET_HOB_LENGTH (HobStart) == 0) {
>
> As Laszlo points out this error condition is likely memory corruption. Thus 
> it would be better to check for all know illegal values?
>
> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>

Thanks, I have adjusted the check.

With manual calls and printf (I don't know  a better way to debug ovmf
;), I try to locate the issue. It's somehow related to
RegisterForShadow(). The "corruption" seems to happen during the
second call. After the
PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
function, it fails (with the same arguments). Right after it succeeds
again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
suppose there is some kind of wrapping code, but I fail to find where.
Any idea?

thanks for your help

-- 
Marc-André Lureau
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-02-23 Thread Andrew Fish 


> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
> 
> From: Marc-André Lureau 
> 
> Without this hack, GetNextHob() loops infinitely with the next patch.
> I don't understand the reason.
> 
> The loop is triggered by the GetFirstGuidHob () call.
> 
> CC: Laszlo Ersek 
> CC: Stefan Berger 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau 
> ---
> MdePkg/Library/PeiHobLib/HobLib.c | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
> b/MdePkg/Library/PeiHobLib/HobLib.c
> index 5c0eeb992f..ed3c5fbd6d 100644
> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> @@ -89,6 +89,10 @@ GetNextHob (
> if (Hob.Header->HobType == Type) {
>   return Hob.Raw;
> }
> +if (GET_HOB_LENGTH (HobStart) == 0) {

As Laszlo points out this error condition is likely memory corruption. Thus it 
would be better to check for all know illegal values? 

if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)

Thanks,

Andrew Fish

> +DEBUG ((DEBUG_INFO, "FIXME: GetNextHob length == 0"));
> +return NULL;
> +}
> Hob.Raw = GET_NEXT_HOB (Hob);
>   }
>   return NULL;
> -- 
> 2.16.1.73.g5832b7e9f2
> 
> ___
> 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

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

2018-02-23 Thread Laszlo Ersek 
On 02/23/18 14:23, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> Without this hack, GetNextHob() loops infinitely with the next patch.
> I don't understand the reason.
> 
> The loop is triggered by the GetFirstGuidHob () call.
> 
> CC: Laszlo Ersek 
> CC: Stefan Berger 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau 
> ---
>  MdePkg/Library/PeiHobLib/HobLib.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
> b/MdePkg/Library/PeiHobLib/HobLib.c
> index 5c0eeb992f..ed3c5fbd6d 100644
> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> @@ -89,6 +89,10 @@ GetNextHob (
>  if (Hob.Header->HobType == Type) {
>return Hob.Raw;
>  }
> +if (GET_HOB_LENGTH (HobStart) == 0) {
> +DEBUG ((DEBUG_INFO, "FIXME: GetNextHob length == 0"));
> +return NULL;
> +}
>  Hob.Raw = GET_NEXT_HOB (Hob);
>}
>return NULL;
> 

Strange. The HobLength field is supposed to include the size of the HOB header, 
so it should never be zero.

Furthermore, the PEI core initializes the HOB list; it should be terminated 
with an End-of-HOB-List HOB:

PeiCore() [MdeModulePkg/Core/Pei/PeiMain/PeiMain.c]
  InitializeMemoryServices()  
[MdeModulePkg/Core/Pei/Memory/MemoryServices.c]
PeiCoreBuildHobHandoffInfoTable() [MdeModulePkg/Core/Pei/Hob/Hob.c]

I tried to reproduce this issue by:
- applying patches 1, 2, and 4
- in function PeimEntryMA(), file "SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c", moving 
the GetFirstGuidHob () call to the top of the function.

It didn't hang for me.

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