Re: [edk2] EmulatorPkg Unix Host Segmentation fault.

[Andrew Fish via edk2-devel]

> On Feb 15, 2019, at 11:40 PM, Ni, Ray  > wrote:
> 
> I also met this issue.
> I found three solutions:
> 1. Forcing PeiMain CC flag to "-O0" works.
> 2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also works.
> 3. Implement the temporary migration routine as below in EmulatorPkg/Sec 
> module.
> 
> EFI_STATUS
> EFIAPI
> SecTemporaryRamSupport (
>  IN CONST EFI_PEI_SERVICES   **PeiServices,
>  IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
>  IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
>  IN UINTNCopySize
>  )
> {
>  VOID *OldHeap;
>  VOID *NewHeap;
>  VOID *OldStack;
>  VOID *NewStack;
>  UINTNStackMigrateOffset;
>  BASE_LIBRARY_JUMP_BUFFER JumpBuffer;
> 
>  DEBUG ((EFI_D_INFO,
>"TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
>TemporaryMemoryBase,
>PermanentMemoryBase,
>(UINT64)CopySize
>));
> 
>  //
>  // Assume Host prepare the stack and heap in the temprary ram that stack
>  // is below heap (stack is in smaller address).
>  // Stack/heap migration depends on the stack/heap location information
>  // in the temporary ram.
>  //
>  OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
>  NewStack = (VOID*)((UINTN)PermanentMemoryBase);
> 
>  OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
>  NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));
> 
>  StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;
> 
>  //
>  // Migrate Heap and Stack
>  //
>  CopyMem (NewHeap, OldHeap, CopySize >> 1);
>  CopyMem (NewStack, OldStack, CopySize >> 1);
> 
>  //
>  // Use SetJump()/LongJump() to switch to a new stack.
>  //
>  if (SetJump (&JumpBuffer) == 0) {
> #if defined (MDE_CPU_IA32)
>JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
>JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
> #endif
> #if defined (MDE_CPU_X64)
>JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
>JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
> #endif
>LongJump (&JumpBuffer, (UINTN)-1);
>  }
> 
>  ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);
> 
>  return EFI_SUCCESS;
> }
> 
> 
> Andrew,
> I'd like to know why you chose to produce the migration PPI from
> EmulatorPkg/Sec module.
> Based on PI spec and current PeiCore implementation, PeiCore can do the 
> migration when PPI is absent.
> 

Ray,

Sorry I don't remember if it was due to the code being so old, or if I was 
trying to better emulate a real platform.

Thanks,

Andrew Fish

> 
> 
> -- 
> Thanks,
> Ray
> ___
> 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] OvmfPkg/Sec: Clear the Cache Disable flag in the CR0 register

[Andrew Fish via edk2-devel]

> On Feb 18, 2019, at 5:23 AM, Laszlo Ersek  wrote:
> 
> generic comment (applies to all NASM usage I guess):
> 
> On 02/18/19 11:10, Jordan Justen wrote:
> 
>> +mov eax, cr0
>> +and eax, ~(1 << 30)
>> +mov cr0, eax
> 
>> +mov rax, cr0
>> +and eax, ~(1 << 30)
>> +mov cr0, rax
> 
> I've read up on the << and ~ operators in the NASM documentation, and I
> think the above build-time calculations of the masks are well-defined
> and correct.
> 
> - bit shifts are always unsigned
> - given bit position 30, ~(1 << 30) will be a value with 32 bits
> - bit-neg simply flips bits (one's complement)
> 
> On the other hand, I find these NASM specifics counter-intuitive. The
> expression ~(1 << 30) looks like valid C, but in C, it means a quite
> different thing.
> 
> I think calculating the mask with "strict dword" somehow (not exactly
> sure how) would make this more readable; or else the BTR instruction would.
> 
> Opinions? (Again, pertaining to all NASM usage in edk2.)
> 

Lazlo,

I guess comments, or #defines, are other options?

Thanks,

Andrew Fish

> Thanks
> Laszlo
> ___
> 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] Maintainers.txt: clarify Reviewer requirements and responsibilities

[Andrew Fish via edk2-devel]

Reviewed-by: Andrew Fish 

> On Feb 27, 2019, at 5:12 PM, Gao, Liming  wrote:
> 
> Reviewed-by: Liming Gao 
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, February 28, 2019 5:22 AM
>> To: edk2-devel-01 
>> Cc: Gao, Liming ; Kinney, Michael D
>> 
>> Subject: [edk2] [PATCH] Maintainers.txt: clarify Reviewer requirements and
>> responsibilities
>> 
>> The current language for "Package Reviewer" only vaguely hints that
>> Package Reviewers should be able to provide guidance and directions.
>> Make this more obvious.
>> 
>> Cc: Andrew Fish 
>> Cc: Ard Biesheuvel 
>> Cc: Leif Lindholm 
>> Cc: Liming Gao 
>> Cc: Michael D Kinney 
>> Cc: Philippe Mathieu-Daude 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek 
>> ---
>> 
>> Notes:
>>   - this is clearly a political patch; feel free to disagree
>>   - I'm proposing this for the next development cycle
>> 
>> Maintainers.txt | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Maintainers.txt b/Maintainers.txt
>> index 7772926b2fb1..ed090aeb4bdb 100644
>> --- a/Maintainers.txt
>> +++ b/Maintainers.txt
>> @@ -20,7 +20,10 @@ Descriptions of section entries:
>>  M: Package Maintainer: Cc address for patches and questions. Responsible
>> for reviewing and pushing package changes to source control.
>>  R: Package Reviewer: Cc address for patches and questions. Reviewers help
>> - maintainers review code, but don't have push access.
>> + maintainers review code, but don't have push access. A designated
>> Package
>> + Reviewer is reasonably familiar with the Package (or some modules
>> + thereof), and/or provides testing or regression testing for the Package
>> + (or some modules thereof), in certain platforms and environments.
>>  W: Web-page with status/info
>>  T: SCM tree type and location.  Type is one of: git, svn.
>>  S: Status, one of the following:
>> --
>> 2.19.1.3.g30247aa5d201
>> 
>> ___
>> 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] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

[Andrew Fish via edk2-devel]

Shenglei,

I was confused by the names of these patches. From a C point of view this is 
inline assembly:

VOID
EFIAPI
CpuBreakpoint (
VOID
)
{
__asm__ __volatile__ ("int $3");
}

These patches seem to be removing GAS and MASM assembly files, but not the 
inline assembly *.c files?

We don't want to remove the inline assembly from the BaseLib as that could have 
size, performance, and compiler optimization impact. 

For example CpuBreakpoint() for clang with LTO would end up inlining a single 
byte. For i385 a call to assembler would be 5 bytes at each call location plus 
the overhead of the function. So that is a size increase and a performance 
overhead. For a C complier calling an assembly function is a serializing event 
an that can restrict optimizations. Thus having some limited inline assembly 
support is very useful. 

Thanks,

Andrew Fish

> On Mar 4, 2019, at 5:40 PM, Shenglei Zhang  wrote:
> 
> MdePkg BaseSynchronizationLib still uses the inline X86 assembly code
> in C code files.It should be updated to consume nasm only.
> https://bugzilla.tianocore.org/show_bug.cgi?id=1163
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Shenglei Zhang 
> ---
> .../Library/BaseSynchronizationLib/BaseSynchronizationLib.inf   | 2 --
> 1 file changed, 2 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf 
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> index 32414b29fa..719dc1938d 100755
> --- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> +++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> @@ -75,13 +75,11 @@
> 
> [Sources.ARM]
> Synchronization.c
> -  Arm/Synchronization.asm   | RVCT
> Arm/Synchronization.S | GCC
> 
> [Sources.AARCH64]
> Synchronization.c
> AArch64/Synchronization.S | GCC
> -  AArch64/Synchronization.asm   | MSFT
> 
> [Packages]
> MdePkg/MdePkg.dec
> -- 
> 2.18.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] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

[Andrew Fish via edk2-devel]

> On Mar 5, 2019, at 1:32 PM, Gao, Liming  wrote:
> 
> Andrew:
>  BZ 1163 is to remove inline X86 assembly code in C source file. But, this 
> patch is wrong. I have gave my comments to update this patch.
> 

Why do we want to remove inline X86 assembly. As I mentioned it will lead to 
larger binaries, slower binaries, and less optimized code.

For example take this simple inline assembly function:

VOID
EFIAPI
CpuBreakpoint (
  VOID
  )
{
  __asm__ __volatile__ ("int $3");
}

Today with clang LTO turned on calling CpuBreakpoint() looks like this from the 
callers point of view. 

a.out[0x1fa5] <+6>:  cc  int3   

But if we move that to NASM:

a.out[0x1fa6] <+7>:  e8 07 00 00 00  calll  0x1fb2; 
CpuBreakpoint

plus:
a.out`CpuBreakpoint:
a.out[0x1fb2] <+0>: cc int3   
a.out[0x1fb3] <+1>: c3 retl   

And there is also an extra push and pop on the stack. The other issue is the 
call to the function that is unknown to the compiler will act like a 
_ReadWriteBarrier (Yikes I see _ReadWriteBarrier is deprecated in VC++ 2017). 
Is the depreciation of some of these intrinsics in VC++ driving the removal of 
inline assembly? For GCC inline assembly works great for local compile, and for 
clang LTO it works in entire program optimization.

The LTO bitcode includes inline assembly and constraints so that the optimizer 
knows what to do so it can get optimized just like the abstract bitcode during 
the Link Time Optimization. 

This is CpuBreakpoint():
; Function Attrs: noinline nounwind optnone ssp uwtable
define void @CpuBreakpoint() #0 {
  call void asm sideeffect "int $$3", "~{dirflag},~{fpsr},~{flags}"() #2, 
!srcloc !3
  ret void
}

This is AsmReadMsr64():
; Function Attrs: noinline nounwind optnone ssp uwtable
define i64 @AsmReadMsr64(i32) #0 {
  %2 = alloca i32, align 4
  %3 = alloca i64, align 8
  store i32 %0, i32* %2, align 4
  %4 = load i32, i32* %2, align 4
  %5 = call i64 asm sideeffect "rdmsr", 
"=A,{cx},~{dirflag},~{fpsr},~{flags}"(i32 %4) #2, !srcloc !4
  store i64 %5, i64* %3, align 8
  %6 = load i64, i64* %3, align 8
  ret i64 %6
}


I agree it make sense to remove .S and .asm files and only have .nasm files. 

Thanks,

Andrew Fish

PS For the Xcode clang since it emits frame pointers by default we need to add 
the extra 4 bytes to save the assembly functions so the stack can get unwound. 

a.out`CpuBreakpoint:
a.out[0x1f99] <+0>: 55 pushl  %ebp
a.out[0x1f9a] <+1>: 89 e5  movl   %esp, %ebp
a.out[0x1f9c] <+3>: cc int3   
a.out[0x1f9d] <+4>: 5d popl   %ebp
a.out[0x1f9e] <+5>: c3 retl   

So breakpoint goes from 1 byte to 11 bytes if we get rid of the intrinsics. 

>  The change is to reduce the duplicated implementation. It will be good on 
> the code maintain. Recently, one patch has to update .c and .nasm both. 
> Please see 
> https://lists.01.org/pipermail/edk2-devel/2019-February/037165.html 
> . Beside 
> this change, I propose to remove all GAS assembly code for IA32 and X64 arch. 
> After those change, the patch owner will collect the impact of the image 
> size. 
> 
> Thanks
> Liming

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

Re: [edk2] Does ARM platform produce MP protocol?

[Andrew Fish via edk2-devel]

> On Mar 6, 2019, at 5:22 AM, Ard Biesheuvel  wrote:
> 
> On Wed, 6 Mar 2019 at 13:41, Achin Gupta  > wrote:
>> 
>> On Wed, Mar 06, 2019 at 10:37:58AM +0100, Ard Biesheuvel wrote:
>>> (adding Achin and Charles)
>>> 
>>> On Wed, 6 Mar 2019 at 10:16, Ni, Ray  wrote:
 
> -Original Message-
> From: edk2-devel  On Behalf Of Ard
> Biesheuvel
> Sent: Wednesday, March 6, 2019 3:38 PM
> To: Ni, Ray 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] Does ARM platform produce MP protocol?
> 
> On Wed, 6 Mar 2019 at 06:44, Ni, Ray  wrote:
>> 
>> Ard, Leif,
>> I am a bit interested in how ARM platform supports the MP?
>> PI Spec defines below protocol but I failed to find a driver in ARM 
>> platform
> producing this protocol.
>> Or did I miss anything?
>> 
> 
> No you are right. We don't expose that on ARM, since UEFI only runs on a
> single core. Bringing up and taking down cores is done via a protocol 
> called
> PSCI, which is implemented by firmware running at a higher privilege 
> level.
> 
> So while it would be possible to implement the MP protocol on top of PSCI,
> we haven't identified a use case for it yet. (The OS calls PSCI directly 
> to boot
> the secondary cores)
>> 
>> IIUC, the MP protocol enables communication between processors that are 
>> already
>> up instead of bringing them up or taking them down. So, it is orthogonal to
>> PSCI. Is that what you meant?
>> 
> 
> Surely, StartupThisAP starts up the AP, no?
> 
> In any case, I didn't dig any deeper, but I know that PSCI can be used
> (even in the UEFI context) to execute pieces of code on another core
> (ACS uses this IIRC)

Ard,

I take it that the PSCI is replacing the INIT SIPI SIPI mechanism on x86. For 
PI you need to setup the APs (BSP is running EFI) and the MP protocol gets used 
for this on boot. Things like setting up SMM, syncing page tables, and MTRR 
registers. Seems like PSCI kind of replaces this usage on ARM. 

Since the APs don't run EFI, and it is not safe to call EFI APIs on APIs the 
usage for running code on the APs is limited. What I've seen is mostly 
utilities and diagnostics. For example you can write a utility to dump out the 
MTRR and MSR registers that are per CPU. From a diagnostic point of view it is 
easier to generate more memory and cache traffic if you have code running on 
the APs. 

Note: BSP and AP are terms from the original MP Spec on x86. BSP == Boot Strap 
Processor (so runs EFI) and AP == Application Processor (all the CPUs not 
running EFI).

Note: The INIT SIPI SIPI is painful to code since the SIPI sends a real mode 
address for code to run so it involves a lot of complex mode transitions so 
this is why it makes sense to centralize that code for x86. 

Thanks,

Andrew Fish


> 
 
 Is below EFI_MM_MP_PROTOCOL (added in PI spec 1.6) implemented in ARM?
 Or will it be implemented by an ARM module?
>>> 
>>> No it is currently not implemented, and I am not aware of any plans to do 
>>> so.
>> 
>> +1. There is no need to do this until UEFI runs on a single core on Arm.
>> 
> 
> until -> as long as ??
> 
>>> 
 I am asking this because MP_SERVICES protocol exposes CPU location 
 information
 (Package/Core/Thread) through *GetProcessorInfo*, but MM_MP protocol 
 doesn't
 have a way to expose that information.
 If such location information isn't exposed by MM_MP, is that because ARM 
 doesn't
 care about the location information? Or because ARM cares but forgot to 
 add similar
 *GetProcessorInfo* interface to MM_MP when changing the PI spec?
 Or ARM doesn't use the MM_MP at all?
>> 
>> Even if Arm used this protocol, it can work with the logical processor 
>> number. I
>> don't see a need to expose the location information to the caller. It seems 
>> very
>> Intel specific. Is the EFI_MP_SERVICES_PROTOCOL used on Arm?
>> 
> 
> No, that is what started the discussion.
> 
 
>>> 
>>> I don't know the history of this protocol and who proposed it, but
>>> today, we don't have a need for running per-CPU initialization code in
>>> the context of MM. Even if MM is a component of the more privileged
>>> firmware running on an ARM system, it is running in a sandbox that was
>>> primarily designed around exposing MM services to UEFI code running at
>>> the same privilege level as the OS (such as the authenticated variable
>>> store). Platform initialization code (which is more likely to require
>>> dispatch to each core) runs in the secure world as well, but not in
>>> the context of MM.
>>> 
>>> I will let Achin chime in here as well.
>> 
>> +1.
>> 
>> I will let Charles comment on the history. Maybe this protocol was designed
>> for Arm systems where MM is the most privileged firmware. The upstream
>> implementation runs MM in the lowest privilege level. Either way, this 
>> protocol
>> sense only when MM 

Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

[Andrew Fish via edk2-devel]

> On Mar 6, 2019, at 4:41 PM, Kinney, Michael D  
> wrote:
> 
> Liming,
>  
> That does not work either because inline assembly uses compiler specific 
> syntax.
>  
> Please update with the specific list of functions that you think the inline 
> should be removed to improve maintenance with no impacts in size/perf.
>  

Mike,

It is easy to find the gcc/clang inline assembler, just `git grep "__asm__ 
__volatile__"`

The main files are:
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X64/GccInline.c
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/Ia32/GccInline.c
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c

This is really just compiler optimizer control. 
Library/BaseSynchronizationLib/SynchronizationGcc.c:21:#define 
_ReadWriteBarrier() do { __asm__ __volatile__ ("": : : "memory"); } while(0)

Looks like this is working around the alignment ASSERT in BaseIoLibIntrinsic. 
OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c:43:  __asm__ __volatile__ ( "outl %0, %1" 
: : "a" (Value), "d" ((UINT16)Port) );
OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c:67:  __asm__ __volatile__ ( "inl %1, %0" 
: "=a" (Data) : "d" ((UINT16)Port) );

It seems like we have a reasonable set and should not use the inline assembly 
for any more complex code. 

Thanks,

Andrew Fish

