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

[Ard Biesheuvel]

On 31 July 2015 at 02:02, Laszlo Ersek  wrote:
> On 07/31/15 01:49, Ard Biesheuvel wrote:
>> On 31 July 2015 at 01:17, Laszlo Ersek  wrote:
>>> On 07/27/15 15:52, Ard Biesheuvel wrote:
 On 27 July 2015 at 15:34, Liu, Yingke D  wrote:
> Reviewed-by: Yingke Liu 
>

 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

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

[Ard Biesheuvel]

On 31 July 2015 at 02:10, Laszlo Ersek  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 
>> ---
>>  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 

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)

[Ard Biesheuvel]

On 31 July 2015 at 03:57, Gao, Liming  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  wrote:
>> Reviewed-by: Yingke Liu 
>>
>
> 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

Re: [edk2] [PATCH v4 00/13] BaseTools: unify all GCC linker scripts

[Ard Biesheuvel]

On 30 July 2015 at 16:16, Ard Biesheuvel  wrote:
> Fourth attempt at unifying the various GCC linker scripts for different
> architectures, GCC versions and minimum alignments.
>
> Changes since v3:
> - added patch #5 which updates the various IA32/X86 linker scripts to take 
> their
>   PE/COFF header size and section alignment from the command line, before
>   switching to the unified version which does the same
> - added Jordan's Reviewed-by, which he gave on the condition that patch #5
>   be added
> - added Liming's Tested-by to the patches that apply to IA32 and X86
> - added Leif's Tested-by to the patches that apply to AARCH64 (except the
>   ArmVirtPkg which he didn't test, but this was my testbed during development)
>
> Changes since v2:
> - for easier bisection, factor out the differences between the original
>   and the unified linker scripts for X86 before making the switch
>   (patches #1 - #4 and #6)
> - add Intel copyright notice to unified version (patch #5)
> - avoid defining *_*_*_DLINK2_FLAGS so that we don't pollute the variable
>   definition space of non-GCC toolchains (patches #8 and #12)
> - added Laszlo's ack to patch #10
>
>  v2 blurb -
> This time, I have added only a single unified GCC linker script that
> can be parametrised by ld command line options:
> - --defsym=PECOFF_HEADER_SIZE sets the size of the PE/COFF header
> - -z common-page-size sets the minimum alignment
>
> This use of common-page-size is entirely legal: it sets an internal LD
> constant which can be referred to as CONSTANT(COMMONPAGESIZE) in linker
> scripts, and is otherwise unused internally by the linker.
>
> Tested with ArmVirtQemu/AARCH64 and Ovmf/X64
>
> Branch is here
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/unify-gcc-ld-scripts-v4
> (now correctly based on the GitHub repo)
>

@Liming, Jordan:
now that everybody seems ok with these patches, can I go ahead and commit them?
I did not receive any feedback from Dennis, who is listed in
Maintainers.txt as the sole maintainer of BaseTools/

Thanks,
Ard.



> Ard Biesheuvel (13):
>   BaseTools IA32/X64: remove NOP padding from X86/IA32 GCC linker
> scripts
>   BaseTools IA32/X64: move .rodata to PE/COFF .text section
>   BaseTools IA32/X64: drop redundant alignment from linker script
>   BaseTools IA32/X64: move .got contents to the PE/COFF .text section
>   BaseTools IA32/X64: get header size and alignment from ld commandline
>   BaseTools GCC: add unified GCC linker script for all archs and
> versions
>   BaseTools GCC: align start of .data to .text alignment
>   BaseTools GCC: move AutoGen.obj contents to .text section
>   BaseTools AARCH64: move to unified GCC linker script
>   ArmPlatformPkg/ArmVExpressPkg: move to unified GCC linker script
>   ArmVirtPkg: move to unified GCC linker script
>   BaseTools AARCH64: remove incremental linker script for 64K alignment
>   BaseTools IA32/X64: Use GccBase.lds instead of gcc*-ld-script
>
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc |  2 +-
>  ArmVirtPkg/ArmVirt.dsc.inc|  2 +-
>  BaseTools/Conf/tools_def.template | 37 ++-
>  BaseTools/Scripts/GccBase.lds | 70 
>  BaseTools/Scripts/gcc-4K-align-ld-script  | 44 
>  BaseTools/Scripts/gcc-aarch64-64K-align-ld-script |  4 --
>  BaseTools/Scripts/gcc-aarch64-ld-script   | 39 ---
>  BaseTools/Scripts/gcc4.4-ld-script| 44 
>  BaseTools/Scripts/gcc4.9-ld-script| 44 
>  9 files changed, 106 insertions(+), 180 deletions(-)
>  create mode 100644 BaseTools/Scripts/GccBase.lds
>  delete mode 100644 BaseTools/Scripts/gcc-4K-align-ld-script
>  delete mode 100644 BaseTools/Scripts/gcc-aarch64-64K-align-ld-script
>  delete mode 100644 BaseTools/Scripts/gcc-aarch64-ld-script
>  delete mode 100644 BaseTools/Scripts/gcc4.4-ld-script
>  delete mode 100644 BaseTools/Scripts/gcc4.9-ld-script
>
> --
> 1.9.1
>
___
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

[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

[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 
---
 .../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 
-#include 
-
-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

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

[Ryan Harkin]

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 
---
 .../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 
 #include 
 
-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 (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);
   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 
 #include 
 
-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 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 +61,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

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

[Ard Biesheuvel]

On 31 July 2015 at 14:06, Ryan Harkin  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 
> ---
>  .../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 
>  #include 
>
> -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.

Reviewed-by: Ard Biesheuvel 

>  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);
>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 
>  #include 
>
> -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 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 +61,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

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

[Ard Biesheuvel]

On 31 July 2015 at 14:06, Ryan Harkin  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 

Reviewed-by: Ard Biesheuvel 

> ---
>  .../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 
> -#include 
> -
> -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 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

[Ryan Harkin]

On 31 July 2015 at 13:13, Ard Biesheuvel  wrote:

> On 31 July 2015 at 14:06, Ryan Harkin  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 
> > ---
> >  .../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 
> >  #include 
> >
> > -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.


>
> Reviewed-by: Ard Biesheuvel 
>

Thanks for the quick review turnaround :-)

>
> >  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);
> >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 
> >  #include 
> >
> > -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 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 +6

[edk2] [PATCH] MdeModulePkg DxeIpl: Add stack NX support

[Star Zeng]

This feature is added for UEFI spec that says
"Stack may be marked as non-executable in identity mapped page tables".
A PCD PcdSetNxForStack is added to turn on/off this feature, and it is
FALSE by default.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |   3 +-
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 185 ++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  10 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 160 +---
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  61 +++-
 MdeModulePkg/MdeModulePkg.dec|  11 +-
 MdeModulePkg/MdeModulePkg.uni| Bin 166792 -> 168862 bytes
 7 files changed, 392 insertions(+), 38 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf 
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 1473ccd..66c58b1 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -5,7 +5,7 @@
 #  PPI to discover and dispatch the DXE Foundation and components that are
 #  needed to run the DXE Foundation.
 #
-#  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
+#  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD 
License
 #  which accompanies this distribution.  The full text of the license may be 
found at
@@ -104,6 +104,7 @@ [FeaturePcd]
 
 [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack   ## 
SOMETIMES_CONSUMES
 
 [Depex]
   gEfiPeiMemoryDiscoveredPpiGuid AND gEfiPeiLoadFilePpiGuid AND 
gEfiPeiMasterBootModePpiGuid
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c 
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 38832cc..c1ff4b7 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -1,7 +1,7 @@
 /** @file
   Ia32-specific functionality for DxeLoad.
 
-Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -57,6 +57,151 @@ GLOBAL_REMOVE_IF_UNREFERENCED  IA32_DESCRIPTOR 
gLidtDescriptor = {
 };
 
 /**
+  Allocates and fills in the Page Directory and Page Table Entries to
+  establish a 4G page table.
+
+  @param[in] StackBase  Stack base address.
+  @param[in] StackSize  Stack size.
+
+  @return The address of page table.
+
+**/
+UINTN
+Create4GPageTablesIa32Pae (
+  IN EFI_PHYSICAL_ADDRESS   StackBase,
+  IN UINTN  StackSize
+  )
+{  
+  UINT8 PhysicalAddressBits;
+  EFI_PHYSICAL_ADDRESS  PhysicalAddress;
+  UINTN IndexOfPdpEntries;
+  UINTN IndexOfPageDirectoryEntries;
+  UINT32NumberOfPdpEntriesNeeded;
+  PAGE_MAP_AND_DIRECTORY_POINTER*PageMap;
+  PAGE_MAP_AND_DIRECTORY_POINTER*PageDirectoryPointerEntry;
+  PAGE_TABLE_ENTRY  *PageDirectoryEntry;
+  UINTN TotalPagesNum;
+  UINTN PageAddress;
+
+  PhysicalAddressBits = 32;
+
+  //
+  // Calculate the table entries needed.
+  //
+  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, (PhysicalAddressBits - 
30));
+
+  TotalPagesNum = NumberOfPdpEntriesNeeded + 1;
+  PageAddress = (UINTN) AllocatePages (TotalPagesNum);
+  ASSERT (PageAddress != 0);
+
+  PageMap = (VOID *) PageAddress;
+  PageAddress += SIZE_4KB;
+
+  PageDirectoryPointerEntry = PageMap;
+  PhysicalAddress = 0;
+
+  for (IndexOfPdpEntries = 0; IndexOfPdpEntries < NumberOfPdpEntriesNeeded; 
IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
+//
+// Each Directory Pointer entries points to a page of Page Directory 
entires.
+// So allocate space for them and fill them in in the 
IndexOfPageDirectoryEntries loop.
+//   
+PageDirectoryEntry = (VOID *) PageAddress;
+PageAddress += SIZE_4KB;
+
+//
+// Fill in a Page Directory Pointer Entries
+//
+PageDirectoryPointerEntry->Uint64 = (UINT64) (UINTN) PageDirectoryEntry;
+PageDirectoryPointerEntry->Bits.Present = 1;
+
+for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; 
IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += 
SIZE_2MB) {
+  if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + 

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

[Ard Biesheuvel]

On 31 July 2015 at 14:21, Ryan Harkin  wrote:
>
>
> On 31 July 2015 at 13:13, Ard Biesheuvel  wrote:
>>
>> On 31 July 2015 at 14:06, Ryan Harkin  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 
>> > ---
>> >  .../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 
>> >  #include 
>> >
>> > -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 
>
>
> 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 
>> >  #include 
>> >
>> > -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
>> >

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

[Ryan Harkin]

On 31 July 2015 at 13:28, Ard Biesheuvel  wrote:

> On 31 July 2015 at 14:21, Ryan Harkin  wrote:
> >
> >
> > On 31 July 2015 at 13:13, Ard Biesheuvel 
> wrote:
> >>
> >> On 31 July 2015 at 14:06, Ryan Harkin  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 
> >> > ---
> >> >  .../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 
> >> >  #include 
> >> >
> >> > -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.
>

Sure, good point.  I'll add it for v2.


>
> >>
> >>
> >> Reviewed-by: Ard Biesheuvel 
> >
> >
> > Thanks for the quick review turnaround :-)
>
> Too quick, as it seems. Spotted an inadvertent white space change below :-)
>

Drat, I would have got away with it if it wasn't for you meddling kids :-)



>
> >>
> >>
> >> >  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 ^^^
>

I sort-of did that on purpose to make the two files identical.  The space
vanished when the original copy/paste/hack operation happened and it crept
back in when I made the Arm and Aarch64 files identical.

I know it's not important, but if I omit that whitespace change, then the
two files won't actually be identical before moving to a single common
source.

Now, do you *really* want me to drop this hunk for v2??



> >> >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..b092e3

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

[Ryan Harkin]

On 31 July 2015 at 14:22, Ryan Harkin  wrote:

>
>
> On 31 July 2015 at 13:28, Ard Biesheuvel 
> wrote:
>
>> On 31 July 2015 at 14:21, Ryan Harkin  wrote:
>> >
>> >
>> > On 31 July 2015 at 13:13, Ard Biesheuvel 
>> wrote:
>> >>
>> >> On 31 July 2015 at 14:06, Ryan Harkin  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 
>> >> > ---
>> >> >  .../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 
>> >> >  #include 
>> >> >
>> >> > -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.
>>
>
And thinking about this, as it isn't exported, I don't need the
ArmGicArchLib prefix, so I can just call it "GicSystemRegistersSupported"
and be done with it.


I do recommend STATIC for the function though.
>>
>
> Sure, good point.  I'll add it for v2.
>
>
>>
>> >>
>> >>
>> >> Reviewed-by: Ard Biesheuvel 
>> >
>> >
>> > Thanks for the quick review turnaround :-)
>>
>> Too quick, as it seems. Spotted an inadvertent white space change below
>> :-)
>>
>
> Drat, I would have got away with it if it wasn't for you meddling kids :-)
>
>
>
>>
>> >>
>> >>
>> >> >  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 ^^^
>>
>
> I sort-of did that on purpose to make the two files identical.  The space
> vanished when the original copy/paste/hack operation happened and it crept
> back in when I made the Arm and Aarch64 files identical.
>
> I know it's not important, but if I omit that whitespace change, then the
> two files won't actually be identical before moving to a single common
> source.
>
> Now, do you *really* want me to drop this hunk for v2??
>
>
>
>> >> >IccSre = ArmGicV3GetControlSystemRegisterEnable ();
>> >> >  }
>> >> >  if (IccSre & ICC_SRE_EL2_SRE) {
>> >> >mGicAr

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

[Ard Biesheuvel]

On 31 July 2015 at 15:27, Ryan Harkin  wrote:
>
>
> On 31 July 2015 at 14:22, Ryan Harkin  wrote:
>>
>>
>>
>> On 31 July 2015 at 13:28, Ard Biesheuvel 
>> wrote:
>>>
>>> On 31 July 2015 at 14:21, Ryan Harkin  wrote:
>>> >
>>> >
>>> > On 31 July 2015 at 13:13, Ard Biesheuvel 
>>> > wrote:
>>> >>
>>> >> On 31 July 2015 at 14:06, Ryan Harkin  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 
>>> >> > ---
>>> >> >  .../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 
>>> >> >  #include 
>>> >> >
>>> >> > -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.
>
>
> And thinking about this, as it isn't exported, I don't need the
> ArmGicArchLib prefix, so I can just call it "GicSystemRegistersSupported"
> and be done with it.
>

Indeed.

>
>>> I do recommend STATIC for the function though.
>>
>>
>> Sure, good point.  I'll add it for v2.
>>
>>>
>>>
>>> >>
>>> >>
>>> >> Reviewed-by: Ard Biesheuvel 
>>> >
>>> >
>>> > Thanks for the quick review turnaround :-)
>>>
>>> Too quick, as it seems. Spotted an inadvertent white space change below
>>> :-)
>>
>>
>> Drat, I would have got away with it if it wasn't for you meddling kids :-)
>>
>>
>>>
>>>
>>> >>
>>> >>
>>> >> >  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 ^^^
>>
>>
>> I sort-of did that on purpose to make the two files identical.  The space
>> vanished when the original copy/paste/hack operation happened and it crept
>> back in when I made the Arm and Aarch64 files identical.
>>
>> I know it's not important, but if I omit that whitespace change, then the
>> two files won't actually be identical before

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

