Re: [edk2] [PATCH] PcAtChipsetPkg/AcpiTimerLib: Support Standalone MM.

[Marvin H?user]

Best regards,
Marvin.

> -Original Message-
> From: Gao, Liming 
> Sent: Monday, July 23, 2018 8:46 AM
> To: marvin.haeu...@outlook.com; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu 
> Subject: RE: [PATCH] PcAtChipsetPkg/AcpiTimerLib: Support Standalone MM.
> 
> Marvin:
>   If this library supports standalone MM module only, I agree its source file
> includes Standalone MM
>   But, this library instance also supports DXE, SMM. I don't think
> StandaloneMm name is good for them.

I called all files "DxeStandaloneMm", the same way as other libraries append 
environment types ("PeiDxePostCodeLib", etc).
Did I overlook something?

> 
>   Besides, I want to know why you changes this library instance. Are there the
> standalone MM module to depend on this AcpiTimerLib?

I wanted to try out porting a private module which happened to depend on 
AcpiTimerLib and thus HobLib.
HobLib will follow once the MmServicesTableLib patch has been decided on and I 
will adapt others too once I run into needing them.

> 
> Thanks
> Liming
> >-Original Message-----
> >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >Marvin H?user
> >Sent: Monday, July 23, 2018 7:05 AM
> >To: edk2-devel@lists.01.org
> >Cc: Ni, Ruiyu 
> >Subject: [edk2] [PATCH] PcAtChipsetPkg/AcpiTimerLib: Support Standalone
> >MM.
> >
> >To support Standalone MM, the current DXE implementation, which is also
> >used to support DXE SMM Drivers, has been modified. Its type was
> >changed to BASE to make the constructor function generic,
> MM_STANDALONE
> >modules types have been added to the support list and the internal
> >files were adapted to show support.
> >
> >"DxeAcpiTimerLib.inf" has not been renamed to not break packages.
> >This might be addressed with a separate patch.
> >
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Marvin Haeuser 
> >---
> > PcAtChipsetPkg/Library/AcpiTimerLib/{DxeAcpiTimerLib.c =>
> >DxeStandaloneMmAcpiTimerLib.c} |  9 +++--
> > PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> >| 14 +++---
> > PcAtChipsetPkg/Library/AcpiTimerLib/{DxeAcpiTimerLib.uni =>
> >StandaloneMmDxeAcpiTimerLib.uni} |  2 +-
> > 3 files changed, 11 insertions(+), 14 deletions(-)
> >
> >diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> >b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> >similarity index 88%
> >rename from PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> >rename to
> >PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> >index 9ed10ef2e297..784f33871d75 100644
> >--- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
> >+++
> >b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> >@@ -13,6 +13,7 @@
> > **/
> >
> > #include 
> >+#include 
> > #include 
> > #include 
> > #include 
> >@@ -78,17 +79,13 @@ InternalGetPerformanceCounterFrequency (
> > /**
> >   The constructor function enables ACPI IO space, and caches
> >PerformanceCounterFrequency.
> >
> >-  @param  ImageHandle   The firmware allocated handle for the EFI image.
> >-  @param  SystemTable   A pointer to the EFI System Table.
> >-
> >   @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
> >
> > **/
> > EFI_STATUS
> > EFIAPI
> >-DxeAcpiTimerLibConstructor (
> >-  IN EFI_HANDLEImageHandle,
> >-  IN EFI_SYSTEM_TABLE  *SystemTable
> >+DxeStandaloneMmAcpiTimerLibConstructor (
> >+  VOID
> >   )
> > {
> >   EFI_HOB_GUID_TYPE   *GuidHob;
> >diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> >b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> >index 601041c80137..f1f62247649e 100644
> >--- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> >+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> >@@ -1,5 +1,5 @@
> > ## @file
> >-#  DXE ACPI Timer Library
> >+#  DXE and Standalone MM ACPI Timer Library
> > #
> > #  Provides basic timer support using the ACPI timer hardware.  The
> >performance  #  counter features are provided by the processors time
> >stamp counter.
> >@@ -20,17 +20,17 @@
> >
> > [Defines]
> >   INF_VERSION= 0x00010005
> >-  BASE_NAME  = DxeAcpiTimerLib
> >+  BASE_NAME  = DxeStandaloneMmAcpiTimerLib
> >   FILE_GUID  = E624B98C-845A-4b94-9B

[edk2] StandaloneMmPkg comments

[Marvin H?user]

Good day,

I have been reading through the recently imported StandaloneMmPkg and found 
three odd things.


  1.  GUID prefixes: GUIDs defined in StandaloneMmPkg.dec either have no common 
prefix at all ("gMmFv") or use the "gEfi" prefix. Maybe the MdeModulePkg-style 
"gEdkii" prefix could be used for a uniform style?
  2.  The "gEfiMmConfigurationProtocolGuid" name is common between Standalone 
(StandaloneMmPkg.dec) and Traditional (MdePkg.dec) MM context despite having a 
different value of course. Shouldn't the naming reflect which is traditional 
and which is Standalone? I haven't checked in depth, but which is chosen when a 
module consumes both MdePkg and StandaloneMmPkg?
  3.  While .dec already uses the "Mmram" naming scheme, its header declares 
the MemoryReserve GUID as "gEfiMmPeiS(!)mramMemoryReserveGuid". Furthermore, 
the header references the SMM CIS (which has been replaced with "MM CIS" as 
part of the renaming), while the GUID has changed and the structure does not 
match the deprecated specification anyway. May I suggest to turn this GUID into 
a "gEdkii"-style GUID and remove all references to the SMM CIS? Maybe use the 
"EDKII_" prefix for "EFI_MMRAM_HOB_DESCRIPTOR_BLOCK" too? I wanted to prepare a 
patch, but I cannot compile the package at the moment and don't want to risk 
submitting anything broken.

Thanks for your time!

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

Re: [edk2] Help on AutoGen Files

[Marvin H?user]

Hey Udit,

You cannot explicitly influence the order of the calls, but implicitly via the 
dependency tree, which means you need to make SerialPortLib depend on your 
LibraryClass instance.
You did not mention which SerialPortLib instance you use, but probably you need 
to execute FpgaInterfaceInit() earlier in platform code or fork SerialPortLib 
for now.

Regards,
Marvin

> -Original Message-
> From: edk2-devel  On Behalf Of Udit
> Kumar
> Sent: Thursday, July 19, 2018 9:33 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Help on AutoGen Files
> 
> Hi Experts,
> How I can change the order of initialization in Constructor list of autogen 
> file.
> In my build system, if I look at
> MdeModulePkg/Universal/PCD/Pei/Pcd/DEBUG/AutoGen.c
> Below is function of Library Constructor List
> 
> VOID
> EFIAPI
> ProcessLibraryConstructorList (
>   IN   EFI_PEI_FILE_HANDLE   FileHandle,
>   IN CONST EFI_PEI_SERVICES  **PeiServices
>   )
> {
>   EFI_STATUS  Status;
> 
>   Status = BaseDebugLibSerialPortConstructor ();
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = PeiServicesTablePointerLibConstructor (FileHandle, PeiServices);
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = TimerConstructor ();
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = FpgaInterfaceInit ();
>   ASSERT_EFI_ERROR (Status);
> 
> }
> 
> 
> My problem is SerialPortConstructor needs frequency, which can be
> retrieved after  FpgaInterfaceInit() Therefore, my preferred way for this
> constructor list will be
> FpgaInterfaceInit() followed by  BaseDebugLibSerialPortConstructor()
> 
> how I can achieve this.
> 
> 
> Many Thanks
> Udit
> ___
> 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] Inquiry regarding early DxeIplPeim loading.

[Marvin H?user]

Hey Eugene,

I do not think this is a hack, I don't think it uses DxeIplPeim for "actual 
booting", but more for the compression PPIs it exposes.
FVs might be compressed for performance reason and be loaded into memory (DRAM 
or cache).
Though to be honest, I am not sure why these PPIs are exposed by DxeIplPeim 
instead of a dedicated module or maybe even PeiCore itself, if it's the de 
facto standard.

Also, if the change was done to avoid disabling CAR prematurely, why wasn't SEC 
code adapted?
UefiCpuPkg SecCore calls the PlatformSecLib function to disable CAR when 
permanent memory has been installed.
>From what I can see, the described change only makes sense if InstallMemory is 
>not called during the entire PEI phase. Am I missing something?

Thanks,
Marvin

> -Original Message-
> From: Cohen, Eugene 
> Sent: Wednesday, July 18, 2018 11:37 PM
> To: Marvin H?user ; edk2-
> de...@lists.01.org
> Cc: eric.d...@intel.com; Zeng, Star ; Cohen, Eugene
> 
> Subject: RE: Inquiry regarding early DxeIplPeim loading.
> 
> The depex change did not pertain to S3 resume without memory initialization
> (obviously that wouldn't work).  The change was whether "permanent
> memory" was installed from a PI architecture perspective.  We had a
> situation that required that the S3 Resume path remain in PEI Cache-as-RAM
> (actual code in CAR, not just data) and this change was necessary to prevent
> CAR from being turned off prematurely on S3 resume.
> 
> By the way it was never our goal for DXE IPL to host the S3 resume path - my
> understanding is that this was a matter of convenience back in ancient
> history of S3 development (according to an old thread I dug up from a few
> years ago).  If S3 resume were parked somewhere else such that DXE IPL
> only did the IPL of DXE then this issue might go away.  So you're seeing a
> secondary effect of hosting S3 resume in DXE IPL and DXE IPL depex changes
> to support an unusual use case.
> 
> 
> > -Original Message-
> > From: edk2-devel  On Behalf Of Marvin
> > H?user
> > Sent: Friday, July 13, 2018 7:26 AM
> > To: edk2-devel@lists.01.org
> > Cc: eric.d...@intel.com; Zeng, Star 
> > Subject: Re: [edk2] Inquiry regarding early DxeIplPeim loading.
> >
> > Hey Star,
> >
> > Thank you very much for your reply.
> > Interesting, that is basically the case I described as "insane"
> > because I did not consider any platform to allow S3 resume without
> > memory initialization. So, this code definitely makes sense.
> > You are right, according to the specification, moving it to the
> > PostMem FV should be fine. However that will cost a shadow call and a
> > re-entry for non-
> > S3 and an event registration for the S3 boot path.
> > As the information whether S3 resume without meminit is intended is
> > known at compile-time, what's your opinion on a FeatureFlag PCD which
> > chooses between direct calls and the shadow/event system?
> > I would prepare a patch as soon as I can properly test its working, if
> > you are interested. The changes would be most minimal, I imagine.
> >
> > Thanks,
> > Marvin.
> >
> > > -Original Message-
> > > From: Zeng, Star 
> > > Sent: Friday, July 13, 2018 11:24 AM
> > > To: Marvin H?user ; edk2-
> > > de...@lists.01.org
> > > Cc: Dong, Eric ; Cohen, Eugene
> > ;
> > > Gao, Liming ; Zeng, Star 
> > > Subject: RE: Inquiry regarding early DxeIplPeim loading.
> > >
> > > Marvin,
> > >
> > > You can check SHA-1: ebaafbe62c70309d0ceb44a0c4199093d0a823c4.
> > > It is for the case "Allow S3 Resume without having installed
> > > permanent memory (via InstallPeiMemory)" (PI Mantis 1532, you can
> > > search the sentence in PI spec) requested by HP.
> > > Yes before ebaafbe62c70309d0ceb44a0c4199093d0a823c4, DxeIpl.inf had
> > > gEfiPeiMemoryDiscoveredPpiGuid DEPEX.
> > > For the case you mentioned about MinPlatformPkg, I think you can put
> > > the DxeIpl.inf into a Post Memory FV if the platform will publish
> > > gEfiPeiMemoryDiscoveredPpiGuid indeed.
> > >
> > >
> > > Thanks,
> > > Star
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > Of
> > > Marvin H?user
> > > Sent: Friday, July 13, 2018 7:19 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Dong, Eric ; Zeng, Star
> > > 
> > > Subject: [edk2] Inquiry regarding early DxeIplPeim loading.
> > >
> > > Good day developers,
> >

Re: [edk2] Inquiry regarding early DxeIplPeim loading.

[Marvin H?user]

Hey Star,

Thank you very much for your reply.
Interesting, that is basically the case I described as "insane" because I did 
not consider any platform to allow S3 resume without memory initialization. So, 
this code definitely makes sense.
You are right, according to the specification, moving it to the PostMem FV 
should be fine. However that will cost a shadow call and a re-entry for non-S3 
and an event registration for the S3 boot path.
As the information whether S3 resume without meminit is intended is known at 
compile-time, what's your opinion on a FeatureFlag PCD which chooses between 
direct calls and the shadow/event system?
I would prepare a patch as soon as I can properly test its working, if you are 
interested. The changes would be most minimal, I imagine.

Thanks,
Marvin.

> -Original Message-
> From: Zeng, Star 
> Sent: Friday, July 13, 2018 11:24 AM
> To: Marvin H?user ; edk2-
> de...@lists.01.org
> Cc: Dong, Eric ; Cohen, Eugene ;
> Gao, Liming ; Zeng, Star 
> Subject: RE: Inquiry regarding early DxeIplPeim loading.
> 
> Marvin,
> 
> You can check SHA-1: ebaafbe62c70309d0ceb44a0c4199093d0a823c4.
> It is for the case "Allow S3 Resume without having installed permanent
> memory (via InstallPeiMemory)" (PI Mantis 1532, you can search the
> sentence in PI spec) requested by HP.
> Yes before ebaafbe62c70309d0ceb44a0c4199093d0a823c4, DxeIpl.inf had
> gEfiPeiMemoryDiscoveredPpiGuid DEPEX.
> For the case you mentioned about MinPlatformPkg, I think you can put the
> DxeIpl.inf into a Post Memory FV if the platform will publish
> gEfiPeiMemoryDiscoveredPpiGuid indeed.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Friday, July 13, 2018 7:19 AM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star 
> Subject: [edk2] Inquiry regarding early DxeIplPeim loading.
> 
> Good day developers,
> 
> While checking out which edk2 modules request being shadowed, I came
> across DxeIplPeim being one of them, however I am not sure why it was
> designed this way.
> 
> If the Boot Mode is != S3, the module will register for shadowing and
> immediately return during the pre-memory phase
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe
> IplPeim/DxeLoad.c#L92
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe
> IplPeim/DxeLoad.c#L111
> 
> If the Boot Mode is S3, the module will register a Memory Discovered event
> to install crucial PPIs...
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe
> IplPeim/DxeLoad.c#L125
> ... and install the DxeIpl PPI before returning
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe
> IplPeim/DxeLoad.c#L132
> 
> However, by design, the DxeIpl PPI is not located until the very end of
> PeiCore, meaning the dispatcher ran out of modules to dispatch
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> PeiMain/PeiMain.c#L467
> Hence installing the DxeIpl PPI early in the S3 boot path does not seem to
> have any effect to me, as both paths are left awaiting memory availability
> (Shadow / event). The only functional change would be PeiCore failing to
> locate the DxeIpl PPI in case memory initialization silently fails and code
> execution continues, which is an insane state in the first place.
> 
> Am I missing any scenario where this design is helpful? Is there any
> disadvantage for adding a Depex on MemoryDiscovered PPI? Running only
> after memory initialization would shrink the initialization function by
> removing the shadowing request in non-S3 path and the event registration in
> the S3 path, as well as merging the PPI installation code as both 
> registrations
> end up executing the exact same code
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe
> IplPeim/DxeLoad.c#L118
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe
> IplPeim/DxeLoad.c#L57
> 
> The initialization function would collapse to PPI installations, a shadow or
> event registration call would be saved and platforms could safely embed
> DxeIplPeim into a Post Memory FV, such as MinPlatformPkg is using, to have
> the PEIM loaded directly into memory to gain yet more performance. The
> only restriction would be to prohibit compression.
> 
> Thanks for your time.
> 
> Best regards,
> Marvin.
> ___
> 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] Inquiry regarding early DxeIplPeim loading.

[Marvin H?user]

Good day developers,

While checking out which edk2 modules request being shadowed, I came across 
DxeIplPeim being one of them, however I am not sure why it was designed this 
way.

If the Boot Mode is != S3, the module will register for shadowing and 
immediately return during the pre-memory phase
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L92
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L111

If the Boot Mode is S3, the module will register a Memory Discovered event to 
install crucial PPIs...
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L125
... and install the DxeIpl PPI before returning
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L132

However, by design, the DxeIpl PPI is not located until the very end of 
PeiCore, meaning the dispatcher ran out of modules to dispatch
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#L467
Hence installing the DxeIpl PPI early in the S3 boot path does not seem to have 
any effect to me, as both paths are left awaiting memory availability (Shadow / 
event). The only functional change would be PeiCore failing to locate the 
DxeIpl PPI in case memory initialization silently fails and code execution 
continues, which is an insane state in the first place.

Am I missing any scenario where this design is helpful? Is there any 
disadvantage for adding a Depex on MemoryDiscovered PPI? Running only after 
memory initialization would shrink the initialization function by removing the 
shadowing request in non-S3 path and the event registration in the S3 path, as 
well as merging the PPI installation code as both registrations end up 
executing the exact same code
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L118
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L57

The initialization function would collapse to PPI installations, a shadow or 
event registration call would be saved and platforms could safely embed 
DxeIplPeim into a Post Memory FV, such as MinPlatformPkg is using, to have the 
PEIM loaded directly into memory to gain yet more performance. The only 
restriction would be to prohibit compression.

Thanks for your time.

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

Re: [edk2] [Patch 0/3] Use comparison logic to check UINTN parameter in GetBestLanguage API

[Marvin H?user]

Hey Star and Liming,

May I propose re-considering the introduction of a third named parameter to 
reflect the first Language passed?
This would 1) have the advantage that the BOOLEAN parameter can remain as is, 
which increases readability and
2) require at least two parameters related to the language list passed. Having 
to write "NULL, NULL" is way more obvious than just having to write "NULL" when 
(accidentally?) not passing any language.

And error caused by this change would be calls that do not pass an expected 
amount of parameters for the call to make sense.

Thanks,
Marvin.

> -Original Message-
> From: edk2-devel  On Behalf Of Zeng,
> Star
> Sent: Monday, May 28, 2018 9:54 AM
> To: Gao, Liming ; edk2-devel@lists.01.org
> Cc: Zeng, Star 
> Subject: Re: [edk2] [Patch 0/3] Use comparison logic to check UINTN
> parameter in GetBestLanguage API
> 
> Reviewed-by: Star Zeng 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Liming Gao
> Sent: Monday, May 28, 2018 3:31 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch 0/3] Use comparison logic to check UINTN parameter
> in GetBestLanguage API
> 
> Liming Gao (3):
>   MdePkg UefiLib: Use comparison logic to check UINTN parameter
>   IntelFrameworkPkg UefiLib: Use comparison logic to check UINTN
> parameter
>   MdeModulePkg Variable: Use comparison logic to check UINTN parameter
> 
>  IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c| 6 +++---
>  MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariable.c | 8
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c   | 8 
>  MdePkg/Library/UefiLib/UefiLib.c| 6 +++---
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> --
> 2.8.0.windows.1
> 
> ___
> 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

Re: [edk2] smm lock query