> The issue you pointed to was around SetJump()/LongJump().  Can we limit this 
> BZ to only those 2 functions?
>  
> Mike
>   <>
> From: Gao, Liming 
> Sent: Wednesday, March 6, 2019 4:28 PM
> To: af...@apple.com
> Cc: Zhang, Shenglei ; edk2-devel-01 
> ; Kinney, Michael D 
> Subject: RE: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline 
> X86 assembly code
>  
> Andrew:
>   I want to keep only one implementation. If inline assembly c source is 
> preferred, I suggest to remove its nasm implementation.
>  
> Thanks
> Liming
>  <>From: af...@apple.com  [mailto:af...@apple.com 
> ] 
> Sent: Tuesday, March 5, 2019 2:44 PM
> To: Gao, Liming mailto:liming@intel.com>>
> Cc: Zhang, Shenglei  >; edk2-devel-01  >; Kinney, Michael D 
> mailto:michael.d.kin...@intel.com>>
> Subject: Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline 
> X86 assembly code
>  
>  
>  
> 
> On Mar 5, 2019, at 1:32 PM, Gao, Liming  > wrote:
>  
> Andrew:
>  BZ 1163 is to remove inline X86 assembly code in C source file. But, this 
> patch is wrong. I have gave my comments to update this patch.
> 
>  
> Why do we want to remove inline X86 assembly. As I mentioned it will lead to 
> larger binaries, slower binaries, and less optimized code.
>  
> For example take this simple inline assembly function:
>  
> VOID
> EFIAPI
> CpuBreakpoint (
>   VOID
>   )
> {
>   __asm__ __volatile__ ("int $3");
> }
>  
> 
> Today with clang LTO turned on calling CpuBreakpoint() looks like this from 
> the callers point of view. 
>  
> a.out[0x1fa5] <+6>:  cc  int3   
>  
> 
> But if we move that to NASM:
>  
> 
> a.out[0x1fa6] <+7>:  e8 07 00 00 00  calll  0x1fb2; 
> CpuBreakpoint
>  
> 
> plus:
> a.out`CpuBreakpoint:
> a.out[0x1fb2] <+0>: cc int3   
> a.out[0x1fb3] <+1>: c3 retl   
>  
> 
> And there is also an extra push and pop on the stack. The other issue is the 
> call to the function that is unknown to the compiler will act like a 
> _ReadWriteBarrier (Yikes I see _ReadWriteBarrier is deprecated in VC++ 2017). 
> Is the depreciation of some of these intrinsics in VC++ driving the removal 
> of inline assembly? For GCC inline assembly works great for local compile, 
> and for clang LTO it works in entire program optimization.
>  
> The LTO bitcode includes inline assembly and constraints so that the 
> optimizer knows what to do so it can get optimized just like the abstract 
> bitcode during the Link Time Optimization. 
>  
> This is CpuBreakpoint():
> ; Function Attrs: noinline nounwind optnone ssp uwtable
> define void @CpuBreakpoint() #0 {
>   call void asm sideeffect "int $$3", "~{dirflag},~{fpsr},~{flags}"() #2, 
> !srcloc !3
>   ret void
> }
>  
> 
> This is AsmReadMsr64():
> ; Function Attrs: noinline nounwind optnone ssp uwtable
> define i64 @AsmReadMsr64(i32) #0 {
>   %2 = alloca i32, align 4
>   %3 = alloca i64, align 8
>   store i32 %0, i32* %2, align 4
>   %4 = load i32, i32* %2, align 4
>   %5 = call i64 asm sideeffect "rdmsr", 
> "=A,{cx},~{dirflag},~{fpsr},~{flags}"(i32 %4) #2, !srcloc !4
>   store i64 %5, i64* %3, align 8
>   %6 = load i64, i64* %3, align 8
>   ret i64 %6
> }
>  
> 
>  
> I agree it make sense to remove .S and .asm files and only have .nasm files. 
>  
> Thanks,
>  
> Andrew Fish
>  
> PS

Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

[Andrew Fish via edk2-devel]

re should be a balance between optimization and code 
> readability/maintainability.
> 
> Do we have data on what size benefit we can get with these inline function, 
> from whole image level?
> We can do evaluation at function level, case by case.
> If we see a huge size benefit, I agree to keep this function.
> If we see no size benefit, I suggest we do the cleanup for this function.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Andrew Fish via edk2-devel
>> Sent: Wednesday, March 6, 2019 5:31 PM
>> To: Kinney, Michael D 
>> Cc: edk2-devel-01 ; Gao, Liming
>> 
>> Subject: Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove
>> inline X86 assembly code
>> 
>> 
>> 
>>> On Mar 6, 2019, at 4:41 PM, Kinney, Michael D
>>  wrote:
>>> 
>>> Liming,
>>> 
>>> That does not work either because inline assembly uses compiler specific
>> syntax.
>>> 
>>> Please update with the specific list of functions that you think the inline
>> should be removed to improve maintenance with no impacts in size/perf.
>>> 
>> 
>> Mike,
>> 
>> It is easy to find the gcc/clang inline assembler, just `git grep "__asm__
>> __volatile__"`
>> 
>> The main files are:
>> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLib
>> Intrinsic/IoLibGcc.c
>> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X
>> 64/GccInline.c
>> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/I
>> a32/GccInline.c
>> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseSync
>> hronizationLib/Ia32/GccInline.c
>> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseSync
>> hronizationLib/X64/GccInline.c
>> 
>> This is really just compiler optimizer control.
>> Library/BaseSynchronizationLib/SynchronizationGcc.c:21:#define
>> _ReadWriteBarrier() do { __asm__ __volatile__ ("": : : "memory"); } while(0)
>> 
>> Looks like this is working around the alignment ASSERT in BaseIoLibIntrinsic.
>> OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c:43:  __asm__ __volatile__
>> ( "outl %0, %1" : : "a" (Value), "d" ((UINT16)Port) );
>> OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c:67:  __asm__ __volatile__
>> ( "inl %1, %0" : "=a" (Data) : "d" ((UINT16)Port) );
>> 
>> It seems like we have a reasonable set and should not use the inline
>> assembly for any more complex code.
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>> The issue you pointed to was around SetJump()/LongJump().  Can we limit
>> this BZ to only those 2 functions?
>>> 
>>> Mike
>>>   <>
>>> From: Gao, Liming
>>> Sent: Wednesday, March 6, 2019 4:28 PM
>>> To: af...@apple.com
>>> Cc: Zhang, Shenglei ; edk2-devel-01
>> ; Kinney, Michael D
>> 
>>> Subject: RE: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove
>> inline X86 assembly code
>>> 
>>> Andrew:
>>>  I want to keep only one implementation. If inline assembly c source is
>> preferred, I suggest to remove its nasm implementation.
>>> 
>>> Thanks
>>> Liming
>>> <>From: af...@apple.com <mailto:af...@apple.com>
>> [mailto:af...@apple.com <mailto:af...@apple.com>]
>>> Sent: Tuesday, March 5, 2019 2:44 PM
>>> To: Gao, Liming mailto:liming@intel.com>>
>>> Cc: Zhang, Shenglei > <mailto:shenglei.zh...@intel.com>>; edk2-devel-01
>> mailto:edk2-devel@lists.01.org>>; Kinney,
>> Michael D > <mailto:michael.d.kin...@intel.com>>
>>> Subject: Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove
>> inline X86 assembly code
>>> 
>>> 
>>> 
>>> 
>>> On Mar 5, 2019, at 1:32 PM, Gao, Liming > <mailto:liming@intel.com>> wrote:
>>> 
>>> Andrew:
>>> BZ 1163 is to remove inline X86 assembly code in C source file. But, this
>> patch is wrong. I have gave my comments to update this patch.
>>> 
>>> 
>>> Why do we want to remove inline X86 assembly. As I mentioned it will lead
>> to larger binaries, slower binaries, and less optimized code.
>>> 
>>> For example take this simple inline assembly function:
>>> 
>>> VOID
>>> EFIAPI
>>> CpuBreakpoint (
>>&g

Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

[Andrew Fish via edk2-devel]

 thing. 
>  
> Most of the GCC/Clang inline assembler code is in Gccinline.c and since that 
> code is mostly just abstracting an x86 instruction and the functions are very 
> simple and thus it is NOT code that needs ongoing maintenance. 
>  
> Lets look at the history:
> https://github.com/tianocore/edk2/commits/master/MdePkg/Library/BaseLib/X64/GccInline.c
>  
> <https://github.com/tianocore/edk2/commits/master/MdePkg/Library/BaseLib/X64/GccInline.c>
> The last logic fix was Jun 1, 2010
>  
> https://github.com/tianocore/edk2/commits/master/MdePkg/Library/BaseLib/Ia32/GccInline.c
>  
> <https://github.com/tianocore/edk2/commits/master/MdePkg/Library/BaseLib/Ia32/GccInline.c>
> Ok so Mike had to make a fix in this file in 2015 to make something optional, 
> due to an embedded CPU defeating an instruction. But prior to that it was 
> 2010. 
>  
> The set of things that are C inline assembler we have should be static and 
> not require much maintenance. More complex code should be written in C and 
> use the C inline assembler functions we already have. If any more complex 
> assembly code is required we should just write it in NASM. So I think it is 
> fine to restrict new C inline assembler, or at least have a very high bar to 
> add anything new. Any new inline assembler should also be simple and just be 
> a function abstracting a CPU op-code that is not available to C. This is how 
> we prevent the maintenance issues you are worrying about. 
>  
> I gave an example in this  mail thread on how a Breakpoint goes from being 1 
> byte to 11 bytes if you remove the C inline assembler. For clang with LTO 
> enabled a CpuBreakpoint will always get inlined into the callers code and it 
> will only take the one byte for int 3 instruction. If that code moves to NASM 
> then it get replaces with a 5 byte call instruction and an actual C ABI 
> function for the breakpoint. 
>  
> VOID
> EFIAPI
> CpuBreakpoint (
>   VOID
>   )
> {
>   __asm__ __volatile__ ("int $3");
> }
> 
> 
> Today with clang LTO turned on calling CpuBreakpoint() looks like this from 
> the callers point of view. 
>  
> a.out[0x1fa5] <+6>:  cc  int3   
> 
> 
> But if we move that to NASM:
> 
> 
> a.out[0x1fa6] <+7>:  e8 07 00 00 00  calll  0x1fb2; 
> CpuBreakpoint
> 
> 
> plus:
> a.out`CpuBreakpoint:
> a.out[0x1f99] <+0>: 55 pushl  %ebp
> a.out[0x1f9a] <+1>: 89 e5  movl   %esp, %ebp
> a.out[0x1f9c] <+3>: cc int3   
> a.out[0x1f9d] <+4>: 5d popl   %ebp
> a.out[0x1f9e] <+5>: c3 retl   
> 
> 
> For any compiler that emits the frame pointer if you move the INT 3 to 
> assembly you need the frame pointer or the Exception Lib is not going to be 
> able to print out the stack backtrace of the code when you hit a breakpoint. 
> So this is how I get to 11 vs. 1 bytes. 
> 
> 
> Thanks,
>  
> Andrew Fish
>  
> PS for clang LTO the compiler compiles to bitcode and that is an abstracted 
> assembly language. At link time all that code gets optimized to together and 
> then passed through the CPU specific code gen. For C inline assembler the 
> assembly instructions end up in the bitcode with lots of information about 
> the constraints. That is why these GccInline.c functions almost always get 
> inlined with clang and LTO. 
>  
> The CpuBreakpoint() function looks like this in bitcode, but the function 
> call gets optimized away by LTO in the code gen. 
>  
> ; Function Attrs: noinline nounwind optnone ssp uwtable
> define void @CpuBreakpoint() #0 {
>   call void asm sideeffect "int $$3", "~{dirflag},~{fpsr},~{flags}"() #2, 
> !srcloc !3
>   ret void
> }
> 
> 
> Even something more complex like AsmReadMsr64() can get inlined, but it 
> contains a lot more info about the constraints:
>  
> ; Function Attrs: noinline nounwind optnone ssp uwtable
> define i64 @AsmReadMsr64(i32) #0 {
>   %2 = alloca i32, align 4
>   %3 = alloca i64, align 8
>   store i32 %0, i32* %2, align 4
>   %4 = load i32, i32* %2, align 4
>   %5 = call i64 asm sideeffect "rdmsr", 
> "=A,{cx},~{dirflag},~{fpsr},~{flags}"(i32 %4) #2, !srcloc !4
>   store i64 %5, i64* %3, align 8
>   %6 = load i64, i64* %3, align 8
>   ret i64 %6
> }
> 
> 
> The alloca, store, load, etc are the same bitcode instructions you would see 
> with C code. 
> 
> 
> A recently case that SetJump/LongJump, I updated the NASM file for both IA32 
> and X64.
> 
> Later, to my surprise, only X64 version NASM works, and IA32 version NASM 
> does not work.
> Then I notice that there is a different c

[edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

I'm trying to understand why gdtPtr.Base is casting to (UINT32)? 
1) gdtPtr.Base is a a UINTN
2) It is legal for AllocateRuntimePool() to return an address > 4GB

It seems like the code should just cast to (UINTN)?


https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuGdt.c#L151



VOID
InitGlobalDescriptorTable (
  VOID
  )
{
  GDT_ENTRIES *gdt;
  IA32_DESCRIPTOR gdtPtr;

  //
  // Allocate Runtime Data for the GDT
  //
  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
  ASSERT (gdt != NULL);
  gdt = ALIGN_POINTER (gdt, 8);

  //
  // Initialize all GDT entries
  //
  CopyMem (gdt, &GdtTemplate, sizeof (GdtTemplate));

  //
  // Write GDT register
  //
  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
  gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
  AsmWriteGdtr (&gdtPtr);

Thanks,

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

Re: [edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

Actually it looks like the the CpuDxe driver is coded to only run if it it is 
loaded under 4 GB? Is that following the spec? Is that intentional?

I noticed that SetCodeSelector is coded to use a far jump and that is a 32-bit 
absolute value? Note [rsp+4]
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm#L28
ASM_PFX(SetCodeSelector):
sub rsp, 0x10
lea rax, [setCodeSelectorLongJump]
mov [rsp], rax
mov [rsp+4], cx
jmp dword far [rsp]
setCodeSelectorLongJump:
add rsp, 0x10
ret


Thanks,

Andrew Fish

> On Mar 7, 2019, at 2:37 PM, Andrew Fish  wrote:
> 
> I'm trying to understand why gdtPtr.Base is casting to (UINT32)? 
> 1) gdtPtr.Base is a a UINTN
> 2) It is legal for AllocateRuntimePool() to return an address > 4GB
> 
> It seems like the code should just cast to (UINTN)?
> 
> 
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuGdt.c#L151 
> 
> 
> 
> 
> VOID
> InitGlobalDescriptorTable (
>   VOID
>   )
> {
>   GDT_ENTRIES *gdt;
>   IA32_DESCRIPTOR gdtPtr;
> 
>   //
>   // Allocate Runtime Data for the GDT
>   //
>   gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
>   ASSERT (gdt != NULL);
>   gdt = ALIGN_POINTER (gdt, 8);
> 
>   //
>   // Initialize all GDT entries
>   //
>   CopyMem (gdt, &GdtTemplate, sizeof (GdtTemplate));
> 
>   //
>   // Write GDT register
>   //
>   gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
>   gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
>   AsmWriteGdtr (&gdtPtr);
> 
> Thanks,
> 
> Andrew Fish

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

Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove inline X86 assembly code

[Andrew Fish via edk2-devel]

  int3   
>  
> 
> But if we move that to NASM:
>  
> 
> a.out[0x1fa6] <+7>:  e8 07 00 00 00  calll  0x1fb2; 
> CpuBreakpoint
>  
> 
> plus:
> a.out`CpuBreakpoint:
> a.out[0x1f99] <+0>: 55 pushl  %ebp
> a.out[0x1f9a] <+1>: 89 e5  movl   %esp, %ebp
> a.out[0x1f9c] <+3>: cc int3   
> a.out[0x1f9d] <+4>: 5d popl   %ebp
> a.out[0x1f9e] <+5>: c3 retl   
>  
> 
> For any compiler that emits the frame pointer if you move the INT 3 to 
> assembly you need the frame pointer or the Exception Lib is not going to be 
> able to print out the stack backtrace of the code when you hit a breakpoint. 
> So this is how I get to 11 vs. 1 bytes. 
>  
> 
> Thanks,
>  
> Andrew Fish
>  
> PS for clang LTO the compiler compiles to bitcode and that is an abstracted 
> assembly language. At link time all that code gets optimized to together and 
> then passed through the CPU specific code gen. For C inline assembler the 
> assembly instructions end up in the bitcode with lots of information about 
> the constraints. That is why these GccInline.c functions almost always get 
> inlined with clang and LTO. 
>  
> The CpuBreakpoint() function looks like this in bitcode, but the function 
> call gets optimized away by LTO in the code gen. 
>  
> ; Function Attrs: noinline nounwind optnone ssp uwtable
> define void @CpuBreakpoint() #0 {
>   call void asm sideeffect "int $$3", "~{dirflag},~{fpsr},~{flags}"() #2, 
> !srcloc !3
>   ret void
> }
>  
> 
> Even something more complex like AsmReadMsr64() can get inlined, but it 
> contains a lot more info about the constraints:
>  
> ; Function Attrs: noinline nounwind optnone ssp uwtable
> define i64 @AsmReadMsr64(i32) #0 {
>   %2 = alloca i32, align 4
>   %3 = alloca i64, align 8
>   store i32 %0, i32* %2, align 4
>   %4 = load i32, i32* %2, align 4
>   %5 = call i64 asm sideeffect "rdmsr", 
> "=A,{cx},~{dirflag},~{fpsr},~{flags}"(i32 %4) #2, !srcloc !4
>   store i64 %5, i64* %3, align 8
>   %6 = load i64, i64* %3, align 8
>   ret i64 %6
> }
>  
> 
> The alloca, store, load, etc are the same bitcode instructions you would see 
> with C code. 
>  
> 
> A recently case that SetJump/LongJump, I updated the NASM file for both IA32 
> and X64.
> 
> Later, to my surprise, only X64 version NASM works, and IA32 version NASM 
> does not work.
> Then I notice that there is a different copy if IA32 Jump.c MS inline - I 
> also need update. That is frustrated.
> 
> I think there should be a balance between optimization and code 
> readability/maintainability.
> 
> Do we have data on what size benefit we can get with these inline function, 
> from whole image level?
> We can do evaluation at function level, case by case.
> If we see a huge size benefit, I agree to keep this function.
> If we see no size benefit, I suggest we do the cleanup for this function.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org 
> <mailto:edk2-devel-boun...@lists.01.org>] On Behalf Of
> Andrew Fish via edk2-devel
> Sent: Wednesday, March 6, 2019 5:31 PM
> To: Kinney, Michael D  <mailto:michael.d.kin...@intel.com>>
> Cc: edk2-devel-01 mailto:edk2-devel@lists.01.org>>; 
> Gao, Liming
> mailto:liming@intel.com>>
> Subject: Re: [edk2] [PATCH 3/3] MdePkg/BaseSynchronizationLib: Remove
> inline X86 assembly code
> 
> 
> 
> 
> On Mar 6, 2019, at 4:41 PM, Kinney, Michael D
> mailto:michael.d.kin...@intel.com>> wrote:
> 
> 
> Liming,
> 
> That does not work either because inline assembly uses compiler specific
> syntax.
> 
> 
> Please update with the specific list of functions that you think the inline
> should be removed to improve maintenance with no impacts in size/perf.
> 
>  
> 
> Mike,
> 
> It is easy to find the gcc/clang inline assembler, just `git grep "__asm__
> __volatile__"`
> 
> The main files are:
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLib 
> <https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLib>
> Intrinsic/IoLibGcc.c
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X 
> <https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X>
> 64/GccInline.c
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/I 
> <https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/I>
> a32/GccInline.c
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseSy

