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

["Abdul Lateef Attar via groups.io" <AbdulLateef.Attar=amd.com@groups.io>]

[AMD Official Use Only - General]

Hi Sami,
        Thanks for the in-depth review and making the changes.
Regarding below comment.
[SAMI] This does not seem to be related to this patch. Were you getting any issues when building? If so, can you submit a separate patch, please?

If you agree, I will drop the above line from this patch and merge.

[/SAMI]

[Abdul] With VS2019 I am seeing compilation error, I'll submit a separate patch for this.

Thanks
AbduL


-----Original Message-----
From: Sami Mujawar <sami.mujawar@arm.com>
Sent: Thursday, December 21, 2023 7:26 PM
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io
Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Pierre Gondois <pierre.gondois@arm.com>; nd@arm.com
Subject: Re: [Resend PATCH v5 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.


Hi Abdul,

I have some minor feedback maked inline as [SAMI].

I think other than the typecast change for the value returned by StrSize(), the remainder of the patch looks ok to me (i.e. with the minor changes I suggested).

Please let me know if you agree with my suggestions and I will make the changes and get this patch merged.

Regards,

Sami Mujawar

On 20/12/2023 09:38 am, 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        | 235 +++++++++++++++++-
>   2 files changed, 346 insertions(+), 1 deletion(-)
>
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> index eb8740692f..043ec3d842 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 a 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 to 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 to 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 to the following ASL code:
> +    Method (MET2, 2, Serialized)
> +    {
> +      MET3 (Arg0, 0x0100)
> +    }
> +
> +  @param [in] MethodNameString  The method name to be called or 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        The parent node to which the method invocation
> +                                nodes are attached.
> +
> +  @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..ddec9b67ed 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),

[SAMI] This does not seem to be related to this patch. Were you getting any issues when building? If so, can you submit a separate patch, please?

If you agree, I will drop the above line from this patch and merge.

[/SAMI]

>                &DataNode
>                );
>     if (EFI_ERROR (Status)) {
> @@ -3849,3 +3850,235 @@ exit_handler:
>
>     return Status;
>   }
> +
> +/** AML code generation to invoke/call another method.
> +
> +  This method is a 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 to 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 to 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 to the following ASL code:
> +    Method (MET2, 2, Serialized)
> +    {
> +      MET3 (Arg0, 0x0100)
> +    }
> +
> +  @param [in] MethodNameString  The method name to be called or 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        The parent node to which the method invocation
> +                                nodes are attached.
> +
> +  @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
> +  )
> +{
> +  EFI_STATUS       Status;
> +  UINT8            Index;
> +  CHAR8            *AmlNameString;
> +  UINT32           AmlNameStringSize;
> +  AML_DATA_NODE    *DataNode;
> +  AML_OBJECT_NODE  *ObjectNode;
> +  AML_NODE_HANDLE  *NodeStream;
> +
> +  if ((MethodNameString == NULL) || (ParentNode == NULL)) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((NumArgs > 7) ||
> +      ((Parameters == NULL) && (NumArgs > 0)))  {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  /// Allocate space to store methodname, object, data node pointers
> + NodeStream = AllocateZeroPool (sizeof (AML_NODE_HANDLE) * (NumArgs +
> + 1));  if (NodeStream == NULL) {
> +    ASSERT (0);
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  /// Create a called or 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;
> +  }
> +
> +  NodeStream[0] = (AML_NODE_HANDLE)DataNode;
> +
> +  if (Parameters != NULL) {
> +    /// Validate and convert the Parameters to the stream of nodes.
> +    for (Index = 0; Index < NumArgs; Index++) {
> +      switch (Parameters[Index].Type) {
> +        case AmlMethodParamTypeInteger:
> +          ObjectNode = NULL;
[SAMI] I think the above line can be moved right after the 'for'
statement. If you agree I can fix this before merging.
> +          Status     = AmlCodeGenInteger (Parameters[Index].Data.Integer, &ObjectNode);
> +          if (EFI_ERROR (Status)) {
> +            ASSERT_EFI_ERROR (Status);
> +            goto exit_handler;
> +          }
> +
> +          NodeStream[Index+1] = (AML_NODE_HANDLE)ObjectNode;
[SAMI] Minor, a space is required before and after '+'. I will fix that before merging.
> +          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);
[SAMI] Minor, I prefer to keep the code within 80 characters. I will fix that before merging.
> +          if (EFI_ERROR (Status)) {
> +            ASSERT_EFI_ERROR (Status);
> +            goto exit_handler;
> +          }
> +
> +          NodeStream[Index+1] = (AML_NODE_HANDLE)ObjectNode;
[SAMI] I believe the above line can be moved after the switch block. I can do that if you agree.
> +          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;
> +          }
> +
> +          NodeStream[Index+1] = (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;
> +          }
> +
> +          NodeStream[Index+1] = (AML_NODE_HANDLE)ObjectNode;
> +          break;
> +        default:
> +          ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> +          Status = EFI_INVALID_PARAMETER;
> +          goto exit_handler;
> +          break;
> +      }
> +    }
> +  }
> +
> +  /// Index <= NumArgs, because an additional method name was added.
> +  for (Index = 0; Index <= NumArgs; Index++) {
> +    Status = AmlVarListAddTail (
> +               (AML_NODE_HANDLE)ParentNode,
> +                (AML_NODE_HANDLE)NodeStream[Index]
> +               );
> +    if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);
> +      goto exit_handler_detach;
> +    }
> +  }
> +
> +  FreePool (NodeStream);
> +  return Status;
> +
> +exit_handler_detach:
> +  /// The index contains the last successful node attached.
> +  for ( ; Index > 0; Index--) {
> +    /// Index contains the node number that is failed for AmlVarListAddTail().
> +    /// Hence, start detaching from the last successful
> +    AmlDetachNode (NodeStream[Index-1]);
> +  }
> +
> +exit_handler:
> +  /// Index <= NumArgs, because an additional method name was added.
> +  for (Index = 0; Index <= NumArgs; Index++) {
> +    if (NodeStream[Index] != 0) {
> +      AmlDeleteTree (NodeStream[Index]);
> +    }
> +  }
> +
> +  FreePool (NodeStream);
> +  return Status;
> +}


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