[Marvin H?user]

Good day Abhishek,

I CC'd the MdeModulePkg maintainers, Ruiyu for the Platform BDS aspect (exposes 
the ReadyToLock protocol) and Laszlo for his high-quality answers.

Strictly speaking you are, right, because of the description for the MM 
protocol:
"Indicates that MM resources and services that should not be used by the third 
party code are about[Marvin: (!)] to be locked."
Practically however, I don't see any issue with the current implementation. 
Code inside MMRAM is not affected directly by the lock, it is just notified.
However, either the code or the specification should be slightly updated to be 
in sync. A code update might require review of the caller assumptions, just to 
be sure.

I have a different concern though and hope I'm actually overlooking something.
If I understand the code correctly, it is the Platform BDS that exposes the 
(S)MmReadyToLock protocol. PiSmmIpl seems to consume that event and lock SMM 
resources based on the event.
Because of latter being an event however, I don't think it is, or can be, 
guaranteed to be the last event group member executing.
When it is not the last, the "about to be locked" part is not true for any 
subsequent callbacks, that could actually be a risky break of the specification 
- if it is.
If it is a break of the specification, I can only think of letting Platform BDS 
expose an "internal" event group, which is only caught by PiSmmIpl, which then 
drives the actual SmmReadyToLock flow.
This would require updates to all platform trees and hence I would propose a 
temporary backwards-compatibility.

Any comments? Did I overlook something (I hope)?

Thanks and regards,
Marvin

> -Original Message-
> From: edk2-devel  On Behalf Of
> Abhishek Singh
> Sent: Saturday, May 26, 2018 5:05 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] smm lock query
> 
> Hi,
> 
> This is the first time I am mailing to this list. If this is not the right 
> place for the
> kind of questions I am asking please let me know where to direct my queries.
> 
> I have been looking at the SMM IPL code and a portion of the code is a little
> confusing to me. In the function SmmIplReadyToLockEventNotify, smram is
> locked (mSmmAccess->Lock) before the ready to lock notifications are sent
> through SmmIplGuidedEventNotify. Shouldn't the lock be placed after the
> ready to lock notifications?
> 
> Best regards,
> Abhishek
> ___
> 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] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Marvin H?user]

Good day,

While I was inspecting CpuS3DataDxe and the modules depending on its PCD 
PcdCpuS3DataAddress, I noticed that DxeRegisterCpuFeaturesLib seemingly has an 
asserted dependency on the PCD being ready when it its executed. I did neither 
see a Depex entry, nor an event callback ensuring CpuS3DataDxe has been loaded, 
neither exposed by CpuS3DataDxe, nor consumed by this library.
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c#L211

Is there anything I'm missing that ensures the execution of CpuS3DataDxe prior 
to executing the dependent code? If not, should there be a dummy protocol 
exposed? PiSmmCpuDxeSmm also retrieves this PCD, however safely quits when it 
has not been set. However, this could cause unexpected behavior when the PCD is 
set after this code has been executed. I did not notice any dependency 
satisfaction actions here either.

Furthermore, not directly related to this dependency issue, the DXE code 
obviously does not implement AllocateAcpiCpuData() entirely. Hence, the 
if-branch following its call, will either add another layer of firing ASSERTs, 
or it will plainly do nothing. Maybe it could be moved into the current 
AllocateAcpiCpuData() function and it be renamed accordingly?
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c#L526

Thanks for your time.

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

Re: [edk2] [PATCH] MdePkg/Hpet: Add Event Timer Block ID definition.

[Marvin H?user]

Hey Star,

Actually the definition added is part of the definition of the ACPI Table 
header.
I think there might be confusion because the first 32 bits match the first 32 
bits of the Capabilities register.
However, they are defined separately and the ACPI field lacks the upper 32 bits.

Thanks,
Marvin.

> -Original Message-
> From: Zeng, Star <star.z...@intel.com>
> Sent: Thursday, May 17, 2018 3:31 AM
> To: marvin.haeu...@outlook.com; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> <liming@intel.com>; Yao, Jiewen <jiewen@intel.com>; Zeng, Star
> <star.z...@intel.com>
> Subject: RE: [PATCH] MdePkg/Hpet: Add Event Timer Block ID definition.
> 
> Was HighPrecisionEventTimerTable.h just created for ACPI related, but not
> for HPET register related?
> 
> We also see AlertStandardFormatTable.h, DmaRemappingReportingTable.h,
> etc. They are all ACPI related.
> What is the criteria about including ACPI related, and including
> register/command/message related?
> Should they be included in same header file, or separated header files?
> 
> We also see HEPT register related defined in
> PcAtChipsetPkg\Include\Register\Hpet.h.
> 
> 
> Thanks,
> Star
> -----Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Wednesday, May 16, 2018 3:35 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> <liming@intel.com>
> Subject: [edk2] [PATCH] MdePkg/Hpet: Add Event Timer Block ID definition.
> 
> This patch adds the HPET Event Timer Block ID definition that can be found in
> the IA-PC HPET Specification, section 3.2.4.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <marvin.haeu...@outlook.com>
> ---
>  MdePkg/Include/IndustryStandard/HighPrecisionEventTimerTable.h | 18
> +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git
> a/MdePkg/Include/IndustryStandard/HighPrecisionEventTimerTable.h
> b/MdePkg/Include/IndustryStandard/HighPrecisionEventTimerTable.h
> index 0d83cd5335de..926445233944 100644
> --- a/MdePkg/Include/IndustryStandard/HighPrecisionEventTimerTable.h
> +++ b/MdePkg/Include/IndustryStandard/HighPrecisionEventTimerTable.h
> @@ -2,7 +2,7 @@
>ACPI high precision event timer table definition, at www.intel.com
>Specification name is IA-PC HPET (High Precision Event Timers)
> Specification.
> 
> -  Copyright (c) 2007 - 2008, Intel Corporation. All rights reserved.
> +  Copyright (c) 2007 - 2018, Intel Corporation. All rights
> + reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution.  The full text of the license may be
> found at
> @@ -22,6 +22,22 @@
>  //
>  #pragma pack(1)
> 
> +///
> +/// HPET Event Timer Block ID described in IA-PC HPET Specification, 3.2.4.
> +///
> +typedef union {
> +  struct {
> +UINT32 Revision   : 8;
> +UINT32 NumberOfTimers : 5;
> +UINT32 CounterSize: 1;
> +UINT32 Reserved   : 1;
> +UINT32 LegacyRoute: 1;
> +UINT32 VendorId   : 16;
> +  }  Bits;
> +  UINT32 Uint32;
> +} EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_BLOCK_ID;
> +
> +
>  ///
>  /// High Precision Event Timer Table header definition.
>  ///
> --
> 2.17.0.windows.1
> 
> ___
> 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] Query regarding hole in EFI Memory Map

[Marvin H?user]

Hey Sai and others,

I did not verify this is actually the case for QEMU, but the mentioned range is 
usually the SMRAM ASEG.
SMRAM ranges are not reported in the Memory Map by-design.

Regards,
Marvin

> -Original Message-
> From: edk2-devel  On Behalf Of Prakhya,
> Sai Praneeth
> Sent: Tuesday, May 15, 2018 3:29 AM
> To: Bill Paul ; edk2-devel@lists.01.org
> Cc: Neri, Ricardo 
> Subject: Re: [edk2] Query regarding hole in EFI Memory Map
> 
> > Of all the gin joints in all the towns in all the world, Prakhya, Sai
> > Praneeth had to walk into mine at 16:30 on Monday 14 May 2018 and say:
> >
> > > Hi All,
> > >
> > > Recently, I have observed that there was a hole in EFI Memory Map
> > > passed by firmware to Linux kernel. So, wanted to check with you if
> > > this is expected or not.
> > >
> > > My Test setup:
> > > I usually boot qemu with OVMF and Linux kernel. I use below command
> > > to boot kernel. "qemu-system-x86_64 -cpu host -hda 
> > > -serial stdio -bios  -m 2G -enable-kvm -smp 2"
> > >
> > > I have noticed that the EFI Memory Map (printed by kernel) is almost
> > > contiguous but with only one hole ranging from 0xA to 0x10.
> > > As far as I know, kernel hasn't modified this EFI Memory Map, so I
> > > am assuming that firmware has passed memory map with a hole. I have
> > > looked at UEFI spec "GetMemoryMap()" definition, and it says "The
> > > map describes all of memory, no matter how it is being used". So, I
> > > am thinking that EFI Memory Map shouldn't have any holes, am I correct?
> > > If not, could someone please explain me the reason for this hole in
> > > EFI
> > Memory Map.
> >
> > The map may describe all of physical RAM, however it is not
> > necessarily the case that all available RAM be physically contiguous.
> >
> > With older IBM PCs based on the Intel 8088 processor, you could only
> > have a 1MB address space. The first 640KB was available for RAM. The
> > remaining space traditionally contained memory-mapped option ROMs,
> > particularly for things like the video BIOS routines. The VGA text screen 
> > was
> also mapped to 0xB8000.
> >
> > Obviously, later processors made it possible to have additional memory
> > above 1MB (sometimes called "high memory"), but for backward
> > compatibility purposes, the gap from 0xA to 0xF remained.
> >
> > So basically, on Intel machines you will always see this gap in RAM
> > due to "hysterical raisins." It's just an artifact of the platform
> > design. (And for that reason you'll see it both with the UEFI memory
> > map facility and the legacy E820 BIOS facility).
> 
> Thanks a lot! for the explanation Bill. I really appreciate it :)
> 
> Regards,
> Sai
> ___
> 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] Proposition of a BmEnumerateBootOptions() hook.

[Marvin H?user]

Hey Star, Eric and everyone else,

I have seen that some platforms add a Boot Option for the UEFI Shell in 
"PlatformBootManagerBeforeConsole()", which is called as part of the regular 
boot flow.
This is surely beneficial for development platforms that are supposed to boot 
to UEFI Shell by default when no other option has been registered, however for 
retail platforms it usually makes more sense to show the UEFI Boot Menu, which 
renders adding the Shell Boot Option as part of the regular boot flow obsolete 
and just adds up to the boot time. Meanwhile, there is a function in the 
UefiBootManagerLib, "BmEnumerateBootOptions()", which is called prior to 
entering the Boot Menu and, in my opinion, would be the perfect place to 
introduce another PlatformBootManagerLib hook, which retrieves 
platform-specific boot options, such as an UEFI Shell or other utilities like a 
Memory Test application.
If you have a few spare minutes, I'll be happy for feedback.

Thanks in advance for your time.

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

Re: [edk2] Trying to build OVMF fails

[Marvin H?user]

OvmfPkg builds fine here for X64/IA32-RELEASE-VS2013x86.
"'c:\Program' is not recognized as an internal or external command, operable 
program or batch file." Indicates something trying to access a path with spaces 
lacking proper quotation, so it's definitely a setup error on your side.
Maybe verify your environment variables and the tool paths (such as NASM) set 
for edk2?

> -Original Message-
> From: edk2-devel  On Behalf Of
> apia...@aol.com
> Sent: Thursday, May 10, 2018 3:38 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Trying to build OVMF fails
> 
> I am having a problem building OVMF to use as firmware for QEMU to test
> EFI binaries. I can only build the X64 arch of OVMF in linux. In windows both
> IA32, IA32/X64, and X64 all fail with some variation of this error:
> 
> 
> 
> 
> 
> "C:\Program Files (x86)\Microsoft Visual Studio
> 12.0\Vc\bin\x86_amd64\cl.exe"
> /Fod:\development\edk2\Build\OvmfX64\RELEASE_VS2013x86\X64\MdeMo
> dulePkg\Bus\Pci\EhciDxe\EhciDxe\OUTPUT\.\EhciUrb.obj /nologo /c /WX
> /GS- /W4 /Gs32768 /D UNICODE /O1b2s /GL /Gy /FIAutoGen.h /EHs-c- /GR-
> /GF /Gw /D MDEPKG_NDEBUG /D DISABLE_NEW_DEPRECATED_INTERFACES
> /Id:\development\edk2\MdeModulePkg\Bus\Pci\EhciDxe
> /Id:\development\edk2\Build\OvmfX64\RELEASE_VS2013x86\X64\MdeMod
> ulePkg\Bus\Pci\EhciDxe\EhciDxe\DEBUG  /Id:\development\edk2\MdePkg
> /Id:\development\edk2\MdePkg\Include
> /Id:\development\edk2\MdePkg\Include\X64
> /Id:\development\edk2\MdeModulePkg
> /Id:\development\edk2\MdeModulePkg\Include
> d:\development\edk2\MdeModulePkg\Bus\Pci\EhciDxe\EhciUrb.c
> 'c:\Program' is not recognized as an internal or external command, operable
> program or batch file.
> NMAKE : fatal error U1077: '"c:\Program Files (x86)\Windows
> Kits\8.0\bin\x64\rc.exe' : return code '0x1'
> Stop.
> EhciUrb.c
> Xhci.c
> "C:\Program Files (x86)\Microsoft Visual Studio
> 12.0\Vc\bin\x86_amd64\cl.exe"
> /Fod:\development\edk2\Build\OvmfX64\RELEASE_VS2013x86\X64\MdeMo
> dulePkg\Bus\Pci\UhciDxe\UhciDxe\OUTPUT\.\UhciSched.obj /nologo /c
> /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2s /GL /Gy /FIAutoGen.h /EHs-c-
> /GR- /GF /Gw /D MDEPKG_NDEBUG /D
> DISABLE_NEW_DEPRECATED_INTERFACES
> /Id:\development\edk2\MdeModulePkg\Bus\Pci\UhciDxe
> /Id:\development\edk2\Build\OvmfX64\RELEASE_VS2013x86\X64\MdeMod
> ulePkg\Bus\Pci\UhciDxe\UhciDxe\DEBUG  /Id:\development\edk2\MdePkg
> /Id:\development\edk2\MdePkg\Include
> /Id:\development\edk2\MdePkg\Include\X64
> /Id:\development\edk2\MdeModulePkg
> /Id:\development\edk2\MdeModulePkg\Include
> d:\development\edk2\MdeModulePkg\Bus\Pci\UhciDxe\UhciSched.c
> 
> 
> UhciSched.c
> 
> 
> build...
>  : error 7000: Failed to execute command
> C:\Program Files (x86)\Microsoft Visual Studio 12.0\Vc\bin\nmake.exe
> /nologo tbuild
> [d:\development\edk2\Build\OvmfX64\RELEASE_VS2013x86\X64\ShellPkg\D
> ynamicCommand\TftpDynamicCommand\TftpDynamicCommand]
> 
> 
> 
> 
> build...
>  : error F002: Failed to build module
> 
> d:\development\edk2\ShellPkg\DynamicCommand\TftpDynamicCommand\
> TftpDynamicCommand.inf [X64, VS2013x86, RELEASE]
> 
> 
> 
> And in linux IA32 and IA32/X64 fail with some varation of this error:
> 
> 
> 
> "GenFw" -e DXE_DRIVER -o
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.efi
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.dll
> GenFw: ERROR 3000: Invalid
> 
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.dll unsupported ELF EM_386 relocation 0xa.
> GenFw: ERROR 3000: Invalid
> 
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.dll unsupported ELF EM_386 relocation 0x9.
> GenFw: ERROR 3000: Invalid
> 
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.dll unsupported ELF EM_386 relocation 0xa.
> GenFw: ERROR 3000: Invalid
> 
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.dll unsupported ELF EM_386 relocation 0x9.
> GenFw: ERROR 3000: Invalid
> 
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> EBUG/FaultTolerantWriteDxe.dll unsupported ELF EM_386 relocation 0x9.
> GenFw: ERROR 3000: Invalid
> 
> /home/development/Desktop/edk2/Build/OvmfIa32/RELEASE_GCC5/IA32/
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/D
> 

Re: [edk2] [Patch v2] MdePkg/Include/IndustryStandard: Add PCI Express 4.0 header file

[Marvin H?user]

Good day,

Please consider for compatibility with some toolchains, byte-packed structs and 
unions must be decorated with the define 'PACKED'.

Thanks,
Marvin.

