* [PATCH] DynamicTablesPkg: Add support to build _DSD
@ 2022-08-05 15:14 Jeff Brasen
2022-08-12 13:36 ` [edk2-devel] " PierreGondois
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Brasen @ 2022-08-05 15:14 UTC (permalink / raw)
To: devel; +Cc: Sami.Mujawar, Alexei.Fedorov, Jeff Brasen
Add APIs needed to build _DSD with different UUIDs.
This is per ACPI specification 6.4 s6.2.5.
Adds support for building data packages with format
Package {"Name", Integer}
Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
.../Include/Library/AmlLib/AmlLib.h | 50 ++++
.../Common/AmlLib/CodeGen/AmlCodeGen.c | 236 ++++++++++++++++++
2 files changed, 286 insertions(+)
diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
index 6f214c0dfa..30cab3f6bb 100644
--- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
+++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
@@ -1280,6 +1280,56 @@ AmlAddLpiState (
IN AML_OBJECT_NODE_HANDLE LpiNode
);
+/** AML code generation for a _DSD device data object.
+
+ AmlAddDeviceDataDescriptorPackage (Uuid, ParentNode, NewObjectNode) is
+ equivalent of the following ASL code:
+ ToUUID(Uuid),
+ Package () {}
+
+ @ingroup CodeGenApis
+
+ @param [in] Uuid The Uuid of the descriptor to be created
+ @param [in] DsdNode Node of the DSD Package.
+ @param [out] PackageNode If success, contains the created package node.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+**/
+EFI_STATUS
+EFIAPI
+AmlAddDeviceDataDescriptorPackage (
+ IN CONST EFI_GUID *Uuid,
+ IN AML_OBJECT_NODE_HANDLE DsdNode,
+ OUT AML_OBJECT_NODE_HANDLE *PackageNode
+ );
+
+/** AML code generation to add a package with a name and value,
+ * to a parent package
+
+ AmlAddNameValuePackage ("Name", Value, ParentNode) is
+ equivalent of the following ASL code:
+ Package (2) {"Name", Value}
+
+ @ingroup CodeGenApis
+
+ @param [in] Name String to place in first entry of package
+ @param [in] Value Integer to place in second entry of package
+ @param [in] PackageNode Package to add new sub package to.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+**/
+EFI_STATUS
+EFIAPI
+AmlAddNameIntegerPackage (
+ IN CHAR8 *Name,
+ IN UINT64 Value,
+ IN AML_OBJECT_NODE_HANDLE PackageNode
+ );
+
// DEPRECATED APIS
#ifndef DISABLE_NEW_DEPRECATED_INTERFACES
diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
index e51d2dd7f0..80e43d0e62 100644
--- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
+++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
@@ -2600,3 +2600,239 @@ error_handler:
return Status;
}
+
+/** AML code generation for a _DSD device data object.
+
+ AmlAddDeviceDataDescriptorPackage (Uuid, ParentNode, NewObjectNode) is
+ equivalent of the following ASL code:
+ ToUUID(Uuid),
+ Package () {}
+
+ @ingroup CodeGenApis
+
+ @param [in] Uuid The Uuid of the descriptor to be created
+ @param [in] DsdNode Node of the DSD Package.
+ @param [out] PackageNode If success, contains the created package node.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+**/
+EFI_STATUS
+EFIAPI
+AmlAddDeviceDataDescriptorPackage (
+ IN CONST EFI_GUID *Uuid,
+ IN AML_OBJECT_NODE_HANDLE DsdNode,
+ OUT AML_OBJECT_NODE_HANDLE *PackageNode
+ )
+{
+ EFI_STATUS Status;
+ AML_OBJECT_NODE *UuidNode;
+ AML_DATA_NODE *UuidDataNode;
+ AML_OBJECT_NODE_HANDLE DsdEntryList;
+
+ if ((Uuid == NULL) ||
+ (PackageNode == NULL) ||
+ (AmlGetNodeType ((AML_NODE_HANDLE)DsdNode) != EAmlNodeObject) ||
+ (!AmlNodeHasOpCode (DsdNode, AML_NAME_OP, 0)) ||
+ !AmlNameOpCompareName (DsdNode, "_DSD"))
+ {
+ ASSERT (0);
+ return EFI_INVALID_PARAMETER;
+ }
+
+ // Get the Package object node of the _DSD node,
+ // which is the 2nd fixed argument (i.e. index 1).
+ DsdEntryList = (AML_OBJECT_NODE_HANDLE)AmlGetFixedArgument (
+ DsdNode,
+ EAmlParseIndexTerm1
+ );
+ if ((DsdEntryList == NULL) ||
+ (AmlGetNodeType ((AML_NODE_HANDLE)DsdEntryList) != EAmlNodeObject) ||
+ (!AmlNodeHasOpCode (DsdEntryList, AML_PACKAGE_OP, 0)))
+ {
+ ASSERT (0);
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *PackageNode = NULL;
+ UuidNode = NULL;
+
+ Status = AmlCodeGenBuffer (NULL, 0, &UuidNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ Status = AmlCreateDataNode (
+ EAmlNodeDataTypeRaw,
+ (CONST UINT8 *)Uuid,
+ sizeof (EFI_GUID),
+ &UuidDataNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ Status = AmlVarListAddTail (
+ (AML_NODE_HEADER *)UuidNode,
+ (AML_NODE_HEADER *)UuidDataNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+ UuidDataNode = NULL;
+
+ // Append to the the list of _DSD entries.
+ Status = AmlVarListAddTail (
+ (AML_NODE_HANDLE)DsdEntryList,
+ (AML_NODE_HANDLE)UuidNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ Status = AmlCodeGenPackage (PackageNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ // Append to the the list of _DSD entries.
+ Status = AmlVarListAddTail (
+ (AML_NODE_HANDLE)DsdEntryList,
+ (AML_NODE_HANDLE)*PackageNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ return Status;
+
+error_handler:
+ if (UuidNode != NULL) {
+ //Need to check to detach as there is an error path that have
+ //this attached to the DsdEntry
+ if (AML_NODE_HAS_PARENT (UuidNode)) {
+ AmlDetachNode ((AML_NODE_HANDLE)UuidNode);
+ }
+ AmlDeleteTree ((AML_NODE_HANDLE)UuidNode);
+ }
+
+ if (*PackageNode != NULL) {
+ AmlDeleteTree ((AML_NODE_HANDLE)*PackageNode);
+ *PackageNode = NULL;
+ }
+ if (UuidDataNode != NULL) {
+ AmlDeleteTree ((AML_NODE_HANDLE)UuidDataNode);
+ }
+
+ return Status;
+}
+
+/** AML code generation to add a package with a name and value,
+ * to a parent package
+
+ AmlAddNameValuePackage ("Name", Value, ParentNode) is
+ equivalent of the following ASL code:
+ Package (2) {"Name", Value}
+
+ @ingroup CodeGenApis
+
+ @param [in] Name String to place in first entry of package
+ @param [in] Value Integer to place in second entry of package
+ @param [in] PackageNode Package to add new sub package to.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+**/
+EFI_STATUS
+EFIAPI
+AmlAddNameIntegerPackage (
+ IN CHAR8 *Name,
+ IN UINT64 Value,
+ IN AML_OBJECT_NODE_HANDLE PackageNode
+ )
+{
+ EFI_STATUS Status;
+ AML_OBJECT_NODE *NameNode;
+ AML_OBJECT_NODE *ValueNode;
+ AML_OBJECT_NODE *NewPackageNode;
+
+ if ((Name == NULL) ||
+ (AmlGetNodeType ((AML_NODE_HANDLE)PackageNode) != EAmlNodeObject) ||
+ (!AmlNodeHasOpCode (PackageNode, AML_PACKAGE_OP, 0)))
+ {
+ ASSERT (0);
+ return EFI_INVALID_PARAMETER;
+ }
+
+ NewPackageNode = NULL;
+ // The new package entry.
+ Status = AmlCodeGenPackage (&NewPackageNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ return Status;
+ }
+
+ Status = AmlCodeGenString (Name, &NameNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ goto error_handler;
+ }
+
+ Status = AmlVarListAddTail (
+ (AML_NODE_HANDLE)NewPackageNode,
+ (AML_NODE_HANDLE)NameNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+ NameNode = NULL;
+
+ Status = AmlCodeGenInteger (Value, &ValueNode);
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ Status = AmlVarListAddTail (
+ (AML_NODE_HANDLE)NewPackageNode,
+ (AML_NODE_HANDLE)ValueNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+ ValueNode = NULL;
+
+ Status = AmlVarListAddTail (
+ (AML_NODE_HANDLE)PackageNode,
+ (AML_NODE_HANDLE)NewPackageNode
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (0);
+ goto error_handler;
+ }
+
+ return Status;
+
+error_handler:
+ if (NewPackageNode != NULL) {
+ AmlDeleteTree ((AML_NODE_HANDLE)NewPackageNode);
+ }
+ if (NameNode != NULL) {
+ AmlDeleteTree ((AML_NODE_HANDLE)NameNode);
+ }
+ if (ValueNode != NULL) {
+ AmlDeleteTree ((AML_NODE_HANDLE)ValueNode);
+ }
+
+ return Status;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Add support to build _DSD 2022-08-05 15:14 [PATCH] DynamicTablesPkg: Add support to build _DSD Jeff Brasen @ 2022-08-12 13:36 ` PierreGondois 2022-08-15 9:49 ` Sami Mujawar 0 siblings, 1 reply; 3+ messages in thread From: PierreGondois @ 2022-08-12 13:36 UTC (permalink / raw) To: devel, jbrasen; +Cc: Sami.Mujawar, Alexei.Fedorov Hi Jeff, Please find some inline comments below: On 8/5/22 17:14, Jeff Brasen via groups.io wrote: > Add APIs needed to build _DSD with different UUIDs. > > This is per ACPI specification 6.4 s6.2.5. > > > > Adds support for building data packages with format > > Package {"Name", Integer} > > > > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com> > > --- > > .../Include/Library/AmlLib/AmlLib.h | 50 ++++ > > .../Common/AmlLib/CodeGen/AmlCodeGen.c | 236 ++++++++++++++++++ > > 2 files changed, 286 insertions(+) > > > > diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > index 6f214c0dfa..30cab3f6bb 100644 > > --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > @@ -1280,6 +1280,56 @@ AmlAddLpiState ( > > IN AML_OBJECT_NODE_HANDLE LpiNode > > ); > > > > +/** AML code generation for a _DSD device data object. > > + > > + AmlAddDeviceDataDescriptorPackage (Uuid, ParentNode, NewObjectNode) is > > + equivalent of the following ASL code: > > + ToUUID(Uuid), > > + Package () {} > > + > > + @ingroup CodeGenApis > > + > > + @param [in] Uuid The Uuid of the descriptor to be created > > + @param [in] DsdNode Node of the DSD Package. > > + @param [out] PackageNode If success, contains the created package node. > > + > > + @retval EFI_SUCCESS Success. > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +AmlAddDeviceDataDescriptorPackage ( > > + IN CONST EFI_GUID *Uuid, > > + IN AML_OBJECT_NODE_HANDLE DsdNode, > > + OUT AML_OBJECT_NODE_HANDLE *PackageNode > > + ); > > + > > +/** AML code generation to add a package with a name and value, > > + * to a parent package> > + > > + AmlAddNameValuePackage ("Name", Value, ParentNode) is > > + equivalent of the following ASL code: > > + Package (2) {"Name", Value} > > + > > + @ingroup CodeGenApis > > + > > + @param [in] Name String to place in first entry of package > > + @param [in] Value Integer to place in second entry of package > > + @param [in] PackageNode Package to add new sub package to. > > + > > + @retval EFI_SUCCESS Success. > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +AmlAddNameIntegerPackage ( > > + IN CHAR8 *Name, > > + IN UINT64 Value, > > + IN AML_OBJECT_NODE_HANDLE PackageNode > > + ); > > + > > // DEPRECATED APIS > > #ifndef DISABLE_NEW_DEPRECATED_INTERFACES > > > > diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c > > index e51d2dd7f0..80e43d0e62 100644 > > --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c > > +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c > > @@ -2600,3 +2600,239 @@ error_handler: > > > > return Status; > > } > > + > > +/** AML code generation for a _DSD device data object. > > + > > + AmlAddDeviceDataDescriptorPackage (Uuid, ParentNode, NewObjectNode) is I think this is DsdNode and PackageNode (same for the header file). > > + equivalent of the following ASL code: > > + ToUUID(Uuid), > > + Package () {} > > + > > + @ingroup CodeGenApis > > + > > + @param [in] Uuid The Uuid of the descriptor to be created > > + @param [in] DsdNode Node of the DSD Package. > > + @param [out] PackageNode If success, contains the created package node. > > + > > + @retval EFI_SUCCESS Success. > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +AmlAddDeviceDataDescriptorPackage ( > > + IN CONST EFI_GUID *Uuid, > > + IN AML_OBJECT_NODE_HANDLE DsdNode, > > + OUT AML_OBJECT_NODE_HANDLE *PackageNode > > + ) > > +{ > > + EFI_STATUS Status; > > + AML_OBJECT_NODE *UuidNode; > > + AML_DATA_NODE *UuidDataNode; > > + AML_OBJECT_NODE_HANDLE DsdEntryList; > > + > > + if ((Uuid == NULL) || > > + (PackageNode == NULL) || > > + (AmlGetNodeType ((AML_NODE_HANDLE)DsdNode) != EAmlNodeObject) || > > + (!AmlNodeHasOpCode (DsdNode, AML_NAME_OP, 0)) || > > + !AmlNameOpCompareName (DsdNode, "_DSD")) > > + { > > + ASSERT (0); This is not consistent in the file, but it could be: ASSERT_EFI_ERROR (ASSERT_EFI_ERROR) > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // Get the Package object node of the _DSD node, > > + // which is the 2nd fixed argument (i.e. index 1). > > + DsdEntryList = (AML_OBJECT_NODE_HANDLE)AmlGetFixedArgument ( > > + DsdNode, > > + EAmlParseIndexTerm1 > > + ); > > + if ((DsdEntryList == NULL) || > > + (AmlGetNodeType ((AML_NODE_HANDLE)DsdEntryList) != EAmlNodeObject) || > > + (!AmlNodeHasOpCode (DsdEntryList, AML_PACKAGE_OP, 0))) > > + { > > + ASSERT (0); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + *PackageNode = NULL; > > + UuidNode = NULL; As UuidNode is the first node allocated, we either fail and return (no need to free it), or succeed and the error handler should free it. So it is not necessary to initialize it to NULL. However UuidDataNode should be initialized to NULL. > > + > > + Status = AmlCodeGenBuffer (NULL, 0, &UuidNode); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; If it failed here then no allocation happened yet, so it could just be a return. > > + } > > + > > + Status = AmlCreateDataNode ( > > + EAmlNodeDataTypeRaw, > > + (CONST UINT8 *)Uuid, > > + sizeof (EFI_GUID), > > + &UuidDataNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } For the record, I tested the function and the Uuid is correctly generated. For Sami: would it be good to have a ToUUID() function as in ASL ? > > + > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HEADER *)UuidNode, > > + (AML_NODE_HEADER *)UuidDataNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + UuidDataNode = NULL; > > + > > + // Append to the the list of _DSD entries. > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HANDLE)DsdEntryList, > > + (AML_NODE_HANDLE)UuidNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + > > + Status = AmlCodeGenPackage (PackageNode); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + > > + // Append to the the list of _DSD entries. duplicated 'the' here and above > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HANDLE)DsdEntryList, > > + (AML_NODE_HANDLE)*PackageNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + > > + return Status; > > + > > +error_handler: > > + if (UuidNode != NULL) { > > + //Need to check to detach as there is an error path that have > > + //this attached to the DsdEntry > > + if (AML_NODE_HAS_PARENT (UuidNode)) { Would it be possible to have a second error_handler instead ? Ideally AmlCodeGen.c should not be able to see the internal node structure. With the second error handler, it should look like: Status = AmlCodeGenPackage (PackageNode); if (EFI_ERROR (Status)) { ASSERT (0); goto error_handler1; } // Append to the the list of _DSD entries. Status = AmlVarListAddTail ( (AML_NODE_HANDLE)DsdEntryList, (AML_NODE_HANDLE)*PackageNode ); if (EFI_ERROR (Status)) { ASSERT (0); goto error_handler1; } return Status; error_handler1: AmlDetachNode ((AML_NODE_HANDLE)UuidNode); error_handler: if (UuidNode != NULL) { AmlDeleteTree ((AML_NODE_HANDLE)UuidNode); } ... > > + AmlDetachNode ((AML_NODE_HANDLE)UuidNode); > > + } > > + AmlDeleteTree ((AML_NODE_HANDLE)UuidNode); > > + } > > + > > + if (*PackageNode != NULL) { > > + AmlDeleteTree ((AML_NODE_HANDLE)*PackageNode); > > + *PackageNode = NULL; > > + } > > + if (UuidDataNode != NULL) { > > + AmlDeleteTree ((AML_NODE_HANDLE)UuidDataNode); > > + } > > + > > + return Status; > > +} > > + > > +/** AML code generation to add a package with a name and value, > > + * to a parent package Is it possible to remove the '*' at the beginning of the line (same for the prototype) and also add a reference to s6.2.5 _DSD (Device Specific Data) in the two new function descriptions ? > > + > > + AmlAddNameValuePackage ("Name", Value, ParentNode) is > > + equivalent of the following ASL code: > > + Package (2) {"Name", Value} > > + > > + @ingroup CodeGenApis > > + > > + @param [in] Name String to place in first entry of package > > + @param [in] Value Integer to place in second entry of package > > + @param [in] PackageNode Package to add new sub package to. > > + > > + @retval EFI_SUCCESS Success. > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +AmlAddNameIntegerPackage ( To regroup the functions related to _DSD objects, would it be better to name this function: AmlAddDsdIntegerProperty() and the function above: AmlAddDsdDescriptor() (Sami/Jeff this is an open question) > > + IN CHAR8 *Name, > > + IN UINT64 Value, > > + IN AML_OBJECT_NODE_HANDLE PackageNode > > + ) > > +{ > > + EFI_STATUS Status; > > + AML_OBJECT_NODE *NameNode; > > + AML_OBJECT_NODE *ValueNode; > > + AML_OBJECT_NODE *NewPackageNode; > > + > > + if ((Name == NULL) || > > + (AmlGetNodeType ((AML_NODE_HANDLE)PackageNode) != EAmlNodeObject) || > > + (!AmlNodeHasOpCode (PackageNode, AML_PACKAGE_OP, 0))) > > + { > > + ASSERT (0); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + NewPackageNode = NULL; I think setting NewPackageNode to NULL is not necessary as: - if AmlCodeGenPackage() fails, we return without going to the error handler and without trying to free it - if we fail later, NameNode contains a valid node which is freed in the error handler However NameNode and ValueNode should be initialized to NULL as if AmlCodeGenString() fails, we go to the error handler and try to free non-initialized values. > > + // The new package entry. > > + Status = AmlCodeGenPackage (&NewPackageNode); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); Even though this is not consistent in the file, it could be ASSERT_EFI_ERROR (Status) (same comment for the other ASSERT). > > + return Status; > > + } > > + > > + Status = AmlCodeGenString (Name, &NameNode); > > + if (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + goto error_handler; > > + } > > + > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HANDLE)NewPackageNode, > > + (AML_NODE_HANDLE)NameNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + NameNode = NULL; > > + > > + Status = AmlCodeGenInteger (Value, &ValueNode); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HANDLE)NewPackageNode, > > + (AML_NODE_HANDLE)ValueNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + ValueNode = NULL; > > + > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HANDLE)PackageNode, > > + (AML_NODE_HANDLE)NewPackageNode > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT (0); > > + goto error_handler; > > + } > > + > > + return Status; > > + > > +error_handler: > > + if (NewPackageNode != NULL) { > > + AmlDeleteTree ((AML_NODE_HANDLE)NewPackageNode); > > + } > > + if (NameNode != NULL) { > > + AmlDeleteTree ((AML_NODE_HANDLE)NameNode); > > + } > > + if (ValueNode != NULL) { > > + AmlDeleteTree ((AML_NODE_HANDLE)ValueNode); > > + } > > + > > + return Status; > > +} > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Add support to build _DSD 2022-08-12 13:36 ` [edk2-devel] " PierreGondois @ 2022-08-15 9:49 ` Sami Mujawar 0 siblings, 0 replies; 3+ messages in thread From: Sami Mujawar @ 2022-08-15 9:49 UTC (permalink / raw) To: Pierre Gondois, devel, jbrasen; +Cc: Alexei.Fedorov, nd@arm.com Hi Jeff, Pierre, Please findmy response inline marked [SAMI]. Regards, Sami Mujawar On 12/08/2022 02:36 pm, Pierre Gondois wrote: > Hi Jeff, > Please find some inline comments below: > > On 8/5/22 17:14, Jeff Brasen via groups.io wrote: >> Add APIs needed to build _DSD with different UUIDs. >> >> This is per ACPI specification 6.4 s6.2.5. >> >> >> >> Adds support for building data packages with format >> >> Package {"Name", Integer} >> >> >> >> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com> >> >> --- >> >> .../Include/Library/AmlLib/AmlLib.h | 50 ++++ >> >> .../Common/AmlLib/CodeGen/AmlCodeGen.c | 236 ++++++++++++++++++ >> >> 2 files changed, 286 insertions(+) >> >> >> >> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h >> b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h >> >> index 6f214c0dfa..30cab3f6bb 100644 >> >> --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h >> >> +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h >> >> @@ -1280,6 +1280,56 @@ AmlAddLpiState ( >> >> IN AML_OBJECT_NODE_HANDLE LpiNode >> >> ); >> >> >> +/** AML code generation for a _DSD device data object. >> >> + >> >> + AmlAddDeviceDataDescriptorPackage (Uuid, ParentNode, >> NewObjectNode) is >> >> + equivalent of the following ASL code: >> >> + ToUUID(Uuid), >> >> + Package () {} >> >> + >> >> + @ingroup CodeGenApis >> >> + >> >> + @param [in] Uuid The Uuid of the descriptor to be created >> >> + @param [in] DsdNode Node of the DSD Package. >> >> + @param [out] PackageNode If success, contains the created >> package node. >> >> + >> >> + @retval EFI_SUCCESS Success. >> >> + @retval EFI_INVALID_PARAMETER Invalid parameter. >> >> + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. >> >> +**/ >> >> +EFI_STATUS >> >> +EFIAPI >> >> +AmlAddDeviceDataDescriptorPackage ( >> >> + IN CONST EFI_GUID *Uuid, >> >> + IN AML_OBJECT_NODE_HANDLE DsdNode, >> >> + OUT AML_OBJECT_NODE_HANDLE *PackageNode >> >> + ); >> >> + >> >> +/** AML code generation to add a package with a name and value, >> >> + * to a parent package> + >> >> + AmlAddNameValuePackage ("Name", Value, ParentNode) is >> >> + equivalent of the following ASL code: >> >> + Package (2) {"Name", Value} >> >> + >> >> + @ingroup CodeGenApis >> >> + >> >> + @param [in] Name String to place in first entry of package >> >> + @param [in] Value Integer to place in second entry of >> package >> >> + @param [in] PackageNode Package to add new sub package to. >> >> + >> >> + @retval EFI_SUCCESS Success. >> >> + @retval EFI_INVALID_PARAMETER Invalid parameter. >> >> + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. >> >> +**/ >> >> +EFI_STATUS >> >> +EFIAPI >> >> +AmlAddNameIntegerPackage ( >> >> + IN CHAR8 *Name, >> >> + IN UINT64 Value, >> >> + IN AML_OBJECT_NODE_HANDLE PackageNode >> >> + ); >> >> + >> >> // DEPRECATED APIS >> >> #ifndef DISABLE_NEW_DEPRECATED_INTERFACES >> >> >> diff --git >> a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c >> b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c >> >> index e51d2dd7f0..80e43d0e62 100644 >> >> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c >> >> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c >> >> @@ -2600,3 +2600,239 @@ error_handler: >> >> >> return Status; >> >> } >> >> + >> >> +/** AML code generation for a _DSD device data object. >> >> + >> >> + AmlAddDeviceDataDescriptorPackage (Uuid, ParentNode, >> NewObjectNode) is > > I think this is DsdNode and PackageNode (same for the header file). >> >> + equivalent of the following ASL code: >> >> + ToUUID(Uuid), >> >> + Package () {} >> >> + >> >> + @ingroup CodeGenApis >> >> + >> >> + @param [in] Uuid The Uuid of the descriptor to be created >> >> + @param [in] DsdNode Node of the DSD Package. >> >> + @param [out] PackageNode If success, contains the created >> package node. >> >> + >> >> + @retval EFI_SUCCESS Success. >> >> + @retval EFI_INVALID_PARAMETER Invalid parameter. >> >> + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. >> >> +**/ >> >> +EFI_STATUS >> >> +EFIAPI >> >> +AmlAddDeviceDataDescriptorPackage ( >> >> + IN CONST EFI_GUID *Uuid, >> >> + IN AML_OBJECT_NODE_HANDLE DsdNode, >> >> + OUT AML_OBJECT_NODE_HANDLE *PackageNode >> >> + ) >> >> +{ >> >> + EFI_STATUS Status; >> >> + AML_OBJECT_NODE *UuidNode; >> >> + AML_DATA_NODE *UuidDataNode; >> >> + AML_OBJECT_NODE_HANDLE DsdEntryList; >> >> + >> >> + if ((Uuid == NULL) || >> >> + (PackageNode == NULL) || >> >> + (AmlGetNodeType ((AML_NODE_HANDLE)DsdNode) != EAmlNodeObject) || >> >> + (!AmlNodeHasOpCode (DsdNode, AML_NAME_OP, 0)) || >> >> + !AmlNameOpCompareName (DsdNode, "_DSD")) >> >> + { >> >> + ASSERT (0); > > This is not consistent in the file, but it could be: > ASSERT_EFI_ERROR (ASSERT_EFI_ERROR) [SAMI] I am fine with using either ASSERTxxx macros. The only think that needs to be taken care of is that the errors are handled in release builds (as the ASSERTxxx macros will vanish in release builds). > >> >> + return EFI_INVALID_PARAMETER; >> >> + } >> >> + >> >> + // Get the Package object node of the _DSD node, >> >> + // which is the 2nd fixed argument (i.e. index 1). >> >> + DsdEntryList = (AML_OBJECT_NODE_HANDLE)AmlGetFixedArgument ( >> >> + DsdNode, >> >> + EAmlParseIndexTerm1 >> >> + ); >> >> + if ((DsdEntryList == >> NULL) || >> >> + (AmlGetNodeType ((AML_NODE_HANDLE)DsdEntryList) != >> EAmlNodeObject) || >> >> + (!AmlNodeHasOpCode (DsdEntryList, AML_PACKAGE_OP, 0))) >> >> + { >> >> + ASSERT (0); >> >> + return EFI_INVALID_PARAMETER; >> >> + } >> >> + >> >> + *PackageNode = NULL; >> >> + UuidNode = NULL; > > As UuidNode is the first node allocated, we either fail and return > (no need to free it), or succeed and the error handler should > free it. So it is not necessary to initialize it to NULL. > However UuidDataNode should be initialized to NULL. > >> >> + >> >> + Status = AmlCodeGenBuffer (NULL, 0, &UuidNode); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; > > If it failed here then no allocation happened yet, > so it could just be a return. > >> >> + } >> >> + >> >> + Status = AmlCreateDataNode ( >> >> + EAmlNodeDataTypeRaw, >> >> + (CONST UINT8 *)Uuid, >> >> + sizeof (EFI_GUID), >> >> + &UuidDataNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } > > For the record, I tested the function and the Uuid is correctly > generated. > For Sami: would it be good to have a ToUUID() function as in ASL ? [SAMI] Yes, that could be useful. > >> >> + >> >> + Status = AmlVarListAddTail ( >> >> + (AML_NODE_HEADER *)UuidNode, >> >> + (AML_NODE_HEADER *)UuidDataNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + UuidDataNode = NULL; >> >> + >> >> + // Append to the the list of _DSD entries. >> >> + Status = AmlVarListAddTail ( >> >> + (AML_NODE_HANDLE)DsdEntryList, >> >> + (AML_NODE_HANDLE)UuidNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + >> >> + Status = AmlCodeGenPackage (PackageNode); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + >> >> + // Append to the the list of _DSD entries. > > duplicated 'the' here and above > >> >> + Status = AmlVarListAddTail ( >> >> + (AML_NODE_HANDLE)DsdEntryList, >> >> + (AML_NODE_HANDLE)*PackageNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + >> >> + return Status; >> >> + >> >> +error_handler: >> >> + if (UuidNode != NULL) { >> >> + //Need to check to detach as there is an error path that have >> >> + //this attached to the DsdEntry >> >> + if (AML_NODE_HAS_PARENT (UuidNode)) { > > Would it be possible to have a second error_handler instead ? > Ideally AmlCodeGen.c should not be able to see the internal node > structure. > With the second error handler, it should look like: > > Status = AmlCodeGenPackage (PackageNode); > if (EFI_ERROR (Status)) { > ASSERT (0); > goto error_handler1; > } > > // Append to the the list of _DSD entries. > Status = AmlVarListAddTail ( > (AML_NODE_HANDLE)DsdEntryList, > (AML_NODE_HANDLE)*PackageNode > ); > if (EFI_ERROR (Status)) { > ASSERT (0); > goto error_handler1; > } > > return Status; > > error_handler1: > AmlDetachNode ((AML_NODE_HANDLE)UuidNode); > > error_handler: > if (UuidNode != NULL) { > AmlDeleteTree ((AML_NODE_HANDLE)UuidNode); > } > ... > > >> >> + AmlDetachNode ((AML_NODE_HANDLE)UuidNode); >> >> + } >> >> + AmlDeleteTree ((AML_NODE_HANDLE)UuidNode); >> >> + } >> >> + >> >> + if (*PackageNode != NULL) { >> >> + AmlDeleteTree ((AML_NODE_HANDLE)*PackageNode); >> >> + *PackageNode = NULL; >> >> + } >> >> + if (UuidDataNode != NULL) { >> >> + AmlDeleteTree ((AML_NODE_HANDLE)UuidDataNode); >> >> + } >> >> + >> >> + return Status; >> >> +} >> >> + >> >> +/** AML code generation to add a package with a name and value, >> >> + * to a parent package > > Is it possible to remove the '*' at the beginning of the line > (same for the prototype) and also add a reference to > s6.2.5 _DSD (Device Specific Data) in the two new function > descriptions ? > >> >> + >> >> + AmlAddNameValuePackage ("Name", Value, ParentNode) is >> >> + equivalent of the following ASL code: >> >> + Package (2) {"Name", Value} >> >> + >> >> + @ingroup CodeGenApis >> >> + >> >> + @param [in] Name String to place in first entry of package >> >> + @param [in] Value Integer to place in second entry of >> package >> >> + @param [in] PackageNode Package to add new sub package to. >> >> + >> >> + @retval EFI_SUCCESS Success. >> >> + @retval EFI_INVALID_PARAMETER Invalid parameter. >> >> + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. >> >> +**/ >> >> +EFI_STATUS >> >> +EFIAPI >> >> +AmlAddNameIntegerPackage ( > > To regroup the functions related to _DSD objects, > would it be better to name this function: > AmlAddDsdIntegerProperty() > and the function above: > AmlAddDsdDescriptor() > (Sami/Jeff this is an open question) [SAMI] Is there any other usecase where 'Package (2) {"Name", Value}' may be required? I think AmlAddNameIntegerPackage() is generic and could be useful elsewhere. Would defining AmlAddDsdIntegerProperty as a macro that maps to AmlAddNameIntegerPackage() be an alternative? We could then use the macro in the DSD specific code. [/SAMI] >> >> + IN CHAR8 *Name, >> >> + IN UINT64 Value, >> >> + IN AML_OBJECT_NODE_HANDLE PackageNode >> >> + ) >> >> +{ >> >> + EFI_STATUS Status; >> >> + AML_OBJECT_NODE *NameNode; >> >> + AML_OBJECT_NODE *ValueNode; >> >> + AML_OBJECT_NODE *NewPackageNode; >> >> + >> >> + if ((Name == NULL) || >> >> + (AmlGetNodeType ((AML_NODE_HANDLE)PackageNode) != >> EAmlNodeObject) || >> >> + (!AmlNodeHasOpCode (PackageNode, AML_PACKAGE_OP, 0))) >> >> + { >> >> + ASSERT (0); >> >> + return EFI_INVALID_PARAMETER; >> >> + } >> >> + >> >> + NewPackageNode = NULL; > > I think setting NewPackageNode to NULL is not necessary as: > - if AmlCodeGenPackage() fails, we return without going to the error > handler > and without trying to free it > - if we fail later, NameNode contains a valid node which is freed in the > error handler > > However NameNode and ValueNode should be initialized to NULL as if > AmlCodeGenString() fails, we go to the error handler and try to free > non-initialized values. > >> >> + // The new package entry. >> >> + Status = AmlCodeGenPackage (&NewPackageNode); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); > > Even though this is not consistent in the file, > it could be ASSERT_EFI_ERROR (Status) > (same comment for the other ASSERT). [SAMI] I am fine with either ASSERT versions being used. But please handle errors for release builds. > >> >> + return Status; >> >> + } >> >> + >> >> + Status = AmlCodeGenString (Name, &NameNode); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT_EFI_ERROR (Status); >> >> + goto error_handler; >> >> + } >> >> + >> >> + Status = AmlVarListAddTail ( >> >> + (AML_NODE_HANDLE)NewPackageNode, >> >> + (AML_NODE_HANDLE)NameNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + NameNode = NULL; >> >> + >> >> + Status = AmlCodeGenInteger (Value, &ValueNode); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + >> >> + Status = AmlVarListAddTail ( >> >> + (AML_NODE_HANDLE)NewPackageNode, >> >> + (AML_NODE_HANDLE)ValueNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + ValueNode = NULL; >> >> + >> >> + Status = AmlVarListAddTail ( >> >> + (AML_NODE_HANDLE)PackageNode, >> >> + (AML_NODE_HANDLE)NewPackageNode >> >> + ); >> >> + if (EFI_ERROR (Status)) { >> >> + ASSERT (0); >> >> + goto error_handler; >> >> + } >> >> + >> >> + return Status; >> >> + >> >> +error_handler: >> >> + if (NewPackageNode != NULL) { >> >> + AmlDeleteTree ((AML_NODE_HANDLE)NewPackageNode); >> >> + } >> >> + if (NameNode != NULL) { >> >> + AmlDeleteTree ((AML_NODE_HANDLE)NameNode); >> >> + } >> >> + if (ValueNode != NULL) { >> >> + AmlDeleteTree ((AML_NODE_HANDLE)ValueNode); >> >> + } >> >> + >> >> + return Status; >> >> +} >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-15 9:49 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-05 15:14 [PATCH] DynamicTablesPkg: Add support to build _DSD Jeff Brasen 2022-08-12 13:36 ` [edk2-devel] " PierreGondois 2022-08-15 9:49 ` Sami Mujawar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox