Re: [edk2-devel] [Resend PATCH v4 4/4] DynamicTablesPkg: AML Code generation to invoke a method

["PierreGondois" <pierre.gondois@arm.com>]

Hello Abdul,
The new function looks good, there is just one small issue with
the tree layout,

Regards,
Pierre

On 12/19/23 04:06, Abdul Lateef Attar wrote:
> From: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> 
> Adds API to generate AML code to invoke/call another
> method. Also provides ability to pass arguments of
> type integer, string, ArgObj or LocalObj.
> 
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> ---
>   .../Include/Library/AmlLib/AmlLib.h           | 112 +++++++++
>   .../Common/AmlLib/CodeGen/AmlCodeGen.c        | 236 +++++++++++++++++-
>   2 files changed, 347 insertions(+), 1 deletion(-)
> 
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> index eb8740692f..fb16637f02 100644
> --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> @@ -101,6 +101,56 @@ typedef enum {
>     AmlAddressRangeMax      = 4
>   } AML_MEMORY_ATTRIBUTES_MTP;
>   
> +/** Method parameter types
> +
> +  Possible values are:
> +    0 - AmlMethodParamTypeInteger
> +    1 - AmlMethodParamTypeString
> +    2 - AmlMethodParamTypeArg
> +    3 - AmlMethodParamTypeLocal
> +
> +  @par Reference(s)
> +  - ACPI 6.5, s20.2.5 "Term Objects Encoding"
> +
> +**/
> +typedef enum {
> +  AmlMethodParamTypeInteger = 0,
> +  AmlMethodParamTypeString  = 1,
> +  AmlMethodParamTypeArg     = 2,
> +  AmlMethodParamTypeLocal   = 3
> +} AML_METHOD_PARAM_TYPE;
> +
> +/** AML Method parameter data
> +  holds the AML method parameter data.
> +**/
> +typedef union {
> +  UINT8     Arg;
> +  UINT8     Local;
> +  UINT64    Integer;
> +  VOID      *Buffer;
> +} AML_METHOD_PARAM_DATA;
> +
> +/** structure to hold AML method parameter types
> +  Type  -   Type of parameter
> +  Data  -   holds data of parameter
> +            if Type is AmlMethodParamTypeInteger
> +              then Data is of type Integer to hold integer value.
> +            if Type is AmlMethodParamTypeString
> +              then Data contains null terminated string.
> +            If Type is AmlMethodParamTypeArg
> +              then Data contains the Argument number,
> +              0 to 6 are supported value.
> +            If Type is AmlMethodParamTypeLocal
> +              then Data contains the Local variable number,
> +              0 to 7 are supported value.
> +  DataSize - for future use
> +**/
> +typedef struct {
> +  AML_METHOD_PARAM_TYPE    Type;
> +  AML_METHOD_PARAM_DATA    Data;
> +  UINTN                    DataSize;
> +} AML_METHOD_PARAM;
> +
>   /** Parse the definition block.
>   
>     The function parses the whole AML blob. It starts with the ACPI DSDT/SSDT
> @@ -1693,4 +1743,66 @@ AmlAddNameStringToNamedPackage (
>     IN AML_OBJECT_NODE_HANDLE  NamedNode
>     );
>   
> +/** AML code generation to invoke/call another method.
> +
> +  This method is subset implementation of MethodInvocation
> +  defined in the ACPI specification 6.5,
> +  section 20.2.5 "Term Objects Encoding".
> +  Added integer, string, ArgObj and LocalObj support.
> +
> +  Example 1:
> +    AmlCodeGenInvokeMethod ("MET0", 0, NULL, ParentNode);
> +    is equivalent of the following ASL code:
> +      MET0 ();
> +
> +  Example 2:
> +    AML_METHOD_PARAM  Param[4];
> +    Param[0].Data.Integer = 0x100;
> +    Param[0].Type = AmlMethodParamTypeInteger;
> +    Param[1].Data.Buffer = "TEST";
> +    Param[1].Type = AmlMethodParamTypeString;
> +    Param[2].Data.Arg = 0;
> +    Param[2].Type = AmlMethodParamTypeArg;
> +    Param[3].Data.Local = 2;
> +    Param[3].Type = AmlMethodParamTypeLocal;
> +    AmlCodeGenInvokeMethod ("MET0", 4, Param, ParentNode);
> +
> +    is equivalent of the following ASL code:
> +      MET0 (0x100, "TEST", Arg0, Local2);
> +
> +  Example 3:
> +    AML_METHOD_PARAM  Param[2];
> +    Param[0].Data.Arg = 0;
> +    Param[0].Type = AmlMethodParamTypeArg;
> +    Param[1].Data.Integer = 0x100;
> +    Param[1].Type = AmlMethodParamTypeInteger;
> +    AmlCodeGenMethodRetNameString ("MET2", NULL, 2, TRUE, 0, ParentNode, &MethodNode);
> +    AmlCodeGenInvokeMethod ("MET3", 2, Param, MethodNode);
> +
> +    is equivalent of the following ASL code:
> +    Method (MET2, 2, Serialized)
> +    {
> +      MET3 (Arg0, 0x0100)
> +    }
> +
> +  @param [in] MethodNameString  Method name to be called/invoked.
> +  @param [in] NumArgs           Number of arguments to be passed,
> +                                0 to 7 are permissible values.
> +  @param [in] Parameters        contains the parameter data.
> +  @param [in] ParentNode        set ParentNode as the parent> +                                of the node created.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> + **/
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenInvokeMethod (
> +  IN  CONST CHAR8             *MethodNameString,
> +  IN        UINT8             NumArgs,
> +  IN        AML_METHOD_PARAM  *Parameters   OPTIONAL,
> +  IN        AML_NODE_HANDLE   ParentNode
> +  );
> +
>   #endif // AML_LIB_H_
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
> index a6db34fb97..688eefdcef 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
> @@ -2,6 +2,7 @@
>     AML Code Generation.
>   
>     Copyright (c) 2020 - 2022, Arm Limited. All rights reserved.<BR>
> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   **/
> @@ -921,7 +922,7 @@ AmlCodeGenNameUnicodeString (
>     Status = AmlCreateDataNode (
>                EAmlNodeDataTypeRaw,
>                (CONST UINT8 *)String,
> -             StrSize (String),
> +             (UINT32)StrSize (String),
>                &DataNode
>                );
>     if (EFI_ERROR (Status)) {
> @@ -3849,3 +3850,236 @@ exit_handler:
>   
>     return Status;
>   }
> +
> +/** AML code generation to invoke/call another method.
> +
> +  This method is subset implementation of MethodInvocation
> +  defined in the ACPI specification 6.5,
> +  section 20.2.5 "Term Objects Encoding".
> +  Added integer, string, ArgObj and LocalObj support.
> +
> +  Example 1:
> +    AmlCodeGenInvokeMethod ("MET0", 0, NULL, ParentNode);
> +    is equivalent of the following ASL code:
> +      MET0 ();
> +
> +  Example 2:
> +    AML_METHOD_PARAM  Param[4];
> +    Param[0].Data.Integer = 0x100;
> +    Param[0].Type = AmlMethodParamTypeInteger;
> +    Param[1].Data.Buffer = "TEST";
> +    Param[1].Type = AmlMethodParamTypeString;
> +    Param[2].Data.Arg = 0;
> +    Param[2].Type = AmlMethodParamTypeArg;
> +    Param[3].Data.Local = 2;
> +    Param[3].Type = AmlMethodParamTypeLocal;
> +    AmlCodeGenInvokeMethod ("MET0", 4, Param, ParentNode);
> +
> +    is equivalent of the following ASL code:
> +      MET0 (0x100, "TEST", Arg0, Local2);
> +
> +  Example 3:
> +    AML_METHOD_PARAM  Param[2];
> +    Param[0].Data.Arg = 0;
> +    Param[0].Type = AmlMethodParamTypeArg;
> +    Param[1].Data.Integer = 0x100;
> +    Param[1].Type = AmlMethodParamTypeInteger;
> +    AmlCodeGenMethodRetNameString ("MET2", NULL, 2, TRUE, 0, ParentNode, &MethodNode);
> +    AmlCodeGenInvokeMethod ("MET3", 2, Param, MethodNode);
> +
> +    is equivalent of the following ASL code:
> +    Method (MET2, 2, Serialized)
> +    {
> +      MET3 (Arg0, 0x0100)
> +    }
> +
> +  @param [in] MethodNameString  Method name to be called/invoked.
> +  @param [in] NumArgs           Number of arguments to be passed,
> +                                0 to 7 are permissible values.
> +  @param [in] Parameters        contains the parameter data.
> +  @param [in] ParentNode        set ParentNode as the parent

NIT: set->Set

> +                                of the node created.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> + **/
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenInvokeMethod (
> +  IN  CONST CHAR8             *MethodNameString,
> +  IN        UINT8             NumArgs,
> +  IN        AML_METHOD_PARAM  *Parameters   OPTIONAL,
> +  IN        AML_NODE_HANDLE   ParentNode

Maybe it would be worth also having an OUT parameter in case
someone doesn't want to attach the method invocation directly
(cf. other methods in the file):
   OUT AML_OBJECT_NODE_HANDLE  *NewObjectNode   OPTIONAL


> +  )
> +{
> +  EFI_STATUS       Status;
> +  UINT8            Index;
> +  CHAR8            *AmlNameString;
> +  UINT32           AmlNameStringSize;
> +  AML_DATA_NODE    *DataNode;
> +  AML_OBJECT_NODE  *IntNode;
> +  AML_NODE_HANDLE  HeadNode;
> +  AML_NODE_HANDLE  CurrentNode;
> +  AML_OBJECT_NODE  *ObjectNode;
> +
> +  if ((MethodNameString == NULL) || (ParentNode == NULL)) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((NumArgs > 7) ||
> +      ((Parameters == NULL) && (NumArgs > 0)))
> +  {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Parameters != NULL) {
> +    /// Validate and convert the Parameters to the stream of node
> +    HeadNode    = NULL;
> +    CurrentNode = NULL;
> +    for (Index = 0; Index < NumArgs; Index++) {
> +      switch (Parameters[Index].Type) {
> +        case AmlMethodParamTypeInteger:
> +          IntNode = NULL;
> +          Status  = AmlCodeGenInteger (Parameters[Index].Data.Integer, &IntNode);
> +          if (EFI_ERROR (Status)) {
> +            ASSERT_EFI_ERROR (Status);
> +            goto exit_handler;
> +          }
> +
> +          CurrentNode = (AML_NODE_HANDLE)IntNode;
> +          break;
> +        case AmlMethodParamTypeString:
> +          ObjectNode = NULL;
> +          if (Parameters[Index].Data.Buffer == NULL) {
> +            ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> +            Status = EFI_INVALID_PARAMETER;
> +            goto exit_handler;
> +          }
> +
> +          Status = AmlCodeGenString (Parameters[Index].Data.Buffer, &ObjectNode);
> +          if (EFI_ERROR (Status)) {
> +            ASSERT_EFI_ERROR (Status);
> +            goto exit_handler;
> +          }
> +
> +          CurrentNode = (AML_NODE_HANDLE)ObjectNode;
> +          break;
> +        case AmlMethodParamTypeArg:
> +          ObjectNode = NULL;
> +          if (Parameters[Index].Data.Arg > (UINT8)(AML_ARG6 - AML_ARG0)) {
> +            ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> +            Status = EFI_INVALID_PARAMETER;
> +            goto exit_handler;
> +          }
> +
> +          Status = AmlCreateObjectNode (
> +                     AmlGetByteEncodingByOpCode (AML_ARG0 + Parameters[Index].Data.Arg, 0),
> +                     0,
> +                     &ObjectNode
> +                     );
> +          if (EFI_ERROR (Status)) {
> +            ASSERT_EFI_ERROR (Status);
> +            goto exit_handler;
> +          }
> +
> +          CurrentNode = (AML_NODE_HANDLE)ObjectNode;
> +          break;
> +        case AmlMethodParamTypeLocal:
> +          ObjectNode = NULL;
> +          if (Parameters[Index].Data.Local > (UINT8)(AML_LOCAL7 - AML_LOCAL0)) {
> +            ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> +            Status = EFI_INVALID_PARAMETER;
> +            goto exit_handler;
> +          }
> +
> +          Status = AmlCreateObjectNode (
> +                     AmlGetByteEncodingByOpCode (AML_LOCAL0 + Parameters[Index].Data.Local, 0),
> +                     0,
> +                     &ObjectNode
> +                     );
> +          if (EFI_ERROR (Status)) {
> +            ASSERT_EFI_ERROR (Status);
> +            goto exit_handler;
> +          }
> +
> +          CurrentNode = (AML_NODE_HANDLE)ObjectNode;
> +          break;
> +        default:
> +          ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> +          Status = EFI_INVALID_PARAMETER;
> +          goto exit_handler;
> +          break;
> +      }
> +
> +      if (HeadNode == NULL) {
> +        HeadNode = CurrentNode;

I think there is an issue here. The method arguments should be attached
to the variable list of arguments of the parent, not of another argument.

For instance, the following:
Scope (_SB) {
   MET0 (Arg0, Arg1, Arg2)
}

Should create a tree as:
_SB
\- (Variable list of arguments of _SB)
   - MET0
   - Arg0
   - Arg1
   - Arg2

With the present code, the created tree is:
_SB
\- (Variable list of arguments of _SB)
   - MET0
   - Arg0
   \- (Variable list of arguments of Arg0)
     - Arg1
     - Arg2

When serializing the tree, the generated binary is identical, but
the tree is incorrect.

> +      } else {
> +        Status = AmlVarListAddTail (
> +                   HeadNode,
> +                   CurrentNode
> +                   );
> +        if (EFI_ERROR (Status)) {
> +          ASSERT_EFI_ERROR (Status);
> +          goto exit_handler;
> +        }
> +      }
> +    }
> +  }
> +
> +  /// Create called/invoked method name string
> +  Status = ConvertAslNameToAmlName (MethodNameString, &AmlNameString);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlGetNameStringSize (AmlNameString, &AmlNameStringSize);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    FreePool (AmlNameString);
> +    goto exit_handler;
> +  }
> +
> +  DataNode = NULL;
> +  Status   = AmlCreateDataNode (
> +               EAmlNodeDataTypeNameString,
> +               (UINT8 *)AmlNameString,
> +               AmlNameStringSize,
> +               &DataNode
> +               );
> +  FreePool (AmlNameString);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlVarListAddTail (
> +             (AML_NODE_HANDLE)ParentNode,
> +             (AML_NODE_HANDLE)DataNode
> +             );
> +  if (EFI_ERROR (Status)) {

I think DataNode would not be freed if we fail here.

> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlVarListAddTail (
> +             (AML_NODE_HANDLE)ParentNode,
> +             HeadNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  return Status;
> +
> +exit_handler:
> +  if (HeadNode != NULL) {
> +    AmlDeleteTree (HeadNode);
> +  }
> +
> +  return Status;
> +}


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112732): https://edk2.groups.io/g/devel/message/112732
Mute This Topic: https://groups.io/mt/103256864/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-