> -Original Message-
> From: edk2-devel  On Behalf Of Felix
> Polyudov
> Sent: Tuesday, February 27, 2018 10:07 PM
> To: edk2-devel@lists.01.org
> Cc: michael.d.kin...@intel.com; manickavasak...@ami.com;
> liming@intel.com
> Subject: [edk2] [Patch v2] MdePkg/Include/IndustryStandard: Add PCI
> Express 4.0 header file
> 
> v2: The structure is updated to include all the fields defined in the PCI-E
> specification.
> 
> The header includes Physical Layer PCI Express Extended Capability
> definitions described in section 7.7.5 of PCI Express Base Specification rev.
> 4.0.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Felix Polyudov 
> ---
>  MdePkg/Include/IndustryStandard/PciExpress40.h | 89
> ++
>  1 file changed, 89 insertions(+)
>  create mode 100644 MdePkg/Include/IndustryStandard/PciExpress40.h
> 
> diff --git a/MdePkg/Include/IndustryStandard/PciExpress40.h
> b/MdePkg/Include/IndustryStandard/PciExpress40.h
> new file mode 100644
> index 000..a832259
> --- /dev/null
> +++ b/MdePkg/Include/IndustryStandard/PciExpress40.h
> @@ -0,0 +1,89 @@
> +/** @file
> +Support for the PCI Express 4.0 standard.
> +
> +This header file may not define all structures.  Please extend as required.
> +
> +Copyright (c) 2018, American Megatrends, Inc. All rights reserved.
> +This program and the accompanying materials are licensed and made
> +available under the terms and conditions of the BSD License which
> +accompanies this distribution.  The full text of the license may be
> +found at http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef _PCIEXPRESS40_H_
> +#define _PCIEXPRESS40_H_
> +
> +#include 
> +
> +#pragma pack(1)
> +
> +/// The Physical Layer PCI Express Extended Capability definitions.
> +///
> +/// Based on section 7.7.5 of PCI Express Base Specification 4.0.
> +///@{
> +#define PCI_EXPRESS_EXTENDED_CAPABILITY_PHYSICAL_LAYER_16_0_ID
> 0x0026
> +#define
> PCI_EXPRESS_EXTENDED_CAPABILITY_PHYSICAL_LAYER_16_0_VER1  0x1
> +
> +// Register offsets from Physical Layer PCI-E Ext Cap Header
> +#define PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_CAPABILITIES_OFFSET
> 0x04
> +#define PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_CONTROL_OFFSET
> 0x08
> +#define PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_STATUS_OFFSET
> 0x0C
> +#define
> PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_LOCAL_DATA_PARITY_STATUS_
> OFFSET 0x10
> +#define
> PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_FIRST_RETIMER_DATA_PARITY_
> STATUS_OFFSET 0x14
> +#define
> PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_SECOND_RETIMER_DATA_PARIT
> Y_STATUS_OFFSET0x18
> +#define
> PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_LANE_EQUALIZATION_CONTROL
> _OFFSET0x20
> +
> +typedef union {
> +  struct {
> +UINT32 Reserved  : 32; // Reserved bit 0:31
> +  } Bits;
> +  UINT32   Uint32;
> +} PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_CAPABILITIES;
> +
> +typedef union {
> +  struct {
> +UINT32 Reserved  : 32; // Reserved bit 0:31
> +  } Bits;
> +  UINT32   Uint32;
> +} PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_CONTROL;
> +
> +typedef union {
> +  struct {
> +UINT32 EqualizationComplete  : 1; // bit 0
> +UINT32 EqualizationPhase1Success : 1; // bit 1
> +UINT32 EqualizationPhase2Success : 1; // bit 2
> +UINT32 EqualizationPhase3Success : 1; // bit 3
> +UINT32 LinkEqualizationRequest   : 1; // bit 4
> +UINT32 Reserved  : 27; // Reserved bit 5:31
> +  } Bits;
> +  UINT32   Uint32;
> +} PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_STATUS;
> +
> +typedef union {
> +  struct {
> +UINT8 DownstreamPortTransmitterPreset : 4; //bit 0..3
> +UINT8 UpstreamPortTransmitterPreset   : 4; //bit 4..7
> +  } Bits;
> +  UINT8   Uint8;
> +}
> PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_LANE_EQUALIZATION_CONTROL
> ;
> +
> +typedef struct {
> +  PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER  Header;
> +  PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_CAPABILITIES
> Capablities;
> +  PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_CONTROL   Control;
> +  PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_STATUSStatus;
> +  UINT32
> LocalDataParityMismatchStatus;
> +  UINT32
> FirstRetimerDataParityMismatchStatus;
> +  UINT32
> SecondRetimerDataParityMismatchStatus;
> +  UINT32Reserved;
> +
> PCI_EXPRESS_REG_PHYSICAL_LAYER_16_0_LANE_EQUALIZATION_CONTROL
> +LaneEqualizationControl; }
> 

Re: [edk2] MinPlatformPkg/PlatformInit: FV code

[Marvin H?user]

Good point with the DxeCore, I didn't consider that. Though OsBoot would be 
irrelevant to the PEI phase, wouldn't it be?

Thanks,
Marvin

From: Yao, Jiewen [mailto:jiewen@intel.com]
Sent: Friday, February 2, 2018 1:40 PM
To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-devel@lists.01.org
Subject: RE: MinPlatformPkg/PlatformInit: FV code

Excellent question.

Comment inline.

From: Marvin H?user [mailto:marvin.haeu...@outlook.com]
Sent: Wednesday, January 31, 2018 1:54 AM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen 
<jiewen@intel.com<mailto:jiewen@intel.com>>
Subject: MinPlatformPkg/PlatformInit: FV code

Dear developers, dear Jiewen,

I have been investigating the devel-MinPlatform branch of edk2-platforms for 
educational purposes and got two questions regarding the Firmware Volume code 
in PlatformInitPreMem, if you do not mind. I assume the tree was tested, so 
most likely I misunderstood some things.


  1.  Why is a Firmware Volume HOB built to cover the entire flash range 
(https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c#L379)?
 Am I correct that this implies a FV spanning through the entire flash MMIO 
range, which would then imply all other FVs are contained within it? This would 
make sense, however that's not what I saw in the KabylakeOpenBoardPkg Flash 
Map, which has the NV Storage first 
(https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/KabylakeOpenBoardPkg/Include/Fdf/FlashMapInclude.fdf#L25).

[Jiewen] You are right. We should not use FD region for FV. Will fix it.



  1.  Why are FV Info PPIs installed for the UefiBoot and the OsBoot FVs 
(https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c#L344)?
 If I checked correctly, installing this PPI type will trigger PeiCore to 
dispatch PEIMs in the FVs, however there are only DXE drivers in these. Why are 
no FV HOBs installed, which are gotten by DxeCore?

[Jiewen] In DxeIpl, PeiServicesFfsFindNextVolume() is used to search DxeCore.
In PeiCore, PeiFfsFindNextVolume() calls FindNextCoreFvHandle() for DxeCore one 
by one. If PcdFrameworkCompatibilitySupport is FALSE, it returns 
>Fv[Instance] directly.

And Fv[Instance] is added in FirmwareVolmeInfoPpiNotifyCallback(), when 
gEfiPeiFirmwareVolumeInfo2PpiGuid is installed.

So if PcdFrameworkCompatibilitySupport is FALSE, install PPI is the only way to 
let PEI core discover DxeCore.
Only if PcdFrameworkCompatibilitySupport is TRUE, install PPI is not required, 
but the FindNextCoreFvHandle() will install the PPI for the HobFv. The result 
is same.


Thank you
Yao Jiewen




Thanks in advance for your time!

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

[edk2] MinPlatformPkg/PlatformInit: FV code

[Marvin H?user]

Dear developers, dear Jiewen,

I have been investigating the devel-MinPlatform branch of edk2-platforms for 
educational purposes and got two questions regarding the Firmware Volume code 
in PlatformInitPreMem, if you do not mind. I assume the tree was tested, so 
most likely I misunderstood some things.


  1.  Why is a Firmware Volume HOB built to cover the entire flash range 
(https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c#L379)?
 Am I correct that this implies a FV spanning through the entire flash MMIO 
range, which would then imply all other FVs are contained within it? This would 
make sense, however that's not what I saw in the KabylakeOpenBoardPkg Flash 
Map, which has the NV Storage first 
(https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/KabylakeOpenBoardPkg/Include/Fdf/FlashMapInclude.fdf#L25).
  2.  Why are FV Info PPIs installed for the UefiBoot and the OsBoot FVs 
(https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c#L344)?
 If I checked correctly, installing this PPI type will trigger PeiCore to 
dispatch PEIMs in the FVs, however there are only DXE drivers in these. Why are 
no FV HOBs installed, which are gotten by DxeCore?

Thanks in advance for your time!

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

Re: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management mode FV file type and depex.

[Marvin H?user]

Hey Tim and Supreeth,

Sorry, 0x0C was a typo, 0x0D is the correct one.
The values of the SMM and MM constants are identical, this is just a naming 
update plus the introduction of the new value, MM Standalone Core, so 
backwards-compatibility is given.
Regarding the traditional MM Core, I just assumed it was an oversight as MdePkg 
is generic. If it wasn't, sorry.

Thanks,
Marvin

> -Original Message-
> From: Tim Lewis [mailto:tim.le...@insyde.com]
> Sent: Thursday, January 25, 2018 6:53 PM
> To: 'Supreeth Venkatesh' <supreeth.venkat...@arm.com>; 'Marvin H?user'
> <marvin.haeu...@outlook.com>; edk2-devel@lists.01.org
> Cc: michael.d.kin...@intel.com; liming@intel.com
> Subject: RE: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management mode
> FV file type and depex.
> 
> Supreeth --
> 
> Doesn't Appendix A of the PI 1.6 define this as:
> 
> #define EFI_FV_FILETYPE_SMM EFI_FV_FILETYPE_MM #define
> EFI_FV_FILETYPE_SMM_CORE EFI_FV_FILETYPE_MM_CORE
> 
> Thanks,
> 
> Tim
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Supreeth Venkatesh
> Sent: Thursday, January 25, 2018 9:45 AM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: michael.d.kin...@intel.com; liming@intel.com
> Subject: Re: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management
> mode FV file type and depex.
> 
> Marvin,
> 
> Thanks for your comments.
> As per PI v1.6 specification,
> EFI_FV_FILETYPE_MM_CORE value is 0x0D (MM Foundation that support
> MM Traditional Mode.)
> 
> This is traditional MM mode, which ARM is not supporting at this point. We
> are more interested in MM_CORE_STANDALONE mode.
> However, I have no issues in adding this in the patch, but would prefer if 
> this
> is added when MM traditional mode is supported in ARM.
> 
> Further,
> w.r.t defining *_SMM_* definitions via the *_MM_* definitions. I don't
> want to break backwards compatibility with existing SMM traditional mode
> implementations.
> I will be happy to let folks who have migrated to _MM_ definitions from
> _SMM_ definitions to send the patch across.
> 
> Thanks,
> Supreeth
> 
> -Original Message-
> From: Marvin H?user [mailto:marvin.haeu...@outlook.com]
> Sent: Tuesday, January 23, 2018 6:34 PM
> To: edk2-devel@lists.01.org
> Cc: Supreeth Venkatesh <supreeth.venkat...@arm.com>;
> michael.d.kin...@intel.com; liming@intel.com
> Subject: RE: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management mode
> FV file type and depex.
> 
> Good day,
> 
> I noticed this patch lacks the definition of "EFI_FV_FILETYPE_MM_CORE"
> (0x0C).
> Furthermore, may I suggest changing the *_SMM_* definitions to be
> defined via the *_MM_* definitions?
> 
> Best regards,
> Marvin.
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Supreeth Venkatesh
> > Sent: Tuesday, January 23, 2018 9:03 PM
> > To: edk2-devel@lists.01.org
> > Cc: michael.d.kin...@intel.com; liming@intel.com
> > Subject: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management mode
> FV
> > file type and depex.
> >
> > As per PI specification v1.6,
> > The following new file types are added:
> > EFI_FV_FILETYPE_MM_STANDALONE
> > EFI_FV_FILETYPE_MM_CORE_STANDALONE
> >
> > The following new section type is added:
> > EFI_SECTION_MM_DEPEX
> >
> > This patch adds the management mode FV file type and depex.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Achin Gupta <achin.gu...@arm.com>
> > Signed-off-by: Supreeth Venkatesh <supreeth.venkat...@arm.com>
> > Reviewed-by: Jiewen Yao <jiewen@intel.com>
> > ---
> >  MdePkg/Include/Pi/PiFirmwareFile.h | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h
> > b/MdePkg/Include/Pi/PiFirmwareFile.h
> > index b982c9eda3..6086d1bb2f 100644
> > --- a/MdePkg/Include/Pi/PiFirmwareFile.h
> > +++ b/MdePkg/Include/Pi/PiFirmwareFile.h
> > @@ -72,9 +72,12 @@ typedef UINT8 EFI_FFS_FILE_STATE;  #define
> > EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER  0x08
> >  #define EFI_FV_FILETYPE_APPLICATION   0x09
> >  #define EFI_FV_FILETYPE_SMM   0x0A
> > +#define EFI_FV_FILETYPE_MM0x0A
> >  #define EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE 0x0B
> >  #define EFI_FV_FILETYPE_COMBINED_SMM_DXE  0x0C
> > +#define EFI_FV_FILETYPE_COMBINED_MM_DXE   0x0C
> >  #define EFI_FV_FILETYPE_SMM_CORE  0x0D
&g

Re: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management mode FV file type and depex.

[Marvin H?user]

Good day,

I noticed this patch lacks the definition of "EFI_FV_FILETYPE_MM_CORE" (0x0C).
Furthermore, may I suggest changing the *_SMM_* definitions to be defined via 
the *_MM_* definitions?

Best regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Supreeth Venkatesh
> Sent: Tuesday, January 23, 2018 9:03 PM
> To: edk2-devel@lists.01.org
> Cc: michael.d.kin...@intel.com; liming@intel.com
> Subject: [edk2] [PATCH v1 1/1] MdePkg/Include: Add management mode FV
> file type and depex.
> 
> As per PI specification v1.6,
> The following new file types are added:
> EFI_FV_FILETYPE_MM_STANDALONE
> EFI_FV_FILETYPE_MM_CORE_STANDALONE
> 
> The following new section type is added:
> EFI_SECTION_MM_DEPEX
> 
> This patch adds the management mode FV file type and depex.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta 
> Signed-off-by: Supreeth Venkatesh 
> Reviewed-by: Jiewen Yao 
> ---
>  MdePkg/Include/Pi/PiFirmwareFile.h | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h
> b/MdePkg/Include/Pi/PiFirmwareFile.h
> index b982c9eda3..6086d1bb2f 100644
> --- a/MdePkg/Include/Pi/PiFirmwareFile.h
> +++ b/MdePkg/Include/Pi/PiFirmwareFile.h
> @@ -72,9 +72,12 @@ typedef UINT8 EFI_FFS_FILE_STATE;  #define
> EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER  0x08
>  #define EFI_FV_FILETYPE_APPLICATION   0x09
>  #define EFI_FV_FILETYPE_SMM   0x0A
> +#define EFI_FV_FILETYPE_MM0x0A
>  #define EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE 0x0B
>  #define EFI_FV_FILETYPE_COMBINED_SMM_DXE  0x0C
> +#define EFI_FV_FILETYPE_COMBINED_MM_DXE   0x0C
>  #define EFI_FV_FILETYPE_SMM_CORE  0x0D
> +#define EFI_FV_FILETYPE_MM_STANDALONE 0x0E
>  #define EFI_FV_FILETYPE_OEM_MIN   0xc0
>  #define EFI_FV_FILETYPE_OEM_MAX   0xdf
>  #define EFI_FV_FILETYPE_DEBUG_MIN 0xe0
> @@ -218,6 +221,7 @@ typedef UINT8 EFI_SECTION_TYPE;
>  #define EFI_SECTION_RAW   0x19
>  #define EFI_SECTION_PEI_DEPEX 0x1B
>  #define EFI_SECTION_SMM_DEPEX 0x1C
> +#define EFI_SECTION_MM_DEPEX  0x1C
> 
>  ///
>  /// Common section header.
> --
> 2.14.1
> 
> ___
> 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] S3 wake restore operations, regarding S3IoLib and S3PciLib

[Marvin H?user]

Dear developers,

First off, I'm writing this mail for educational purposes, so I would be very 
thankful if I got a reply, though you will not miss anything if you skip it.

I was browsing several initialization codes of Intel platforms (the ones 
included in the main edk2 repository, as well as MinPlatformPkg from 
edk2-platforms), as well as recent imports of library code into MdePkg (S3IoLib 
and S3PciLib).
While checking out latter, I noticed that all functions, including 'And' and 
'Or' operations, do not store the actual operations, but just the value that 
results from applying them to the register at the current time in execution.
While I checked out former, I noticed that actions are mixed (some operations 
using SaveMemWrite, which saves the entire register's value, some operations 
using SaveMemReadWrite, which saves the operations (and/or) involved).

Now, I do not have a lot of knowledge and/or experience with low-level x86 
initialization. I was checking out the PantherPoint PCH specification, trying 
to find a clue on when to use which operation precisely. Every register (be it 
a MMIO or PCI register) I was looking at (with a few exceptions, as I have just 
noticed), had a clear, defined value upon power-up. I'm also certain that there 
are no conditional executions possible by the BootScript table, which makes me 
think that storing the and/or operations and storing just the register values 
is basically achieving the same state in the end.

I hope for your experience regarding the following questions:

  1.  Is my quite poorly researched assumption that there is no actual 
difference between these two types of operation (for registers with a clearly 
defined default value) usually/always correct (for x86 platforms)?
 *   If they are, why are there different opcodes in the BootScript table? 
To support platforms for which that is not the case?
 *   If they are not, how could one be sure on what to use? Which 
specification or datasheet contains the necessary information? Is S3*Lib safe 
to use without explicate knowledge?
 *   If they are not, can there be a drawback to always storing the and/or 
operations to apply, such as applying the operations on a random register value 
(undefined default value)?

   i.  If there 
is no drawback, why are the S3*Libs using the method of storing the final value 
rather than the operation?

  1.  Just after composing the questions, I noticed two registers that indeed 
do not have a defined power-up value. In that case, I wonder whether it is 
unsafe to run a complete write, as it overwrites whatever is there (it's 
unpredictable, I suppose), or whether it is unsafe to use the and/or 
operations, as there may be unwanted bits set due to the undefined value.

Thank you in advance for your time, if you have read till here!

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

Re: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based x86 systems.

[Marvin H?user]

Hi Leo,

Yes, that's right. That PCD would be the only needed to be set by the platform 
DSC.

Thanks,
Marvin

> -Original Message-
> From: Duran, Leo [mailto:leo.du...@amd.com]
> Sent: Thursday, October 5, 2017 4:57 PM
> To: 'Marvin H?user' <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: Yao, Jiewen <jiewen@intel.com>
> Subject: RE: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based
> x86 systems.
> 
> 
> 
> > -Original Message-
> > From: Marvin H?user [mailto:marvin.haeu...@outlook.com]
> > Sent: Wednesday, October 04, 2017 8:19 PM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen@intel.com>; Duran, Leo
> <leo.du...@amd.com>
> > Subject: RE: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based
> > x86 systems.
> >
> > Hey Jiewen,
> > Hey Leo,
> >
> > May I suggest replacing "StandardSignatureIsAuthenticAMD()" and the
> > PCDs introduced by the series with a Fixed "PcdCpuVendor" enum or alike?
> 
> Hi Marvin,
> I'm not sure I follow your suggestion.
> It seems like the platform .DSC would then need to set PcdCpuVendor to an
> appropriate value at build-time... No?
> Leo.
> 
> > The contra of "StandardSignatureIsAuthenticAMD()" is that it's a
> > runtime action. From my point of view, this has no notable advantage
> > as boards use either an Intel or an AMD chipset and hence the EDK2
> > Firmware Package has compilation-time knowledge of the target platform.
> > On the other hand, the PCDs introduced by this patch cause the contra
> > that the platform DSC must set the correct vendor-specific (intel, AMD)
> value.
> > If the code checked for the CPU Vendor PCD and used the correct values
> > based on that, the values for the other vendors would get optimized
> > away (no unnecessary runtime actions) and the platform package owner
> > does not need to worry about setting PCDs to AMD-specific values
> > except for the CPU Vendor PCD.
> > Backwards-compatibility could be ensured by defaulting to some
> > reserved value for the PCD and handling detection via
> > StandardSignatureIsAuthenticAMD() if the platform DSC does not change
> it.
> >
> > Thanks,
> > Marvin.
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > Of Yao, Jiewen
> > > Sent: Thursday, October 5, 2017 2:49 AM
> > > To: Leo Duran <leo.du...@amd.com>; edk2-devel@lists.01.org
> > > Subject: Re: [edk2] [PATCH v4 0/5] Enhanced SMM support for
> > > AMD-based
> > > x86 systems.
> > >
> > > Hi Leo
> > > I do not suggest we introduce PcdCpuSmmSmramSaveStateMapOffset.
> > >
> > > This is unnecessary, because it is CPU attribute but not some end
> > > user configurable data.
> > >
> > > I think we can use CPUID to distinguish AMD from INTEL. Is that
> > > technically possible?
> > >
> > > I found we already have code at
> > >
> > >
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\BaseXApicX2ApicLib\BaseXApic
> > > X2ApicLib.c(1206):if (StandardSignatureIsAuthenticAMD()) {
> > >
> > >
> >
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\BaseXApicLib\BaseXApicLib.c(1
> > 1
> > > 11):if (StandardSignatureIsAuthenticAMD()) {
> > >
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On
> > > > Behalf Of Leo Duran
> > > > Sent: Thursday, October 5, 2017 3:02 AM
> > > > To: edk2-devel@lists.01.org
> > > > Subject: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based
> > > > x86 systems.
> > > >
> > > > This patch-set introduces a couple of FixedPCDs to replace
> > > > Intel-specific macros, and better support AMD-based x86 systems.
> > > >
> > > > 1) PcdCpuSmmSmramSaveStateMapOffset - SMRAM Save State Map
> > > Offset.
> > > > 2) PcdCpuSmmPSDOffset - Processor SMM Descriptor Offset in
> SMRAM.
> > > >
> > > > Changes since v3:
> > > > Correction on cover letter.
> > > >
> > > > Changes since v2:
> > > > The intent of this revision is to maintain compatibility with
> > > > existing packages. To that end, changes to OvmgfPkg and
> > > > QuarkSocPkg 

Re: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based x86 systems.

[Marvin H?user]

Hey Jiewen,
Hey Leo,

May I suggest replacing "StandardSignatureIsAuthenticAMD()" and the PCDs 
introduced by the series with a Fixed "PcdCpuVendor" enum or alike?
The contra of "StandardSignatureIsAuthenticAMD()" is that it's a runtime 
action. From my point of view, this has no notable advantage as boards use 
either an Intel or an AMD chipset and hence the EDK2 Firmware Package has 
compilation-time knowledge of the target platform.
On the other hand, the PCDs introduced by this patch cause the contra that the 
platform DSC must set the correct vendor-specific (intel, AMD) value.
If the code checked for the CPU Vendor PCD and used the correct values based on 
that, the values for the other vendors would get optimized away (no unnecessary 
runtime actions) and the platform package owner does not need to worry about 
setting PCDs to AMD-specific values except for the CPU Vendor PCD.
Backwards-compatibility could be ensured by defaulting to some reserved value 
for the PCD and handling detection via StandardSignatureIsAuthenticAMD() if the 
platform DSC does not change it.

Thanks,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Yao, Jiewen
> Sent: Thursday, October 5, 2017 2:49 AM
> To: Leo Duran ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based
> x86 systems.
> 
> Hi Leo
> I do not suggest we introduce PcdCpuSmmSmramSaveStateMapOffset.
> 
> This is unnecessary, because it is CPU attribute but not some end user
> configurable data.
> 
> I think we can use CPUID to distinguish AMD from INTEL. Is that technically
> possible?
> 
> I found we already have code at
> 
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\BaseXApicX2ApicLib\BaseXApic
> X2ApicLib.c(1206):if (StandardSignatureIsAuthenticAMD()) {
> 
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\BaseXApicLib\BaseXApicLib.c(11
> 11):if (StandardSignatureIsAuthenticAMD()) {
> 
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Leo Duran
> > Sent: Thursday, October 5, 2017 3:02 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based x86
> > systems.
> >
> > This patch-set introduces a couple of FixedPCDs to replace
> > Intel-specific macros, and better support AMD-based x86 systems.
> >
> > 1) PcdCpuSmmSmramSaveStateMapOffset - SMRAM Save State Map
> Offset.
> > 2) PcdCpuSmmPSDOffset - Processor SMM Descriptor Offset in SMRAM.
> >
> > Changes since v3:
> > Correction on cover letter.
> >
> > Changes since v2:
> > The intent of this revision is to maintain compatibility with existing
> > packages. To that end, changes to OvmgfPkg and QuarkSocPkg are
> reverted.
> > Moreover, pertinent macros are replaced in the C code, rather than on
> > header files that are shared globally.
> >
> > Changes since v1:
> > Revision to Cc list for UefiCpuPkg.
> >
> > Leo Duran (5):
> >   UefiCpuPkg/UefiCpuPkg.dec: Create FixedPCDs for SMM support
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Consume FixedPCDs to enhance SMM
> support
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Use FixedPCDs to enhance SMM
> support
> >   UefiCpuPkg/SmmCpuFeaturesLib: Consume FixedPCD to enhance SMM
> > support
> >   UefiCpuPkg/SmmCpuFeaturesLib: Use FixedPCD on non-STM library
> >
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf|  5
> > +
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf |  5
> > +
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Semaphore.c|  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm   |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm  |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c|
> > 10 +-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h|
> > 2 --
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |
> > 4 
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > |  4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c|  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c |
> > 4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S  |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm|  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm   |  4
> > +++-
> >  UefiCpuPkg/UefiCpuPkg.dec |  9
> > +
> >  17 files changed, 61 insertions(+), 18 deletions(-)
> >
> > --
> > 2.7.4
> >
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > 

Re: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38 toolchain

[Marvin H?user]

Hey,

I just noticed this patch as it recently has been pushed. I found this has been 
a reaction to https://bugzilla.tianocore.org/show_bug.cgi?id=410
Though as Clang correctly detected, this is Undefined Behavior per the C 
specification, so why was the warning hidden?
In context of the issue in UefiLib, providing the first element of the VA list 
as a prototyped argument, would have solved the issue without UB.

Do you wish such a patch?

Thanks,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Gao, Liming
> Sent: Monday, August 28, 2017 9:19 AM
> To: Shi, Steven ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in
> CLANG38 toolchain
> 
> Reviewed-by: Liming Gao 
> 
> >-Original Message-
> >From: Shi, Steven
> >Sent: Wednesday, August 23, 2017 3:01 PM
> >To: edk2-devel@lists.01.org; Gao, Liming 
> >Cc: Zhu, Yonghong ; Shi, Steven
> >
> >Subject: [PATCH] BaseTools/Conf: Support LLVM39 and LLVM40 in CLANG38
> >toolchain
> >
> >From: "Shi, Steven" 
> >
> >Add LLVM39 and LLVM40 support in CLANG38 toolchain
> >
> >Contributed-under: TianoCore Contribution Agreement 1.0
> >Signed-off-by: Steven Shi 
> >---
> > BaseTools/Conf/tools_def.template | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> >diff --git a/BaseTools/Conf/tools_def.template
> >b/BaseTools/Conf/tools_def.template
> >index 1fa3ca3..2f83341 100755
> >--- a/BaseTools/Conf/tools_def.template
> >+++ b/BaseTools/Conf/tools_def.template
> >@@ -380,7 +380,8 @@ DEFINE SOURCERY_CYGWIN_TOOLS =
> /cygdrive/c/Program
> >Files/CodeSourcery/Sourcery G
> > #   Intel(r) ACPI Compiler from
> > #   https://acpica.org/downloads
> > #   CLANG38  -Linux-  Requires:
> >-# Clang v3.8 or later, LLVMgold plugin and GNU 
> >binutils 2.26
> >targeting x86_64-linux-gnu
> >+# Clang v3.8, LLVMgold plugin and GNU binutils 
> >2.26
> targeting
> >x86_64-linux-gnu
> >+# Clang v3.9 or later, LLVMgold plugin and GNU 
> >binutils 2.28
> >targeting x86_64-linux-gnu
> > #Optional:
> > # Required to build platforms or ACPI tables:
> > #   Intel(r) ACPI Compiler from
> >@@ -5512,7 +5513,7 @@ DEFINE CLANG38_X64_PREFIX   =
> >ENV(CLANG38_BIN)
> > DEFINE CLANG38_IA32_TARGET  = -target i686-pc-linux-gnu
> > DEFINE CLANG38_X64_TARGET   = -target x86_64-pc-linux-gnu
> >
> >-DEFINE CLANG38_ALL_CC_FLAGS = DEF(GCC44_ALL_CC_FLAGS) -Wno-
> >empty-body -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-
> >negative-value -Wno-parentheses-equality -Wno-unknown-pragmas -
> Wno-
> >tautological-constant-out-of-range-compare -Wno-incompatible-library-
> >redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -
> msoft-
> >float -mno-implicit-float  -ftrap-
> >function=undefined_behavior_has_been_optimized_away_by_clang -
> >funsigned-char -fno-ms-extensions -Wno-null-dereference -Wno-
> >tautological-compare -Wno-unknown-warning-option
> >+DEFINE CLANG38_ALL_CC_FLAGS = DEF(GCC44_ALL_CC_FLAGS) -
> Wno-
> >empty-body -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-
> >negative-value -Wno-parentheses-equality -Wno-unknown-pragmas -
> Wno-
> >tautological-constant-out-of-range-compare -Wno-incompatible-library-
> >redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -
> msoft-
> >float -mno-implicit-float  -ftrap-
> >function=undefined_behavior_has_been_optimized_away_by_clang -
> >funsigned-char -fno-ms-extensions -Wno-null-dereference -Wno-
> >tautological-compare -Wno-unknown-warning-option -Wno-varargs
> >
> > ###
> > # CLANG38 IA32 definitions
> >--
> >2.7.4
> 
> ___
> 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] PI 1.6: Issues within the SPI sections.

[Marvin H?user]

Sorry, but there are even more:

* "EFI_LEGACY_SPI_CONTROLLER_GUID" contains the non-hex character "l", which I 
suspect to be "1".
* "EFI_SPI_HOST_GUID" does not follow the usual naming scheme. Wouldn't 
"EFI_SPI_HC_PROTOCOL_GUID" be more fiting?
* The possible values for EFI_SPI_HC_PROTOCOL.Attributes are not defined.
* The possible values for EFI_SPI_IO_PROTOCOL.Attributes are not defined.

Thanks again for your time!

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Wednesday, September 6, 2017 5:42 AM
> To: edk2-devel@lists.01.org
> Subject: Re: [edk2] PI 1.6: Issues within the SPI sections.
> 
> Sorry, I forgot to mention that the new SPI protocols still mention "SMM"
> rather than "MM". Is this intended?
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marvin H?user
> > Sent: Wednesday, September 6, 2017 5:22 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] PI 1.6: Issues within the SPI sections.
> >
> > Dear UEFI contributors,
> >
> > I am not an UEFI contributor and hence cannot submit changes. Could
> > somebody please take note of the following?
> >
> >
> >   1.  I do not see EFI_SPI_TRANSACTION_TYPE defined anywhere. There is
> > a list of names for possible values with descriptions (PI 1.6, Vol. 5,
> > page 368), though not assigned to numerical values. The size of the
> > type is unknown, as far as I can see.
> >   2.  Some parameter names in the protocol descriptions are flawed
> > (e.g. PI 1.6, Vol. 5, page 368: "Read Bytes", "Read Buffer").
> >   3.  Some protorype return types are flawed (e.g. PI 1.6, Vol. 5,
> > page 367: "EFI STATUS" (space instead of underscore); page 364: "FI
> > STATUS" (same as before, also "E" missing)).
> >   4.  Some prototype parameter types are flawed (e.g. PI 1.6, Vol. 5, page
> 364:
> > "EFI- LEGACY- SPI- FLASH- PROTOCOL").
> >   5.  Some status code descriptions are flawed (e.g. PI 1.6, Vol. 5, page 
> > 364:
> > "BLocksToProtect" (capital "L")).
> >   6.  Some formatations are flawed (e.g. PI 1.6, Vol. 5, page 358:
> > multiple parameters in one line).
> >   7.  Some function decorators are flawed (e.g. PI 1.6, Vol. 5, page 357: 
> > "In"
> > (lower-case "n")).
> >   8.  Some function parameter names are flawed (e.g. PI 1.6, Vol. 5, page
> 356:
> > "LengthinBytes" (lower-case "i")).
> >   9.  Some descriptions contain spaces in inappropiate places (e.g. PI 1.5,
> Vol.
> > 5, page 349: "EFI_SPI_P ART").
> >   10. Occasionally incorrect punctuation (e.g. PI 1.6, Vol. 5, page
> > 346: "[...] SPI busses, The SPI bus layer [...]" (comma instead of period)).
> >   11. PI 1.6, Vol. 5, page 349: The description of "SpiPeripheralDriverGuid"
> > speaks of a "Driversupported" routine. Is
> > EFI_DRIVER_BINDING_PROTOCOL.Supported() meant by this?
> >   12. PI 1.6, Vol. 5, page 350: The description of "ChipSelectParameter"
> > contains spaces, though they are barely noticable when having the PDF
> > opened with the latest version of Acrobat DC. Can this be fixed?
> >   13. UEFI PI 1.6, Vol. 5, 18.2 contains a reference to "EDK2". Is this
> intended?
> >
> > Please note that this list is not complete. Maybe the entire section
> > 18 should be reviewed again?
> >
> > Thank you in advance!
> >
> > Regards,
> > Marvin.
> > ___
> > 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

Re: [edk2] PI 1.6: Issues within the SPI sections.

[Marvin H?user]

Sorry, I forgot to mention that the new SPI protocols still mention "SMM" 
rather than "MM". Is this intended?

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Wednesday, September 6, 2017 5:22 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] PI 1.6: Issues within the SPI sections.
> 
> Dear UEFI contributors,
> 
> I am not an UEFI contributor and hence cannot submit changes. Could
> somebody please take note of the following?
> 
> 
>   1.  I do not see EFI_SPI_TRANSACTION_TYPE defined anywhere. There is a
> list of names for possible values with descriptions (PI 1.6, Vol. 5, page 
> 368),
> though not assigned to numerical values. The size of the type is unknown, as
> far as I can see.
>   2.  Some parameter names in the protocol descriptions are flawed (e.g. PI
> 1.6, Vol. 5, page 368: "Read Bytes", "Read Buffer").
>   3.  Some protorype return types are flawed (e.g. PI 1.6, Vol. 5, page 367: 
> "EFI
> STATUS" (space instead of underscore); page 364: "FI STATUS" (same as
> before, also "E" missing)).
>   4.  Some prototype parameter types are flawed (e.g. PI 1.6, Vol. 5, page 
> 364:
> "EFI- LEGACY- SPI- FLASH- PROTOCOL").
>   5.  Some status code descriptions are flawed (e.g. PI 1.6, Vol. 5, page 364:
> "BLocksToProtect" (capital "L")).
>   6.  Some formatations are flawed (e.g. PI 1.6, Vol. 5, page 358: multiple
> parameters in one line).
>   7.  Some function decorators are flawed (e.g. PI 1.6, Vol. 5, page 357: "In"
> (lower-case "n")).
>   8.  Some function parameter names are flawed (e.g. PI 1.6, Vol. 5, page 356:
> "LengthinBytes" (lower-case "i")).
>   9.  Some descriptions contain spaces in inappropiate places (e.g. PI 1.5, 
> Vol.
> 5, page 349: "EFI_SPI_P ART").
>   10. Occasionally incorrect punctuation (e.g. PI 1.6, Vol. 5, page 346: 
> "[...] SPI
> busses, The SPI bus layer [...]" (comma instead of period)).
>   11. PI 1.6, Vol. 5, page 349: The description of "SpiPeripheralDriverGuid"
> speaks of a "Driversupported" routine. Is
> EFI_DRIVER_BINDING_PROTOCOL.Supported() meant by this?
>   12. PI 1.6, Vol. 5, page 350: The description of "ChipSelectParameter"
> contains spaces, though they are barely noticable when having the PDF
> opened with the latest version of Acrobat DC. Can this be fixed?
>   13. UEFI PI 1.6, Vol. 5, 18.2 contains a reference to "EDK2". Is this 
> intended?
> 
> Please note that this list is not complete. Maybe the entire section 18 should
> be reviewed again?
> 
> Thank you in advance!
> 
> Regards,
> Marvin.
> ___
> 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] PI 1.6: Issues within the SPI sections.

[Marvin H?user]

Dear UEFI contributors,

I am not an UEFI contributor and hence cannot submit changes. Could somebody 
please take note of the following?


  1.  I do not see EFI_SPI_TRANSACTION_TYPE defined anywhere. There is a list 
of names for possible values with descriptions (PI 1.6, Vol. 5, page 368), 
though not assigned to numerical values. The size of the type is unknown, as 
far as I can see.
  2.  Some parameter names in the protocol descriptions are flawed (e.g. PI 
1.6, Vol. 5, page 368: "Read Bytes", "Read Buffer").
  3.  Some protorype return types are flawed (e.g. PI 1.6, Vol. 5, page 367: 
"EFI STATUS" (space instead of underscore); page 364: "FI STATUS" (same as 
before, also "E" missing)).
  4.  Some prototype parameter types are flawed (e.g. PI 1.6, Vol. 5, page 364: 
"EFI- LEGACY- SPI- FLASH- PROTOCOL").
  5.  Some status code descriptions are flawed (e.g. PI 1.6, Vol. 5, page 364: 
"BLocksToProtect" (capital "L")).
  6.  Some formatations are flawed (e.g. PI 1.6, Vol. 5, page 358: multiple 
parameters in one line).
  7.  Some function decorators are flawed (e.g. PI 1.6, Vol. 5, page 357: "In" 
(lower-case "n")).
  8.  Some function parameter names are flawed (e.g. PI 1.6, Vol. 5, page 356: 
"LengthinBytes" (lower-case "i")).
  9.  Some descriptions contain spaces in inappropiate places (e.g. PI 1.5, 
Vol. 5, page 349: "EFI_SPI_P ART").
  10. Occasionally incorrect punctuation (e.g. PI 1.6, Vol. 5, page 346: "[...] 
SPI busses, The SPI bus layer [...]" (comma instead of period)).
  11. PI 1.6, Vol. 5, page 349: The description of "SpiPeripheralDriverGuid" 
speaks of a "Driversupported" routine. Is 
EFI_DRIVER_BINDING_PROTOCOL.Supported() meant by this?
  12. PI 1.6, Vol. 5, page 350: The description of "ChipSelectParameter" 
contains spaces, though they are barely noticable when having the PDF opened 
with the latest version of Acrobat DC. Can this be fixed?
  13. UEFI PI 1.6, Vol. 5, 18.2 contains a reference to "EDK2". Is this 
intended?

Please note that this list is not complete. Maybe the entire section 18 should 
be reviewed again?

Thank you in advance!

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

Re: [edk2] Accessing RT services from OS

[Marvin H?user]

Good morning,

1.) Do you mean whether the OS exposes the Runtime Services? Windows and Linux 
expose the Variable Services (Linux even more, if I remember correctly) and 
macOS (not entirely sure about the latest version) the entire table via 
DeviceTree.
2.) Yes, you need to write a DXE Runtime Driver. One way to do it is install an 
UEFI Protocol and let the UEFI OS loader store its address (pay attention to 
allocate the structure from Runtime memory, update the pointers when going 
virtual and not use any Boot Services), another is to use the UEFI 
Configuration Table. Though remember that the OS still has hardware ownership, 
you might need to use Management Mode for your ideas. If you target Windows, 
I'm afraid software MMIs/ACPI or a shim for the RT Variable Services ("execute 
on variable write") are the only ways I know as you of course cannot alter the 
bootloader or access the System Table at runtime.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> ankit.sin...@dell.com
> Sent: Tuesday, September 5, 2017 7:21 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Accessing RT services from OS
> 
> Hi All,
> 
> Wanted to access RT services from OS.
>   1.) Are there any already such exposed OS function or utilities ?
>   2.) Can we plugin our own service/function to RT at run-time. ?
> 
> Regards,
> Ankit Singh
> 
> ___
> 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] PciLib/PciExpressLib: 64-bit r/w functions

[Marvin H?user]

Dear developers,

While browsing the KabylakeSiPkg code, I noticed that 64-bit registers are read 
directly via MmioRead64() as there is no PciRead64() function.
Is there a specific reason there are no PciRead64() and PciExpressRead64() 
functions or were they simply not needed previously?

Thanks and regards,
Marvin.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications.

[Marvin H?user]

Hey Liming,

I noticed that and assumed that a second call is not going to hurt much (only 
used in DEBUG afterall).
To work around that, the macro could be modified to either call IsNodeInList() 
or InternalBaseLibIsListValid() based on the PCD value,
though one would have to know/check the PCD's value to know what actually 
triggered the ASSERT(), so I opted to separate the ASSERTs (this also looks 
cleaner in my opinion).
The macro could also be modified to carry two separate ASSERT calls, which 
would solve the issue above,
though I would need to think of a proper name for a macro that checks list 
membership *and* whether the list is valid.
If you have a preference, please let me know, otherwise I'll come up with a V3 
soon.

Thanks and regards,
Marvin.

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Thursday, August 3, 2017 5:28 PM
> To: marvin.haeu...@outlook.com; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList
> verifications.
> 
> Marvin:
>   If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be
> checked twice in two ASSERT(). In the original logic, 
> InternalBaseLibIsListValid
> only runs once. Could we work out the same check logic?
> 
> Thanks
> Liming
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marvin H?user
> > Sent: Wednesday, August 2, 2017 9:12 PM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> > <liming@intel.com>
> > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList
> verifications.
> >
> > 1) Replace InternalBaseLibIsNodeInList() with
> >InternalBaseLibIsListValid().
> >- The verification whether Node is within the doubly-linked List
> >  is now done by IsNodeInList().
> >- Whether the list is valid is returned.
> >
> > 2) The comments within InsertHeadList() and InsertTailList() stated
> >that it is checked whether Entry is not part of the doubly-linked
> >list. This was not done as argument 3 of
> >InternalBaseLibIsNodeInList() indicated whether the check is done,
> >not whether to check if the node is or is not in the list. This
> >has been fixed by using IsNodeInList() for the ASSERTs.
> >
> > V2:
> >   - Fix IsListEmpty() to ASSERT when the passed list is invalid.
> >   - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether
> the
> > passed node is part of the list when PcdVerifyNodeInList is TRUE.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marvin Haeuser <marvin.haeu...@outlook.com>
> > ---
> >  MdePkg/Library/BaseLib/LinkedList.c | 105 +---
> >  1 file changed, 47 insertions(+), 58 deletions(-)
> >
> > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
> > b/MdePkg/Library/BaseLib/LinkedList.c
> > index 484ee836b8c2..6864fae7d939 100644
> > --- a/MdePkg/Library/BaseLib/LinkedList.c
> > +++ b/MdePkg/Library/BaseLib/LinkedList.c
> > @@ -15,25 +15,38 @@
> >  #include "BaseLibInternals.h"
> >
> >  /**
> > -  Worker function that locates the Node in the List.
> > +  If PcdVerifyNodeInList is TRUE, checks whether FirstNode and
> > + SecondNode are  part of the same doubly-linked list.
> >
> > -  By searching the List, finds the location of the Node in List. At
> > the same time,
> > -  verifies the validity of this list.
> > +  If FirstEntry is NULL, then ASSERT().
> > +  If FirstEntry->ForwardLink is NULL, then ASSERT().
> > +  If FirstEntry->BackLink is NULL, then ASSERT().
> > +  If SecondEntry is NULL, then ASSERT();  If
> > + PcdMaximumLinkedListLength is not zero, and List contains more than
> > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > +
> > +  @param  FirstEntry   A pointer to a node in a linked list.
> > +  @param  SecondEntry  A pointer to the node to locate.
> > +
> > +  @retval TRUE   SecondEntry is in the same doubly-linked list as 
> > FirstEntry
> > + or PcdVerifyNodeInList is FALSE.
> > +  @retval FALSE  SecondEntry isn't in the same doubly-linked list as
> FirstEntry,
> > + or FirstEntry is invalid.
> > +
> > +**/
> > +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry)  \
> > +  (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList
> > +((FirstEntry), (SecondEntry)))
> > +

