[edk2] Concerns with IP4Config2 - Part 3 - No RegisterDataNotify() on DHCP config

2015-07-31 Thread El-Haj-Mahmoud, Samer
This is Part 3 of the issues we observed in IP4Config2 in EDK2

The UEFI 2.5 spec provides a method RegisterDataNotify() as part of the 
Ip4Config2 protocol that is used by callers to get notified of asynchronous 
configuration completion. The current EDK2 implementation is covering setting 
the interface to the static policy, but not for the dhcp policy. The EDK2 
implementation of the Ip4Config2 protocol does not trigger any callbacks on the 
DHCP status. In fact, the protocol returns a EFI_SUCCESS as soon as the policy 
is set to DHCP, while the actual DHCP process is initiated in the background. 

Even the ifconfig Shell command just sets the policy to DHCP and does nothing 
else. On UEFI 2.4 / Ip4Config protocol, one could start the IP4 driver after 
applying the configuration and poll the child IP4 instance to know the 
configuration status. This method doesn't work with the Ip4Config2.

The UEFI spec does not specify what needs to be done after setting the policy 
to DHCP, because this is also an asynchronous process that runs in the 
background until the DHCP process completes. Even the example given in the 
SetData() takes a static (manual) case:

This function is always non-blocking. When setting some type of configuration 
data, an asynchronous process is invoked to check the correctness of the data, 
such as doing address conflict detection on the manually set local IPv4 
address. EFI_NOT_READY is returned immediately to indicate that such an 
asynchronous process is invoked and the process is not finished yet. The caller 
willing to get the result of the asynchronous process is required to call 
RegisterDataNotify() to register an event on the specified configuration data. 
Once the event is signaled, the caller can call GetData() to get back the 
configuration data in order to know the result. For other types of 
configuration data that do not require an asynchronous configuration process, 
the result of the operation is immediately returned.


Is this an implementation issue that needs to be addressed in EDK2 (to treat 
DHCP policy event notifications like static policy), or do we think a 
clarification is needed in the UEFI spec ?


Samer El-Haj-Mahmoud
System Firmware Architect
HP Servers 

el...@hp.com 
T +1.281.514.5973
C +1.512.659.1523
Hewlett-Packard Company
http://hp.com/go/proliant/uefi



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


[edk2] [PATCH v2 2/2] ArmPkg/ArmGicArchLib: use common source file

2015-07-31 Thread Ryan Harkin
Now that the Arm and Aarch64 source files are identical and rely on
conditional compilation to provide arch specific code, remove the
duplicated files and use one common file.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ryan Harkin ryan.har...@linaro.org
Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
---
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 77 --
 .../ArmGicArchLib/{AArch64 = }/ArmGicArchLib.c|  0
 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf |  7 +-
 3 files changed, 2 insertions(+), 82 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c 