Re: [edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

> On Mar 8, 2019, at 7:08 AM, Laszlo Ersek  wrote:
> 
> On 03/08/19 15:13, Yao, Jiewen wrote:
>> I guess the historic reason is that AP and BSP share same GDT before. As 
>> such, the GDT need to be below 4G, to let AP switch from real mode to 
>> protected mode.
>> We don't get issue, because Runtime memory is in BIN, and most platform 
>> allocates BIN under 4G.
>> 
>> Some thought:
>> 1) I am think we not sure if AP is using same GDT as BSP today. If yes, we 
>> need GDT under 4G, by using MaxAddress. If no, there should be no 
>> restriction for BSP GDT. The (UINT32) case should be removed for BSP. But we 
>> still AP GDT below 4G, to support wake from INIT-SIPI-SIPI.

Jiewen,

It looks like there are several places that assume memory is < 4 GB in the 
CpuDxe driver. 

I noticed SetCodeSelector () is using a far jump to change the CS at that is 
limited < 4 GB. I had to hack around it via:
   popq%rax
   pushq   %rcx
   pushq   %rax
   lretq

There is some other crash later on. 


>> 2) I am not sure why we need runtime memory. Do we need touch GDT at UEFI 
>> runtime?
> 
> I could be confusing things *very badly*, but I vaguely remember that
> APs could be woken up spuriously later, and they must be able to execute
> code "enough" to go back to sleep.
> 
> The following commits look relevant:
> 
> - 7615702169b8 ("UefiCpuPkg/MpInitLib: Add AsmRelocateApLoop() assembly
> code", 2016-08-17)
> 
> - 4d3314f69488 ("UefiCpuPkg/MpInitLib: Place APs in safe loop before
> hand-off to OS", 2016-08-17)
> 
> - bf2786dc7900 ("UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB",
> 2016-11-28)
> 

If I'm remembering correctly there are optional idle states for the AP. I think 
the real mode hlt loop might have used too much power on an UP OS. 

It is also not clear to me that we don't need the GDT when in real mode. In 
big-real mode you go to protected mode set the GDT and then it can get used for 
big-real so it seems like  the GDT is in play even in real mode. 

Thanks,

Andrew Fish


> Laszlo
> 
>> 
>> 
>> 
>> Thank you
>> Yao Jiewen
>> 
>> 
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Friday, March 8, 2019 12:00 AM
>>> To: Andrew Fish ; edk2-devel 
>>> Subject: Re: [edk2] UefiCpuPkg CpuDxe GDT init question?
>>> 
>>> Hi Andrew,
>>> 
>>> On 03/07/19 23:37, Andrew Fish via edk2-devel wrote:
>>>> I'm trying to understand why gdtPtr.Base is casting to (UINT32)?
>>>> 1) gdtPtr.Base is a a UINTN
>>>> 2) It is legal for AllocateRuntimePool() to return an address > 4GB
>>>> 
>>>> It seems like the code should just cast to (UINTN)?
>>>> 
>>>> 
>>>> 
>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuG
>>> dt.c#L151
>>> 
>>> I think you are right.
>>> 
>>> I'm missing the background on this too. I tried to see if any
>>> justification was given in a git commit message, but according to "git
>>> blame", this code dates back to the original addition of the driver,
>>> namely commit a47463f28382 ("Add CPU DXE driver for IA32 & X64
>>> processor
>>> architectures.", 2009-05-27). The commit message is unhelpful (for 3119
>>> lines added).
>>> 
>>> Thanks
>>> Laszlo
>>> 
>>>> 
>>>> 
>>>> 
>>>> VOID
>>>> InitGlobalDescriptorTable (
>>>>  VOID
>>>>  )
>>>> {
>>>>  GDT_ENTRIES *gdt;
>>>>  IA32_DESCRIPTOR gdtPtr;
>>>> 
>>>>  //
>>>>  // Allocate Runtime Data for the GDT
>>>>  //
>>>>  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
>>>>  ASSERT (gdt != NULL);
>>>>  gdt = ALIGN_POINTER (gdt, 8);
>>>> 
>>>>  //
>>>>  // Initialize all GDT entries
>>>>  //
>>>>  CopyMem (gdt, &GdtTemplate, sizeof (GdtTemplate));
>>>> 
>>>>  //
>>>>  // Write GDT register
>>>>  //
>>>>  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
>>>>  gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
>>>>  AsmWriteGdtr (&gdtPtr);
>>>> 
>>>> Thanks,
>>>> 
>>>> Andrew Fish
>>>> ___
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>> 
>>> 
>>> ___
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

Re: [edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

Jiewen,

These three fixes got me past the CpuDxe driver: 
https://bugzilla.tianocore.org/show_bug.cgi?id=1613

Now I getting page faults loading SMM

Thanks,

Andrew Fish

> On Mar 8, 2019, at 7:10 PM, Andrew Fish  wrote:
> 
> 
> 
>> On Mar 8, 2019, at 7:08 AM, Laszlo Ersek > <mailto:ler...@redhat.com>> wrote:
>> 
>> On 03/08/19 15:13, Yao, Jiewen wrote:
>>> I guess the historic reason is that AP and BSP share same GDT before. As 
>>> such, the GDT need to be below 4G, to let AP switch from real mode to 
>>> protected mode.
>>> We don't get issue, because Runtime memory is in BIN, and most platform 
>>> allocates BIN under 4G.
>>> 
>>> Some thought:
>>> 1) I am think we not sure if AP is using same GDT as BSP today. If yes, we 
>>> need GDT under 4G, by using MaxAddress. If no, there should be no 
>>> restriction for BSP GDT. The (UINT32) case should be removed for BSP. But 
>>> we still AP GDT below 4G, to support wake from INIT-SIPI-SIPI.
> 
> Jiewen,
> 
> It looks like there are several places that assume memory is < 4 GB in the 
> CpuDxe driver. 
> 
> I noticed SetCodeSelector () is using a far jump to change the CS at that is 
> limited < 4 GB. I had to hack around it via:
>popq%rax
>pushq   %rcx
>pushq   %rax
>lretq
> 
> There is some other crash later on. 
> 
> 
>>> 2) I am not sure why we need runtime memory. Do we need touch GDT at UEFI 
>>> runtime?
>> 
>> I could be confusing things *very badly*, but I vaguely remember that
>> APs could be woken up spuriously later, and they must be able to execute
>> code "enough" to go back to sleep.
>> 
>> The following commits look relevant:
>> 
>> - 7615702169b8 ("UefiCpuPkg/MpInitLib: Add AsmRelocateApLoop() assembly
>> code", 2016-08-17)
>> 
>> - 4d3314f69488 ("UefiCpuPkg/MpInitLib: Place APs in safe loop before
>> hand-off to OS", 2016-08-17)
>> 
>> - bf2786dc7900 ("UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB",
>> 2016-11-28)
>> 
> 
> If I'm remembering correctly there are optional idle states for the AP. I 
> think the real mode hlt loop might have used too much power on an UP OS. 
> 
> It is also not clear to me that we don't need the GDT when in real mode. In 
> big-real mode you go to protected mode set the GDT and then it can get used 
> for big-real so it seems like  the GDT is in play even in real mode. 
> 
> Thanks,
> 
> Andrew Fish
> 
> 
>> Laszlo
>> 
>>> 
>>> 
>>> 
>>> Thank you
>>> Yao Jiewen
>>> 
>>> 
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org 
>>>> <mailto:edk2-devel-boun...@lists.01.org>] On Behalf Of
>>>> Laszlo Ersek
>>>> Sent: Friday, March 8, 2019 12:00 AM
>>>> To: Andrew Fish mailto:af...@apple.com>>; edk2-devel 
>>>> mailto:edk2-devel@lists.01.org>>
>>>> Subject: Re: [edk2] UefiCpuPkg CpuDxe GDT init question?
>>>> 
>>>> Hi Andrew,
>>>> 
>>>> On 03/07/19 23:37, Andrew Fish via edk2-devel wrote:
>>>>> I'm trying to understand why gdtPtr.Base is casting to (UINT32)?
>>>>> 1) gdtPtr.Base is a a UINTN
>>>>> 2) It is legal for AllocateRuntimePool() to return an address > 4GB
>>>>> 
>>>>> It seems like the code should just cast to (UINTN)?
>>>>> 
>>>>> 
>>>>> 
>>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuG 
>>>> <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuG>
>>>> dt.c#L151
>>>> 
>>>> I think you are right.
>>>> 
>>>> I'm missing the background on this too. I tried to see if any
>>>> justification was given in a git commit message, but according to "git
>>>> blame", this code dates back to the original addition of the driver,
>>>> namely commit a47463f28382 ("Add CPU DXE driver for IA32 & X64
>>>> processor
>>>> architectures.", 2009-05-27). The commit message is unhelpful (for 3119
>>>> lines added).
>>>> 
>>>> Thanks
>>>> Laszlo
>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> VOID
>>>>> InitGlobalDescriptorTable (
>>>>>  VOID
>>>>

Re: [edk2] UefiCpuPkg CpuDxe GDT init question?

[Andrew Fish via edk2-devel]

> On Mar 11, 2019, at 9:04 AM, Yao, Jiewen  wrote:
> 
> Thanks Andrew.
> + More CPU people in our team.
> 
> Do you want to post your patch there for reference? :-)
> 
> BTW: Would you please share how to trigger such case at your side?
> Did you report above 4GB memory to be tested?

Jiewen,

I'm working on a chipset that is not open source and I or'ed in 
EFI_RESOURCE_ATTRIBUTE_TESTED to the above 4GB allocation in 
BuildResourceDescriptorHob(). The DXE Core then picked this area for the heap, 
as I seem to remember the 1st loop favors the PHIT, and the 2nd loop favors the 
largest area of free memory. 

> As such all drivers are loaded to above 4GB?

DXE Core is loaded low, and all the drivers loaded by DXE are in > 4GB memory. 

> Do you also allocate BIN to above 4GB?
> 

No I just marked the above 4G memory as tested. 

Thanks,

Andrew Fish

> Thank you
> Yao Jiewen
> 
> 
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Andrew Fish via edk2-devel
>> Sent: Monday, March 11, 2019 11:59 PM
>> To: Yao, Jiewen 
>> Cc: edk2-devel ; Laszlo Ersek 
>> Subject: Re: [edk2] UefiCpuPkg CpuDxe GDT init question?
>> 
>> Jiewen,
>> 
>> These three fixes got me past the CpuDxe driver:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=1613
>> 
>> Now I getting page faults loading SMM
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>> On Mar 8, 2019, at 7:10 PM, Andrew Fish  wrote:
>>> 
>>> 
>>> 
>>>> On Mar 8, 2019, at 7:08 AM, Laszlo Ersek > <mailto:ler...@redhat.com>> wrote:
>>>> 
>>>> On 03/08/19 15:13, Yao, Jiewen wrote:
>>>>> I guess the historic reason is that AP and BSP share same GDT before. As
>> such, the GDT need to be below 4G, to let AP switch from real mode to
>> protected mode.
>>>>> We don't get issue, because Runtime memory is in BIN, and most
>> platform allocates BIN under 4G.
>>>>> 
>>>>> Some thought:
>>>>> 1) I am think we not sure if AP is using same GDT as BSP today. If yes, we
>> need GDT under 4G, by using MaxAddress. If no, there should be no
>> restriction for BSP GDT. The (UINT32) case should be removed for BSP. But
>> we still AP GDT below 4G, to support wake from INIT-SIPI-SIPI.
>>> 
>>> Jiewen,
>>> 
>>> It looks like there are several places that assume memory is < 4 GB in the
>> CpuDxe driver.
>>> 
>>> I noticed SetCodeSelector () is using a far jump to change the CS at that is
>> limited < 4 GB. I had to hack around it via:
>>>  popq%rax
>>>  pushq   %rcx
>>>  pushq   %rax
>>>  lretq
>>> 
>>> There is some other crash later on.
>>> 
>>> 
>>>>> 2) I am not sure why we need runtime memory. Do we need touch GDT
>> at UEFI runtime?
>>>> 
>>>> I could be confusing things *very badly*, but I vaguely remember that
>>>> APs could be woken up spuriously later, and they must be able to execute
>>>> code "enough" to go back to sleep.
>>>> 
>>>> The following commits look relevant:
>>>> 
>>>> - 7615702169b8 ("UefiCpuPkg/MpInitLib: Add AsmRelocateApLoop()
>> assembly
>>>> code", 2016-08-17)
>>>> 
>>>> - 4d3314f69488 ("UefiCpuPkg/MpInitLib: Place APs in safe loop before
>>>> hand-off to OS", 2016-08-17)
>>>> 
>>>> - bf2786dc7900 ("UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB",
>>>> 2016-11-28)
>>>> 
>>> 
>>> If I'm remembering correctly there are optional idle states for the AP. I
>> think the real mode hlt loop might have used too much power on an UP OS.
>>> 
>>> It is also not clear to me that we don't need the GDT when in real mode. In
>> big-real mode you go to protected mode set the GDT and then it can get
>> used for big-real so it seems like  the GDT is in play even in real mode.
>>> 
>>> Thanks,
>>> 
>>> Andrew Fish
>>> 
>>> 
>>>> Laszlo
>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> Thank you
>>>>> Yao Jiewen
>>>>> 
>>>>> 
>>>>>> -Original Message-
>>>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org
>> <mailto:edk2-devel-boun...@lists.01.org>] On Behalf 

Re: [edk2] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Andrew Fish via edk2-devel]

> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek  wrote:
> 
> Hello Heyi,
> 
> On 03/12/19 07:56, Heyi Guo wrote:
>> Hi Laszlo,
>> 
>> I'm thinking about a proposal as below:
>> 
>> 1. Reuse the framework of
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> 
>> 2. The boot time behavior of this module is not changed
>> 3. Switch to status code protocol for runtime debug
>> 4. Use an overridden SerialPortLib instance for
>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>> 
>> Then we can have below benefits:
>> 1. the boot time firmware log, and the OS log, went to one port; and
>> there is no dependence for runtime drivers to output serial message at
>> boot time.
>> 2. By implementing different instances of SerialPortLib for
>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>> runtime serial message to platform specific serial port.
> 
> This looks a bit over-engineered to me. Ultimately our goal is:
> - for DEBUGs to reach "a" serial port at runtime -- namely one that
> differs from the "normal" serial port.
> 
> Given that you'd have to implement a "special" SerialPortLib instance
> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
> 
> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
> stick universally with BaseDebugLibSerialPort, for DebugLib,
> 
> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
> (a) runtime behavior and (b) handling of a special serial port?
> 
> In other words, push down the "runtime?" distinction from DebugLib (see
> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
> would be used only with DXE_RUNTIME_DRIVERs.
> 
> The lib instance would prepare everything in advance for accessing the
> "special" serial port at runtime (which could require allocating runtime
> memory in advance). Once ExitBootServices() is called, a callback should
> switch over the behavior of the lib instance, without allocating further
> memory. (The switched-over behavior would not have to be functional
> until the virtual address map is set.)
> 

Laszlo,

Runtime does not quite work like that. As soon as the Set Virtual Address map 
fires it is only legal to call things EFI RT from the virtual mapping. The last 
stage of the Set Virtual Address Map call is to reapply the fixups in the 
Runtime Drivers for the virtual address space. That means any absolute address 
reference in the code now points to the virtual address. Also the Set Virtual 
Address event told the Runtime Driver to convert all its pointers to point to 
the new virtual address space. 

The blackout and the fact you need to hide the hardware from the OS make doing 
things at EFI Runtime Time hard. 

I agree with you that since our logging is really just a serial stream we don't 
require Report Status Code. Report Status Code exists so that the old PC POST 
Cards could still be supported, see: 
https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is also 
a Report Status Code layer that allows redirecting the stream to multiple 
agents, so for example you could send data to port 80 (POST Card) and write out 
the same values in the serial stream. I find lookup up Report Status Code codes 
very tedious and not really helpful for debugging vs. DEBUG prints. 

Thanks,

Andrew Fish

> I've always seen status code reporting cumbersome in comparison to plain
> DEBUG messages. My understanding is that status code reporting targets
> devices that are even dumber than serial ports. But, we do target a
> second serial port. So if we can keep the switchover internal to the
> SerialPortLib class, at least for runtime DXE drivers, then I think we
> should do that.
> 
> Thanks,
> Laszlo
> 
>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>> +Peter, for the last few paragraphs
>>> 
>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
 On Thu, 28 Feb 2019 at 09:06, Heyi Guo  wrote:
> Serial port output is useful when debugging UEFI runtime services in
> OS runtime.
> The patches are trying to provide a handy method to enable runtime
> serial port
> debug for ArmVirtQemu.
> 
> Cc: Jian J Wang 
> Cc: Hao Wu 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Cc: Julien Grall 
> 
> Heyi Guo (3):
>MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>ArmVirtQemu: enable runtime debug by build flag
> 
 Hello Heyi,
 
 We have had this discussion before, and IIRC, the proposed solution
 was to use status codes.
 
 I'm not sure how that is supposed to work though - hopefully Laszlo or
 one of the MdeModulePkg maintainers can elaborate.
>>> Here's the basic idea.
>>> 
>>> Status Code reporting and routing are defined by the PI spec for OS
>>> runtime as well, not just boot time.
>>> 
>>> Recently we added status code *routing* to ArmVirtPkg,

Re: [edk2] F29 Build Issue.

[Andrew Fish via edk2-devel]

Andrew,

This looks like a newer warning added in GCC 8 -Wall?

BaseTools/Source/C/GenVtf/GenVtf.h:46:#define   FIT_SIGNATURE   "_FIT_  
 " 

So it looks like the intent is to copy the 8 chars in the string, but not the 
NULL. That is the intent of  -Werror=stringop-truncation.

I think this code needs to replace strncpy with memcpy. Please file a bugzilla: 
https://bugzilla.tianocore.org

Thanks,

Andrew Fish

> On Mar 13, 2019, at 6:27 AM, Andrew J. Hutton  
> wrote:
> 
> Unsure if this is a missing build dependency (since there doesn't appear to 
> be a check for this) am I missing a make dep or configure step somewhere?
> 
> This is stock F29, following the website instructions; the make -C 
> BaseTools/Source/C results in the following errror:
> 
> make[1]: Entering directory 
> '/home/ajh/KVM/Clover/edk2/BaseTools/Source/C/GenVtf'
> cc  -c -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror 
> -Wno-deprecated-declarations -nostdlib -c -g  -I .. -I ../Include/Common -I 
> ../Include/ -I ../Include/IndustryStandard -I ../Common/ -I .. -I . -I 
> ../Include/X64/  GenVtf.c -o GenVtf.o
> GenVtf.c: In function ‘CreateFitTableAndInitialize’:
> GenVtf.c:1473:3: error: ‘strncpy’ output truncated before terminating nul 
> copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
>strncpy ((CHAR8 *) &FitStartPtr->CompAddress, FIT_SIGNATURE, 8);  // 
> "_FIT_ "
>^~~
> cc1: all warnings being treated as errors
> make[1]: *** [../Makefiles/footer.makefile:27: GenVtf.o] Error 1
> make[1]: Leaving directory 
> '/home/ajh/KVM/Clover/edk2/BaseTools/Source/C/GenVtf'
> make: *** [GNUmakefile:73: GenVtf] Error 2
> make: Leaving directory '/home/ajh/KVM/Clover/edk2/BaseTools/Source/C'
> 
> ___
> 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] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Andrew Fish via edk2-devel]