Re: [edk2] [Patch 4/4] MdePkg: Fix Xcode 9 Beta treating 32-bit left shift as undefined

[Marvin H?user]

Hello Yonghong and Andrew,

This patch may cause issues when compiling MSVC x IA32 as the optimizer might 
replace the shift with an intrinsic.
I suggest you to use LShiftU64() rather than the << operator.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Yonghong Zhu
> Sent: Wednesday, August 2, 2017 11:28 AM
> To: edk2-devel@lists.01.org
> Cc: Michael D Kinney ; Andrew Fish
> ; Liming Gao 
> Subject: [edk2] [Patch 4/4] MdePkg: Fix Xcode 9 Beta treating 32-bit left 
> shift
> as undefined
> 
> Bug: https://bugzilla.tianocore.org/show_bug.cgi?id=635
> 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Andrew Fish 
> ---
>  MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git
> a/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
> b/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
> index e3b2846..ce1fe0a 100644
> --- a/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
> +++ b/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
> @@ -38,18 +38,18 @@ FillBuf (
>)
>  {
>//
>// Left shift NumOfBits of bits in advance
>//
> -  Sd->mBitBuf = (UINT32) (Sd->mBitBuf << NumOfBits);
> +  Sd->mBitBuf = (UINT32) (((UINT64)Sd->mBitBuf) << NumOfBits);
> 
>//
>// Copy data needed in bytes into mSbuBitBuf
>//
>while (NumOfBits > Sd->mBitCount) {
> 
> -Sd->mBitBuf |= (UINT32) (Sd->mSubBitBuf << (NumOfBits = (UINT16)
> (NumOfBits - Sd->mBitCount)));
> +Sd->mBitBuf |= (UINT32) (((UINT64)Sd->mSubBitBuf) << (NumOfBits =
> (UINT16) (NumOfBits - Sd->mBitCount)));
> 
>  if (Sd->mCompSize > 0) {
>//
>// Get 1 byte into SubBitBuf
>//
> --
> 2.6.1.windows.1
> 
> ___
> 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] MdePkg: Add PcdPciExpressRegionLength PCD Token

[Marvin H?user]

A bunch of platforms, such as QuarkSocPkg, already declare such a PCD named 
PcdPciExpressSize of type UINT64.
Maybe PcdPciExpressSize|UINT64 should be 'promoted' to a MdePkg PCD and the 
platform-defined PCDs replaced?

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Felix Poludov
> Sent: Wednesday, August 2, 2017 3:48 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH] MdePkg: Add PcdPciExpressRegionLength PCD
> Token
> 
> Add PcdPciExpressRegionLength PCD Token to MdePkg.
> The new token can be used in conjunction with PcdPciExpressBaseAddress
> PCD token to describe PCI Express MMIO region.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Felix Polyudov 
> ---
> 
> Resending with inline patch.
> 
> --
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index
> d6928b3..7e55019 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2089,6 +2089,10 @@
># @Prompt PCI Express Base Address.
> 
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE000|UINT6
> 4|0x000a
> +  ## Length of the PCI express region.
> +  # @Prompt PCI Express Region Length.
> +
> +
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressRegionLength|0x1000|UINT
> 32|0
> + x0031
> +
>## Default current ISO 639-2 language: English & French.
># @Prompt Default Value of LangCodes Variable.
> 
> gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes|"engfraengf
> ra"|VOID*|0x001c
> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni index
> a110e45..62df5dc 100644
> --- a/MdePkg/MdePkg.uni
> +++ b/MdePkg/MdePkg.uni
> @@ -276,6 +276,10 @@
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP
> #language en-US "This value is used to set the base address of PCI express
> hierarchy."
> +#string
> STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressRegionLength_PROMPT
> #language en-US "PCI Express Region Length"
> +
> +#string
> STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressRegionLength_HELP
> #language en-US "Length of the PCI express region."
> +
> #string
> STR_gEfiMdePkgTokenSpaceGuid_PcdUefiVariableDefaultLangCodes_PROM
> PT  #language en-US "Default Value of LangCodes Variable"
>  #string
> STR_gEfiMdePkgTokenSpaceGuid_PcdUefiVariableDefaultLangCodes_HELP
> #language en-US "Default current ISO 639-2 language: English & French."
> --
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and
> proprietary to American Megatrends, Inc.  This communication is intended to
> be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited.  Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.
> ___
> 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] [UEFI PI 1.6/EDK2] Missing decorators for EFI_PEI_GET_VARIABLE2.