b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
deleted file mode 100644
index 75ba156..000
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ /dev/null
@@ -1,77 +0,0 @@
-/** @file
-*
-*  Copyright (c) 2014, ARM 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 Library/ArmLib.h
-#include Library/ArmGicLib.h
-
-STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision = ARM_GIC_ARCH_REVISION_2;
-
-STATIC
-RETURN_STATUS
-EFIAPI
-GicSystemRegistersSupported (
-  VOID
-  )
-{
-#if defined (MDE_CPU_ARM)
-  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
-#elif defined(MDE_CPU_AARCH64)
-  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
-#else
- #error Unknown chipset.
-#endif
-}
-
-RETURN_STATUS
-EFIAPI
-ArmGicArchLibInitialize (
-  VOID
-  )
-{
-  UINT32IccSre;
-
-  // Ideally we would like to use the GICC IIDR Architecture version here, but
-  // this does not seem to be very reliable as the implementation could easily
-  // get it wrong. It is more reliable to check if the GICv3 System Register
-  // feature is implemented on the CPU. This is also convenient as our GICv3
-  // driver requires SRE. If only Memory mapped access is available we try to
-  // drive the GIC as a v2.
-  if (GicSystemRegistersSupported()) {
-// Make sure System Register access is enabled (SRE). This depends on the
-// higher privilege level giving us permission, otherwise we will either
-// cause an exception here, or the write doesn't stick in which case we 
need
-// to fall back to the GICv2 MMIO interface.
-// Note: We do not need to set ICC_SRE_EL2.Enable because the OS is started
-// at the same exception level.
-// It is the OS responsibility to set this bit.
-IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-if (!(IccSre  ICC_SRE_EL2_SRE)) {
-  ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);
-  IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-}
-if (IccSre  ICC_SRE_EL2_SRE) {
-  mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
-}
-  }
-  return RETURN_SUCCESS;
-}
-
-ARM_GIC_ARCH_REVISION
-EFIAPI
-ArmGicGetSupportedArchRevision (
-  VOID
-  )
-{
-  return mGicArchRevision;
-}
diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c 
b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
similarity index 100%
rename from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf 
b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
index 7dbcb08..5c968e6 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -20,11 +20,8 @@
   LIBRARY_CLASS  = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER 
UEFI_APPLICATION
   CONSTRUCTOR= ArmGicArchLibInitialize
 
-[Sources.ARM]
-  Arm/ArmGicArchLib.c
-
-[Sources.AARCH64]
-  AArch64/ArmGicArchLib.c
+[Sources.common]
+  ArmGicArchLib.c
 
 [Packages]
   MdePkg/MdePkg.dec
-- 
2.1.0

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


[edk2] Concerns with IP4Config2 - Part 1 - DHCP default policy

2015-07-31 Thread El-Haj-Mahmoud, Samer
We have observed an interesting behavior in the IP4Config2 implementation in 
EDK2 and have a concern with it.



This is Part 1 of multiple parts, sent in separate email threads to make it 
easier to track the discussion on each issue



The IPv4 config policy for all NICs defaults to DHCP.



  - In UEFI 2.4, using Ip4Config protocol, the state was unconfigured or 
rather the policy not applied, until a manual configuration attempt was 
initiated (via ifconfig or programmatically) with the appropriate policy (DHCP 
or static).



- With the latest UEFI 2.5 EDK2 implementation for Ip4Config2, the DHCP policy 
(default) is applied at boot time on all NICs in the system. Which results in 
all NIC ports attempting DHCP and trying to acquire IP addresses during boot 
(aft eth Connect sequence is initiated in BDS). Even NICs that do not have 
cable connected, or those that cannot reach a DHCP server, will try to get a 
DHCP, and will get a 0.0.0.0, while the others show IP addresses acquired 
from a DHCP server.



The concerns with this behavior are:





1.   Possible impact to boot time. On servers with large number of NICs 
(could be in the orders of 100s of NIC ports), this can be pretty huge. IT may 
not be obvious on some implementations because this is happening the in the 
background while other UEFI drivers are being connected. But that does not 
change the fact that it is still an issue.



2.   There is also a concern about overhead. Why start DHCP on every NIC 
even if that NIC is not needed for boot or pre-boot network operations. This is 
similar to MnpSystemPoll on all NICs with background polling (and media status 
detection). Why is this needed for all NICs?



3.   Not to mention other issues. DHCP IPv4 addresses are in shortage on 
may subnets, and having every NIC port trying to get a DHCP address on every 
boot (when the OS may have the ports configured for instance static addresses) 
is a problem.



The UEFI spec does not state that all NICs should attempt to acquire IP 
addresses during boot. The issue seems to be an implementation choice in Ip4Dxe:



Ip4DriverBindingStart - Ip4CreateService - Ip4Config2InitInstance (new code):

  //

  // Try to set the configured parameter.

  //

  for (Index = Ip4Config2DataTypePolicy; Index  Ip4Config2DataTypeMaximum; 
Index++) {

DataItem = IpSb-Ip4Config2Instance.DataItem[Index];

if (DataItem-Data.Ptr != NULL) {

  DataItem-SetData (

  IpSb-Ip4Config2Instance,

  DataItem-DataSize,

  DataItem-Data.Ptr

  );

}

  }





This code is setting the default policy on every NIC handle that the 
IP4DriverBinding instance comes across. And this is going to trigger DHCP in 
the background on that port since that is the default policy.



Samer El-Haj-Mahmoud
System Firmware Architect
HP Servers

el...@hp.commailto:el...@hp.com
T +1.281.514.5973
C +1.512.659.1523
Hewlett-Packard Company
hp.com/go/proliant/uefihttp://hp.com/go/proliant/uefi

[Description: Description: C:\Users\elhajmah\HpLogo.png]

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


Re: [edk2] [PATCH v2 2/2] ArmPkg/ArmGicArchLib: use common source file

2015-07-31 Thread Ryan Harkin
On 31 July 2015 at 15:49, Ryan Harkin ryan.har...@linaro.org wrote:

 Now that the Arm and Aarch64 source files are identical and rely on
 conditional compilation to provide arch specific code, remove the
 duplicated files and use one common file.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ryan Harkin ryan.har...@linaro.org
 Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 77
 --
  .../ArmGicArchLib/{AArch64 = }/ArmGicArchLib.c|  0
  ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf |  7 +-
  3 files changed, 2 insertions(+), 82 deletions(-)

 diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 deleted file mode 100644
 index 75ba156..000
 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 +++ /dev/null
 @@ -1,77 +0,0 @@
 -/** @file
 -*
 -*  Copyright (c) 2014, ARM 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 Library/ArmLib.h
 -#include Library/ArmGicLib.h
 -
 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
 ARM_GIC_ARCH_REVISION_2;
 -
 -STATIC
 -RETURN_STATUS
 -EFIAPI
 -GicSystemRegistersSupported (
 -  VOID
 -  )
 -{
 -#if defined (MDE_CPU_ARM)
 -  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 -#elif defined(MDE_CPU_AARCH64)
 -  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 -#else
 - #error Unknown chipset.
 -#endif
 -}
 -
 -RETURN_STATUS
 -EFIAPI
 -ArmGicArchLibInitialize (
 -  VOID
 -  )
 -{
 -  UINT32IccSre;
 -
 -  // Ideally we would like to use the GICC IIDR Architecture version
 here, but
 -  // this does not seem to be very reliable as the implementation could
 easily
 -  // get it wrong. It is more reliable to check if the GICv3 System
 Register
 -  // feature is implemented on the CPU. This is also convenient as our
 GICv3
 -  // driver requires SRE. If only Memory mapped access is available we
 try to
 -  // drive the GIC as a v2.
 -  if (GicSystemRegistersSupported()) {
 -// Make sure System Register access is enabled (SRE). This depends on
 the
 -// higher privilege level giving us permission, otherwise we will
 either
 -// cause an exception here, or the write doesn't stick in which case
 we need
 -// to fall back to the GICv2 MMIO interface.
 -// Note: We do not need to set ICC_SRE_EL2.Enable because the OS is
 started
 -// at the same exception level.
 -// It is the OS responsibility to set this bit.
 -IccSre = ArmGicV3GetControlSystemRegisterEnable ();
 -if (!(IccSre  ICC_SRE_EL2_SRE)) {
 -  ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);


Ach!  No, now I need a v3.

Sorry for the noise



 -  IccSre = ArmGicV3GetControlSystemRegisterEnable ();
 -}
 -if (IccSre  ICC_SRE_EL2_SRE) {
 -  mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
 -}
 -  }
 -  return RETURN_SUCCESS;
 -}
 -
 -ARM_GIC_ARCH_REVISION
 -EFIAPI
 -ArmGicGetSupportedArchRevision (
 -  VOID
 -  )
 -{
 -  return mGicArchRevision;
 -}
 diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
 similarity index 100%
 rename from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
 diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
 b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
 index 7dbcb08..5c968e6 100644
 --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
 +++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
 @@ -20,11 +20,8 @@
LIBRARY_CLASS  = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER
 UEFI_APPLICATION
CONSTRUCTOR= ArmGicArchLibInitialize

 -[Sources.ARM]
 -  Arm/ArmGicArchLib.c
 -
 -[Sources.AARCH64]
 -  AArch64/ArmGicArchLib.c
 +[Sources.common]
 +  ArmGicArchLib.c

  [Packages]
MdePkg/MdePkg.dec
 --
 2.1.0


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


Re: [edk2] [PATCH 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

2015-07-31 Thread Ard Biesheuvel
On 31 July 2015 at 14:21, Ryan Harkin ryan.har...@linaro.org wrote:


 On 31 July 2015 at 13:13, Ard Biesheuvel ard.biesheu...@linaro.org wrote:

 On 31 July 2015 at 14:06, Ryan Harkin ryan.har...@linaro.org wrote:
  Make Arm and Aarch64 both use the same code, conditionally compiled, to
  check if the platform has GICv3.
 
  Both source files for Arm and Aarch64 are now identical and the next
  step is to move to a common source file.
 
  Contributed-under: TianoCore Contribution Agreement 1.0
  Signed-off-by: Ryan Harkin ryan.har...@linaro.org
  ---
   .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 26
  +++---
   ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 24
  ++--
   2 files changed, 35 insertions(+), 15 deletions(-)
 
  diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  index 9853c7b..b092e3a 100644
  --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  @@ -15,7 +15,22 @@
   #include Library/ArmLib.h
   #include Library/ArmGicLib.h
 
  -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
  +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
  ARM_GIC_ARCH_REVISION_2;
  +
  +RETURN_STATUS
  +EFIAPI
  +ArmGicArchLibHasGicv3 (
  +  VOID
  +  )
  +{
  +#if defined (MDE_CPU_ARM)
  +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
  +#elif defined(MDE_CPU_AARCH64)
  +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
  +#else
  + #error Unknown chipset.
  +#endif
  +}
 

 Officially, the bit indicates whether the GIC system register
 interface is supported at the current exception level, and from that
 we infer that the system most likely has a GICv3.
 So if we don't have the SRE enabled, we try to drive it as a GICv2
 which only happens to work if this particular GICv3 has the GICv2
 compatibility mode implemented. (This is also mentioned in the
 comments)

 So the function name is a bit misleading here, although I won't insist
 that you change it.


 I'm happy to change it if you want to recommend a better name, this seems a
 but unwieldy:

 HasGicv3 - GicSystemRegistersSupported

 ... but is perhaps more correct.


Well, the function and the caller are so close together that you
shouldn't be able to miss the comment.
I do recommend STATIC for the function though.



 Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org


 Thanks for the quick review turnaround :-)

Too quick, as it seems. Spotted an inadvertent white space change below :-)



   RETURN_STATUS
   EFIAPI
  @@ -31,7 +46,7 @@ ArmGicArchLibInitialize (
 // feature is implemented on the CPU. This is also convenient as our
  GICv3
 // driver requires SRE. If only Memory mapped access is available we
  try to
 // drive the GIC as a v2.
  -  if (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC) {
  +  if (ArmGicArchLibHasGicv3()) {
   // Make sure System Register access is enabled (SRE). This depends
  on the
   // higher privilege level giving us permission, otherwise we will
  either
   // cause an exception here, or the write doesn't stick in which
  case we need
  @@ -41,18 +56,13 @@ ArmGicArchLibInitialize (
   // It is the OS responsibility to set this bit.
   IccSre = ArmGicV3GetControlSystemRegisterEnable ();
   if (!(IccSre  ICC_SRE_EL2_SRE)) {
  -  ArmGicV3SetControlSystemRegisterEnable (IccSre |
  ICC_SRE_EL2_SRE);
  +  ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);

here ^^^

 IccSre = ArmGicV3GetControlSystemRegisterEnable ();
   }
   if (IccSre  ICC_SRE_EL2_SRE) {
 mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
  -  goto Done;
   }
 }
  -
  -  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
  -
  -Done:
 return RETURN_SUCCESS;
   }
 
  diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
  b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
  index f8822a2..b092e3a 100644
  --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
  +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
  @@ -15,7 +15,22 @@
   #include Library/ArmLib.h
   #include Library/ArmGicLib.h
 
  -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
  +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
  ARM_GIC_ARCH_REVISION_2;
  +
  +RETURN_STATUS
  +EFIAPI
  +ArmGicArchLibHasGicv3 (
  +  VOID
  +  )
  +{
  +#if defined (MDE_CPU_ARM)
  +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
  +#elif defined(MDE_CPU_AARCH64)
  +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
  +#else
  + #error Unknown chipset.
  +#endif
  +}
 
   RETURN_STATUS
   EFIAPI
  @@ -31,7 +46,7 @@ ArmGicArchLibInitialize (
 // feature is implemented on the CPU. This is also convenient as our
  GICv3
 // driver requires SRE. If only Memory mapped access is available we
  try to
 // drive the GIC as a v2.
  -  if (ArmReadIdPfr1 ()  ARM_PFR1_GIC) {
  +  if (ArmGicArchLibHasGicv3()) {
   // Make sure System 

Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

2015-07-31 Thread Ryan Harkin
On 31 July 2015 at 17:16, Andrew Fish af...@apple.com wrote:


 On Jul 31, 2015, at 7:49 AM, Ryan Harkin ryan.har...@linaro.org wrote:

 Make Arm and Aarch64 both use the same code, conditionally compiled, to
 check if the platform has GICv3.

 Both source files for Arm and Aarch64 are now identical and the next
 step is to move to a common source file.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ryan Harkin ryan.har...@linaro.org
 Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
 .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 25
 --
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 25
 --
 2 files changed, 36 insertions(+), 14 deletions(-)

 diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 index 9853c7b..36dbe41 100644
 --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h

 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined (MDE_CPU_ARM)
 +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 +#elif defined(MDE_CPU_AARCH64)
 +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 +#else
 + #error Unknown chipset.
 +#endif
 +}


 We usually don’t use processor specific #ifdef’s in code like this in the
 edk2.


That's probably why the code is filled with vast swathes of
copy/paste/hack-one-line happening all over Arm*Pkg.

Although, I could split the only GicSystemRegistersSupported function into
it's own arch specific files to fit within that regime.

Seems over the top to me.  So unless it's a hard requirement, I'd resist it.




 We usually isolate the code to a file that is processor specific, and use
 the INF file to point at the correct one.

 Here is an example:
 https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf

 [Sources]
   BasePeCoffLibInternals.h
   BasePeCoff.c

 [Sources.IA32, Sources.X64, Sources.EBC]
   PeCoffLoaderEx.c

 [Sources.IPF]
   Ipf/PeCoffLoaderEx.c

 [Sources.ARM]
   Arm/PeCoffLoaderEx.c

 [Sources.AARCH64]
   AArch64/PeCoffLoaderEx.c


This is a good example of where the arch specific sources are not
copy/paste/hack, but split out for good reason.


Thanks,

 Andrew Fish

 RETURN_STATUS
 EFIAPI
 @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
   // feature is implemented on the CPU. This is also convenient as our
 GICv3
   // driver requires SRE. If only Memory mapped access is available we try
 to
   // drive the GIC as a v2.
 -  if (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC) {
 +  if (GicSystemRegistersSupported()) {
 // Make sure System Register access is enabled (SRE). This depends on
 the
 // higher privilege level giving us permission, otherwise we will
 either
 // cause an exception here, or the write doesn't stick in which case
 we need
 @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
 }
 if (IccSre  ICC_SRE_EL2_SRE) {
   mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
 -  goto Done;
 }
   }
 -
 -  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
 -
 -Done:
   return RETURN_SUCCESS;
 }

 diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 index f8822a2..75ba156 100644
 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h

 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined (MDE_CPU_ARM)
 +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 +#elif defined(MDE_CPU_AARCH64)
 +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 +#else
 + #error Unknown chipset.
 +#endif
 +}

 RETURN_STATUS
 EFIAPI
 @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
   // feature is implemented on the CPU. This is also convenient as our
 GICv3
   // driver requires SRE. If only Memory mapped access is available we try
 to
   // drive the GIC as a v2.
 -  if (ArmReadIdPfr1 ()  ARM_PFR1_GIC) {
 +  if (GicSystemRegistersSupported()) {
 // Make sure System Register access is enabled (SRE). This depends on
 the
 // higher privilege level giving us permission, otherwise we will
 either
 // cause an exception here, or the write doesn't stick in which case
 we need
 @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
 }
 if (IccSre  ICC_SRE_EL2_SRE) {
   mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
 -  goto Done;
 }
   }
 -
 -  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
 -
 -Done:
   return 

Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

2015-07-31 Thread Andrew Fish

 On Jul 31, 2015, at 9:37 AM, Ryan Harkin ryan.har...@linaro.org wrote:
 
 On 31 July 2015 at 17:16, Andrew Fish af...@apple.com wrote:
 
 
 On Jul 31, 2015, at 7:49 AM, Ryan Harkin ryan.har...@linaro.org wrote:
 
 Make Arm and Aarch64 both use the same code, conditionally compiled, to
 check if the platform has GICv3.
 
 Both source files for Arm and Aarch64 are now identical and the next
 step is to move to a common source file.
 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ryan Harkin ryan.har...@linaro.org
 Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
 .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 25
 --
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 25
 --
 2 files changed, 36 insertions(+), 14 deletions(-)
 
 diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 index 9853c7b..36dbe41 100644
 --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h
 
 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined (MDE_CPU_ARM)
 +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 +#elif defined(MDE_CPU_AARCH64)
 +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 +#else
 + #error Unknown chipset.
 +#endif
 +}
 
 
 We usually don’t use processor specific #ifdef’s in code like this in the
 edk2.
 
 
 That's probably why the code is filled with vast swathes of
 copy/paste/hack-one-line happening all over Arm*Pkg.
 

The previous code was a bad porting job. it should have been:

[Sources]
  CommonCode.c

[Sources.ARM]
  Arm/ArmSpecific.c

[Sources.AARCH64]
  AArch64/AArch64Specific.c


 Although, I could split the only GicSystemRegistersSupported function into
 it's own arch specific files to fit within that regime.
 

This is a good form as porting to a new processor architecture is quite easy. 
The process specific code is in a single file. 

The #ifdefs, and possible including processor specific includes can get really 
unwieldy in the general edk2 case that supports 6 CPU architectures. This form 
also makes porting to new processor architectures a lot easier. 

 Seems over the top to me.  So unless it's a hard requirement, I'd resist it.
 
 

Well the original issue was assumptions about what would happen in the code 
moving forward. What happens if some SOC comes along and uses a GIC for another 
CPU arch, or UEFI adds another CPU architecture?

I’m kind of 50/50 on this specific case. I would definitely reject if it was 
generic code. But for me if its 50/50 I vote against the code with #ifdefs. 

Thanks,

Andrew Fish

 
 
 We usually isolate the code to a file that is processor specific, and use
 the INF file to point at the correct one.
 
 Here is an example:
 https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
 
 [Sources]
  BasePeCoffLibInternals.h
  BasePeCoff.c
 
 [Sources.IA32, Sources.X64, Sources.EBC]
  PeCoffLoaderEx.c
 
 [Sources.IPF]
  Ipf/PeCoffLoaderEx.c
 
 [Sources.ARM]
  Arm/PeCoffLoaderEx.c
 
 [Sources.AARCH64]
  AArch64/PeCoffLoaderEx.c
 
 
 This is a good example of where the arch specific sources are not
 copy/paste/hack, but split out for good reason.
 
 
 Thanks,
 
 Andrew Fish
 
 RETURN_STATUS
 EFIAPI
 @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
  // feature is implemented on the CPU. This is also convenient as our
 GICv3
  // driver requires SRE. If only Memory mapped access is available we try
 to
  // drive the GIC as a v2.
 -  if (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC) {
 +  if (GicSystemRegistersSupported()) {
// Make sure System Register access is enabled (SRE). This depends on
 the
// higher privilege level giving us permission, otherwise we will
 either
// cause an exception here, or the write doesn't stick in which case
 we need
 @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
}
if (IccSre  ICC_SRE_EL2_SRE) {
  mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
 -  goto Done;
}
  }
 -
 -  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
 -
 -Done:
  return RETURN_SUCCESS;
 }
 
 diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 index f8822a2..75ba156 100644
 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h
 
 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined 

Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

2015-07-31 Thread Andrew Fish

 On Jul 31, 2015, at 7:49 AM, Ryan Harkin ryan.har...@linaro.org wrote:
 
 Make Arm and Aarch64 both use the same code, conditionally compiled, to
 check if the platform has GICv3.
 
 Both source files for Arm and Aarch64 are now identical and the next
 step is to move to a common source file.
 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ryan Harkin ryan.har...@linaro.org
 Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
 .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 25 --
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 25 --
 2 files changed, 36 insertions(+), 14 deletions(-)
 
 diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c 
 b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 index 9853c7b..36dbe41 100644
 --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h
 
 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision = 
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined (MDE_CPU_ARM)
 +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 +#elif defined(MDE_CPU_AARCH64)
 +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 +#else
 + #error Unknown chipset.
 +#endif
 +}
 

We usually don’t use processor specific #ifdef’s in code like this in the edk2.

We usually isolate the code to a file that is processor specific, and use the 
INF file to point at the correct one. 

Here is an example: 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf

[Sources]
  BasePeCoffLibInternals.h
  BasePeCoff.c

[Sources.IA32, Sources.X64, Sources.EBC]
  PeCoffLoaderEx.c

[Sources.IPF]
  Ipf/PeCoffLoaderEx.c

[Sources.ARM]
  Arm/PeCoffLoaderEx.c

[Sources.AARCH64]
  AArch64/PeCoffLoaderEx.c

Thanks,

Andrew Fish

 RETURN_STATUS
 EFIAPI
 @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
   // feature is implemented on the CPU. This is also convenient as our GICv3
   // driver requires SRE. If only Memory mapped access is available we try to
   // drive the GIC as a v2.
 -  if (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC) {
 +  if (GicSystemRegistersSupported()) {
 // Make sure System Register access is enabled (SRE). This depends on the
 // higher privilege level giving us permission, otherwise we will either
 // cause an exception here, or the write doesn't stick in which case we 
 need
 @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
 }
 if (IccSre  ICC_SRE_EL2_SRE) {
   mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
 -  goto Done;
 }
   }
 -
 -  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
 -
 -Done:
   return RETURN_SUCCESS;
 }
 
 diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c 
 b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 index f8822a2..75ba156 100644
 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h
 
 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision = 
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined (MDE_CPU_ARM)
 +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 +#elif defined(MDE_CPU_AARCH64)
 +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 +#else
 + #error Unknown chipset.
 +#endif
 +}
 
 RETURN_STATUS
 EFIAPI
 @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
   // feature is implemented on the CPU. This is also convenient as our GICv3
   // driver requires SRE. If only Memory mapped access is available we try to
   // drive the GIC as a v2.
 -  if (ArmReadIdPfr1 ()  ARM_PFR1_GIC) {
 +  if (GicSystemRegistersSupported()) {
 // Make sure System Register access is enabled (SRE). This depends on the
 // higher privilege level giving us permission, otherwise we will either
 // cause an exception here, or the write doesn't stick in which case we 
 need
 @@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
 }
 if (IccSre  ICC_SRE_EL2_SRE) {
   mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
 -  goto Done;
 }
   }
 -
 -  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
 -
 -Done:
   return RETURN_SUCCESS;
 }
 
 -- 
 2.1.0
 
 ___
 edk2-devel mailing list
 edk2-devel@lists.01.org
 https://lists.01.org/mailman/listinfo/edk2-devel

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


[edk2] [PATCH 0/9] OvmfPkg, ArmVirtPkg: SMBIOS 3.0, round 2

2015-07-31 Thread Laszlo Ersek
The SMBIOS 3.x entry point is structurally different from the 2.x
variant, therefore the current code used by OvmfPkg and ArmVirtPkg
doesn't recognize an SMBIOS 3.0 entry point structure even if QEMU
exports one. This series rectifies that.

The first four patches implement Jordan's great idea: they refactor the
code without any observable changes, unifying the currently duplicated
QEMU SMBIOS entry point checks / version setting between OvmfPkg and
ArmVirtPkg.

Patches 5 to 7, inclusive, eliminate yet another instance of the entry
point checks, this time the last one, from SmbiosPlatformDxe.

Building on the centralized QEMU SMBIOS entry point examination, patches
8 and 9 add SMBIOS 3.0 support for real.

The series builds at every stage. I regression-tested it with OVMF (2.8
entry point), and tested the 3.0 entry point support using Wei's QEMU
series pending at
http://thread.gmane.org/gmane.comp.emulators.qemu/353282, with a small
QEMU fix on top (which I posted in that thread).

As I wrote in that thread as well, I didn't test with dmidecode in an
aarch64 Linux guest just yet; I used the smbiosview UEFI shell command
in the guest.

Public branch: https://github.com/lersek/edk2/commits/smbios30_round2.

Wei, you can find a binary at
http://people.redhat.com/~lersek/smbios30_round2_v1/QEMU_EFI.fd.xz,
for testing any further QEMU changes.

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Cc: Jordan Justen jordan.l.jus...@intel.com
Cc: Gabriel L. Somlo so...@cmu.edu

Thanks
Laszlo

Laszlo Ersek (9):
  OvmfPkg: SmbiosVersionLib: add plugin for detecting SMBIOS version
  OvmfPkg: set SMBIOS version in DetectSmbiosVersionLib instead of
PlatformPei
  ArmVirtPkg: set SMBIOS version in DetectSmbiosVersionLib instead of
QemuFwCfgToPcdDxe
  ArmVirtPkg: revert ArmVirtPkg: add QemuFwCfgToPcdDxe
  OvmfPkg: introduce PcdQemuSmbiosValidated
  ArmVirtPkg/ArmVirtQemu.dsc: set default for PcdQemuSmbiosValidated
  OvmfPkg: SmbiosPlatformDxe: eliminate duplicate entry point validation
  ArmVirtPkg: set default for PcdSmbiosDocRev
  OvmfPkg: SmbiosVersionLib: recognize SMBIOS 3.x entry point

 ArmVirtPkg/ArmVirtQemu.dsc  |   8 +-
 ArmVirtPkg/ArmVirtQemu.fdf  |   2 -
 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c   |  73 -
 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf |  46 -
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c   | 107 

 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf |  53 ++
 OvmfPkg/OvmfPkg.dec |   1 +
 OvmfPkg/OvmfPkgIa32.dsc |   7 +-
 OvmfPkg/OvmfPkgIa32X64.dsc  |   7 +-
 OvmfPkg/OvmfPkgX64.dsc  |   7 +-
 OvmfPkg/PlatformPei/Platform.c  |  39 ---
 OvmfPkg/PlatformPei/PlatformPei.inf |   2 -
 OvmfPkg/SmbiosPlatformDxe/Qemu.c|  30 ++
 OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf |   4 +
 14 files changed, 198 insertions(+), 188 deletions(-)
 create mode 100644 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
 create mode 100644 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
 delete mode 100644 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c
 delete mode 100644 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf

-- 
1.8.3.1

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


[edk2] [PATCH 1/9] OvmfPkg: SmbiosVersionLib: add plugin for detecting SMBIOS version

2015-07-31 Thread Laszlo Ersek
Introduce a minimal library instance for fetching and validating the
SMBIOS entry point structure exposed by QEMU over fw_cfg. This library is
meant to be hooked into MdeModulePkg/Universal/SmbiosDxe by platform DSC
files, so that the library can set the PCD(s) that SmbiosDxe consumes at
the right moment.

At the moment only SMBIOS 2.x entry points are recognized.

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Cc: Jordan Justen jordan.l.jus...@intel.com
Cc: Gabriel L. Somlo so...@cmu.edu
Suggested-by: Jordan Justen jordan.l.jus...@intel.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf | 51 +
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c   | 76 

 2 files changed, 127 insertions(+)

diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf 
b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
new file mode 100644
index 000..14c25c9
--- /dev/null
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
@@ -0,0 +1,51 @@
+## @file
+#
+# A hook-in library for MdeModulePkg/Universal/SmbiosDxe, in order to set
+# gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion (and possibly other PCDs)
+# just before SmbiosDxe consumes them.
+#
+# Copyright (C) 2013, 2015, Red Hat, Inc.
+# Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.BR
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License which accompanies this
+# distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN AS IS BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+##
+
+[Defines]
+  INF_VERSION= 0x00010005
+  BASE_NAME  = DetectSmbiosVersionLib
+  FILE_GUID  = 6c633bb2-ae33-49ae-9f89-b5aa999fe3ae
+  MODULE_TYPE= BASE
+  VERSION_STRING = 1.0
+  LIBRARY_CLASS  = SmbiosVersionLib|DXE_DRIVER
+  CONSTRUCTOR= DetectSmbiosVersion
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+#  VALID_ARCHITECTURES   = IA32 X64 ARM AARCH64
+#
+
+[Sources]
+  DetectSmbiosVersionLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  BaseMemoryLib
+  DebugLib
+  PcdLib
+  QemuFwCfgLib
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion
diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c 
b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
new file mode 100644
index 000..0efe020
--- /dev/null
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
@@ -0,0 +1,76 @@
+/** @file
+
+  A hook-in library for MdeModulePkg/Universal/SmbiosDxe, in order to set
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion (and possibly other PCDs)
+  just before SmbiosDxe consumes them.
+
+  Copyright (C) 2013, 2015, Red Hat, Inc.
+  Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.BR
+
+  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 IndustryStandard/SmBios.h
+
+#include Base.h
+#include Library/BaseMemoryLib.h
+#include Library/DebugLib.h
+#include Library/PcdLib.h
+#include Library/QemuFwCfgLib.h
+
+typedef union {
+  SMBIOS_TABLE_ENTRY_POINT V2;
+} QEMU_SMBIOS_ANCHOR;
+
+RETURN_STATUS
+EFIAPI
+DetectSmbiosVersion (
+  VOID
+  )
+{
+  FIRMWARE_CONFIG_ITEM Anchor, Tables;
+  UINTNAnchorSize, TablesSize;
+  QEMU_SMBIOS_ANCHOR   QemuAnchor;
+  UINT16   SmbiosVersion;
+
+  if (RETURN_ERROR (QemuFwCfgFindFile (
+  etc/smbios/smbios-anchor, Anchor, AnchorSize)) ||
+  RETURN_ERROR (QemuFwCfgFindFile (
+  etc/smbios/smbios-tables, Tables, TablesSize)) ||
+  TablesSize == 0) {
+return RETURN_SUCCESS;
+  }
+
+  QemuFwCfgSelectItem (Anchor);
+
+  switch (AnchorSize) {
+  case sizeof QemuAnchor.V2:
+QemuFwCfgReadBytes (AnchorSize, QemuAnchor);
+
+if (QemuAnchor.V2.MajorVersion != 2 ||
+QemuAnchor.V2.TableLength != TablesSize ||
+CompareMem (QemuAnchor.V2.AnchorString, _SM_, 4) != 0 ||
+CompareMem (QemuAnchor.V2.IntermediateAnchorString, _DMI_, 5) != 0) {
+  return RETURN_SUCCESS;
+}
+SmbiosVersion = 

[edk2] [PATCH 3/9] ArmVirtPkg: set SMBIOS version in DetectSmbiosVersionLib instead of QemuFwCfgToPcdDxe

2015-07-31 Thread Laszlo Ersek
This patch de-duplicates the logic added in commit

  ArmVirtPkg: QemuFwCfgToPcdDxe: set SMBIOS entry point version
  dynamically

(git c98da334, SVN r18043) by hooking DetectSmbiosVersionLib into
SmbiosDxe.

Although said commit was supposed to work with SMBIOS 3.0 payloads from
QEMU, in practice that never worked, because the size / signature checks
in SmbiosVersionInitialization() would always fail, due to the SMBIOS 3.0
entry point being structurally different. Therefore this patch doesn't
regress ArmVirtPkg.

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Suggested-by: Jordan Justen jordan.l.jus...@intel.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf |  4 --
 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c   | 40 
 ArmVirtPkg/ArmVirtQemu.dsc  |  5 ++-
 3 files changed, 4 insertions(+), 45 deletions(-)

diff --git a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf 
b/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
index 649cfdc..a9983be 100644
--- a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
+++ b/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
@@ -29,18 +29,14 @@ [Sources]
 
 [Packages]
   MdePkg/MdePkg.dec
-  MdeModulePkg/MdeModulePkg.dec
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
-  BaseMemoryLib
-  DebugLib
   PcdLib
   QemuFwCfgLib
   UefiDriverEntryPoint
 
 [Pcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion
 
 [Depex]
   TRUE
diff --git a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c 
b/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c
index 814bb5c..8f60e21 100644
--- a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c
+++ b/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c
@@ -19,48 +19,9 @@
 #include Uefi/UefiBaseType.h
 #include Uefi/UefiSpec.h
 
-#include IndustryStandard/SmBios.h
-
-#include Library/BaseMemoryLib.h
-#include Library/DebugLib.h
 #include Library/PcdLib.h
 #include Library/QemuFwCfgLib.h
 
-
-/**
-  Set the SMBIOS entry point version for the generic SmbiosDxe driver.
-**/
-STATIC
-VOID
-SmbiosVersionInitialization (
-  VOID
-  )
-{
-  FIRMWARE_CONFIG_ITEM Anchor;
-  UINTNAnchorSize;
-  SMBIOS_TABLE_ENTRY_POINT QemuAnchor;
-  UINT16   SmbiosVersion;
-
-  if (RETURN_ERROR (QemuFwCfgFindFile (etc/smbios/smbios-anchor, Anchor,
-  AnchorSize)) ||
-  AnchorSize != sizeof QemuAnchor) {
-return;
-  }
-
-  QemuFwCfgSelectItem (Anchor);
-  QemuFwCfgReadBytes (AnchorSize, QemuAnchor);
-  if (CompareMem (QemuAnchor.AnchorString, _SM_, 4) != 0 ||
-  CompareMem (QemuAnchor.IntermediateAnchorString, _DMI_, 5) != 0) {
-return;
-  }
-
-  SmbiosVersion = (UINT16)(QemuAnchor.MajorVersion  8 |
-   QemuAnchor.MinorVersion);
-  DEBUG ((EFI_D_INFO, %a: SMBIOS version from QEMU: 0x%04x\n, __FUNCTION__,
-SmbiosVersion));
-  PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
-}
-
 EFI_STATUS
 EFIAPI
 ParseQemuFwCfgToPcd (
@@ -68,6 +29,5 @@ ParseQemuFwCfgToPcd (
   IN EFI_SYSTEM_TABLE *SystemTable
   )
 {
-  SmbiosVersionInitialization ();
   return EFI_SUCCESS;
 }
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0b69134..00571d4 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -349,7 +349,10 @@ [Components.common]
   #
   # SMBIOS Support
   #
-  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
+LibraryClasses
+  NULL|OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
+  }
   OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
 
   #