[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.com
T +1.281.514.5973
C +1.512.659.1523
Hewlett-Packard Company
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

[edk2] Concerns with IP4Config2 - Part 2 - DHCP re-initiate on connectivity issues

[El-Haj-Mahmoud, Samer]

This is Part 2 of the issues we observed in IP4Config2 in EDK2

On the UEFI 2.4 IP4Config Protocol, if the NIC is configured for DHCP, the IP4 
driver instance for that NIC would re-initiate a DHCP if it detects that there 
is a network blip (cable remove, reconnect for example). On the UEFI 2.5 EDK2 
(Ip4Config2 protocol) this doesn't happen. 

Can someone confirm please if this is an issue or is by design? This seems like 
a regression from the previous IP4Config implementation.



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] Concerns with IP4Config2 - Part 3 - No RegisterDataNotify() on DHCP config

[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

[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 
Reviewed-by: Ard Biesheuvel 
---
 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 
-#include 
-
-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] [PATCH v2 0/2] ArmPkg/ArmGicArchLib: use common source

[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 v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

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

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

Changes since v1
 - rename new function from ArmGicArchLibHasGicv3 to
GicSystemRegistersSupported
 - make new function STATIC
 - remove whitespace change
 - keep the version with the space before and after '|'

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

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

[Ryan Harkin]

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 
Reviewed-by: Ard Biesheuvel 
---
 .../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 
 #include 
 
-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 (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 
 #include 
 
-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

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

[Ryan Harkin]

On 31 July 2015 at 15:49, Ryan Harkin  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 
> Reviewed-by: Ard Biesheuvel 
> ---
>  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 
> -#include 
> -
> -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 v2 2/2] ArmPkg/ArmGicArchLib: use common source file

[Ryan Harkin]

On 31 July 2015 at 15:50, Ryan Harkin  wrote:

>
>
> On 31 July 2015 at 15:49, Ryan Harkin  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 
>> Reviewed-by: Ard Biesheuvel 
>> ---
>>  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 
>> -#include 
>> -
>> -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
>

False alarm, I'm mis-reading how git send-email -M munged the report and I
thought I'd kept the version without the space before the '|'.

v2 is what I expected afterall.


>
>
>
>> -  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 v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3

[Andrew Fish]

> On Jul 31, 2015, at 7:49 AM, Ryan Harkin  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 
> Reviewed-by: Ard Biesheuvel 
> ---
> .../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 
> #include 
> 
> -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
 


[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 
> #include 
> 
> -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

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

[Ryan Harkin]

On 31 July 2015 at 17:16, Andrew Fish  wrote:

>
> On Jul 31, 2015, at 7:49 AM, Ryan Harkin  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 
> Reviewed-by: Ard Biesheuvel 
> ---
> .../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 
> #include 
>
> -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 
> #include 
>
> -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;
> -

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

[Andrew Fish]

> On Jul 31, 2015, at 9:37 AM, Ryan Harkin  wrote:
> 
> On 31 July 2015 at 17:16, Andrew Fish  wrote:
> 
>> 
>> On Jul 31, 2015, at 7:49 AM, Ryan Harkin  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 
>> Reviewed-by: Ard Biesheuvel 
>> ---
>> .../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 
>> #include 
>> 
>> -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 
>> #include 
>> 
>> -STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision;
>> +STATIC ARM_GIC_ARCH_REVISIONmGicArchRevision =
>> ARM_GIC_ARCH_REVISION_2;
>> +
>> +STATIC

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

[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 
Cc: Jordan Justen 
Cc: Gabriel L. Somlo 
Suggested-by: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #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 {
+
+  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 {
+
+  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 {
+
+  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 3/9] ArmVirtPkg: set SMBIOS version in DetectSmbiosVersionLib instead of QemuFwCfgToPcdDxe

[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 
Cc: Wei Huang 
Suggested-by: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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 
 #include 
 
-#include 
-
-#include 
-#include 
 #include 
 #include 
 
-
-/**
-  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 {
+
+  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"

[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 ).

Cc: Ard Biesheuvel 
Cc: Wei Huang 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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.
-#
-#  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.
-*
-*  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 
-#include 
-
-#include 
-#include 
-
-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 5/9] OvmfPkg: introduce PcdQemuSmbiosValidated

[Laszlo Ersek]

This dynamic PCD will enable a small code de-duplication between
OvmfPkg/SmbiosPlatformDxe and OvmfPkg/Library/SmbiosVersionLib. Since both
of those are also used in ArmVirtQemu.dsc, and we should avoid
cross-package commits when possible, this patch declares
PcdQemuSmbiosValidated first, and sets defaults for it in the OvmfPkg DSC
files.

Cc: Ard Biesheuvel 
Cc: Wei Huang 
Cc: Jordan Justen 
Cc: Gabriel L. Somlo 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 OvmfPkg/OvmfPkg.dec| 1 +
 OvmfPkg/OvmfPkgIa32.dsc| 1 +
 OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
 OvmfPkg/OvmfPkgX64.dsc | 1 +
 4 files changed, 4 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 80816e9..d79aff4 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -115,6 +115,7 @@ [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId|0|UINT16|0x1b
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE|BOOLEAN|0x21
 
 [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|FALSE|BOOLEAN|3
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index d79c58a..16f366d 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -385,6 +385,7 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0208
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
 

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

 #
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index fb785e2..2921c2e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -390,6 +390,7 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0208
+  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 6/9] ArmVirtPkg/ArmVirtQemu.dsc: set default for PcdQemuSmbiosValidated

[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 
Cc: Wei Huang 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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

[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 
Cc: Wei Huang 
Cc: Jordan Justen 
Cc: Gabriel L. Somlo 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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 
 #include 
+#include 
 
 /**
   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-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

[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
, 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: .

Wei, you can find a binary at
,
for testing any further QEMU changes.

Cc: Ard Biesheuvel 
Cc: Wei Huang 
Cc: Jordan Justen 
Cc: Gabriel L. Somlo 

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

[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 
Cc: Wei Huang 
Cc: Jordan Justen 
Cc: Gabriel L. Somlo 
Suggested-by: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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.
+#
+# 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.
+
+  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 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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 = (UINT16)(QemuAnchor.V2.MajorVersion << 8 |
+ QemuAnchor.V2.MinorVersion);
+break;
+
+  default:
+return RETURN_SUCCESS;
+  }
+
+  DEBUG ((EFI_D_INFO, "%a: SMBIOS version from QEMU: 0x%04x\n", __FUNCTION

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

[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 
Cc: Wei Huang 
Cc: Jordan Justen 
Cc: Gabriel L. Somlo 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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

[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 
Cc: Wei Huang 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 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

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

[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 
Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List 
; Leif Lindholm ; Ard 
Biesheuvel 
Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for 
GICv3


> On Jul 31, 2015, at 9:37 AM, Ryan Harkin  wrote:
> 
> On 31 July 2015 at 17:16, Andrew Fish  wrote:
> 
>> 
>> On Jul 31, 2015, at 7:49 AM, Ryan Harkin  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 
>> Reviewed-by: Ard Biesheuvel 
>> ---
>> .../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 
>> #include 
>> 
>> -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 (GicSystemRegistersSupport

[edk2] AMD's UEFI strategy? - AMD Response

[Simpson, Gary]

> Is anyone from AMD on this list? If you know the right person, could you 
> please forward?
[Gary Simpson] Hi, Lee.  My name is Gary Simpson (Firmware Architect at AMD).  
Your post was forwarded to me.  (Thanks to both people who did so.)  If 
desired, you can contact me directly at: gary.simp...@amd.com, but I have now 
joined this list.

> Can anyone explain AMD's strategy w/r/t UEFI and BIOS, UEFI and coreboot?
[Gary Simpson] I'll try.  Here's some quick background: AMD is a founding Board 
member (i.e. Promoter) of the UEFI Forum and an active member in most of the 
work groups.  We are proponents of the UEFI and ACPI interfaces (because they 
provide standardized firmware API's, allowing shrink-wrapped OS distributions, 
without customized drivers, enabling end-user OS flexibility and choice).  
Also, despite some birthing pains with individual implementations, UEFI is 
enormously more secure than legacy BIOS was.  AMD's evolution from legacy BIOS 
to UEFI has happened over the last ten years in sync with the schedules of our 
industry partners (IBV's, OEM's) and their code bases.  We're not seeing any 
demand for legacy BIOS enablement anymore, so we no longer focus any effort 
there.  Coreboot is the only remaining legacy code base we enable.  Coreboot 
enablement is provided by AMD's embedded group for a market-appropriate subset 
of our chips.

By the way, you may be assuming that the traditional competitiveness between 
companies persists in the UEFI Forum and the spec work groups that it oversees. 
 But there is actually very little of that (especially compared with a lot of 
other industry-standards bodies).  The general attitude within UEFI is that the 
firmware layer should be unified, interoperable, well-specified and secure.  
There is no room for competition or company-specific advantage in the firmware 
layer.  (Then, of course, we all go home to our individual companies and work 
to create competitive advantage at other layers, such as hardware or 
higher-level software.)  I just want to make sure you understand the atmosphere 
of cooperation and common-cause that exists between the various OEM's, Silicon 
Vendors, OS Vendors, IBV's, and others that make up the UEFI Forum.  That 
cooperative atmosphere pervades the UEFI work groups, as well as the UEFI Board 
of Directors.

> For x64/EMT64 systems:
[Gary Simpson] My answers only apply to X64 chips from AMD (not EMT64, which is 
not an AMD thing).

> What models use UEFI, what models use BIOS, what models use coreboot?
[Gary Simpson] We don't specify or control this.  Our customers can implement 
whatever platform firmware solution they choose.  However, the firmware 
components AMD provides focus primarily on UEFI solutions.  As mentioned, our 
embedded group also enables coreboot for a selected subset of our chips.  
Coreboot is the only legacy code base we still target.  For coreboot, we 
maintain wrappers and a centralized function dispatcher, but our core code is 
natively targeted at the various UEFI-style code bases used by our IBV 
partners, our OEM customers, and Tianocore (e.g. EDKII).

> I'm unclear if they're still using BIOS on most or only some of their 
> systems. And coreboot -vs- UEFI usage and future plans.
[Gary Simpson] Internally, we create Customer Reference Boards (CRB's) and 
build platform firmware in-house to support them.  These in-house BIOS's, which 
we use to bring-up and validate our new silicon designs, are all UEFI-based.  
These are almost always based on our AGESA firmware (see below) combined with a 
platform code base from one of the IBV's.  Additionally, AMD's embedded team 
ports coreboot to their versions of the CRB's.

> Are there different goals for UEFI/BIOS/coreboot for consumer desktop/laptop 
> models -vs- server models? I've heard one person speculate that servers are 
> focusing on BIOS, laptops are focusing
> on GPUs/DirectX [and perhaps UEFI].
[Gary Simpson] AMD's goal is simply to provide what our customers want and 
need.  Server manufacturers were, in general, slower to transition from legacy 
to UEFI,  but we are no longer seeing any demand from them for legacy BIOS.

> I'm really unclear how they can get Win8 logos if they're using BIOS. If 
> they're getting logos for those systems. Do AMD systems have less Win8 
> technical restrictions than Intel systems in 
> this regard?
[Gary Simpson] In combination with the BIOS Vendors and/or the OEM's, AMD makes 
UEFI solutions (supporting Secure Boot, etc.) available for all our chips.  We 
qualify for our Win8 and Win10 logo certifications the old fashioned way - by 
passing the tests.  We make sure that all of our CRB's pass the certifications 
tests, and we assist our OEM customers as needed to make sure that their 
production systems pass as well.

> What is AMD equivalent of Intel FSP, for closed-source blobs need alongside 
> Tianocore open source?
[Gary Simpson] Our deliverable is called AGESA (AMD Generic Encapsulated SW 
Architec

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

[Ryan Harkin]

On 31 Jul 2015 21:21, "Cohen, Eugene"  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 
> Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List <
linaro-u...@lists.linaro.org>; Leif Lindholm ;
Ard Biesheuvel 
> Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check
for GICv3
>
>
> > On Jul 31, 2015, at 9:37 AM, Ryan Harkin  wrote:
> >
> > On 31 July 2015 at 17:16, Andrew Fish  wrote:
> >
> >>
> >> On Jul 31, 2015, at 7:49 AM, Ryan Harkin 
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 
> >> Reviewed-by: Ard Biesheuvel 
> >> ---
> >> .../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 
> >> #include 
> >>
> >> -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