[Marvin H?user]

Thank you very much!
I will request a new member account soon (due to departure, my E-Mail account 
was deactivated and I had to delete my account).
Is a 'Member' status enough to submit ECRs?

Thanks,
Marvin.

> -Original Message-
> From: Zeng, Star [mailto:star.z...@intel.com]
> Sent: Tuesday, July 25, 2017 3:14 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> ECR 1828: Add decorator 'OPTIONAL' for Attributes parameter of
> EFI_PEI_GET_VARIABLE2 has been submitted.
> 
> Thanks,
> Star
> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, July 25, 2017 5:46 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> Sure, I will help do that. :)
> 
> Thanks,
> Star
> -----Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Tuesday, July 25, 2017 5:43 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: Re: [edk2] [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> Sorry, I remembered 'IN OUT' incorrectly then, you are correct. Only
> 'OPTIONAL' is lacking.
> Would be very kind of you if you could help submitting the ECR, I do not have
> an active account at this point.
> 
> Thanks,
> Marvin.
> 
> > -Original Message-
> > From: Zeng, Star [mailto:star.z...@intel.com]
> > Sent: Tuesday, July 25, 2017 11:09 AM
> > To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> > de...@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>
> > Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> > EFI_PEI_GET_VARIABLE2.
> >
> > As I know submitting ECR needs log in as a member at
> > http://www.uefi.org/memberslogin, I am not sure the whole process.
> > If needed, I can help to submit the ECR.
> >
> > According to EDK2 coding style doc at
> > https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-
> > specification/content/5_source_files/57_c_programming.html#table-9-
> > parameter-modifiers
> > IN OUT  Passed by reference, and the passed-in referenced data is
> consumed
> > and then modified by the routine.
> >
> > But the code will not consume the pass in referenced data. Personally,
> > I don't think just checking NULL pointer should be marked as 'IN'
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Marvin H?user [mailto:marvin.haeu...@outlook.com]
> > Sent: Tuesday, July 25, 2017 4:58 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>
> > Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> > EFI_PEI_GET_VARIABLE2.
> >
> > Hey Star,
> >
> > Thanks for your comment! Sorry, I never submited such a report, could
> > you please point me in the right direction? The only way of contact I
> > found on the site of the UEFI Forum were Administration and Press,
> > both don't sound like the right place to post to. Or Is
> > 'Administration' specification administration rather than forum
> administration?
> >
> > Regarding 'IN': Correct me if I'm wrong, but in contrast to just 'OUT'
> > parameters, which are 'blindly' written to, 'Attributes' must be
> > checked against NULL first before attempting a dereference. Is this
> > not the correct usage of IN? Is IN only used, when the pointer's destination
> is read?
> >
> > Thanks again!
> >
> > Regards,
> > Marvin.
> >
> > > -Original Message-
> > > From: Zeng, Star [mailto:star.z...@intel.com]
> > > Sent: Tuesday, July 25, 2017 10:25 AM
> > > To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> > > de...@lists.01.org
> > > Cc: Zeng, Star <star.z...@intel.com>
> > > Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> > > EFI_PEI_GET_VARIABLE2.
> > >
> > > Marvin,
> > >
> > > I think you are right about the statement of decorator 'OPTIONAL',
> > > you can submit PI ECR, then it can be aligned with UEFI
> EFI_GET_VARIABLE.
> > > typedef
> > > EFI_STATUS
> > > (EFIAPI *EFI_GET_VARIABLE)(
> > >   IN CHAR16  *VariableName,
> > >   IN EFI_GUID   

Re: [edk2] [UEFI PI 1.6/EDK2] Missing decorators for EFI_PEI_GET_VARIABLE2.

[Marvin H?user]

Sorry, I remembered 'IN OUT' incorrectly then, you are correct. Only 'OPTIONAL' 
is lacking.
Would be very kind of you if you could help submitting the ECR, I do not have 
an active account at this point.

Thanks,
Marvin.

> -Original Message-
> From: Zeng, Star [mailto:star.z...@intel.com]
> Sent: Tuesday, July 25, 2017 11:09 AM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> As I know submitting ECR needs log in as a member at
> http://www.uefi.org/memberslogin, I am not sure the whole process.
> If needed, I can help to submit the ECR.
> 
> According to EDK2 coding style doc at
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-
> specification/content/5_source_files/57_c_programming.html#table-9-
> parameter-modifiers
> IN OUTPassed by reference, and the passed-in referenced data is 
> consumed
> and then modified by the routine.
> 
> But the code will not consume the pass in referenced data. Personally, I don't
> think just checking NULL pointer should be marked as 'IN'
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Marvin H?user [mailto:marvin.haeu...@outlook.com]
> Sent: Tuesday, July 25, 2017 4:58 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> Hey Star,
> 
> Thanks for your comment! Sorry, I never submited such a report, could you
> please point me in the right direction? The only way of contact I found on the
> site of the UEFI Forum were Administration and Press, both don't sound like
> the right place to post to. Or Is 'Administration' specification 
> administration
> rather than forum administration?
> 
> Regarding 'IN': Correct me if I'm wrong, but in contrast to just 'OUT'
> parameters, which are 'blindly' written to, 'Attributes' must be checked
> against NULL first before attempting a dereference. Is this not the correct
> usage of IN? Is IN only used, when the pointer's destination is read?
> 
> Thanks again!
> 
> Regards,
> Marvin.
> 
> > -Original Message-
> > From: Zeng, Star [mailto:star.z...@intel.com]
> > Sent: Tuesday, July 25, 2017 10:25 AM
> > To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> > de...@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>
> > Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> > EFI_PEI_GET_VARIABLE2.
> >
> > Marvin,
> >
> > I think you are right about the statement of decorator 'OPTIONAL', you
> > can submit PI ECR, then it can be aligned with UEFI EFI_GET_VARIABLE.
> > typedef
> > EFI_STATUS
> > (EFIAPI *EFI_GET_VARIABLE)(
> >   IN CHAR16  *VariableName,
> >   IN EFI_GUID*VendorGuid,
> >   OUTUINT32  *Attributes,OPTIONAL
> >   IN OUT UINTN   *DataSize,
> >   OUTVOID    *Data   OPTIONAL
> >   );
> >
> > And since the passed-in state of the referenced data is not used by
> > the routine, I don't think decorator 'IN' should be added.
> >
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marvin H?user
> > Sent: Tuesday, July 25, 2017 12:42 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [UEFI PI 1.6/EDK2] Missing decorators for
> > EFI_PEI_GET_VARIABLE2.
> >
> > Dear developers,
> >
> > I noticed that EFI_PEI_GET_VARIABLE2 is lacking decorators in both the
> > UEFI PI 1.6 specification and the EDK2 codebase. The parameter
> > description for 'Attributes' starts with 'If non-NULL', hence it may
> > be NULL, which is not reflected in the function prototype with the
> decorator 'OPTIONAL'.
> > Furthermore, as the possibility of it being NULL is not coupled to the
> > values of the other parameters, 'IN' should be added as well.
> >
> > Can someone please forward this to the right working group?
> >
> > Thanks and best regards,
> > Marvin.
> > ___
> > 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] [UEFI PI 1.6/EDK2] Missing decorators for EFI_PEI_GET_VARIABLE2.

[Marvin H?user]

Hey Star,

Thanks for your comment! Sorry, I never submited such a report, could you please
point me in the right direction? The only way of contact I found on the site of 
the
UEFI Forum were Administration and Press, both don't sound like the right place 
to
post to. Or Is 'Administration' specification administration rather than forum
administration?

Regarding 'IN': Correct me if I'm wrong, but in contrast to just 'OUT' 
parameters, which
are 'blindly' written to, 'Attributes' must be checked against NULL first 
before attempting
a dereference. Is this not the correct usage of IN? Is IN only used, when the 
pointer's
destination is read?

Thanks again!

Regards,
Marvin.

> -Original Message-
> From: Zeng, Star [mailto:star.z...@intel.com]
> Sent: Tuesday, July 25, 2017 10:25 AM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: RE: [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> Marvin,
> 
> I think you are right about the statement of decorator 'OPTIONAL', you can
> submit PI ECR, then it can be aligned with UEFI EFI_GET_VARIABLE.
> typedef
> EFI_STATUS
> (EFIAPI *EFI_GET_VARIABLE)(
>   IN CHAR16  *VariableName,
>   IN EFI_GUID*VendorGuid,
>   OUTUINT32  *Attributes,OPTIONAL
>   IN OUT UINTN   *DataSize,
>   OUTVOID*Data   OPTIONAL
>   );
> 
> And since the passed-in state of the referenced data is not used by the
> routine, I don't think decorator 'IN' should be added.
> 
> 
> 
> Thanks,
> Star
> -----Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Tuesday, July 25, 2017 12:42 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [UEFI PI 1.6/EDK2] Missing decorators for
> EFI_PEI_GET_VARIABLE2.
> 
> Dear developers,
> 
> I noticed that EFI_PEI_GET_VARIABLE2 is lacking decorators in both the UEFI
> PI 1.6 specification and the EDK2 codebase. The parameter description for
> 'Attributes' starts with 'If non-NULL', hence it may be NULL, which is not
> reflected in the function prototype with the decorator 'OPTIONAL'.
> Furthermore, as the possibility of it being NULL is not coupled to the values
> of the other parameters, 'IN' should be added as well.
> 
> Can someone please forward this to the right working group?
> 
> Thanks and best regards,
> Marvin.
> ___
> 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] [UEFI PI 1.6/EDK2] Missing decorators for EFI_PEI_GET_VARIABLE2.

[Marvin H?user]

Dear developers,

I noticed that EFI_PEI_GET_VARIABLE2 is lacking decorators in both the UEFI PI 
1.6 specification and the EDK2
codebase. The parameter description for 'Attributes' starts with 'If non-NULL', 
hence it may be NULL, which is
not reflected in the function prototype with the decorator 'OPTIONAL'. 
Furthermore, as the possibility of it
being NULL is not coupled to the values of the other parameters, 'IN' should be 
added as well.

Can someone please forward this to the right working group?

Thanks and best regards,
Marvin.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Suggestion/Bug] Extend the usage of 'shared' modules

[Marvin H?user]

Dear developers,

Dear KabylakeSiliconPkg devs: If you are not interested in the suggestion of 
generic, shared modules, please read my comment on 2) nevertheless, as it 
contains a potential bug report. Sorry, but I could not verify this yet.

I have been exploring the code of most of the Intel 'open platforms' lately and 
noticed they shared a bunch of slightly different modules, which are diverse to 
fit the platform's requirements.
One of them is PchSmmDispatcher/QNCSmmDispatcher. I'm not sure about non-Intel 
platforms, though in my opinion, there should be a generic module in 
MdeModulePkg, if it can be shared across brands, or one in In IntelSiliconPkg, 
if it's useful for Intel platforms only. To see why I think a generic, shared 
module that consumes a platform library would be hugely beneficial, please take 
a look at these lines:


  1.  
https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/PchSmmCore.c#L361
  2.  
https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/PchSmmCore.c#L890
  3.  
https://github.com/tianocore/edk2/blob/master/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c#L753

1) shows a hook to SmmReadyToLock, which I first and only saw in 
KabylakeSiliconPkg. This addition has not been brought to the Quark platform, 
for example. With a shared module, all platforms that embed it would have 
profited.

