Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Yao, Jiewen]

I am OK on that.

> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, November 22, 2017 3:57 PM
> To: Laszlo Ersek ; Wang, Jian J 
> Cc: edk2-devel@lists.01.org; Yao, Jiewen ; Zeng, Star
> 
> Subject: RE: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> How about we have the v6 patch series in first with the feedback from Jiewen
> (about comments) and you (about MemoryMapStart) addressed?
> 
> Then we can have a separated patch for the merging.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
> >> v7:
> >>   Merge memory map after filtering paging attributes
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> > some old OSs. This patch will fix this issue to keep OS compatibility
> > as much as possible.
> >
> > Jian J Wang (2):
> >   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.h  | 18 ++
> >  MdeModulePkg/Core/Dxe/Mem/Page.c | 21 +++
> >  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 94
> +---
> >  4 files changed, 112 insertions(+), 22 deletions(-)
> >
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this 
> because I
> didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this because 
> I've
> put a lot of BZ writeup, and patch review and testing effort for this series, 
> and I'd
> like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a separate
> patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't think 
> I can
> find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it will 
> provide
> the desired result for the code as well.
> 
> 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 v7 0/2] Fix multiple entries of RT_CODE in memory map

[Zeng, Star]

How about we have the v6 patch series in first with the feedback from Jiewen 
(about comments) and you (about MemoryMapStart) addressed?

Then we can have a separated patch for the merging.


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Tuesday, November 21, 2017 9:38 PM
To: Wang, Jian J 
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Jian,

On 11/21/17 07:17, Jian J Wang wrote:
>> v7:
>>   Merge memory map after filtering paging attributes
> 
> More than one entry of RT_CODE memory might cause boot problem for 
> some old OSs. This patch will fix this issue to keep OS compatibility 
> as much as possible.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h  | 18 ++
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 21 +++
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 
> +---
>  4 files changed, 112 insertions(+), 22 deletions(-)
> 

I don't have capacity to retest and re-review the series.

Considering the following two options, I like none of them:

(1) Version 7 is merged with my feedback tags from v6. I don't like this 
because I didn't review or test version 7.

(2) Version 7 is merged without my feedback tags. I don't like this because 
I've put a lot of BZ writeup, and patch review and testing effort for this 
series, and I'd like the commit log to reflect that.


Instead, I would like to request the following, for v8:

Please submit a series that consists of three patches:

- patch v8 1/3: identical to v6 1/2, except for the code comment update,
- patch v8 2/3: identical to v6 2/2,
- patch v8 3/3: please implement the merging of the memory map as a separate 
patch.

Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my Reviewed-by 
tags, from v6.

Patch v8 3/3 should be reviewed / tested separately by others. I don't think I 
can find the capacity for that at the moment.

This approach will correctly reflect all the work done thus far, and it will 
provide the desired result for the code as well.

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 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Wang, Jian J 
Sent: Thursday, November 16, 2017 3:27 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric ; Yao, Jiewen ; Zeng, 
Star ; Laszlo Ersek ; Ard Biesheuvel 

Subject: [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in 
memory map

> v6:
>Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP

> v5:
>Coding style clean-up

> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>a logic hole
> b. Add warning message if failed to update capability c. Add local 
> variable to hold new attributes to make code cleaner

> v3:
> a. Add comment to explain more on updating memory capabilities b. Fix 
> logic hole in updating attributes c. Instead of checking illegal 
> memory space address and size, use return
>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>cannot be updated with new capabilities.

> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
>than a page.

More than one entry of RT_CODE memory might cause boot problem for some old 
OSs. This patch will fix this issue to keep OS compatibility as much as 
possible.

More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++-
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..3297c1900b 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -769,6 +769,20 @@ AssignMemoryPageAttributes (
   return Status;
 }
 
+/**
+ Check if Execute Disable feature is enabled or not.
+**/
+BOOLEAN
+IsExecuteDisableEnabled (
+  VOID
+  )
+{
+  MSR_CORE_IA32_EFER_REGISTERMsrEfer;
+
+  MsrEfer.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
+  return (MsrEfer.Bits.NXE == 1);
+}
+
 /**
   Update GCD memory space attributes according to current page table setup.
 **/
@@ -790,7 +804,7 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64  PageStartAddress;
   UINT64  Attributes;
   UINT64  Capabilities;
-  BOOLEAN DoUpdate;
+  UINT64  NewAttributes;
   UINTN   Index;
 
   //
@@ -802,17 +816,50 @@ RefreshGcdMemoryAttributesFromPaging (
 
   GetCurrentPagingContext ();
 
-  DoUpdate  = FALSE;
-  Capabilities  = 0;
-  Attributes= 0;
-  BaseAddress   = 0;
-  PageLength= 0;
+  Attributes  = 0;
+  NewAttributes   = 0;
+  BaseAddress = 0;
+  PageLength  = 0;
+
+  if (IsExecuteDisableEnabled ()) {
+Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;  } 
+ else {
+Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP;  }
 
   for (Index = 0; Index < NumberOfDescriptors; Index++) {
 if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
   continue;
 }
 
+//
+// Sync the actual paging related capabilities back to GCD service first.
+// As a side effect (good one), this can also help to avoid unnecessary
+// memory map entries due to the different capabilities of the same type
+// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+// which could cause boot failure of some old Linux distro (before v4.3).
+//
+Status = gDS->SetMemorySpaceCapabilities (
+MemorySpaceMap[Index].BaseAddress,
+MemorySpaceMap[Index].Length,
+MemorySpaceMap[Index].Capabilities | Capabilities
+);
+if (EFI_ERROR (Status)) {
+  //
+  // If we cannot udpate the capabilities, we cannot update its
+  // attributes either. So just simply skip current block of memory.
+  //
+  DEBUG ((
+DEBUG_WARN,
+"Failed to update capability: [%lu] %016lx - %016lx (%016lx -> 
%016lx)\r\n",
+(UINT64)Index, MemorySpaceMap[Index].BaseAddress,
+MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[Index].Length - 1,
+MemorySpaceMap[Index].Capabilities,
+MemorySpaceMap[Index].Capabilities | Capabilities
+));
+  continue;
+}
+
 if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
   //
   // Current memory space starts at a new page. Resetting PageLength will 
@@ -826,7 +873,9 @@ RefreshGcdMemoryAttributesFromPaging (
   

Re: [edk2] [Patch] MdeModulePkg: Free NET_BUF data after it is sent out to avoid memory leak

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 



> -Original Message-
> From: Wang, Fan
> Sent: Wednesday, November 22, 2017 2:56 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Ye, Ting ; Fu,
> Siyuan ; Wang, Fan 
> Subject: [Patch] MdeModulePkg: Free NET_BUF data after it is sent out to
> avoid memory leak
> 
> When build a DHCP message in function DhcpSendMessage() or
> DhcpRetransmit(),
> a new NET_BUF is created by the library of NetbufFromExt, but it's not
> freed
> after it is sent out. This patch is to fix this memory leak issue.
> 
> Cc: Jiaxin Wu 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan 
> ---
>  MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
> b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
> index 3898223..d90dc34 100644
> --- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
> +++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
> @@ -1397,23 +1397,22 @@ DhcpSendMessage (
>  EndPoint.LocalAddr.Addr[0]  = DhcpSb->ClientAddr;
>  UdpIo   = DhcpSb->LeaseIoPort;
>}
> 
>ASSERT (UdpIo != NULL);
> -  NET_GET_REF (Wrap);
> -
> +
>Status = UdpIoSendDatagram (
>   UdpIo,
>   Wrap,
>   ,
>   NULL,
>   DhcpOnPacketSent,
>   DhcpSb
>   );
> 
>if (EFI_ERROR (Status)) {
> -NET_PUT_REF (Wrap);
> +NetbufFree (Wrap);
>  return EFI_ACCESS_DENIED;
>}
> 
>return EFI_SUCCESS;
>  }
> @@ -1475,22 +1474,21 @@ DhcpRetransmit (
>  UdpIo   = DhcpSb->LeaseIoPort;
>}
> 
>ASSERT (UdpIo != NULL);
> 
> -  NET_GET_REF (Wrap);
>Status = UdpIoSendDatagram (
>   UdpIo,
>   Wrap,
>   ,
>   NULL,
>   DhcpOnPacketSent,
>   DhcpSb
>   );
> 
>if (EFI_ERROR (Status)) {
> -NET_PUT_REF (Wrap);
> +NetbufFree (Wrap);
>  return EFI_ACCESS_DENIED;
>}
> 
>return EFI_SUCCESS;
>  }
> --
> 1.9.5.msysgit.1

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

Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities

[Zeng, Star]

Reviewed-by: Star Zeng  if the feedback from Jiewen (about 
comments) and Laszlo (about MemoryMapStart) has been addressed, and the merging 
will be done in a separated patch.


Thanks,
Star
-Original Message-
From: Wang, Jian J 
Sent: Friday, November 17, 2017 10:49 AM
To: Yao, Jiewen ; edk2-devel@lists.01.org
Cc: Zeng, Star ; Laszlo Ersek ; Ard 
Biesheuvel ; Kinney, Michael D 

Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging 
capabilities

1) Sure. I'll update the wording.
2) I still think this is just a workaround . In the long run, I don't think the 
merge is a good idea. It looks like it will "fix" more issues, but actually it 
just "hide" them and would cause more and more workaround in the future. 
Anyway, if no one else has objections, I'll update the code.

> -Original Message-
> From: Yao, Jiewen
> Sent: Friday, November 17, 2017 9:37 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Laszlo Ersek 
> ; Ard Biesheuvel 
> Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all 
> paging capabilities
> 
> HI
> I have 2 comments:
> 
> 1) I do not think we need mention: WORKAROUND.
> I suggest we just use "NOTE".
> 
> We have similar example before, see
> MdePkg\Library\BasePeCoffLib\BasePeCoff.c
>   //
>   // NOTE: Some versions of Linux ELILO for Itanium have an incorrect 
> magic value
>   //   in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
>   //   Magic value in the OptionalHeader is
> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>   //   then override the returned value to
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>   //
> 
> 2) I agree with Star. I think we should merge the final result.
> The suggestion before is: *Keep current UEFI memory map unchanged.* 
> Changing it brings lots of risk without validating all UEFI OS.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Thursday, November 16, 2017 3:27 PM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Zeng, Star 
> > ; Laszlo Ersek ; Ard 
> > Biesheuvel
> 
> > Subject: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging
> capabilities
> >
> > Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really set 
> > attributes and change memory paging attribute accordingly.
> > But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by value 
> > from Capabilities in GCD memory map. This might cause boot problems. 
> > Clearing all paging related capabilities can workaround it. The code 
> > added in this patch is supposed to be removed once the usage of 
> > EFI_MEMORY_DESCRIPTOR.Attribute is clarified in UEFI spec and 
> > adopted by both EDK-II Core and all supported OSs.
> >
> > Cc: Jiewen Yao 
> > Cc: Star Zeng 
> > Cc: Laszlo Ersek 
> > Cc: Ard Biesheuvel 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index c9219cc068..783b576e35 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> >//
> >BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
> >
> > +  //
> > +  // WORKAROUND: Some OSs will treat
> EFI_MEMORY_DESCRIPTOR.Attribute
> > as really
> > +  // set attributes and change memory paging attribute
> > accordingly.
> > +  // But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned
> > by
> > +  // value from Capabilities in GCD memory map. This might
> > cause
> > +  // boot problems. Clearing all paging related capabilities 
> > can
> > +  // workaround it. Following code is supposed to be removed
> > once
> > +  // the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified
> > in
> > +  // UEFI spec and adopted by both EDK-II Core and all
> > supported
> > +  // OSs.
> > +  //
> > +  while (MemoryMapStart < MemoryMap) {
> > +MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> > EFI_MEMORY_RO |
> > +   EFI_MEMORY_XP);
> > +MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart,
> > Size);
> > +  }
> > +
> >Status = EFI_SUCCESS;
> >
> >  Done:
> > --
> > 2.14.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org

[edk2] [Patch] MdeModulePkg: Free NET_BUF data after it is sent out to avoid memory leak

[fanwang2]

When build a DHCP message in function DhcpSendMessage() or DhcpRetransmit(),
a new NET_BUF is created by the library of NetbufFromExt, but it's not freed
after it is sent out. This patch is to fix this memory leak issue.

Cc: Jiaxin Wu 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c 
b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
index 3898223..d90dc34 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
@@ -1397,23 +1397,22 @@ DhcpSendMessage (
 EndPoint.LocalAddr.Addr[0]  = DhcpSb->ClientAddr;
 UdpIo   = DhcpSb->LeaseIoPort;
   }
 
   ASSERT (UdpIo != NULL);
-  NET_GET_REF (Wrap);
-  
+
   Status = UdpIoSendDatagram (
  UdpIo, 
  Wrap, 
  , 
  NULL, 
  DhcpOnPacketSent, 
  DhcpSb
  );
 
   if (EFI_ERROR (Status)) {
-NET_PUT_REF (Wrap);
+NetbufFree (Wrap);
 return EFI_ACCESS_DENIED;
   }
 
   return EFI_SUCCESS;
 }
@@ -1475,22 +1474,21 @@ DhcpRetransmit (
 UdpIo   = DhcpSb->LeaseIoPort;
   }
 
   ASSERT (UdpIo != NULL);
 
-  NET_GET_REF (Wrap);
   Status = UdpIoSendDatagram (
  UdpIo,
  Wrap,
  ,
  NULL,
  DhcpOnPacketSent,
  DhcpSb
  );
 
   if (EFI_ERROR (Status)) {
-NET_PUT_REF (Wrap);
+NetbufFree (Wrap);
 return EFI_ACCESS_DENIED;
   }
 
   return EFI_SUCCESS;
 }
-- 
1.9.5.msysgit.1

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

Re: [edk2] [PATCH] Add NetworkPkg maintainer as MdeModulePkg/Universal/Network maintainer

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 



> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, November 22, 2017 2:34 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Fu, Siyuan ; Wu,
> Jiaxin 
> Subject: [PATCH] Add NetworkPkg maintainer as
> MdeModulePkg/Universal/Network maintainer
> 
> Cc: Siyuan Fu 
> Cc: Jiaxin Wu 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  Maintainers.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 3930ff07a90d..ffb69c17fe2e 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -167,6 +167,7 @@ MdeModulePkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
>  M: Star Zeng 
>  M: Eric Dong 
> +M: NetworkPkg maintainers (Universal/Network)
> 
>  MdePkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdePkg
> --
> 2.7.0.windows.1

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

Re: [edk2] [PATCH] Add NetworkPkg maintainer as MdeModulePkg/Universal/Network maintainer

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu 


> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, November 22, 2017 2:34 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Fu, Siyuan ; Wu,
> Jiaxin 
> Subject: [PATCH] Add NetworkPkg maintainer as
> MdeModulePkg/Universal/Network maintainer
> 
> Cc: Siyuan Fu 
> Cc: Jiaxin Wu 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  Maintainers.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 3930ff07a90d..ffb69c17fe2e 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -167,6 +167,7 @@ MdeModulePkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
>  M: Star Zeng 
>  M: Eric Dong 
> +M: NetworkPkg maintainers (Universal/Network)
> 
>  MdePkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdePkg
> --
> 2.7.0.windows.1

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

Re: [edk2] [Patch] NetworkPkg/HttpDxe: Fix the incorrect SizeofHeaders in HttpTcpReceiveHeader().

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 