> On Mar 13, 2019, at 1:16 PM, Brian J. Johnson  wrote:
> 
> On 3/12/19 12:28 PM, Andrew Fish via edk2-devel wrote:
>>> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek  wrote:
>>> 
>>> Hello Heyi,
>>> 
>>> On 03/12/19 07:56, Heyi Guo wrote:
>>>> Hi Laszlo,
>>>> 
>>>> I'm thinking about a proposal as below:
>>>> 
>>>> 1. Reuse the framework of
>>>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>>>> 
>>>> 2. The boot time behavior of this module is not changed
>>>> 3. Switch to status code protocol for runtime debug
>>>> 4. Use an overridden SerialPortLib instance for
>>>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>>>> 
>>>> Then we can have below benefits:
>>>> 1. the boot time firmware log, and the OS log, went to one port; and
>>>> there is no dependence for runtime drivers to output serial message at
>>>> boot time.
>>>> 2. By implementing different instances of SerialPortLib for
>>>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>>>> runtime serial message to platform specific serial port.
>>> 
>>> This looks a bit over-engineered to me. Ultimately our goal is:
>>> - for DEBUGs to reach "a" serial port at runtime -- namely one that
>>> differs from the "normal" serial port.
>>> 
>>> Given that you'd have to implement a "special" SerialPortLib instance
>>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>> 
>>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>> 
>>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>>> (a) runtime behavior and (b) handling of a special serial port?
>>> 
>>> In other words, push down the "runtime?" distinction from DebugLib (see
>>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>>> would be used only with DXE_RUNTIME_DRIVERs.
>>> 
>>> The lib instance would prepare everything in advance for accessing the
>>> "special" serial port at runtime (which could require allocating runtime
>>> memory in advance). Once ExitBootServices() is called, a callback should
>>> switch over the behavior of the lib instance, without allocating further
>>> memory. (The switched-over behavior would not have to be functional
>>> until the virtual address map is set.)
>>> 
>> Laszlo,
>> Runtime does not quite work like that. As soon as the Set Virtual Address 
>> map fires it is only legal to call things EFI RT from the virtual mapping. 
>> The last stage of the Set Virtual Address Map call is to reapply the fixups 
>> in the Runtime Drivers for the virtual address space. That means any 
>> absolute address reference in the code now points to the virtual address. 
>> Also the Set Virtual Address event told the Runtime Driver to convert all 
>> its pointers to point to the new virtual address space.
>> The blackout and the fact you need to hide the hardware from the OS make 
>> doing things at EFI Runtime Time hard.
>> I agree with you that since our logging is really just a serial stream we 
>> don't require Report Status Code. Report Status Code exists so that the old 
>> PC POST Cards could still be supported, see: 
>> https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is 
>> also a Report Status Code layer that allows redirecting the stream to 
>> multiple agents, so for example you could send data to port 80 (POST Card) 
>> and write out the same values in the serial stream. I find lookup up Report 
>> Status Code codes very tedious and not really helpful for debugging vs. 
>> DEBUG prints.
>> Thanks,
>> Andrew Fish
> 
> Andrew, wasn't Report Status Code also intended to reduce code size? 
> PeiDxeDebugLibReportStatusCode packages up DEBUG() arguments into a structure 
> and passes it to the status code handler.  It avoids having to link a 
> PrintLib and SerialPortLib instance into essentially every module.  At least 
> that was the intent... IIRC at some point a few years ago folks realized that 
> VA_LIST wasn't portable across compilers, so now 
> PeiDxeDebugLibReportStatusCode converts VA_LIST into BASE_LIST, which 
> requires parsing the format string much like PrintLib does.  No idea if that 
> actually res

Re: [edk2] [staging/PRMCaseStudy]: Creating a new feature branch "PRMCaseStudy"

[Andrew Fish via edk2-devel]

> On Mar 14, 2019, at 7:28 AM, Laszlo Ersek  wrote:
> 
> On 03/14/19 09:46, You, Benjamin wrote:
>> Hi,
>> 
>> 
>> 
>> A new feature branch "PRMCaseStudy" is being created in edk2-staging.
>> 
>> 
>> Platform Runtime Mechanism (PRM) is an architecture for ACPI codes to call 
>> into UEFI BIOS's Runtime Services at OS runtime. Traditionally, ACPI codes 
>> call into BIOS through the invocation of Software SMIs. When applicable, PRM 
>> provides an alternative for ACPI codes to invoke UEFI Runtime Services, 
>> which runs in OS kernel space, without invoking SMM.
>> 
>> This package contains proof-of-concept codes that implement the ideas of PRM.
>> 
>> Resources:
>> 
>> - PRM introduction: https://www.opencompute.org/events/past-summits (Please 
>> search for "UEFI Implementation Intel(r) Xeon Based OCP Platform" in the 
>> webpage, which lists pointers to video and presentation that introduce the 
>> concept of PRM)
>> - TianoCore: http://www.tianocore.org
>> - UEFI Forum: https://uefi.org/
> 
> (I've read the slides now. Hopefully I understand them sufficiently for
> making reasonable comments below.)
> 
> While this feature looks exciting and nice from a technical standpoint,
> I think it would have a bad effect on operating system drivers, in
> particular in open source operating systems.
> 
> Currently, as far as I know, the best quality OS hw drivers are those
> that (a) manipulate hardware directly, and (b) are developed against
> open hardware specs.
> 
> In comparison, this feature promotes a generic OS->firmware call
> interface. It allows platform vendors to liberally define UEFI runtime
> services, and OS-level drivers to call those platform-specific UEFI
> runtime services through semi-standardized ACPI methods.
> 
> As a result, more and more OS hw drivers (especially for platform
> hardware) would be written on top of ACPI, and not on top of hardware
> access / hardware specs. I don't think that ACPI-based OS drivers for
> platform hardware are a bad thing; I think that making that kind of
> driver development *too easy* is a bad thing. Here's why:
> 
> - every such OS driver is harder to develop & debug for the OS
> developer, because there's another layer of software between them and
> the hardware that they cannot change, and can debug only with difficulties
> 
> - if the semi-standardized ACPI interfaces are too heavy-weight or too
> coarse-grained, then they might not integrate well into the driver
> framework of an OS. This can lead to bad performance and/or missing
> features.
> 
> - the *easy* availability of such a runtime OS->firmware interface will
> tempt hardware vendors to cut corners and add platform hacks to e.g. PCI
> Express or USB devices, or even to implement devices that should be PCIE
> or USB in the first place as platform devices. We've already seen too
> much of that mess. Devices with platform hacks, and platform devices,o
> are very hard to isolate and to use from virtual machines (as assigned
> (aka "passthrough") devices), for example. But even without
> virtualization, the same kind of isolation may be necessary for hw
> drivers implemented in userspace.
> 
> I'm totally a fan of ACPI (over DT, e.g.) for devices that *must* be
> platform devices. I'm very much not a fan of platform devices themselves.
> 
> In summary:
> 
> The runtime involvement of firmware should be minimized. The PRM
> approach seems to make that problem *worse* however, by widening the
> firmware's runtime role, through side-stepping the current problems of SMM.
> 

Laszlo,

I understand your concerns, but I think the reality today for OS/Firmware 
interactions that are not UEFI Spec Runtime services are currently implemented 
in ACPI, and anything you can't do in ACPI ASL ends up in either an EC 
(Embedded Controller) or SMM. There seems to be a trend to put more and more 
features into SMM, especially on  server. I recently saw a server code base 
that had a SMM memory region that defaulted to 256 MiB.

Writing a buch of platform features in SMM that has more privilege than the OS 
seems to be a security disaster waiting to happen. It looks like the PRM 
(Protected Runtime Mechanism) is a hybrid of the UEFI runtime model and ACPI. 
The most interesting statement is PRM runs Ring-0, in kernel driver context, 
and uses ASL to access hardware. Given all that it seems like it would be a lot 
easier to debug PRM code vs. SMM code. Also hopefully having the code tied to 
ACPI hardware access mechanisms would make it hard to move things into PRM that 
should be really done with an OS driver. I also think going forward it is going 
to be important to be able to disambiguate between UEFI Runtime and PRM 
drivers. 

As you know I'm the one always pointing out how complex it is to write EFI 
Runtime drivers so I'm not really pushing for an expansion of the role of EFI 
at OS runtime. I coined the term a long time ago that SMM was crack as the 1st 
time you use it fells OK, but things don't turn out

Re: [edk2] [PATCH V2 17/17] MdeModulePkg: Add PEIM and lib to dsc file

[Andrew Fish via edk2-devel]

I understand the motivation for this change as I've done something much less 
portable that looks a lot like this to save the PEI XIP space

I seem to remember a long time ago we add a public VA_LIST to an API and we ran 
into an issue due to the marker format being compiler specific. You don't tend 
to see this on VC++ as it mostly uses a pointer to the frame, but GCC and clang 
can use a data structure especially not on IA32 (i386) for a VA_LIST (this is 
part of the reason there are so many #ifdefs in the Base.h VA_LIST section).

In the past to fix this Mike Kinney added BASE_LIST.  I seem to remember 
MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode passes a BASE_LIST into 
ReportStatusCode vs. a VA_LIST for this reason. Was this VA_LIST portability 
concern taken into consideration for this new API?

In general VA_LIST is not an issue as long as all the code is compiled with the 
same compiler but if binaries are in play then you can have issues. So things 
like FSB, Option ROMs, OS Loaders, EFI Shell, EFI Apps. are when you can see 
the issue. 

Thanks,

Andrew Fish

> On Mar 14, 2019, at 10:17 PM, Zhichao Gao  wrote:
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1395
> 
> Add the new PEIM DebugServicePei and library instance
> PeiDebugLibDebugPpi to dsc file to verify it can build
> correctly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zhichao Gao 
> Cc: Jian J Wang 
> Cc: Hao Wu 
> Cc: Ray Ni 
> Cc: Star Zeng 
> Cc: Liming Gao 
> Cc: Sean Brogan 
> Cc: Michael Turner 
> Cc: Bret Barkelew 
> ---
> MdeModulePkg/MdeModulePkg.dsc | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index 6cd1727a0d..dec441e23e 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -297,6 +297,7 @@
>   
> MdeModulePkg/Library/PlatformHookLibSerialPortPpi/PlatformHookLibSerialPortPpi.inf
>   MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>   
> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
> +  MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf
>   MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
>   
> MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManagerLibNull.inf
>   MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
> @@ -423,6 +424,8 @@
>   MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
>   MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.inf
> 
> +  MdeModulePkg/Universal/DebugServicePei/DebugServicePei.inf
> +
>   MdeModulePkg/Application/CapsuleApp/CapsuleApp.inf
>   MdeModulePkg/Library/FmpAuthenticationLibNull/FmpAuthenticationLibNull.inf
>   MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf
> -- 
> 2.16.2.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 v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Andrew Fish via edk2-devel]

> On Mar 29, 2019, at 2:22 PM, Desimone, Nathaniel L 
>  wrote:
> 
> 1. Why would you do this for 64 bit but not 32 bit?

Is paging enabled on 32-bit, it is required for Long mode?

Also I'm not clear why it is an enhancement given you could take a periodic SMM 
in the kernels page fault handler and trashing CR2 seems bad.  Maybe there is 
some behavior I'm missing?

I'm not sure how big an issue this is but if SMM is modifying CR2 it is leaking 
information about SMM operations outside of SMM. 

Thanks,

Andrew Fish