-- 
1.8.3.1


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


[edk2] [PATCH 4/9] ArmVirtPkg: revert ArmVirtPkg: add QemuFwCfgToPcdDxe

2015-07-31 Thread Laszlo Ersek
This reverts git commit d2733aa9 (SVN r18042), because it is empty now.
The original problem:

  Many universal DXE drivers in edk2 can be controlled by setting dynamic
  PCDs. Such a PCD must be set before the consumer DXE driver is
  dispatched.

should be hereafter solved similarly to how
OvmfPkg/Library/SmbiosVersionLib is plugged into
MdeModulePkg/Universal/SmbiosDxe now (originally suggested by Jordan
Justen jordan.l.jus...@intel.com).

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf | 42 
 ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c   | 33 ---
 ArmVirtPkg/ArmVirtQemu.dsc  |  1 -
 ArmVirtPkg/ArmVirtQemu.fdf  |  2 -
 4 files changed, 78 deletions(-)

diff --git a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf 
b/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
deleted file mode 100644
index a9983be..000
--- a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
+++ /dev/null
@@ -1,42 +0,0 @@
-## @file
-#  An early DXE driver that parses well-known fw-cfg files into dynamic PCDs
-#  that control other (universal) DXE drivers.
-#
-#  Copyright (C) 2015, Red Hat, Inc.
-#  Copyright (c) 2014, Linaro Ltd. All rights reserved.BR
-#
-#  This program and the accompanying materials are licensed and made available
-#  under the terms and conditions of the BSD License which accompanies this
-#  distribution.  The full text of the license may be found at
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN AS IS BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
-#  IMPLIED.
-#
-##
-
-[Defines]
-  INF_VERSION= 0x00010005
-  BASE_NAME  = QemuFwCfgToPcdDxe
-  FILE_GUID  = 5bb7cc92-1a36-4833-84cf-db7f8258e48d
-  MODULE_TYPE= DXE_DRIVER
-  VERSION_STRING = 1.0
-  ENTRY_POINT= ParseQemuFwCfgToPcd
-
-[Sources]
-  QemuFwCfgToPcd.c
-
-[Packages]
-  MdePkg/MdePkg.dec
-  OvmfPkg/OvmfPkg.dec
-
-[LibraryClasses]
-  PcdLib
-  QemuFwCfgLib
-  UefiDriverEntryPoint
-
-[Pcd]
-
-[Depex]
-  TRUE
diff --git a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c 
b/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c
deleted file mode 100644
index 8f60e21..000
--- a/ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c
+++ /dev/null
@@ -1,33 +0,0 @@
-/** @file
-*  An early DXE driver that parses well-known fw-cfg files into dynamic PCDs
-*  that control other (universal) DXE drivers.
-*
-*  Copyright (C) 2015, Red Hat, Inc.
-*  Copyright (c) 2014, Linaro Ltd. All rights reserved.BR
-*
-*  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 Uefi/UefiBaseType.h
-#include Uefi/UefiSpec.h
-
-#include Library/PcdLib.h
-#include Library/QemuFwCfgLib.h
-
-EFI_STATUS
-EFIAPI
-ParseQemuFwCfgToPcd (
-  IN EFI_HANDLE   ImageHandle,
-  IN EFI_SYSTEM_TABLE *SystemTable
-  )
-{
-  return EFI_SUCCESS;
-}
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 00571d4..618a158 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -316,7 +316,6 @@ [Components.common]
   # Platform Driver
   #
   ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
-  ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
   OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
   OvmfPkg/VirtioNetDxe/VirtioNet.inf
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index 3c0487c..f378da5 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -104,12 +104,10 @@ [FV.FvMain]
   APRIORI DXE {
 INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
 INF ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
-INF ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
   }
   INF MdeModulePkg/Core/Dxe/DxeMain.inf
   INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
   INF ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
-  INF ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.inf
 
   #
   # PI DXE Drivers producing Architectural Protocols (EFI Services)
-- 
1.8.3.1


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


[edk2] [PATCH 6/9] ArmVirtPkg/ArmVirtQemu.dsc: set default for PcdQemuSmbiosValidated

2015-07-31 Thread Laszlo Ersek
The upcoming OvmfPkg patches will implicitly affect the ArmVirtQemu.dsc
build, necessitating a default value for the new dynamic
PcdQemuSmbiosValidated. Add it.

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 ArmVirtPkg/ArmVirtQemu.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 618a158..8c842c7 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -237,6 +237,7 @@ [PcdsDynamicDefault.common]
   # SMBIOS entry point version
   #
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0300
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
 

 #
-- 
1.8.3.1


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


[edk2] [PATCH 7/9] OvmfPkg: SmbiosPlatformDxe: eliminate duplicate entry point validation

2015-07-31 Thread Laszlo Ersek
At this point all platforms that use OvmfPkg/SmbiosPlatformDxe in edk2,
namely ArmVirtQemu.dsc and OvmfPkg*.dsc, have been migrated to
SmbiosVersionLib. Therefore SmbiosPlatformDxe itself can forego verifying
QEMU's SMBIOS entry point; if SmbiosVersionLib's validation was
successful, it should just rely on that.

(Note that SmbiosPlatformDxe has a depex on EFI_SMBIOS_PROTOCOL, installed
by SmbiosDxe, containing SmbiosVersionLib, therefore the set/get order of
PcdQemuSmbiosValidated is ensured.)

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Cc: Jordan Justen jordan.l.jus...@intel.com
Cc: Gabriel L. Somlo so...@cmu.edu
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf |  1 +
 OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf |  4 +++
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c   | 14 +
 OvmfPkg/SmbiosPlatformDxe/Qemu.c| 30 
++--
 4 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf 
b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
index 14c25c9..8487e73 100644
--- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
@@ -49,3 +49,4 @@ [LibraryClasses]
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated
diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf 
b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
index 6935397..c8fa3bb 100644
--- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
+++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
@@ -57,6 +57,10 @@ [LibraryClasses]
   HobLib
   QemuFwCfgLib
   MemoryAllocationLib
+  PcdLib
+
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated
 
 [Protocols]
   gEfiSmbiosProtocolGuid  # PROTOCOL ALWAYS_CONSUMED
diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c 
b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
index 0efe020..9d5e337 100644
--- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
@@ -40,6 +40,15 @@ DetectSmbiosVersion (
   QEMU_SMBIOS_ANCHOR   QemuAnchor;
   UINT16   SmbiosVersion;
 
+  if (PcdGetBool (PcdQemuSmbiosValidated)) {
+//
+// Some other module, linked against this library, has already performed
+// the task at hand. This should never happen, but it's easy to handle;
+// just exit early.
+//
+return RETURN_SUCCESS;
+  }
+
   if (RETURN_ERROR (QemuFwCfgFindFile (
   etc/smbios/smbios-anchor, Anchor, AnchorSize)) ||
   RETURN_ERROR (QemuFwCfgFindFile (
@@ -72,5 +81,10 @@ DetectSmbiosVersion (
 SmbiosVersion));
   PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
 
+  //
+  // SMBIOS platform drivers can now fetch and install
+  // etc/smbios/smbios-tables from QEMU.
+  //
+  PcdSetBool (PcdQemuSmbiosValidated, TRUE);
   return RETURN_SUCCESS;
 }
diff --git a/OvmfPkg/SmbiosPlatformDxe/Qemu.c b/OvmfPkg/SmbiosPlatformDxe/Qemu.c
index f7ace4f..9466b95 100644
--- a/OvmfPkg/SmbiosPlatformDxe/Qemu.c
+++ b/OvmfPkg/SmbiosPlatformDxe/Qemu.c
@@ -15,6 +15,7 @@
 #include SmbiosPlatformDxe.h
 #include Library/QemuFwCfgLib.h
 #include Library/MemoryAllocationLib.h
+#include Library/PcdLib.h
 
 /**
   Locates and extracts the QEMU SMBIOS data if present in fw_cfg
@@ -27,32 +28,19 @@ GetQemuSmbiosTables (
   VOID
   )
 {
-  SMBIOS_TABLE_ENTRY_POINT QemuAnchor;
-  FIRMWARE_CONFIG_ITEM Anchor, Tables;
-  UINTNAnchorSize, TablesSize;
+  EFI_STATUS   Status;
+  FIRMWARE_CONFIG_ITEM Tables;
+  UINTNTablesSize;
   UINT8*QemuTables;
 
-  if (EFI_ERROR (QemuFwCfgFindFile (
-   etc/smbios/smbios-anchor, Anchor, AnchorSize)) ||
-  EFI_ERROR (QemuFwCfgFindFile (
-   etc/smbios/smbios-tables, Tables, TablesSize)) ||
-  AnchorSize != sizeof (QemuAnchor) ||
-  TablesSize == 0) {
+  if (!PcdGetBool (PcdQemuSmbiosValidated)) {
 return NULL;
   }
 
-  //
-  // We copy the entry point structure to perform some additional checks,
-  // but discard it upon return.
-  //
-  QemuFwCfgSelectItem (Anchor);
-  QemuFwCfgReadBytes (AnchorSize, QemuAnchor);
-
-  if (AsciiStrnCmp ((CHAR8 *)QemuAnchor.AnchorString, _SM_, 4) ||
-  AsciiStrnCmp ((CHAR8 *)QemuAnchor.IntermediateAnchorString, _DMI_, 5) 
||
-  TablesSize != QemuAnchor.TableLength) {
-return NULL;
-  }
+  Status = QemuFwCfgFindFile (etc/smbios/smbios-tables, Tables,
+ TablesSize);
+  ASSERT_EFI_ERROR (Status);
+  ASSERT (TablesSize  0);
 
   QemuTables = AllocatePool (TablesSize);
   if (QemuTables == NULL) {
-- 
1.8.3.1



[edk2] [PATCH 9/9] OvmfPkg: SmbiosVersionLib: recognize SMBIOS 3.x entry point

2015-07-31 Thread Laszlo Ersek
Also set the DocRev field the way QEMU exposes it, because
MdeModulePkg/Universal/SmbiosDxe lets us control that field too.

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Cc: Jordan Justen jordan.l.jus...@intel.com
Cc: Gabriel L. Somlo so...@cmu.edu
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf |  1 +
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c   | 17 
+
 OvmfPkg/OvmfPkgIa32.dsc |  1 +
 OvmfPkg/OvmfPkgIa32X64.dsc  |  1 +
 OvmfPkg/OvmfPkgX64.dsc  |  1 +
 5 files changed, 21 insertions(+)

diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf 
b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
index 8487e73..45d953a 100644
--- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
@@ -49,4 +49,5 @@ [LibraryClasses]
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated
diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c 
b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
index 9d5e337..950c3f7 100644
--- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
@@ -27,6 +27,7 @@
 
 typedef union {
   SMBIOS_TABLE_ENTRY_POINT V2;
+  SMBIOS_TABLE_3_0_ENTRY_POINT V3;
 } QEMU_SMBIOS_ANCHOR;
 
 RETURN_STATUS
@@ -73,6 +74,22 @@ DetectSmbiosVersion (
  QemuAnchor.V2.MinorVersion);
 break;
 
+  case sizeof QemuAnchor.V3:
+QemuFwCfgReadBytes (AnchorSize, QemuAnchor);
+
+if (QemuAnchor.V3.MajorVersion != 3 ||
+QemuAnchor.V3.TableMaximumSize != TablesSize ||
+CompareMem (QemuAnchor.V3.AnchorString, _SM3_, 5) != 0) {
+  return RETURN_SUCCESS;
+}
+SmbiosVersion = (UINT16)(QemuAnchor.V3.MajorVersion  8 |
+ QemuAnchor.V3.MinorVersion);
+
+DEBUG ((EFI_D_INFO, %a: SMBIOS 3.x DocRev from QEMU: 0x%02x\n,
+  __FUNCTION__, QemuAnchor.V3.DocRev));
+PcdSet8 (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
+break;
+
   default:
 return RETURN_SUCCESS;
   }
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 16f366d..48118cc 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -385,6 +385,7 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0208
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
 

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 9b44a0f..6860ad7 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -391,6 +391,7 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0208
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
 

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2921c2e..f877fda 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -390,6 +390,7 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0208
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
 

-- 
1.8.3.1

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


[edk2] [PATCH 8/9] ArmVirtPkg: set default for PcdSmbiosDocRev

2015-07-31 Thread Laszlo Ersek
When MdeModulePkg/Universal/SmbiosDxe is instructed to compose  install
an SMBIOS 3.0 entry point, it keys the Docrev (specification document
revision) field of that structure off of PcdSmbiosDocRev. An upcoming
OvmfPkg patch will have OvmfPkg/Library/SmbiosVersionLib set this PCD
dynamically. Because we use that driver in the ArmVirtQemu.dsc platform,
we must provide a default for the dynamic PCD.

Cc: Ard Biesheuvel ard.biesheu...@linaro.org
Cc: Wei Huang w...@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 ArmVirtPkg/ArmVirtQemu.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 8c842c7..ab94cbc 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -237,6 +237,7 @@ [PcdsDynamicDefault.common]
   # SMBIOS entry point version
   #
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0300
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
 

-- 
1.8.3.1


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


[edk2] [PATCH 2/9] OvmfPkg: set SMBIOS version in DetectSmbiosVersionLib instead of PlatformPei

2015-07-31 Thread Laszlo Ersek
This patch de-duplicates the logic added in commit

  OvmfPkg: PlatformPei: set SMBIOS entry point version dynamically

(git 37baf06b, SVN r17676) by hooking DetectSmbiosVersionLib into
SmbiosDxe.

Although said commit was supposed to work with SMBIOS 3.0 payloads from
QEMU, in practice that never worked, because the size / signature checks
in SmbiosVersionInitialization() would always fail, due to the SMBIOS 3.0
entry point being structurally different. Therefore this patch doesn't
regress OvmfPkg.

Cc: Wei Huang w...@redhat.com
Cc: Jordan Justen jordan.l.jus...@intel.com
Cc: Gabriel L. Somlo so...@cmu.edu
Suggested-by: Jordan Justen jordan.l.jus...@intel.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 OvmfPkg/PlatformPei/PlatformPei.inf |  2 -
 OvmfPkg/PlatformPei/Platform.c  | 39 
 OvmfPkg/OvmfPkgIa32.dsc |  5 ++-
 OvmfPkg/OvmfPkgIa32X64.dsc  |  5 ++-
 OvmfPkg/OvmfPkgX64.dsc  |  5 ++-
 5 files changed, 12 insertions(+), 44 deletions(-)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
b/OvmfPkg/PlatformPei/PlatformPei.inf
index cb7d7dd..81335a9 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -58,7 +58,6 @@ [LibraryClasses]
   QemuFwCfgLib
   MtrrLib
   PcdLib
-  BaseMemoryLib
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
@@ -82,7 +81,6 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
   gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 87c51d7..9970d14 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -32,11 +32,9 @@
 #include Library/PeiServicesLib.h
 #include Library/QemuFwCfgLib.h
 #include Library/ResourcePublicationLib.h
-#include Library/BaseMemoryLib.h
 #include Guid/MemoryTypeInformation.h
 #include Ppi/MasterBootMode.h
 #include IndustryStandard/Pci22.h
-#include IndustryStandard/SmBios.h
 #include OvmfPlatforms.h
 
 #include Platform.h
@@ -382,41 +380,6 @@ DebugDumpCmos (
 
 
 /**
-  Set the SMBIOS entry point version for the generic SmbiosDxe driver.
-**/
-STATIC
-VOID
-SmbiosVersionInitialization (
-  VOID
-  )
-{
-  FIRMWARE_CONFIG_ITEM Anchor;
-  UINTNAnchorSize;
-  SMBIOS_TABLE_ENTRY_POINT QemuAnchor;
-  UINT16   SmbiosVersion;
-
-  if (RETURN_ERROR (QemuFwCfgFindFile (etc/smbios/smbios-anchor, Anchor,
-  AnchorSize)) ||
-  AnchorSize != sizeof QemuAnchor) {
-return;
-  }
-
-  QemuFwCfgSelectItem (Anchor);
-  QemuFwCfgReadBytes (AnchorSize, QemuAnchor);
-  if (CompareMem (QemuAnchor.AnchorString, _SM_, 4) != 0 ||
-  CompareMem (QemuAnchor.IntermediateAnchorString, _DMI_, 5) != 0) {
-return;
-  }
-
-  SmbiosVersion = (UINT16)(QemuAnchor.MajorVersion  8 |
-   QemuAnchor.MinorVersion);
-  DEBUG ((EFI_D_INFO, %a: SMBIOS version from QEMU: 0x%04x\n, __FUNCTION__,
-SmbiosVersion));
-  PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
-}
-
-
-/**
   Perform Platform PEI initialization.
 
   @param  FileHandle  Handle of the file being invoked.
@@ -466,8 +429,6 @@ InitializePlatform (
 PeiFvInitialization ();
 
 MemMapInitialization ();
-
-SmbiosVersionInitialization ();
   }
 
   MiscInitialization ();
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 04c4204..d79c58a 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -527,7 +527,10 @@ [Components]
   #
   # SMBIOS Support
   #
-  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
+LibraryClasses
+  NULL|OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
+  }
   OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
 
   #
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 652126e..e891126 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -534,7 +534,10 @@ [Components.X64]
   #
   # SMBIOS Support
   #
-  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
+LibraryClasses
+  NULL|OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
+  }
   OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
 
   #
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index da820b0..fb785e2 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -532,7 +532,10 @@ [Components]
   #
   # SMBIOS Support
   #
-  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
+LibraryClasses
+  

Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

2015-07-31 Thread Cohen, Eugene
The .inf/source file approach is the way to be consistent with the rest of 
edk2.  I think it would be messy for the rest of edk2 to use .inf binding and 
for Arm packages to use a combination of #ifdef and .inf techniques.   As 
someone who has ported UEFI to a new processor architecture I appreciate the 
.inf approach.  When you try to build the component for a new architecture and 
you're missing the source file you I get nice linker errors, a virtual 'todo' 
list for the port.

There's a right way to do this - refactor to minimize the difference to the 
smallest possible chunk of code and avoid the temptation to copy and paste the 
existing function / source file.

Eugene

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Andrew 
Fish
Sent: Friday, July 31, 2015 10:54 AM
To: Ryan Harkin ryan.har...@linaro.org
Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List 
linaro-u...@lists.linaro.org; Leif Lindholm leif.lindh...@linaro.org; Ard 
Biesheuvel ard.biesheu...@linaro.org
Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for 
GICv3


 On Jul 31, 2015, at 9:37 AM, Ryan Harkin ryan.har...@linaro.org wrote:
 
 On 31 July 2015 at 17:16, Andrew Fish af...@apple.com wrote:
 
 
 On Jul 31, 2015, at 7:49 AM, Ryan Harkin ryan.har...@linaro.org wrote:
 
 Make Arm and Aarch64 both use the same code, conditionally compiled, to
 check if the platform has GICv3.
 
 Both source files for Arm and Aarch64 are now identical and the next
 step is to move to a common source file.
 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ryan Harkin ryan.har...@linaro.org
 Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
 .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 25
 --
 ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 25
 --
 2 files changed, 36 insertions(+), 14 deletions(-)
 
 diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 index 9853c7b..36dbe41 100644
 --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
 @@ -15,7 +15,23 @@
 #include Library/ArmLib.h
 #include Library/ArmGicLib.h
 
 -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
 +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
 ARM_GIC_ARCH_REVISION_2;
 +
 +STATIC
 +RETURN_STATUS
 +EFIAPI
 +GicSystemRegistersSupported (
 +  VOID
 +  )
 +{
 +#if defined (MDE_CPU_ARM)
 +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
 +#elif defined(MDE_CPU_AARCH64)
 +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
 +#else
 + #error Unknown chipset.
 +#endif
 +}
 
 
 We usually don’t use processor specific #ifdef’s in code like this in the
 edk2.
 
 
 That's probably why the code is filled with vast swathes of
 copy/paste/hack-one-line happening all over Arm*Pkg.
 

The previous code was a bad porting job. it should have been:

[Sources]
  CommonCode.c

[Sources.ARM]
  Arm/ArmSpecific.c

[Sources.AARCH64]
  AArch64/AArch64Specific.c


 Although, I could split the only GicSystemRegistersSupported function into
 it's own arch specific files to fit within that regime.
 

This is a good form as porting to a new processor architecture is quite easy. 
The process specific code is in a single file. 

The #ifdefs, and possible including processor specific includes can get really 
unwieldy in the general edk2 case that supports 6 CPU architectures. This form 
also makes porting to new processor architectures a lot easier. 

 Seems over the top to me.  So unless it's a hard requirement, I'd resist it.
 
 

Well the original issue was assumptions about what would happen in the code 
moving forward. What happens if some SOC comes along and uses a GIC for another 
CPU arch, or UEFI adds another CPU architecture?

I’m kind of 50/50 on this specific case. I would definitely reject if it was 
generic code. But for me if its 50/50 I vote against the code with #ifdefs. 

Thanks,

Andrew Fish

 
 
 We usually isolate the code to a file that is processor specific, and use
 the INF file to point at the correct one.
 
 Here is an example:
 https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
 
 [Sources]
  BasePeCoffLibInternals.h
  BasePeCoff.c
 
 [Sources.IA32, Sources.X64, Sources.EBC]
  PeCoffLoaderEx.c
 
 [Sources.IPF]
  Ipf/PeCoffLoaderEx.c
 
 [Sources.ARM]
  Arm/PeCoffLoaderEx.c
 
 [Sources.AARCH64]
  AArch64/PeCoffLoaderEx.c
 
 
 This is a good example of where the arch specific sources are not
 copy/paste/hack, but split out for good reason.
 
 
 Thanks,
 
 Andrew Fish
 
 RETURN_STATUS
 EFIAPI
 @@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
  // feature is implemented on the CPU. This is also convenient as our
 GICv3
  // driver requires SRE. If only Memory mapped access is available we try
 to
  // drive the GIC as a v2.
 -  if (ArmReadIdPfr0 ()  

Re: [edk2] [PATCH v2 0/4] FFS/FV aligment optimization

2015-07-31 Thread Ard Biesheuvel
On 31 July 2015 at 02:02, Laszlo Ersek ler...@redhat.com wrote:
 On 07/31/15 01:49, Ard Biesheuvel wrote:
 On 31 July 2015 at 01:17, Laszlo Ersek ler...@redhat.com wrote:
 On 07/27/15 15:52, Ard Biesheuvel wrote:
 On 27 July 2015 at 15:34, Liu, Yingke D yingke.d@intel.com wrote:
 Reviewed-by: Yingke Liu yingke.d@intel.com


 Thank you

 Committed as SVN r18077 ... r18080

 I do have another question related to the use of FIXED in a [Rule] section:
 since the increased alignment on AARCH64 will only be necessary for
 some toolchains, is it possible to have separate [Rule] sections?
 I did notice the 'DEBUG_MYTOOLS_IA32' in [Rule] section (for instance,
 EmbeddedPkg/EmbeddedPkg.fdf:102) but I don't quite understand how it
 works.

 Looking at the __GetFileOpts() method in
 BaseTools/Source/Python/GenFds/FdfParser.py, I think that pattern of
 tool chain keywords serves exactly the purpose you have in mind. I
 checked a couple occurrences of DEBUG_MYTOOLS_IA32 in various FDF files,
 and it always seems to restrict rules for the file type
 [Rule.Common.PEIM.TIANOCOMPRESSED]. So, in practice, it doesn matter:
 for ARM you usually (universally?) don't have LZMA compressed PEIMs,
 because PEIMs run from flash. The DEBUG_MYTOOLS_IA32 restriction doesn't
 matter because you don't have that module type anyway.


 OK, so the restriction means that this particular rule will only be
 applied if you are building DEBUG for IA32 with the MYTOOLS
 toolchains?

 I believe so, yes.


 Anyway, I think if you are going to add (or try) this kind of
 restriction, then I should postpone reviewing

   [edk2] [PATCH 2/4] ArmVirtPkg: use 'auto' alignment and FIXED
  placement for XIP modules
   http://thread.gmane.org/gmane.comp.bios.edk2.devel/547/focus=545

 because you might want to post a new version of it. Isn't that so?


 No, not really. The reason I was asking is for the CLANG patches I
 posted as well. As explained before, CLANG requires 4 kB alignment so
 that its relative symbol references resolve correctly.

 Okay. I thought that you wanted to add *_CLANG_* keywords or some such
 to the rules, after the FIXED keywod.


To be honest, I can't remember exactly what i wanted, but since the
optimization is beneficial to e.g., the ArmPlatformPkg PrePeicCore as
well, even built with GCC, I think it makes sense to make FIXED the
general rule. (The module in question has a 2 kB aligned vector table,
and it happens to line up correctly with the Align=128 that we use
currently. Once I change that to the correct 'auto' in this patch, we
lose 4 kB to padding unless we enable the optimization as well)

-- 
Ard.


 In any case, my most important question follows: the FDF spec says that
 FIXED means the file cannot be moved, it is not relocateable. But, how
 does that enable new optimizations in GenFfs, as you write in the
 linked patch? I found this in SVN r18079:

 + //
 + // Only add a special reducible padding section if
 + // - this FFS has the FFS_ATTRIB_FIXED attribute,
 + // - none of the preceding sections have alignment requirements,
 + // - the size of the padding is sufficient for the
 + // EFI_SECTION_FREEFORM_SUBTYPE_GUID header.
 + //
 + if ((FfsAttrib  FFS_ATTRIB_FIXED) != 0 
 + MaxEncounteredAlignment = 1 
 + Offset = sizeof (EFI_FREEFORM_SUBTYPE_GUID_SECTION)) {
 + SectHeader-CommonHeader.Type = EFI_SECTION_FREEFORM_SUBTYPE_GUID;
 + SectHeader-SubTypeGuid = mEfiFfsSectionAlignmentPaddingGuid;
 + } else {
 + SectHeader-CommonHeader.Type = EFI_SECTION_RAW;
 + }

 I'm quite sure you've explained why FIXED is important for this, but I
 can't recall the reason. Care to refresh my memory? Is it inherently
 related to mcmodel=small?


 No, it has nothing to do with that.

 The optimization I implemented results in the FFS to be unmovable,
 i.e., you can no longer infer from the metadata at which alignment you
 would need to load it to get the various sections to line up
 correctly. So initially, I suggested that GenFfs would set the
 FFS_ATTRIB_FIXED flag when applying the optimization.

 However, as Liming pointed out, this may affect use cases where some
 FFS is loaded into memory programmatically: such images would lose
 their ability to be loaded anywhere all of a sudden due to this
 optimization being applied. So instead, we set the attribute upfront
 for files that don't need to be moved around, and only apply the
 optimization if it has the flag set already.

 Okay. More or less understood. Thanks.
 Laszlo


 Note that this does not affect the ability to shadow the PEIMs, those
 are loaded from the TE images, not from the FFS containers.


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


[edk2] [PATCH 0/2] ArmPkg/ArmGicArchLib: use common source file

2015-07-31 Thread Ryan Harkin
The Arm and Aarch64 source files for ArmGicArchLib are copy/paste of 
each other with one minor difference to how they check for if the
platform has GICv3.

I made the change in two patches so that the diff could be identified 
separately from the move commit.

The first patch makes both the Arm and Aarch64 versions identical and
uses conditional compilation to handle Arm vs Aarch64.

  [PATCH 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

The second patch then removes the duplication in favour of a single source file.

  [PATCH 2/2] ArmPkg/ArmGicArchLib: use common source file
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [PATCH 2/2] ArmPkg/ArmGicArchLib: use common source file

2015-07-31 Thread Ryan Harkin
Now that the Arm and Aarch64 source files are identical and rely on
conditional compilation to provide arch specific code, remove the
duplicated files and use one common file.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ryan Harkin ryan.har...@linaro.org
---
 .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 76 --
 .../ArmGicArchLib/{Arm = }/ArmGicArchLib.c|  0
 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf |  7 +-
 3 files changed, 2 insertions(+), 81 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c 
b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
deleted file mode 100644
index b092e3a..000
--- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
+++ /dev/null
@@ -1,76 +0,0 @@
-/** @file
-*
-*  Copyright (c) 2014, ARM 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 Library/ArmLib.h
-#include Library/ArmGicLib.h
-
-STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision = ARM_GIC_ARCH_REVISION_2;
-
-RETURN_STATUS
-EFIAPI
-ArmGicArchLibHasGicv3 (
-  VOID
-  )
-{
-#if defined (MDE_CPU_ARM)
-  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
-#elif defined(MDE_CPU_AARCH64)
-  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
-#else
- #error Unknown chipset.
-#endif
-}
-
-RETURN_STATUS
-EFIAPI
-ArmGicArchLibInitialize (
-  VOID
-  )
-{
-  UINT32IccSre;
-
-  // Ideally we would like to use the GICC IIDR Architecture version here, but
-  // this does not seem to be very reliable as the implementation could easily
-  // get it wrong. It is more reliable to check if the GICv3 System Register
-  // feature is implemented on the CPU. This is also convenient as our GICv3
-  // driver requires SRE. If only Memory mapped access is available we try to
-  // drive the GIC as a v2.
-  if (ArmGicArchLibHasGicv3()) {
-// Make sure System Register access is enabled (SRE). This depends on the
-// higher privilege level giving us permission, otherwise we will either
-// cause an exception here, or the write doesn't stick in which case we 
need
-// to fall back to the GICv2 MMIO interface.
-// Note: We do not need to set ICC_SRE_EL2.Enable because the OS is started
-// at the same exception level.
-// It is the OS responsibility to set this bit.
-IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-if (!(IccSre  ICC_SRE_EL2_SRE)) {
-  ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);
-  IccSre = ArmGicV3GetControlSystemRegisterEnable ();
-}
-if (IccSre  ICC_SRE_EL2_SRE) {
-  mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
-}
-  }
-  return RETURN_SUCCESS;
-}
-
-ARM_GIC_ARCH_REVISION
-EFIAPI
-ArmGicGetSupportedArchRevision (
-  VOID
-  )
-{
-  return mGicArchRevision;
-}
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c 
b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
similarity index 100%
rename from ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c
diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf 
b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
index 7dbcb08..5c968e6 100644
--- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
+++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
@@ -20,11 +20,8 @@
   LIBRARY_CLASS  = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER 
UEFI_APPLICATION
   CONSTRUCTOR= ArmGicArchLibInitialize
 
-[Sources.ARM]
-  Arm/ArmGicArchLib.c
-
-[Sources.AARCH64]
-  AArch64/ArmGicArchLib.c
+[Sources.common]
+  ArmGicArchLib.c
 
 [Packages]
   MdePkg/MdePkg.dec
-- 
2.1.0

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


Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

2015-07-31 Thread Ryan Harkin
On 31 Jul 2015 21:21, Cohen, Eugene eug...@hp.com wrote:

 The .inf/source file approach is the way to be consistent with the rest
of edk2.  I think it would be messy for the rest of edk2 to use .inf
binding and for Arm packages to use a combination of #ifdef and .inf
techniques.   As someone who has ported UEFI to a new processor
architecture I appreciate the .inf approach.  When you try to build the
component for a new architecture and you're missing the source file you I
get nice linker errors, a virtual 'todo' list for the port

I'm not in favour of over engineering code for the sake of it. A #error
exactly at the line of code you need to examine is precise.  Linker errors
are never nice.

If someone wishes to expand this code beyond Arm, they will need to do more
than create a simple source file to provide a helper function.  The entire
package will need reworking.


 There's a right way to do this - refactor to minimize the difference to
the smallest possible chunk of code and avoid the temptation to copy and
paste the existing function / source file.

That's exactly what my patch achieves.


 Eugene

 -Original Message-
 From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
Andrew Fish
 Sent: Friday, July 31, 2015 10:54 AM
 To: Ryan Harkin ryan.har...@linaro.org
 Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List 
linaro-u...@lists.linaro.org; Leif Lindholm leif.lindh...@linaro.org;
Ard Biesheuvel ard.biesheu...@linaro.org
 Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check
for GICv3


  On Jul 31, 2015, at 9:37 AM, Ryan Harkin ryan.har...@linaro.org wrote:
 
  On 31 July 2015 at 17:16, Andrew Fish af...@apple.com wrote:
 
 
  On Jul 31, 2015, at 7:49 AM, Ryan Harkin ryan.har...@linaro.org
wrote:
 
  Make Arm and Aarch64 both use the same code, conditionally compiled, to
  check if the platform has GICv3.
 
  Both source files for Arm and Aarch64 are now identical and the next
  step is to move to a common source file.
 
  Contributed-under: TianoCore Contribution Agreement 1.0
  Signed-off-by: Ryan Harkin ryan.har...@linaro.org
  Reviewed-by: Ard Biesheuvel ard.biesheu...@linaro.org
  ---
  .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 25
  --
  ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 25
  --
  2 files changed, 36 insertions(+), 14 deletions(-)
 
  diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  index 9853c7b..36dbe41 100644
  --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
  @@ -15,7 +15,23 @@
  #include Library/ArmLib.h
  #include Library/ArmGicLib.h
 
  -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
  +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
  ARM_GIC_ARCH_REVISION_2;
  +
  +STATIC
  +RETURN_STATUS
  +EFIAPI
  +GicSystemRegistersSupported (
  +  VOID
  +  )
  +{
  +#if defined (MDE_CPU_ARM)
  +  return (ArmReadIdPfr1 ()  ARM_PFR1_GIC);
  +#elif defined(MDE_CPU_AARCH64)
  +  return (ArmReadIdPfr0 ()  AARCH64_PFR0_GIC);
  +#else
  + #error Unknown chipset.
  +#endif
  +}
 
 
  We usually don’t use processor specific #ifdef’s in code like this in
the
  edk2.
 
 
  That's probably why the code is filled with vast swathes of
  copy/paste/hack-one-line happening all over Arm*Pkg.
 

 The previous code was a bad porting job. it should have been:

 [Sources]
   CommonCode.c

 [Sources.ARM]
   Arm/ArmSpecific.c

 [Sources.AARCH64]
   AArch64/AArch64Specific.c


  Although, I could split the only GicSystemRegistersSupported function
into
  it's own arch specific files to fit within that regime.
 

 This is a good form as porting to a new processor architecture is quite
easy. The process specific code is in a single file.

 The #ifdefs, and possible including processor specific includes can get
really unwieldy in the general edk2 case that supports 6 CPU architectures.
This form also makes porting to new processor architectures a lot easier.

  Seems over the top to me.  So unless it's a hard requirement, I'd
resist it.
 
 

 Well the original issue was assumptions about what would happen in the
code moving forward. What happens if some SOC comes along and uses a GIC
for another CPU arch, or UEFI adds another CPU architecture?

 I’m kind of 50/50 on this specific case. I would definitely reject if it
was generic code. But for me if its 50/50 I vote against the code with
#ifdefs.

 Thanks,

 Andrew Fish

 
 
  We usually isolate the code to a file that is processor specific, and
use
  the INF file to point at the correct one.
 
  Here is an example:
 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
 
  [Sources]
   BasePeCoffLibInternals.h
   BasePeCoff.c
 
  [Sources.IA32, Sources.X64, Sources.EBC]
   PeCoffLoaderEx.c
 
  [Sources.IPF]
   Ipf/PeCoffLoaderEx.c
 
  [Sources.ARM]
   

Re: [edk2] [PATCH 2/4] ArmVirtPkg: use 'auto' alignment and FIXED placement for XIP modules

2015-07-31 Thread Ard Biesheuvel
On 31 July 2015 at 02:10, Laszlo Ersek ler...@redhat.com wrote:
 On 07/28/15 18:42, Ard Biesheuvel wrote:
 Now that GenFw correctly propagates the minimum alignment of the ELF
 input sections to the PE/COFF binary, we can simply select 'auto'
 alignment in the FDF Rule section instead of tweaking it by hand.

 Also add the FIXED FFS attribute to the module types that may execute
 in place. This enables a newly added optimization in GenFfs that strips
 redundant padding, preventing excessive waste of FV space if the section
 alignment is considerable (i.e., 2 KB or 4 KB)

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  ArmVirtPkg/ArmVirtQemu.fdf | 12 ++--
  ArmVirtPkg/ArmVirtXen.fdf  | 12 ++--
  2 files changed, 12 insertions(+), 12 deletions(-)

 diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
 index 4ef0f8bb6aa4..3c0487cd95b6 100644
 --- a/ArmVirtPkg/ArmVirtQemu.fdf
 +++ b/ArmVirtPkg/ArmVirtQemu.fdf
 @@ -309,20 +309,20 @@ [FV.FVMAIN_COMPACT]
  

  [Rule.Common.SEC]
 -  FILE SEC = $(NAMED_GUID) RELOCS_STRIPPED {
 -TE  TE Align = 128  $(INF_OUTPUT)/$(MODULE_NAME).efi
 +  FILE SEC = $(NAMED_GUID) RELOCS_STRIPPED FIXED {
 +TE  TE Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi
}

  [Rule.Common.PEI_CORE]
 -  FILE PEI_CORE = $(NAMED_GUID) {
 -TE TE Align = 8 $(INF_OUTPUT)/$(MODULE_NAME).efi
 +  FILE PEI_CORE = $(NAMED_GUID) FIXED {
 +TE TE Align = Auto  $(INF_OUTPUT)/$(MODULE_NAME).efi
  UI STRING =$(MODULE_NAME) Optional
}

  [Rule.Common.PEIM]
 -  FILE PEIM = $(NAMED_GUID) {
 +  FILE PEIM = $(NAMED_GUID) FIXED {
   PEI_DEPEX PEI_DEPEX Optional   $(INF_OUTPUT)/$(MODULE_NAME).depex
 - TE   TE Align = 8  $(INF_OUTPUT)/$(MODULE_NAME).efi
 + TE   TE Align = Auto   $(INF_OUTPUT)/$(MODULE_NAME).efi
   UI   STRING=$(MODULE_NAME) Optional
}

 diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
 index f98772b7191d..97cab4b058f2 100644
 --- a/ArmVirtPkg/ArmVirtXen.fdf
 +++ b/ArmVirtPkg/ArmVirtXen.fdf
 @@ -220,20 +220,20 @@ [FV.FVMAIN_COMPACT]
  

  [Rule.Common.SEC]
 -  FILE SEC = $(NAMED_GUID) RELOCS_STRIPPED {
 -TE  TE Align = 4K   $(INF_OUTPUT)/$(MODULE_NAME).efi
 +  FILE SEC = $(NAMED_GUID) RELOCS_STRIPPED FIXED {
 +TE  TE Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi
}

  [Rule.Common.PEI_CORE]
 -  FILE PEI_CORE = $(NAMED_GUID) {
 -TE TE Align = 8 $(INF_OUTPUT)/$(MODULE_NAME).efi
 +  FILE PEI_CORE = $(NAMED_GUID) FIXED {
 +TE TE Align = Auto  $(INF_OUTPUT)/$(MODULE_NAME).efi
  UI STRING =$(MODULE_NAME) Optional
}

  [Rule.Common.PEIM]
 -  FILE PEIM = $(NAMED_GUID) {
 +  FILE PEIM = $(NAMED_GUID) FIXED {
   PEI_DEPEX PEI_DEPEX Optional   $(INF_OUTPUT)/$(MODULE_NAME).depex
 - TE   TE Align = 8  $(INF_OUTPUT)/$(MODULE_NAME).efi
 + TE   TE Align = Auto   $(INF_OUTPUT)/$(MODULE_NAME).efi
   UI   STRING=$(MODULE_NAME) Optional
}



 So, if I understand correctly, FIXED has no drawback even when building
 with gcc, and when used in a clang build, it enables an optimization
 that is *then* significant. And, because there is no penalty when using
 gcc, it's simpler to add FIXED in a fixed way (pun intended) than to
 create clang-specific rules (eg. by duplicating the current rules and
 restricting them with *_XCODE5_* and *_GCC4x_* respectively).

 ... I hope the above mishmash can be called review. :)

 Reviewed-by: Laszlo Ersek ler...@redhat.com

Thanks Laszlo.

Since the Align=128 is arguably a bug, and I am getting the following
runtime error now when running ArmVirtQemu with the latest builds from
master

ASSERT /home/ard/build/uefi-next/ArmPlatformPkg/PrePeiCore/PrePeiCore.c(91):
((UINTN)PeiVectorTable  ((1  11)-1)) == 0

I have broken out this patch from the series and applied it separately
as SVN r18122.

(It seems that our CI is not cleaning BaseTools/ sufficiently between
builds, since the latest snapshot builds still contain .debug sections
that were removed by 0192b71ca322 BaseTools/GenFw: move .debug
contents to .data to save space))

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


Re: [edk2] [PATCH v2 0/4] FFS/FV aligment optimization (was: [RFC] small C model and LLVM/clang support for AARCH64)

2015-07-31 Thread Ard Biesheuvel
On 31 July 2015 at 03:57, Gao, Liming liming@intel.com wrote:
 Ard:
   This way may not work. I need to confirm it.

   I think you can provide the two Ffs Rules. One is with Fixed and 4K, 
 another is no. They have the different Rule names. Then, apply the different 
 one in the different tool chains. The example is like below. Could you try it?

 !If $(TOOL_CHAIN_TAG) == GCC49
 INF RuleOverride = FIXED4K Pei.inf
 !else
 INF Pei.inf
 !endif

 [Rule.Common.PEIM.FIXED4K]
 FILE PEIM = $(NAMED_GUID) FIXED {
 ..
 }

 [Rule.Common.PEIM]
 FILE PEIM = $(NAMED_GUID) {
 ...
 }


Yes, that works.

But as I explained in the other thread (to myself as well, to be
honest) is that, since there is no downside to using FIXED for these
FFS files, and GCC may benefit from the optimization as well in some
cases, I prefer to just set the FIXED attributes for these modules
unconditionally.

Thanks,
Ard.


 -Original Message-
 From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
 Sent: Monday, July 27, 2015 9:53 PM
 To: Liu, Yingke D
 Cc: Gao, Liming; edk2-devel@lists.01.org; leif.lindh...@linaro.org; 
 eugene.co...@hp.com
 Subject: Re: [PATCH v2 0/4] FFS/FV aligment optimization (was: [RFC] small C 
 model and LLVM/clang support for AARCH64)

 On 27 July 2015 at 15:34, Liu, Yingke D yingke.d@intel.com wrote:
 Reviewed-by: Yingke Liu yingke.d@intel.com


 Thank you

 Committed as SVN r18077 ... r18080

 I do have another question related to the use of FIXED in a [Rule] section:
 since the increased alignment on AARCH64 will only be necessary for some 
 toolchains, is it possible to have separate [Rule] sections?
 I did notice the 'DEBUG_MYTOOLS_IA32' in [Rule] section (for instance,
 EmbeddedPkg/EmbeddedPkg.fdf:102) but I don't quite understand how it works.

 Thanks,
 Ard.


 -Original Message-
 From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
 Sent: Monday, July 27, 2015 8:32 PM
 To: Gao, Liming; Liu, Yingke D; edk2-devel@lists.01.org
 Cc: leif.lindh...@linaro.org; eugene.co...@hp.com; Ard Biesheuvel
 Subject: [PATCH v2 0/4] FFS/FV aligment optimization (was: [RFC] small
 C model and LLVM/clang support for AARCH64)

 This series deals with the basetools optimizations to get rid of excessive 
 XIP alignment padding when using 2 KB or 4 KB alignment, which is common on 
 AARCH64, since the exception vector table requires 2 KB alignment, and the 
 small code model (which is the only one supported by the commercial LLVM 
 based compiler supplied by ARM) requires 4 KB alignment.


 --
 --
 v2 changelog
 - patches #1 and #2 are unchanged
 - patch #3 and #4 have been updated to only emit or adjust the special 
 padding
   section if the FDF [Rule] defines the FFS_ATTRIB_FIXED attributed
 for the file
 - the adjustment logic in patch #4 has been reordered and the comments 
 updated
   to reflect more clearly that the misalignment and adjustment are not 
 specific
   to a single FFS section, but to the alignment of the FFS file as a
 whole
 - patch #4 now clears the alignment bits in the FFS header since they have no
   meaning in FFS files that have been modified in place
 - replaced pointer arithmetic using VOID* pointers

 --
 --
 v1 changelog compared to 'RFC: small C model and LLVM/clang support for 
 AARCH64'

 Patches #1 and #2 (formerly #2 and #3) are unchanged.

 Patch #3 (formerly #4) has been updated to ensure that a special reducible 
 padding section is only emitted right before the first section in a FFS that 
 has alignment requirements. Reducing the size of such a section will shift 
 all subsequent sections into alignment, provided that the size of the 
 padding is sufficient. In some cases, for instance, when the padding section 
 is based on a section that has a minimum alignment of 32 bytes, and is 
 followed by a section which requires 4 KB alignment, the size of the padding 
 section may be too small and the adjustment will not be possible. In this 
 case, we simply proceed as if the padding section is an ordinary padding 
 section, and everything will just work as before.

 Patch #4 is unchanged, except for a clarification in the comments, to 
 explain that the misalignment is calculated based on the first byte of the 
 FFS payload, and not of the aligned section. Since all FFS sections are 
 padded out relative to the first byte of the FFS payload, compensating its 
 misalignment will shift all sections into place.

 Ard Biesheuvel (4):
   BaseTools/GenFw: move .debug contents to .data to save space
   BaseTools/GenFw: move PE/COFF header closer to payload
   BaseTools: use GUID identifiable section for FFS alignment padding
   BaseTools/GenFv: optimize away redundant padding

  BaseTools/Source/C/GenFfs/GenFfs.c   |  99 
 +++-
  BaseTools/Source/C/GenFv/GenFvInternalLib.c  |