> -Original Message-
> From: Wu, Jiaxin
> Sent: Wednesday, November 22, 2017 2:36 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan ; Wu,
> Jiaxin 
> Subject: [Patch] NetworkPkg/HttpDxe: Fix the incorrect SizeofHeaders in
> HttpTcpReceiveHeader().
> 
> Commit 19bd133562df951ae7ff7e1fff99b11a25b4cb6d is to fix the incorrect
> SizeofHeaders
> returned from HttpTcpReceiveHeader(). But it missed the "\r\n\r\n"
> calculation, which
> will cause the later HttpHeaders parsing failure.
> 
> This patch is fix the above issue.
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin 
> ---
>  NetworkPkg/HttpDxe/HttpProto.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c
> b/NetworkPkg/HttpDxe/HttpProto.c
> index 1aa1816..925281a 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -1875,13 +1875,10 @@ HttpTcpReceiveHeader (
> 
>//
>// Check whether we received end of HTTP headers.
>//
>*EndofHeader = AsciiStrStr (*HttpHeaders, HTTP_END_OF_HDR_STR);
> -  if (*EndofHeader != NULL) {
> -*SizeofHeaders = *EndofHeader - *HttpHeaders;
> -  }
>  };
> 
>  //
>  // Free the buffer.
>  //
> @@ -1977,13 +1974,10 @@ HttpTcpReceiveHeader (
> 
>//
>// Check whether we received end of HTTP headers.
>//
>*EndofHeader = AsciiStrStr (*HttpHeaders, HTTP_END_OF_HDR_STR);
> -  if (*EndofHeader != NULL) {
> -*SizeofHeaders = *EndofHeader - *HttpHeaders;
> -  }
>  };
> 
>  //
>  // Free the buffer.
>  //
> @@ -2000,11 +1994,13 @@ HttpTcpReceiveHeader (
>}
> 
>//
>// Skip the CRLF after the HTTP headers.
>//
> -  *EndofHeader = *EndofHeader + AsciiStrLen (HTTP_END_OF_HDR_STR);
> +  *EndofHeader = *EndofHeader + AsciiStrLen (HTTP_END_OF_HDR_STR);
> +
> +  *SizeofHeaders = *EndofHeader - *HttpHeaders;
> 
>return EFI_SUCCESS;
>  }
> 
>  /**
> --
> 1.9.5.msysgit.1

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

[edk2] [Patch] NetworkPkg/HttpDxe: Fix the incorrect SizeofHeaders in HttpTcpReceiveHeader().

[Jiaxin Wu]

Commit 19bd133562df951ae7ff7e1fff99b11a25b4cb6d is to fix the incorrect 
SizeofHeaders
returned from HttpTcpReceiveHeader(). But it missed the "\r\n\r\n" calculation, 
which
will cause the later HttpHeaders parsing failure.

This patch is fix the above issue.

Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin 
---
 NetworkPkg/HttpDxe/HttpProto.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 1aa1816..925281a 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -1875,13 +1875,10 @@ HttpTcpReceiveHeader (
 
   //
   // Check whether we received end of HTTP headers.
   //
   *EndofHeader = AsciiStrStr (*HttpHeaders, HTTP_END_OF_HDR_STR);
-  if (*EndofHeader != NULL) {
-*SizeofHeaders = *EndofHeader - *HttpHeaders;
-  }
 };
 
 //
 // Free the buffer.
 //
@@ -1977,13 +1974,10 @@ HttpTcpReceiveHeader (
 
   //
   // Check whether we received end of HTTP headers.
   //
   *EndofHeader = AsciiStrStr (*HttpHeaders, HTTP_END_OF_HDR_STR); 
-  if (*EndofHeader != NULL) {
-*SizeofHeaders = *EndofHeader - *HttpHeaders;
-  }
 };
 
 //
 // Free the buffer.
 //
@@ -2000,11 +1994,13 @@ HttpTcpReceiveHeader (
   } 
 
   //
   // Skip the CRLF after the HTTP headers.
   //
-  *EndofHeader = *EndofHeader + AsciiStrLen (HTTP_END_OF_HDR_STR);  
+  *EndofHeader = *EndofHeader + AsciiStrLen (HTTP_END_OF_HDR_STR);
+
+  *SizeofHeaders = *EndofHeader - *HttpHeaders;
 
   return EFI_SUCCESS;
 }
 
 /**
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH] Add NetworkPkg maintainer as MdeModulePkg/Universal/Network maintainer

[Star Zeng]

Cc: Siyuan Fu 
Cc: Jiaxin Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 Maintainers.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 3930ff07a90d..ffb69c17fe2e 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -167,6 +167,7 @@ MdeModulePkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
 M: Star Zeng 
 M: Eric Dong 
+M: NetworkPkg maintainers (Universal/Network)
 
 MdePkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/MdePkg
-- 
2.7.0.windows.1

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

Re: [edk2] [PATCH] MdeModulePkg UhciPei: Support IoMmu

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 21, 2017 5:01 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] MdeModulePkg UhciPei: Support IoMmu
> 
> Update the UhciPei driver to consume IOMMU_PPI to allocate DMA buffer.
> 
> If no IOMMU_PPI exists, this driver still calls PEI service to allocate
> DMA buffer, with assumption that DRAM==DMA.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c| 251
> +++
>  MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.c   | 398
> +++
>  MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.h   | 164 -
>  MdeModulePkg/Bus/Pci/UhciPei/UhciPei.inf |   5 +-
>  4 files changed, 719 insertions(+), 99 deletions(-)
>  create mode 100644 MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c
> 
> diff --git a/MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c
> b/MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c
> new file mode 100644
> index ..c92bee429835
> --- /dev/null
> +++ b/MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c
> @@ -0,0 +1,251 @@
> +/** @file
> +The DMA memory help functions.
> +
> +Copyright (c) 2017, Intel Corporation. All rights reserved.
> +
> +This program and the accompanying materials
> +are licensed and made available under the terms and conditions
> +of the BSD License which accompanies this distribution.  The
> +full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> +
> +**/
> +
> +#include "UhcPeim.h"
> +
> +/**
> +  Provides the controller-specific addresses required to access system memory
> from a
> +  DMA bus master.
> +
> +  @param IoMmu  Pointer to IOMMU PPI.
> +  @param Operation  Indicates if the bus master is going to read
> or write to system memory.
> +  @param HostAddressThe system memory address to map to
> the PCI controller.
> +  @param NumberOfBytes  On input the number of bytes to map.
> On output the number of bytes
> +that were mapped.
> +  @param DeviceAddress  The resulting map address for the bus
> master PCI controller to use to
> +access the hosts HostAddress.
> +  @param MappingA resulting value to pass to Unmap().
> +
> +  @retval EFI_SUCCESS   The range was mapped for the returned
> NumberOfBytes.
> +  @retval EFI_UNSUPPORTED   The HostAddress cannot be mapped as a
> common buffer.
> +  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
> +  @retval EFI_OUT_OF_RESOURCES  The request could not be completed due
> to a lack of resources.
> +  @retval EFI_DEVICE_ERROR  The system hardware could not map the
> requested address.
> +
> +**/
> +EFI_STATUS
> +IoMmuMap (
> +  IN EDKII_IOMMU_PPI*IoMmu,
> +  IN EDKII_IOMMU_OPERATION  Operation,
> +  IN VOID   *HostAddress,
> +  IN OUT UINTN  *NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
> +  OUT VOID  **Mapping
> +  )
> +{
> +  EFI_STATUSStatus;
> +  UINT64Attribute;
> +
> +  if (IoMmu != NULL) {
> +Status = IoMmu->Map (
> +  IoMmu,
> +  Operation,
> +  HostAddress,
> +  NumberOfBytes,
> +  DeviceAddress,
> +  Mapping
> +  );
> +if (EFI_ERROR (Status)) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +switch (Operation) {
> +case EdkiiIoMmuOperationBusMasterRead:
> +case EdkiiIoMmuOperationBusMasterRead64:
> +  Attribute = EDKII_IOMMU_ACCESS_READ;
> +  break;
> +case EdkiiIoMmuOperationBusMasterWrite:
> +case EdkiiIoMmuOperationBusMasterWrite64:
> +  Attribute = EDKII_IOMMU_ACCESS_WRITE;
> +  break;
> +case EdkiiIoMmuOperationBusMasterCommonBuffer:
> +case EdkiiIoMmuOperationBusMasterCommonBuffer64:
> +  Attribute = EDKII_IOMMU_ACCESS_READ |
> EDKII_IOMMU_ACCESS_WRITE;
> +  break;
> +default:
> +  ASSERT(FALSE);
> +  return EFI_INVALID_PARAMETER;
> +}
> +Status = IoMmu->SetAttribute (
> +  IoMmu,
> +  *Mapping,
> +  Attribute
> +  );
> +if (EFI_ERROR (Status)) {
> +  IoMmu->Unmap (IoMmu, Mapping);
> +  *Mapping = NULL;
> +  return Status;
> +}
> +  } else {
> +*DeviceAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> +*Mapping = NULL;
> +Status = EFI_SUCCESS;
> +  }
> +  return Status;
> +}
> +
> +/**
> +  Completes the Map() operation and 

Re: [edk2] [PATCH] MdeModulePkg EhciPei: Minor refinement about IOMMU

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 21, 2017 5:04 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] MdeModulePkg EhciPei: Minor refinement about IOMMU
> 
> This patch is following 2c656af04d7f.
> 1. Fix typo "XHC" to "EHC".
> 2. Reinitialize Request(Phy/Map) and Data(Phy/Map)
> in Urb, otherwise the last time value of them may
> be used in error handling when error happens.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h |  2 +-
>  MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c | 13 ++---
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
> b/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
> index 279407475b66..715a5ab1c142 100644
> --- a/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
> +++ b/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
> @@ -99,7 +99,7 @@ struct _PEI_USB2_HC_DEV {
>EDKII_IOMMU_PPI *IoMmu;
>EFI_PEI_PPI_DESCRIPTOR  PpiDescriptor;
>//
> -  // EndOfPei callback is used to stop the XHC DMA operation
> +  // EndOfPei callback is used to stop the EHC DMA operation
>// after exit PEI phase.
>//
>EFI_PEI_NOTIFY_DESCRIPTOR   EndOfPeiNotifyList;
> diff --git a/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
> b/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
> index 3dadcd60b6fe..baacf5d56080 100644
> --- a/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
> +++ b/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
> @@ -576,7 +576,12 @@ EhcCreateUrb (
>if (Urb->Qh == NULL) {
>  goto ON_ERROR;
>}
> -
> +
> +  Urb->RequestPhy = NULL;
> +  Urb->RequestMap = NULL;
> +  Urb->DataPhy  = NULL;
> +  Urb->DataMap  = NULL;
> +
>//
>// Map the request and user data
>//
> @@ -591,9 +596,6 @@ EhcCreateUrb (
> 
>  Urb->RequestPhy = (VOID *) ((UINTN) PhyAddr);
>  Urb->RequestMap = Map;
> -  } else {
> -Urb->RequestPhy = NULL;
> -Urb->RequestMap = NULL;
>}
> 
>if (Data != NULL) {
> @@ -613,9 +615,6 @@ EhcCreateUrb (
> 
>  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
>  Urb->DataMap  = Map;
> -  } else {
> -Urb->DataPhy  = NULL;
> -Urb->DataMap  = NULL;
>}
> 
>Status = EhcCreateQtds (Ehc, Urb);
> --
> 2.7.0.windows.1

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

[edk2] [PATCH] CryptoPkg/IntrinsicLib: Fix the warning on memset

[Gary Lin]

Gcc issued the warning when compiling CryptoPkg:

CryptoPkg/Library/Include/CrtLibSupport.h:135:17: warning: type of 'memset' 
does not match original declaration [-Wlto-type-mismatch]
 void   *memset (void *, int, size_t);
 ^
CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c:27:8: note: type mismatch in 
parameter 2
 void * memset (void *dest, char ch, size_t count)
^

This commit changes the type of ch from char to int to match the
declaration.

Cc: Qin Long 
Cc: Ting Ye 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Gary Lin 
---
 CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c 
b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
index bf485d680d..e095f9aa0d 100644
--- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
+++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
@@ -24,7 +24,7 @@ typedef UINTN  size_t;
 int _fltused = 1;
 
 /* Sets buffers to a specified character */