> 2. Why don't you add the if statement to MpService.c instead of spreading it 
> to PageTbl.c?
> 3. What is the reason for this anyway? Adding the conditional is probably 
> more execution time than just reading CR2 always.
> 
> Thanks,
> Nate
> 
> -Original Message-
> From: edk2-devel  On Behalf Of nkvangup
> Sent: Friday, March 29, 2019 8:45 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ; 
> Laszlo Ersek 
> Subject: [edk2] [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand 
> paging in SMM
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> 
> For every SMI occurrence, save and restore CR2 register only when SMM 
> on-demand paging support is enabled in 64 bit operation mode.
> This is not a bug but to have better improvement of code.
> 
> Patch5 is updated with separate functions for Save and Restore of CR2 based 
> on review feedback.
> 
> Patch6 - Removed Global Cr2 instead used function parameter
> 
> Patch7 - Removed checking Cr2 with 0 as per feedback
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Yao Jiewen 
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 26 ++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |  9 ++---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 30 ++
> 4 files changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index b734a1ea8c..d3f62ed806 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -316,3 +316,29 @@ SetPageTableAttributes (
> 
>   return ;
> }
> +
> +/**
> +  This function returns with no action for 32 bit.
> +
> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value **/ 
> +VOID
> +SaveCr2 (
> +  UINTN  *Cr2
> +  )
> +{
> +  return ;
> +}
> +
> +/**
> +  This function returns with no action for 32 bit.
> +
> +  @param[in]  Cr2  Value to write into CR2 register **/ VOID
> +RestoreCr2 (
> +  UINTN  Cr2
> +  )
> +{
> +  return ;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..ce70f77709 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1112,9 +1112,11 @@ SmiRendezvous (
>   ASSERT(CpuIndex < mMaxNumberOfCpus);
> 
>   //
> -  // Save Cr2 because Page Fault exception in SMM may override its value
> +  // Save Cr2 because Page Fault exception in SMM may override its 
> + value,  // when using on-demand paging for above 4G memory.
>   //
> -  Cr2 = AsmReadCr2 ();
> +  Cr2 = 0;
> +  SaveCr2 (&Cr2);
> 
>   //
>   // Perform CPU specific entry hooks
> @@ -1253,10 +1255,11 @@ SmiRendezvous (
> 
> Exit:
>   SmmCpuFeaturesRendezvousExit (CpuIndex);
> +
>   //
>   // Restore Cr2
>   //
> -  AsmWriteCr2 (Cr2);
> +  RestoreCr2 (Cr2);
> }
> 
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 84efb22981..05e1b54ed2 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1243,4 +1243,26 @@ EFIAPI
> PiSmmCpuSmiEntryFixupAddress (
>  );
> 
> +/**
> +  This function reads CR2 register when on-demand paging is enabled
> +  for 64 bit and no action for 32 bit.
> +
> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value **/ 
> +VOID
> +SaveCr2 (
> +  UINTN  *Cr2
> +  );
> +
> +/**
> +  This function writes into CR2 register when on-demand paging is 
> +enabled
> +  for 64 bit and no action for 32 bit.
> +
> +  @param[in]  Cr2  Value to write into CR2 register **/ VOID
> +RestoreCr2 (
> +  UINTN  Cr2
> +  );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..e60628c080 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1053,3 +1053,33 @@ SetPageTableAttributes (
> 
>   return ;
> }
> +
> +/**
> +  This function reads CR2 register when on-demand paging is enabled
> +
> +  @param[out] 

Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Andrew Fish via edk2-devel]

> On Apr 1, 2019, at 9:47 AM, Laszlo Ersek  wrote:
> 
> On 04/01/19 10:16, nkvangup wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
>> 
>> For every SMI occurrence, save and restore CR2 register only when SMM
>> on-demand paging support is enabled in 64 bit operation mode.
>> This is not a bug but to have better improvement of code.
>> 
>> Patch5 is updated with separate functions for Save and Restore of CR2
>> based on review feedback.
>> 
>> Patch6 - Removed Global Cr2 instead used function parameter.
>> 
>> Patch7 - Removed checking Cr2 with 0 as per feedback.
>> 
>> Patch8 and 9 - Aligned with EDK2 Coding style.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Vanguput Narendra K 
>> Cc: Eric Dong 
>> Cc: Ray Ni 
>> Cc: Laszlo Ersek 
>> Cc: Yao Jiewen 
>> ---
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 26 ++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |  9 ++---
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 30 
>> ++
>> 4 files changed, 84 insertions(+), 3 deletions(-)
>> 
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> index b734a1ea8c..d1e146a70c 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> @@ -316,3 +316,29 @@ SetPageTableAttributes (
>> 
>>   return ;
>> }
>> +
>> +/**
>> +  This function returns with no action for 32 bit.
>> +
>> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value.
>> +**/
>> +VOID
>> +SaveCr2 (
>> +  OUT UINTN  *Cr2
>> +  )
>> +{
>> +  return ;
>> +}
>> +
>> +/**
>> +  This function returns with no action for 32 bit.
>> +
>> +  @param[in]  Cr2  Value to write into CR2 register.
>> +**/
>> +VOID
>> +RestoreCr2 (
>> +  IN UINTN  Cr2
>> +  )
>> +{
>> +  return ;
>> +}
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> index 3b0b3b52ac..ce70f77709 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> @@ -1112,9 +1112,11 @@ SmiRendezvous (
>>   ASSERT(CpuIndex < mMaxNumberOfCpus);
>> 
>>   //
>> -  // Save Cr2 because Page Fault exception in SMM may override its value
>> +  // Save Cr2 because Page Fault exception in SMM may override its value,
>> +  // when using on-demand paging for above 4G memory.
>>   //
>> -  Cr2 = AsmReadCr2 ();
>> +  Cr2 = 0;
>> +  SaveCr2 (&Cr2);
>> 
>>   //
>>   // Perform CPU specific entry hooks
>> @@ -1253,10 +1255,11 @@ SmiRendezvous (
>> 
>> Exit:
>>   SmmCpuFeaturesRendezvousExit (CpuIndex);
>> +
>>   //
>>   // Restore Cr2
>>   //
>> -  AsmWriteCr2 (Cr2);
>> +  RestoreCr2 (Cr2);
>> }
>> 
>> /**
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index 84efb22981..38f9104117 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -1243,4 +1243,26 @@ EFIAPI
>> PiSmmCpuSmiEntryFixupAddress (
>>  );
>> 
>> +/**
>> +  This function reads CR2 register when on-demand paging is enabled
>> +  for 64 bit and no action for 32 bit.
>> +
>> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value.
>> +**/
>> +VOID
>> +SaveCr2 (
>> +  OUT UINTN  *Cr2
>> +  );
>> +
>> +/**
>> +  This function writes into CR2 register when on-demand paging is enabled
>> +  for 64 bit and no action for 32 bit.
>> +
>> +  @param[in]  Cr2  Value to write into CR2 register.
>> +**/
>> +VOID
>> +RestoreCr2 (
>> +  IN UINTN  Cr2
>> +  );
>> +
>> #endif
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> index 2c77cb47a4..95eaf0b016 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> @@ -1053,3 +1053,33 @@ SetPageTableAttributes (
>> 
>>   return ;
>> }
>> +
>> +/**
>> +  This function reads CR2 register when on-demand paging is enabled.
>> +
>> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value.
>> +**/
>> +VOID
>> +SaveCr2 (
>> +  OUT UINTN  *Cr2
>> +  )
>> +{
>> +  if (!mCpuSmmStaticPageTable) {
>> +*Cr2 = AsmReadCr2 ();
>> +  }
>> +}
>> +
>> +/**
>> +  This function restores CR2 register when on-demand paging is enabled.
>> +
>> +  @param[in]  Cr2  Value to write into CR2 register.
>> +**/
>> +VOID
>> +RestoreCr2 (
>> +  IN UINTN  Cr2
>> +  )
>> +{
>> +  if (!mCpuSmmStaticPageTable) {
>> +AsmWriteCr2 (Cr2);
>> +  }
>> +}
>> 
> 
> I agree *how* this patch is implemented is correct, wrt. the IA32 / X64
> split.
> 
> A slight improvement for edk2 coding style would be to replace "*Cr2"
> with just "Cr2" in the @param[out] comments, but there's no need to
> repost the patch just because of that.
> 
> Regarding the "what" and "why", Nate's and Andrew's comments under v8
> make me uncomfortable about the pa

Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

[Andrew Fish via edk2-devel]

> On Apr 2, 2019, at 8:18 PM, Dong, Eric  wrote:
> 
> Hi Andrew,
>  
> I double confirmed in SDM, CR2 is not included in SMRAM State Save Map. Do 
> you means we should add this info in the commit message?
>  

Eric,

Sorry I was confused by the commit message. I thought the state save was being 
added, vs. being removed in paths that don't modify CR2. 

I realize now that the fix did not end up in SmiRendezvous() like this. 

if (!mCpuSmmStaticPageTable) {
  Cr2 = AsmReadCr2 ();
}
...
if (!mCpuSmmStaticPageTable) {
  AsmWriteCr2 (Cr2);
}

As mCpuSmmStaticPageTable is local to X64/PageTbl.c

So we would have to do something like this to not have the functions. 
Ia32/PageTbl.c
mCpuSmmStaticPageTable = FALSE;

Thus "This is not a bug but to have better improvement of code." actually means 
don't save CR2 if it is not modified in SMM context vs. unconditionally saving 
it. 

Sorry I have a high error rate on text diffs. In my day job I always use a 
difftool or grab the entire branch. 

Thanks,

Andrew Fish


> Thanks
> Eric
> From: af...@apple.com [mailto:af...@apple.com] 
> Sent: Tuesday, April 2, 2019 1:01 AM
> To: Laszlo Ersek 
> Cc: Vanguput, Narendra K ; edk2-devel 
> ; Yao, Jiewen ; Dong, Eric 
> 
> Subject: Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 
> on-demand paging in SMM
>  
>  
> 
> 
> On Apr 1, 2019, at 9:47 AM, Laszlo Ersek  > wrote:
>  
> On 04/01/19 10:16, nkvangup wrote:
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593 
> 
> 
> For every SMI occurrence, save and restore CR2 register only when SMM
> on-demand paging support is enabled in 64 bit operation mode.
> This is not a bug but to have better improvement of code.
> 
> Patch5 is updated with separate functions for Save and Restore of CR2
> based on review feedback.
> 
> Patch6 - Removed Global Cr2 instead used function parameter.
> 
> Patch7 - Removed checking Cr2 with 0 as per feedback.
> 
> Patch8 and 9 - Aligned with EDK2 Coding style.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K  >
> Cc: Eric Dong mailto:eric.d...@intel.com>>
> Cc: Ray Ni mailto:ray...@intel.com>>
> Cc: Laszlo Ersek mailto:ler...@redhat.com>>
> Cc: Yao Jiewen mailto:jiewen@intel.com>>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 26 ++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |  9 ++---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 30 ++
> 4 files changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index b734a1ea8c..d1e146a70c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -316,3 +316,29 @@ SetPageTableAttributes (
> 
>   return ;
> }
> +
> +/**
> +  This function returns with no action for 32 bit.
> +
> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value.
> +**/
> +VOID
> +SaveCr2 (
> +  OUT UINTN  *Cr2
> +  )
> +{
> +  return ;
> +}
> +
> +/**
> +  This function returns with no action for 32 bit.
> +
> +  @param[in]  Cr2  Value to write into CR2 register.
> +**/
> +VOID
> +RestoreCr2 (
> +  IN UINTN  Cr2
> +  )
> +{
> +  return ;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..ce70f77709 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1112,9 +1112,11 @@ SmiRendezvous (
>   ASSERT(CpuIndex < mMaxNumberOfCpus);
> 
>   //
> -  // Save Cr2 because Page Fault exception in SMM may override its value
> +  // Save Cr2 because Page Fault exception in SMM may override its value,
> +  // when using on-demand paging for above 4G memory.
>   //
> -  Cr2 = AsmReadCr2 ();
> +  Cr2 = 0;
> +  SaveCr2 (&Cr2);
> 
>   //
>   // Perform CPU specific entry hooks
> @@ -1253,10 +1255,11 @@ SmiRendezvous (
> 
> Exit:
>   SmmCpuFeaturesRendezvousExit (CpuIndex);
> +
>   //
>   // Restore Cr2
>   //
> -  AsmWriteCr2 (Cr2);
> +  RestoreCr2 (Cr2);
> }
> 
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 84efb22981..38f9104117 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1243,4 +1243,26 @@ EFIAPI
> PiSmmCpuSmiEntryFixupAddress (
>  );
> 
> +/**
> +  This function reads CR2 register when on-demand paging is enabled
> +  for 64 bit and no action for 32 bit.
> +
> +  @param[out]  *Cr2  Pointer to variable to hold CR2 register value.
> +**/
> +VOID
> +SaveCr2 (
> +  OUT UINTN  *Cr2
> +  );
> +
> +/**
> +  This function writes into CR2 register when on-demand paging is enabled
> +  for 64 bit and no action for 32 bit.
>

[edk2] History question about Base.h and its alternate parallel name space.... Should we change it?

[Andrew Fish via edk2-devel]

I had some one ask me recently why EFI_GUID does not work with #include 
. I explained they needed to use GUID vs. EFI_GUID. That prompted the 
question of why we have 2 names for the same thing. Well the historical 
answer was kind of political as some team wanted to use edk2, but not implement 
EFI. Thus we have EFI types without the EFI_ prefix in Base.h.

So all this got me thinking  Maybe it makes sense to move some of the 
renaming from MdePkg/Include/Uefi/UefiBaseType.h to Base.h? Removing the Base.h 
duplicate types would potentially hit lots of code [1] and break merges with 
other code bases (break other peoples Base libs etc.).

These lines in MdePkg/Include/Uefi/UefiBaseType.h would get moved to 
MdePkg/Include/Base.h:
typedef GUID  EFI_GUID;
typedef RETURN_STATUS EFI_STATUS;
#define EFIERR(_a)ENCODE_ERROR(_a)
#define EFI_ERROR(A)  RETURN_ERROR(A)

#define EFI_SUCCESS   RETURN_SUCCESS  
#define EFI_LOAD_ERRORRETURN_LOAD_ERROR   
#define EFI_INVALID_PARAMETER RETURN_INVALID_PARAMETER
#define EFI_UNSUPPORTED   RETURN_UNSUPPORTED  
#define EFI_BAD_BUFFER_SIZE   RETURN_BAD_BUFFER_SIZE  
#define EFI_BUFFER_TOO_SMALL  RETURN_BUFFER_TOO_SMALL 
#define EFI_NOT_READY RETURN_NOT_READY
#define EFI_DEVICE_ERROR  RETURN_DEVICE_ERROR 
#define EFI_WRITE_PROTECTED   RETURN_WRITE_PROTECTED  
#define EFI_OUT_OF_RESOURCES  RETURN_OUT_OF_RESOURCES 
#define EFI_VOLUME_CORRUPTED  RETURN_VOLUME_CORRUPTED 
#define EFI_VOLUME_FULL   RETURN_VOLUME_FULL  
#define EFI_NO_MEDIA  RETURN_NO_MEDIA 
#define EFI_MEDIA_CHANGED RETURN_MEDIA_CHANGED
#define EFI_NOT_FOUND RETURN_NOT_FOUND
#define EFI_ACCESS_DENIED RETURN_ACCESS_DENIED
#define EFI_NO_RESPONSE   RETURN_NO_RESPONSE  
#define EFI_NO_MAPPINGRETURN_NO_MAPPING   
#define EFI_TIMEOUT   RETURN_TIMEOUT  
#define EFI_NOT_STARTED   RETURN_NOT_STARTED  
#define EFI_ALREADY_STARTED   RETURN_ALREADY_STARTED  
#define EFI_ABORTED   RETURN_ABORTED  
#define EFI_ICMP_ERRORRETURN_ICMP_ERROR   
#define EFI_TFTP_ERRORRETURN_TFTP_ERROR   
#define EFI_PROTOCOL_ERRORRETURN_PROTOCOL_ERROR   
#define EFI_INCOMPATIBLE_VERSION  RETURN_INCOMPATIBLE_VERSION 
#define EFI_SECURITY_VIOLATIONRETURN_SECURITY_VIOLATION   
#define EFI_CRC_ERROR RETURN_CRC_ERROR   
#define EFI_END_OF_MEDIA  RETURN_END_OF_MEDIA
#define EFI_END_OF_FILE   RETURN_END_OF_FILE
#define EFI_INVALID_LANGUAGE  RETURN_INVALID_LANGUAGE
#define EFI_COMPROMISED_DATA  RETURN_COMPROMISED_DATA
#define EFI_HTTP_ERRORRETURN_HTTP_ERROR

#define EFI_WARN_UNKNOWN_GLYPHRETURN_WARN_UNKNOWN_GLYPH   
#define EFI_WARN_DELETE_FAILURE   RETURN_WARN_DELETE_FAILURE  
#define EFI_WARN_WRITE_FAILURERETURN_WARN_WRITE_FAILURE   
#define EFI_WARN_BUFFER_TOO_SMALL RETURN_WARN_BUFFER_TOO_SMALL
#define EFI_WARN_STALE_DATA   RETURN_WARN_STALE_DATA
#define EFI_WARN_FILE_SYSTEM  RETURN_WARN_FILE_SYSTEM

I'm interested what folks think about a change like this? This change makes the 
alternate names optional. 

I guess we could also leave the old Base.h definitions in Base.h and cleanup 
the code to only use the EFI form, but that is a much bigger change? 

[1] RETURN_SUCCSS usage: git grep -w RETURN_SUCCESS

Thanks,

Andrew Fish

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

Re: [edk2] History question about Base.h and its alternate parallel name space.... Should we change it?

[Andrew Fish via edk2-devel]

> On Jan 16, 2019, at 1:19 PM, Kinney, Michael D  
> wrote:
> 
> Hi Andrew,
> 
> I though the reason was a bit more technical.  We have a
> MODULE_TYPE of BASE.  Library instances that use the BASE
> MODULE_TYPE are declaring that the library interfaces are
> safe to be linked against a module of any other type (SEC,
> PEI, DXE, SMM, DXE_RUNTIME, UEFI_DRIVER, UEFI_APP).
> 
> We needed to make sure that a lib of type BASE that
> includes Base.h as its top level include file only has
> visibility to the types that are safe for all the other
> module types.  It is up to the top level include files
> for these other module types to provide the gasket to
> the types in Base.h.
> 
> If we add aliases in Base.h, then we may not get build
> breaks when a lib of type BASE includes files that are
> not compatible with BASE.
> 

Mike,

I don't think having aliases for return types really helps Base code quality  
as RETURN_SUCCESS is almost always just a comment in a header file, and only 
exists in a .c file. Thus RETURN_* seem like a needless duplication, best case 
it is a comment that the code is Base. 

I will agree that not having EFI_GUID defined does case all the PPI and 
Protocol files to blow up if you try to include them. The failure case I was 
helping explain was some one trying to include a PPI, that included a Protocol 
that contained a data structure that was needed. But I would posit that the 
definition of a (EFI_)GUID is state agnostic. Having access to a PPI or 
Protocol definition does not break Base code, what breaks Base code is trying 
to access some service that does not exist. To get more that EFI_GUID you are 
going to need to include Uefi.h, PiPei.h, PiDxe.h, etc. and that will block 
doing anything that is not Base. 

So I'm asking if redefining the name for EFI_GUID to GUID for Base is really 
that helpful? 

Thanks,

Andrew Fish


> Thanks,
> 
> Mike
> 
>> -Original Message-----
>> From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org] On Behalf Of Andrew Fish via edk2-
>> devel
>> Sent: Wednesday, January 16, 2019 1:00 PM
>> To: edk2-devel 
>> Subject: [edk2] History question about Base.h and its
>> alternate parallel name space Should we change it?
>> 
>> I had some one ask me recently why EFI_GUID does not
>> work with #include . I explained they needed to
>> use GUID vs. EFI_GUID. That prompted the question of why
>> we have 2 names for the same thing. Well the
>> historical answer was kind of political as some team
>> wanted to use edk2, but not implement EFI. Thus we have
>> EFI types without the EFI_ prefix in Base.h.
>> 
>> So all this got me thinking  Maybe it makes sense to
>> move some of the renaming from
>> MdePkg/Include/Uefi/UefiBaseType.h to Base.h? Removing
>> the Base.h duplicate types would potentially hit lots of
>> code [1] and break merges with other code bases (break
>> other peoples Base libs etc.).
>> 
>> These lines in MdePkg/Include/Uefi/UefiBaseType.h would
>> get moved to MdePkg/Include/Base.h:
>> typedef GUID  EFI_GUID;
>> typedef RETURN_STATUS EFI_STATUS;
>> #define EFIERR(_a)ENCODE_ERROR(_a)
>> #define EFI_ERROR(A)  RETURN_ERROR(A)
>> 
>> #define EFI_SUCCESS   RETURN_SUCCESS
>> #define EFI_LOAD_ERRORRETURN_LOAD_ERROR
>> #define EFI_INVALID_PARAMETER
>> RETURN_INVALID_PARAMETER
>> #define EFI_UNSUPPORTED   RETURN_UNSUPPORTED
>> #define EFI_BAD_BUFFER_SIZE   RETURN_BAD_BUFFER_SIZE
>> #define EFI_BUFFER_TOO_SMALL
>> RETURN_BUFFER_TOO_SMALL
>> #define EFI_NOT_READY RETURN_NOT_READY
>> #define EFI_DEVICE_ERROR  RETURN_DEVICE_ERROR
>> #define EFI_WRITE_PROTECTED   RETURN_WRITE_PROTECTED
>> #define EFI_OUT_OF_RESOURCES
>> RETURN_OUT_OF_RESOURCES
>> #define EFI_VOLUME_CORRUPTED
>> RETURN_VOLUME_CORRUPTED
>> #define EFI_VOLUME_FULL   RETURN_VOLUME_FULL
>> #define EFI_NO_MEDIA  RETURN_NO_MEDIA
>> #define EFI_MEDIA_CHANGED RETURN_MEDIA_CHANGED
>> #define EFI_NOT_FOUND RETURN_NOT_FOUND
>> #define EFI_ACCESS_DENIED RETURN_ACCESS_DENIED
>> #define EFI_NO_RESPONSE   RETURN_NO_RESPONSE
>> #define EFI_NO_MAPPINGRETURN_NO_MAPPING
>> #define EFI_TIMEOUT   RETURN_TIMEOUT
>> #define EFI_NOT_STARTED   RETURN_NOT_STARTED
>> #define EFI_ALREADY_STARTED   RETURN_ALREADY_STARTED
>> #define EFI_ABORTED   RETURN_ABORTED
>> #define EFI_ICMP_ERRORRETURN_ICMP_ERROR
>> #define EFI_TFTP_ERROR  

Re: [edk2] Query on Variable Services

[Andrew Fish via edk2-devel]

Galla,

The PCD value usually get set as the result of the build. 

EFI_MEMORY_RUNTIME attribute is used to request a virtual mapping from the OS. 
When the variable services are called from the OS the run in a virtual address 
space provided by the OS. Thus trying to access 0xFFE0 would page fault. 

You can run the memmap command from the EFI Shell and see if bit 63 is set. If 
your SPI controller is a memory mapped hardware device you may also need to map 
the SPI register via EFI_MEMORY_RUNTIME. The SPI driver also needs to deal with 
the SetVirtualAddress map event to convert its pointers over to the new OS 
provided virtual memory space.

Thanks,

Andrew Fish


> On Jan 17, 2019, at 9:23 AM, galla rao  wrote:
> 
> Hi All,
> 
> Have a question for Variable services
> 
> Given PCD's are initialized
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   | 0xFFE0
> 
>  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase |
> 0xFFE3E000
> 
>  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase   |
> 0xFFE4
> 
> Do i need to make these regions as *EFI_MEMORY_RUNTIME *through
> *gDS->SetMemorySpaceAttributes*
> 
> SPI Flash writes within BIOS works good!
> 
> *when trying to change BootOrder from efibootmgr, the failure is seen*
> 
> It would be useful if someone has faced this issue earlier and can respond
> kindly.
> 
> Best Regards
> Galla
> ___
> 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 on Variable Services

[Andrew Fish via edk2-devel]

> On Jan 17, 2019, at 10:26 AM, galla rao  wrote:
> 
> Thanks Andrew!
> 
> SPI driver has failed the call to, in the log i could see the failure
> 
> Status = gDS->SetMemorySpaceAttributes (
> 
> BaseAddress,
> 
> Length,
> 
> GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> 
> );
> 
> if (EFI_ERROR (Status)) {
> 
>   DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME 
> attribute to Flash. %r \n", Status));

You should dump out the Status value. I guess you could also make sure 
BaseAddress, and Length look correct. 

https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Pi/PiDxeCis.h#L368

/**
  Modifies the attributes for a memory region in the global coherency domain of 
the
  processor.
  @param  BaseAddress  The physical address that is the start address of a 
memory region.
  @param  Length   The size in bytes of the memory region.
  @param  Attributes   The bit mask of attributes to set for the memory 
region.
  @retval EFI_SUCCESS   The attributes were set for the memory region.
  @retval EFI_INVALID_PARAMETER Length is zero.
  @retval EFI_UNSUPPORTED   The processor does not support one or more 
bytes of the memory
resource range specified by BaseAddress and 
Length.
  @retval EFI_UNSUPPORTED   The bit mask of attributes is not support for 
the memory resource
range specified by BaseAddress and Length.
  @retval EFI_ACCESS_DENIED The attributes for the memory resource range 
specified by
BaseAddress and Length cannot be modified.
  @retval EFI_OUT_OF_RESOURCES  There are not enough system resources to modify 
the attributes of
the memory resource range.
  @retval EFI_NOT_AVAILABLE_YET The attributes cannot be set because CPU 
architectural protocol is
not available yet.
**/
typedef
EFI_STATUS
(EFIAPI *EFI_SET_MEMORY_SPACE_ATTRIBUTES)(
  IN EFI_PHYSICAL_ADDRESS BaseAddress,
  IN UINT64   Length,
  IN UINT64   Attributes
  );


Thanks,

Andrew Fish

> 
> }
> 
> if this call has failed SetVirtualAddress would not be useful for Virtual 
> Address conversion for this region I believe
>   
> Best Regards
> Ranga
> 
> On Thu, Jan 17, 2019 at 5:55 PM Andrew Fish  > wrote:
> Galla,
> 
> The PCD value usually get set as the result of the build. 
> 
> EFI_MEMORY_RUNTIME attribute is used to request a virtual mapping from the 
> OS. When the variable services are called from the OS the run in a virtual 
> address space provided by the OS. Thus trying to access 0xFFE0 would page 
> fault. 
> 
> You can run the memmap command from the EFI Shell and see if bit 63 is set. 
> If your SPI controller is a memory mapped hardware device you may also need 
> to map the SPI register via EFI_MEMORY_RUNTIME. The SPI driver also needs to 
> deal with the SetVirtualAddress map event to convert its pointers over to the 
> new OS provided virtual memory space.
> 
> Thanks,
> 
> Andrew Fish
> 
> 
> > On Jan 17, 2019, at 9:23 AM, galla rao  > > wrote:
> > 
> > Hi All,
> > 
> > Have a question for Variable services
> > 
> > Given PCD's are initialized
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   | 0xFFE0
> > 
> >  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase |
> > 0xFFE3E000
> > 
> >  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase   |
> > 0xFFE4
> > 
> > Do i need to make these regions as *EFI_MEMORY_RUNTIME *through
> > *gDS->SetMemorySpaceAttributes*
> > 
> > SPI Flash writes within BIOS works good!
> > 
> > *when trying to change BootOrder from efibootmgr, the failure is seen*
> > 
> > It would be useful if someone has faced this issue earlier and can respond
> > kindly.
> > 
> > Best Regards
> > Galla
> > ___
> > 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] History question about Base.h and its alternate parallel name space.... Should we change it?

[Andrew Fish via edk2-devel]

> On Jan 18, 2019, at 9:08 AM, Felix Polyudov  wrote:
> 
> Mike,
> 
> I think EFI_GUID and EFI_STATUS should cover most of the use cases.
> 

I think I missed a couple in my original mail

But here are the typedef and #define names that get remapped (or redone) in 
MdePkg/Include/Uefi/UefiBaseType.h

typedef struct {
  UINT32  Data1;
  UINT16  Data2;
  UINT16  Data3;
  UINT8   Data4[8];
} GUID;

typedef struct {
  UINT8 Addr[4];
} IPv4_ADDRESS;

typedef struct {
  UINT8 Addr[16];
} IPv6_ADDRESS;

typedef UINT64 PHYSICAL_ADDRESS;
typedef UINTN RETURN_STATUS;

#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | (StatusCode)))
#define ENCODE_WARNING(StatusCode)   ((RETURN_STATUS)(StatusCode))
#define RETURN_ERROR(StatusCode) (((INTN)(RETURN_STATUS)(StatusCode)) < 0)
#define RETURN_SUCCESS   0
#define RETURN_LOAD_ERRORENCODE_ERROR (1)
#define RETURN_INVALID_PARAMETER ENCODE_ERROR (2)



I think the cleanup would be as simple as moving things from 
MdePkg/Include/Uefi/UefiBaseType.h to MdePkg/Include/Base.h.

So:

typedef struct {
  UINT32  Data1;
  UINT16  Data2;
  UINT16  Data3;
  UINT8   Data4[8];
} GUID;

Becomes:

typedef struct {
  UINT32  Data1;
  UINT16  Data2;
  UINT16  Data3;
  UINT8   Data4[8];
} GUID, EFI_GUID;

Thanks,

Andrew Fish


> -Original Message-
> From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> Sent: Thursday, January 17, 2019 3:04 PM
> To: Felix Polyudov; 'Andrew Fish'; Kinney, Michael D
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] History question about Base.h and its alternate parallel 
> name space Should we change it?
> 
> Felix,
> 
> Is there a specific set that would have the most benefit?
> 
> Is EFI_GUID sufficient?
> 
> Mike
> 
>> -Original Message-
>> From: Felix Polyudov [mailto:fel...@ami.com]
>> Sent: Wednesday, January 16, 2019 3:10 PM
>> To: 'Andrew Fish' ; Kinney, Michael D
>> 
>> Cc: edk2-devel@lists.01.org
>> Subject: RE: [edk2] History question about Base.h and
>> its alternate parallel name space Should we change
>> it?
>> 
>> I agree with Andrew.
>> In my opinion, moving alias types to Base.h will help to
>> overcome certain practical inconveniences without
>> a significant impact on the ability to detect poorly
>> written Base libraries.
>> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org] On Behalf Of Andrew Fish via edk2-
>> devel
>> Sent: Wednesday, January 16, 2019 5:18 PM
>> To: Mike Kinney
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] History question about Base.h and
>> its alternate parallel name space Should we change
>> it?
>> 
>> 
>> 
>>> On Jan 16, 2019, at 1:19 PM, Kinney, Michael D
>>  wrote:
>>> 
>>> Hi Andrew,
>>> 
>>> I though the reason was a bit more technical.  We have
>> a
>>> MODULE_TYPE of BASE.  Library instances that use the
>> BASE
>>> MODULE_TYPE are declaring that the library interfaces
>> are
>>> safe to be linked against a module of any other type
>> (SEC,
>>> PEI, DXE, SMM, DXE_RUNTIME, UEFI_DRIVER, UEFI_APP).
>>> 
>>> We needed to make sure that a lib of type BASE that
>>> includes Base.h as its top level include file only has
>>> visibility to the types that are safe for all the
>> other
>>> module types.  It is up to the top level include files
>>> for these other module types to provide the gasket to
>>> the types in Base.h.
>>> 
>>> If we add aliases in Base.h, then we may not get build
>>> breaks when a lib of type BASE includes files that are
>>> not compatible with BASE.
>>> 
>> 
>> Mike,
>> 
>> I don't think having aliases for return types really
>> helps Base code quality  as RETURN_SUCCESS is almost
>> always just a comment in a header file, and only exists
>> in a .c file. Thus RETURN_* seem like a needless
>> duplication, best case it is a comment that the code is
>> Base.
>> 
>> I will agree that not having EFI_GUID defined does case
>> all the PPI and Protocol files to blow up if you try to
>> include them. The failure case I was helping explain was
>> some one trying to include a PPI, that included a
>> Protocol that contained a data structure that was
>> needed. But I would posit that the definition of a
>> (EFI_)GUID is state agnostic. Having access to a PPI or
>> Protocol definition does not break Base code, what
>> breaks Base code is trying to access some service t

Re: [edk2] History question about Base.h and its alternate parallel name space.... Should we change it?

[Andrew Fish via edk2-devel]

> On Jan 22, 2019, at 9:49 AM, Kinney, Michael D  
> wrote:
> 
> Andrew,
> 
> If we move all of those, then what are the code review rules for a lib of 
> type BASE?
> 
> Is there a preference to use the current BASE types?  Under what conditions 
> are EFI type names allowed?
> 
> Is the a preference to stop using the current BASE types all together?
> 

Mike,

it seems messy to add types to Base.h to support Base Libs. So for example 
IPv4_ADDRESS and IPv6_ADDRESS got sucked into Base.h since an MdePkg Base Lib 
got added that used it. That does not really seem to scale to Base Libs in 
other packages. Thus I guess the preference would be to use the common EFI_* 
names. I'd not advocate removing the old names as that is going to break other 
folks Base LIbs. We could do the cleanup in TianoCore. If we want to deprecate 
the old names form, I guess that could be an ifdef to allow some depreciation 
in the future? 

Thanks,

Andrew Fish

> Thanks,
> 
> Mike
> 
>> -Original Message-
>> From: af...@apple.com [mailto:af...@apple.com]
>> Sent: Friday, January 18, 2019 9:24 AM
>> To: Felix Poludov 
>> Cc: Kinney, Michael D ;
>> edk2-devel@lists.01.org
>> Subject: Re: [edk2] History question about Base.h and
>> its alternate parallel name space Should we change
>> it?
>> 
>> 
>> 
>>> On Jan 18, 2019, at 9:08 AM, Felix Polyudov
>>  wrote:
>>> 
>>> Mike,
>>> 
>>> I think EFI_GUID and EFI_STATUS should cover most of
>> the use cases.
>>> 
>> 
>> I think I missed a couple in my original mail
>> 
>> But here are the typedef and #define names that get
>> remapped (or redone) in
>> MdePkg/Include/Uefi/UefiBaseType.h
>> 
>> typedef struct {
>>  UINT32  Data1;
>>  UINT16  Data2;
>>  UINT16  Data3;
>>  UINT8   Data4[8];
>> } GUID;
>> 
>> typedef struct {
>>  UINT8 Addr[4];
>> } IPv4_ADDRESS;
>> 
>> typedef struct {
>>  UINT8 Addr[16];
>> } IPv6_ADDRESS;
>> 
>> typedef UINT64 PHYSICAL_ADDRESS;
>> typedef UINTN RETURN_STATUS;
>> 
>> #define ENCODE_ERROR(StatusCode)
>> ((RETURN_STATUS)(MAX_BIT | (StatusCode)))
>> #define ENCODE_WARNING(StatusCode)
>> ((RETURN_STATUS)(StatusCode))
>> #define RETURN_ERROR(StatusCode)
>> (((INTN)(RETURN_STATUS)(StatusCode)) < 0)
>> #define RETURN_SUCCESS   0
>> #define RETURN_LOAD_ERRORENCODE_ERROR (1)
>> #define RETURN_INVALID_PARAMETER ENCODE_ERROR (2)
>> 
>> 
>> 
>> I think the cleanup would be as simple as moving things
>> from MdePkg/Include/Uefi/UefiBaseType.h to
>> MdePkg/Include/Base.h.
>> 
>> So:
>> 
>> typedef struct {
>>  UINT32  Data1;
>>  UINT16  Data2;
>>  UINT16  Data3;
>>  UINT8   Data4[8];
>> } GUID;
>> 
>> Becomes:
>> 
>> typedef struct {
>>  UINT32  Data1;
>>  UINT16  Data2;
>>  UINT16  Data3;
>>  UINT8   Data4[8];
>> } GUID, EFI_GUID;
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>> 
>>> -Original Message-
>>> From: Kinney, Michael D
>> [mailto:michael.d.kin...@intel.com]
>>> Sent: Thursday, January 17, 2019 3:04 PM
>>> To: Felix Polyudov; 'Andrew Fish'; Kinney, Michael D
>>> Cc: edk2-devel@lists.01.org
>>> Subject: RE: [edk2] History question about Base.h and
>> its alternate parallel name space Should we change
>> it?
>>> 
>>> Felix,
>>> 
>>> Is there a specific set that would have the most
>> benefit?
>>> 
>>> Is EFI_GUID sufficient?
>>> 
>>> Mike
>>> 
>>>> -Original Message-
>>>> From: Felix Polyudov [mailto:fel...@ami.com]
>>>> Sent: Wednesday, January 16, 2019 3:10 PM
>>>> To: 'Andrew Fish' ; Kinney, Michael
>> D
>>>> 
>>>> Cc: edk2-devel@lists.01.org
>>>> Subject: RE: [edk2] History question about Base.h and
>>>> its alternate parallel name space Should we
>> change
>>>> it?
>>>> 
>>>> I agree with Andrew.
>>>> In my opinion, moving alias types to Base.h will help
>> to
>>>> overcome certain practical inconveniences without
>>>> a significant impact on the ability to detect poorly
>>>> written Base libraries.
>>>> 
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-
>>>> boun...@lists.01.org]