2) on the other hand shows KabylakeSiliconPkg's PchSmmDispatcher accessing 
RecordInDb after the callback function has been executed, which QuarkSocPkg's 
QNCSmmDispatcher explicitely forbids in 3), as it might have been freed by the 
call. I am not sure whether it was a bug in the Quark firmware that has been 
fixed by the time the Kabylake code was written, though I don't believe so, 
because the changes to the loop to process multiple SMI sources in one go is 
not present in the Kabylake code either. If there was a shared module, the fix 
to Quark would have profited all other platforms.

It's obvious to me that KabylakeSiliconPkg has a bunch of things that were 
previously only distributed privately as part of the private Reference Code. I 
think that keeping the RC as small as possible (the very best case would be 
only platform library classes consumed by generic, open source modules) is a 
goal worth persuing, as more code can be shared and community-verified as part 
of EDK2.

Thanks for your time!

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

Re: [edk2] EDK II license query

[Marvin H?user]

Hey,

I'm not sure if the situation changed as I did not check the license of every 
single package, though in the past, at least the FAT driver was released under 
a different license than the rest, due to Microsoft.
That has been resolved, though personally, I don't see a benefit in a top-level 
license file if there's the possibility of new packages still being furnished 
under a license different than the main one.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Varun Sethi
> Sent: Friday, July 14, 2017 1:18 PM
> To: edk2-devel@lists.01.org
> Cc: Peter W Newton 
> Subject: [edk2] EDK II license query
> 
> Hi,
> I don't see a top-level license in the EDKII repository. As per the following 
> link
> EDKII uses BSD license.
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
> 
> Is there a specific reason for not having a top-level license in the EDK II
> repository?
> 
> Regards
> Varun
> 
> ___
> 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][edk2-platforms] Vlv2TbltDevicePkg: Add SMBIOS Type 19.

[Marvin H?user]

