Re: [edk2-devel] [PATCH v2] DynamicTablesPkg/SSDT: Require Package node in hierarchy

2024-03-12 Thread Jeshua Smith via groups.io 
Can we get this reviewed/merged?

> -Original Message-
> From: Jeshua Smith 
> Sent: Monday, February 5, 2024 12:01 PM
> To: devel@edk2.groups.io
> Cc: ardb+tianoc...@kernel.org; quic_llind...@quicinc.com;
> pierre.gond...@arm.com; sami.muja...@arm.com; Jeshua Smith
> 
> Subject: [PATCH v2] DynamicTablesPkg/SSDT: Require Package node in
> hierarchy
> 
> The code was incorrectly assuming that root nodes had to be physical package
> nodes and vice versa. This is not always true, so update the check to simply
> require exactly one package node somewhere in the hierarchy.
> 
> Signed-off-by: Jeshua Smith 
> Reviewed-by: Pierre Gondois 
> ---
> Note: This is a complete replacement for [PATCH] DynamicTablesPkg/SSDT:
> Remove incorrect root node check
> 
> Version 2: added documentation for the PackageNodeSeen parameter
> 
>  .../SsdtCpuTopologyGenerator.c| 32 +--
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpu
> TopologyGenerator.c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpu
> TopologyGenerator.c
> index 9e3efb49e6..40ed10eae6 100644
> ---
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpu
> TopologyGenerator.c
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> +++ uTopologyGenerator.c
> @@ -1072,6 +1072,7 @@ CreateAmlProcessorContainer (
>@param [in]  IsLeaf   The ProcNode is a leaf.
>@param [in]  NodeTokenNodeToken of the ProcNode.
>@param [in]  ParentNodeToken  Parent NodeToken of the ProcNode.
> +  @param [in]  PackageNodeSeen  A parent of the ProcNode has the physical
> package flag set.
> 
>@retval EFI_SUCCESS Success.
>@retval EFI_INVALID_PARAMETER   Invalid parameter.
> @@ -1083,23 +1084,24 @@ CheckProcNode (
>UINT32   NodeFlags,
>BOOLEAN  IsLeaf,
>CM_OBJECT_TOKEN  NodeToken,
> -  CM_OBJECT_TOKEN  ParentNodeToken
> +  CM_OBJECT_TOKEN  ParentNodeToken,
> +  BOOLEAN  PackageNodeSeen
>)
>  {
>BOOLEAN  InvalidFlags;
>BOOLEAN  HasPhysicalPackageBit;
> -  BOOLEAN  IsTopLevelNode;
> 
>HasPhysicalPackageBit = (NodeFlags &
> EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL) ==
>EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
> -  IsTopLevelNode = (ParentNodeToken == CM_NULL_TOKEN);
> 
> -  // A top-level node is a Physical Package and conversely.
> -  InvalidFlags = HasPhysicalPackageBit ^ IsTopLevelNode;
> +  // Only one Physical Package flag is allowed in the hierarchy
> + InvalidFlags = HasPhysicalPackageBit && PackageNodeSeen;
> 
>// Check Leaf specific flags.
>if (IsLeaf) {
>  InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
> +// Must have Physical Package flag somewhere in the hierarchy
> +InvalidFlags |= !(HasPhysicalPackageBit || PackageNodeSeen);
>} else {
>  InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);
>}
> @@ -1130,6 +1132,7 @@ CheckProcNode (
>node to.
>@param [in,out] ProcContainerIndex  Pointer to the current processor
> container
>index to be used as UID.
> +  @param [in]  PackageNodeSeenA parent of the ProcNode has the
> physical package flag set.
> 
>@retval EFI_SUCCESS Success.
>@retval EFI_INVALID_PARAMETER   Invalid parameter.
> @@ -1143,7 +1146,8 @@ CreateAmlCpuTopologyTree (
>IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> CfgMgrProtocol,
>INCM_OBJECT_TOKEN   NodeToken,
>INAML_NODE_HANDLE   ParentNode,
> -  IN OUTUINT32*ProcContainerIndex
> +  IN OUTUINT32
> *ProcContainerIndex,
> +  INBOOLEAN   PackageNodeSeen
>)
>  {
>EFI_STATUS  Status;
> @@ -1153,6 +1157,7 @@ CreateAmlCpuTopologyTree (
>AML_OBJECT_NODE_HANDLE  ProcContainerNode;
>UINT32  Uid;
>UINT16  Name;
> +  BOOLEAN HasPhysicalPackageBit;
> 
>ASSERT (Generator != NULL);
>ASSERT (Generator->ProcNodeList != NULL); @@ -1175,7 +1180,8 @@
> CreateAmlCpuTopologyTree (
> Generator->ProcNodeList[Index].Flags,
> TRUE,
> Generator->ProcNodeList[Index].Token,
> -   NodeToken
> +   NodeToken,
> +   PackageNodeSeen
> );
>  if (EFI_ERROR (Status)) {
>ASSERT (0);
> @@ -1208,7 +1214,8 @@ CreateAmlCpuTopologyTree (
> Generator->ProcNodeList[Index].Flags,
> FALSE,
> Generator->ProcNodeList[Index].Token,
> -   NodeToken
> +   NodeToken,

Re: [edk2-devel] [RFC PATCH v1 00/20] DynamicTablesPkg: Prepare to add RISC-V support

2024-01-10 Thread Jeshua Smith via groups.io 
> > It looks like instead of moving the common code to
> EObjNameSpaceStandard namespace or a new (Arch? Common?) namespace,
> you're renaming the entire EObjNameSpaceArm namespace to
> EObjNameSpaceArch. It seems to me that if ARM code vs. common code is
> being separated out, then the EObjNameSpaceArm namespace should
> continue to be used for the ARM-specific code and a common namespace
> should be used for the common code.
> 
> I agree. I started with separating common things into new common space and
> create one for risc-v. However, I dropped that approach for two reasons.
> 
> 1) The commit "b2bbe3df5470 DynamicTablesPkg: Remove PPTT ID structure
> from ACPI 6.4 generator" when removed one of the enums from
> ArmObjectID, didn't change the other values for other enums but reserved the
> removed one. So, I thought there may be some assumptions which will break
> if the enum value changes.

I'm not familiar with why that was done. Hopefully someone else can comment. I 
do know that 
edk2/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
 has arrays (StdNamespaceObjectParser and ArmNamespaceObjectParser) that need 
to be kept in sync with all of the namespace enums, but other than that I'm not 
aware of any places that need to be changes when the enums are changed.

> 2) DynamicPlatformRepositoryInfo structure has ArmCmObjList and
> ArmCmObjArray. With separate spaces for Arm, RiscV and Common, list
> management needs some redesign and I was not sure it is worth it.
> 
> Hence, I thought a single list of all possible Obj Ids for all architectures 
> and
> common things would be a good trade off. But I can go back to that approach
> in v2 if above issues are fine.

Hopefully ARM can give input on the best direction before you make more 
changes. The DynamicPlatRepo currently only supports the ARM namespace, but 
comments such as "only Arm objects are supported for now." (line 144) seem to 
imply that support for more namespaces was considered.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113542): https://edk2.groups.io/g/devel/message/113542
Mute This Topic: https://groups.io/mt/103622702/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v1 00/20] DynamicTablesPkg: Prepare to add RISC-V support

2024-01-09 Thread Jeshua Smith via groups.io 
> From: devel@edk2.groups.io  On Behalf Of Sunil V L
> via groups.io
> Sent: Tuesday, January 9, 2024 9:29 AM
> DynamicTablesPkg can be used by RISC-V platforms to generate ACPI tables
> from FDT passed from previous stage FW. However, DynamicTablesPkg
> currently is ARM specific even though several parsers and ACPI generators can
> be used across architectures. For ex: SSDT (PCIe), SSDT (CPU), MCFG, SPCR,
> DBG2, FADT, SRAT, Raw (DSDT) are mostly common across architectures. Only
> MADT, IORT and GTDT are ARM specific.
> 
> This series tries to refactor the DynamicTablesPkg so that RISC-V support can
> be added fairly easily later.

It looks like instead of moving the common code to EObjNameSpaceStandard 
namespace or a new (Arch? Common?) namespace, you're renaming the entire 
EObjNameSpaceArm namespace to EObjNameSpaceArch. It seems to me that if ARM 
code vs. common code is being separated out, then the EObjNameSpaceArm 
namespace should continue to be used for the ARM-specific code and a common 
namespace should be used for the common code.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113493): https://edk2.groups.io/g/devel/message/113493
Mute This Topic: https://groups.io/mt/103622702/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

2024-01-09 Thread Jeshua Smith via groups.io 
> > Two physical packages are on a multi-chip module and share resources on
> the module. The module then plugs into the baseboard/motherboard.
> 
> Is it possible to elaborate on the resource being shared ?

In our specific case the problem is related to the PPTT's "Identical 
Implementation" flag. We need a top level node above the physical package nodes 
to be able to set the "Identical Implementation" flag to indicate that all of 
the procs in all of the child packages are the same identical implementation. 
Without this (ie. forcing each physical package to be its own root node) Linux 
will fail to load the SPE driver when there are multiple identical packages 
because it detects that some of the procs have a different root node than other 
procs, implying that the packages don't have identical implementations.

> Does it fall into the subject of this thread ? Some resources might be aswell
> described in other ACPI tables.

The thread you linked looks like it is about non-processor resources, and 
therefore wasn't in scope for PPTT. I don't think this is related.

> > Note: While investigating this we noticed that another vendor also has a
> similar PPTT topology to what is being flagged as invalid, so either that 
> vendor
> isn't using EDK2 or they have done something to avoid this check without
> submitting a patch to EDK2.
> 
> This check is only present in the DynamicTablesPkg, so it shouldn't be too
> restrictive.

Ah, correct. If they're not using DynamicTablesPkg to generate the PPTT/SSDT, 
then they wouldn't hit this.

> If the platform is known to use it, is it possible to share which platform it 
> is ?

We see the topology in question (ie. a root node is a parent node to physical 
package nodes) in Ampere's two-socket Altra PPTT.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113485): https://edk2.groups.io/g/devel/message/113485
Mute This Topic: https://groups.io/mt/103603398/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

2024-01-09 Thread Jeshua Smith via groups.io 
> From: Pierre Gondois 
> Sent: Tuesday, January 9, 2024 1:22 AM

> On 1/8/24 19:12, Jeshua Smith wrote:
> > The code was incorrectly assuming that root nodes had to be physical
> > package nodes and vice versa. This is not always true, so the check is
> > being removed.
> 
> Does it mean that you have a topology where the top-level node is not a
> physical package ? If yes, does it also mean that multiple physical packages
> share a resource (which belong to the top-level node) ?

Yes, this change is due to the check incorrectly flagging our topology as 
invalid. Simply removing the check fixed the problem for us.
 
> It is correct that the check is a bit stronger than what the specification 
> states,
> but it was handling all topologies so far, so would it be possible to 
> describe the
> topology that you have ?

Two physical packages are on a multi-chip module and share resources on the 
module. The module then plugs into the baseboard/motherboard. 

Note: While investigating this we noticed that another vendor also has a 
similar PPTT topology to what is being flagged as invalid, so either that 
vendor isn't using EDK2 or they have done something to avoid this check without 
submitting a patch to EDK2.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113455): https://edk2.groups.io/g/devel/message/113455
Mute This Topic: https://groups.io/mt/103603398/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

2024-01-08 Thread Jeshua Smith via groups.io 
The code was incorrectly assuming that root nodes had to be physical
package nodes and vice versa. This is not always true, so the check
is being removed.

Signed-off-by: Jeshua Smith 
Tested-by: Ashish Singhal 
Reviewed-by: Ashish Singhal 
---
 .../SsdtCpuTopologyGenerator.c| 23 ---
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index 724f33c660..4ad9508f57 100644
--- 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -983,7 +983,6 @@ CreateAmlProcessorContainer (
   @param [in]  NodeFlagsFlags of the ProcNode to check.
   @param [in]  IsLeaf   The ProcNode is a leaf.
   @param [in]  NodeTokenNodeToken of the ProcNode.
-  @param [in]  ParentNodeToken  Parent NodeToken of the ProcNode.
 
   @retval EFI_SUCCESS Success.
   @retval EFI_INVALID_PARAMETER   Invalid parameter.
@@ -994,26 +993,16 @@ EFIAPI
 CheckProcNode (
   UINT32   NodeFlags,
   BOOLEAN  IsLeaf,
-  CM_OBJECT_TOKEN  NodeToken,
-  CM_OBJECT_TOKEN  ParentNodeToken
+  CM_OBJECT_TOKEN  NodeToken
   )
 {
   BOOLEAN  InvalidFlags;
-  BOOLEAN  HasPhysicalPackageBit;
-  BOOLEAN  IsTopLevelNode;
-
-  HasPhysicalPackageBit = (NodeFlags & EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL) ==
-  EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
-  IsTopLevelNode = (ParentNodeToken == CM_NULL_TOKEN);
-
-  // A top-level node is a Physical Package and conversely.
-  InvalidFlags = HasPhysicalPackageBit ^ IsTopLevelNode;
 
   // Check Leaf specific flags.
   if (IsLeaf) {
-InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
+InvalidFlags = ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
   } else {
-InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);
+InvalidFlags = ((NodeFlags & PPTT_LEAF_MASK) != 0);
   }
 
   if (InvalidFlags) {
@@ -1086,8 +1075,7 @@ CreateAmlCpuTopologyTree (
 Status = CheckProcNode (
Generator->ProcNodeList[Index].Flags,
TRUE,
-   Generator->ProcNodeList[Index].Token,
-   NodeToken
+   Generator->ProcNodeList[Index].Token
);
 if (EFI_ERROR (Status)) {
   ASSERT (0);
@@ -1119,8 +1107,7 @@ CreateAmlCpuTopologyTree (
 Status = CheckProcNode (
Generator->ProcNodeList[Index].Flags,
FALSE,
-   Generator->ProcNodeList[Index].Token,
-   NodeToken
+   Generator->ProcNodeList[Index].Token
);
 if (EFI_ERROR (Status)) {
   ASSERT (0);
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113407): https://edk2.groups.io/g/devel/message/113407
Mute This Topic: https://groups.io/mt/103603398/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support

2023-10-23 Thread Jeshua Smith via groups.io 
This series has received:
Reviewed-by: Pierre Gondois 

And also For 1-2,12-13:
Reviewed-by: Leif Lindholm 

Can this be merged?

-Original Message-
From: Pierre Gondois 
Sent: Friday, September 22, 2023 8:51 AM
To: Sami Mujawar ; devel@edk2.groups.io
Cc: ardb+tianoc...@kernel.org; quic_llind...@quicinc.com; 
michael.d.kin...@intel.com; gaolim...@byosoft.com.cn; zhiguang@intel.com; 
zhichao@intel.com; anshuman.khand...@arm.com; matteo.carl...@arm.com; 
akanksha.ja...@arm.com; sibel.allin...@arm.com; Jeshua Smith 
; n...@arm.com
Subject: Re: [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE 
support

External email: Use caution opening links or attachments


Hi Sami,
Thanks for the update:
Reviewed-by: Pierre Gondois 

Regards,
Pierre

On 9/22/23 16:35, Sami Mujawar wrote:
> This patch series provides the following updates:
> - The patches 1 & 2 add the new fields introduced
>in MADT (APIC table) by ACPI 6.5 and the patch
>7/11 updates the Acpiview MADT parser accordingly.
> - The patches 3, 4 & 5 adds TRBE support to the MADT
>table generator in DynamicTablesPkg.
> - Patch 6/11 updates the FADT ACPI revision to 6.5.
> - The patches 8, 9 & 10 add support to generate ETE
>device nodes.
> - The 3rd last last patch series fixes a bug wherein
>the CPC token was incorrectly referenced.
> - The last 2 patches in the series introduce helper
>functions to detect if TRBE and ETE features are
>supported.
>
> Updates from v2 patch series:
> - Updated patch 5 to removed superfluous initialisation
>of TRBE interrupt field for ACPI 6.4.
> - Patch 12/13 introduces a helper function in ArmLib to
>detect if TRBE is supported.
> - Patch 13/13 introduces a helper function in ArmLib to
>detect if ETE is supported.
>
> Updates from v1 patch series:
>- Fixed issue with setting TRBE interrupt in patch 5/11.
>
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/2620_ete_dev_fvp_v3
>
> Sami Mujawar (13):
>MdePkg: MADT: Add Online capable flag in GICC
>MdePkg: MADT: Add TRBE interrupt to GICC
>DynamicTablesPkg: Add TRBE interrupt to GICC object
>DynamicTablesPkg: Add TRBE interrupt to GICC object parser
>DynamicTablesPkg: Update MADT generator for ACPI 6.5
>DynamicTablesPkg: Update FADT generator to ACPI 6.5
>ShellPkg: Acpiview: Update MADT parser for TRBE interrupt
>DynamicTablesPkg: Add an ET info object to Arm namespace
>DynamicTablesPkg: Add an ET info object parser
>DynamicTablesPkg: Add ETE device to CPU node in AML
>DynamicTablesPkg: Fix referencing of CPC token
>ArmPkg/ArmLib: Add ArmHasTrbe () helper function
>ArmPkg/ArmLib: Add ArmHasEte () helper function
>
>   ArmPkg/Include/Chipset/AArch64.h
>|   4 +
>   ArmPkg/Include/Library/ArmLib.h 
>|  25 +++
>   ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c  
>|  31 
>   DynamicTablesPkg/Include/ArmNameSpaceObjects.h  
>|  32 +++-
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
>| 108 +--
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
>|  79 
>   
> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
>  | 188 +++-
>   
> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h
>  |  11 +-
>   
> DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
>   |  11 +-
>   MdePkg/Include/IndustryStandard/Acpi65.h
>|   4 +-
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c  
>|  48 -
>   11 files changed, 442 insertions(+), 99 deletions(-)
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109931): https://edk2.groups.io/g/devel/message/109931
Mute This Topic: https://groups.io/mt/101522262/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

2023-10-23 Thread Jeshua Smith via groups.io 
The patches in this series have both individually received:
Reviewed-by: Pierre Gondois 

Can this be merged?

-Original Message-
From: Jeshua Smith  
Sent: Friday, October 6, 2023 10:28 AM
To: devel@edk2.groups.io
Cc: sami.muja...@arm.com; pierre.gond...@arm.com; Jeshua Smith 

Subject: [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

While using the ConfigurationManagerObjectParser to dump objects and debug 
adding new objects, I noticed some bugs and deficiencies. This series is 
intended to address those.

Jeshua Smith (2):
  DynamicTablesPkg/TableHelperLib: Fix and improve text handling
  DynamicTablesPkg/TableHelperLib: Enhance error handling

 .../ConfigurationManagerObjectParser.c| 223 ++
 1 file changed, 176 insertions(+), 47 deletions(-)

--
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109930): https://edk2.groups.io/g/devel/message/109930
Mute This Topic: https://groups.io/mt/101801382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] DynamicTablesPkg/AmlLib: Enumerate memory attributes

2023-10-23 Thread Jeshua Smith via groups.io 
Can this be merged?

-Original Message-
From: Pierre Gondois 
Sent: Tuesday, October 10, 2023 1:31 AM
To: Jeshua Smith ; devel@edk2.groups.io
Cc: sami.muja...@arm.com; quic_llind...@quicinc.com; ardb+tianoc...@kernel.org
Subject: Re: [PATCH v2] DynamicTablesPkg/AmlLib: Enumerate memory attributes

External email: Use caution opening links or attachments


Hi Jeshua,
Thanks for the v2,

Reviewed-by: Pierre Gondois 

Sami:
There was also a tag from Leif:
https://edk2.groups.io/g/devel/message/109285

Regards,
Pierre

On 10/5/23 18:38, Jeshua Smith wrote:
> AmlCodeGenRdQWordMemory's and AmlCodeGenRdDWordMemory's Cacheable and
> MemoryRangeType parameters treat specific values as having specific
> meanings as defined by the spec. This change adds enums to map those
> meanings to their corresponding values.
>
> Signed-off-by: Jeshua Smith 
> ---
>
> Notes:
>   v2: based on comments from Pierre Gondois
>- Added documentation reference
>- Changed enum type and member names to closer align with documentation
>- Changed enum member names to CamelCase
>- Added *Max members to enums
>- Updated the signatures of relevant functions to use the enum types
>  instead of UNIT8
>
>   .../Include/Library/AmlLib/AmlLib.h   | 49 +--
>   .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c| 12 ++---
>   .../AmlLib/CodeGen/AmlResourceDataCodeGen.c   |  8 +--
>   3 files changed, 55 insertions(+), 14 deletions(-)
>
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> index 510c79a399..71e8539b30 100644
> --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> @@ -59,6 +59,47 @@ typedef void *AML_DATA_NODE_HANDLE;
>
>   #endif // AML_HANDLE
>
> +/** Memory attributes, _MEM (2 bits)
> +
> +  Possible values are:
> +0-The memory is non-cacheable
> +1-The memory is cacheable (DEPRECATED)
> +2-The memory is cacheable and supports
> +  write combining (DEPRECATED)
> +3-The memory is cacheable and prefetchable
> +
> +  @par Reference(s):
> +  - ACPI 6.5, s6.4.3.5.5 "Resource Type Specific Flags"
> +
> +**/
> +typedef enum {
> +  AmlMemoryNonCacheable  = 0,
> +  AmlMemoryCacheable = 1,
> +  AmlMemoryCacheableWriteCombine = 2,
> +  AmlMemoryCacheablePrefetch = 3,
> +  AmlMemoryCacheablityMax= 4
> +} AML_MEMORY_ATTRIBUTES_MEM;
> +
> +/** Memory attributes, _MTP (2 bits)
> +
> +  Possible values are:
> +0-AddressRangeMemory
> +1-AddressRangeReserved
> +2-AddressRangeACPI
> +3-AddressRangeNVS
> +
> +  @par Reference(s):
> +  - ACPI 6.5, s6.4.3.5.5 "Resource Type Specific Flags"
> +
> +**/
> +typedef enum {
> +  AmlAddressRangeMemory   = 0,
> +  AmlAddressRangeReserved = 1,
> +  AmlAddressRangeACPI = 2,
> +  AmlAddressRangeNVS  = 3,
> +  AmlAddressRangeMax  = 4
> +} AML_MEMORY_ATTRIBUTES_MTP;
> +
>   /** Parse the definition block.
>
> The function parses the whole AML blob. It starts with the ACPI
> DSDT/SSDT @@ -578,7 +619,7 @@ AmlCodeGenRdDWordMemory (
> INBOOLEAN IsPosDecode,
> INBOOLEAN IsMinFixed,
> INBOOLEAN IsMaxFixed,
> -  INUINT8 Cacheable,
> +  INAML_MEMORY_ATTRIBUTES_MEM Cacheable,
> INBOOLEAN IsReadWrite,
> INUINT32 AddressGranularity,
> INUINT32 AddressMinimum,
> @@ -587,7 +628,7 @@ AmlCodeGenRdDWordMemory (
> INUINT32 RangeLength,
> INUINT8 ResourceSourceIndex,
> IN  CONST CHAR8 *ResourceSource,
> -  INUINT8 MemoryRangeType,
> +  INAML_MEMORY_ATTRIBUTES_MTP MemoryRangeType,
> INBOOLEAN IsTypeStatic,
> INAML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL
> OUT   AML_DATA_NODE_HANDLE*NewRdNode  OPTIONAL
> @@ -809,7 +850,7 @@ AmlCodeGenRdQWordMemory (
> INBOOLEAN IsPosDecode,
> INBOOLEAN IsMinFixed,
> INBOOLEAN IsMaxFixed,
> -  INUINT8 Cacheable,
> +  INAML_MEMORY_ATTRIBUTES_MEM Cacheable,
> INBOOLEAN IsReadWrite,
> INUINT64 AddressGranularity,
> INUINT64 AddressMinimum,
> @@ -818,7 +859,7 @@ AmlCodeGenRdQWordMemory (
> INUINT64 RangeLength,
> INUINT8 ResourceSourceIndex,
> IN  CONST CHAR8 *ResourceSource,
> -  INUINT8 MemoryRangeType,
> +  INAML_MEMORY_ATTRIBUTES_MTP MemoryRangeType,
> INBOOLEAN IsTypeStatic,
> INAML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL
> OUT   AML_DATA_NODE_HANDLE*NewRdNode  OPTIONAL
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerat
> or.c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerat
> or.c
> index 9ddaddc198..72873709aa 100644
> ---
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerat
> or.c
> +++

Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

2023-10-10 Thread Jeshua Smith via groups.io 
#3 is not currently used by any published code. It is a development aid, which 
the ObjectParser itself seems to be.

Here's why I added it. Several people on our team have (not yet upstreamed) 
changes that resulted in additional ObjectIDs being added to the ObjectID 
enums, but without corresponding parsers being added to the ObjectParser. 
Dumping of the objects with the ObjectParser wasn't enabled by default, so this 
wasn't detected by them. Without #1 the ObjectParser code does out of bounds 
array accesses, sometimes leading to crashes. #1 will now detect and report 
that problem. When I enabled dumping of objects to debug my new code, I hit 
this bug leading me to write #1. For me to work around the issue of missing 
parsers in order to be able to continue debug with my work, the "easy" solution 
was for me to temporarily add the new ObjectIDs to the parser list with NULL 
parsers and then inform the responsible parties that they need to go and add 
parsers for their new ObjectIDs. Doing this required #3 (support for NULL 
parsers) to be added. Ideally any code that is upstreamed back to EDKII will 
have non-NULL parsers at the point it is sent upstream, but allowing 
in-development changes to temporarily use NULL parsers is helpful.

Hopefully that clarifies things.

-Original Message-
From: Pierre Gondois  
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith ; devel@edk2.groups.io
Cc: sami.muja...@arm.com
Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

External email: Use caution opening links or attachments


Hello Jeshua,

On 10/6/23 18:28, Jeshua Smith wrote:
> This patch enhances error handling and reporting in the CM ObjectParser.
> Specifically:
> 1. ObjectIDs used as array indexes are checked for being out of bounds,
> and if so an error message is printed before the assert.
> 2. An error message is printed for unsupported NameSpaceIDs.
> 3. Adds support for unimplemented parsers by allowing IDs to list a
> NULL parser, resulting in an unimplemented message being printed.

I am not sure I see in which context 3. would be used/necessary. Is it possible 
to detail ?

(Code-wise everything looks good to me)

Regards,
Pierre


>
> Signed-off-by: Jeshua Smith 
> ---
>   .../ConfigurationManagerObjectParser.c| 47 +--
>   1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c 
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> index 92df1efee8..22b8fdb906 100644
> --- 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
> +++ gerObjectParser.c
> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  
> StdNamespaceObjectParser[] = {
>   ARRAY_SIZE (StdObjAcpiTableInfoParser) },
> { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>   ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> +  { "EStdObjMax", NULL,   0}
>   };
>
>   /** Print string data.
> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>   return;
> }
>
> +  if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
> +DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the 
> StdNamespaceObjectParser array\n", ObjId));
> +ASSERT (0);
> +return;
> +  }
> +
> ParserArray = [ObjId];
> break;
>   case EObjNameSpaceArm:
> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>   return;
> }
>
> +  if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
> +DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the 
> ArmNamespaceObjectParser array\n", ObjId));
> +ASSERT (0);
> +return;
> +  }
> +
> ParserArray = [ObjId];
> break;
>   default:
> // Not supported
> +  DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not 
> + supported by the parser\n", NameSpaceId, ObjId));
> ASSERT (0);
> return;
> } // switch
> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
> ObjIndex + 1,
> ObjectCount
> ));
> -PrintCmObjDesc (
> -  (VOID *)((UINTN)CmObjDesc->Data + Offset),
> -  ParserArray->Parser,
> -  ParserArray->ItemCount,
> -  ,
> -  1
> -  );
> -if ((RemainingSize > CmObjDesc->Size) ||
> -(RemainingSize < 0))
> -{
> -  ASSERT (0);
> -  return;
> -}
> +if (ParserArray->Parser == NULL) {
> +  DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
> +  RemainingSize = 0;
> +} else {
> +  PrintCmObjDesc (
> +(VOID *)((UINTN)CmObjDesc->Data + Offset),
> +ParserArray->Parser,
> +ParserArray->ItemCount,
> +,
> +1
> +);
> +  if ((RemainingSize > CmObjDesc->Size) ||
> +  (RemainingSize < 0))
> +  {
> +

Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling

2023-10-10 Thread Jeshua Smith via groups.io 
Thanks for the review. Reply inline.

-Original Message-
From: Pierre Gondois  
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith ; devel@edk2.groups.io
Cc: sami.muja...@arm.com
Subject: Re: [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text 
handling

External email: Use caution opening links or attachments


Hello Jeshua,
Just a small remark, but this should be equivalent, so:

Reviewed-by: Pierre Gondois 

On 10/6/23 18:28, Jeshua Smith wrote:
> This fixes two bugs and adds some enhancements to the handling of 
> characters and strings in objects being printed by the CM ObjectParser.
>
> Bug fixes:
> 1. PrintOemID() currently attempts to print characters with "%C",
> but the correct syntax is (lowercase) "%c". This bug results in
> "CC" being printed instead of the actual ASCII characters.
> 2. PrintString() is being passed a pointer to data in objects, but in
> some cases this data is the actual string to print and other cases
> it is a pointer to the string to print. This adds a PrintStringPtr
> function and uses the correct functions depending on the situation.
>
> Enhancements:
> 1. Some objects contain ASCII characters, which are currently printed
> as their hex values. This adds functions to print out ASCII
> character fields as text rather than hex, and uses those functions in
> several cases where the object data is defined to be ASCII.
> 2. The PrintOemID() function is replaced with the new identical but more
> generecically-named PrintChar6() function.
>
> Signed-off-by: Jeshua Smith 
> ---
>   .../ConfigurationManagerObjectParser.c| 176 ++
>   1 file changed, 143 insertions(+), 33 deletions(-)
>
> diff --git 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c 
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> index 99d6032510..92df1efee8 100644
> --- 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
> +++ gerObjectParser.c
> @@ -14,7 +14,7 @@
>   STATIC
>   VOID
>   EFIAPI
> -PrintOemId (
> +PrintString (
> CONST CHAR8  *Format,
> UINT8*Ptr
> );
> @@ -22,7 +22,31 @@ PrintOemId (
>   STATIC
>   VOID
>   EFIAPI
> -PrintString (
> +PrintStringPtr (
> +  CONST CHAR8  *Format,
> +  UINT8*Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar4 (
> +  CONST CHAR8  *Format,
> +  UINT8*Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> +  CONST CHAR8  *Format,
> +  UINT8*Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar8 (
> CONST CHAR8  *Format,
> UINT8*Ptr
> );
> @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER  CmArmItsGroupNodeParser[] = 
> {
>   /** A parser for EArmObjNamedComponent.
>   */
>   STATIC CONST CM_OBJ_PARSER  CmArmNamedComponentNodeParser[] = {
> -  { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},
> -  { "IdMappingCount",4,"0x%x", NULL},
> -  { "IdMappingToken",sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},
> -  { "Flags", 4,"0x%x", NULL},
> -  { "CacheCoherent", 4,"0x%x", NULL},
> -  { "AllocationHints",   1,"0x%x", NULL},
> -  { "MemoryAccessFlags", 1,"0x%x", NULL},
> -  { "AddressSizeLimit",  1,"0x%x", NULL},
> -  { "ObjectName",sizeof (CHAR8 *), NULL,   PrintString },
> -  { "Identifier",4,"0x%x", NULL},
> +  { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL   },
> +  { "IdMappingCount",4,"0x%x", NULL   },
> +  { "IdMappingToken",sizeof (CM_OBJECT_TOKEN), "0x%p", NULL   },
> +  { "Flags", 4,"0x%x", NULL   },
> +  { "CacheCoherent", 4,"0x%x", NULL   },
> +  { "AllocationHints",   1,"0x%x", NULL   },
> +  { "MemoryAccessFlags", 1,"0x%x", NULL   },
> +  { "AddressSizeLimit",  1,"0x%x", NULL   },
> +  { "ObjectName",sizeof (CHAR8 *), NULL,   PrintStringPtr },
> +  { "Identifier",4,"0x%x", NULL   },
>   };
>
>   /** A parser for EArmObjRootComplex.
> @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  
> ArmNamespaceObjectParser[] = {
>   */
>   STATIC CONST CM_OBJ_PARSER  StdObjCfgMgrInfoParser[] = {
> { "Revision", 4, "0x%x", NULL   },
> -  { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId }
> +  { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 }

NIT:
Is it necessary, since "%c%c%c%c%c%c" is the default format of

[edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling

2023-10-06 Thread Jeshua Smith via groups.io 
This fixes two bugs and adds some enhancements to the handling of
characters and strings in objects being printed by the CM ObjectParser.

Bug fixes:
1. PrintOemID() currently attempts to print characters with "%C",
   but the correct syntax is (lowercase) "%c". This bug results in
   "CC" being printed instead of the actual ASCII characters.
2. PrintString() is being passed a pointer to data in objects, but in
   some cases this data is the actual string to print and other cases
   it is a pointer to the string to print. This adds a PrintStringPtr
   function and uses the correct functions depending on the situation.

Enhancements:
1. Some objects contain ASCII characters, which are currently printed
   as their hex values. This adds functions to print out ASCII
   character fields as text rather than hex, and uses those functions in
   several cases where the object data is defined to be ASCII.
2. The PrintOemID() function is replaced with the new identical but more
   generecically-named PrintChar6() function.

Signed-off-by: Jeshua Smith 
---
 .../ConfigurationManagerObjectParser.c| 176 ++
 1 file changed, 143 insertions(+), 33 deletions(-)

diff --git 
a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
 
b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 99d6032510..92df1efee8 100644
--- 
a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ 
b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -14,7 +14,7 @@
 STATIC
 VOID
 EFIAPI
-PrintOemId (
+PrintString (
   CONST CHAR8  *Format,
   UINT8*Ptr
   );
@@ -22,7 +22,31 @@ PrintOemId (
 STATIC
 VOID
 EFIAPI
-PrintString (
+PrintStringPtr (
+  CONST CHAR8  *Format,
+  UINT8*Ptr
+  );
+
+STATIC
+VOID
+EFIAPI
+PrintChar4 (
+  CONST CHAR8  *Format,
+  UINT8*Ptr
+  );
+
+STATIC
+VOID
+EFIAPI
+PrintChar6 (
+  CONST CHAR8  *Format,
+  UINT8*Ptr
+  );
+
+STATIC
+VOID
+EFIAPI
+PrintChar8 (
   CONST CHAR8  *Format,
   UINT8*Ptr
   );
@@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER  CmArmItsGroupNodeParser[] = {
 /** A parser for EArmObjNamedComponent.
 */
 STATIC CONST CM_OBJ_PARSER  CmArmNamedComponentNodeParser[] = {
-  { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},
-  { "IdMappingCount",4,"0x%x", NULL},
-  { "IdMappingToken",sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},
-  { "Flags", 4,"0x%x", NULL},
-  { "CacheCoherent", 4,"0x%x", NULL},
-  { "AllocationHints",   1,"0x%x", NULL},
-  { "MemoryAccessFlags", 1,"0x%x", NULL},
-  { "AddressSizeLimit",  1,"0x%x", NULL},
-  { "ObjectName",sizeof (CHAR8 *), NULL,   PrintString },
-  { "Identifier",4,"0x%x", NULL},
+  { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL   },
+  { "IdMappingCount",4,"0x%x", NULL   },
+  { "IdMappingToken",sizeof (CM_OBJECT_TOKEN), "0x%p", NULL   },
+  { "Flags", 4,"0x%x", NULL   },
+  { "CacheCoherent", 4,"0x%x", NULL   },
+  { "AllocationHints",   1,"0x%x", NULL   },
+  { "MemoryAccessFlags", 1,"0x%x", NULL   },
+  { "AddressSizeLimit",  1,"0x%x", NULL   },
+  { "ObjectName",sizeof (CHAR8 *), NULL,   PrintStringPtr },
+  { "Identifier",4,"0x%x", NULL   },
 };
 
 /** A parser for EArmObjRootComplex.
@@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  
ArmNamespaceObjectParser[] = {
 */
 STATIC CONST CM_OBJ_PARSER  StdObjCfgMgrInfoParser[] = {
   { "Revision", 4, "0x%x", NULL   },
-  { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId }
+  { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 }
 };
 
 /** A parser for EStdObjAcpiTableList.
 */
 STATIC CONST CM_OBJ_PARSER  StdObjAcpiTableInfoParser[] = {
-  { "AcpiTableSignature", 4,  "0x%x",   
NULL },
-  { "AcpiTableRevision",  1,  "%d", 
NULL },
-  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),   "0x%x",   
NULL },
-  { "AcpiTableData",  sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",   
NULL },
-  { "OemTableId", 8,  "0x%LLX", 
NULL },
-  { "OemRevision",4,  "0x%x",   
NULL },
-  { "MinorRevision",  1,  "0x%x",   
NULL },
+  { "AcpiTableSignature", 4,  "%c%c%c%c",  
   PrintChar4 },
+

[edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

2023-10-06 Thread Jeshua Smith via groups.io 
This patch enhances error handling and reporting in the CM ObjectParser.
Specifically:
1. ObjectIDs used as array indexes are checked for being out of bounds,
   and if so an error message is printed before the assert.
2. An error message is printed for unsupported NameSpaceIDs.
3. Adds support for unimplemented parsers by allowing IDs to list a
   NULL parser, resulting in an unimplemented message being printed.

Signed-off-by: Jeshua Smith 
---
 .../ConfigurationManagerObjectParser.c| 47 +--
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git 
a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
 
b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 92df1efee8..22b8fdb906 100644
--- 
a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ 
b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  
StdNamespaceObjectParser[] = {
 ARRAY_SIZE (StdObjAcpiTableInfoParser) },
   { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
 ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
+  { "EStdObjMax", NULL,   0}
 };
 
 /** Print string data.
@@ -1066,6 +1067,12 @@ ParseCmObjDesc (
 return;
   }
 
+  if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
+DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the 
StdNamespaceObjectParser array\n", ObjId));
+ASSERT (0);
+return;
+  }
+
   ParserArray = [ObjId];
   break;
 case EObjNameSpaceArm:
@@ -1074,10 +1081,17 @@ ParseCmObjDesc (
 return;
   }
 
+  if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
+DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the 
ArmNamespaceObjectParser array\n", ObjId));
+ASSERT (0);
+return;
+  }
+
   ParserArray = [ObjId];
   break;
 default:
   // Not supported
+  DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by 
the parser\n", NameSpaceId, ObjId));
   ASSERT (0);
   return;
   } // switch
@@ -1095,21 +1109,26 @@ ParseCmObjDesc (
   ObjIndex + 1,
   ObjectCount
   ));
-PrintCmObjDesc (
-  (VOID *)((UINTN)CmObjDesc->Data + Offset),
-  ParserArray->Parser,
-  ParserArray->ItemCount,
-  ,
-  1
-  );
-if ((RemainingSize > CmObjDesc->Size) ||
-(RemainingSize < 0))
-{
-  ASSERT (0);
-  return;
-}
+if (ParserArray->Parser == NULL) {
+  DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
+  RemainingSize = 0;
+} else {
+  PrintCmObjDesc (
+(VOID *)((UINTN)CmObjDesc->Data + Offset),
+ParserArray->Parser,
+ParserArray->ItemCount,
+,
+1
+);
+  if ((RemainingSize > CmObjDesc->Size) ||
+  (RemainingSize < 0))
+  {
+ASSERT (0);
+return;
+  }
 
-Offset = CmObjDesc->Size - RemainingSize;
+  Offset = CmObjDesc->Size - RemainingSize;
+}
   } // for
 
   ASSERT (RemainingSize == 0);
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109364): https://edk2.groups.io/g/devel/message/109364
Mute This Topic: https://groups.io/mt/101801385/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

2023-10-06 Thread Jeshua Smith via groups.io 
While using the ConfigurationManagerObjectParser to dump objects and debug
adding new objects, I noticed some bugs and deficiencies. This series is
intended to address those.

Jeshua Smith (2):
  DynamicTablesPkg/TableHelperLib: Fix and improve text handling
  DynamicTablesPkg/TableHelperLib: Enhance error handling

 .../ConfigurationManagerObjectParser.c| 223 ++
 1 file changed, 176 insertions(+), 47 deletions(-)

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109363): https://edk2.groups.io/g/devel/message/109363
Mute This Topic: https://groups.io/mt/101801382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2] DynamicTablesPkg/AmlLib: Enumerate memory attributes

2023-10-05 Thread Jeshua Smith via groups.io 
AmlCodeGenRdQWordMemory's and AmlCodeGenRdDWordMemory's Cacheable
and MemoryRangeType parameters treat specific values as having
specific meanings as defined by the spec. This change adds enums to map
those meanings to their corresponding values.

Signed-off-by: Jeshua Smith 
---

Notes:
 v2: based on comments from Pierre Gondois
  - Added documentation reference
  - Changed enum type and member names to closer align with documentation
  - Changed enum member names to CamelCase
  - Added *Max members to enums
  - Updated the signatures of relevant functions to use the enum types
instead of UNIT8

 .../Include/Library/AmlLib/AmlLib.h   | 49 +--
 .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c| 12 ++---
 .../AmlLib/CodeGen/AmlResourceDataCodeGen.c   |  8 +--
 3 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h 
b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
index 510c79a399..71e8539b30 100644
--- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
+++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
@@ -59,6 +59,47 @@ typedef void *AML_DATA_NODE_HANDLE;
 
 #endif // AML_HANDLE
 
+/** Memory attributes, _MEM (2 bits)
+
+  Possible values are:
+0-The memory is non-cacheable
+1-The memory is cacheable (DEPRECATED)
+2-The memory is cacheable and supports
+  write combining (DEPRECATED)
+3-The memory is cacheable and prefetchable
+
+  @par Reference(s):
+  - ACPI 6.5, s6.4.3.5.5 "Resource Type Specific Flags"
+
+**/
+typedef enum {
+  AmlMemoryNonCacheable  = 0,
+  AmlMemoryCacheable = 1,
+  AmlMemoryCacheableWriteCombine = 2,
+  AmlMemoryCacheablePrefetch = 3,
+  AmlMemoryCacheablityMax= 4
+} AML_MEMORY_ATTRIBUTES_MEM;
+
+/** Memory attributes, _MTP (2 bits)
+
+  Possible values are:
+0-AddressRangeMemory
+1-AddressRangeReserved
+2-AddressRangeACPI
+3-AddressRangeNVS
+
+  @par Reference(s):
+  - ACPI 6.5, s6.4.3.5.5 "Resource Type Specific Flags"
+
+**/
+typedef enum {
+  AmlAddressRangeMemory   = 0,
+  AmlAddressRangeReserved = 1,
+  AmlAddressRangeACPI = 2,
+  AmlAddressRangeNVS  = 3,
+  AmlAddressRangeMax  = 4
+} AML_MEMORY_ATTRIBUTES_MTP;
+
 /** Parse the definition block.
 
   The function parses the whole AML blob. It starts with the ACPI DSDT/SSDT
@@ -578,7 +619,7 @@ AmlCodeGenRdDWordMemory (
   INBOOLEAN IsPosDecode,
   INBOOLEAN IsMinFixed,
   INBOOLEAN IsMaxFixed,
-  INUINT8 Cacheable,
+  INAML_MEMORY_ATTRIBUTES_MEM Cacheable,
   INBOOLEAN IsReadWrite,
   INUINT32 AddressGranularity,
   INUINT32 AddressMinimum,
@@ -587,7 +628,7 @@ AmlCodeGenRdDWordMemory (
   INUINT32 RangeLength,
   INUINT8 ResourceSourceIndex,
   IN  CONST CHAR8 *ResourceSource,
-  INUINT8 MemoryRangeType,
+  INAML_MEMORY_ATTRIBUTES_MTP MemoryRangeType,
   INBOOLEAN IsTypeStatic,
   INAML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL
   OUT   AML_DATA_NODE_HANDLE*NewRdNode  OPTIONAL
@@ -809,7 +850,7 @@ AmlCodeGenRdQWordMemory (
   INBOOLEAN IsPosDecode,
   INBOOLEAN IsMinFixed,
   INBOOLEAN IsMaxFixed,
-  INUINT8 Cacheable,
+  INAML_MEMORY_ATTRIBUTES_MEM Cacheable,
   INBOOLEAN IsReadWrite,
   INUINT64 AddressGranularity,
   INUINT64 AddressMinimum,
@@ -818,7 +859,7 @@ AmlCodeGenRdQWordMemory (
   INUINT64 RangeLength,
   INUINT8 ResourceSourceIndex,
   IN  CONST CHAR8 *ResourceSource,
-  INUINT8 MemoryRangeType,
+  INAML_MEMORY_ATTRIBUTES_MTP MemoryRangeType,
   INBOOLEAN IsTypeStatic,
   INAML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL
   OUT   AML_DATA_NODE_HANDLE*NewRdNode  OPTIONAL
diff --git 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index 9ddaddc198..72873709aa 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -566,7 +566,7 @@ GeneratePciCrs (
IsPosDecode,
TRUE,
TRUE,
-   TRUE,
+   AmlMemoryCacheable,
TRUE,
0,
AddrMapInfo->PciAddress,
@@ -575,7 +575,7 @@ GeneratePciCrs (
AddrMapInfo->AddressSize,
0,
NULL,
-   0,
+   AmlAddressRangeMemory,
TRUE,
CrsNode,
NULL
@@ -588,7 +588,7 @@ GeneratePciCrs (
IsPosDecode,
TRUE,
TRUE,
-   TRUE,
+   AmlMemoryCacheable,
TRUE,
0,

[edk2-devel] [PATCH] DynamicTablesPkg/AmlLib: Enumerate memory cacheability and type

2023-10-02 Thread Jeshua Smith via groups.io 
AmlCodeGenRdQWordMemory's and AmlCodeGenRdDWordMemory's Cacheable
and MemoryRangeType parameters treat specific values as having
specific meanings. This change adds enums to map those meanings to their
corresponding values.

Signed-off-by: Jeshua Smith 
---
 .../Include/Library/AmlLib/AmlLib.h   | 33 +++
 .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c| 12 +++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h 
b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
index 510c79a399..6a273059fb 100644
--- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
+++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
@@ -59,6 +59,39 @@ typedef void *AML_DATA_NODE_HANDLE;
 
 #endif // AML_HANDLE
 
+/** Cacheable parameter values
+
+  Possible values are:
+0-The memory is non-cacheable
+1-The memory is cacheable
+2-The memory is cacheable and supports
+  write combining
+3-The memory is cacheable and prefetchable
+
+**/
+typedef enum {
+  AML_MEMORY_NONCACHEABLE = 0,
+  AML_MEMORY_CACHEABLE= 1,
+  AML_MEMORY_CACHEABLE_WC = 2,
+  AML_MEMORY_CACHEABLE_PF = 3
+} AML_MEMORY_CACHEABILITY;
+
+/** MemoryRangeType parameter values
+
+  Possible values are:
+0-AddressRangeMemory
+1-AddressRangeReserved
+2-AddressRangeACPI
+3-AddressRangeNVS
+
+**/
+typedef enum {
+  AML_MEMORY_RANGE_TYPE_MEMORY   = 0,
+  AML_MEMORY_RANGE_TYPE_RESERVED = 1,
+  AML_MEMORY_RANGE_TYPE_ACPI = 2,
+  AML_MEMORY_RANGE_TYPE_NVS  = 3
+} AML_MEMORY_RANGE_TYPE;
+
 /** Parse the definition block.
 
   The function parses the whole AML blob. It starts with the ACPI DSDT/SSDT
diff --git 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index 9ddaddc198..7df7117352 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -566,7 +566,7 @@ GeneratePciCrs (
IsPosDecode,
TRUE,
TRUE,
-   TRUE,
+   AML_MEMORY_CACHEABLE,
TRUE,
0,
AddrMapInfo->PciAddress,
@@ -575,7 +575,7 @@ GeneratePciCrs (
AddrMapInfo->AddressSize,
0,
NULL,
-   0,
+   AML_MEMORY_RANGE_TYPE_MEMORY,
TRUE,
CrsNode,
NULL
@@ -588,7 +588,7 @@ GeneratePciCrs (
IsPosDecode,
TRUE,
TRUE,
-   TRUE,
+   AML_MEMORY_CACHEABLE,
TRUE,
0,
AddrMapInfo->PciAddress,
@@ -597,7 +597,7 @@ GeneratePciCrs (
AddrMapInfo->AddressSize,
0,
NULL,
-   0,
+   AML_MEMORY_RANGE_TYPE_MEMORY,
TRUE,
CrsNode,
NULL
@@ -718,7 +718,7 @@ ReserveEcamSpace (
  TRUE,
  TRUE,
  TRUE,
- FALSE,  // non-cacheable
+ AML_MEMORY_NONCACHEABLE,
  TRUE,
  0,
  AddressMinimum,
@@ -727,7 +727,7 @@ ReserveEcamSpace (
  AddressMaximum - AddressMinimum + 1,
  0,
  NULL,
- 0,
+ AML_MEMORY_RANGE_TYPE_MEMORY,
  TRUE,
  CrsNode,
  NULL
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109265): https://edk2.groups.io/g/devel/message/109265
Mute This Topic: https://groups.io/mt/101722936/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 10/11] DynamicTablesPkg: Add ETE device to CPU node in AML

2023-09-13 Thread Jeshua Smith via groups.io 
Not sure it's worth creating a new patchset for, but the "GetEArmObjEtInfo 
(OPTIONAL)" comment should probably not have the "Get" prefix (ie. " 
EArmObjEtInfo (OPTIONAL)").

-Original Message-
From: Sami Mujawar  
Sent: Wednesday, September 13, 2023 6:50 AM
To: devel@edk2.groups.io
Cc: Sami Mujawar ; pierre.gond...@arm.com; 
anshuman.khand...@arm.com; matteo.carl...@arm.com; akanksha.ja...@arm.com; 
sibel.allin...@arm.com; Jeshua Smith ; n...@arm.com
Subject: [PATCH v2 10/11] DynamicTablesPkg: Add ETE device to CPU node in AML

External email: Use caution opening links or attachments


The Coresight Embedded Trace Extension (ETE) feature can be detected by the 
platform firmware by examining the debug feature register 
ID_AA64DFR0_EL1.TraceVer field.
The platform configuration manager can then describe the ETE by creating 
CM_ARM_ET_INFO object(s) and referencing these in CM_ARM_GICC_INFO.EtToken.

The 'Table 3: Compatible IDs for architected CoreSight components' in the 'ACPI 
for CoreSight
1.2 Platform Design Document' specifies the HID value for Coresight ETE and 
CoreSight Embedded Trace Macrocell (ETM) v4.x as ARMH C500.

Therefore, update the SsdtCpuTopologyGenerator to add an ETE device to the CPU 
node in the AML CPU hierarchy so that an OS can utilise this information.

Note: Although ETE and ETM share the same HID, ETE has a system register 
interfaces, unlike ETM which requires memory mapped registers.
Since this patch aims to support ETE, the AML description does not describe any 
memory mapped registers. However, support for ETM can be added in the future.

Signed-off-by: Sami Mujawar 
---

Notes:
v2:
 - No code change from v1 patch series.  [SAMI]

 
DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
 | 186 +++-  
DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h
 |  11 +-
 2 files changed, 195 insertions(+), 2 deletions(-)

diff --git 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index 
6fb131b664820adca63c9efa6d8b0e17fc64284e..6fbba12a010bf987797f0901a032735e8e0be598
 100644
--- 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
+++ uTopologyGenerator.c
@@ -1,11 +1,17 @@
 /** @file
   SSDT Cpu Topology Table Generator.

-  Copyright (c) 2021, Arm Limited. All rights reserved.
+  Copyright (c) 2021 - 2023, Arm Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent

   @par Reference(s):
 - ACPI 6.3 Specification - January 2019 - s8.4 Declaring Processors
+- ACPI for CoreSight version 1.2 Platform Design Document
+  
+ (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev
+ eloper.arm.com%2Fdocumentation%2Fden0067%2Fa%2F%3Flang%3Den=05%7C
+ 01%7Cjeshuas%40nvidia.com%7C26e54e899c78479dfa9708dbb457f3cd%7C43083d1
+ 5727340c1b7db39efd9ccc17a%7C0%7C0%7C638302062091232606%7CUnknown%7CTWF
+ pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
+ n0%3D%7C3000%7C%7C%7C=PV447Rl4K7EXTgSc9g%2BcjTzXYmMsKs0QMkpXRnwx
+ KkI%3D=0)
+
+  @par Glossary:
+- ETE - Embedded Trace Extension.
+- ETM - Embedded Trace Macrocell.
 **/

 #include 
@@ -35,6 +41,7 @@ Requirements:
   - EArmObjProcHierarchyInfo (OPTIONAL) along with
   - EArmObjCmRef (OPTIONAL)
   - EArmObjLpiInfo (OPTIONAL)
+  - GetEArmObjEtInfo (OPTIONAL)
 */

 /** This macro expands to a function that retrieves the GIC @@ -86,6 +93,16 @@ 
GET_OBJECT_LIST (
   CM_ARM_CPC_INFO
   );

+/**
+  This macro expands to a function that retrieves the ET device
+  information from the Configuration Manager.
+*/
+GET_OBJECT_LIST (
+  EObjNameSpaceArm,
+  EArmObjEtInfo,
+  CM_ARM_ET_INFO
+  );
+
 /** Initialize the TokenTable.

   One entry should be allocated for each CM_ARM_PROC_HIERARCHY_INFO @@ -326,6 
+343,144 @@ CreateAmlCpcNode (
   return Status;
 }

+/** Create an embedded trace device and add it to the Cpu Node in the
+AML namespace.
+
+  This generates the following ASL code:
+  Device (E002)
+  {
+  Name (_UID, 2)
+  Name (_HID, "ARMHC500")
+  }
+
+  Note: Currently we only support generating ETE nodes. Unlike ETM,  
+ ETE has a system register interface and therefore does not need  the 
+ MMIO range to be described.
+
+  @param [in]  GeneratorThe SSDT Cpu Topology generator.
+  @param [in]  ParentNode   Parent node to attach the Cpu node to.
+  @param [in]  CpuName  Value used to generate the node name.
+  @param [out] EtNodePtr   If not NULL, return the created Cpu node.
+
+  @retval EFI_SUCCESS Success.
+  @retval EFI_INVALID_PARAMETER   Invalid parameter.
+  @retval EFI_OUT_OF_RESOURCESFailed to allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+CreateAmlEtd (
+  IN

Re: [edk2-devel] [PATCH v1 05/11] DynamicTablesPkg: Update MADT generator for ACPI 6.5

2023-09-11 Thread Jeshua Smith via groups.io 
Hi Sami,

What is the status of getting this series updated, reviewed, and merged?

From: Sami Mujawar 
Sent: Thursday, August 3, 2023 1:08 AM
To: Jeshua Smith ; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v1 05/11] DynamicTablesPkg: Update MADT 
generator for ACPI 6.5

External email: Use caution opening links or attachments

Hi Jeshua,

Thank you for the feedback.

On Tue, Aug 1, 2023 at 08:44 AM, Jeshua Smith wrote:
It looks like you are setting the wrong field here (should be TrbeInterrupt, 
not SpeOverflowInterrupt):
+ // Setting TrbeInterrupt to 0 ensures backward compatibility with
+ // ACPI 6.4
+ Gicc->SpeOverflowInterrupt = 0;
Indeed this is a bug. I will address this in the v2 series.

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108493): https://edk2.groups.io/g/devel/message/108493
Mute This Topic: https://groups.io/mt/100347387/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v1 2/3] Platform/ARM: FVP: Specify TRBE interrupt in MADT GICC

2023-08-03 Thread Jeshua Smith via groups.io 
My apologies, I see it there now. I had gotten zero results when I searched for 
it on github.com/tianocore/edk2, but it looks like that was user error on my 
part.

From: Sami Mujawar 
Sent: Thursday, August 3, 2023 1:01 AM
To: Jeshua Smith ; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH edk2-platforms v1 2/3] Platform/ARM: FVP: 
Specify TRBE interrupt in MADT GICC

External email: Use caution opening links or attachments

Hi Jeshua,
On Wed, Aug 2, 2023 at 02:48 PM, Jeshua Smith wrote:
This code depends on ArmReadIdAA64Dfr0(), which as far as I can tell is not 
present in the EDK2 repo or the patch series mentioned in your 0/3 message.
toggle quoted message Show quoted text

Apparently the ArmReadIdAA64Dfr0() was added 4 months ago and is already 
present in ArmLib in upstream edk2, see 
https://github.com/tianocore/edk2/blame/master/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S#L478-L480
 and this is also present in my edk2 ETE patch series at 
https://github.com/samimujawar/edk2/blame/2620_ete_dev_fvp_v1/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S#L478-L480
 can you check, please?

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107549): https://edk2.groups.io/g/devel/message/107549
Mute This Topic: https://groups.io/mt/100347409/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v1 2/3] Platform/ARM: FVP: Specify TRBE interrupt in MADT GICC

2023-08-02 Thread Jeshua Smith via groups.io 
This code depends on ArmReadIdAA64Dfr0(), which as far as I can tell is not 
present in the EDK2 repo or the patch series mentioned in your 0/3 message.

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Jeshua Smith via 
groups.io
Sent: Tuesday, August 1, 2023 9:38 AM
To: devel@edk2.groups.io; sami.muja...@arm.com
Cc: ardb+tianoc...@kernel.org; thomas.abra...@arm.com; pierre.gond...@arm.com; 
anshuman.khand...@arm.com; matteo.carl...@arm.com; akanksha.ja...@arm.com; 
sibel.allin...@arm.com; n...@arm.com
Subject: Re: [edk2-devel] [PATCH edk2-platforms v1 2/3] Platform/ARM: FVP: 
Specify TRBE interrupt in MADT GICC

External email: Use caution opening links or attachments


This comment in the code looks wrong:

+// TRBE Interrupt is PPI 13 on FVP model.
+TrbeInterrupt = 31;

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Sami Mujawar via 
groups.io
Sent: Tuesday, July 25, 2023 4:31 AM
To: devel@edk2.groups.io
Cc: Sami Mujawar ; ardb+tianoc...@kernel.org; 
thomas.abra...@arm.com; pierre.gond...@arm.com; anshuman.khand...@arm.com; 
matteo.carl...@arm.com; akanksha.ja...@arm.com; sibel.allin...@arm.com; 
n...@arm.com
Subject: [edk2-devel] [PATCH edk2-platforms v1 2/3] Platform/ARM: FVP: Specify 
TRBE interrupt in MADT GICC

External email: Use caution opening links or attachments


When TRBE is enabled the FVP model uses the PPI 15 (i.e. INT ID 31) as the TRBE 
interrupt.
Ref: https://www.kernel.org/doc/Documentation/
devicetree/bindings/arm/arm,trace-buffer-extension.yaml

Therefore, check the debug feature register ID_AA64DFR0_EL1.TraceBuffer field 
to see if TRBE is enabled and configure the TRBE interrupt in the GICC 
structure in the MADT ACPI table.

Note: To enable TRBE support in the FVP REvC model 1. Build TF-A with the 
CTX_INCLUDE_AARCH32_REGS=0
   build flag set, otherwise this results in an
   exception when booting TF-A.
2. Set the model parameters to enable TRBE
-C cluster0.has_trbe=1 -C cluster1.has_trbe=1

Signed-off-by: Sami Mujawar 
---
 
Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
  | 39 
 
Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
 |  3 +-
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git 
a/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
 
b/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
index 
4df2d6cdae58df344804a8b41208a3adb8ee0110..03393905be1c627b7cdbaa0efed33e920072c8cb
 100644
--- 
a/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
+++ b/Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManager
+++ Dxe/ConfigurationManager.c
@@ -1,7 +1,7 @@
 /** @file
   Configuration Manager Dxe

-  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.
+  Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -37,8 +38,8 @@ EDKII_PLATFORM_REPOSITORY_INFO VExpressPlatRepositoryInfo = {
   {
 // FADT Table
 {
-  EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
-  EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION,
+  EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
+  EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_REVISION,
   CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt),
   NULL
 },
@@ -51,8 +52,8 @@ EDKII_PLATFORM_REPOSITORY_INFO VExpressPlatRepositoryInfo = {
 },
 // MADT Table
 {
-  EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
-  EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION,
+  EFI_ACPI_6_5_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
+  EFI_ACPI_6_5_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION,
   CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMadt),
   NULL
 },
@@ -109,15 +110,15 @@ EDKII_PLATFORM_REPOSITORY_INFO VExpressPlatRepositoryInfo 
= {
   },

   // Boot architecture information
-  { EFI_ACPI_6_3_ARM_PSCI_COMPLIANT },  // BootArchFlags
+  { EFI_ACPI_6_5_ARM_PSCI_COMPLIANT },  // BootArchFlags

 #ifdef HEADLESS_PLATFORM
   // Fixed feature flag information
-  { EFI_ACPI_6_3_HEADLESS },// Fixed feature flags
+  { EFI_ACPI_6_5_HEADLESS },// Fixed feature flags
 #endif

   // Power management profile information
-  { EFI_ACPI_6_3_PM_PROFILE_ENTERPRISE_SERVER },// PowerManagement Profile
+  { EFI_ACPI_6_5_PM_PROFILE_ENTERPRISE_SERVER },// PowerManagement Profile

   /* GIC CPU Interface information
  GIC_ENTRY (CPUInterfaceNumber, Mpidr, PmuIrq, VGicIrq, EnergyEfficiency) 
@@ -474,6 +475,9 @@ InitializePlatformRepository (
   )
 {
   EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+  UINT64 DbgFeatures;
+  UINTN  Index

Re: [edk2-devel] [PATCH v1 05/11] DynamicTablesPkg: Update MADT generator for ACPI 6.5

2023-08-01 Thread Jeshua Smith via groups.io 
It looks like you are setting the wrong field here (should be TrbeInterrupt, 
not SpeOverflowInterrupt):
+// Setting TrbeInterrupt to 0 ensures backward compatibility with
+// ACPI 6.4
+Gicc->SpeOverflowInterrupt = 0;

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Sami Mujawar via 
groups.io
Sent: Tuesday, July 25, 2023 4:28 AM
To: devel@edk2.groups.io
Cc: Sami Mujawar ; pierre.gond...@arm.com; 
anshuman.khand...@arm.com; matteo.carl...@arm.com; akanksha.ja...@arm.com; 
sibel.allin...@arm.com; n...@arm.com
Subject: [edk2-devel] [PATCH v1 05/11] DynamicTablesPkg: Update MADT generator 
for ACPI 6.5

External email: Use caution opening links or attachments


The ACPI 6.5 specification updates the MADT table to add a new field to GICC 
for specifying the TRBE interrupt and also adds support for Online Capable flag 
to the GICC flags.

The Online Capable flags should be passed transparently through as specified in 
the CM_ARM_GICC_INFO.Flags field and only require the MADT table revision to be 
setup to
6 to reflect the ACPI 6.5 specification.

The TRBE field needs to be appropriately setup in the GICC structure.

Therefore, update the MADT generator to reflect the above updates required for 
supporting ACPI 6.5

Signed-off-by: Sami Mujawar 
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c | 83 
+++-
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
index 
2102a59faf498eaabc509443461ada999610..c90548bc97aa1b086f21c8378f242b2b073307f3
 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
@@ -1,11 +1,11 @@
 /** @file
   MADT Table Generator

-  Copyright (c) 2017 - 2020, ARM Limited. All rights reserved.
+  Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent

   @par Reference(s):
-  - ACPI 6.3 Specification - January 2019
+  - ACPI 6.5 Specification - Aug 29, 2022

 **/

@@ -82,7 +82,7 @@ GET_OBJECT_LIST (
   );

 /** This function updates the GIC CPU Interface Information in the
-EFI_ACPI_6_3_GIC_STRUCTURE structure.
+EFI_ACPI_6_5_GIC_STRUCTURE structure.

   @param [in]  Gicc   Pointer to GIC CPU Interface structure.
   @param [in]  GicCInfo   Pointer to the GIC CPU Interface Information.
@@ -91,7 +91,7 @@ GET_OBJECT_LIST (
 STATIC
 VOID
 AddGICC (
-  INEFI_ACPI_6_3_GIC_STRUCTURE  *CONST  Gicc,
+  INEFI_ACPI_6_5_GIC_STRUCTURE  *CONST  Gicc,
   IN  CONST CM_ARM_GICC_INFO*CONST  GicCInfo,
   IN  CONST UINT8   MadtRev
   )
@@ -100,9 +100,9 @@ AddGICC (
   ASSERT (GicCInfo != NULL);

   // UINT8 Type
-  Gicc->Type = EFI_ACPI_6_3_GIC;
+  Gicc->Type = EFI_ACPI_6_5_GIC;
   // UINT8 Length
-  Gicc->Length = sizeof (EFI_ACPI_6_3_GIC_STRUCTURE);
+  Gicc->Length = sizeof (EFI_ACPI_6_5_GIC_STRUCTURE);
   // UINT16 Reserved
   Gicc->Reserved = EFI_ACPI_RESERVED_WORD;

@@ -148,6 +148,15 @@ AddGICC (
 // in EFI_ACPI_6_2_GIC_STRUCTURE.
 Gicc->SpeOverflowInterrupt = 0;
   }
+
+  // UINT16  TrbeInterrupt
+  if (MadtRev > EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION) {
+Gicc->TrbeInterrupt = GicCInfo->TrbeInterrupt;  } else {
+// Setting TrbeInterrupt to 0 ensures backward compatibility with
+// ACPI 6.4
+Gicc->SpeOverflowInterrupt = 0;
+  }
 }

 /**
@@ -214,7 +223,7 @@ IsAcpiUidEqual (
 STATIC
 EFI_STATUS
 AddGICCList (
-  IN  EFI_ACPI_6_3_GIC_STRUCTURE  *Gicc,
+  IN  EFI_ACPI_6_5_GIC_STRUCTURE  *Gicc,
   IN  CONST CM_ARM_GICC_INFO  *GicCInfo,
   INUINT32GicCCount,
   IN  CONST UINT8 MadtRev
@@ -252,7 +261,7 @@ AddGICCList (
 STATIC
 VOID
 AddGICD (
-  EFI_ACPI_6_3_GIC_DISTRIBUTOR_STRUCTURE  *CONST  Gicd,
+  EFI_ACPI_6_5_GIC_DISTRIBUTOR_STRUCTURE  *CONST  Gicd,
   CONST CM_ARM_GICD_INFO  *CONST  GicDInfo
   )
 {
@@ -260,9 +269,9 @@ AddGICD (
   ASSERT (GicDInfo != NULL);

   // UINT8 Type
-  Gicd->Type = EFI_ACPI_6_3_GICD;
+  Gicd->Type = EFI_ACPI_6_5_GICD;
   // UINT8 Length
-  Gicd->Length = sizeof (EFI_ACPI_6_3_GIC_DISTRIBUTOR_STRUCTURE);
+  Gicd->Length = sizeof (EFI_ACPI_6_5_GIC_DISTRIBUTOR_STRUCTURE);
   // UINT16 Reserved
   Gicd->Reserved1 = EFI_ACPI_RESERVED_WORD;
   // UINT32 Identifier
@@ -289,15 +298,15 @@ AddGICD (
 STATIC
 VOID
 AddGICMsiFrame (
-  IN  EFI_ACPI_6_3_GIC_MSI_FRAME_STRUCTURE  *CONST  GicMsiFrame,
+  IN  EFI_ACPI_6_5_GIC_MSI_FRAME_STRUCTURE  *CONST  GicMsiFrame,
   IN  CONST CM_ARM_GIC_MSI_FRAME_INFO   *CONST  GicMsiFrameInfo
   )
 {
   ASSERT (GicMsiFrame != NULL);
   ASSERT (GicMsiFrameInfo != NULL);

-  GicMsiFrame->Type= EFI_ACPI_6_3_GIC_MSI_FRAME;
-  GicMsiFrame->Length  = sizeof 
(EFI_ACPI_6_3_GIC_MSI_FRAME_STRUCTURE);
+

Re: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext

2023-02-14 Thread Jeshua Smith via groups.io 
; behavior and I would be happy to have it be that way always without a 
> PCD (see my discussion above), but since it's possible that someone 
> somewhere was relying on the existing behavior I didn't want to break them. I 
> can't think of any advantage the existing (FALSE) behavior has, but that 
> behavior does seem to be intentional based on Ray's response.
>
>
> -Original Message-
> From: Ni, Ray 
> Sent: Thursday, February 9, 2023 10:42 PM
> To: Kinney, Michael D ; 
> devel@edk2.groups.io; Jeshua Smith ; Demeter, Miki 
> ; Wang, Jian J ; Gao, 
> Liming ; Gao, Zhichao 
> 
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow 
> PlatformBootManagerLib to use BootNext
>
> External email: Use caution opening links or attachments
>
>
> It's the intention to cache BootNext to avoid BootNext change from 
> PlatformBootManagerLib taking affect in this boot.
> Per spec, BootNext selects the boot option of next boot.
> If PlatformBootManagerLib wants to change the boot option for this 
> boot, either it can change the BootOrder, or it can use
> EfiBootManagerBoot() API to boot directly.
>
>
> > -Original Message-
> > From: Kinney, Michael D 
> > Sent: Friday, February 10, 2023 11:50 AM
> > To: devel@edk2.groups.io; jesh...@nvidia.com; Demeter, Miki 
> > ; Ni, Ray ; Wang, Jian J 
> > ; Gao, Liming ; 
> > Gao, Zhichao 
> > Cc: Kinney, Michael D 
> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow 
> > PlatformBootManagerLib to use BootNext
> >
> > Hi Jeshua,
> >
> > I prefer to not add more PCDs if it can be avoided.
> >
> > Do you think the current behavior is a bug/gap and that the proposed 
> > new behavior when the PCD is TRUE is the correct behavior?
> >
> > What would be the impact of not adding the PCD and just implementing 
> > the new behavior?
> >
> > Mike
> >
> >
> > > -Original Message-
> > > From: devel@edk2.groups.io  On Behalf Of
> > Jeshua Smith via groups.io
> > > Sent: Thursday, February 9, 2023 12:01 PM
> > > To: Demeter, Miki 
> > > Cc: devel@edk2.groups.io
> > > Subject: FW: [edk2-devel] [PATCH] MdeModulePkg: allow
> > PlatformBootManagerLib to use BootNext
> > >
> > > Miki, this patch, as well as my queries on the mailing list about 
> > > this topic
> > prior to the patch, hasn't received any response on
> > > the mailing list. One kind person did respond privately with some
> > information about my questions.
> > >
> > > -Original Message-
> > > From: devel@edk2.groups.io  On Behalf Of
> > Jeshua Smith via groups.io
> > > Sent: Thursday, January 19, 2023 10:36 AM
> > > To: devel@edk2.groups.io
> > > Cc: jian.j.w...@intel.com; gaolim...@byosoft.com.cn;
> > zhichao@intel.com; ray...@intel.com; Jeshua Smith 
> > 
> > > Subject: [edk2-devel] [PATCH] MdeModulePkg: allow
> > PlatformBootManagerLib to use BootNext
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Currently BdsEntry caches BootNext before calling
> > PlatformBootManagerLib APIs, with the result that:
> > > - If BootNext is already set, a BootNext value written by the APIs 
> > > will be
> > ignored and deleted, and the current boot will use
> > > the cached BootNext value.
> > > - If BootNext is not present, a BootNext value written by the APIs 
> > > will have
> > no effect on the current boot, but will be used by
> > > the next boot.
> > >
> > > This patch adds PcdAllowBootNextFromPlatformBootManagerLib so that 
> > > a
> > platform can enable PlatformBootManagerLib API calls to set
> > > BootNext to control the current boot.
> > > - If the PCD is FALSE (default), there is no change.
> > > - If the PCD is TRUE, then a BootNext value written by the
> > PlatformBootManagerLib APIs will affect the current boot.
> > >
> > > Signed-off-by: Jeshua Smith 
> > > ---
> > >  MdeModulePkg/MdeModulePkg.dec|  7 +
> > >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 27 ++-
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 34
> > > ++--
> > >  3 files changed, 47 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index 9605c617b7..0e74131712
> > 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg

Re: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext

2023-02-13 Thread Jeshua Smith via groups.io 
 2023 10:42 PM
To: Kinney, Michael D ; devel@edk2.groups.io; 
Jeshua Smith ; Demeter, Miki ; 
Wang, Jian J ; Gao, Liming ; 
Gao, Zhichao 
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to 
use BootNext

External email: Use caution opening links or attachments


It's the intention to cache BootNext to avoid BootNext change from 
PlatformBootManagerLib taking affect in this boot.
Per spec, BootNext selects the boot option of next boot.
If PlatformBootManagerLib wants to change the boot option for this boot, either 
it can change the BootOrder, or it can use EfiBootManagerBoot() API to boot 
directly.


> -Original Message-
> From: Kinney, Michael D 
> Sent: Friday, February 10, 2023 11:50 AM
> To: devel@edk2.groups.io; jesh...@nvidia.com; Demeter, Miki 
> ; Ni, Ray ; Wang, Jian J 
> ; Gao, Liming ; Gao, 
> Zhichao 
> Cc: Kinney, Michael D 
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow 
> PlatformBootManagerLib to use BootNext
>
> Hi Jeshua,
>
> I prefer to not add more PCDs if it can be avoided.
>
> Do you think the current behavior is a bug/gap and that the proposed 
> new behavior when the PCD is TRUE is the correct behavior?
>
> What would be the impact of not adding the PCD and just implementing 
> the new behavior?
>
> Mike
>
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of
> Jeshua Smith via groups.io
> > Sent: Thursday, February 9, 2023 12:01 PM
> > To: Demeter, Miki 
> > Cc: devel@edk2.groups.io
> > Subject: FW: [edk2-devel] [PATCH] MdeModulePkg: allow
> PlatformBootManagerLib to use BootNext
> >
> > Miki, this patch, as well as my queries on the mailing list about 
> > this topic
> prior to the patch, hasn't received any response on
> > the mailing list. One kind person did respond privately with some
> information about my questions.
> >
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of
> Jeshua Smith via groups.io
> > Sent: Thursday, January 19, 2023 10:36 AM
> > To: devel@edk2.groups.io
> > Cc: jian.j.w...@intel.com; gaolim...@byosoft.com.cn;
> zhichao@intel.com; ray...@intel.com; Jeshua Smith 
> 
> > Subject: [edk2-devel] [PATCH] MdeModulePkg: allow
> PlatformBootManagerLib to use BootNext
> >
> > External email: Use caution opening links or attachments
> >
> >
> > Currently BdsEntry caches BootNext before calling
> PlatformBootManagerLib APIs, with the result that:
> > - If BootNext is already set, a BootNext value written by the APIs 
> > will be
> ignored and deleted, and the current boot will use
> > the cached BootNext value.
> > - If BootNext is not present, a BootNext value written by the APIs 
> > will have
> no effect on the current boot, but will be used by
> > the next boot.
> >
> > This patch adds PcdAllowBootNextFromPlatformBootManagerLib so that a
> platform can enable PlatformBootManagerLib API calls to set
> > BootNext to control the current boot.
> > - If the PCD is FALSE (default), there is no change.
> > - If the PCD is TRUE, then a BootNext value written by the
> PlatformBootManagerLib APIs will affect the current boot.
> >
> > Signed-off-by: Jeshua Smith 
> > ---
> >  MdeModulePkg/MdeModulePkg.dec|  7 +
> >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 27 ++-
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 34
> > ++--
> >  3 files changed, 47 insertions(+), 21 deletions(-)
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 9605c617b7..0e74131712
> 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1093,6 +1093,13 @@
> ># @Prompt Enable UEFI Stack Guard.
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0
> x30001055
> >
> > +  ## Indicates whether PlatformBootManagerLib code can set BootNext
> for the current boot.
> > +  #  If enabled, setting BootNext in PlatformBootManagerLib 
> > + controls the
> current boot.
> > +  #   TRUE  - BootNext value from PlatformBootManagerLib will affect the
> current boot.
> > +  #   FALSE - BootNext value from PlatformBootManagerLib will affect the
> subsequent boot (or be ignored if already set).
> > +  # @Prompt Allow PlatformBootManagerLib to set BootNext for the
> current boot.
> > +
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdAllowBootNextFromPlatformBootM
> anager
> > + Lib|FALSE|BOOLEAN|0x30001056
> > +
> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> >## Dynamic type PCD ca

FW: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext

2023-02-09 Thread Jeshua Smith via groups.io 
Miki, this patch, as well as my queries on the mailing list about this topic 
prior to the patch, hasn't received any response on the mailing list. One kind 
person did respond privately with some information about my questions.

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Jeshua Smith via 
groups.io
Sent: Thursday, January 19, 2023 10:36 AM
To: devel@edk2.groups.io
Cc: jian.j.w...@intel.com; gaolim...@byosoft.com.cn; zhichao@intel.com; 
ray...@intel.com; Jeshua Smith 
Subject: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use 
BootNext

External email: Use caution opening links or attachments


Currently BdsEntry caches BootNext before calling PlatformBootManagerLib APIs, 
with the result that:
- If BootNext is already set, a BootNext value written by the APIs will be 
ignored and deleted, and the current boot will use the cached BootNext value.
- If BootNext is not present, a BootNext value written by the APIs will have no 
effect on the current boot, but will be used by the next boot.

This patch adds PcdAllowBootNextFromPlatformBootManagerLib so that a platform 
can enable PlatformBootManagerLib API calls to set BootNext to control the 
current boot.
- If the PCD is FALSE (default), there is no change.
- If the PCD is TRUE, then a BootNext value written by the 
PlatformBootManagerLib APIs will affect the current boot.

Signed-off-by: Jeshua Smith 
---
 MdeModulePkg/MdeModulePkg.dec|  7 +
 MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 27 ++-  
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 34 ++--
 3 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec 
index 9605c617b7..0e74131712 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1093,6 +1093,13 @@
   # @Prompt Enable UEFI Stack Guard.
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055

+  ## Indicates whether PlatformBootManagerLib code can set BootNext for the 
current boot.
+  #  If enabled, setting BootNext in PlatformBootManagerLib controls the 
current boot.
+  #   TRUE  - BootNext value from PlatformBootManagerLib will affect the 
current boot.
+  #   FALSE - BootNext value from PlatformBootManagerLib will affect the 
subsequent boot (or be ignored if already set).
+  # @Prompt Allow PlatformBootManagerLib to set BootNext for the current boot.
+  
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAllowBootNextFromPlatformBootManager
+ Lib|FALSE|BOOLEAN|0x30001056
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting 
action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of 
callback function diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf 
b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
index 5bac635def..b7a3560f5f 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -85,19 +85,20 @@
   gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate## CONSUMES

 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLangCodes## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang ## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision  ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand  ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable  ## 
SOMETIMES_CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed   ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport  ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport   ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes  ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLangCodes  ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang   ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel  ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut   ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor  ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision## 
CONSUMES

Re: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

2023-01-31 Thread Jeshua Smith via groups.io 
Thanks. Does this work for you?
https://github.com/NVIDIA/edk2/pull/19

-Original Message-
From: Gao, Zhichao  
Sent: Friday, January 27, 2023 8:59 PM
To: devel@edk2.groups.io; Jeshua Smith 
Cc: Ni, Ray 
Subject: RE: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

External email: Use caution opening links or attachments


Can you share then code change link of your personal repo? The patch I download 
from this email cannot apply to the open source branch.

Thanks,
Zhichao

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Jeshua 
> Smith via groups.io
> Sent: Wednesday, January 25, 2023 4:55 AM
> To: Gao, Zhichao ; devel@edk2.groups.io
> Cc: Ni, Ray 
> Subject: Re: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser
>
> Thanks for the review. What is the next step to get this merged?
>
> -Original Message-
> From: Gao, Zhichao 
> Sent: Sunday, January 15, 2023 5:59 PM
> To: Jeshua Smith ; devel@edk2.groups.io
> Cc: Ni, Ray 
> Subject: RE: [PATCH] ShellPkg/AcpiView: ERST Parser
>
> External email: Use caution opening links or attachments
>
>
> Reviewed-by: Zhichao Gao 
>
> Thanks,
> Zhichao
>
> > -Original Message-
> > From: Jeshua Smith 
> > Sent: Thursday, December 1, 2022 2:31 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray ; Gao, Zhichao 
> > ; Jeshua Smith 
> > Subject: [PATCH] ShellPkg/AcpiView: ERST Parser
> >
> > Add a new parser for the Error Record Serialization Table.
> >
> > The ERST table describes how an OS can save and retrieve
> >
> > hardware error information to and from a persistent store.
> >
> >
> >
> > Signed-off-by: Jeshua Smith 
> >
> > ---
> >
> >  .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  22 ++
> >
> >  .../Parsers/Erst/ErstParser.c | 278 ++
> >
> >  .../UefiShellAcpiViewCommandLib.c |   2 +
> >
> >  .../UefiShellAcpiViewCommandLib.inf   |   2 +
> >
> >  4 files changed, 304 insertions(+)
> >
> >  create mode 100644
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser
> > .c
> >
> >
> >
> > diff --git 
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> >
> > index db8c88f6df..66c992c55c 100644
> >
> > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> >
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> >
> > @@ -1,6 +1,7 @@
> >
> >  /** @file
> >
> >Header file for ACPI parser
> >
> >
> >
> > +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> >
> >Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
> >
> >Copyright (c) 2022, AMD Incorporated. All rights reserved.
> >
> >SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -594,6 +595,27 @@ ParseAcpiDsdt (
> >
> >IN UINT8AcpiTableRevision
> >
> >);
> >
> >
> >
> > +/**
> >
> > +  This function parses the ACPI ERST table.
> >
> > +  When trace is enabled this function parses the ERST table and
> >
> > +  traces the ACPI table fields.
> >
> > +
> >
> > +  This function also performs validation of the ACPI table fields.
> >
> > +
> >
> > +  @param [in] Trace  If TRUE, trace the ACPI fields.
> >
> > +  @param [in] PtrPointer to the start of the buffer.
> >
> > +  @param [in] AcpiTableLengthLength of the ACPI table.
> >
> > +  @param [in] AcpiTableRevision  Revision of the ACPI table.
> >
> > +**/
> >
> > +VOID
> >
> > +EFIAPI
> >
> > +ParseAcpiErst (
> >
> > +  IN BOOLEAN  Trace,
> >
> > +  IN UINT8*Ptr,
> >
> > +  IN UINT32   AcpiTableLength,
> >
> > +  IN UINT8AcpiTableRevision
> >
> > +  );
> >
> > +
> >
> >  /**
> >
> >This function parses the ACPI FACS table.
> >
> >When trace is enabled this function parses the FACS table and
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstPars
> > er
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstPars
> > er
> > .c
> >
> > new file mode 100644
> >
> > index 00..e2af0c44b4
> >
> > --- /dev/null
> >
> > +++
> > b/

Re: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

2023-01-24 Thread Jeshua Smith via groups.io 
Thanks for the review. What is the next step to get this merged?

-Original Message-
From: Gao, Zhichao  
Sent: Sunday, January 15, 2023 5:59 PM
To: Jeshua Smith ; devel@edk2.groups.io
Cc: Ni, Ray 
Subject: RE: [PATCH] ShellPkg/AcpiView: ERST Parser

External email: Use caution opening links or attachments


Reviewed-by: Zhichao Gao 

Thanks,
Zhichao

> -Original Message-
> From: Jeshua Smith 
> Sent: Thursday, December 1, 2022 2:31 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray ; Gao, Zhichao ; 
> Jeshua Smith 
> Subject: [PATCH] ShellPkg/AcpiView: ERST Parser
>
> Add a new parser for the Error Record Serialization Table.
>
> The ERST table describes how an OS can save and retrieve
>
> hardware error information to and from a persistent store.
>
>
>
> Signed-off-by: Jeshua Smith 
>
> ---
>
>  .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  22 ++
>
>  .../Parsers/Erst/ErstParser.c | 278 ++
>
>  .../UefiShellAcpiViewCommandLib.c |   2 +
>
>  .../UefiShellAcpiViewCommandLib.inf   |   2 +
>
>  4 files changed, 304 insertions(+)
>
>  create mode 100644
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c
>
>
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>
> index db8c88f6df..66c992c55c 100644
>
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>
> @@ -1,6 +1,7 @@
>
>  /** @file
>
>Header file for ACPI parser
>
>
>
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
>
>Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
>
>Copyright (c) 2022, AMD Incorporated. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -594,6 +595,27 @@ ParseAcpiDsdt (
>
>IN UINT8AcpiTableRevision
>
>);
>
>
>
> +/**
>
> +  This function parses the ACPI ERST table.
>
> +  When trace is enabled this function parses the ERST table and
>
> +  traces the ACPI table fields.
>
> +
>
> +  This function also performs validation of the ACPI table fields.
>
> +
>
> +  @param [in] Trace  If TRUE, trace the ACPI fields.
>
> +  @param [in] PtrPointer to the start of the buffer.
>
> +  @param [in] AcpiTableLengthLength of the ACPI table.
>
> +  @param [in] AcpiTableRevision  Revision of the ACPI table.
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +ParseAcpiErst (
>
> +  IN BOOLEAN  Trace,
>
> +  IN UINT8*Ptr,
>
> +  IN UINT32   AcpiTableLength,
>
> +  IN UINT8AcpiTableRevision
>
> +  );
>
> +
>
>  /**
>
>This function parses the ACPI FACS table.
>
>When trace is enabled this function parses the FACS table and
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser
> .c 
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser
> .c
>
> new file mode 100644
>
> index 00..e2af0c44b4
>
> --- /dev/null
>
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser
> .c
>
> @@ -0,0 +1,278 @@
>
> +/** @file
>
> +  ERST table parser
>
> +
>
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
>
> +  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +  @par Reference(s):
>
> +- ACPI 6.5 Specification - August 2022
>
> +**/
>
> +
>
> +#include 
>
> +#include 
>
> +#include "AcpiParser.h"
>
> +#include "AcpiTableParser.h"
>
> +
>
> +// Local variables
>
> +STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
>
> +STATIC UINT32*InstructionEntryCount;
>
> +
>
> +/**
>
> +  An array of strings describing the Erst actions
>
> +**/
>
> +STATIC CONST CHAR16  *ErstActionTable[] = {
>
> +  L"BEGIN_WRITE_OPERATION",
>
> +  L"BEGIN_READ_OPERATION",
>
> +  L"BEGIN_CLEAR_OPERATION",
>
> +  L"END_OPERATION",
>
> +  L"SET_RECORD_OFFSET",
>
> +  L"EXECUTE_OPERATION",
>
> +  L"CHECK_BUSY_STATUS",
>
> +  L"GET_COMMAND_STATUS",
>
> +  L"GET_RECORD_IDENTIFIER",
>
> +  L"SET_RECORD_IDENTIFIER",
>
> +  L"GET_RECORD_COUNT",
>
> +  L"BEGIN_DUMMY_WRITE_OPERATION",
>
> +  L"RESERVED",
>
> +  L"GET_ERROR_LOG_ADDRESS_RANGE",
>
> +  L"GET_ERROR_LOG_ADDRESS_RANGE_LENGTH",
>
> +  L"GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES",
>
> +  L"GET_EXECUTE_OPERATION_TIMINGS"
>
> +};
>
> +
>
> +/**
>
> +  An array of strings describing the Erst instructions
>
> +**/
>
> +STATIC CONST CHAR16  *ErstInstructionTable[] = {
>
> +  L"READ_REGISTER",
>
> +  L"READ_REGISTER_VALUE",
>
> +  L"WRITE_REGISTER",
>
> +  L"WRITE_REGISTER_VALUE",
>
> +  L"NOOP",
>
> +  L"LOAD_VAR1",
>
> +  L"LOAD_VAR2",
>
> +  L"STORE_VAR1",
>
> +  L"ADD",
>
> +  L"SUBTRACT",
>
> +  L"ADD_VALUE",
>
> +  L"SUBTRACT_VALUE",
>
> +  L"STALL",
>
> +  L"STALL_WHILE_TRUE",
>
> +  L"SKIP_NEXT_INSTRUCTION_IF_TRUE",
>
> +  L"GOTO",
>
> +  L"SET_SRC_ADDRESS_BASE",
>
> +  L"SET_DST_ADDRESS_BASE",
>
>

[edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext

2023-01-19 Thread Jeshua Smith via groups.io 
Currently BdsEntry caches BootNext before calling PlatformBootManagerLib
APIs, with the result that:
- If BootNext is already set, a BootNext value written by the APIs will
be ignored and deleted, and the current boot will use the cached
BootNext value.
- If BootNext is not present, a BootNext value written by the APIs will
have no effect on the current boot, but will be used by the next boot.

This patch adds PcdAllowBootNextFromPlatformBootManagerLib so that a
platform can enable PlatformBootManagerLib API calls to set BootNext
to control the current boot.
- If the PCD is FALSE (default), there is no change.
- If the PCD is TRUE, then a BootNext value written by the
PlatformBootManagerLib APIs will affect the current boot.

Signed-off-by: Jeshua Smith 
---
 MdeModulePkg/MdeModulePkg.dec|  7 +
 MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 27 ++-
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 34 ++--
 3 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 9605c617b7..0e74131712 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1093,6 +1093,13 @@
   # @Prompt Enable UEFI Stack Guard.
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
 
+  ## Indicates whether PlatformBootManagerLib code can set BootNext for the 
current boot.
+  #  If enabled, setting BootNext in PlatformBootManagerLib controls the 
current boot.
+  #   TRUE  - BootNext value from PlatformBootManagerLib will affect the 
current boot.
+  #   FALSE - BootNext value from PlatformBootManagerLib will affect the 
subsequent boot (or be ignored if already set).
+  # @Prompt Allow PlatformBootManagerLib to set BootNext for the current boot.
+  
gEfiMdeModulePkgTokenSpaceGuid.PcdAllowBootNextFromPlatformBootManagerLib|FALSE|BOOLEAN|0x30001056
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting 
action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of 
callback function
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf 
b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
index 5bac635def..b7a3560f5f 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -85,19 +85,20 @@
   gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate## CONSUMES
 
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLangCodes## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang ## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel## 
CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision  ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand  ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable  ## 
SOMETIMES_CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed   ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport  ## 
CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport   ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes  ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLangCodes  ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang   ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel  ## 
CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut   ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor  ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowBootNextFromPlatformBootManagerLib ## 
CONSUMES
 
 [Depex]
   TRUE
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index

Re: [edk2-devel] How to select boot device for current boot in response to IPMI System Boot Options commands?

2023-01-10 Thread Jeshua Smith via groups.io 
Any input on this?

It seems that the current behavior isn't ideal:

  *   If BootNext is already set when PlatformBootManagerLib APIs are called, a 
new BootNext value from the PlatformBootManagerLib code will be ignored 
(because the original value is cached before the APIs have a chance to set it) 
and then deleted (when the cached value is consumed after the APIs have been 
called)
  *   If BootNext is not already set, then the current boot will not be 
affected by the BootNext value that PlatformBootManagerLib API code sets 
(because there was no value when the value was cached), but the subsequent boot 
will boot with the BootNext value set by the APIs during the current boot 
(because not having a cached value skips the deletion of BootNext).

To me this seems inconsistent and confusing.

From: devel@edk2.groups.io  On Behalf Of Jeshua Smith via 
groups.io
Sent: Tuesday, January 3, 2023 11:32 AM
To: devel@edk2.groups.io
Cc: Zhichao Gao ; Ray Ni 
Subject: [edk2-devel] How to select boot device for current boot in response to 
IPMI System Boot Options commands?

External email: Use caution opening links or attachments

Happy New Year!

I'm trying to figure out the proper place to add code to allow the EFI boot 
code to respond to the IPMI System Boot Options request to boot a device on the 
current boot. My initial thought was to change BootNext in the 
PlatformBootManagerLib APIs, but based on the comment 
https://www.mail-archive.com/edk2-devel@lists.01.org/msg30378.html it looks 
like that is *intentionally* unsupported. Does anyone know why we want to avoid 
PlatformBootManagerLib hooks from being able to set BootNext to control what 
gets booted on the current boot? Is there an intended alternative way to 
support the IPMI System Boot Options Command request to use a boot device for 
the current boot?

Thanks,

Jeshua Smith



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98250): https://edk2.groups.io/g/devel/message/98250
Mute This Topic: https://groups.io/mt/96034184/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

2023-01-05 Thread Jeshua Smith via groups.io 
Resending after the holidays.

-Original Message-
From: Jeshua Smith 
Sent: Wednesday, December 14, 2022 8:30 AM
To: devel@edk2.groups.io; Jeshua Smith 
Cc: ray...@intel.com; zhichao@intel.com
Subject: RE: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

Is there anything holding this up from getting reviewed?

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Jeshua Smith via 
groups.io
Sent: Wednesday, November 30, 2022 11:31 AM
To: devel@edk2.groups.io
Cc: ray...@intel.com; zhichao@intel.com; Jeshua Smith 
Subject: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

External email: Use caution opening links or attachments


Add a new parser for the Error Record Serialization Table.

The ERST table describes how an OS can save and retrieve

hardware error information to and from a persistent store.



Signed-off-by: Jeshua Smith 

---

 .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  22 ++

 .../Parsers/Erst/ErstParser.c | 278 ++

 .../UefiShellAcpiViewCommandLib.c |   2 +

 .../UefiShellAcpiViewCommandLib.inf   |   2 +

 4 files changed, 304 insertions(+)

 create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c



diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

index db8c88f6df..66c992c55c 100644

--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

@@ -1,6 +1,7 @@

 /** @file

   Header file for ACPI parser



+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

   Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.

   Copyright (c) 2022, AMD Incorporated. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -594,6 +595,27 @@ ParseAcpiDsdt (

   IN UINT8AcpiTableRevision

   );



+/**

+  This function parses the ACPI ERST table.

+  When trace is enabled this function parses the ERST table and

+  traces the ACPI table fields.

+

+  This function also performs validation of the ACPI table fields.

+

+  @param [in] Trace  If TRUE, trace the ACPI fields.

+  @param [in] PtrPointer to the start of the buffer.

+  @param [in] AcpiTableLengthLength of the ACPI table.

+  @param [in] AcpiTableRevision  Revision of the ACPI table.

+**/

+VOID

+EFIAPI

+ParseAcpiErst (

+  IN BOOLEAN  Trace,

+  IN UINT8*Ptr,

+  IN UINT32   AcpiTableLength,

+  IN UINT8AcpiTableRevision

+  );

+

 /**

   This function parses the ACPI FACS table.

   When trace is enabled this function parses the FACS table and

diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c

new file mode 100644

index 00..e2af0c44b4

--- /dev/null

+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c

@@ -0,0 +1,278 @@

+/** @file

+  ERST table parser

+

+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

+  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+  @par Reference(s):

+- ACPI 6.5 Specification - August 2022

+**/

+

+#include 

+#include 

+#include "AcpiParser.h"

+#include "AcpiTableParser.h"

+

+// Local variables

+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;

+STATIC UINT32*InstructionEntryCount;

+

+/**

+  An array of strings describing the Erst actions

+**/

+STATIC CONST CHAR16  *ErstActionTable[] = {

+  L"BEGIN_WRITE_OPERATION",

+  L"BEGIN_READ_OPERATION",

+  L"BEGIN_CLEAR_OPERATION",

+  L"END_OPERATION",

+  L"SET_RECORD_OFFSET",

+  L"EXECUTE_OPERATION",

+  L"CHECK_BUSY_STATUS",

+  L"GET_COMMAND_STATUS",

+  L"GET_RECORD_IDENTIFIER",

+  L"SET_RECORD_IDENTIFIER",

+  L"GET_RECORD_COUNT",

+  L"BEGIN_DUMMY_WRITE_OPERATION",

+  L"RESERVED",

+  L"GET_ERROR_LOG_ADDRESS_RANGE",

+  L"GET_ERROR_LOG_ADDRESS_RANGE_LENGTH",

+  L"GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES",

+  L"GET_EXECUTE_OPERATION_TIMINGS"

+};

+

+/**

+  An array of strings describing the Erst instructions

+**/

+STATIC CONST CHAR16  *ErstInstructionTable[] = {

+  L"READ_REGISTER",

+  L"READ_REGISTER_VALUE",

+  L"WRITE_REGISTER",

+  L"WRITE_REGISTER_VALUE",

+  L"NOOP",

+  L"LOAD_VAR1",

+  L"LOAD_VAR2",

+  L"STORE_VAR1",

+  L"ADD",

+  L"SUBTRACT",

+  L"ADD_VALUE",

+  L"SUBTRACT_VALUE",

+  L"STALL",

+  L"STALL_WHILE_TRUE",

+  L"SKIP_NEXT_INSTRUCTION_IF_TRUE",

+  L"GOTO",

+

[edk2-devel] How to select boot device for current boot in response to IPMI System Boot Options commands?

2023-01-03 Thread Jeshua Smith via groups.io 
Happy New Year!

I'm trying to figure out the proper place to add code to allow the EFI boot 
code to respond to the IPMI System Boot Options request to boot a device on the 
current boot. My initial thought was to change BootNext in the 
PlatformBootManagerLib APIs, but based on the comment 
https://www.mail-archive.com/edk2-devel@lists.01.org/msg30378.html it looks 
like that is *intentionally* unsupported. Does anyone know why we want to avoid 
PlatformBootManagerLib hooks from being able to set BootNext to control what 
gets booted on the current boot? Is there an intended alternative way to 
support the IPMI System Boot Options Command request to use a boot device for 
the current boot?

Thanks,

Jeshua Smith


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97888): https://edk2.groups.io/g/devel/message/97888
Mute This Topic: https://groups.io/mt/96034184/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/Logo: Add a PCD to control the position of the Logo

2022-12-16 Thread Jeshua Smith via groups.io 
I haven’t tried it, but farther down on the page steps 9 and 10 look like 
they’re related to the option Mike suggested, so they might be required for it 
to work?


From: devel@edk2.groups.io  On Behalf Of Sean Rhodes via 
groups.io
Sent: Friday, December 16, 2022 1:58 AM
To: devel@edk2.groups.io; michael.d.kin...@intel.com
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Logo: Add a PCD to control the 
position of the Logo

External email: Use caution opening links or attachments

Hi Mike

Thanks; didn't work but I'll have a play wth it!

Sean

On Thu, 15 Dec 2022 at 22:55, Michael D Kinney 
mailto:michael.d.kin...@intel.com>> wrote:
Hi Sean,

Yes, that is the correct section.  Hard to tell from patch email alone.

There is a git config that can always include the name of the section of the 
INF/DEC/DSC/FDF file where a change is made.
Can make it a bit easier to review.

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

Specifically this one I think:

git config diff.ini.xfuncname '^\[[A-Za-z0-9_., ]+]'

Mike

From: Sean Rhodes mailto:sean@starlabs.systems>>
Sent: Thursday, December 15, 2022 2:17 PM
To: Kinney, Michael D 
mailto:michael.d.kin...@intel.com>>
Cc: devel@edk2.groups.io; Gao, Zhichao 
mailto:zhichao@intel.com>>; Ni, Ray 
mailto:ray...@intel.com>>; Wang, Jian J 
mailto:jian.j.w...@intel.com>>; Gao, Liming 
mailto:gaolim...@byosoft.com.cn>>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Logo: Add a PCD to control the 
position of the Logo

Hi Mike

Thank you; changed to PcdGetBool.

It's in `[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]` 
- is that not right?

Thanks

Sean

On Thu, 15 Dec 2022 at 22:09, Kinney, Michael D 
mailto:michael.d.kin...@intel.com>> wrote:
Hi Sean,

A couple comments related to the PCD type below.

Mike

> -Original Message-
> From: devel@edk2.groups.io 
> mailto:devel@edk2.groups.io>> On Behalf Of Sean Rhodes
> Sent: Thursday, December 15, 2022 1:12 PM
> To: devel@edk2.groups.io
> Cc: Rhodes, Sean mailto:sean@starlabs.systems>>; Gao, 
> Zhichao mailto:zhichao@intel.com>>; Ni, Ray 
> mailto:ray...@intel.com>>; Wang, Jian J
> mailto:jian.j.w...@intel.com>>; Gao, Liming 
> mailto:gaolim...@byosoft.com.cn>>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/Logo: Add a PCD to control the 
> position of the Logo
>
> When set to true, the Logo is positioned according to the BGRT
> specification, 38.2% from the top of the screen. When set to false,
> no behaviour is changed and the logo is positioned centrally.
>
> Cc: Zhichao Gao mailto:zhichao@intel.com>>
> Cc: Ray Ni mailto:ray...@intel.com>>
> Cc: Jian J Wang mailto:jian.j.w...@intel.com>>
> Cc: Liming Gao mailto:gaolim...@byosoft.com.cn>>
> Signed-off-by: Sean Rhodes 
> mailto:sean@starlabs.systems>>
> ---
>  MdeModulePkg/Logo/Logo.c  | 28 +++-
>  MdeModulePkg/Logo/LogoDxe.inf |  4 
>  MdeModulePkg/MdeModulePkg.dec |  6 ++
>  MdeModulePkg/MdeModulePkg.uni |  6 ++
>  4 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Logo/Logo.c b/MdeModulePkg/Logo/Logo.c
> index 8ab874d2da..48862d3207 100644
> --- a/MdeModulePkg/Logo/Logo.c
> +++ b/MdeModulePkg/Logo/Logo.c
> @@ -13,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include 
>
>  #include 
>
>  #include 
>
> +#include 
>
>
>
>  typedef struct {
>
>EFI_IMAGE_ID ImageId;
>
> @@ -51,12 +52,14 @@ GetImage (
>IN EDKII_PLATFORM_LOGO_PROTOCOL*This,
>
>IN OUT UINT32  *Instance,
>
>OUT EFI_IMAGE_INPUT*Image,
>
> +  EFI_GRAPHICS_OUTPUT_PROTOCOL   *GraphicsOutput,
>
>OUT EDKII_PLATFORM_LOGO_DISPLAY_ATTRIBUTE  *Attribute,
>
>OUT INTN   *OffsetX,
>
>OUT INTN   *OffsetY
>
>)
>
>  {
>
> -  UINT32  Current;
>
> +  UINT32  Current;
>
> +  EFI_STATUS  Status;
>
>
>
>if ((Instance == NULL) || (Image == NULL) ||
>
>(Attribute == NULL) || (OffsetX == NULL) || (OffsetY == NULL))
>
> @@ -69,6 +72,29 @@ GetImage (
>  return EFI_NOT_FOUND;
>
>}
>
>
>
> +  if (FixedPcdGetBool (PcdFollowMicrosoftRecommended)) {

Should be PcdGetBool().  The only time FixedPcdGetxxx() is required is
if the PCD value is being used to initialize a struct where the value
is needed at build time.  This allows the PCD type to be flexible and
can be set in platform scope in the DSC file.

>
> +//
>
> +// Get current video resolution and text mode
>
> +//
>
> +Status = gBS->HandleProtocol (
>
> +gST->ConsoleOutHandle,
>
> +,
>
> +(VOID **)
>
> +);
>
> +if (!EFI_ERROR (Status)) {
>
> +  //
>
> +  // Center of LOGO is

Re: [edk2-devel] [edk2-platforms][PATCH] Platform/Sgi: Add VariableFlashInfoLib to fix missing dependency

2022-12-16 Thread Jeshua Smith via groups.io 
It looks like that function call takes a double as a parameter, which is a type 
of floating point number. I’m guessing that’s why the compiler is complaining 
that the ‘+nofp’ feature can’t be used with that function.

From: devel@edk2.groups.io  On Behalf Of Sami Mujawar via 
groups.io
Sent: Friday, December 16, 2022 3:29 AM
To: Vijayenthiran Subramaniam ; 
devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-platforms][PATCH] Platform/Sgi: Add 
VariableFlashInfoLib to fix missing dependency

External email: Use caution opening links or attachments

Hi Vijay,

Thank you for this patch.
This patch as such looks ok to me but I am getting a build the following 
failure when openssl library is being built.

build -a AARCH64 -p Platform\ARM\SgiPkg\PlatformStandaloneMm2.dsc -t GCC5 -b 
DEBUG  -n 1 -D EDK2_OUT_DIR=Build\ArmSgiPlatStmm2 -D SECURE_STORAGE_ENABLE
...
"D:\linaro_toolchain\gcc-arm-11.2-2022.02-mingw-w64-i686-aarch64-none-elf\bin\aarch64-none-elf-gcc"
  
@w:\edk2-maintenance\Build\SgiMmStandalone\DEBUG_GCC5\AARCH64\CryptoPkg\Library\OpensslLib\OpensslLib\OUTPUT\cc_resp.txt
  -c -o 
w:\ekd2-maintenance\Build\SgiMmStandalone\DEBUG_GCC5\AARCH64\CryptoPkg\Library\OpensslLib\OpensslLib\OUTPUT\openssl\crypto\rand\drbg_lib.obj
  
w:\edk2-maintenance\edk2\CryptoPkg\Library\OpensslLib\openssl\crypto\rand\drbg_lib.c
w:\edk2-maintenance\edk2\CryptoPkg\Library\OpensslLib\openssl\crypto\rand\drbg_lib.c:
 In function 'drbg_add':
w:\edk2-maintenance\edk2\CryptoPkg\Library\OpensslLib\openssl\crypto\rand\drbg_lib.c:999:12:
 error: '+nofp' feature modifier is incompatible with the use of floating-point 
types
  999 | static int drbg_add(const void *buf, int num, double randomness)
  |^~~~
NMAKE : fatal error U1077: 
'D:\linaro_toolchain\gcc-arm-11.2-2022.02-mingw-w64-i686-aarch64-none-elf\bin\aarch64-none-elf-gcc.EXE'
 : return code '0x1'
Stop.
...

This may not be related to your patch, but can you check, please?

Regards,

Sami Mujawar



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97509): https://edk2.groups.io/g/devel/message/97509
Mute This Topic: https://groups.io/mt/93808344/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

2022-12-14 Thread Jeshua Smith via groups.io 
Is there anything holding this up from getting reviewed?

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Jeshua Smith via 
groups.io
Sent: Wednesday, November 30, 2022 11:31 AM
To: devel@edk2.groups.io
Cc: ray...@intel.com; zhichao@intel.com; Jeshua Smith 
Subject: [edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

External email: Use caution opening links or attachments


Add a new parser for the Error Record Serialization Table.

The ERST table describes how an OS can save and retrieve

hardware error information to and from a persistent store.



Signed-off-by: Jeshua Smith 

---

 .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  22 ++

 .../Parsers/Erst/ErstParser.c | 278 ++

 .../UefiShellAcpiViewCommandLib.c |   2 +

 .../UefiShellAcpiViewCommandLib.inf   |   2 +

 4 files changed, 304 insertions(+)

 create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c



diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

index db8c88f6df..66c992c55c 100644

--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

@@ -1,6 +1,7 @@

 /** @file

   Header file for ACPI parser



+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

   Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.

   Copyright (c) 2022, AMD Incorporated. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -594,6 +595,27 @@ ParseAcpiDsdt (

   IN UINT8AcpiTableRevision

   );



+/**

+  This function parses the ACPI ERST table.

+  When trace is enabled this function parses the ERST table and

+  traces the ACPI table fields.

+

+  This function also performs validation of the ACPI table fields.

+

+  @param [in] Trace  If TRUE, trace the ACPI fields.

+  @param [in] PtrPointer to the start of the buffer.

+  @param [in] AcpiTableLengthLength of the ACPI table.

+  @param [in] AcpiTableRevision  Revision of the ACPI table.

+**/

+VOID

+EFIAPI

+ParseAcpiErst (

+  IN BOOLEAN  Trace,

+  IN UINT8*Ptr,

+  IN UINT32   AcpiTableLength,

+  IN UINT8AcpiTableRevision

+  );

+

 /**

   This function parses the ACPI FACS table.

   When trace is enabled this function parses the FACS table and

diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c

new file mode 100644

index 00..e2af0c44b4

--- /dev/null

+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c

@@ -0,0 +1,278 @@

+/** @file

+  ERST table parser

+

+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

+  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+  @par Reference(s):

+- ACPI 6.5 Specification - August 2022

+**/

+

+#include 

+#include 

+#include "AcpiParser.h"

+#include "AcpiTableParser.h"

+

+// Local variables

+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;

+STATIC UINT32*InstructionEntryCount;

+

+/**

+  An array of strings describing the Erst actions

+**/

+STATIC CONST CHAR16  *ErstActionTable[] = {

+  L"BEGIN_WRITE_OPERATION",

+  L"BEGIN_READ_OPERATION",

+  L"BEGIN_CLEAR_OPERATION",

+  L"END_OPERATION",

+  L"SET_RECORD_OFFSET",

+  L"EXECUTE_OPERATION",

+  L"CHECK_BUSY_STATUS",

+  L"GET_COMMAND_STATUS",

+  L"GET_RECORD_IDENTIFIER",

+  L"SET_RECORD_IDENTIFIER",

+  L"GET_RECORD_COUNT",

+  L"BEGIN_DUMMY_WRITE_OPERATION",

+  L"RESERVED",

+  L"GET_ERROR_LOG_ADDRESS_RANGE",

+  L"GET_ERROR_LOG_ADDRESS_RANGE_LENGTH",

+  L"GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES",

+  L"GET_EXECUTE_OPERATION_TIMINGS"

+};

+

+/**

+  An array of strings describing the Erst instructions

+**/

+STATIC CONST CHAR16  *ErstInstructionTable[] = {

+  L"READ_REGISTER",

+  L"READ_REGISTER_VALUE",

+  L"WRITE_REGISTER",

+  L"WRITE_REGISTER_VALUE",

+  L"NOOP",

+  L"LOAD_VAR1",

+  L"LOAD_VAR2",

+  L"STORE_VAR1",

+  L"ADD",

+  L"SUBTRACT",

+  L"ADD_VALUE",

+  L"SUBTRACT_VALUE",

+  L"STALL",

+  L"STALL_WHILE_TRUE",

+  L"SKIP_NEXT_INSTRUCTION_IF_TRUE",

+  L"GOTO",

+  L"SET_SRC_ADDRESS_BASE",

+  L"SET_DST_ADDRESS_BASE",

+  L"MOVE_DATA"

+};

+

+/**

+  Validate Erst action.

+

+  @param [in] Ptr   Pointer to the start of the field data.

+  @param [in] Context   Pointer to c

Re: [edk2-devel] [PATCH v3] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

2022-12-14 Thread Jeshua Smith via groups.io 
Is there anything blocking this patch from being merged?

-Original Message-
From: Michael Kubacki  
Sent: Wednesday, November 30, 2022 4:07 PM
To: devel@edk2.groups.io; Jeshua Smith 
Cc: michael.d.kin...@intel.com; sean.bro...@microsoft.com
Subject: Re: [edk2-devel] [PATCH v3] UnitTestFrameworkPkg/UnitTestLib: Print 
expected Status on ASSERT fail

External email: Use caution opening links or attachments


Reviewed-by: Michael Kubacki 

On 11/30/2022 6:02 PM, Jeshua Smith via groups.io wrote:
> Update the UnitTestAssertStatusEqual error message to print out the 
> expected value in addition to the seen value.
>
> Signed-off-by: Jeshua Smith 
> ---
>   UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c 
> b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> index dc05bbd438..0d8e36c938 100644
> --- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> +++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> @@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (
>   {
> CHAR8  TempStr[MAX_STRING_SIZE];
>
> -  snprintf (TempStr, sizeof (TempStr), 
> "UT_ASSERT_STATUS_EQUAL(%s:%p)", Description, (VOID *)Status);
> +  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p 
> + expected:%p)", Description, (VOID *)Status, (VOID *)Expected);
> _assert_true ((Status == Expected), TempStr, FileName, 
> (INT32)LineNumber);
>
> return (Status == Expected);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97374): https://edk2.groups.io/g/devel/message/97374
Mute This Topic: https://groups.io/mt/95370662/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

2022-11-30 Thread Jeshua Smith via groups.io 
Update the UnitTestAssertStatusEqual error message to print out the
expected value in addition to the seen value.

Signed-off-by: Jeshua Smith 
---
 UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c 
b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
index dc05bbd438..0d8e36c938 100644
--- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
+++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
@@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (
 {
   CHAR8  TempStr[MAX_STRING_SIZE];
 
-  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p)", 
Description, (VOID *)Status);
+  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p 
expected:%p)", Description, (VOID *)Status, (VOID *)Expected);
   _assert_true ((Status == Expected), TempStr, FileName, (INT32)LineNumber);
 
   return (Status == Expected);
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96783): https://edk2.groups.io/g/devel/message/96783
Mute This Topic: https://groups.io/mt/95370662/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

2022-11-30 Thread Jeshua Smith via groups.io 
Update the UnitTestAssertStatusEqual error message to print out the
expected value in addition to the seen value.

Change-Id: Ic651584dcdbcf1f8cd8166ad8058744fc0587d72
Signed-off-by: Jeshua Smith 
---
 UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c 
b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
index dc05bbd438..0d8e36c938 100644
--- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
+++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
@@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (
 {

   CHAR8  TempStr[MAX_STRING_SIZE];

 

-  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p)", 
Description, (VOID *)Status);

+  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p 
expected:%p)", Description, (VOID *)Status, (VOID *)Expected);

   _assert_true ((Status == Expected), TempStr, FileName, (INT32)LineNumber);

 

   return (Status == Expected);

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96782): https://edk2.groups.io/g/devel/message/96782
Mute This Topic: https://groups.io/mt/95370574/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

2022-11-30 Thread Jeshua Smith via groups.io 
Hi Mike,

Thanks for the explanation. That makes sense.

I think Option 1 is the simplest in this case, and I'll resubmit the patch with 
that.
Option 2 seems like an unnecessary workaround, since I can't think of a good 
reason to make the printed value a 64 bit value on 32-bit platforms.
Option 3 is interesting. The similar implementation of the function in Assert.c 
already uses the %r capability of PrintLib, but since this is the 
AssertCmocka.c implementation I assume the intent is to stick with only libc as 
a dependency.
Option 4 would perhaps be nicest, but has the downside of becoming an 
additional place to maintain the EFI_STATUS codes. I suppose the status codes 
will likely never change value, but if for some reason they did I can image 
that updating this lookup table might end up being missed, resulting in a very 
frustrating debug for someone who finds that the error message told them the 
wrong status code.

Jeshua

-Original Message-
From: Kinney, Michael D  
Sent: Wednesday, November 30, 2022 1:57 PM
To: devel@edk2.groups.io; Jeshua Smith ; Kinney, Michael D 

Cc: mikub...@linux.microsoft.com; sean.bro...@microsoft.com
Subject: RE: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print 
expected Status on ASSERT fail

External email: Use caution opening links or attachments


Hi Jeshuas,

This is a good idea to show the expected value.

%p was used on purpose because unit tests can be built for 32-bit or 64-bit and 
the EFI_STATUS is same as RETURN_STATUS which is same as UINTN.  UINTN is 
32-bits for 32-bit unit test apps and 64-bit for 64-bit unit test apps.  %p 
prints a pointer sized value, which happens to match the UINTN for support CPU 
archs.

A couple options to consider:
1) Keep using %p instead of %llx.
2) Use UINT64 local variables to hold Status and Expected values and use %llx.
3) Use the MdePkg PrintLib to convert Status and Expected to string names and
   update message to show the name of the status value instead of the hex value.
4) Don't add a dependency on PrintLib and instead convert to string names in
   this same C file.

Best regards,

Mike


> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Jeshua 
> Smith via groups.io
> Sent: Wednesday, November 30, 2022 12:39 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D ; 
> mikub...@linux.microsoft.com; sean.bro...@microsoft.com; Jeshua Smith 
> 
> Subject: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print 
> expected Status on ASSERT fail
>
> Update the UnitTestAssertStatusEqual error message to print out the 
> expected value in addition to the seen value.
>
> Signed-off-by: Jeshua Smith 
> ---
>  UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> index dc05bbd438..322daf318a 100644
> --- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> +++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> @@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (  {
>CHAR8  TempStr[MAX_STRING_SIZE];
>
> -  snprintf (TempStr, sizeof (TempStr), 
> "UT_ASSERT_STATUS_EQUAL(%s:%p)", Description, (VOID *)Status);
> +  snprintf (TempStr, sizeof (TempStr), 
> + "UT_ASSERT_STATUS_EQUAL(%s:0x%llx expected:0x%llx)", Description, 
> + Status,
> Expected);
>_assert_true ((Status == Expected), TempStr, FileName, 
> (INT32)LineNumber);
>
>return (Status == Expected);
> --
> 2.25.1
>
>
>
> 
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96781): https://edk2.groups.io/g/devel/message/96781
Mute This Topic: https://groups.io/mt/95367691/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

2022-11-30 Thread Jeshua Smith via groups.io 
Update the UnitTestAssertStatusEqual error message to print out the
expected value in addition to the seen value.

Signed-off-by: Jeshua Smith 
---
 UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c 
b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
index dc05bbd438..322daf318a 100644
--- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
+++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
@@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (
 {
   CHAR8  TempStr[MAX_STRING_SIZE];
 
-  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p)", 
Description, (VOID *)Status);
+  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:0x%llx 
expected:0x%llx)", Description, Status, Expected);
   _assert_true ((Status == Expected), TempStr, FileName, (INT32)LineNumber);
 
   return (Status == Expected);
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96761): https://edk2.groups.io/g/devel/message/96761
Mute This Topic: https://groups.io/mt/95367691/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] ShellPkg/AcpiView: ERST Parser

2022-11-30 Thread Jeshua Smith via groups.io 
Add a new parser for the Error Record Serialization Table.

The ERST table describes how an OS can save and retrieve

hardware error information to and from a persistent store.



Signed-off-by: Jeshua Smith 

---

 .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  22 ++

 .../Parsers/Erst/ErstParser.c | 278 ++

 .../UefiShellAcpiViewCommandLib.c |   2 +

 .../UefiShellAcpiViewCommandLib.inf   |   2 +

 4 files changed, 304 insertions(+)

 create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c



diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

index db8c88f6df..66c992c55c 100644

--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

@@ -1,6 +1,7 @@

 /** @file

   Header file for ACPI parser

 

+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

   Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.

   Copyright (c) 2022, AMD Incorporated. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -594,6 +595,27 @@ ParseAcpiDsdt (

   IN UINT8AcpiTableRevision

   );

 

+/**

+  This function parses the ACPI ERST table.

+  When trace is enabled this function parses the ERST table and

+  traces the ACPI table fields.

+

+  This function also performs validation of the ACPI table fields.

+

+  @param [in] Trace  If TRUE, trace the ACPI fields.

+  @param [in] PtrPointer to the start of the buffer.

+  @param [in] AcpiTableLengthLength of the ACPI table.

+  @param [in] AcpiTableRevision  Revision of the ACPI table.

+**/

+VOID

+EFIAPI

+ParseAcpiErst (

+  IN BOOLEAN  Trace,

+  IN UINT8*Ptr,

+  IN UINT32   AcpiTableLength,

+  IN UINT8AcpiTableRevision

+  );

+

 /**

   This function parses the ACPI FACS table.

   When trace is enabled this function parses the FACS table and

diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c

new file mode 100644

index 00..e2af0c44b4

--- /dev/null

+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c

@@ -0,0 +1,278 @@

+/** @file

+  ERST table parser

+

+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

+  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+  @par Reference(s):

+- ACPI 6.5 Specification - August 2022

+**/

+

+#include 

+#include 

+#include "AcpiParser.h"

+#include "AcpiTableParser.h"

+

+// Local variables

+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;

+STATIC UINT32*InstructionEntryCount;

+

+/**

+  An array of strings describing the Erst actions

+**/

+STATIC CONST CHAR16  *ErstActionTable[] = {

+  L"BEGIN_WRITE_OPERATION",

+  L"BEGIN_READ_OPERATION",

+  L"BEGIN_CLEAR_OPERATION",

+  L"END_OPERATION",

+  L"SET_RECORD_OFFSET",

+  L"EXECUTE_OPERATION",

+  L"CHECK_BUSY_STATUS",

+  L"GET_COMMAND_STATUS",

+  L"GET_RECORD_IDENTIFIER",

+  L"SET_RECORD_IDENTIFIER",

+  L"GET_RECORD_COUNT",

+  L"BEGIN_DUMMY_WRITE_OPERATION",

+  L"RESERVED",

+  L"GET_ERROR_LOG_ADDRESS_RANGE",

+  L"GET_ERROR_LOG_ADDRESS_RANGE_LENGTH",

+  L"GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES",

+  L"GET_EXECUTE_OPERATION_TIMINGS"

+};

+

+/**

+  An array of strings describing the Erst instructions

+**/

+STATIC CONST CHAR16  *ErstInstructionTable[] = {

+  L"READ_REGISTER",

+  L"READ_REGISTER_VALUE",

+  L"WRITE_REGISTER",

+  L"WRITE_REGISTER_VALUE",

+  L"NOOP",

+  L"LOAD_VAR1",

+  L"LOAD_VAR2",

+  L"STORE_VAR1",

+  L"ADD",

+  L"SUBTRACT",

+  L"ADD_VALUE",

+  L"SUBTRACT_VALUE",

+  L"STALL",

+  L"STALL_WHILE_TRUE",

+  L"SKIP_NEXT_INSTRUCTION_IF_TRUE",

+  L"GOTO",

+  L"SET_SRC_ADDRESS_BASE",

+  L"SET_DST_ADDRESS_BASE",

+  L"MOVE_DATA"

+};

+

+/**

+  Validate Erst action.

+

+  @param [in] Ptr   Pointer to the start of the field data.

+  @param [in] Context   Pointer to context specific information e.g. this

+could be a pointer to the ACPI table header.

+**/

+STATIC

+VOID

+EFIAPI

+ValidateErstAction (

+  IN UINT8  *Ptr,

+  IN VOID   *Context

+  )

+{

+  if (*Ptr > EFI_ACPI_6_4_ERST_GET_EXECUTE_OPERATION_TIMINGS) {

+IncrementErrorCount ();

+Print (L"\nError: 0x%02x is not a valid action encoding", *Ptr);

+  }

+}

+

+/**

+  Validate Erst instruction.

+

+  @param [in] Ptr   Pointer to the start of the field data.

+  @param [in] Context   Pointer to context specific information e.g. this

+could be a pointer to the ACPI table header.

+**/

+STATIC

+VOID

+EFIAPI

+ValidateErstInstruction (

+  IN UINT8  *Ptr,

+  IN VOID   *Context

+  )

+{

+  if (*Ptr >