Re: [edk2] Drop CSM support in OvmfPkg?

[Andrew Fish via edk2-devel]

> On Jan 22, 2019, at 7:43 PM, Ni, Ray  wrote:
> 
>> -Original Message-
>> From: David Woodhouse mailto:dw...@infradead.org>>
>> Sent: Wednesday, January 23, 2019 12:23 AM
>> To: Ni, Ray mailto:ray...@intel.com>>; Gerd Hoffmann 
>> mailto:kra...@redhat.com>>;
>> Laszlo Ersek mailto:ler...@redhat.com>>; Richardson, 
>> Brian
>> mailto:brian.richard...@intel.com>>
>> Cc: Justen, Jordan L > >; edk2-devel@lists.01.org 
>> ;
>> Kevin O'Connor mailto:ke...@koconnor.net>>; Anthony 
>> Perard
>> mailto:anthony.per...@citrix.com>>
>> Subject: Re: Drop CSM support in OvmfPkg?
>> 
>> On Tue, 2019-01-22 at 16:13 +, Ni, Ray wrote:
>>> David,
>>> I'd like to re-start the discussion.
>>> Could you please kindly explain the background/reason of adding CSM
>>> support in OVMF?
>>> Maybe knowing the reason can help to make further decisions of
>>> whether to
>>> A. keep it outside OvmfPkg
>>> B. keep it inside OvmfPkg
>>> C. maybe have a chance to just remove the CSM support after
>>> revisiting
>> 
>> 
>> The idea was to make it simple to have a single firmware image for
>> virtual machines which would support both UEFI and Legacy boot for
>> guests simultaneously.
>> 
>> In libvirt there has been an alternative approach, where the BIOS image
>> is switched between OVMF and SeaBIOS according to the configuration of
>> the guest VM.
>> 
>> That's fine for libvirt, but in situations where VM hosting is provided
>> as a service, it becomes quite painful to manage the 'UEFI' vs.
>> 'Legacy' flags on guest images and then switch firmware images
>> accordingly. A one-size-fits-all BIOS using OVMF+CSM is very much
>> preferable.
> 
> David,
> Thanks for sharing. I now understand that you do have a need of
> CSM + UEFI OVMF image.
> A very straightforward idea is to move all COM components you needed
> into OvmfPkg. But Laszlo as the OvmfPkg owner may disagree with this.
> So maybe you could set up another (github) repo and clone all the CSM 
> components
> there.
> EDKII build tool supports to build firmware from multiple repos.
> That's how we can have edk2-platforms and to-be-created edk2-app.
> In practical, you could create a new csm repo.
> Laszlo/Gerd who don't care about CSM can just build OVMF image from edk2 repo.
> You can build the OVMF image from edk2 and csm repo.
> 

Ray,