-void * memset (void *dest, char ch, size_t count)
+void * memset (void *dest, int ch, size_t count)
 {
   //
   // NOTE: Here we use one base implementation for memset, instead of the 
direct
@@ -42,7 +42,7 @@ void * memset (void *dest, char ch, size_t count)
 
   Pointer = (UINT8 *)dest;
   while (count-- != 0) {
-*(Pointer++) = ch;
+*(Pointer++) = (UINT8)ch;
   }
   
   return dest;
-- 
2.15.0

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

Re: [edk2] [patch] PcAtChipsetPkg/IsaAcpiDxe: Fix VS2012 build failure

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni 

> -Original Message-
> From: Bi, Dandan
> Sent: Wednesday, November 22, 2017 10:02 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu 
> Subject: [patch] PcAtChipsetPkg/IsaAcpiDxe: Fix VS2012 build failure
> 
> Done:
> if (EFI_ERROR (Status)) {
>   if (PciIo != NULL && Enabled) {
> PciIo->Attributes (
>  PciIo,
>  EfiPciIoAttributeOperationSet,
>  OriginalAttributes,
>  NULL
>  );
>   }
> }
> In above codes, VS2012/VS2010 will report that "OriginalAttributes"
> will be used without initialization. But in fact, when the if expression is 
> true(if
> (PciIo != NULL && Enabled)), the "OriginalAttributes" must be initialized. In 
> order
> to fix this false positive issue, we initialize the "OriginalAttributes" after
> declaration.
> 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi 
> ---
>  PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
> b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
> index d82e2c4..c7ea559 100644
> --- a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
> +++ b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
> @@ -176,10 +176,11 @@ PcatIsaAcpiDriverBindingStart (
>BOOLEAN  Enabled;
> 
>Enabled = FALSE;
>Supports = 0;
>PcatIsaAcpiDev = NULL;
> +  OriginalAttributes = 0;
>//
>// Open the PCI I/O Protocol Interface
>//
>PciIo = NULL;
>Status = gBS->OpenProtocol (
> --
> 1.9.5.msysgit.1

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

[edk2] [patch] PcAtChipsetPkg/IsaAcpiDxe: Fix VS2012 build failure

[Dandan Bi]

Done:
if (EFI_ERROR (Status)) {
  if (PciIo != NULL && Enabled) {
PciIo->Attributes (
 PciIo,
 EfiPciIoAttributeOperationSet,
 OriginalAttributes,
 NULL
 );
  }
}
In above codes, VS2012/VS2010 will report that "OriginalAttributes"
will be used without initialization. But in fact, when the if expression
is true(if (PciIo != NULL && Enabled)), the "OriginalAttributes" must be
initialized. In order to fix this false positive issue, we initialize the
"OriginalAttributes" after declaration.

Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi 
---
 PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c 
b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
index d82e2c4..c7ea559 100644
--- a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
+++ b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
@@ -176,10 +176,11 @@ PcatIsaAcpiDriverBindingStart (
   BOOLEAN  Enabled;
 
   Enabled = FALSE;
   Supports = 0;
   PcatIsaAcpiDev = NULL;
+  OriginalAttributes = 0;
   //
   // Open the PCI I/O Protocol Interface
   //
   PciIo = NULL;
   Status = gBS->OpenProtocol (
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH] SecurityPkg/SecureBootConfigDxe: Fix deleting signature data issue.

[chenc2]

Replace "(UINT8 *)NewVariableData" with (UINT8 *)NewVariableData + Offset"
to avoid the header of EFI_SIGNATURE_LIST being copied to the front of
NewVariableData every time and update ListWalker when handling the current
EFI_SIGNATURE_LIST finishes.

Cc: Zhang Chao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: chenc2 
---
 .../SecureBootConfigDxe/SecureBootConfigImpl.c   | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git 
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c 
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index d035763106..e3066f77be 100644
--- 
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ 
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
@@ -3145,6 +3145,9 @@ DeleteSignatureEx (
   if (DelType == Delete_Signature_List_All) {
 VariableDataSize = 0;
   } else {
+//
+//  Traverse to target EFI_SIGNATURE_LIST but others will be skipped.
+//
 while ((RemainingSize > 0) && (RemainingSize >= 
ListWalker->SignatureListSize) && ListIndex < PrivateData->ListIndex) {
   CopyMem ((UINT8 *)NewVariableData + Offset, ListWalker, 
ListWalker->SignatureListSize);
   Offset += ListWalker->SignatureListSize;
@@ -3154,15 +3157,17 @@ DeleteSignatureEx (
   ListIndex++;
 }
 
-if (CheckedCount == SIGNATURE_DATA_COUNTS (ListWalker) || DelType == 
Delete_Signature_List_One) {
-  RemainingSize -= ListWalker->SignatureListSize;
-  ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + 
ListWalker->SignatureListSize);
-} else {
+//
+//  Handle the target EFI_SIGNATURE_LIST.
+//  If CheckedCount == SIGNATURE_DATA_COUNTS (ListWalker) or DelType == 
Delete_Signature_List_One
+//  it means delete the whole EFI_SIGNATURE_LIST, So we just skip this 
EFI_SIGNATURE_LIST.
+//
+if (CheckedCount < SIGNATURE_DATA_COUNTS (ListWalker) && DelType == 
Delete_Signature_Data) {
   NewCertList = (EFI_SIGNATURE_LIST *)(NewVariableData + Offset);
   //
   // Copy header.
   //
-  CopyMem ((UINT8 *)NewVariableData, ListWalker, sizeof 
(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
+  CopyMem ((UINT8 *)NewVariableData + Offset, ListWalker, sizeof 
(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
   Offset += sizeof (EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize;
 
   DataWalker = (EFI_SIGNATURE_DATA *)((UINT8 *)ListWalker + 
sizeof(EFI_SIGNATURE_LIST) + ListWalker->SignatureHeaderSize);
@@ -3181,10 +3186,11 @@ DeleteSignatureEx (
 }
 DataWalker = (EFI_SIGNATURE_DATA *)((UINT8 *)DataWalker + 
ListWalker->SignatureSize);
   }
-
-  RemainingSize -= ListWalker->SignatureListSize;
 }
 
+RemainingSize -= ListWalker->SignatureListSize;
+ListWalker = (EFI_SIGNATURE_LIST *)((UINT8 *)ListWalker + 
ListWalker->SignatureListSize);
+
 //
 // Copy remaining data, maybe 0.
 //
-- 
2.13.2.windows.1

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

Re: [edk2] [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library

[Ard Biesheuvel]

On 21 November 2017 at 20:17, Laszlo Ersek  wrote:
> On 11/21/17 18:57, Ard Biesheuvel wrote:
>>
>>
>>> On 21 Nov 2017, at 17:49, Laszlo Ersek  wrote:
>>>
 On 11/17/17 17:09, Ard Biesheuvel wrote:
 Equivalent to the PrePi based platforms, this patch implements the
 newly introduced ArmVirtMemInfo library class via a separate PEIM
 and PPI.

 The reason is that ArmVirtPlatformLib has populated the ArmPlatformLib
 API function ArmPlatformInitializeSystemMemory () to retrieve memory
 information from the DT, ensuring that it will be present when
 MemoryPeim() is executed. This is a bit of a hack, and someting we
 will need to get rid of if we want to reduce our dependency on
 ArmPlatformLib altogether.
>>>
>>> OK, so whenever I try to look at this code, my brain crashes. This
>>> occasion is no exception. All the more reason to clean it all up; thanks
>>> for doing that.
>>>
>>> So, I guess we are talking about the following call stack. If you agree,
>>> please add it to the commit message (IMO it's OK to exceed the preferred
>>> 74 chars width for such sections):
>>>
>>>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf 
>>> [ArmVirtPkg/ArmVirtQemu.dsc]
>>>InitializeMemory()
>>> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>>>  ArmPlatformInitializeSystemMemory() 
>>> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>>>//
>>>// set PcdSystemMemorySize from the DT
>>>//
>>>  MemoryPeim()
>>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>>InitMmu() 
>>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>>  ArmPlatformGetVirtualMemoryMap()
>>> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>>>//
>>>// consume PcdSystemMemorySize
>>>//
>>>
>>> Here we have two ArmVirtPlatformLib actions:
>>>
>>> - The "PCD consumption" half of that has been moved -- well, copied, for
>>>  now -- to QemuVirtMemInfoLib, in patch 12/15.
>>>
>>> - And we are now looking for a new home for the "PCD production" half.
>>>
>>
>> Yes
>>
 Putting this code in a ArmVirtMemInfo library constructor is
 problematic as well, given that the implementation uses other
 libraries, among which PcdLib, and so we need to find another way to
 run it before MemoryPeim()
>>>
>>> Hm, this claim could be true :) , but I'm not totally convinced by the
>>> way you put it.
>>>
>>> First off, I notice that
>>> "ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf" does not
>>> spell out PcdLib as a dependency. This is quite bad, because it uses
>>> PCDs.
>>>
>>> However, ultimately, "gEfiPeiPcdPpiGuid" does end up in the final DEPEX
>>> of "MemoryInitPeim.inf", according to the ArmVirtQemu build report file.
>>> This must be due to some of the library instances already pulling in
>>> PeiPcdLib.
>>>
>>> Therefore, if we modify the constructor of QemuVirtMemInfoLib to parse
>>> the DT and to set the PCD, *plus* we spell out PcdLib in the
>>> QemuVirtMemInfoLib INF file, then the ultimate DEPEX for the
>>> MemoryInitPeim module should remain the same. And, the PeiPcdLib
>>> constructor should run before the QemuVirtMemInfoLib constructor
>>> (parsing the DT and setting the PCD).
>>>
>>> What's wrong with this argument?
>>>
>>
>> I guess you’re right. Direct dependencies between libraries with 
>> constructors are handled correctly by the tools, I simply managed to confuse 
>> myself due to the issues with transitive dependencies which you surely 
>> remember.
>
> Right, I suspected that this experience was in the background. If I
> remember correctly, the issue was when some libraries had constructors
> while some others had none (and required explicit init function calls
> instead). In some cases this mixing was not possible to avoid due to
> circular dependencies between constructors, but in turn the explicit
> calls didn't get ordered right, or some such. *shudder* :)
>

The core of the issue is that transitive library dependencies are not
honoured in the ordering of constructor invocations if they pass
through a library that has no constructor itself.

I.e., libA with a constructor

depending on libB with no constructor

depending on libC with a constructor

Currently, libA's constructor may be called before libC's constructor,
which is clearly a bug, and which is the reason why I /think/ we
generally shouldn't be relying on other libraries in constructor
implementations.

>>
 So instead, create a separate PEIM that encapsulates the
 ArmVirtMemInfo code and exposes it via a PPI. Another ArmVirtMemInfo
 library class implementation is also provided that depexes on the PPI,
 which ensures that the code is called in the correct order.
>>>
>>> I understand what this does, but I find it very 

Re: [edk2] [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library

[Laszlo Ersek]

On 11/21/17 18:57, Ard Biesheuvel wrote:
> 
> 
>> On 21 Nov 2017, at 17:49, Laszlo Ersek  wrote:
>>
>>> On 11/17/17 17:09, Ard Biesheuvel wrote:
>>> Equivalent to the PrePi based platforms, this patch implements the
>>> newly introduced ArmVirtMemInfo library class via a separate PEIM
>>> and PPI.
>>>
>>> The reason is that ArmVirtPlatformLib has populated the ArmPlatformLib
>>> API function ArmPlatformInitializeSystemMemory () to retrieve memory
>>> information from the DT, ensuring that it will be present when
>>> MemoryPeim() is executed. This is a bit of a hack, and someting we
>>> will need to get rid of if we want to reduce our dependency on
>>> ArmPlatformLib altogether.
>>
>> OK, so whenever I try to look at this code, my brain crashes. This
>> occasion is no exception. All the more reason to clean it all up; thanks
>> for doing that.
>>
>> So, I guess we are talking about the following call stack. If you agree,
>> please add it to the commit message (IMO it's OK to exceed the preferred
>> 74 chars width for such sections):
>>
>>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQemu.dsc]
>>InitializeMemory()
>> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>>  ArmPlatformInitializeSystemMemory() 
>> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>>//
>>// set PcdSystemMemorySize from the DT
>>//
>>  MemoryPeim()
>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>InitMmu() 
>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>  ArmPlatformGetVirtualMemoryMap()
>> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>>//
>>// consume PcdSystemMemorySize
>>//
>>
>> Here we have two ArmVirtPlatformLib actions:
>>
>> - The "PCD consumption" half of that has been moved -- well, copied, for
>>  now -- to QemuVirtMemInfoLib, in patch 12/15.
>>
>> - And we are now looking for a new home for the "PCD production" half.
>>
> 
> Yes
> 
>>> Putting this code in a ArmVirtMemInfo library constructor is
>>> problematic as well, given that the implementation uses other
>>> libraries, among which PcdLib, and so we need to find another way to
>>> run it before MemoryPeim()
>>
>> Hm, this claim could be true :) , but I'm not totally convinced by the
>> way you put it.
>>
>> First off, I notice that
>> "ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf" does not
>> spell out PcdLib as a dependency. This is quite bad, because it uses
>> PCDs.
>>
>> However, ultimately, "gEfiPeiPcdPpiGuid" does end up in the final DEPEX
>> of "MemoryInitPeim.inf", according to the ArmVirtQemu build report file.
>> This must be due to some of the library instances already pulling in
>> PeiPcdLib.
>>
>> Therefore, if we modify the constructor of QemuVirtMemInfoLib to parse
>> the DT and to set the PCD, *plus* we spell out PcdLib in the
>> QemuVirtMemInfoLib INF file, then the ultimate DEPEX for the
>> MemoryInitPeim module should remain the same. And, the PeiPcdLib
>> constructor should run before the QemuVirtMemInfoLib constructor
>> (parsing the DT and setting the PCD).
>>
>> What's wrong with this argument?
>>
> 
> I guess you’re right. Direct dependencies between libraries with constructors 
> are handled correctly by the tools, I simply managed to confuse myself due to 
> the issues with transitive dependencies which you surely remember.

Right, I suspected that this experience was in the background. If I
remember correctly, the issue was when some libraries had constructors
while some others had none (and required explicit init function calls
instead). In some cases this mixing was not possible to avoid due to
circular dependencies between constructors, but in turn the explicit
calls didn't get ordered right, or some such. *shudder* :)

> 
>>> So instead, create a separate PEIM that encapsulates the
>>> ArmVirtMemInfo code and exposes it via a PPI. Another ArmVirtMemInfo
>>> library class implementation is also provided that depexes on the PPI,
>>> which ensures that the code is called in the correct order.
>>
>> I understand what this does, but I find it very complex.
>>
>> Sometimes, whenever we want to make sure that a PCD is set dynamically
>> "no later than" it's consumed by a "common" module outside of
>> ArmVirtPkg, we create a dedicated library instance (with all the right
>> library dependencies spelled out in its INF file), set the PCD in its
>> constructor, and hook it into the consumer module via NULL class
>> resolution.
>>
>> In this case, we have a lib instance that is used through an actual lib
>> class already, so I would suggest to add a constructor function, and to
>> spell out the PcdLib dependency in the INF file.
>>
>> ... In fact, this is something that I missed in patch 12 --
>> QemuVirtMemInfoLib already uses PCDs, but 

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Udit Kumar]

Thanks Ard, 
For internal SOC devices, this is perfectly ok to drop PRP0001 from CID. 

> This could be a valid reason to use PRP0001 + compatible, for things like I2C
> slaves that are external to the SoC

For external devices (for which HID is not available), you suggest to go
with PRP0001 + compatible or that device driver needs add ACPI HID support.

As you pointed out, are external devices to SOC such exception to use PRP0001 + 
compatible be it 
i2c slave or SPI slave ?


Regards
Udit

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, November 21, 2017 7:34 PM
> To: Udit Kumar 
> Cc: Leif Lindholm ; edk2-devel@lists.01.org; Varun
> Sethi ; Daniel Thompson ;
> Graeme Gregory 
> Subject: Re: [RFC] ACPI table HID/CID allocation
> 
> On 21 November 2017 at 13:24, Udit Kumar  wrote:
> > Thanks Ard,
> >
> > My intend of this email to know, what is right way to define HID and
> > CID in ACPI firmware i.e
> >
> > Device(XYZ) {
> > Name(_HID, "NXP0001")
> > Name(_CID, "PRP0001")
> >   Device(Slave1) {
> > Name(_CID, "PRP0001")
> >  }
> > }
> > is ok or
> >
> >
> > Device(XYZ) {
> > Name(_HID, "NXP0001")
> > Name(_CID, " NXP0001")
> >   Device(Slave1) {
> > Name(_CID, " NXP0002")
> >  }
> > }
> > Seems good
> >
> 
> I don't think it makes a lot of sense to use the same value for _HID and 
> _CID, so
> you can just drop the latter.

Sure, 
 
> > For sure, AML methods (as needed _ON/OFF/RST/STA etc) /_DSD will to be
> coded in table using either of.
> >
> > Please see more in line
> >
> > Regards
> > Udit
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: Tuesday, November 21, 2017 5:59 PM
> >> To: Udit Kumar 
> >> Cc: Leif Lindholm ;
> >> edk2-devel@lists.01.org; Varun Sethi ; Daniel
> >> Thompson ; Graeme Gregory
> >> 
> >> Subject: Re: [RFC] ACPI table HID/CID allocation
> >>
> >> On 21 November 2017 at 11:32, Udit Kumar  wrote:
> >> >
> >> >> On 21 November 2017 at 09:59, Udit Kumar 
> wrote:
> >> >> > Thanks Ard.
> >> >> > Below table was for example. I am not converting whole DT to
> >> >> > ACPI tables :) My idea is to reduce Linux patches for probing as 
> >> >> > possible.
> >> >> > Also keeping firmware and OS separately then Let firmware expose
> >> >> > both way (HID and PRP1) and Linux to decide binding
> >> >>
> >> >> No.
> >> >>
> >> >> You are still assuming ACPI and DT device drivers bind at the same
> >> >> level, and they don't.
> >> >
> >> > No, my above comments was just limited to binding.
> >>
> >> Yes, but if you leave it to the OS to decide which binding it uses,
> >> you will have to support all of them into eternity. And the more
> >> detailed binding you need to support may limit you in the available
> >> choices when it comes to making hardware changes, something ACPI allows
> you to do.
> >
> > Thanks,
> > Is this ok to say , we can provide max same properties in driver as of
> > DT (with _DSD) , But prefer to use AML methods.
> > With note, clocking regulators are out of scope here.
> >
> 
> Yes. _DSD may be used to describe device specific data that goes beyond what
> ACPI can express natively. Using _DSD to describe clocks and regulators is an
> absolute no-go. Putting things like "status" or "dma-coherent" in _DSD
> properties is absolutely unacceptable as well.
> But even things like initialization data that the driver programs into the 
> device a
> single time really do not belong in _DSD. Instead, it should be the firmware 
> that
> initializes the device, and presents it to the OS in its initialized state.
>

Agreed, I never meant something to add in DSD which was prohibited in ACPI 
spces.

> >
> >> > Right, here device driver should know that device is present in
> >> > system, I see probe function (device-driver binding) is way to know this.
> >> > Then driver can execute AML methods exposed by firmware.
> >> >
> >>
> >> Yes, declaring the presence of the device is the main purpose of
> >> describing it both in ACPI and in DT.
> >>
> >> >> An ACPI device has AML methods to manage power state and perform
> >> >> other device related low-level tasks. The device driver has no
> >> >> knowledge of the hardware beyond what it needs to invoke those
> >> >> abstract
> >> methods.
> >> >
> >> > You meant, If we need to update driver for AML methods then why not
> >> > to
> >> update binding as well . ?
> >> >
> >>
> >> No. What I am saying is that you should not expose two different
> >> bindings, and 

Re: [edk2] [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library

[Ard Biesheuvel]

> On 21 Nov 2017, at 17:49, Laszlo Ersek  wrote:
> 
>> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> Equivalent to the PrePi based platforms, this patch implements the
>> newly introduced ArmVirtMemInfo library class via a separate PEIM
>> and PPI.
>> 
>> The reason is that ArmVirtPlatformLib has populated the ArmPlatformLib
>> API function ArmPlatformInitializeSystemMemory () to retrieve memory
>> information from the DT, ensuring that it will be present when
>> MemoryPeim() is executed. This is a bit of a hack, and someting we
>> will need to get rid of if we want to reduce our dependency on
>> ArmPlatformLib altogether.
> 
> OK, so whenever I try to look at this code, my brain crashes. This
> occasion is no exception. All the more reason to clean it all up; thanks
> for doing that.
> 
> So, I guess we are talking about the following call stack. If you agree,
> please add it to the commit message (IMO it's OK to exceed the preferred
> 74 chars width for such sections):
> 
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQemu.dsc]
>InitializeMemory()
> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>  ArmPlatformInitializeSystemMemory() 
> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>//
>// set PcdSystemMemorySize from the DT
>//
>  MemoryPeim()
> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>InitMmu() 
> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>  ArmPlatformGetVirtualMemoryMap()
> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>//
>// consume PcdSystemMemorySize
>//
> 
> Here we have two ArmVirtPlatformLib actions:
> 
> - The "PCD consumption" half of that has been moved -- well, copied, for
>  now -- to QemuVirtMemInfoLib, in patch 12/15.
> 
> - And we are now looking for a new home for the "PCD production" half.
> 

Yes

>> Putting this code in a ArmVirtMemInfo library constructor is
>> problematic as well, given that the implementation uses other
>> libraries, among which PcdLib, and so we need to find another way to
>> run it before MemoryPeim()
> 
> Hm, this claim could be true :) , but I'm not totally convinced by the
> way you put it.
> 
> First off, I notice that
> "ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf" does not
> spell out PcdLib as a dependency. This is quite bad, because it uses
> PCDs.
> 
> However, ultimately, "gEfiPeiPcdPpiGuid" does end up in the final DEPEX
> of "MemoryInitPeim.inf", according to the ArmVirtQemu build report file.
> This must be due to some of the library instances already pulling in
> PeiPcdLib.
> 
> Therefore, if we modify the constructor of QemuVirtMemInfoLib to parse
> the DT and to set the PCD, *plus* we spell out PcdLib in the
> QemuVirtMemInfoLib INF file, then the ultimate DEPEX for the
> MemoryInitPeim module should remain the same. And, the PeiPcdLib
> constructor should run before the QemuVirtMemInfoLib constructor
> (parsing the DT and setting the PCD).
> 
> What's wrong with this argument?
> 

I guess you’re right. Direct dependencies between libraries with constructors 
are handled correctly by the tools, I simply managed to confuse myself due to 
the issues with transitive dependencies which you surely remember.

>> So instead, create a separate PEIM that encapsulates the
>> ArmVirtMemInfo code and exposes it via a PPI. Another ArmVirtMemInfo
>> library class implementation is also provided that depexes on the PPI,
>> which ensures that the code is called in the correct order.
> 
> I understand what this does, but I find it very complex.
> 
> Sometimes, whenever we want to make sure that a PCD is set dynamically
> "no later than" it's consumed by a "common" module outside of
> ArmVirtPkg, we create a dedicated library instance (with all the right
> library dependencies spelled out in its INF file), set the PCD in its
> constructor, and hook it into the consumer module via NULL class
> resolution.
> 
> In this case, we have a lib instance that is used through an actual lib
> class already, so I would suggest to add a constructor function, and to
> spell out the PcdLib dependency in the INF file.
> 
> ... In fact, this is something that I missed in patch 12 --
> QemuVirtMemInfoLib already uses PCDs, but doesn't include "PcdLib" under
> [LibraryClasses]. That is a bug inherited from ArmVirtPlatformLib (see
> above), and should be fixed. And then we should only need the
> constructor.
> 
> What do you think?
> 

I think you’re right.

So how do you propose i go about creating two versions of QemuVirtMemInfoLib, 
only one of which has a constructor? Share the .c file between two infs in the 
same directory?


> 
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>> 

Re: [edk2] [PATCH 15/15] ArmVirtPkg: remove ArmPlatformLib implementations

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> These libraries are no longer used, so remove them from the tree.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.dsc 
>   |   1 -
>  ArmVirtPkg/ArmVirtXen.dsc
>   |   1 -
>  
> ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/AARCH64/RelocatableVirtHelper.S
>| 141 -
>  ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ARM/RelocatableVirtHelper.S 
>   | 123 ---
>  
> ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ArmQemuRelocatablePlatformLib.inf
>  |  64 
>  ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/FdtParser.c 
>   |  90 ---
>  ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c   
>   | 106 -
>  ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/RelocatableVirt.c   
>   |  70 -
>  ArmVirtPkg/Library/ArmVirtPlatformLib/AARCH64/VirtHelper.S   
>   |  70 -
>  ArmVirtPkg/Library/ArmVirtPlatformLib/ARM/VirtHelper.S   
>   |  57 ---
>  ArmVirtPkg/Library/ArmVirtPlatformLib/ARM/VirtHelper.asm 
>   |  71 -
>  ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf 
>   |  64 
>  ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c 
>   | 160 
>  ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c  
>   | 102 -
>  
> ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/AARCH64/RelocatableVirtHelper.S
> | 140 -
>  ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/ARM/RelocatableVirtHelper.S  
>   | 123 ---
>  
> ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/ArmXenRelocatablePlatformLib.inf
>|  63 
>  ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/FdtParser.c  
>   |  89 ---
>  ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/RelocatableVirt.c
>   |  70 -
>  ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/XenVirtMem.c 
>   |  82 --
>  20 files changed, 1687 deletions(-)

This feels awesome.

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

Re: [edk2] [PATCH 14/15] ArmVirtPkg/ArmVirtMemoryInitPeiLib: move to ArmVirtMemInfoLib

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Move to the new ArmVirtMemInfoLib library to retrieve DRAM information
> from the platform, so that we can phase out ArmPlatformLib going forward.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc | 7 
> ++-
>  ArmVirtPkg/ArmVirtQemu.fdf | 1 +
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 4 
> ++--
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 3 
> ++-
>  4 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index d14a0dd0d1d9..5c59f05187fa 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -48,7 +48,8 @@ [LibraryClasses.common]
>QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>  
> -  ArmPlatformLib|ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
> +  
> ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
> +  
> ArmVirtMemInfoLib|ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.inf
>  
>TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> @@ -233,6 +234,10 @@ [Components.common]
>  
>
> NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>}
> +  ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf {
> +
> +  
> ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> +  }
>  
>#
># DXE
> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> index c6a22dc018f3..696dec264639 100644
> --- a/ArmVirtPkg/ArmVirtQemu.fdf
> +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> @@ -109,6 +109,7 @@ [FV.FVMAIN_COMPACT]
>INF ArmPkg/Drivers/CpuPei/CpuPei.inf
>INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> +  INF ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf
>INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>  
>FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
> diff --git 
> a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c 
> b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> index 6f3e54b7afcb..05afd1282422 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> @@ -16,7 +16,7 @@
>  #include 
>  
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -39,7 +39,7 @@ InitMmu (
>RETURN_STATUS Status;
>  
>// Get Virtual Memory Map from the Platform Library
> -  ArmPlatformGetVirtualMemoryMap ();
> +  ArmVirtGetMemoryMap ();
>  
>//Note: Because we called PeiServicesInstallPeiMemory() before to call 
> InitMmu() the MMU Page Table resides in
>//  DRAM (even at the top of DRAM as it is the first permanent memory 
> allocation)
> diff --git 
> a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf 
> b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> index 028d6fb5ac28..54879d590a8a 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> @@ -29,13 +29,14 @@ [Packages]
>EmbeddedPkg/EmbeddedPkg.dec
>ArmPkg/ArmPkg.dec
>ArmPlatformPkg/ArmPlatformPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
>  
>  [LibraryClasses]
>DebugLib
>HobLib
>ArmLib
>ArmMmuLib
> -  ArmPlatformLib
> +  ArmVirtMemInfoLib
>CacheMaintenanceLib
>  
>  [Guids]
> 

This patch is the logical continuation of patch #13, so my feedback
depends on your answer to my patch #13 feedback :)

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

Re: [edk2] [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Equivalent to the PrePi based platforms, this patch implements the
> newly introduced ArmVirtMemInfo library class via a separate PEIM
> and PPI.
>
> The reason is that ArmVirtPlatformLib has populated the ArmPlatformLib
> API function ArmPlatformInitializeSystemMemory () to retrieve memory
> information from the DT, ensuring that it will be present when
> MemoryPeim() is executed. This is a bit of a hack, and someting we
> will need to get rid of if we want to reduce our dependency on
> ArmPlatformLib altogether.

OK, so whenever I try to look at this code, my brain crashes. This
occasion is no exception. All the more reason to clean it all up; thanks
for doing that.

So, I guess we are talking about the following call stack. If you agree,
please add it to the commit message (IMO it's OK to exceed the preferred
74 chars width for such sections):

  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQemu.dsc]
InitializeMemory()
[ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
  ArmPlatformInitializeSystemMemory() 
[ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
//
// set PcdSystemMemorySize from the DT
//
  MemoryPeim()
[ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
InitMmu() 
[ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
  ArmPlatformGetVirtualMemoryMap()
[ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
//
// consume PcdSystemMemorySize
//

Here we have two ArmVirtPlatformLib actions:

- The "PCD consumption" half of that has been moved -- well, copied, for
  now -- to QemuVirtMemInfoLib, in patch 12/15.

- And we are now looking for a new home for the "PCD production" half.

> Putting this code in a ArmVirtMemInfo library constructor is
> problematic as well, given that the implementation uses other
> libraries, among which PcdLib, and so we need to find another way to
> run it before MemoryPeim()

Hm, this claim could be true :) , but I'm not totally convinced by the
way you put it.

First off, I notice that
"ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf" does not
spell out PcdLib as a dependency. This is quite bad, because it uses
PCDs.

However, ultimately, "gEfiPeiPcdPpiGuid" does end up in the final DEPEX
of "MemoryInitPeim.inf", according to the ArmVirtQemu build report file.
This must be due to some of the library instances already pulling in
PeiPcdLib.

Therefore, if we modify the constructor of QemuVirtMemInfoLib to parse
the DT and to set the PCD, *plus* we spell out PcdLib in the
QemuVirtMemInfoLib INF file, then the ultimate DEPEX for the
MemoryInitPeim module should remain the same. And, the PeiPcdLib
constructor should run before the QemuVirtMemInfoLib constructor
(parsing the DT and setting the PCD).

What's wrong with this argument?

> So instead, create a separate PEIM that encapsulates the
> ArmVirtMemInfo code and exposes it via a PPI. Another ArmVirtMemInfo
> library class implementation is also provided that depexes on the PPI,
> which ensures that the code is called in the correct order.

I understand what this does, but I find it very complex.

Sometimes, whenever we want to make sure that a PCD is set dynamically
"no later than" it's consumed by a "common" module outside of
ArmVirtPkg, we create a dedicated library instance (with all the right
library dependencies spelled out in its INF file), set the PCD in its
constructor, and hook it into the consumer module via NULL class
resolution.

In this case, we have a lib instance that is used through an actual lib
class already, so I would suggest to add a constructor function, and to
spell out the PcdLib dependency in the INF file.

... In fact, this is something that I missed in patch 12 --
QemuVirtMemInfoLib already uses PCDs, but doesn't include "PcdLib" under
[LibraryClasses]. That is a bug inherited from ArmVirtPlatformLib (see
above), and should be fixed. And then we should only need the
constructor.

What do you think?

Thanks,
Laszlo

>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtPkg.dec  |   3 +
>  ArmVirtPkg/Include/Ppi/ArmVirtMemInfo.h|  48 
>  ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.c   |  46 
>  ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.inf |  42 +++
>  ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.c   | 121 
> 
>  ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf |  60 ++
>  6 files changed, 320 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index 8f656fd2739d..260849dc845c 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ 

Re: [edk2] [PATCH 12/15] ArmVirtPkg/ArmVirtQemu: add ArmVirtMemInfoLib implementation

[Ard Biesheuvel]

On 21 November 2017 at 16:56, Laszlo Ersek  wrote:
> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
>> clean it up slightly (by removing the support for uncached DRAM mappings),
>> and turn it into a new ArmVirtMemInfoLib implementation.
>
> I've looked at this patch with "git show --find-copies-harder". It looks
> OK, but the commit message could be improved:
>
> (1) the support for uncached DRAM mappings is removed in the copy-origin
> lib instance, in patch 09/15 ("ArmVirtPkg/ArmVirtPlatformLib: remove
> support for uncached mappings"). I think this sentence should be dropped
> from the commit message.
>
> (2) There are other cleanups however:
>
> - factor out TableSize and TopOfMemory
>
> - replace EFI_D_* with DEBUG_*
>
> - fetch PcdFdBaseAddress with PcdGet64(), not FixedPcdGet64(). (This is
> matched by the [Pcd] / [FixedPcd] sections in the new INF file.)
>
> Can you elaborate on the last item? I wonder if that change qualifies as
> cleanup. (I'm fine if the change is justified by some other flexibility,
> but it should be documented please.)
>
> With the commit message updated:
>
> Reviewed-by: Laszlo Ersek 
>

Thanks. But is actually based on
ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib, which explains the
latter point.

However, given that we will be sharing it between ArmVirtQemu and
ArmVirtQemuKernel later on (which is apparently justified, since git
can't even tell them apart), it makes sense to elaborate a bit on the
differences and changes wrt the originals.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 12/15] ArmVirtPkg/ArmVirtQemu: add ArmVirtMemInfoLib implementation

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
> clean it up slightly (by removing the support for uncached DRAM mappings),
> and turn it into a new ArmVirtMemInfoLib implementation.

I've looked at this patch with "git show --find-copies-harder". It looks
OK, but the commit message could be improved:

(1) the support for uncached DRAM mappings is removed in the copy-origin
lib instance, in patch 09/15 ("ArmVirtPkg/ArmVirtPlatformLib: remove
support for uncached mappings"). I think this sentence should be dropped
from the commit message.

(2) There are other cleanups however:

- factor out TableSize and TopOfMemory

- replace EFI_D_* with DEBUG_*

- fetch PcdFdBaseAddress with PcdGet64(), not FixedPcdGet64(). (This is
matched by the [Pcd] / [FixedPcd] sections in the new INF file.)

Can you elaborate on the last item? I wonder if that change qualifies as
cleanup. (I'm fine if the change is justified by some other flexibility,
but it should be documented please.)

With the commit message updated:

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

> 
> In a future patch, we will add this library to the ordinary ArmVirtQemu
> platform as well.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.dsc |   1 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  |  39 
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S  |  24 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   | 101 
> 
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  53 ++
>  5 files changed, 218 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc 
> b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 7e5d584344b4..f50d30388cf2 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -49,6 +49,7 @@ [LibraryClasses.common]
>QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>  
>
> ArmPlatformLib|ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ArmQemuRelocatablePlatformLib.inf
> +  
> ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
>  
>TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S 
> b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> new file mode 100644
> index ..a1f6a194d59b
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> @@ -0,0 +1,39 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD 
> License
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +#
> +
> +#include 
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mrs   x0, id_aa64mmfr0_el1
> +  adr   x1, .LPARanges
> +  and   x0, x0, #7
> +  ldrb  w1, [x1, x0]
> +  mov   x0, #1
> +  lsl   x0, x0, x1
> +  ret
> +
> +//
> +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> +// physical address space support on this CPU:
> +// 0 == 32 bits, 1 == 36 bits, etc etc
> +// 6 and 7 are reserved
> +//
> +.LPARanges:
> +  .byte 32, 36, 40, 42, 44, 48, -1, -1
> +
> +ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S 
> b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> new file mode 100644
> index ..9cd81529fb3d
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> @@ -0,0 +1,24 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD 
> License
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +#
> +
> +#include 
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)

Re: [edk2] [PATCH] BaseTools/tools_def AARCH64 ARM: suppres PIE sections via linker script

[Marcin Wojtas]

Hi Ard,

2017-11-21 17:10 GMT+01:00 Ard Biesheuvel :
> Recent distro builds of GCC 6 enable PIE linking by default, and allow
> the previous behavior to be restored by passing the -no-pie command line
> argument. This was implemented by commits 1894a7c64c0a and 3380a591232d
> but unfortunately, it turns out that GCC 5 does not support this command
> line argument, and exits with an error.
>
> To avoid the need for yet another toolchain tag, to distinguish between
> GCC 5 and GCC 6, let's use our GCC linker scripts when building objects
> from .aslc files. This will ensure that the extra sections that are added
> by the PIE linker are discarded from the ELF binary, and so they will not
> corrupt the resulting .acpi file.
>
> This reverts
>
> 1894a7c64c0a BaseTools/tools_def AARCH64 ARM: disable PIE linking
> 3380a591232d BaseTools/tools_def AARCH64 ARM: disable PIE linking for .aslc 
> sources
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  BaseTools/Conf/tools_def.template | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/BaseTools/Conf/tools_def.template 
> b/BaseTools/Conf/tools_def.template
> index aebd7d558633..4d2a3b7dbe56 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4356,9 +4356,10 @@ DEFINE GCC_IA32_X64_DLINK_COMMON   = 
> DEF(GCC_DLINK_FLAGS_COMMON) --gc-sections
>  DEFINE GCC_ARM_AARCH64_DLINK_COMMON= -Wl,--emit-relocs -nostdlib 
> -Wl,--gc-sections -u $(IMAGE_ENTRY_POINT) 
> -Wl,-e,$(IMAGE_ENTRY_POINT),-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>  DEFINE GCC_ARM_DLINK_FLAGS = DEF(GCC_ARM_AARCH64_DLINK_COMMON) -z 
> common-page-size=0x20
>  DEFINE GCC_AARCH64_DLINK_FLAGS = DEF(GCC_ARM_AARCH64_DLINK_COMMON) -z 
> common-page-size=0x20
> +DEFINE GCC_ARM_AARCH64_ASLDLINK_FLAGS = -Wl,--defsym=PECOFF_HEADER_SIZE=0 
> DEF(GCC_DLINK2_FLAGS_COMMON) -z common-page-size=0x20
>  DEFINE GCC_IA32_X64_ASLDLINK_FLAGS = DEF(GCC_IA32_X64_DLINK_COMMON) --entry 
> _ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)
> -DEFINE GCC_ARM_ASLDLINK_FLAGS  = DEF(GCC_ARM_DLINK_FLAGS) 
> -Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)
> -DEFINE GCC_AARCH64_ASLDLINK_FLAGS  = DEF(GCC_AARCH64_DLINK_FLAGS) 
> -Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)
> +DEFINE GCC_ARM_ASLDLINK_FLAGS  = DEF(GCC_ARM_DLINK_FLAGS) 
> -Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT) 
> DEF(GCC_ARM_AARCH64_ASLDLINK_FLAGS)
> +DEFINE GCC_AARCH64_ASLDLINK_FLAGS  = DEF(GCC_AARCH64_DLINK_FLAGS) 
> -Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT) 
> DEF(GCC_ARM_AARCH64_ASLDLINK_FLAGS)
>  DEFINE GCC_IA32_X64_DLINK_FLAGS= DEF(GCC_IA32_X64_DLINK_COMMON) --entry 
> _$(IMAGE_ENTRY_POINT) --file-alignment 0x20 --section-alignment 0x20 -Map 
> $(DEST_DIR_DEBUG)/$(BASE_NAME).map
>  DEFINE GCC_IPF_DLINK_FLAGS = -nostdlib -O2 --gc-sections --dll 
> -static --entry $(IMAGE_ENTRY_POINT) --undefined $(IMAGE_ENTRY_POINT) -Map 
> $(DEST_DIR_DEBUG)/$(BASE_NAME).map
>  DEFINE GCC_IPF_OBJCOPY_FLAGS   = -I elf64-ia64-little -O efi-bsdrv-ia64
> @@ -4494,12 +4495,12 @@ DEFINE GCC5_ARM_CC_FLAGS = 
> DEF(GCC49_ARM_CC_FLAGS)
>  DEFINE GCC5_ARM_CC_XIPFLAGS  = DEF(GCC49_ARM_CC_XIPFLAGS)
>  DEFINE GCC5_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS)
>  DEFINE GCC5_AARCH64_CC_XIPFLAGS  = DEF(GCC49_AARCH64_CC_XIPFLAGS)
> -DEFINE GCC5_ARM_DLINK_FLAGS  = DEF(GCC49_ARM_DLINK_FLAGS) -no-pie
> +DEFINE GCC5_ARM_DLINK_FLAGS  = DEF(GCC49_ARM_DLINK_FLAGS)
>  DEFINE GCC5_ARM_DLINK2_FLAGS = DEF(GCC49_ARM_DLINK2_FLAGS) -Wno-error
> -DEFINE GCC5_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS) -no-pie
> +DEFINE GCC5_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS)
>  DEFINE GCC5_AARCH64_DLINK2_FLAGS = DEF(GCC49_AARCH64_DLINK2_FLAGS) 
> -Wno-error
> -DEFINE GCC5_ARM_ASLDLINK_FLAGS   = DEF(GCC49_ARM_ASLDLINK_FLAGS) -no-pie
> -DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   = DEF(GCC49_AARCH64_ASLDLINK_FLAGS) 
> -no-pie
> +DEFINE GCC5_ARM_ASLDLINK_FLAGS   = DEF(GCC49_ARM_ASLDLINK_FLAGS)
> +DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   = DEF(GCC49_AARCH64_ASLDLINK_FLAGS)
>
>  
> 
>  #
> --

Thanks for the patch - this fixes regression, I observed when building
edk2 with gcc-linaro-5.3.1-2016.05-x86_64_aarch64-linux-gnu, so

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

Re: [edk2] [PATCH 12/15] ArmVirtPkg/ArmVirtQemu: add ArmVirtMemInfoLib implementation

[Ard Biesheuvel]

On 17 November 2017 at 16:09, Ard Biesheuvel  wrote:
> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
> clean it up slightly (by removing the support for uncached DRAM mappings),
> and turn it into a new ArmVirtMemInfoLib implementation.
>
> In a future patch, we will add this library to the ordinary ArmVirtQemu
> platform as well.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.dsc |   1 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  |  39 
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S  |  24 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   | 101 
> 
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  53 ++
>  5 files changed, 218 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc 
> b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 7e5d584344b4..f50d30388cf2 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -49,6 +49,7 @@ [LibraryClasses.common]
>QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>
>
> ArmPlatformLib|ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ArmQemuRelocatablePlatformLib.inf
> +  
> ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
>
>TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S 
> b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> new file mode 100644
> index ..a1f6a194d59b
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> @@ -0,0 +1,39 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD 
> License
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +#
> +
> +#include 
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mrs   x0, id_aa64mmfr0_el1
> +  adr   x1, .LPARanges
> +  and   x0, x0, #7
> +  ldrb  w1, [x1, x0]
> +  mov   x0, #1
> +  lsl   x0, x0, x1
> +  ret
> +
> +//
> +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> +// physical address space support on this CPU:
> +// 0 == 32 bits, 1 == 36 bits, etc etc
> +// 6 and 7 are reserved
> +//
> +.LPARanges:
> +  .byte 32, 36, 40, 42, 44, 48, -1, -1
> +
> +ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S 
> b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> new file mode 100644
> index ..9cd81529fb3d
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> @@ -0,0 +1,24 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD 
> License
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +#
> +
> +#include 
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mov   r0, #0x
> +  mov   r1, #0x1
> +  bxlr
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c 
> b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> new file mode 100644
> index ..b08305fe4a4f
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -0,0 +1,101 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY 

Re: [edk2] [PATCH 10/15] ArmVirtPkg: introduce ArmVirtMemInfoLib library class

[Ard Biesheuvel]

On 21 November 2017 at 16:27, Laszlo Ersek  wrote:
> On 11/21/17 17:23, Laszlo Ersek wrote:
>> On 11/17/17 17:09, Ard Biesheuvel wrote:
>>> As part of the effort to get rid of ArmPlatformLib (which incorporates
>>> far too many duties in a single library), introduce ArmVirtMemInfoLib
>>> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
>>> get a description of the virtual address space. This will allow us to
>>> remove this functionality from ArmPlatformLib later, or, in the case of
>>> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel 
>>> ---
>>>  ArmVirtPkg/ArmVirtPkg.dec  |  3 ++
>>>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 
>>>  2 files changed, 42 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index a8603e1b80e5..8f656fd2739d 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -30,6 +30,9 @@ [Defines]
>>>  [Includes.common]
>>>Include# Root include for the package
>>>
>>> +[LibraryClasses]
>>> +  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
>>> +
>>>  [Guids.common]
>>>gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 
>>> 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>>>gEarlyPL011BaseAddressGuid   = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 
>>> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>>> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h 
>>> b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>>> new file mode 100644
>>> index ..65be2cbd8082
>>> --- /dev/null
>>> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>>> @@ -0,0 +1,39 @@
>>> +/** @file
>>> +
>>> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
>>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>>> +
>>> +  This program and the accompanying materials are licensed and made 
>>> available
>>> +  under the terms and conditions of the BSD License which accompanies this
>>> +  distribution.  The full text of the license may be found at
>>> +  http://opensource.org/licenses/bsd-license.php
>>> +
>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>>> IMPLIED.
>>> +
>>> +**/
>>> +
>>> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
>>> +#define _ARM_VIRT_MEMINFO_LIB_H_
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +/**
>>> +  Return the Virtual Memory Map of your platform
>>> +
>>> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize 
>>> the MMU
>>> +  on your platform.
>>> +
>>> +  @param[out]   VirtualMemoryMapArray of ARM_MEMORY_REGION_DESCRIPTOR
>>> +describing a Physical-to-Virtual Memory
>>> +mapping. This array must be ended by a
>>> +zero-filled entry
>>> +
>>> +**/
>>> +VOID
>>> +ArmVirtGetMemoryMap (
>>> +  OUT ARM_MEMORY_REGION_DESCRIPTOR**VirtualMemoryMap
>>> +  );
>>> +
>>> +#endif
>>>
>>
>> (1) Since this is a library API, please add EFIAPI to the declaration.
>>
>> (This will affect the instance(s) too.)
>>
>>
>> (2) If it's not overly restrictive, then please mention in the
>> "VirtualMemoryMap" param comment that the map is supposed to be
>> allocated dynamically within the function, using the phase-matching
>> MemoryAllocationLib instance.
>>
>> (Judged from the AllocatePages() call in
>> "ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)
>
> Looking at the patch right after this one, dynamic memory allocation
> appears wrong to spell out in the library interface.
>
> Then I guess the right thing to say would be, "the returned array is
> never supposed to be freed; it is released at the latest when the OS
> takes control".
>
>> With those addressed,
>>
>> Reviewed-by: Laszlo Ersek 
>
> My R-b stands, just please clarify the expected lifetime of the array
> returned, one way or another.
>

Thanks. Simply not freeing it is the current practice everywhere,
given that PrePi and PEI MemoryAllocationLib implementations don't do
FreePages() in the first place. But I agree it should be mentioned
explicitly.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 11/15] ArmVirtPkg/ArmVirtXen: add ArmVirtMemInfoLib implementation

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
> clean it up slightly (by using a static buffer rather than a heap
> allocation, and removing the support for uncached DRAM mappings), and
> turn it into a new ArmVirtMemInfoLib implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtXen.dsc  |  1 +
>  ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S | 39 +
>  ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S | 24 
>  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c   | 61 
> 
>  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf | 41 +
>  5 files changed, 166 insertions(+)

Based on the diffstat, I'm happy to ACK this quickly:

Acked-by: Laszlo Ersek 

I'm adding Julien to the CC list (see commit f724f9d9c72a,
"Maintainers.txt: add Xen reviewer for ArmVirtPkg", 2017-09-22), in case
he wants to comment.

Thanks
Laszlo

> 
> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> index 7a443483d1ac..3df684d13cb0 100644
> --- a/ArmVirtPkg/ArmVirtXen.dsc
> +++ b/ArmVirtPkg/ArmVirtXen.dsc
> @@ -44,6 +44,7 @@ [LibraryClasses]
>
> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
>  
>
> ArmPlatformLib|ArmVirtPkg/Library/ArmXenRelocatablePlatformLib/ArmXenRelocatablePlatformLib.inf
> +  
> ArmVirtMemInfoLib|ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
>  
>TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>  
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S 
> b/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> new file mode 100644
> index ..a1f6a194d59b
> --- /dev/null
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> @@ -0,0 +1,39 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD 
> License
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +#
> +
> +#include 
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mrs   x0, id_aa64mmfr0_el1
> +  adr   x1, .LPARanges
> +  and   x0, x0, #7
> +  ldrb  w1, [x1, x0]
> +  mov   x0, #1
> +  lsl   x0, x0, x1
> +  ret
> +
> +//
> +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> +// physical address space support on this CPU:
> +// 0 == 32 bits, 1 == 36 bits, etc etc
> +// 6 and 7 are reserved
> +//
> +.LPARanges:
> +  .byte 32, 36, 40, 42, 44, 48, -1, -1
> +
> +ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S 
> b/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> new file mode 100644
> index ..9cd81529fb3d
> --- /dev/null
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> @@ -0,0 +1,24 @@
> +#
> +#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD 
> License
> +#  which accompanies this distribution.  The full text of the license may be 
> found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +#
> +
> +#include 
> +
> +//EFI_PHYSICAL_ADDRESS
> +//GetPhysAddrTop (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmGetPhysAddrTop)
> +  mov   r0, #0x
> +  mov   r1, #0x1
> +  bxlr
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c 
> b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> new file mode 100644
> index ..cc806b474560
> --- /dev/null
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> @@ -0,0 +1,61 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE 

Re: [edk2] [PATCH 10/15] ArmVirtPkg: introduce ArmVirtMemInfoLib library class

[Laszlo Ersek]

On 11/21/17 17:23, Laszlo Ersek wrote:
> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> As part of the effort to get rid of ArmPlatformLib (which incorporates
>> far too many duties in a single library), introduce ArmVirtMemInfoLib
>> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
>> get a description of the virtual address space. This will allow us to
>> remove this functionality from ArmPlatformLib later, or, in the case of
>> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmVirtPkg/ArmVirtPkg.dec  |  3 ++
>>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>> index a8603e1b80e5..8f656fd2739d 100644
>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>> @@ -30,6 +30,9 @@ [Defines]
>>  [Includes.common]
>>Include# Root include for the package
>>  
>> +[LibraryClasses]
>> +  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
>> +
>>  [Guids.common]
>>gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 
>> 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>>gEarlyPL011BaseAddressGuid   = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 
>> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h 
>> b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> new file mode 100644
>> index ..65be2cbd8082
>> --- /dev/null
>> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> @@ -0,0 +1,39 @@
>> +/** @file
>> +
>> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials are licensed and made 
>> available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>> IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
>> +#define _ARM_VIRT_MEMINFO_LIB_H_
>> +
>> +#include 
>> +#include 
>> +
>> +/**
>> +  Return the Virtual Memory Map of your platform
>> +
>> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the 
>> MMU
>> +  on your platform.
>> +
>> +  @param[out]   VirtualMemoryMapArray of ARM_MEMORY_REGION_DESCRIPTOR
>> +describing a Physical-to-Virtual Memory
>> +mapping. This array must be ended by a
>> +zero-filled entry
>> +
>> +**/
>> +VOID
>> +ArmVirtGetMemoryMap (
>> +  OUT ARM_MEMORY_REGION_DESCRIPTOR**VirtualMemoryMap
>> +  );
>> +
>> +#endif
>>
> 
> (1) Since this is a library API, please add EFIAPI to the declaration.
> 
> (This will affect the instance(s) too.)
> 
> 
> (2) If it's not overly restrictive, then please mention in the
> "VirtualMemoryMap" param comment that the map is supposed to be
> allocated dynamically within the function, using the phase-matching
> MemoryAllocationLib instance.
> 
> (Judged from the AllocatePages() call in
> "ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)

Looking at the patch right after this one, dynamic memory allocation
appears wrong to spell out in the library interface.

Then I guess the right thing to say would be, "the returned array is
never supposed to be freed; it is released at the latest when the OS
takes control".

> With those addressed,
> 
> Reviewed-by: Laszlo Ersek 

My R-b stands, just please clarify the expected lifetime of the array
returned, one way or another.

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

Re: [edk2] [PATCH 10/15] ArmVirtPkg: introduce ArmVirtMemInfoLib library class

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> As part of the effort to get rid of ArmPlatformLib (which incorporates
> far too many duties in a single library), introduce ArmVirtMemInfoLib
> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
> get a description of the virtual address space. This will allow us to
> remove this functionality from ArmPlatformLib later, or, in the case of
> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtPkg.dec  |  3 ++
>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 
>  2 files changed, 42 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index a8603e1b80e5..8f656fd2739d 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -30,6 +30,9 @@ [Defines]
>  [Includes.common]
>Include# Root include for the package
>  
> +[LibraryClasses]
> +  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
> +
>  [Guids.common]
>gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 
> 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>gEarlyPL011BaseAddressGuid   = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 
> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h 
> b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> new file mode 100644
> index ..65be2cbd8082
> --- /dev/null
> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> @@ -0,0 +1,39 @@
> +/** @file
> +
> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
> +#define _ARM_VIRT_MEMINFO_LIB_H_
> +
> +#include 
> +#include 
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the 
> MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMapArray of ARM_MEMORY_REGION_DESCRIPTOR
> +describing a Physical-to-Virtual Memory
> +mapping. This array must be ended by a
> +zero-filled entry
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR**VirtualMemoryMap
> +  );
> +
> +#endif
> 

(1) Since this is a library API, please add EFIAPI to the declaration.

(This will affect the instance(s) too.)


(2) If it's not overly restrictive, then please mention in the
"VirtualMemoryMap" param comment that the map is supposed to be
allocated dynamically within the function, using the phase-matching
MemoryAllocationLib instance.

(Judged from the AllocatePages() call in
"ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)


With those addressed,

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [PATCH 09/15] ArmVirtPkg/ArmVirtPlatformLib: remove support for uncached mappings

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> QEMU/KVM has very little tolerance for using anything except writeback
> cacheable mappings of DRAM, so let's remove the 'feature' that allows
> us to select uncached mappings at build time.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 15 ++-
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c 
> b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> index d10548f86dfc..4368d05f76ef 100644
> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> @@ -22,10 +22,6 @@
>  // Number of Virtual Memory Map Descriptors
>  #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  5
>  
> -// DDR attributes
> -#define DDR_ATTRIBUTES_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> -#define DDR_ATTRIBUTES_UNCACHED  
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
> -
>  EFI_PHYSICAL_ADDRESS
>  ArmGetPhysAddrTop (
>VOID
> @@ -48,7 +44,6 @@ ArmPlatformGetVirtualMemoryMap (
>IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
>)
>  {
> -  ARM_MEMORY_REGION_ATTRIBUTES  CacheAttributes;
>ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
>  
>ASSERT (VirtualMemoryMap != NULL);
> @@ -65,17 +60,11 @@ ArmPlatformGetVirtualMemoryMap (
>  return;
>}
>  
> -  if (FeaturePcdGet (PcdCacheEnable) == TRUE) {
> -CacheAttributes = DDR_ATTRIBUTES_CACHED;
> -  } else {
> -CacheAttributes = DDR_ATTRIBUTES_UNCACHED;
> -  }
> -
>// System DRAM
>VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
>VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
>VirtualMemoryTable[0].Length   = PcdGet64 (PcdSystemMemorySize);
> -  VirtualMemoryTable[0].Attributes   = CacheAttributes;
> +  VirtualMemoryTable[0].Attributes   = 
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>DEBUG ((EFI_D_INFO, "%a: Dumping System DRAM Memory Map:\n"
>"\tPhysicalBase: 0x%lX\n"
> @@ -104,7 +93,7 @@ ArmPlatformGetVirtualMemoryMap (
>VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
>VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
>VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
> -  VirtualMemoryTable[3].Attributes   = CacheAttributes;
> +  VirtualMemoryTable[3].Attributes   = 
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>// End of Table
>ZeroMem ([4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> 

Looks OK.

I see that PcdCacheEnable is removed from -- or, well, "with" -- all the
affected INF files in the last patch. I also see that the EmbeddedPkg
default for the PCD is FALSE. So it likely makes sense to keep our TRUE
default in "ArmVirt.dsc.inc", for any other (external) modules that
depend on the PCD.

Reviewed-by: Laszlo Ersek 

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

[edk2] [PATCH] BaseTools/tools_def AARCH64 ARM: suppres PIE sections via linker script

[Ard Biesheuvel]

Recent distro builds of GCC 6 enable PIE linking by default, and allow
the previous behavior to be restored by passing the -no-pie command line
argument. This was implemented by commits 1894a7c64c0a and 3380a591232d
but unfortunately, it turns out that GCC 5 does not support this command
line argument, and exits with an error.

To avoid the need for yet another toolchain tag, to distinguish between
GCC 5 and GCC 6, let's use our GCC linker scripts when building objects
from .aslc files. This will ensure that the extra sections that are added
by the PIE linker are discarded from the ELF binary, and so they will not
corrupt the resulting .acpi file.

This reverts

1894a7c64c0a BaseTools/tools_def AARCH64 ARM: disable PIE linking
3380a591232d BaseTools/tools_def AARCH64 ARM: disable PIE linking for .aslc 
sources

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 BaseTools/Conf/tools_def.template | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template 
b/BaseTools/Conf/tools_def.template
index aebd7d558633..4d2a3b7dbe56 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -4356,9 +4356,10 @@ DEFINE GCC_IA32_X64_DLINK_COMMON   = 
DEF(GCC_DLINK_FLAGS_COMMON) --gc-sections
 DEFINE GCC_ARM_AARCH64_DLINK_COMMON= -Wl,--emit-relocs -nostdlib 
-Wl,--gc-sections -u $(IMAGE_ENTRY_POINT) 
-Wl,-e,$(IMAGE_ENTRY_POINT),-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
 DEFINE GCC_ARM_DLINK_FLAGS = DEF(GCC_ARM_AARCH64_DLINK_COMMON) -z 
common-page-size=0x20
 DEFINE GCC_AARCH64_DLINK_FLAGS = DEF(GCC_ARM_AARCH64_DLINK_COMMON) -z 
common-page-size=0x20
+DEFINE GCC_ARM_AARCH64_ASLDLINK_FLAGS = -Wl,--defsym=PECOFF_HEADER_SIZE=0 
DEF(GCC_DLINK2_FLAGS_COMMON) -z common-page-size=0x20
 DEFINE GCC_IA32_X64_ASLDLINK_FLAGS = DEF(GCC_IA32_X64_DLINK_COMMON) --entry 
_ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)
-DEFINE GCC_ARM_ASLDLINK_FLAGS  = DEF(GCC_ARM_DLINK_FLAGS) 
-Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)
-DEFINE GCC_AARCH64_ASLDLINK_FLAGS  = DEF(GCC_AARCH64_DLINK_FLAGS) 
-Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)
+DEFINE GCC_ARM_ASLDLINK_FLAGS  = DEF(GCC_ARM_DLINK_FLAGS) 
-Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT) 
DEF(GCC_ARM_AARCH64_ASLDLINK_FLAGS)
+DEFINE GCC_AARCH64_ASLDLINK_FLAGS  = DEF(GCC_AARCH64_DLINK_FLAGS) 
-Wl,--entry,ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT) 
DEF(GCC_ARM_AARCH64_ASLDLINK_FLAGS)
 DEFINE GCC_IA32_X64_DLINK_FLAGS= DEF(GCC_IA32_X64_DLINK_COMMON) --entry 
_$(IMAGE_ENTRY_POINT) --file-alignment 0x20 --section-alignment 0x20 -Map 
$(DEST_DIR_DEBUG)/$(BASE_NAME).map
 DEFINE GCC_IPF_DLINK_FLAGS = -nostdlib -O2 --gc-sections --dll -static 
--entry $(IMAGE_ENTRY_POINT) --undefined $(IMAGE_ENTRY_POINT) -Map 
$(DEST_DIR_DEBUG)/$(BASE_NAME).map
 DEFINE GCC_IPF_OBJCOPY_FLAGS   = -I elf64-ia64-little -O efi-bsdrv-ia64
@@ -4494,12 +4495,12 @@ DEFINE GCC5_ARM_CC_FLAGS = 
DEF(GCC49_ARM_CC_FLAGS)
 DEFINE GCC5_ARM_CC_XIPFLAGS  = DEF(GCC49_ARM_CC_XIPFLAGS)
 DEFINE GCC5_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS)
 DEFINE GCC5_AARCH64_CC_XIPFLAGS  = DEF(GCC49_AARCH64_CC_XIPFLAGS)
-DEFINE GCC5_ARM_DLINK_FLAGS  = DEF(GCC49_ARM_DLINK_FLAGS) -no-pie
+DEFINE GCC5_ARM_DLINK_FLAGS  = DEF(GCC49_ARM_DLINK_FLAGS)
 DEFINE GCC5_ARM_DLINK2_FLAGS = DEF(GCC49_ARM_DLINK2_FLAGS) -Wno-error
-DEFINE GCC5_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS) -no-pie
+DEFINE GCC5_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS)
 DEFINE GCC5_AARCH64_DLINK2_FLAGS = DEF(GCC49_AARCH64_DLINK2_FLAGS) 
-Wno-error
-DEFINE GCC5_ARM_ASLDLINK_FLAGS   = DEF(GCC49_ARM_ASLDLINK_FLAGS) -no-pie
-DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   = DEF(GCC49_AARCH64_ASLDLINK_FLAGS) 
-no-pie
+DEFINE GCC5_ARM_ASLDLINK_FLAGS   = DEF(GCC49_ARM_ASLDLINK_FLAGS)
+DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   = DEF(GCC49_AARCH64_ASLDLINK_FLAGS)
 
 

 #
-- 
2.11.0

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

Re: [edk2] [PATCH 08/15] ArmVirtPkg/PrePi: remove bogus IntelFrameworkModulePkg.dec dependency

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> PrePi doesn't use anything defined by this package so drop the bogus
> dependency.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf 
> b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> index ae9a088c7256..58290d2d1b76 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -40,7 +40,6 @@ [Packages]
>ArmPkg/ArmPkg.dec
>ArmPlatformPkg/ArmPlatformPkg.dec
>ArmVirtPkg/ArmVirtPkg.dec
> -  IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
>  
>  [LibraryClasses]
>BaseLib
> 

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [PATCH 07/15] ArmVirtPkg/PrePi: remove ArmPlatformStackLib dependency

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> ArmPlatformStackLib has hooks into primary/secondary core PCDs and
> other ArmPlatformLib related junk, so let's simply set the stack
> pointer directly. This is trivial given that our PrePi is unicore
> only.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc  |  1 -
>  ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S | 14 ++
>  ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S | 14 ++
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf |  1 -
>  4 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 50eb8675d1c0..5d7edff104b5 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -93,7 +93,6 @@ [LibraryClasses.common]
>ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
>ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
>ArmGicArchLib|ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
> -  
> ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
>ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
>ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf
>
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.inf
> diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S 
> b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> index 3296aedfe9aa..891cf1fcab40 100644
> --- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> @@ -111,22 +111,12 @@ _GetBaseUefiMemory:
>  
>  _GetStackBase:
>// r1 = The top of the Mpcore Stacks

Before pushing the patch, please consider updating the "r1" comment
here, in the aarch64 version.

Acked-by: Laszlo Ersek 

Thanks
Laszlo

> +  mov   sp, x1
> +
>// Stack for the primary core = PrimaryCoreStack
>MOV32 (x2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
>sub   x22, x1, x2
>  
> -  // Stack for the secondary core = Number of Cores - 1
> -  MOV32 (x1, (FixedPcdGet32(PcdCoreCount) - 1) * 
> FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
> -  sub   x22, x22, x1
> -
> -  // x22 = The base of the MpCore Stacks (primary stack & secondary stacks)
> -  mov   x0, x22
> -  mov   x1, x20
> -  //ArmPlatformStackSet(StackBase, MpId, PrimaryStackSize, 
> SecondaryStackSize)
> -  MOV32 (x2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> -  MOV32 (x3, FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
> -  blASM_PFX(ArmPlatformStackSet)
> -
>mov   x0, x20
>mov   x1, x21
>mov   x2, x22
> diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S 
> b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> index a918c191432e..ced08593e9de 100644
> --- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> @@ -120,22 +120,12 @@ _GetBaseUefiMemory:
>  
>  _GetStackBase:
>// r1 = The top of the Mpcore Stacks
> +  mov   sp, r1
> +
>// Stack for the primary core = PrimaryCoreStack
>MOV32 (r2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
>sub   r9, r1, r2
>  
> -  // Stack for the secondary core = Number of Cores - 1
> -  MOV32 (r1, (FixedPcdGet32(PcdCoreCount) - 1) * 
> FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
> -  sub   r9, r9, r1
> -
> -  // r9 = The base of the MpCore Stacks (primary stack & secondary stacks)
> -  mov   r0, r9
> -  mov   r1, r10
> -  //ArmPlatformStackSet(StackBase, MpId, PrimaryStackSize, 
> SecondaryStackSize)
> -  MOV32 (r2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> -  MOV32 (r3, FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
> -  blASM_PFX(ArmPlatformStackSet)
> -
>mov   r0, r10
>mov   r1, r11
>mov   r2, r9
> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf 
> b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> index e816e9583da8..ae9a088c7256 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -54,7 +54,6 @@ [LibraryClasses]
>LzmaDecompressLib
>PeCoffGetEntryPointLib
>PrePiLib
> -  ArmPlatformStackLib
>MemoryAllocationLib
>HobLib
>PrePiHobListPointerLib
> 

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

Re: [edk2] [PATCH 06/15] ArmVirtPkg/PrePi: move DRAM discovery code into PrePi

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> ArmVirtQemuKernel and ArmVirtXen use essentially the same code to
> retrieve DRAM information from the DT /memory node at early boot,
> and invoke it via the ArmPlatformPeiBootAction () hook exposed by
> ArmPlatformLib. Let's move this code into the PrePi implementation
> these platforms share between them (and not with any other platforms)
> so we can eliminate another dependency on the messy ArmPlatformLib.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S | 77 -
>  ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S | 71 +++
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf |  2 +
>  ArmVirtPkg/PrePi/FdtParser.c| 90 
>  4 files changed, 238 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S 
> b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> index 7a9c0c3787cc..3296aedfe9aa 100644
> --- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> @@ -49,8 +49,7 @@ ASM_FUNC(_ModuleEntryPoint)
>b .Lreloc_loop
>  .Lreloc_done:
>  
> -  // Do early platform specific actions
> -  blASM_PFX(ArmPlatformPeiBootAction)
> +  blASM_PFX(DiscoverDramFromDt)
>  
>// Get ID of this CPU in Multicore system
>blASM_PFX(ArmReadMpidr)
> @@ -140,3 +139,77 @@ _GetStackBase:
>  
>  _NeverReturn:
>b _NeverReturn
> +
> +// VOID
> +// DiscoverDramFromDt (
> +//   VOID   *DeviceTreeBaseAddress,   // passed by loader in x0
> +//   VOID   *ImageBase// passed by FDF trampoline in x1
> +//   );
> +ASM_PFX(DiscoverDramFromDt):
> +  //
> +  // If we are booting from RAM using the Linux kernel boot protocol, x0 will
> +  // point to the DTB image in memory. Otherwise, use the default value 
> defined
> +  // by the platform.
> +  //
> +  cbnz  x0, 0f
> +  ldr   x0, PcdGet64 (PcdDeviceTreeInitialBaseAddress)
> +
> +0:mov   x29, x30// preserve LR
> +  mov   x28, x0 // preserve DTB pointer
> +  mov   x27, x1 // preserve base of image pointer
> +
> +  //
> +  // The base of the runtime image has been preserved in x1. Check whether
> +  // the expected magic number can be found in the header.
> +  //
> +  ldr   w8, .LArm64LinuxMagic
> +  ldr   w9, [x1, #0x38]
> +  cmp   w8, w9
> +  bne   .Lout
> +
> +  //
> +  //
> +  // OK, so far so good. We have confirmed that we likely have a DTB and are
> +  // booting via the arm64 Linux boot protocol. Update the base-of-image PCD
> +  // to the actual relocated value, and add the shift of PcdFdBaseAddress to
> +  // PcdFvBaseAddress as well
> +  //
> +  adr   x8, PcdGet64 (PcdFdBaseAddress)
> +  adr   x9, PcdGet64 (PcdFvBaseAddress)
> +  ldr   x6, [x8]
> +  ldr   x7, [x9]
> +  sub   x7, x7, x6
> +  add   x7, x7, x1
> +  str   x1, [x8]
> +  str   x7, [x9]
> +
> +  //
> +  // Discover the memory size and offset from the DTB, and record in the
> +  // respective PCDs. This will also return false if a corrupt DTB is
> +  // encountered. Since we are calling a C function, use the window at the
> +  // beginning of the FD image as a temp stack.
> +  //
> +  adr   x1, PcdGet64 (PcdSystemMemoryBase)
> +  adr   x2, PcdGet64 (PcdSystemMemorySize)
> +  mov   sp, x7
> +  blFindMemnode
> +  cbz   x0, .Lout
> +
> +  //
> +  // Copy the DTB to the slack space right after the 64 byte arm64/Linux 
> style
> +  // image header at the base of this image (defined in the FDF), and record 
> the
> +  // pointer in PcdDeviceTreeInitialBaseAddress.
> +  //
> +  adr   x8, PcdGet64 (PcdDeviceTreeInitialBaseAddress)
> +  add   x27, x27, #0x40
> +  str   x27, [x8]
> +
> +  mov   x0, x27
> +  mov   x1, x28
> +  blCopyFdt
> +
> +.Lout:
> +  retx29
> +
> +.LArm64LinuxMagic:
> +  .byte   0x41, 0x52, 0x4d, 0x64
> diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S 
> b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> index eebf660acdb2..a918c191432e 100644
> --- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> @@ -148,3 +148,74 @@ _GetStackBase:
>  
>  _NeverReturn:
>b _NeverReturn
> +
> +ASM_PFX(ArmPlatformPeiBootAction):
> +  //
> +  // If we are booting from RAM using the Linux kernel boot protocol, r0 will
> +  // point to the DTB image in memory. Otherwise, use the default value 
> defined
> +  // by the platform.
> +  //
> +  teq   r0, #0
> +  bne   0f
> +  LDRL  (r0, PcdGet64 (PcdDeviceTreeInitialBaseAddress))
> +
> +0:mov   r11, r14// preserve LR
> +  mov   r10, r0 // preserve DTB pointer
> +  mov   r9, r1  // preserve base of image pointer
> +
> +  //
> +  // The base of the runtime image has been preserved in r1. Check whether
> +  // the expected magic number can be found in the header.
> +  //
> +  ldr   r8, .LArm32LinuxMagic

Re: [edk2] [PATCH 05/15] ArmVirtPkg/PrePi: remove dependency on ArmPlatformLib

[Laszlo Ersek]

On 11/21/17 16:46, Laszlo Ersek wrote:
> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> Remove the pointless dependency on ArmPlatformLib: none of the code we
>> call from it actually does anything useful.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 1 -
>>  ArmVirtPkg/PrePi/PrePi.c| 6 ++
>>  ArmVirtPkg/PrePi/PrePi.h| 1 -
>>  3 files changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf 
>> b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
>> index 5e706934f69f..1d79b1360c22 100755
>> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
>> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
>> @@ -52,7 +52,6 @@ [LibraryClasses]
>>LzmaDecompressLib
>>PeCoffGetEntryPointLib
>>PrePiLib
>> -  ArmPlatformLib
>>ArmPlatformStackLib
>>MemoryAllocationLib
>>HobLib
>> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
>> index c4fa979c43ef..fce4ab9428a5 100755
>> --- a/ArmVirtPkg/PrePi/PrePi.c
>> +++ b/ArmVirtPkg/PrePi/PrePi.c
>> @@ -13,6 +13,7 @@
>>  **/
>>  
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>> @@ -85,7 +86,7 @@ PrePiMain (
>>BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 
>> (PcdPrePiCpuIoSize));
>>  
>>// Set the Boot Mode
>> -  SetBootMode (ArmPlatformGetBootMode ());
>> +  SetBootMode (BOOT_WITH_FULL_CONFIGURATION);
>>  
>>// Initialize Platform HOBs (CpuHob and FvHob)
>>Status = PlatformPeim ();
>> @@ -123,9 +124,6 @@ CEntryPoint (
>>  {
>>UINT64   StartTimeStamp;
>>  
>> -  // Initialize the platform specific controllers
>> -  ArmPlatformInitialize (MpId);
>> -
>>if (PerformanceMeasurementEnabled ()) {
>>  // Initialize the Timer Library to setup the Timer HW controller
>>  TimerConstructor ();
>> diff --git a/ArmVirtPkg/PrePi/PrePi.h b/ArmVirtPkg/PrePi/PrePi.h
>> index d3189c0b8a6f..1ba88e0506cb 100644
>> --- a/ArmVirtPkg/PrePi/PrePi.h
>> +++ b/ArmVirtPkg/PrePi/PrePi.h
>> @@ -25,7 +25,6 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>>  
>>  #define SerialPrint(txt)  SerialPortWrite (txt, AsciiStrLen(txt)+1);
>>  
>>
> 
> ArmPlatformGetBootMode() and ArmPlatformInitialize() have identical
> implementations between the ArmQemuRelocatablePlatformLib and the
> ArmXenRelocatablePlatformLib instances, so I agree common handling is
> justified here.
> 
> The ArmPlatformInitialize() call is not replaced by the function's contents:
> 
>   ASSERT (!FeaturePcdGet (PcdSystemMemoryInitializeInSec));
> 
> but I guess this assertion is satisfied simply by the PrePi nature of
> the platform -- once we stop sharing the code like before, the assert
> becomes useless (there's no possible mis-build to catch). I think,
> 
> Reviewed-by: Laszlo Ersek 
> 

heh, that was supposed to be: "I think.". Nothing else to add :)
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 05/15] ArmVirtPkg/PrePi: remove dependency on ArmPlatformLib

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Remove the pointless dependency on ArmPlatformLib: none of the code we
> call from it actually does anything useful.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 1 -
>  ArmVirtPkg/PrePi/PrePi.c| 6 ++
>  ArmVirtPkg/PrePi/PrePi.h| 1 -
>  3 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf 
> b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> index 5e706934f69f..1d79b1360c22 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -52,7 +52,6 @@ [LibraryClasses]
>LzmaDecompressLib
>PeCoffGetEntryPointLib
>PrePiLib
> -  ArmPlatformLib
>ArmPlatformStackLib
>MemoryAllocationLib
>HobLib
> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> index c4fa979c43ef..fce4ab9428a5 100755
> --- a/ArmVirtPkg/PrePi/PrePi.c
> +++ b/ArmVirtPkg/PrePi/PrePi.c
> @@ -13,6 +13,7 @@
>  **/
>  
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -85,7 +86,7 @@ PrePiMain (
>BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));
>  
>// Set the Boot Mode
> -  SetBootMode (ArmPlatformGetBootMode ());
> +  SetBootMode (BOOT_WITH_FULL_CONFIGURATION);
>  
>// Initialize Platform HOBs (CpuHob and FvHob)
>Status = PlatformPeim ();
> @@ -123,9 +124,6 @@ CEntryPoint (
>  {
>UINT64   StartTimeStamp;
>  
> -  // Initialize the platform specific controllers
> -  ArmPlatformInitialize (MpId);
> -
>if (PerformanceMeasurementEnabled ()) {
>  // Initialize the Timer Library to setup the Timer HW controller
>  TimerConstructor ();
> diff --git a/ArmVirtPkg/PrePi/PrePi.h b/ArmVirtPkg/PrePi/PrePi.h
> index d3189c0b8a6f..1ba88e0506cb 100644
> --- a/ArmVirtPkg/PrePi/PrePi.h
> +++ b/ArmVirtPkg/PrePi/PrePi.h
> @@ -25,7 +25,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  
>  #define SerialPrint(txt)  SerialPortWrite (txt, AsciiStrLen(txt)+1);
>  
> 

ArmPlatformGetBootMode() and ArmPlatformInitialize() have identical
implementations between the ArmQemuRelocatablePlatformLib and the
ArmXenRelocatablePlatformLib instances, so I agree common handling is
justified here.

The ArmPlatformInitialize() call is not replaced by the function's contents:

  ASSERT (!FeaturePcdGet (PcdSystemMemoryInitializeInSec));

but I guess this assertion is satisfied simply by the PrePi nature of
the platform -- once we stop sharing the code like before, the assert
becomes useless (there's no possible mis-build to catch). I think,

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

Re: [edk2] [PATCH 04/15] ArmVirtPkg/PrePi: remove bogus primary core check

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> QEMU and KVM based ARM/AARCH64 virtual machines only enter UEFI on
> a single core, so ArmPlatformIsPrimaryCore() always returns true.
> And even if it didn't, our code does absolutely nothing meaningful
> based on its return value, so don't bother calling it, and remove
> another frivolous dependency on ArmPlatformLib.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S | 7 ---
>  ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S | 7 ---
>  2 files changed, 14 deletions(-)
> 
> diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S 
> b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> index cc8b47e69026..7a9c0c3787cc 100644
> --- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> @@ -128,13 +128,6 @@ _GetStackBase:
>MOV32 (x3, FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
>blASM_PFX(ArmPlatformStackSet)
>  
> -  // Is it the Primary Core ?
> -  mov   x0, x10
> -  blASM_PFX(ArmPlatformIsPrimaryCore)
> -  cmp   x0, #1
> -  bne   _PrepareArguments
> -
> -_PrepareArguments:
>mov   x0, x20
>mov   x1, x21
>mov   x2, x22
> diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S 
> b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> index 59028d0a553e..eebf660acdb2 100644
> --- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> @@ -136,13 +136,6 @@ _GetStackBase:
>MOV32 (r3, FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
>blASM_PFX(ArmPlatformStackSet)
>  
> -  // Is it the Primary Core ?
> -  mov   r0, r10
> -  blASM_PFX(ArmPlatformIsPrimaryCore)
> -  cmp   r0, #1
> -  bne   _PrepareArguments
> -
> -_PrepareArguments:
>mov   r0, r10
>mov   r1, r11
>mov   r2, r9
> 

I'll assume ArmPlatformIsPrimaryCore has no side effects. Beyond that,
even I can see that

  conditional-jump LABEL
LABEL: ...

is a no-op :)

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

Re: [edk2] [PATCH 03/15] ArmVirtPkg/PrePi: remove unused GetPlatformPpi() function

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Remove GetPlatformPpi() from PrePi: it is not used anywhere.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/PrePi/PrePi.c | 24 
>  1 file changed, 24 deletions(-)
> 
> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> index 3679087aec4d..c4fa979c43ef 100755
> --- a/ArmVirtPkg/PrePi/PrePi.c
> +++ b/ArmVirtPkg/PrePi/PrePi.c
> @@ -35,30 +35,6 @@ ProcessLibraryConstructorList (
>VOID
>);
>  
> -EFI_STATUS
> -GetPlatformPpi (
> -  IN  EFI_GUID  *PpiGuid,
> -  OUT VOID  **Ppi
> -  )
> -{
> -  UINTN   PpiListSize;
> -  UINTN   PpiListCount;
> -  EFI_PEI_PPI_DESCRIPTOR  *PpiList;
> -  UINTN   Index;
> -
> -  PpiListSize = 0;
> -  ArmPlatformGetPlatformPpiList (, );
> -  PpiListCount = PpiListSize / sizeof(EFI_PEI_PPI_DESCRIPTOR);
> -  for (Index = 0; Index < PpiListCount; Index++, PpiList++) {
> -if (CompareGuid (PpiList->Guid, PpiGuid) == TRUE) {
> -  *Ppi = PpiList->Ppi;
> -  return EFI_SUCCESS;
> -}
> -  }
> -
> -  return EFI_NOT_FOUND;
> -}
> -
>  VOID
>  PrePiMain (
>IN  UINTN UefiMemoryBase,
> 

I suggest removing the function declaration as well, from "PrePi.h"
(assuming that's possible). With that:

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

Re: [edk2] [PATCH 02/15] ArmVirtPkg/PrePi: run all library constructors by hand

[Laszlo Ersek]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Instead of invoking the library constructors of some libraries by
> hand, invoke the generated function ProcessLibraryConstructorList
> in AutoGen.c so all constructors are executed.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/PrePi/PrePi.c | 13 +++--
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> index c69cff249e80..3679087aec4d 100755
> --- a/ArmVirtPkg/PrePi/PrePi.c
> +++ b/ArmVirtPkg/PrePi/PrePi.c
> @@ -29,15 +29,9 @@
>  #include "PrePi.h"
>  #include "LzmaDecompress.h"
>  
> -EFI_STATUS
> -EFIAPI
> -ExtractGuidedSectionLibConstructor (
> -  VOID
> -  );
> -
> -EFI_STATUS
> +VOID
>  EFIAPI
> -LzmaDecompressLibConstructor (
> +ProcessLibraryConstructorList (
>VOID
>);
>  
> @@ -125,8 +119,7 @@ PrePiMain (
>PERF_START (NULL, "PEI", NULL, StartTimeStamp);
>  
>// SEC phase needs to run library constructors by hand.
> -  ExtractGuidedSectionLibConstructor ();
> -  LzmaDecompressLibConstructor ();
> +  ProcessLibraryConstructorList ();
>  
>// Build HOBs to pass up our version of stuff the DXE Core needs to save 
> space
>BuildPeCoffLoaderHob ();
> 

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

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Ard Biesheuvel]

On 21 November 2017 at 13:24, Udit Kumar  wrote:
> Thanks Ard,
>
> My intend of this email to know, what is right way to define HID and CID in 
> ACPI firmware i.e
>
> Device(XYZ) {
> Name(_HID, "NXP0001")
> Name(_CID, "PRP0001")
>   Device(Slave1) {
> Name(_CID, "PRP0001")
>  }
> }
> is ok or
>
>
> Device(XYZ) {
> Name(_HID, "NXP0001")
> Name(_CID, " NXP0001")
>   Device(Slave1) {
> Name(_CID, " NXP0002")
>  }
> }
> Seems good
>

I don't think it makes a lot of sense to use the same value for _HID
and _CID, so you can just drop the latter.

> For sure, AML methods (as needed _ON/OFF/RST/STA etc) /_DSD will to be coded 
> in table using either of.
>
> Please see more in line
>
> Regards
> Udit
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Tuesday, November 21, 2017 5:59 PM
>> To: Udit Kumar 
>> Cc: Leif Lindholm ; edk2-devel@lists.01.org; Varun
>> Sethi ; Daniel Thompson ;
>> Graeme Gregory 
>> Subject: Re: [RFC] ACPI table HID/CID allocation
>>
>> On 21 November 2017 at 11:32, Udit Kumar  wrote:
>> >
>> >> On 21 November 2017 at 09:59, Udit Kumar  wrote:
>> >> > Thanks Ard.
>> >> > Below table was for example. I am not converting whole DT to ACPI
>> >> > tables :) My idea is to reduce Linux patches for probing as possible.
>> >> > Also keeping firmware and OS separately then Let firmware expose
>> >> > both way (HID and PRP1) and Linux to decide binding
>> >>
>> >> No.
>> >>
>> >> You are still assuming ACPI and DT device drivers bind at the same
>> >> level, and they don't.
>> >
>> > No, my above comments was just limited to binding.
>>
>> Yes, but if you leave it to the OS to decide which binding it uses, you will 
>> have to
>> support all of them into eternity. And the more detailed binding you need to
>> support may limit you in the available choices when it comes to making
>> hardware changes, something ACPI allows you to do.
>
> Thanks,
> Is this ok to say , we can provide max same properties in driver as of DT
> (with _DSD) , But prefer to use AML methods.
> With note, clocking regulators are out of scope here.
>

Yes. _DSD may be used to describe device specific data that goes
beyond what ACPI can express natively. Using _DSD to describe clocks
and regulators is an absolute no-go. Putting things like "status" or
"dma-coherent" in _DSD properties is absolutely unacceptable as well.
But even things like initialization data that the driver programs into
the device a single time really do not belong in _DSD. Instead, it
should be the firmware that initializes the device, and presents it to
the OS in its initialized state.

>
>> > Right, here device driver should know that device is present in
>> > system, I see probe function (device-driver binding) is way to know this.
>> > Then driver can execute AML methods exposed by firmware.
>> >
>>
>> Yes, declaring the presence of the device is the main purpose of describing 
>> it
>> both in ACPI and in DT.
>>
>> >> An ACPI device has AML methods to manage power state and perform
>> >> other device related low-level tasks. The device driver has no
>> >> knowledge of the hardware beyond what it needs to invoke those abstract
>> methods.
>> >
>> > You meant, If we need to update driver for AML methods then why not to
>> update binding as well . ?
>> >
>>
>> No. What I am saying is that you should not expose two different bindings, 
>> and
>> let the OS choose.
>
> Ok, got it , then PRP1 stuff should be used only at urgent or say
> no other choice left , Right ?
>

Yes.

>> > On side track,
>> >  With limited search,  I got many each driver is having (acpi_id and
>> > of_id), looks, acpi support is added on the top of DT flavored drivers.
>> > and therefore acpi tables are following the same.
>> > Sorry to say, reference I am looking at (edk2-platform) JunoPkg and
>> > VExpressPkg, Has following devices has description similar to Device tree
>> > Device (NET0) {
>> > Device (SREG) {
>> > Device (VIRT) {
>>
>> These were taken from the ACPI tables for an emulator
>>
>> >Device(KMI0) {
>>
>> I have no clue what kind of device this is
>>
>> >Device(ETH0) {
>>
>> This one uses _DSD as intended, to set device properties in a DT compatible
>> fashion, but note that this does *not* include the 'compatible' property, nor
>> anything else that ACPI defines itself (status, dma-coherent, etc)
>
> Let me put in simple way,
> A simple driver can survive only with _DSD in acpi world. 
> (clocking/regulators are used
> as said in UEFI/ACPI)
>

Why can a simple driver only survive with _DSD? That statement does
not make any sense to me.

> Copying 

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

Jian,

On 11/21/17 07:17, Jian J Wang wrote:
>> v7:
>>   Merge memory map after filtering paging attributes
> 
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h  | 18 ++
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 21 +++
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 
> +---
>  4 files changed, 112 insertions(+), 22 deletions(-)
> 

I don't have capacity to retest and re-review the series.

Considering the following two options, I like none of them:

(1) Version 7 is merged with my feedback tags from v6. I don't like this
because I didn't review or test version 7.

(2) Version 7 is merged without my feedback tags. I don't like this
because I've put a lot of BZ writeup, and patch review and testing
effort for this series, and I'd like the commit log to reflect that.


Instead, I would like to request the following, for v8:

Please submit a series that consists of three patches:

- patch v8 1/3: identical to v6 1/2, except for the code comment update,
- patch v8 2/3: identical to v6 2/2,
- patch v8 3/3: please implement the merging of the memory map as a
separate patch.

Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
Reviewed-by tags, from v6.

Patch v8 3/3 should be reviewed / tested separately by others. I don't
think I can find the capacity for that at the moment.

This approach will correctly reflect all the work done thus far, and it
will provide the desired result for the code as well.

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

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Udit Kumar]

Thanks Ard, 

My intend of this email to know, what is right way to define HID and CID in 
ACPI firmware i.e 

Device(XYZ) { 
Name(_HID, "NXP0001") 
Name(_CID, "PRP0001")
  Device(Slave1) {
Name(_CID, "PRP0001")
 }
} 
is ok or 


Device(XYZ) { 
Name(_HID, "NXP0001") 
Name(_CID, " NXP0001")
  Device(Slave1) {
Name(_CID, " NXP0002")
 }
} 
Seems good

For sure, AML methods (as needed _ON/OFF/RST/STA etc) /_DSD will to be coded in 
table using either of.

Please see more in line 

Regards
Udit

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, November 21, 2017 5:59 PM
> To: Udit Kumar 
> Cc: Leif Lindholm ; edk2-devel@lists.01.org; Varun
> Sethi ; Daniel Thompson ;
> Graeme Gregory 
> Subject: Re: [RFC] ACPI table HID/CID allocation
> 
> On 21 November 2017 at 11:32, Udit Kumar  wrote:
> >
> >> On 21 November 2017 at 09:59, Udit Kumar  wrote:
> >> > Thanks Ard.
> >> > Below table was for example. I am not converting whole DT to ACPI
> >> > tables :) My idea is to reduce Linux patches for probing as possible.
> >> > Also keeping firmware and OS separately then Let firmware expose
> >> > both way (HID and PRP1) and Linux to decide binding
> >>
> >> No.
> >>
> >> You are still assuming ACPI and DT device drivers bind at the same
> >> level, and they don't.
> >
> > No, my above comments was just limited to binding.
> 
> Yes, but if you leave it to the OS to decide which binding it uses, you will 
> have to
> support all of them into eternity. And the more detailed binding you need to
> support may limit you in the available choices when it comes to making
> hardware changes, something ACPI allows you to do.

Thanks,  
Is this ok to say , we can provide max same properties in driver as of DT
(with _DSD) , But prefer to use AML methods.
With note, clocking regulators are out of scope here. 

 
> > Right, here device driver should know that device is present in
> > system, I see probe function (device-driver binding) is way to know this.
> > Then driver can execute AML methods exposed by firmware.
> >
> 
> Yes, declaring the presence of the device is the main purpose of describing it
> both in ACPI and in DT.
> 
> >> An ACPI device has AML methods to manage power state and perform
> >> other device related low-level tasks. The device driver has no
> >> knowledge of the hardware beyond what it needs to invoke those abstract
> methods.
> >
> > You meant, If we need to update driver for AML methods then why not to
> update binding as well . ?
> >
> 
> No. What I am saying is that you should not expose two different bindings, and
> let the OS choose.

Ok, got it , then PRP1 stuff should be used only at urgent or say 
no other choice left , Right ? 
 
> > On side track,
> >  With limited search,  I got many each driver is having (acpi_id and
> > of_id), looks, acpi support is added on the top of DT flavored drivers.
> > and therefore acpi tables are following the same.
> > Sorry to say, reference I am looking at (edk2-platform) JunoPkg and
> > VExpressPkg, Has following devices has description similar to Device tree
> > Device (NET0) {
> > Device (SREG) {
> > Device (VIRT) {
> 
> These were taken from the ACPI tables for an emulator
> 
> >Device(KMI0) {
> 
> I have no clue what kind of device this is
> 
> >Device(ETH0) {
> 
> This one uses _DSD as intended, to set device properties in a DT compatible
> fashion, but note that this does *not* include the 'compatible' property, nor
> anything else that ACPI defines itself (status, dma-coherent, etc)

Let me put in simple way,
A simple driver can survive only with _DSD in acpi world. (clocking/regulators 
are used
as said in UEFI/ACPI)

Copying below from Juno, 
Are below kind of tables are acceptable ? 

Device(ETH0) {
  Name(_HID, "ARMH9118")
  Name(_UID, Zero)
  Name(_CRS, ResourceTemplate() {
  Memory32Fixed(ReadWrite, 0x1800, 0x1000)
  Interrupt(ResourceConsumer, Level, ActiveHigh, Exclusive) { 192 }
  })
  Name(_DSD, Package() {
   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
   Package() {
   Package(2) {"phy-mode", "mii"},
   Package(2) {"reg-io-width", 4 },
   Package(2) {"smsc,irq-active-high",1},
   Package(2) {"smsc,irq-push-pull",1}
  }
  }) // _DSD()
}

> > Where no AML method is exposed. Then I expect OS driver to manage this
> device.
> > While grepping over few other edk2-platforms.  Looks some of methods
> > are 

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Ard Biesheuvel]

On 21 November 2017 at 11:32, Udit Kumar  wrote:
>
>> On 21 November 2017 at 09:59, Udit Kumar  wrote:
>> > Thanks Ard.
>> > Below table was for example. I am not converting whole DT to ACPI
>> > tables :) My idea is to reduce Linux patches for probing as possible.
>> > Also keeping firmware and OS separately then Let firmware expose both
>> > way (HID and PRP1) and Linux to decide binding
>>
>> No.
>>
>> You are still assuming ACPI and DT device drivers bind at the same level, and
>> they don't.
>
> No, my above comments was just limited to binding.

Yes, but if you leave it to the OS to decide which binding it uses,
you will have to support all of them into eternity. And the more
detailed binding you need to support may limit you in the available
choices when it comes to making hardware changes, something ACPI
allows you to do.

> Right, here device driver should know that device is present in system,
> I see probe function (device-driver binding) is way to know this.
> Then driver can execute AML methods exposed by firmware.
>

Yes, declaring the presence of the device is the main purpose of
describing it both in ACPI and in DT.

>> An ACPI device has AML methods to manage power state and perform other
>> device related low-level tasks. The device driver has no knowledge of the
>> hardware beyond what it needs to invoke those abstract methods.
>
> You meant, If we need to update driver for AML methods then why not to update 
> binding as well . ?
>

No. What I am saying is that you should not expose two different
bindings, and let the OS choose.

> On side track,
>  With limited search,  I got many each driver is having (acpi_id and of_id),
> looks, acpi support is added on the top of DT flavored drivers.
> and therefore acpi tables are following the same.
> Sorry to say, reference I am looking at (edk2-platform) JunoPkg and 
> VExpressPkg,
> Has following devices has description similar to Device tree
> Device (NET0) {
> Device (SREG) {
> Device (VIRT) {

These were taken from the ACPI tables for an emulator

>Device(KMI0) {

I have no clue what kind of device this is

>Device(ETH0) {

This one uses _DSD as intended, to set device properties in a DT
compatible fashion, but note that this does *not* include the
'compatible' property, nor anything else that ACPI defines itself
(status, dma-coherent, etc)

> Where no AML method is exposed. Then I expect OS driver to manage this device.
> While grepping over few other edk2-platforms.  Looks some of methods
> are abstracted, not whole device.
>

So what is your point? Why does this argue in favor of allowing
PRP0001 + compatible?

>> A DT device describes everything in detail, and requires clock and regulator
>> drivers and other bits and pieces that are tightly coupled to details of the
>> hardware.
>>
>> So now, you have the worst of both worlds:
>>
>> - you need to implement all of this in firmware so ACPI can support it,
>> - you have to expose the internals to the OS so DT can support it.
>
> Yes, for time being or may be longer, DT support needs to be there
> along with ACPI introduction.
>
> Are you suggesting here to abstract whole device details from
> OS and expose AML methods to be used by device driver.
> And maintain two drivers instead of fitting DT style driver into ACPI world ?
>

No. You should update the driver so it can support both ACPI and DT
bindings. That way, the driver can use the abstractions offered by
ACPI when it can, and can invoke the clock and regulator frameworks
and other low level infrastructure only when it needs to.

Let me try to illustrate this a bit better: imagine a NXP customer
that runs a datacenter that has 10,000 NXP servers, and is using RHEL
x.y. The business is going well, and at some point, he wants to order
another 2,000 servers. Unfortunately, the vendor cannot supply the
exact same revision of the hardware, and the latest revision uses some
component that is not supported in RHEL x.y.

You now have made your customer very unhappy. He invested in RHEL and
ACPI based servers precisely to avoid this situation. The cost of
adding 2,000 servers now includes the cost of upgrading the other
10,000 servers to a new OS version or the cost of supporting two
different OS versions at the same time, for a reason that is not
justifiable.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] BaseTools/tools_def AARCH64 ARM: disable PIE linking for .aslc sources

[Marcin Wojtas]

Hi Ard,

2017-11-16 16:45 GMT+01:00 Ard Biesheuvel :
> On 16 November 2017 at 15:31, Gao, Liming  wrote:
>> Ard:
>>   Does this error only happen on ACPI table compiling? But, I see -no-pie is 
>> also in normal DLINK flag. Why is the driver not compiled failed?
>>
>
> The main difference is that the ACPI tables don't tolerate any padding
> at the start of the binary image. This is different for ELF binaries
> that are converted to PE/COFF, given that the entry point is exposed
> in the header, so the padding is just ignored. However, we should
> still try to omit those sections if we can.
>
>
>
>>> -Original Message-
>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>> Sent: Thursday, November 16, 2017 11:09 PM
>>> To: Marcin Wojtas 
>>> Cc: Gao, Liming ; edk2-devel@lists.01.org; 
>>> daniel.thomp...@linaro.org; leif.lindh...@linaro.org
>>> Subject: Re: [edk2] [PATCH] BaseTools/tools_def AARCH64 ARM: disable PIE 
>>> linking for .aslc sources
>>>
>>> On 16 November 2017 at 15:07, Marcin Wojtas  wrote:
>>> > Hi Ard,
>>> >
>>> > 2017-11-16 15:48 GMT+01:00 Ard Biesheuvel :
>>> >> On 16 November 2017 at 14:38, Marcin Wojtas  wrote:
>>> >>> Hi Ard,
>>> >>>
>>> >>> With both PIE disabling patches for AARCH64, when compiling ACPI tables 
>>> >>> with
>>> >>> gcc-linaro-5.3.1-2016.05-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-
>>> >>> I get following errors:
>>> >>> [...]
>>> >>> aarch64-linux-gnu-gcc: error: unrecognized command line option '-no-pie'
>>> >>> Do I understand correctly, that I should either revert those patches
>>> >>> or upgrade to the newer toolchain?
>>> >>>
>>> >>
>>> >> Ugh.
>>> >>
>>> >> I thought GCC 5 and later implemented -no-pie, but apparently not.
>>> >>
>>> >> Does this fix your build? I will need to check whether it fixes the
>>> >> original issue, but hopefully your toolchain doesn't choke on this:
>>> >>
>>> >> diff --git a/BaseTools/Conf/tools_def.template
>>> >> b/BaseTools/Conf/tools_def.template
>>> >> index aebd7d558633..111fe8da7773 100755
>>> >> --- a/BaseTools/Conf/tools_def.template
>>> >> +++ b/BaseTools/Conf/tools_def.template
>>> >> @@ -4496,10 +4496,10 @@ DEFINE GCC5_AARCH64_CC_FLAGS =
>>> >> DEF(GCC49_AARCH64_CC_FLAGS)
>>> >>  DEFINE GCC5_AARCH64_CC_XIPFLAGS  = DEF(GCC49_AARCH64_CC_XIPFLAGS)
>>> >>  DEFINE GCC5_ARM_DLINK_FLAGS  = DEF(GCC49_ARM_DLINK_FLAGS) 
>>> >> -no-pie
>>> >>  DEFINE GCC5_ARM_DLINK2_FLAGS = DEF(GCC49_ARM_DLINK2_FLAGS) 
>>> >> -Wno-error
>>> >> -DEFINE GCC5_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS) 
>>> >> -no-pie
>>> >> +DEFINE GCC5_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS)
>>> >> -Wl,-no-pie
>>> >>  DEFINE GCC5_AARCH64_DLINK2_FLAGS =
>>> >> DEF(GCC49_AARCH64_DLINK2_FLAGS) -Wno-error
>>> >>  DEFINE GCC5_ARM_ASLDLINK_FLAGS   = DEF(GCC49_ARM_ASLDLINK_FLAGS) 
>>> >> -no-pie
>>> >> -DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   =
>>> >> DEF(GCC49_AARCH64_ASLDLINK_FLAGS) -no-pie
>>> >> +DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   =
>>> >> DEF(GCC49_AARCH64_ASLDLINK_FLAGS) -Wl,-no-pie
>>> >>
>>> >>  
>>> >> 
>>> >>  #
>>> >>
>>> >
>>> > Unfortunately no change, still:
>>> > aarch64-linux-gnu-gcc: error: unrecognized command line option '-no-pie'
>>> > In order to make sure, I double checked twice cleaninig everything and
>>> > rebuilding from scratch.
>>> >
>>>
>>> Thanks, but it doesn't matter anyway: it doesn't fix the original
>>> issues on affected toolchains.
>>>
>>> It appears the only way we can deal with this is introducing GCC6 and
>>> move the workaround there.
>>>

Do you think it would be reasonable to revert:
3380a59 - BaseTools/tools_def AARCH64 ARM: disable PIE linking for .aslc sources
1894a7c - BaseTools/tools_def AARCH64 ARM: disable PIE linking
on the EDK2 master branch, so that the users are not affected before
the actual workaround for problematic builds is moved to GCC6?

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

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Udit Kumar]

 
> On 21 November 2017 at 09:59, Udit Kumar  wrote:
> > Thanks Ard.
> > Below table was for example. I am not converting whole DT to ACPI
> > tables :) My idea is to reduce Linux patches for probing as possible.
> > Also keeping firmware and OS separately then Let firmware expose both
> > way (HID and PRP1) and Linux to decide binding
> 
> No.
> 
> You are still assuming ACPI and DT device drivers bind at the same level, and
> they don't.

No, my above comments was just limited to binding. 
Right, here device driver should know that device is present in system, 
I see probe function (device-driver binding) is way to know this. 
Then driver can execute AML methods exposed by firmware.
 
> An ACPI device has AML methods to manage power state and perform other
> device related low-level tasks. The device driver has no knowledge of the
> hardware beyond what it needs to invoke those abstract methods.

You meant, If we need to update driver for AML methods then why not to update 
binding as well . ?

On side track, 
 With limited search,  I got many each driver is having (acpi_id and of_id), 
looks, acpi support is added on the top of DT flavored drivers.
and therefore acpi tables are following the same.
Sorry to say, reference I am looking at (edk2-platform) JunoPkg and 
VExpressPkg, 
Has following devices has description similar to Device tree 
Device (NET0) {
Device (SREG) {
Device (VIRT) {
   Device(KMI0) {
   Device(ETH0) {
Where no AML method is exposed. Then I expect OS driver to manage this device.
While grepping over few other edk2-platforms.  Looks some of methods 
are abstracted, not whole device. 


> A DT device describes everything in detail, and requires clock and regulator
> drivers and other bits and pieces that are tightly coupled to details of the
> hardware.
> 
> So now, you have the worst of both worlds:
> 
> - you need to implement all of this in firmware so ACPI can support it,
> - you have to expose the internals to the OS so DT can support it.

Yes, for time being or may be longer, DT support needs to be there 
along with ACPI introduction.

Are you suggesting here to abstract whole device details from 
OS and expose AML methods to be used by device driver. 
And maintain two drivers instead of fitting DT style driver into ACPI world ?
 
> The result is that you lose *all* of the benefits of ACPI, because the power 
> of
> the abstraction is that you can modify the low-level implementation on the
> firmware side without the need for modifying the OS. *That* is the value
> proposition of ACPI, the ability to run last year's OS on this year's 
> hardware.


> Implementing ACPI in the way you propose is absolutely pointless, sorry to be
> harsh about it.
> 
> --
> Ard.
> 
> 
> > Please see inline as well
> >
> > Regards
> > Udit
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: Tuesday, November 21, 2017 3:08 PM
> >> To: Udit Kumar 
> >> Cc: Leif Lindholm ; edk2-devel@lists.01.org; 
> >> Varun
> >> Sethi ; Daniel Thompson ;
> >> Graeme Gregory 
> >> Subject: Re: [RFC] ACPI table HID/CID allocation
> >>
> >> On 21 November 2017 at 09:19, Udit Kumar  wrote:
> >> > Hi,
> >> >
> >> > I am enabling ACPI on NXP platform , In order to do minimal changes in
> >> > Linux driver for device-driver binding.
> >> >
> >> > I want to use PRP0001 device as CID and HID as actual (NXP allocated 
> >> > HID).
> >> >
> >> > So that Linux can bind with PRP0001 and  compatible field, where as
> >> > other OS (Window etc) can rely on HID.
> >> >
> >> > Below is sample, ACPI table for SPI controller and its slave device.
> >> >
> >> >
> >> >
> >> > Hope this approach is ok ?
> >> >
> >>
> >> No, it is not.
> >>
> >> Architecting an ACPI platform is not a matter of taking a device tree and
> >> converting each node into an ACPI device.
> >
> > No , no, Here I am not converting everything from DT to ACPI
> >
> >> Linux/DT makes no assumptions about the presence of firmware. This means
> >> most device drivers have to manage clocks, regulators etc because they will
> not
> >> be in a known state. Also, the OS can own all devices in the system.
> >
> > Thanks to Linux documentation , I noted this, During hand off ACPI
> > firmware needs to ensure proper clocking.
> >
> >> Linux/ACPI relies on the firmware to set up clocks and regulators, and uses
> >> abstract firmware methods to manage power states etc. Also, due to the
> >> dependency on UEFI, things like the RTC and NOR flash are not exposed to
> the
> >> OS via device nodes, but via UEFI runtime services.
> >
> > Agreed, RTC and NOR (containing firmware)  should not exposed to Linux or
> OS.
> >
> >> In a nutshell, the difference between ACPI and DT is that the handoff point
> >> between the OS and the firmware is at a different 

[edk2] [PATCH] [edk2-platforms]:Enabling Secure boot feature support on hikey platfrom

[kalyan-nagabhirava]

Added required library packages related to secure boot  in hikey.dsc and 
Blockvariable driver[
from 96-board edk2 fork] to support the NV storage of the variables.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: kalyan-nagabhirava 
---
 Platform/Hisilicon/HiKey/HiKey.dec |  10 +
 Platform/Hisilicon/HiKey/HiKey.dsc |  56 ++-
 Platform/Hisilicon/HiKey/HiKey.fdf |  13 +-
 Platform/Hisilicon/HiKey/VarStore.fdf.inc  |  72 
 .../Drivers/BlockVariableDxe/BlockVariableDxe.c| 444 +
 .../Drivers/BlockVariableDxe/BlockVariableDxe.h|  51 +++
 .../Drivers/BlockVariableDxe/BlockVariableDxe.inf  |  65 +++
 7 files changed, 706 insertions(+), 5 deletions(-)
 create mode 100644 Platform/Hisilicon/HiKey/VarStore.fdf.inc
 create mode 100644 
Silicon/Hisilicon/Drivers/BlockVariableDxe/BlockVariableDxe.c
 create mode 100644 
Silicon/Hisilicon/Drivers/BlockVariableDxe/BlockVariableDxe.h
 create mode 100644 
Silicon/Hisilicon/Drivers/BlockVariableDxe/BlockVariableDxe.inf

diff --git a/Platform/Hisilicon/HiKey/HiKey.dec 
b/Platform/Hisilicon/HiKey/HiKey.dec
index 537138eb4..e27d70447 100644
--- a/Platform/Hisilicon/HiKey/HiKey.dec
+++ b/Platform/Hisilicon/HiKey/HiKey.dec
@@ -30,7 +30,17 @@
 
 [Guids.common]
   gHiKeyTokenSpaceGuid  =  { 0x91148425, 0xcdd2, 0x4830, { 0x8b, 0xd0, 
0xc6, 0x1c, 0x6d, 0xea, 0x36, 0x21 } }
+  gHwTokenSpaceGuid =  { 0x, 0x74c5, 0x4043, { 0xb4, 0x17, 
0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
 
 [PcdsFixedAtBuild.common]
   gHiKeyTokenSpaceGuid.PcdAndroidFastbootNvmDevicePath|L""|VOID*|0x0001
   gHiKeyTokenSpaceGuid.PcdArmFastbootFlashLimit|L""|VOID*|0x0002
+
+  # NV Block
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockLba|0|UINT32|0x0112
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockSize|0|UINT32|0x0100011
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockCount|0|UINT32|0x0100010
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockDevicePath|L""|VOID*|0x0113
+  
+  # UncachedAllocationLib
+  
gArmTokenSpaceGuid.PcdArmUncachedMemoryMask|0x8000|UINT64|0x0002
diff --git a/Platform/Hisilicon/HiKey/HiKey.dsc 
b/Platform/Hisilicon/HiKey/HiKey.dsc
index 2e3b1c879..a7288b125 100644
--- a/Platform/Hisilicon/HiKey/HiKey.dsc
+++ b/Platform/Hisilicon/HiKey/HiKey.dsc
@@ -26,6 +26,8 @@
   SKUID_IDENTIFIER   = DEFAULT
   FLASH_DEFINITION   = Platform/Hisilicon/HiKey/HiKey.fdf
 
+  DEFINE SECURE_BOOT_ENABLE = FALSE
+
 [LibraryClasses.common]
 !if $(TARGET) == RELEASE
   DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
@@ -125,6 +127,18 @@
   # Add support for GCC stack protector
   NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
 
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
+  
TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+  AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
+  VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
+  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
+  BdsLib|ArmPkg/Library/BdsLib/BdsLib.inf
+  DmaLib|EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
+!endif
+
 [LibraryClasses.common.SEC]
   PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
   
ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf
@@ -160,6 +174,7 @@
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
 
 [BuildOptions]
   GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include 
-I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include
@@ -337,6 +352,29 @@
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbVendorId|0x18d1
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xd00d
 
+  # 
+  # NV Storage PCDs.
+  #
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockCount|0x1000
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockSize|0x0200
+  gHwTokenSpaceGuid.PcdNvStorageVariableBlockLba|0x6000
+  
gHwTokenSpaceGuid.PcdNvStorageVariableBlockDevicePath|L"VenHw(B549F005-4BD4-4020-A0CB-06F42BDA68C3)/HD(5,GPT,00354BCD-BBCB-4CB3-B5AE-CDEFCB5DAC43)"
+
+  #
+  # ARM Pcds
+  #
+  gArmTokenSpaceGuid.PcdArmUncachedMemoryMask|0x
+
+  # Increase storage space of UEFI variable to 2KB so that it can store root 
certificate
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x800
+
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  # override the default values from 

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Ard Biesheuvel]

On 21 November 2017 at 09:59, Udit Kumar  wrote:
> Thanks Ard.
> Below table was for example. I am not converting whole DT to ACPI tables :)
> My idea is to reduce Linux patches for probing as possible.
> Also keeping firmware and OS separately then
> Let firmware expose both way (HID and PRP1) and Linux to decide binding

No.

You are still assuming ACPI and DT device drivers bind at the same
level, and they don't.

An ACPI device has AML methods to manage power state and perform other
device related low-level tasks. The device driver has no knowledge of
the hardware beyond what it needs to invoke those abstract methods.

A DT device describes everything in detail, and requires clock and
regulator drivers and other bits and pieces that are tightly coupled
to details of the hardware.

So now, you have the worst of both worlds:

- you need to implement all of this in firmware so ACPI can support it,
- you have to expose the internals to the OS so DT can support it.

The result is that you lose *all* of the benefits of ACPI, because the
power of the abstraction is that you can modify the low-level
implementation on the firmware side without the need for modifying the
OS. *That* is the value proposition of ACPI, the ability to run last
year's OS on this year's hardware.

Implementing ACPI in the way you propose is absolutely pointless,
sorry to be harsh about it.

-- 
Ard.


> Please see inline as well
>
> Regards
> Udit
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Tuesday, November 21, 2017 3:08 PM
>> To: Udit Kumar 
>> Cc: Leif Lindholm ; edk2-devel@lists.01.org; Varun
>> Sethi ; Daniel Thompson ;
>> Graeme Gregory 
>> Subject: Re: [RFC] ACPI table HID/CID allocation
>>
>> On 21 November 2017 at 09:19, Udit Kumar  wrote:
>> > Hi,
>> >
>> > I am enabling ACPI on NXP platform , In order to do minimal changes in
>> > Linux driver for device-driver binding.
>> >
>> > I want to use PRP0001 device as CID and HID as actual (NXP allocated HID).
>> >
>> > So that Linux can bind with PRP0001 and  compatible field, where as
>> > other OS (Window etc) can rely on HID.
>> >
>> > Below is sample, ACPI table for SPI controller and its slave device.
>> >
>> >
>> >
>> > Hope this approach is ok ?
>> >
>>
>> No, it is not.
>>
>> Architecting an ACPI platform is not a matter of taking a device tree and
>> converting each node into an ACPI device.
>
> No , no, Here I am not converting everything from DT to ACPI
>
>> Linux/DT makes no assumptions about the presence of firmware. This means
>> most device drivers have to manage clocks, regulators etc because they will 
>> not
>> be in a known state. Also, the OS can own all devices in the system.
>
> Thanks to Linux documentation , I noted this, During hand off ACPI
> firmware needs to ensure proper clocking.
>
>> Linux/ACPI relies on the firmware to set up clocks and regulators, and uses
>> abstract firmware methods to manage power states etc. Also, due to the
>> dependency on UEFI, things like the RTC and NOR flash are not exposed to the
>> OS via device nodes, but via UEFI runtime services.
>
> Agreed, RTC and NOR (containing firmware)  should not exposed to Linux or OS.
>
>> In a nutshell, the difference between ACPI and DT is that the handoff point
>> between the OS and the firmware is at a different abstraction level.
>>
>> So no, it is not ok to use PRP0001 + compatible for everything. It may be
>> acceptable in some exceptional cases, but you will have to explain why.
>> Everything else should use proper ACPI bindings.
>
> HID is not going away , I am keeping with PRP0001 + compatible
> and let linux driver to decide what to use.
>
> If PRP0001 + compatible is restricted or meant for limited use
> then I can assign HID for NXP devices or say driver managed by NXP.
> For other vendors, will this be accepted to have HID something like NXP00xx  
> ??
>
>> --
>> Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Udit Kumar]

Thanks Ard. 
Below table was for example. I am not converting whole DT to ACPI tables :) 
My idea is to reduce Linux patches for probing as possible.  
Also keeping firmware and OS separately then 
Let firmware expose both way (HID and PRP1) and Linux to decide binding 
Please see inline as well 

Regards
Udit

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, November 21, 2017 3:08 PM
> To: Udit Kumar 
> Cc: Leif Lindholm ; edk2-devel@lists.01.org; Varun
> Sethi ; Daniel Thompson ;
> Graeme Gregory 
> Subject: Re: [RFC] ACPI table HID/CID allocation
> 
> On 21 November 2017 at 09:19, Udit Kumar  wrote:
> > Hi,
> >
> > I am enabling ACPI on NXP platform , In order to do minimal changes in
> > Linux driver for device-driver binding.
> >
> > I want to use PRP0001 device as CID and HID as actual (NXP allocated HID).
> >
> > So that Linux can bind with PRP0001 and  compatible field, where as
> > other OS (Window etc) can rely on HID.
> >
> > Below is sample, ACPI table for SPI controller and its slave device.
> >
> >
> >
> > Hope this approach is ok ?
> >
> 
> No, it is not.
> 
> Architecting an ACPI platform is not a matter of taking a device tree and
> converting each node into an ACPI device.

No , no, Here I am not converting everything from DT to ACPI
 
> Linux/DT makes no assumptions about the presence of firmware. This means
> most device drivers have to manage clocks, regulators etc because they will 
> not
> be in a known state. Also, the OS can own all devices in the system.

Thanks to Linux documentation , I noted this, During hand off ACPI
firmware needs to ensure proper clocking. 
 
> Linux/ACPI relies on the firmware to set up clocks and regulators, and uses
> abstract firmware methods to manage power states etc. Also, due to the
> dependency on UEFI, things like the RTC and NOR flash are not exposed to the
> OS via device nodes, but via UEFI runtime services.

Agreed, RTC and NOR (containing firmware)  should not exposed to Linux or OS. 
 
> In a nutshell, the difference between ACPI and DT is that the handoff point
> between the OS and the firmware is at a different abstraction level.
> 
> So no, it is not ok to use PRP0001 + compatible for everything. It may be
> acceptable in some exceptional cases, but you will have to explain why.
> Everything else should use proper ACPI bindings.

HID is not going away , I am keeping with PRP0001 + compatible
and let linux driver to decide what to use. 

If PRP0001 + compatible is restricted or meant for limited use
then I can assign HID for NXP devices or say driver managed by NXP.
For other vendors, will this be accepted to have HID something like NXP00xx  ??
 
> --
> Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC] ACPI table HID/CID allocation

[Ard Biesheuvel]

On 21 November 2017 at 09:19, Udit Kumar  wrote:
> Hi,
>
> I am enabling ACPI on NXP platform , In order to do minimal changes in Linux
> driver for device-driver binding.
>
> I want to use PRP0001 device as CID and HID as actual (NXP allocated HID).
>
> So that Linux can bind with PRP0001 and  compatible field, where as other OS
> (Window etc) can rely on HID.
>
> Below is sample, ACPI table for SPI controller and its slave device.
>
>
>
> Hope this approach is ok ?
>

No, it is not.

Architecting an ACPI platform is not a matter of taking a device tree
and converting each node into an ACPI device.

Linux/DT makes no assumptions about the presence of firmware. This
means most device drivers have to manage clocks, regulators etc
because they will not be in a known state. Also, the OS can own all
devices in the system.

Linux/ACPI relies on the firmware to set up clocks and regulators, and
uses abstract firmware methods to manage power states etc. Also, due
to the dependency on UEFI, things like the RTC and NOR flash are not
exposed to the OS via device nodes, but via UEFI runtime services.

In a nutshell, the difference between ACPI and DT is that the handoff
point between the OS and the firmware is at a different abstraction
level.

So no, it is not ok to use PRP0001 + compatible for everything. It may
be acceptable in some exceptional cases, but you will have to explain
why. Everything else should use proper ACPI bindings.

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

[edk2] [RFC] ACPI table HID/CID allocation

[Udit Kumar]

Hi,

I am enabling ACPI on NXP platform , In order to do minimal changes in Linux 
driver for device-driver binding.

I want to use PRP0001 device as CID and HID as actual (NXP allocated HID).

So that Linux can bind with PRP0001 and  compatible field, where as other OS 
(Window etc) can rely on HID.

Below is sample, ACPI table for SPI controller and its slave device.



Hope this approach is ok ?



Thanks

Udit





Scope(_SB)

{

Device(SPI0) {

Name(_HID, "NXP0001")

Name(_CID, "PRP0001")

Name(_CRS, ResourceTemplate() {

}) // end of _CRS for i2c0 device

Name (_DSD, Package () {

ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

Package () {

Package() {"compatible", " fsl,ls2085a-dspi"},

}

}) // end of DSD SPI device

Device (EEP0) {

Name(_HID, " NXP0002")  //HID can be discussed with 
flash vendor

Name(_CID, "PRP0001") // m25p80 flash

Name(_CRS, ResourceTemplate()

{

   SPISerialBus()

}) // ResourceTemplate()

Name (_DSD, Package () {

ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

Package () {

Package() {"compatible", "st,m25p80"},

}

} // End of e2prom device

} // end of SPI device

}



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

[edk2] [PATCH] MdeModulePkg EhciPei: Minor refinement about IOMMU

[Star Zeng]

This patch is following 2c656af04d7f.
1. Fix typo "XHC" to "EHC".
2. Reinitialize Request(Phy/Map) and Data(Phy/Map)
in Urb, otherwise the last time value of them may
be used in error handling when error happens.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h |  2 +-
 MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c | 13 ++---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h 
b/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
index 279407475b66..715a5ab1c142 100644
--- a/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
+++ b/MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.h
@@ -99,7 +99,7 @@ struct _PEI_USB2_HC_DEV {
   EDKII_IOMMU_PPI *IoMmu;
   EFI_PEI_PPI_DESCRIPTOR  PpiDescriptor;
   //
-  // EndOfPei callback is used to stop the XHC DMA operation
+  // EndOfPei callback is used to stop the EHC DMA operation
   // after exit PEI phase.
   //
   EFI_PEI_NOTIFY_DESCRIPTOR   EndOfPeiNotifyList;
diff --git a/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c 
b/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
index 3dadcd60b6fe..baacf5d56080 100644
--- a/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
+++ b/MdeModulePkg/Bus/Pci/EhciPei/EhciUrb.c
@@ -576,7 +576,12 @@ EhcCreateUrb (
   if (Urb->Qh == NULL) {
 goto ON_ERROR;
   }
-  
+
+  Urb->RequestPhy = NULL;
+  Urb->RequestMap = NULL;
+  Urb->DataPhy  = NULL;
+  Urb->DataMap  = NULL;
+
   //
   // Map the request and user data
   //
@@ -591,9 +596,6 @@ EhcCreateUrb (
 
 Urb->RequestPhy = (VOID *) ((UINTN) PhyAddr);
 Urb->RequestMap = Map;
-  } else {
-Urb->RequestPhy = NULL;
-Urb->RequestMap = NULL;
   }
 
   if (Data != NULL) {
@@ -613,9 +615,6 @@ EhcCreateUrb (
 
 Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
 Urb->DataMap  = Map;
-  } else {
-Urb->DataPhy  = NULL;
-Urb->DataMap  = NULL;
   }
 
   Status = EhcCreateQtds (Ehc, Urb);
-- 
2.7.0.windows.1

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

[edk2] [PATCH] MdeModulePkg UhciPei: Support IoMmu

[Star Zeng]

Update the UhciPei driver to consume IOMMU_PPI to allocate DMA buffer.

If no IOMMU_PPI exists, this driver still calls PEI service to allocate
DMA buffer, with assumption that DRAM==DMA.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c| 251 +++
 MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.c   | 398 +++
 MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.h   | 164 -
 MdeModulePkg/Bus/Pci/UhciPei/UhciPei.inf |   5 +-
 4 files changed, 719 insertions(+), 99 deletions(-)
 create mode 100644 MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c

diff --git a/MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c 
b/MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c
new file mode 100644
index ..c92bee429835
--- /dev/null
+++ b/MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c
@@ -0,0 +1,251 @@
+/** @file
+The DMA memory help functions.
+
+Copyright (c) 2017, Intel Corporation. All rights reserved.
+
+This program and the accompanying materials
+are licensed and made available under the terms and conditions
+of the BSD License which accompanies this distribution.  The
+full text of the license may be found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "UhcPeim.h"
+
+/**
+  Provides the controller-specific addresses required to access system memory 
from a
+  DMA bus master.
+
+  @param IoMmu  Pointer to IOMMU PPI.
+  @param Operation  Indicates if the bus master is going to read 
or write to system memory.
+  @param HostAddressThe system memory address to map to the PCI 
controller.
+  @param NumberOfBytes  On input the number of bytes to map. On output 
the number of bytes
+that were mapped.
+  @param DeviceAddress  The resulting map address for the bus master 
PCI controller to use to
+access the hosts HostAddress.
+  @param MappingA resulting value to pass to Unmap().
+
+  @retval EFI_SUCCESS   The range was mapped for the returned 
NumberOfBytes.
+  @retval EFI_UNSUPPORTED   The HostAddress cannot be mapped as a common 
buffer.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The request could not be completed due to a 
lack of resources.
+  @retval EFI_DEVICE_ERROR  The system hardware could not map the 
requested address.
+
+**/
+EFI_STATUS
+IoMmuMap (
+  IN EDKII_IOMMU_PPI*IoMmu,
+  IN EDKII_IOMMU_OPERATION  Operation,
+  IN VOID   *HostAddress,
+  IN OUT UINTN  *NumberOfBytes,
+  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
+  OUT VOID  **Mapping
+  )
+{
+  EFI_STATUSStatus;
+  UINT64Attribute;
+
+  if (IoMmu != NULL) {
+Status = IoMmu->Map (
+  IoMmu,
+  Operation,
+  HostAddress,
+  NumberOfBytes,
+  DeviceAddress,
+  Mapping
+  );
+if (EFI_ERROR (Status)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+switch (Operation) {
+case EdkiiIoMmuOperationBusMasterRead:
+case EdkiiIoMmuOperationBusMasterRead64:
+  Attribute = EDKII_IOMMU_ACCESS_READ;
+  break;
+case EdkiiIoMmuOperationBusMasterWrite:
+case EdkiiIoMmuOperationBusMasterWrite64:
+  Attribute = EDKII_IOMMU_ACCESS_WRITE;
+  break;
+case EdkiiIoMmuOperationBusMasterCommonBuffer:
+case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+  Attribute = EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE;
+  break;
+default:
+  ASSERT(FALSE);
+  return EFI_INVALID_PARAMETER;
+}
+Status = IoMmu->SetAttribute (
+  IoMmu,
+  *Mapping,
+  Attribute
+  );
+if (EFI_ERROR (Status)) {
+  IoMmu->Unmap (IoMmu, Mapping);
+  *Mapping = NULL;
+  return Status;
+}
+  } else {
+*DeviceAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
+*Mapping = NULL;
+Status = EFI_SUCCESS;
+  }
+  return Status;
+}
+
+/**
+  Completes the Map() operation and releases any corresponding resources.
+
+  @param IoMmu  Pointer to IOMMU PPI.
+  @param MappingThe mapping value returned from Map().
+
+**/
+VOID
+IoMmuUnmap (
+  IN EDKII_IOMMU_PPI*IoMmu,
+  IN VOID  *Mapping
+  )
+{
+  if (IoMmu != NULL) {
+IoMmu->SetAttribute (IoMmu, Mapping, 0);
+IoMmu->Unmap (IoMmu, Mapping);
+  }
+}
+
+/**
+  Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
+  OperationBusMasterCommonBuffer64 mapping.
+
+  @param IoMmu  Pointer