One comment regarding the record allocation is inline.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> lushifex
> Sent: Tuesday, July 4, 2017 10:24 AM
> To: edk2-devel@lists.01.org
> Cc: david@intel.com
> Subject: [edk2] [Patch][edk2-platforms] Vlv2TbltDevicePkg: Add SMBIOS
> Type 19.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: lushifex 
> ---
>  .../SmBiosMiscDxe/MiscMemoryArrayMappedAddress.uni | Bin 0 -> 1318
> bytes
>  .../MiscMemoryArrayMappedAddressData.c |  29 
>  .../MiscMemoryArrayMappedAddressFunction.c | 146
> +
>  .../SmBiosMiscDxe/MiscSubclassDriverDataTable.c|   4 +-
>  Vlv2TbltDevicePkg/SmBiosMiscDxe/SmBiosMiscDxe.inf  |   5 +-
>  5 files changed, 182 insertions(+), 2 deletions(-)  create mode 100644
> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddress.uni
>  create mode 100644
> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressData.
> c
>  create mode 100644
> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFunct
> ion.c
> 
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddress.un
> i
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddress.un
> i
> new file mode 100644
> index
> ..27d298a2d858de029b581553f
> 069002872d0e8ea
> GIT binary patch
> literal 1318
> zcmZvcU2oG+42FG;#D6#{H-NSlb~AwxqNN2Q*iyBb2AAo_x=8()k}}wzhdr;;G-
> *Lm
> zj(u#O*YB~N{Qc9hjs@PAJi$KLjb*m9xjnTfSmhhqtvy(QeQGJbU>EpTwy_+0ur
> 4yk
> zBmDw-$1}H?duGU-UBT1FGA5#Dk;Q1iwwzIHu-
> Enmf0eV6!9J4Zj;NgM3wUgaeGZQo
> z$TNGzxihrW{qEdO&?8$DId`r?$idK>V$IOj
> +!U>pkm?$agRv+57*n
> zOJI}{9l+1QU3iWa`;;82z?KNzFNO1zh!v6YJ#cPKe83`B%9o)nL91_{V%6y-zA4(;
> znF+VT*Xh|V!#%osm)e9?=YBd1Vb@H`Tq|c?p@^CIX8Zc+P8(*SKG
> DxYS
> zc`c@_|K2u(jSy9hvR=wnu-C08@ND))RTcXi>}7|<`8P4goDS2bA*`43)Dj-JN
> zMqM>WTRQR9CtMR&)>iSkSGvvZbNbUcs(pcF0`nO$}Mj_#i5Zgu`f1&
> *^o(!
> z?K5kaFF}(!X6?I7utv<n3k9AFRFN`$%(5nsoEh_NHbX!;hlwvfUVdJEa}
> zy>=%JPS^N_8hgfI*F=q+*(LS}RAI!PeZ)@OWmC?CSj8x78TE-
> +W@#Ee>{B
> z%3BQV`yQu8kEq3v$x_aKqk{Ex>F?BW?y~89M*TIv3!i^Ys(N_JU4+{LNj~+cpf4
> >y
> P6Yw-2;WYQhdX?)D9Tmic
> 
> literal 0
> HcmV?d1
> 
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressDat
> a.c
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressDa
> ta.c
> new file mode 100644
> index 000..f71b548
> --- /dev/null
> +++
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressDa
> ta.c
> @@ -0,0 +1,29 @@
> +/** @file
> +  Static data of Physical Memory Array Mapped Address. SMBIOS Type 19.
> +
> +  Copyright (c) 2012 - 2017, Intel Corporation. All rights
> + reserved.
> +
> +  This program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD License  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "CommonHeader.h"
> +#include "MiscSubclassDriver.h"
> +
> +//
> +// Static (possibly build generated) Physical Memory Array Mapped Address
> Data.
> +//
> +MISC_SMBIOS_TABLE_DATA(EFI_MEMORY_ARRAY_START_ADDRESS_DAT
> A,
> +MiscMemoryArrayMappedAddress) = {
> +0,  //< 
> StartingAddress
> +0,  //< EndingAddress
> +0,  //< 
> MemoryArrayHandle
> +0x00//< Partition 
> Width
> +};
> +
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFu
> nction.c
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFu
> nction.c
> new file mode 100644
> index 000..27d0ab7
> --- /dev/null
> +++
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFu
> ncti
> +++ on.c
> @@ -0,0 +1,146 @@
> +/** @file
> +  Dynamic data of Physical Memory Array Mapped Address. SMBIOS Type
> 19.
> +
> +  Copyright (c) 2012 - 2017, Intel Corporation. All rights
> + reserved.
> +
> +  This program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD License  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "CommonHeader.h"
> +#include "MiscSubclassDriver.h"
> +#include 
> +#include 
> +#include 
> +
> +#define MAX_SOCKETS  2
> +#define 

Re: [edk2] writing EDK compatible application.

[Marvin H?user]

Hey,

The entry point declarations and the calling conventions have not changed since 
EFI 1.10, though X64 was not a supported platform for 1.10 if I remember 
correctly.
To be honest, I never saw an x64 EFI 1.10 implementation other than Apple's, 
though even if it still signals that version, it's actually more UEFI by now.
I don't own a Mac, so if it's the platform you tested your app on, I can't tell 
you why it didn't work, but I would suggest you to run the app from an EFI 
Shell.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Amit kumar
> Sent: Tuesday, July 4, 2017 7:21 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] writing EDK compatible application.
> 
> 
> HI,
> 
> I have written a code (say helloworld program ) using edk2 framework,
> named the output efi file as BOOTx64.efi and placed it on a removable media
> in EFI/BOOT/ directory so that the application is listed in one time boot 
> menu.
> When selected from boot menu it prints "Hello World".
> Which works as expected when tested on UEFI 2.3 and above platforms.
> But the same code fails to execute on EFI 1.10 platforms. Which i suppose is
> the problem with application entry point.
> Can somebody suggest me a way so that the application entry function can
> be compatible to both the platform. Even when written according to UEFI
> 2.5+ Spec ?
> 
> Thanks
> Amit
> ___
> 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] UEFI ABI calling convention details

[Marvin H?user]

Hey Rafael,

The UEFI calling conventions are detailed in the UEFI specification, more 
specifically section 2.3 of the UEFI 2.7 specification.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Rafael Machado
> Sent: Wednesday, June 28, 2017 1:55 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] UEFI ABI calling convention details
> 
> Hi everyone
> 
> I have a question.
> Some time ago, at this conversation "
> https://www.mail-archive.com/edk2-devel@lists.01.org/msg25383.html; it
> was mentioned the UEFI ABI calling convention.
> 
> I've tried to search for some document with the details of this calling
> convention, but didn't find anything.
> Does anyone know where can I have more details about this ? (UEFI ABI
> calling convention)
> 
> Thanks and Regards
> Rafael R. Machado
> ___
> 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] Regarding UefiDriverEntryPoint unload handler

[Marvin H?user]

Dear developers,

While performing some research tasks, I noticed that when 
UefiDriverEntryPoint's _gDriverUnloadImageCount is 0 (only MODULE_UNLOAD 
entries are count, DESTRUCTOR as they are used with libraries are not, as far 
as I can see), EFI_LOADED_IMAGE_PROTOCOL.Unload is not set, even if libraries 
with destructors are included by the built module.
Is this intentional, so, is a module without a MODULE_UNLOAD property in its 
build file considered a module that does not support being unloaded? If so, why 
is EFI_LOADED_IMAGE_PROTOCOL.Unload not set to a dummy that returns an EFI 
error code?

For example, DxeDebugPrintErrorLevelLib installs a protocol interface in its 
CONSTRUCTOR function. When this library is included in a module without a 
MODULE_UNLOAD property and that module is unloaded, the 
DxeDebugPrintErrorLevelLib DESTRUCTOR function, which uninstalls the interface, 
is never called and hence the interface remains in the protocol database, 
despite it pointing to a memory location that is now free. If it is called, the 
behavior is obviously undefined.

Did I understand something incorrectly, are these modules not supposed to be 
unloadable or is this a bug?

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

Re: [edk2] Unit tests and the EDK2

[Marvin H?user]

Hey Mike,

Is the framework you plan to RFC a framework within the UEFI environment (UEFI 
Shell) or within the OS?
Using the OS implementations of UEFI (Nt32 & Emulator) to run Unit Tests 
without the need for a separate UEFI device or a reboot sounds pretty 
compelling to me, to be honest.

Thank you very much!

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Kinney, Michael D
> Sent: Sunday, November 6, 2016 8:04 PM
> To: Blibbet ; edk2-devel@lists.01.org; Kinney, Michael
> D 
> Subject: Re: [edk2] Unit tests and the EDK2
> 
> Hi,
> 
> A test framework for EDK2 is one of my highest priorities to complete before
> the end of the year.
> 
> I am evaluating a number of options and hope to put together a complete
> proposal as an RFC for consideration in the next few weeks.
> 
> Thanks,
> 
> Mike
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Blibbet
> > Sent: Sunday, November 6, 2016 9:26 AM
> > To: edk2-devel@lists.01.org
> > Subject: Re: [edk2] Unit tests and the EDK2
> >
> > On 11/06/2016 01:57 AM, Matt Lazarowitz wrote:
> > > I would like to find out if anyone has experience with an off the
> > > shelf unit test framework in the EDK2.[...]
> >
> > If you haven't looked at it, two months ago Microsoft open-sourced
> > some EDK2-centric unit tests.
> >
> > https://firmwaresecurity.com/2016/09/23/microsoft-uefi-unit-tests/
> >
> > Lee Fisher
> >
> >
> > ___
> > 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

Re: [edk2] Sec and Reset vector

[Marvin H?user]

Hey Rafael,

There actually is some generic SEC code in UefiCpuPkg you might want to take a 
look at. It's generic because it does not have "Intel NDA" code, such as CAR 
(Cache-As-RAM) etc.
The Reset Vector may or may not be part of SecCore. It's either embedded within 
the SecCore module, or a separate file in the FFS. You can check the start/end 
address of the modules (e.g. with UEFITool) and find the Reset Vector file that 
way.

PS.: Seems like inline images are not supported by the mailing list (or is it 
my error?). Either way, I do not see the image in my mail client (Outlook 2016).

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Rafael Machado
> Sent: Saturday, October 22, 2016 6:28 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Sec and Reset vector
> 
> Hi eveyrone
> 
> I'm doing some studies on edk2 and coreboot, but I'm having some questions
> that I believe you can help.
> 
> On the journey to try to understand things since the beginning, so they make
> sense in future, I'm trying to understand how does the Initial phases of UEFI
> / PI firmware work. To do that I got a bios image and start to reverse it to
> check the modules and everything present at that bios. Now I understand, at
> least the basics, about DXE and PEI phase.
> 
> The main question that I have now is about the SEC phase.
> To try to understand the SEC phase I tried to reverse this firmware so I could
> check the reset vector's first jump or something like that.
> The surprise I have is that I was not able to find this code.
> 
> To be sure I was reversing on the correct way I generated a coreboot image.
> On the image below we can see the initial code of a firmware generated
> using coreboot
> 
> [image: pasted1]
> 
> But at the UEFI firmware I'm studying I'm not able to find anything similar to
> that.
> My guess before starting this was that at least the SEC initial code should be
> similar to the legacy way of doing things, a jmp at 0xfff:fff0 and after that 
> the
> magic should get started with all uefi phases.
> 
> Could someone please give me some light on that?
> 
> 
> Thanks and Regards
> Rafael R. Machado
> ___
> 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] [MdeModulePkg][PeiCore] I seemed to have crashed the PEI Core by grabbing memory from PeiTemporaryRamBase?

[Marvin H?user]

Sorry. For some reason, I didn't get to read the paragraph about 
TemporaryRamSupportPpi and shomehow skipped to the PS.
I suppose my hint is not related to the crash then, though I hope it was still 
helpful in some way, as it seems to assume that the PPI List is in the 
temporary heap nevertheless.

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Saturday, August 13, 2016 3:47 AM
> To: edk2-devel@lists.01.org
> Cc: Andrew Fish <af...@apple.com>
> Subject: Re: [edk2] [MdeModulePkg][PeiCore] I seemed to have crashed the
> PEI Core by grabbing memory from PeiTemporaryRamBase?
> 
> Hello Andrew,
> 
> Unfortunately I cannot test anything right now and I don't have a whole lot of
> knowledge in this area, though I might have a hint for you.
> 
> While PpiList is equal to the original TempRam base, the TempRam based
> passed to PEI is equal to the original TempRam base + the size of the PpiList,
> hence PpiList is smaller than the base address passed to PEI. The PpiList is
> then installed via the PeiServicesInstallPpi () function:
> 
> call:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> PeiMain/PeiMain.c#L386
> implementation:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> Ppi/Ppi.c#L183
> 
> The list is then added to PpiData.PpiListPtrs.
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> Ppi/Ppi.c#L229
> 
> I am not sure at which point of time you are experiencing the crash, but after
> permanent memory is available, ConvertPpiPointers () is called, which then
> calls ConverSinglePpiPointer () for old heap, old stack and old hole (I'm 
> afraid
> I do not know what TempRam Hole is and if it is related).
> 
> ConvertPpiPointers () call:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> PeiMain/PeiMain.c#L237
> Old Heap ConverSinglePpiPointer () call:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> Ppi/Ppi.c#L127
> 
> The call for the old heap conversion passes the TempRam base, which has
> been incremented as we know, and thus the comparison to TempBottom will
> fail, as TempBottom is PeiTemporaryRamBase, which is larger than PpiList,
> which is one of the items in PpiListPtrs, which is the object of the 
> conversion.
> 
> comparison to TempBottom:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> Ppi/Ppi.c#L60
> 
> As the pointer to the PpiList passed by SecCore is probably not converted as
> detailed above, I assume something post-mem attempts to access this
> former PpiList by the old temporary RAM address and that somehow causes
> trouble; I assume the SEC PpiList being part of the PEI memory is an
> assumption made by the person who wrote this code. I'm not sure about
> why it crashes, as I do not know the entire PEI control flow, though I hope
> this can help you in some way.
> 
> Please excuse me if I have made a mistake in understanding the referenced
> code and wasted your time.
> 
> Regards,
> Marvin.
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Andrew Fish
> > Sent: Saturday, August 13, 2016 1:25 AM
> > To: edk2-devel <edk2-devel@lists.01.org>
> > Subject: [edk2] [MdeModulePkg][PeiCore] I seemed to have crashed the
> > PEI Core by grabbing memory from PeiTemporaryRamBase?
> >
> > I grabbed some memory between SEC and the PEI Core by adjusting
> > SecCoreData-> PeiTemporaryRamBase and SecCoreData->
> > PeiTemporaryRamSize.
> >
> > When looking at the code I don't really understand the logic of the
> algorithm?
> > So maybe I'm doing something wrong.
> >
> > This adjustment does not seem right to me?
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> > Dispatcher/Dispatcher.c#L768
> >   //
> >   // Heap Offset
> >   //
> >   BaseOfNewHeap = TopOfNewStack;
> >   if (BaseOfNewHeap >= (UINTN)SecCoreData->PeiTemporaryRamBase) {
> > Private->HeapOffsetPositive = TRUE;
> > Private->HeapOffset = (UINTN)(BaseOfNewHeap -
> > (UINTN)SecCoreData->PeiTemporaryRamBase);
> >   } else {
> > Private->HeapOffsetPositive = FALSE;
> > Private->HeapOffset = (UINTN)((UINTN)SecCoreData-
> > >PeiTemporaryRamBase - BaseOfNewHeap);
> >   }
> >
> >
> > The above code seems to be making a very strange adjustment. I noticed
> > the adjustment in my failing case was 

Re: [edk2] [MdeModulePkg/PeiCore] How is Data being NULL on entry ensured?

[Marvin H?user]

Hey Andrew,

Thank you very much! I suppose I was misguided by the ENTRY_POINT property 
being PeiCore in the build file.

Regards,
Marvin.

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Saturday, August 13, 2016 4:47 AM
> To: Marvin H?user <marvin.haeu...@outlook.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [MdeModulePkg/PeiCore] How is Data being NULL on
> entry ensured?
> 
> 
> > On Aug 12, 2016, at 7:01 PM, Marvin H?user
> <marvin.haeu...@outlook.com> wrote:
> >
> > Dear list subscribers,
> >
> > I have just been looking around the PeiCore code and wondered, how it is
> ensured, that the third ("Data") argument of the entry point is NULL on the
> first run.
> > EFI_PEI_CORE_ENTRY_POINT only has two arguments and hence most SEC
> implementations, including MdeModulePkg/SecCore, are going to pass only
> the first two arguments to the entry point.
> > I'm aware that the code works and I have never seen an occasion of it
> failing or seeming to fail because of this design, though I wondered, how is 
> it
> assured, that the third argument, which is not part of the first call, is 
> being
> NULL on entry and not some leftover on the temporary stack/in the
> argument 3 register?
> >
> 
> Marvin,
> 
> The code you are looking for is in the entry point library.
> 
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/PeiCoreE
> ntryPoint/PeiCoreEntryPoint.c#L59
> 
> **/
> VOID
> EFIAPI
> _ModuleEntryPoint(
>   IN CONST  EFI_SEC_PEI_HAND_OFF*SecCoreData,
>   IN CONST  EFI_PEI_PPI_DESCRIPTOR  *PpiList
> )
> {
>   ProcessModuleEntryPointList (SecCoreData, PpiList, NULL);
> 
>   //
>   // Should never return
>   //
>   ASSERT(FALSE);
>   CpuDeadLoop ();
> }
> 
> 
> ProcessModuleEntryPointList() call is auto-generated and it will cal the entry
> point listed in the PEI Core INF file. So that is why it is hard to grep for.
> 
> Thanks,
> 
> Andrew Fish
> 
> > Thank you for your time!
> >
> > Regards,
> > Marvin.
> > ___
> > 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] [MdeModulePkg/PeiCore] How is Data being NULL on entry ensured?

[Marvin H?user]

Dear list subscribers,

I have just been looking around the PeiCore code and wondered, how it is 
ensured, that the third ("Data") argument of the entry point is NULL on the 
first run.
EFI_PEI_CORE_ENTRY_POINT only has two arguments and hence most SEC 
implementations, including MdeModulePkg/SecCore, are going to pass only the 
first two arguments to the entry point.
I'm aware that the code works and I have never seen an occasion of it failing 
or seeming to fail because of this design, though I wondered, how is it 
assured, that the third argument, which is not part of the first call, is being 
NULL on entry and not some leftover on the temporary stack/in the argument 3 
register?

Thank you for your time!

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

Re: [edk2] [MdeModulePkg][PeiCore] I seemed to have crashed the PEI Core by grabbing memory from PeiTemporaryRamBase?

[Marvin H?user]

Hello Andrew,

Unfortunately I cannot test anything right now and I don't have a whole lot of 
knowledge in this area, though I might have a hint for you.

While PpiList is equal to the original TempRam base, the TempRam based passed 
to PEI is equal to the original TempRam base + the size of the PpiList, hence 
PpiList is smaller than the base address passed to PEI. The PpiList is then 
installed via the PeiServicesInstallPpi () function:

call: 
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#L386
implementation: 
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Ppi/Ppi.c#L183

The list is then added to PpiData.PpiListPtrs.

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Ppi/Ppi.c#L229

I am not sure at which point of time you are experiencing the crash, but after 
permanent memory is available, ConvertPpiPointers () is called, which then 
calls ConverSinglePpiPointer () for old heap, old stack and old hole (I'm 
afraid I do not know what TempRam Hole is and if it is related).

ConvertPpiPointers () call: 
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#L237
Old Heap ConverSinglePpiPointer () call: 
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Ppi/Ppi.c#L127

The call for the old heap conversion passes the TempRam base, which has been 
incremented as we know, and thus the comparison to TempBottom will fail, as 
TempBottom is PeiTemporaryRamBase, which is larger than PpiList, which is one 
of the items in PpiListPtrs, which is the object of the conversion.

comparison to TempBottom: 
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Ppi/Ppi.c#L60

As the pointer to the PpiList passed by SecCore is probably not converted as 
detailed above, I assume something post-mem attempts to access this former 
PpiList by the old temporary RAM address and that somehow causes trouble; I 
assume the SEC PpiList being part of the PEI memory is an assumption made by 
the person who wrote this code. I'm not sure about why it crashes, as I do not 
know the entire PEI control flow, though I hope this can help you in some way.

Please excuse me if I have made a mistake in understanding the referenced code 
and wasted your time.

Regards,
Marvin.


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Andrew Fish
> Sent: Saturday, August 13, 2016 1:25 AM
> To: edk2-devel 
> Subject: [edk2] [MdeModulePkg][PeiCore] I seemed to have crashed the PEI
> Core by grabbing memory from PeiTemporaryRamBase?
> 
> I grabbed some memory between SEC and the PEI Core by adjusting
> SecCoreData-> PeiTemporaryRamBase and SecCoreData->
> PeiTemporaryRamSize.
> 
> When looking at the code I don't really understand the logic of the algorithm?
> So maybe I'm doing something wrong.
> 
> This adjustment does not seem right to me?
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> Dispatcher/Dispatcher.c#L768
>   //
>   // Heap Offset
>   //
>   BaseOfNewHeap = TopOfNewStack;
>   if (BaseOfNewHeap >= (UINTN)SecCoreData->PeiTemporaryRamBase) {
> Private->HeapOffsetPositive = TRUE;
> Private->HeapOffset = (UINTN)(BaseOfNewHeap -
> (UINTN)SecCoreData->PeiTemporaryRamBase);
>   } else {
> Private->HeapOffsetPositive = FALSE;
> Private->HeapOffset = (UINTN)((UINTN)SecCoreData-
> >PeiTemporaryRamBase - BaseOfNewHeap);
>   }
> 
> 
> The above code seems to be making a very strange adjustment. I noticed the
> adjustment in my failing case was off by 0xC0 which is the amount of
> memory I carved out prior to entering the PEI Core.
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/
> Dispatcher/Dispatcher.c#L796
> 
>   //
>   // Temporary Ram Support PPI is provided by platform, it will copy
>   // temporary memory to permenent memory and do stack switching.
>   // After invoking Temporary Ram Support PPI, the following code's
>   // stack is in permanent memory.
>   //
>   TemporaryRamSupportPpi->TemporaryRamMigration (
> PeiServices,
> TemporaryRamBase,
> (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack -
> TemporaryStackSize),
> TemporaryRamSize
> );
> 
> 
> And this is also a case in which the stack got bigger. But it seems to me the
> shift if really defined by TemporaryRamBase, TopOfNewStack, and
> TemporaryStackSize in this case.
> 
> The failure I hit was OldCoreData->Fv pointer was shifted so when the PPI
> was called the system crashed. Is this a bug in the
> gEfiTemporaryRamSupportPpiGuid path?
> 
> If I changed the HeadOffset algorithm my crash went away? Private-
> >HeapOffset = ((UINTN)TopOfNewStack - TemporaryStackSize) -
> 

Re: [edk2] [RFC 1/2] MdeModulePkg/EbcDxe: Add AARCH64 EBC VM support

[Marvin H?user]

Hey Feng,

According to the Contributions document, BSD 2-clause is one of the licenses 
that can be accepted, if the parent license cannot be used:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Contributions.txt#L25

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Tian, Feng
> Sent: Monday, August 1, 2016 3:50 AM
> To: Leif Lindholm ; edk2-devel@lists.01.org
> Cc: Tian, Feng ; Daniil Egranov
> ; Jeff Brasen ; Zeng,
> Star ; Ard Biesheuvel 
> Subject: Re: [edk2] [RFC 1/2] MdeModulePkg/EbcDxe: Add AARCH64 EBC
> VM support
> 
> Hi, Leif
> 
> If I understand correctly, EDKII doesn't allow such BSD license.
> 
> Correct me if anybody has different opinions.
> 
> Thanks
> Feng
> 
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Saturday, July 30, 2016 12:06 AM
> To: edk2-devel@lists.01.org
> Cc: Jeff Brasen ; Ard Biesheuvel
> ; Daniil Egranov ;
> Tian, Feng ; Zeng, Star 
> Subject: [RFC 1/2] MdeModulePkg/EbcDxe: Add AARCH64 EBC VM support
> 
> From: Jeff Brasen 
> 
> Adds support for the EBC VM for AARCH64 platforms
> 
> Submitted on behalf of a third-party: The Linux Foundation This contribution
> is licensed under the BSD license as found at
> http://opensource.org/licenses/bsd-license.php
> 
> [Taken from https://source.codeaurora.org/external/server/edk2-blue/]
> Signed-off-by: Leif Lindholm 
> ---
>  .../Universal/EbcDxe/AArch64/EbcLowLevel.S | 135 +
>  MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c | 563
> +
>  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf   |   6 +-
>  3 files changed, 703 insertions(+), 1 deletion(-)  create mode 100644
> MdeModulePkg/Universal/EbcDxe/AArch64/EbcLowLevel.S
>  create mode 100644
> MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
> 
> diff --git a/MdeModulePkg/Universal/EbcDxe/AArch64/EbcLowLevel.S
> b/MdeModulePkg/Universal/EbcDxe/AArch64/EbcLowLevel.S
> new file mode 100644
> index 000..e858227
> --- /dev/null
> +++ b/MdeModulePkg/Universal/EbcDxe/AArch64/EbcLowLevel.S
> @@ -0,0 +1,135 @@
> +#/** @file
> +#
> +#This code provides low level routines that support the Virtual Machine
> +#   for option ROMs.
> +#
> +#  Copyright (c) 2015, The Linux Foundation. All rights reserved.
> +#  Copyright (c) 2007 - 2014, Intel Corporation. All rights
> +reserved. #  This program and the accompanying materials #  are
> +licensed and made available under the terms and conditions of the BSD
> +License #  which accompanies this distribution.  The full text of the
> +license may be found at #
> +http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> +BASIS, #  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> +#
> +#**/
> +
> +#--
> +-
> +# Equate files needed.
> +#--
> +-
> +
> +ASM_GLOBAL ASM_PFX(CopyMem);
> +ASM_GLOBAL ASM_PFX(EbcInterpret);
> +ASM_GLOBAL ASM_PFX(ExecuteEbcImageEntryPoint);
> +
> +#
> **
> +**
> +# EbcLLCALLEX
> +#
> +# This function is called to execute an EBC CALLEX instruction.
> +# This instruction requires that we thunk out to external native #
> +code. For AArch64, we copy the VM stack into the main stack and then
> +pop # the first 8 arguments off according to the AArch64 Procedure Call
> +Standard # On return, we restore the stack pointer to its original location.
> +#
> +#
> **
> +** # UINTN EbcLLCALLEXNative(UINTN FuncAddr, UINTN
> NewStackPointer,
> +VOID *FramePtr) ASM_GLOBAL ASM_PFX(EbcLLCALLEXNative);
> +ASM_PFX(EbcLLCALLEXNative):
> +  stp  x19, x20, [sp, #-16]!
> +  stp  x29, x30, [sp, #-16]!
> +
> +  mov  x19, x0
> +  mov  x20, sp
> +  sub  x2, x2, x1   // Length = NewStackPointer-FramePtr
> +  sub  sp, sp, x2
> +  sub  sp, sp, #64  // Make sure there is room for at least 8 args in 
> the new
> stack
> +  mov  x0, sp
> +
> +  bl   CopyMem  // Sp, NewStackPointer, Length
> +
> +  ldp  x0, x1, [sp], #16
> +  ldp  x2, x3, [sp], #16
> +  ldp  x4, x5, [sp], #16
> +  ldp  x6, x7, [sp], #16
> +
> +  blr  x19
> +
> +  mov  sp,  x20
> +  ldp  x29, x30, [sp], #16
> +  ldp  x19, x20, [sp], #16
> +
> +  ret
> +
> +#
> **
> +**
> +# EbcLLEbcInterpret

[edk2] EFI PCI Root Bridge I/O and HII Image Decoder Protocol GUIDs

[Marvin H?user]

Dear community members,

I have just been searching MdePkg for a GUID unknown to me and - to my surprise 
- I found the same GUID twice in the code base: 
EFI_HII_IMAGE_DECODER_PROTOCOL_GUID and EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_GUID. 
As former was added only recently, I went to check the UEFI 2.6 specification 
and discovered that even there the GUIDs match.

Now, please excuse my ignorance, but was the PCI Root Bridge I/O Protocol 
deprecated and the Image Decoder was assigned its GUID, or might this be a 
copy+paste error?
Also, in the specification, the Image Decore GUID has the second opening curly 
brace one byte too late, I suppose.

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

Re: [edk2] Firmware Management Protocol: CHAR16* fields in EFI_FIRMWARE_IMAGE_DESCRIPTOR

[Marvin H?user]

Hey Andrew,

I have missed that it's not about the protocol directly, but about a returned 
data structure, so please excuse me!

Thanks,
Marvin.

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Thursday, July 7, 2016 10:33 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>
> Cc: edk2-devel@lists.01.org; Bruce Cran <br...@cran.org.uk>
> Subject: Re: [edk2] Firmware Management Protocol: CHAR16* fields in
> EFI_FIRMWARE_IMAGE_DESCRIPTOR
> 
> 
> > On Jul 7, 2016, at 1:27 PM, Marvin H?user <marvin.haeu...@outlook.com>
> wrote:
> >
> > Hey Bruce,
> >
> > Sorry if I am wasting your time, but where exactly is the problem?
> >
> > I don't think these strings can ever be statically allocated. If pointers to
> stack variables were used, the strings would be theoretically invalid the
> moment the exposing function returns. Furthermore they can't be part oft
> he struct itself as that would change the offsets of all other members after
> the string. Hence I think it can be concluded they are always dynamically
> allocated.
> > And as one can be sure they are dynamically allocated, I think it's the task
> of the function that uninstalls the protocol to free the two buffers. Should
> the protocol not be uninstalled till ExitBootServices(), it doesn't matter 
> either
> because the OS loader may treat Boot Services code and data as free space.
> >
> > Did I miss something?
> >
> 
> Marvin,
> 
> I think your logic is sound in general. A protocol's structure could be a 
> global
> or malloc'ed and as long as the protocol is installed that data exists. You 
> only
> need to clean up that data if the protocol is uninstalled from the handle.
> Bruce is asking about a protocol member function that returns a data
> structure into a caller allocated buffer (hopefully I got that right) that 
> contains
> pointers to other items. So conceptually it could be a different malloc'ed
> buffer leaked on every call? So I think it is a less clear cut case.
> 
> Thanks,
> 
> Andrew Fish
> 
> > Regards,
> > Marvin.
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >> Of Bruce Cran
> >> Sent: Thursday, July 7, 2016 9:36 PM
> >> To: edk2-devel@lists.01.org
> >> Subject: [edk2] Firmware Management Protocol: CHAR16* fields in
> >> EFI_FIRMWARE_IMAGE_DESCRIPTOR
> >>
> >> Before I go hassling the USWG, I thought I'd check here in case
> >> anyone knows.
> >>
> >> In the Firmware Management Protocol introduced in UEFI 2.3, there's a
> >> EFI_FIRMWARE_IMAGE_DESCRIPTOR structure that has two CHAR16*
> fields,
> >> 'ImageIdName' and 'VersionName'. The UEFI spec doesn't say anything
> >> about memory allocation, but they're strings that at least in our
> >> driver are dynamically allocated.
> >>
> >> Since there's no FMP 'cleanup' function, I'm wondering if there was
> >> some sort of mistake specifying those, because I don't see a way to
> >> avoid leaking memory.
> >>
> >> --
> >> Bruce Cran
> >> ___
> >> 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

Re: [edk2] Firmware Management Protocol: CHAR16* fields in EFI_FIRMWARE_IMAGE_DESCRIPTOR

[Marvin H?user]

Hey Bruce,

Sorry if I am wasting your time, but where exactly is the problem?

I don't think these strings can ever be statically allocated. If pointers to 
stack variables were used, the strings would be theoretically invalid the 
moment the exposing function returns. Furthermore they can't be part oft he 
struct itself as that would change the offsets of all other members after the 
string. Hence I think it can be concluded they are always dynamically allocated.
And as one can be sure they are dynamically allocated, I think it's the task of 
the function that uninstalls the protocol to free the two buffers. Should the 
protocol not be uninstalled till ExitBootServices(), it doesn't matter either 
because the OS loader may treat Boot Services code and data as free space.

Did I miss something?

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Bruce Cran
> Sent: Thursday, July 7, 2016 9:36 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Firmware Management Protocol: CHAR16* fields in
> EFI_FIRMWARE_IMAGE_DESCRIPTOR
> 
> Before I go hassling the USWG, I thought I'd check here in case anyone
> knows.
> 
> In the Firmware Management Protocol introduced in UEFI 2.3, there's a
> EFI_FIRMWARE_IMAGE_DESCRIPTOR structure that has two CHAR16* fields,
> 'ImageIdName' and 'VersionName'. The UEFI spec doesn't say anything
> about memory allocation, but they're strings that at least in our driver are
> dynamically allocated.
> 
> Since there's no FMP 'cleanup' function, I'm wondering if there was some
> sort of mistake specifying those, because I don't see a way to avoid leaking
> memory.
> 
> --
> Bruce Cran
> ___
> 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 v1 1/2] MdeModulePkg: Minimize usage of FreePool() during ExitBS().

[Marvin H?user]

Hey all,

IScsiMisc and Snp (latter not part of V1) are the only code files I found using 
these services during the event (within MdeModulePkg). A patch for NetworkPkg's 
IScsiMisc has already been submitted and it still open for review from my side.
If anyone has found any other example, feel free to make a patch of your own 
including the ones mentioned above, or - if you prefer not to - please reply 
mentioning them so I can include them in a V2 late this or maybe next week.

Regards,
Marvin.

> -Original Message-
> From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> Sent: Wednesday, June 22, 2016 9:55 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com>
> Subject: RE: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> FreePool() during ExitBS().
> 
> Marvin,
> 
> Yes.  A V2 version of the patch that eliminates memory alloc/free actions in
> exit boot services event notification functions looks like a very good idea to
> me.
> 
> Mike
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marvin H?user
> > Sent: Wednesday, June 22, 2016 12:32 PM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> > Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> > FreePool() during ExitBS().
> >
> > Hey Mike,
> >
> > Yes, I'm aware of that and it has been the very reason for this patch.
> > Despite doing the same for SMM memory, which is nonsense (I just
> > didn't think about it being separate, sorry again), I still would like
> > to have feedback of the changes to IScsiMisc.c - there it's the same
> > situation, but with non-SMM memory, so memory that does alter the UEFI
> Memory Map.
> >
> > Regards,
> > Marvin.
> >
> > > -Original Message-
> > > From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> > > Sent: Wednesday, June 22, 2016 9:10 PM
> > > To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> > > de...@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com>
> > > Cc: Zeng, Star <star.z...@intel.com>
> > > Subject: RE: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> > > FreePool() during ExitBS().
> > >
> > > Marvin,
> > >
> > > If none of the exit boot services event handlers in a platform do
> > > any memory allocation/free actions, then only a single call to
> > > GetMemoryMap() is required because the memory map key will be the
> same.
> > >
> > > As Andrew pointed out, these event notifications should not do
> > > alloc/free, so if there are exit boot services notification
> > > functions that are doing alloc/free then those should be cleaned up.
> > >
> > > Are there any specific code change requests here?
> > >
> > > Mike
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On
> > > > Behalf Of Marvin H?user
> > > > Sent: Wednesday, June 22, 2016 4:27 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Zeng, Star <star.z...@intel.com>
> > > > Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> > > > FreePool() during ExitBS().
> > > >
> > > > Hey Star,
> > > >
> > > > > I am still curious what is the real issue Marvin met.
> > > >
> > > > There is no issue I met, I simply want to save another call to
> > > > GetMemoryMap() because it may be altered during ExitBS(). It's a
> > > > break of the specification, but it sounds like Intel complying to
> > > > its own
> > > sepcification is not a good-enough reason...
> > > >
> > > > Regards,
> > > > Marvin.
> > > >
> > > > > -Original Message-
> > > > > From: Zeng, Star [mailto:star.z...@intel.com]
> > > > > Sent: Wednesday, June 22, 2016 11:18 AM
> > > > > To: Bruce Cran <br...@cran.org.uk>; marvin.haeu...@outlook.com;
> > > > > edk2- de...@lists.01.org
> > > > > Cc: Tian, Feng <feng.t...@intel.com>
> > > > > Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage
> > > > > of
> > > > > FreePool() during ExitBS().
> > > > >
> > > > > On 2016/6/22 13:39, Bruce Cran wrote

[edk2] StdLib usage for drivers?

[Marvin H?user]

Dear EDK2 developers,

For an experimental project, I'm currently attempting to write a library 
wrapper for the disassembler library 'Capstone' in a similar manner to 
CryptoPkg's OpensslLib. As most C libraries, it also depends on the standard 
headers, which are not provided by 'stock' EDK2. My first guess has been to use 
StdLib, though its description states:
'Due to the execution environment built by the StdLib component, execution as a 
UEFI driver can cause system stability issues.'

Inspecting OpensslLib I discovered that CryptoPkg deploys its own include files 
for StdLib. Though, from your experience, what are the issues with using 
StdLibPkg with DXE/UEFI drivers? It might be nice to reduce duplicate code, 
though I honestly don't know anything about StdLibPkg and its implementation 
and would be thankful for some insight on that manner.

Thank you in advance for your time!

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

Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of FreePool() during ExitBS().

[Marvin H?user]

Hey Mike,

Yes, I'm aware of that and it has been the very reason for this patch. Despite 
doing the same for SMM memory, which is nonsense (I just didn't think about it 
being separate, sorry again), I still would like to have feedback of the 
changes to IScsiMisc.c - there it's the same situation, but with non-SMM 
memory, so memory that does alter the UEFI Memory Map.

Regards,
Marvin.

> -Original Message-
> From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> Sent: Wednesday, June 22, 2016 9:10 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> FreePool() during ExitBS().
> 
> Marvin,
> 
> If none of the exit boot services event handlers in a platform do any memory
> allocation/free actions, then only a single call to GetMemoryMap() is
> required because the memory map key will be the same.
> 
> As Andrew pointed out, these event notifications should not do alloc/free,
> so if there are exit boot services notification functions that are doing
> alloc/free then those should be cleaned up.
> 
> Are there any specific code change requests here?
> 
> Mike
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marvin H?user
> > Sent: Wednesday, June 22, 2016 4:27 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>
> > Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> > FreePool() during ExitBS().
> >
> > Hey Star,
> >
> > > I am still curious what is the real issue Marvin met.
> >
> > There is no issue I met, I simply want to save another call to
> > GetMemoryMap() because it may be altered during ExitBS(). It's a break
> > of the specification, but it sounds like Intel complying to its own
> sepcification is not a good-enough reason...
> >
> > Regards,
> > Marvin.
> >
> > > -Original Message-
> > > From: Zeng, Star [mailto:star.z...@intel.com]
> > > Sent: Wednesday, June 22, 2016 11:18 AM
> > > To: Bruce Cran <br...@cran.org.uk>; marvin.haeu...@outlook.com;
> > > edk2- de...@lists.01.org
> > > Cc: Tian, Feng <feng.t...@intel.com>
> > > Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> > > FreePool() during ExitBS().
> > >
> > > On 2016/6/22 13:39, Bruce Cran wrote:
> > > > On 6/19/16 9:21 PM, Zeng, Star wrote:
> > > >
> > > >> 1. The memory allocate and free in PiSmmCore are SMRAM that is
> > > >> not related to UEFI memory map.
> > > >> 2. According to UEFI 2.6 spec page 222 below, the UEFI OS loader
> > > >> should have got the memory map before ExitBootServices.
> > > >> That means the memory free should not impact the memory map
> *got
> > > >> by UEFI OS loader*, it will only impact the memory map if the
> > > >> UEFI OS loader will call GetMemoryMap() again.
> > > >
> > > >
> > > > I just found the following post about Linux and how it calls
> > > > ExitBootServices, which may be relevant:
> > > >
> > > > http://linux-kernel.2935.n7.nabble.com/PATCH-x86-efi-retry-ExitBoo
> > > > tSer
> > > > vices-on-failure-td664938.html
> > > >
> > > >
> > > > "ExitBootServices is absolutely supposed to return a failure if
> > > > any ExitBootServices event handler changes the memory map.
> > > > Basically the get_map loop should run again if ExitBootServices
> > > > returns an error the first time.  I would say it would be fair
> > > > that if ExitBootServices gives an error the second time then Linux
> > > > would be fine in returning control back to BIOS."
> > > >
> > >
> > > Good information that does not assume ExitBootServices event
> > > handlers are not using memory allocation services.
> > > In reality, the CoreExitBootServices() in DxeMain.c of EDK2 will
> > > never return error after calling CoreNotifySignalList
> ().
> > >
> > > I am still curious what is the real issue Marvin met.
> > >
> > > If we are going to clean up, we may need to review all the
> > > ExitBootServices event handlers to see if they are using memory
> > > allocation services directly or indirectly and find the method to clean up
> them.
> > >
> > > Cc more related people.
> > >
> > > Thanks,
> > > Star
> > ___
> > 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 v1 1/2] MdeModulePkg: Minimize usage of FreePool() during ExitBS().

[Marvin H?user]

Hey Star,

> I am still curious what is the real issue Marvin met.

There is no issue I met, I simply want to save another call to GetMemoryMap() 
because it may be altered during ExitBS(). It's a break of the specification, 
but it sounds like Intel complying to its own sepcification is not a 
good-enough reason...

Regards,
Marvin.

> -Original Message-
> From: Zeng, Star [mailto:star.z...@intel.com]
> Sent: Wednesday, June 22, 2016 11:18 AM
> To: Bruce Cran ; marvin.haeu...@outlook.com; edk2-
> de...@lists.01.org
> Cc: Tian, Feng 
> Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> FreePool() during ExitBS().
> 
> On 2016/6/22 13:39, Bruce Cran wrote:
> > On 6/19/16 9:21 PM, Zeng, Star wrote:
> >
> >> 1. The memory allocate and free in PiSmmCore are SMRAM that is not
> >> related to UEFI memory map.
> >> 2. According to UEFI 2.6 spec page 222 below, the UEFI OS loader
> >> should have got the memory map before ExitBootServices.
> >> That means the memory free should not impact the memory map *got by
> >> UEFI OS loader*, it will only impact the memory map if the UEFI OS
> >> loader will call GetMemoryMap() again.
> >
> >
> > I just found the following post about Linux and how it calls
> > ExitBootServices, which may be relevant:
> >
> > http://linux-kernel.2935.n7.nabble.com/PATCH-x86-efi-retry-ExitBootSer
> > vices-on-failure-td664938.html
> >
> >
> > "ExitBootServices is absolutely supposed to return a failure if any
> > ExitBootServices event handler changes the memory map.  Basically the
> > get_map loop should run again if ExitBootServices returns an error the
> > first time.  I would say it would be fair that if ExitBootServices
> > gives an error the second time then Linux would be fine in returning
> > control back to BIOS."
> >
> 
> Good information that does not assume ExitBootServices event handlers are
> not using memory allocation services.
> In reality, the CoreExitBootServices() in DxeMain.c of EDK2 will never return
> error after calling CoreNotifySignalList ().
> 
> I am still curious what is the real issue Marvin met.
> 
> If we are going to clean up, we may need to review all the ExitBootServices
> event handlers to see if they are using memory allocation services directly or
> indirectly and find the method to clean up them.
> 
> Cc more related people.
> 
> Thanks,
> Star
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of FreePool() during ExitBS().

[Marvin H?user]

Hey Bruce,

I think the exact behavior is not defined, though I remember reading (I'm not 
sure if from the specification) that the OS loader should only keep calling 
GetMemoryMap() and ExitBootServices() after the first ExitBootServices() call; 
this would apply to drivers hooking the event as well of course.
Though I think the event was specifically designed to give drivers a chance to 
clean up and I suppose every UEFI implementer will make sure Boot Services are 
not terminated till all events have been trigered. But I might just need to 
re-read that part of the specification.

The primary reason I submited this patch was that the ExitBootServices() is 
more likely to succeed at first, saving the OS loader a call to GetMemoryMap().

Thanks,
Marvin.

> -Original Message-
> From: Bruce Cran [mailto:br...@cran.org.uk]
> Sent: Monday, June 20, 2016 6:11 PM
> To: Zeng, Star ; marvin.haeu...@outlook.com; edk2-
> de...@lists.01.org
> Cc: Tian, Feng 
> Subject: Re: [edk2] [PATCH v1 1/2] MdeModulePkg: Minimize usage of
> FreePool() during ExitBS().
> 
> On 6/19/16 9:21 PM, Zeng, Star wrote:
> 
> > 2. According to UEFI 2.6 spec page 222 below, the UEFI OS loader should
> have got the memory map before ExitBootServices.
> > That means the memory free should not impact the memory map *got by
> UEFI OS loader*, it will only impact the memory map if the UEFI OS loader will
> call GetMemoryMap() again.
> 
> But aren't boot services supposed to be unavailable during ExitBootServices?
> So regardless of whether the memory map should or should not be changed,
> it's possible that gBS is NULL and so it's not possible to call gBS->FreePool?
> 
> --
> Bruce
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] ASSERT and static code analysis

[Marvin H?user]

Hey Andrew,

Thanks for your reply!

In case you haven't read Laszlo's reply, he pointed out the same issue and 
proposed that both CpuDeadLoop() and Breakpoint() should be flagged as ' 
analyzer_noreturn', which is ignored by the compiler and thus should not break 
anyone's code, but only suppress the analyzer warnings. :)

Laszlo,

If I understood it correctly, the Clang Static Analyzer is separate from main 
Clang (compiler) and, either way, can be used standalone. For that reason, 
tools_def should probably be avoided as this requires BaseTools invoking the 
analyzer. Instead, one could check for the analyzer define (Clang Static 
Analyzer can be detected via ifdef) and define ANALYZER_NORETURN either as 
empty, when there is no analyzer detected, or as the analyzer-noreturn 
attribute, when a supported analyzer is found. I can get a patch ready by 
tomorrow, I think.

Regards,
Marvin.

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Wednesday, June 1, 2016 10:56 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] ASSERT and static code analysis
> 
> 
> > On Jun 1, 2016, at 1:12 PM, Marvin H?user
> <marvin.haeu...@outlook.com> wrote:
> >
> > Recently I was told that ASSERT() calls to check whether a variable is
> > NULL breaks the Clang Static Analyzer in terms of generating wrong
> > warnings. The reason is that, when a variable/parameter is checked for
> > NULL, this analyzer assumes that it can be. As it doesn't support EDK2
> > ASSERTs, but only compiler-provided asserts, to it, the ASSERT() call
> > is a simple if-check (-> triggers NULL warnings) which does return to
> > normal code flow (-> any further usages may be dereferencing NULL).
> > This behavior is documented here:
> > http://clang-analyzer.llvm.org/faq.html#null_pointer
> >
> > To make clear that EDK2 ASSERT() calls are indeed asserts, in my opinion,
> CpuDeadLoop() should be flagged as 'noreturn' (it indeed should never
> return) and Breakpoint() as 'analyzer_noreturn' (it may return, but the
> analyzer doesn't have to care as the debugger is invoked). If I didn't
> understand the documentation incorrectly, this should fix the issue
> described in the first paragraph.
> >
> 
> Marvin,
> 
> Sometimes people use CpuDeadLoop() to debug with a JTAG debugger so
> they will step over the code. So you can't use noreturn as that tells the
> optimizer it can remove the code following the no return function. So for
> example your entire program could get optimized away if you place a
> CpuBreakpoint() at the start of your function.
> Simple clang example:
> ~/work/Compiler>cat noreturn.c
> 
> int NoReturn(void) __attribute__ ((noreturn));
> 
> int
> main()
> {
>   NoReturn();
>   return 0;
> }
> ~/work/Compiler>clang -Os -S  noreturn.c ~/work/Compiler>cat noreturn.S
>   .section__TEXT,__text,regular,pure_instructions
>   .macosx_version_min 10, 11
>   .globl  _main
> _main:  ## @main
>   pushq   %rbp
>   movq%rsp, %rbp
>   callq   _NoReturn
> 
> 
> .subsections_via_symbols
> 
> 
> Depending on how much Heisenberg uncertainty you can stand
> 
> You can -D MDEPKG_NDEBUG for your analyzer run.
> https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/D
> ebugLib.h#L288
> 
> In the DSC map the DebugLib library class to
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseDeb
> ugLibNull/BaseDebugLibNull.inf all the functions are empty and you would
> avoid your issues.
> 
> PcdDebugPropertyMask, set in DSC, can control what happens after an
> ASSERT(), but I'm guessing that is to far into the optimizer to be useful for
> you? If you compiled with clang LTO the CpuBreakpoint() and CpuDeadLoop()
> would get dead stripped.
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseDeb
> ugLibSerialPort/DebugLib.c#L146
> 
> Thanks,
> 
> Andrew Fish
> 
> > If you have experience with the Clang Static Analyzer or even this specific
> issue, I would be happy if you would share your opinion of the concern.
> >
> > Regards,
> > Marvin.
> > ___
> > 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] ASSERT and static code analysis

[Marvin H?user]

Hey Laszlo,

Thank you very much for your comment!

About the comment in CpuDeadLoop(), once more, I wasn't aware, thanks for the 
hint!
Do you think a define should be introduced ('NORETURN'/'ANALYZE_NORETURN') to 
keep the code style consistent and also allow support for more static analyzers 
in the future?

If it indeed makes sense, I can get a patch ready, maybe today or tomorrow.

Regards,
Marvin.

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, June 1, 2016 10:38 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>
> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
> Subject: Re: [edk2] ASSERT and static code analysis
> 
> On 06/01/16 22:12, Marvin H?user wrote:
> > Recently I was told that ASSERT() calls to check whether a variable is
> > NULL breaks the Clang Static Analyzer in terms of generating wrong
> > warnings. The reason is that, when a variable/parameter is checked for
> > NULL, this analyzer assumes that it can be. As it doesn't support
> > EDK2 ASSERTs, but only compiler-provided asserts, to it, the ASSERT()
> > call is a simple if-check (-> triggers NULL warnings) which does
> > return to normal code flow (-> any further usages may be dereferencing
> > NULL). This behavior is documented here:
> > http://clang-analyzer.llvm.org/faq.html#null_pointer
> 
> Makes sense to me.
> 
> > To make clear that EDK2 ASSERT() calls are indeed asserts, in my
> > opinion, CpuDeadLoop() should be flagged as 'noreturn' (it indeed
> > should never return) and Breakpoint() as 'analyzer_noreturn' (it may
> > return, but the analyzer doesn't have to care as the debugger is
> > invoked).
> 
> Side point: CpuDeadLoop() too should be flagged as "analyzer_noreturn"
> then; please see the comments in
> "MdePkg/Library/BaseLib/CpuDeadLoop.c".
> 
> Thanks
> Laszlo
> 
> > If I didn't understand the documentation incorrectly, this should fix
> > the issue described in the first paragraph.
> >
> > If you have experience with the Clang Static Analyzer or even this
> > specific issue, I would be happy if you would share your opinion of
> > the concern.

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

Re: [edk2] [RFC V2] Proposal to organize packages into directories

[Marvin H?user]

Hey Mike,

Thank you very much for your effort of introducing a new directory structure, 
it looks very nice so far!

Though, may I ask why IntelFspPkg and IntelFspWrapperPkg are now deprecated?
If 'deprecated' does not mean that the packages are likely to be removed, but 
just means, if possible, developers should stop using it, then you can skip 
this paragraph.
I don't know what Intel is sharing with its partners as I am none, but on the 
public site, even the Broadwell FSP is still v1.0 and there are only a few 
following the v1.1 specification. To me, "deprecated" sounds like the package 
will eventually be removed, but that maybe wouldn't be a good idea for these 
as, publicly, there isn't even a single FSP 2.0 release. Also, the 
"Compatibility" part of EdkCompatibilityPkg shouldn't be deprecated either, in 
my opinion, as it is a nice resource for easy-to-use backwards-compatibility.

Furthermore, I recall someone already brought up that 'CorebootModulePkg' 
should probably not be part of the 'Core' directory as it isn't 'Platform 
agnostic'. Didn't you actually list them as part of 'Platform' in a reply to 
your V1 concept, as well as IntelFsp2Pkg and IntelFsp2WrapperPkg? The FSP 
packages should probably go in the same directory as UefiCpuPkg, because to me 
UefiCpuPkg seems to be just an Open Source part of FSP, isn't it that way? 
These three should, in my opinion, go to the Intel directory, as UefiCpuPkg 
only supports Intel code for now anyway... except if you are planning to merge 
ARM and other architecture's code into UefiCpuPkg in the future.

Thank you for your time!

Regards,
Marvin.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Kinney, Michael D
> Sent: Thursday, May 26, 2016 4:04 AM
> To: edk2-devel@lists.01.org; Kinney, Michael D
> 
> Subject: [edk2] [RFC V2] Proposal to organize packages into directories
> 
> Hello,
> 
> I have gone through all the feedback I have received and have updated this
> proposal.  Here is a summary of the changes in V2:
> 
> * IntelFrameworkModulePkg   -> Deprecated
> * IntelFrameworkPkg -> Deprecated
> * IntelFspPkg   -> Deprecated
> * IntelFspWrapperPkg-> Deprecated
> * PerformancePkg-> Deprecated
> * CorebootPayloadPkg-> Platform
> * EmbeddedPkg   -> Driver
> * ArmPlatformPkg/ArmJunoPkg -> Silicon/Arm/ArmJunoPkg
> * ArmPlatformPkg/ArmVExpressPkg -> Silicon/Arm/ ArmVExpressPkg
> * Change Drivers to Driver so no top level directories are plural.
> * Remove Vendor directory from Silicon and Platform to reduce directory
> depth
> * Add Platform/Common directory for non-vendor specific platform
> packages
> * Add Silicon/Common directory for non-vendor specific packages of
>   CPU/Chipset/SoC drivers
> * Keep Vendor directory in Driver.
>   Non-vendor specific packages of drivers are flat just below Driver.
>   Provides area to migrate non-vendor specific drivers from Core over time
> 
> Please let me know if I missed any feedback and if there is new feedback on
> this revised proposal for the directory structure or the directory names.
> 
> 
> 
> # EDK II - Proposal to organize packages into directories
> 
> There have been some discussions about organizing packages into
> directories.
> Below is a proposal for a top level directory structure and a first pass 
> mapping
> of the packages from edk2/master.  Where applicable, vendor specific
> directories can be added.
> 
> The PACKAGES_PATH feature documented in the link below is used to
> support this proposed directory structure with no source file changes.  An
> example of setting PACKAGES_PATH in a windows environment is also
> shown below and I have verified that platforms can be built using this
> proposal.
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Multiple_Workspace
> 
> Please provide feedback on the proposal (for, against, alternate proposal),
> the number/type of top level directories, and the top level directory names.
> 
> I have setup a branch with this directory structure in this proposal to help
> with the review.  I have verified that I can build some platforms in this 
> branch
> using the PACKAGES_PATH settings shown below.
> 
> https://github.com/mdkinney/edk2/tree/NewDirStruct
> 
> 
> # Top Level Directory Structure (Listed Alphabetically) ```
> edk2
>   Application   Applications and application support libraries
>   BaseTools EDK II build tools/scripts
>   Conf  EDK II build configuration files
>   Core  Platform agnostic packages for core FW services
>   DeprecatedPackages that will be removed from edk2/master soon
>   DriverEDK II Drivers (no platform assumptions)
>   Non-Vendor specific EDK II drivers
>   Non-Vendor specific EDK II drivers
> . . .
> Vendor  Vendor specific EDK II drivers
>   
>