I kind or agree at this point CSM is really more interesting for virtual 
machines vs. real platforms. I guess the interesting question to ask is do we 
want to start making it more part of the virtual machines that happen to be 
checked into TianoCore, or should we keep it more generic? The thinking being 
the CSM is likely upstreamed in other more commercial VMs? Can we just add a 
readme to the current CSM package and explain it mostly exists to support VMs? 

Thanks,

Andrew Fish

> We can have a call if you are ok. I can explain how that can work in details.
> 
> Thanks,
> Ray 
> 
> ___
> 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] Network Stack Budgeting

[Andrew Fish via edk2-devel]

> On Jan 23, 2019, at 8:27 AM, Tomas Pilar (tpilar)  
> wrote:
> 
> 
>> The set of devices connected during BDS is platform policy. It is not
>> the "network stack" that calls Snp.Start(), but the platform BDS code
>> that calls gBS->ConnectController() on the device, possibly without a
>> good reason (e.g. without the device being present in a UEFI boot
>> option). The network stack only "reacts" to that connection request.
> Indeed, but even if a SNP handle is present as a boot option in a boot 
> manager, surely the Snp.Start() should be deferred until the user actually 
> chooses to boot from that handle.
> 

Tom,

You don't need to call gBS->ConnectController() on all possible boot options, 
just the one you are currently trying to boot. 

I mostly muck around in a non edk2 BDS, but in general what you do in a BDS is:
1) Connect your graphics console(s) (usually involves ConOut NVRAM)
3) Connect any serial consoles (ConIn/ConOut NVRAM). 
2) Connect any built in keyboard, maybe SPI etc.  (ConIn NVRAM). 
4) Connect any hot pluggable console devices (Connect any existing USB HID 
devices).
5) Connect any other device required in the boot path (like the example entropy 
device. 

The platform can have policy to force values into ConIn/ConOut. For example on 
a laptop maybe you always want the built in keyboard active. 

As you attempt to boot a boot option you can recursively connect the device 
path for that boot option and attempt to boot it. If that option fails you can 
fall back to the next boot option and try to connect that device path and boot. 
Thus you don't need to start things before you are ready.

If you launch Firmware Setup that usually does a Connect All. The same things 
happen when you launch the Shell. 

Also some drivers connect extra stuff. For example when you try to connect a 
specific PCI device all the PCI IO handles get created. This is just how you 
have to enumerate PCI. But the recursive connect should only happen on the PCI 
IO handle you care about.

Thanks,

Andrew Fish


> A workaround that we have in the legacy implementation doesn't start the 
> underlying hardware datapath until the platform tries to send the first 
> packet (since PXE boot is always initiated by the client) but that is a 
> horrible hack that should not be necessary. The distinction between 
> Snp.Initialize() and Snp.Start() is there exactly for that reason, no?
> 
> In other words, ConnectController() should not immediately result in 
> Snp.Start() being called.
> 
> Cheers,
> Tom
> 
> 
> ___
> 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] [question] A piece of log.txt showed in uefi shell is not the same as showed in notepad++

[Andrew Fish via edk2-devel]

> On Jan 23, 2019, at 7:17 PM, krishnaLee  wrote:
> 
> Hi,
> I dumped a small log.txt from my work,but the log.txt showed in uefi shell,is 
> not the same as it showed in notepad++.
> I had upload the log here:
> https://github.com/krishna116/test/blob/master/log.zip
> 
> 
> it seems the log showed in uefi shell had missed some strings...I don't know 
> why,please help,
> 

Krishna,

Your log.txt looks like a normal UTF-16 Unicode file. The leading  FF FE is the 
Byte order mark for UTF-16 Little Endian [1].  The file looks like it has CR 
line endings [2] so maybe that is confusing your editor? There seems to be a CR 
LF at the end, so the mixture of line ending may even be more likely confusing 
the editor. The byte order mark should let your editor know it is 16-bit 
Unicode and the correct endian. Hope this helps...

$ hexdump -C /Users/andrewfish/Downloads/log/log.txt 
  ff fe 20 00 20 00 20 00  4d 00 58 00 32 00 35 00  |.. . . .M.X.2.5.|
0010  4c 00 31 00 32 00 38 00  37 00 35 00 46 00 20 00  |L.1.2.8.7.5.F. .|
0020  20 00 20 00 20 00 49 00  44 00 3a 00 30 00 78 00  | . . .I.D.:.0.x.|
0030  43 00 32 00 32 00 30 00  31 00 38 00 20 00 20 00  |C.2.2.0.1.8. . .|
0040  20 00 20 00 53 00 69 00  7a 00 65 00 3a 00 20 00  | . .S.i.z.e.:. .|
0050  31 00 36 00 33 00 38 00  34 00 4b 00 42 00 20 00  |1.6.3.8.4.K.B. .|
0060  28 00 31 00 33 00 31 00  30 00 37 00 32 00 4b 00  |(.1.3.1.0.7.2.K.|
0070  62 00 29 00 20 00 20 00  20 00 20 00 20 00 20 00  |b.). . . . . . .|
0080  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
00c0  20 00 20 00 20 00 20 00  0d 00 0d 00 20 00 20 00  | . . . . . .|
00d0  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
0190  20 00 20 00 0d 00 0d 00  20 00 20 00 20 00 20 00  | . . . . . .|
01a0  4d 00 58 00 32 00 35 00  4c 00 36 00 34 00 37 00  |M.X.2.5.L.6.4.7.|
01b0  33 00 45 00 20 00 20 00  20 00 20 00 49 00 44 00  |3.E. . . . .I.D.|
01c0  3a 00 30 00 78 00 43 00  32 00 32 00 30 00 31 00  |:.0.x.C.2.2.0.1.|
01d0  37 00 20 00 20 00 20 00  20 00 53 00 69 00 7a 00  |7. . . . .S.i.z.|
01e0  65 00 3a 00 20 00 38 00  31 00 39 00 32 00 4b 00  |e.:. .8.1.9.2.K.|
01f0  42 00 20 00 28 00 36 00  35 00 35 00 33 00 36 00  |B. .(.6.5.5.3.6.|
0200  4b 00 62 00 29 00 20 00  20 00 20 00 20 00 20 00  |K.b.). . . . . .|
0210  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
0260  0d 00 0d 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . .|
0270  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
0320  20 00 20 00 20 00 20 00  20 00 20 00 0d 00 0d 00  | . . . . . .|
0330  53 00 50 00 49 00 20 00  42 00 41 00 52 00 3a 00  |S.P.I. .B.A.R.:.|
0340  20 00 46 00 45 00 30 00  31 00 30 00 30 00 30 00  | .F.E.0.1.0.0.0.|
0350  30 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  |0. . . . . . . .|
0360  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
03f0  20 00 20 00 20 00 20 00  0d 00 0d 00 20 00 20 00  | . . . . . .|
0400  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
04c0  20 00 20 00 0d 00 0d 00  46 00 50 00 54 00 20 00  | . .F.P.T. .|
04d0  4f 00 70 00 65 00 72 00  61 00 74 00 69 00 6f 00  |O.p.e.r.a.t.i.o.|
04e0  6e 00 20 00 53 00 75 00  63 00 63 00 65 00 73 00  |n. .S.u.c.c.e.s.|
04f0  73 00 66 00 75 00 6c 00  2e 00 20 00 20 00 20 00  |s.f.u.l... . . .|
0500  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
0590  0d 00 0d 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . .|
05a0  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
*
0650  20 00 20 00 20 00 20 00  20 00 20 00 0d 00 0d 00  | . . . . . .|
0660  0d 00 0a 00   ||
0664


[1] https://en.wikipedia.org/wiki/Byte_order_mark#UTF-16
[2] https://en.wikipedia.org/wiki/Newline

Thanks,

Andrew Fish


> 
> thank you,
> by krishna.
> 
> 
> 
> 
> 
> ___
> 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] [question] A piece of log.txt showed in uefi shell is not the same as showed in notepad++

[Andrew Fish via edk2-devel]

Krishna,

From an EFI point of view BackSpace 0x08, LF 0x0A, and CR 0xD are defined for a 
ConOut device. 

From Ascii Control Codes:
0x0C - Form Feed
0x0E - Shift Out

The Shift Out is used with graphics characters. 

How did you dump the log? If EFI sends data to a serial terminal then it would 
use terminal emulation and the output stream would look different. 

Thanks,

Andrew Fish

> On Jan 23, 2019, at 11:07 PM, krishnaLee  wrote:
> 
> Andrew Fish,
> thank you,in this case,you are right.
> but I think there may be other case that the user data just contain some 
> nosense-data
> such as { UINT16 MyData[]={0x000c,0x000d,0x000d,0x000e}  } to the editor, 
> the uefi editor may also meet this question,the editor may do not need output 
> this,
> but the editor should make sure other visible chars can output normal...
> 
> I don't know if it is a bug or not.
> 
> Thank you very much.
> krishna.
> 
> 
> 
> 
> 在 2019-01-24 12:13:50，"Andrew Fish"  写道：
>> On Jan 23, 2019, at 7:17 PM, krishnaLee > > wrote:
>> 
>> Hi,
>> I dumped a small log.txt from my work,but the log.txt showed in uefi 
>> shell,is not the same as it showed in notepad++.
>> I had upload the log here:
>> https://github.com/krishna116/test/blob/master/log.zip 
>> 
>> 
>> 
>> it seems the log showed in uefi shell had missed some strings...I don't know 
>> why,please help,
>> 
> 
> Krishna,
> 
> Your log.txt looks like a normal UTF-16 Unicode file. The leading  FF FE is 
> the Byte order mark for UTF-16 Little Endian [1].  The file looks like it has 
> CR line endings [2] so maybe that is confusing your editor? There seems to be 
> a CR LF at the end, so the mixture of line ending may even be more likely 
> confusing the editor. The byte order mark should let your editor know it is 
> 16-bit Unicode and the correct endian. Hope this helps...
> 
> $ hexdump -C /Users/andrewfish/Downloads/log/log.txt 
>   ff fe 20 00 20 00 20 00  4d 00 58 00 32 00 35 00  |.. . . .M.X.2.5.|
> 0010  4c 00 31 00 32 00 38 00  37 00 35 00 46 00 20 00  |L.1.2.8.7.5.F. .|
> 0020  20 00 20 00 20 00 49 00  44 00 3a 00 30 00 78 00  | . . .I.D.:.0.x.|
> 0030  43 00 32 00 32 00 30 00  31 00 38 00 20 00 20 00  |C.2.2.0.1.8. . .|
> 0040  20 00 20 00 53 00 69 00  7a 00 65 00 3a 00 20 00  | . .S.i.z.e.:. .|
> 0050  31 00 36 00 33 00 38 00  34 00 4b 00 42 00 20 00  |1.6.3.8.4.K.B. .|
> 0060  28 00 31 00 33 00 31 00  30 00 37 00 32 00 4b 00  |(.1.3.1.0.7.2.K.|
> 0070  62 00 29 00 20 00 20 00  20 00 20 00 20 00 20 00  |b.). . . . . . .|
> 0080  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 00c0  20 00 20 00 20 00 20 00  0d 00 0d 00 20 00 20 00  | . . . . . .|
> 00d0  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 0190  20 00 20 00 0d 00 0d 00  20 00 20 00 20 00 20 00  | . . . . . .|
> 01a0  4d 00 58 00 32 00 35 00  4c 00 36 00 34 00 37 00  |M.X.2.5.L.6.4.7.|
> 01b0  33 00 45 00 20 00 20 00  20 00 20 00 49 00 44 00  |3.E. . . . .I.D.|
> 01c0  3a 00 30 00 78 00 43 00  32 00 32 00 30 00 31 00  |:.0.x.C.2.2.0.1.|
> 01d0  37 00 20 00 20 00 20 00  20 00 53 00 69 00 7a 00  |7. . . . .S.i.z.|
> 01e0  65 00 3a 00 20 00 38 00  31 00 39 00 32 00 4b 00  |e.:. .8.1.9.2.K.|
> 01f0  42 00 20 00 28 00 36 00  35 00 35 00 33 00 36 00  |B. .(.6.5.5.3.6.|
> 0200  4b 00 62 00 29 00 20 00  20 00 20 00 20 00 20 00  |K.b.). . . . . .|
> 0210  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 0260  0d 00 0d 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . .|
> 0270  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 0320  20 00 20 00 20 00 20 00  20 00 20 00 0d 00 0d 00  | . . . . . .|
> 0330  53 00 50 00 49 00 20 00  42 00 41 00 52 00 3a 00  |S.P.I. .B.A.R.:.|
> 0340  20 00 46 00 45 00 30 00  31 00 30 00 30 00 30 00  | .F.E.0.1.0.0.0.|
> 0350  30 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  |0. . . . . . . .|
> 0360  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 03f0  20 00 20 00 20 00 20 00  0d 00 0d 00 20 00 20 00  | . . . . . .|
> 0400  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 04c0  20 00 20 00 0d 00 0d 00  46 00 50 00 54 00 20 00  | . .F.P.T. .|
> 04d0  4f 00 70 00 65 00 72 00  61 00 74 00 69 00 6f 00  |O.p.e.r.a.t.i.o.|
> 04e0  6e 00 20 00 53 00 75 00  63 00 63 00 65 00 73 00  |n. .S.u.c.c.e.s.|
> 04f0  73 00 66 00 75 00 6c 00  2e 00 20 00 20 00 20 00  |s.f.u.l... . . .|
> 0500  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 0590  0d 00 0d 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . .|
> 05a0  20 00 20 00 20 00 20 00  20 00 20 00 20 00 20 00  | . . . . . . . .|
> *
> 0650  20 00 20 00 20 00 20 00  20 00 20 00 0d 00 0d 00  | . . . . . ...

Re: [edk2] [PATCH edk2-staging 07/20] IntelUndiPkg/XGigUndiDxe: drop definition of gImageHandle

[Andrew Fish via edk2-devel]

> On Jan 30, 2019, at 8:05 AM, Ryszard Knop  
> wrote:
> 
> Hmm, is there a list/something I can generate to see which globals
> build tools emit? There are some more variables I'd happily get rid
> of, eg if I could drop gSystemTable and others.
> 

Ryszard,

You need to use libs by including the include file, and listing the lib in the 
INF. The library constructor initializes the globals. 

https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/DxeServicesTableLib.h
 

https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/UefiBootServicesTableLib.h
 

https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/UefiRuntimeServicesTableLib.h

For edk2 the library constructors get called prior calling your drivers entry 
point. 

Thanks,

Andrew Fish


> Reviewed-by: Ryszard Knop 
> 
> On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela wrote:
>> Remove duplicate definition of gImageHandle, which is emitted by
>> the build tools as well.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>> IntelUndiPkg/XGigUndiDxe/Init.c | 2 --
>> 1 file changed, 2 deletions(-)
>> 
>> diff --git a/IntelUndiPkg/XGigUndiDxe/Init.c
>> b/IntelUndiPkg/XGigUndiDxe/Init.c
>> index 84e06ea071c5..03e3942a1944 100644
>> --- a/IntelUndiPkg/XGigUndiDxe/Init.c
>> +++ b/IntelUndiPkg/XGigUndiDxe/Init.c
>> @@ -47,7 +47,6 @@ UINT16 mActiveChildren= 0;
>> EFI_EVENT  gEventNotifyExitBs;
>> EFI_EVENT  gEventNotifyVirtual;
>> 
>> -EFI_HANDLEgImageHandle;
>> EFI_SYSTEM_TABLE *gSystemTable;
>> 
>> EFI_GUID gEfiNiiPointerGuid = EFI_NII_POINTER_PROTOCOL_GUID;
>> @@ -502,7 +501,6 @@ InitializeXGigUndiDriver (
>> {
>>   EFI_STATUS Status;
>> 
>> -  gImageHandle  = ImageHandle;
>>   gSystemTable  = SystemTable;
>> 
>>   Status = EfiLibInstallDriverBinding (ImageHandle, SystemTable,
>> &gUndiDriverBinding, ImageHandle);
> 
> ___
> 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-staging 10/20] IntelUndiPkg/XGigUndiDxe: drop StdLibC library class reference

[Andrew Fish via edk2-devel]

> On Jan 30, 2019, at 9:26 AM, Ryszard Knop  
> wrote:
> 
> That's actually not quite correct - we need this package to build on
> IA32. It's named rather unfortunately, since it's not the EDK2 StdLibC,
> but rather a package in this repository - see IntelUndiPkg/LibC. It
> contains the bare minimum of functionality required to fix missing 64-
> bit math/shifts on IA32 and missing memcpy/memset intrinsics. We can't
> prevent MSVC from yielding memcpy/memset either, so this was the nasty
> solution for build issues. You have included CompilerIntrinsicsLib for
> the same reason, too :)
> 

Ryszard,

For IA32/X64 we avoid the compiler intrinsic libs via the coding standard. 
1) If you don't assign something too large at execution time with an = the 
compiler will not inline memcpy()/memset()
2) BaseLib.h has all the math functions that generate intrinsics that your code 
can call explicitly: 
https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/BaseLib.h#L3533
 


UINT64 X=0x1;
UINT64 Y=2;

So:
Y = X*Y;
should be:
Y = MultU64x64 (X, Y);

When ARM got added much later and some versions of ARM did not even have a 
divide instruction we gave up on trying to add more functions into all the 
existing IA32 code, and add the intrinsic lib. 

If we are going to add an intrinsic lib for x86 then we should probably add it 
to the MdePkg and it needs to support MSVC and GCC (as far as I can tell clang 
should work with the GCC intrinsics). 

Thanks,

Andrew Fish


> I'm not aware of any X64/IA32 equivalent of your CompilerIntrinsicsLib,
> but I'd be happy to be proven wrong here. I'm off for the rest of the
> week - I'll continue with reviews and merging early next week.
> 
> Thanks, Richard.
> 
> On Wed, 2018-11-14 at 18:33 -0800, ard.biesheuvela wrote:
>> StdLibc should not be used in drivers (it has dependencies on Shell
>> protocols), but in fact, we don't appear to rely on it in the first
>> place, so just drop the reference.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>> IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf | 1 -
>> 1 file changed, 1 deletion(-)
>> 
>> diff --git a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
>> b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
>> index beee8aa8134e..b5747565fbea 100644
>> --- a/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
>> +++ b/IntelUndiPkg/XGigUndiDxe/XGigUndiDxe.inf
>> @@ -132,7 +132,6 @@ GCC:*_*_*_CC_FLAGS = -DEFI32
>>   PrintLib
>>   UefiLib
>>   HiiLib
>> -  StdLibC
>> 
>> [LibraryClasses.X64]
>> 
> 
> ___
> 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] [RFC] Proposal to split Pkgs

