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

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

Hello Abdul,

On 12/20/23 05:33, Attar, AbdulLateef (Abdul Lateef) wrote:
> [Public]
> 
> Hi Pierre,
>          Regarding the below comment
>> +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
> 
> I don’t think its possible because MethodInvocation is a stream of objects, and first object is NameString(method name) which is "Data node".
> AML library doesn’t allow adding variable list arguments to the "Data node", only "object node" is supported.

Yes right,

Regards,
Pierre

> 
> I'll address the remaining comments and submit the v5 patch.
> 
> Thanks
> AbduL
> 
> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Tuesday, December 19, 2023 7:41 PM
> To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io
> Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Sami Mujawar <sami.mujawar@arm.com>
> Subject: Re: [Resend PATCH v4 4/4] DynamicTablesPkg: AML Code generation to invoke a method
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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 (#112750): https://edk2.groups.io/g/devel/message/112750
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]
-=-=-=-=-=-=-=-=-=-=-=-