[Andrew Fish via edk2-devel]

> On Jan 28, 2019, at 9:59 PM, Ni, Ray  wrote:
> 
> Hello,
> I'd like to propose to split today's BIG packages in following ways:
> 
> ==Overview =
> 
> 1. Separate Industry standard definitions from UEFI and PI interfaces.

Ray,

>From a big picture point of view lets talk about customer impact

Splitting the MdePkg means:
1) Every INF file needs a new  [Packages] layout, so has to change. 
2) Every DSC file needs to update [LibraryClasses] pathing.

Moving the drivers around:
1) Every DSC file has to change to update the path to the driver
2) Every FDF file has to change to update the path to the driver. 

I'd point out forcing every 3rd party module (library, driver, or application) 
to change is a much bigger impact that forcing a change to a projects target 
platform (DSC, FDF). 

I'm trying to figure out how to make a common code base that spans several 
generations of vendor code. So obviously today every module depends of MdePkg, 
and each new product is going to require new drivers for the chipset and 
peripherals. So lets say I'm a 3rd party graphics vendor does this mean I need 
and edk2 version of an INF and an edk3 version (no more MdePkg) of the INF as 
my customers are going to move at different rates? On the flip side how do I 
make a common platform tree if my vendor for work station chipsets lags behind 
my other chipset vendor in regards to moving to edk3. Does that mean I have to 
port that vendors code to edk3, which does not sound bad until you realize that 
every code drop from the vendor I need to remerge my edk3 changes back on top 
of it. Seems like a lot of work. 

Thus I think we need to ask the question do we need to leave the MdePkg around 
for compatibility? Do we need an INF syntax that supports edk2 and edk3 (Like 
the #ifdef we had that supported EDK and edk2 includes back in the day). We 
could have edk2 and edk3 INF files for very module?  Do we need to build a 
synthetic MdePkg that gets you the correct packages paths for the new include 
layout? I guess we could just make MdePkg an alias in the INF file for the 3 
(?) new packages? Or do we not change all the include paths?

Seems like we are creating our own Python 2.* vs. 3.* mess to make it easier to 
maintain the packages? I'm not saying it is wrong, but we should ask the 
question how is going to impact the edk2 consumers. 

Thanks,

Andrew Fish

> 2. Separate UEFI and PI interfaces from implementations.
>a. Separate UEFI and PI interfaces to different packages
>b. Separate PI PEI, DXE and MM phase interfaces to different packages
> 3. Separate different features into feature packages.
>Feature interface may be in the feature package to provide minimal
>common interface packages.
> 
> The POC code is in https://github.com/jyao1/edk2/tree/ReOrg.
> It basically split the EDKII common code to three directories:
> Core/, Device/, and Feature/.
> The code is in very early POC phase and only code in Core/ directory
> can pass the build.
> I would like to gather feedbacks through this RFC to see whether
> this splitting direction makes sense.
> Is there any other better ways of splitting?
> Or perhaps do not split in such a small granularity?
> Or perhaps Mike's work to move lib-c content to edk2-libc repo,
> to move real platform code to edk2-platform repo is enough for
> now?
> 
> ==More explanations =
> 
> There are 9 packages inside Core/ directory:
> 1. BasePkg
> Contains industry standard definitions (exclude UEFI and PI) and base
> libraries that non-UEFI and non-PI development can depend on.
> UEFI or PI development can also depend on this package.
> 2. UefiPkg
> Contains UEFI interfaces and libraries that UEFI driver-model driver
> development can depend on.
> 3. PiPeiPkg
> Contains PI interfaces and libraries for PEI phase that PEI module
> development can depend on.
> 4. PiDxePkg
> Contains PI interfaces and libraries for DXE phase that DXE module
> development can depend on.
> 5. PiMmPkg
> Contains PI interfaces and libraries for MM phase that MM/SMM
> module development can depend on.
> 6. MdeModulePkg (TianoPkg? Name is still TBD)
> Contains Tiano implementation specific interfaces and libraries.
> Developing modules for pure UEFI or PI should not depend on this package.
> 7. PeiFoundationPkg
> Contains the PEI foundation modules (PeiCore and DxeIpl) and supported
> libraries.
> 8. DxeFoundationPkg
> Contains the DXE foundation modules (DxeCore and RuntimeDxe) and
> supported libraries.
> 9. SmmFoundationPkg
> Contains the SMM foundation modules (SmmCore, SmmIpl and
> SmmCommunicationBuffer) and supported libraries.
> 
> These packages are positioned in different layers. The package in higher
> layer depends on all packages that are in lower layers.
> Layer 0: BasePkg.
> Layer 1: UefiPkg.
> Layer 2: PiPeiPkg 
> Layer 3: PiDxePkg
> Layer 4: PiMmPkg
> Layer 5: MdeModulePkg (TianoPkg?)
> 
> All other modules are put to small

Re: [edk2] [PATCH v4 edk2-platforms 01/23] Silicon/Broadcom/Bcm282x: Add interrupt driver

[Andrew Fish via edk2-devel]

> On Jan 31, 2019, at 11:57 AM, Leif Lindholm  wrote:
> 
> +Andrew, Laszlo, Mike.
> 
> On Thu, Jan 31, 2019 at 06:19:48PM +0100, Ard Biesheuvel wrote:
>> On Thu, 31 Jan 2019 at 16:24, Leif Lindholm  wrote:
>>> 
>>> On Tue, Jan 29, 2019 at 04:26:33PM +, Pete Batard wrote:
 Contributed-under: TianoCore Contribution Agreement 1.1
 Signed-off-by: Pete Batard 
 
 Reviewed-by: Ard Biesheuvel 
 ---
 Silicon/Broadcom/Bcm283x/Bcm283x.dec   |  23 ++
 Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.c   | 367 
 
 Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf |  48 +++
 Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h|  72 
>>> 
>>> Another generic comment: "IndustryStandard" is something like ACPI,
>>> SMBIOS, PCI, USB, MMC, ... (also including SoC/platform-specific
>>> additions to the same).
>> 
>> Is that your interpretation? Or is this documented somewhere?
> 
> Only in asmuch as it is a clearly descriptive name.
> 
>> I could live with Chipset/, and I'm open to other suggestions, but the
>> Library vs Protocol vs IndustryStandard distinction is very useful
>> imo.
> 
> It is useful because it is descriptive.
> Pretending that an SoC hardware description or a platform description
> header is an "Industry Standard" is disingenuous.
> 
>>> I would be more comfortable with SoC-specific and Platform-specific
>>> include files living directly in Include/.
>> 
>> No, don't drop headers in Include/ please. The namespacing is one of
>> the things EDK2 actually gets right (assuming you define the paths
>> correctly in the package .dec file), and I'd hate to start dumping
>> headers at the root level because we cannot make up our minds what to
>> call the enclosing folder.
> 
> Mike, Andrew - what is your take on this?
> Is there a formal definition of not only what goes in
> IndustryStandard, but where chipset and platform headers should live
> in the namespace?
> 

Leif,

I kind of think IndustryStandard as things that have a public spec I don't know 
if we have a pedantic definition of what that means. 

I'm with Ard on that I like the current Include structure for the generic 
packages and global definitions. 

What seems to work poorly in our packaging scheme is the intersection of 
platforms and chipsets. Everything works OK is you just use PCDs, but if you 
try to use includes it get complicated. I guess that Library instances can help 
here too. An IP block for an SoC could be another example, do we  need a 
package for each IP block? Thus any SoC that uses that IP block can depend on 
that package? If you don't do that you end up copies of that IP block 
definition in every SoC package. In a more traditional C world grabbing 
includes is just a matter of adding an include search path to the build, but in 
the edk2 it is about having the proper package definitions, and the set of 
packages are somewhat fixed in the INF files. But I guess after all this 
rambling I'm really saying it is hard to figure out what package to put a given 
include in. I'm not sure that impacts pathing in a single package?

I think the scope of the package matters too as if you have a package to 
support a chipset (SoC) do you really need Include/Chipset or Include/Soc? As 
long as you have the Guid, Library, Ppi, Protocol, and IndustryStandard the 
package can dump everything in /Include or build some directory hierarchy that 
makes sense. But then again punting on what this hierarchy seems to not answer 
your question. I guess if the package has lots of functionality adding Include 
directory structure makes sense, if it is a smaller focused package it seems 
like having an extra directory for the main focus of the package could be 
redundant. 

Thanks,

Andrew Fish


> Regards,
> 
> Leif
> 
 4 files changed, 510 insertions(+)
 
 diff --git a/Silicon/Broadcom/Bcm283x/Bcm283x.dec 
 b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
 new file mode 100644
 index ..d193da4c0e1e
 --- /dev/null
 +++ b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
 @@ -0,0 +1,23 @@
 +## @file
 +#
 +#  Copyright (c) 2019, Pete Batard 
 +#
 +#  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.
 +#
 +##
 +
 +[Defines]
 +  DEC_SPECIFICATION  = 0x0001001A
 +  PACKAGE_NAME   = Bcm283xPkg
 +  PACKAGE_GUID   = 900C0F44-1152-4FF9-B9C5-933E2918C831
 +  PACKAGE_VERSION= 1.0
 +
>

Re: [edk2] Self-replicating image

[Andrew Fish via edk2-devel]

> On Feb 8, 2019, at 6:42 AM, Tomas Pilar (tpilar)  
> wrote:
> 
> Hi,
> 
> I am currently pondering the most elegant way to implement capsule update for 
> our devices that would work in the presence of multiple devices in the host.
> 
> Capsule allows embedding a driver that is executed prior to the update, which 
> is very handy. Crypto library is quite large and would not fit into an 
> OptionROM, so being able to supply FMP driver in the capsule is great.
> 
> However, if only one instance of the driver loads, the FMP upstream is 
> currently written to support only one device per instance. So I wonder if 
> there is a easy, neat way for my image to replicate on DriverBinding so that 
> I end up with one instance per device.
> 


Tom,

The usually model in EFI is to have one driver handle multiple things. 

> It looks like I should be able to do it with gBS->LoadImage() and passing 
> information about currently loaded image though I might have to CopyMem() the 
> image itself to new location.
> 

gBS->LoadImage() will load and relocate the image to a malloced address of the 
correct memory type for the image. The inputs are just the source of the image, 
so no need to CopyMem() anything. gBS->StartImage() calls the entry point.

Thanks,

Andrew Fish

> Thoughts?
> 
> Cheers,
> Tom
> ___
> 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] Hello Andrew query on BdsSetMemoryTypeInformationVariable

[Andrew Fish via edk2-devel]

Forwarding to the edk2 list 

> On Feb 8, 2019, at 8:28 AM, galla rao  wrote:
> 
> Hi Andrew,
> 
> Am sorry for direct message!
>  
> There is a function BdsSetMemoryTypeInformationVariable which causes a reset 
> when i enabled Secureboot related drivers
> 
> Any clue why this function is added in EDK2?
> 

Yea it writes a variable that records how many pages of each memory type are 
used. This variable is read in PEI and used to pass a HOB into the DXE Core. 
The DXE Core uses these memory buckets to preallocate ranges for each memory 
type. This scheme prevents memory fragmentation and makes sure the runtime 
memory regions are in the same location when the system does an S4 resume from 
disk. 


> is this a serious error, making the PcdResetOnMemoryTypeInformationChange to 
> FALSE will resolve and boots to OS
> 

I think the idea behind that reboot, is the memory map could be different on 
the next boot and if that was an S4 the S4 could fail. 

Thanks,

Andrew Fish


> shed some knowledge if you are aware of this feature
> 
> Thanks & Regards
> Galla

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

Re: [edk2] [edk2-announce] Community Meeting Minutes

[Andrew Fish via edk2-devel]

> On Feb 8, 2019, at 9:33 AM, Rebecca Cran via edk2-devel 
>  wrote:
> 
> 
> On February 8, 2019 at 2:01:59 AM, Laszlo Ersek 
> (ler...@redhat.com(mailto:ler...@redhat.com)) wrote:
> 
>> I don't see the workflow modification as viable. The "patch series"
>> concept is integral to every single open source project that I've ever
>> worked with. The evolution of a feature or a bug fix over a series of
>> patches is a core facet of programming and reviewing. It communicates a
>> thinking process, and that's what programming is about.  
> 
> I don’t recall coming across the patch series (e.g. the 1/5 email patches) in 
> other projects. In other projects people post a single patch and then update 
> it following feedback on the same review. This can be either in a single, 
> rebased commit, or new commits on a bug/feature branch - review systems deal 
> with both.
> 

Rebecca,

I think the patch workflow is kind of like a coding standards. Some folks 
advocate for lots of small patches (common in open source projects), and some 
folks advocate for a patch per bug. I think the biggest upside to the patch 
granularity is it is much easier to bisect a failure. 

So I've used Bitbucket with a branch per commit (you name your branch with a 
standard pattern and the bugzilla  ) model and if your branch has a patch 
series (set of commits) you can view each commit independently from the UI and 
the default view is the entire patch series. So you can see both. 

Thanks,

Andrew Fish


>> So how long do we wait?
>> 
> 
> 
> Good point!  
> 
>> 
>> 
>> What I find practical at this moment is what Stephano has been working
>> on (thank you for all that Stephano) -- collect & file official
>> improvement requests with GitHub, and then see how those things are
>> addressed. In my opinion (not having seen Gerrit anyway, which remains
>> to be evaluated, but not by me), GitHub is the direct runner up to the
>> mailing list, so improving GitHub would be the most practical. In
>> particular I envision the context improvements for the GitHub email
>> notifications as something very doable for GitHub.  
> 
> 
> 
> 
> 
> I’d certainly be happy to use Github, but I do worry about tieing ourselves 
> to such a closed system.
> 
> 
> 
> 
> 
> 
> Rebecca
> 
> ___
> 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] Missing PI definitions?

[Andrew Fish via edk2-devel]

Liming,

Sorry I guess I was confusing this with EFI_RESOURCE_ATTRIBUTE_TESTED. 

I'll a little confused that it is implementation given it is passed into 
gDS->AddMemorySpace() the PI Spec defines the values of Capabilities to be 
defined in the UEFI Spec. as the GetMemoryMap() attributes. It is not clear 
that the implementation actually owns these bits? Almost feels like we should 
update the PI spec to include these #defines. 

I seem to remember we have been using these bits for a long time.

Thanks,

Andrew Fish

> On Dec 4, 2018, at 4:19 PM, Gao, Liming  wrote:
> 
> Andrew:
>  UEFI spec doesn't define them. They are the implement related definitions. 
> They are not required to be exposed to OS. We can add one header file in 
> MdeModulePkg to share them between DxeCore and MemoryTest driver. Besides, 
> ECP package will be retired. There is no change for it. 
> 
> #define EFI_MEMORY_PRESENT  0x0100ULL
> #define EFI_MEMORY_INITIALIZED  0x0200ULL
> #define EFI_MEMORY_TESTED   0x0400ULL
> 
> Thanks
> Liming
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org 
>> ] On Behalf Of
>> Andrew Fish

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

Re: [edk2] [PATCH edk2-platforms] Platform/AMD/OverdriveBoard: fix byte order of default MAC addresses

[Andrew Fish via edk2-devel]

> On Dec 10, 2018, at 3:33 PM, Leif Lindholm  wrote:
> 
> On Mon, Dec 10, 2018 at 11:51:43PM +0100, Ard Biesheuvel wrote:
>> On Mon, 10 Dec 2018 at 23:14, Leif Lindholm  wrote:
>>> 
>>> On Wed, Dec 05, 2018 at 09:10:48PM +0100, Ard Biesheuvel wrote:
 The PCDs containing the default MAC addresses are of type UINT64,
 and so the byte order needs to be inverted. As they are currently,
 both default MAC addresses are invalid since they have the multicast
 bit set.
>>> 
>>> Ah, oops.
>>> That would also prevent them from being "locally administered" and
>>> hence permissible without registering an entry in the OUI.
>>> 
>>> However, to reduce someone interpreting the _new_ values that way
>>> instead, could you do one of:
>>> - Adding a comment explaining these are in reverse byte order.
>>> or
>>> - Convert them to the new Array type PCD[1], to make the bytes appear
>>>  in natural order?
>>> 
>>> [1] edk2 72a1d77694d51914c0dd6aa97dbfa58634b0a4a5
>> 
>> How does one refer to such a PCD from C code?
> 
> Good question :)
> 

Seems like you would need cast the pointer to the data structure in the C code. 
If I understand correctly the change in the PCD creation code is you can now 
better structure the data, but it is no different on the consumer end and you 
got a blob of bytes in both cases. 

Thanks,

Andrew Fish

> /
>Leif
> 
 Contributed-under: TianoCore Contribution Agreement 1.1
 Signed-off-by: Ard Biesheuvel 
 ---
 Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc 
 b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
 index 05433d4472e8..2843e51f93f7 100644
 --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
 +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
 @@ -469,8 +469,8 @@ DEFINE DO_CAPSULE   = FALSE
   gAmdModulePkgTokenSpaceGuid.PcdPort1NetSpeed|1
 
 [PcdsDynamicDefault.common]
 -  gAmdStyxTokenSpaceGuid.PcdEthMacA|0x02A1A2A3A4A5
 -  gAmdStyxTokenSpaceGuid.PcdEthMacB|0x02B1B2B3B4B5
 +  gAmdStyxTokenSpaceGuid.PcdEthMacA|0xA5A4A3A2A102
 +  gAmdStyxTokenSpaceGuid.PcdEthMacB|0xB5B4B3B2B102
 
 [PcdsPatchableInModule]
   gAmdModulePkgTokenSpaceGuid.PcdXgbeUseMacFromIscp|TRUE
 --
 2.19.2
 
> ___
> 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