Re: [PATCH v3 3/3] DynamicTablesPkg: Add AmlCodeGenMethodRetInteger function

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Rebecca,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 13/01/2022 04:40 PM, Rebecca Cran wrote:
> Add AmlCodeGenMethodRetInteger function to generate AML code for
> a Method returning an Integer.
>
> Signed-off-by: Rebecca Cran <quic_rcran@quicinc.com>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h            |  47 ++++++
>   DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c | 156 ++++++++++++++++++++
>   2 files changed, 203 insertions(+)
>
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> index 80d05f74ee69..6f214c0dfad2 100644
> --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> @@ -1118,6 +1118,53 @@ AmlCodeGenMethodRetNameString (
>     OUT       AML_OBJECT_NODE_HANDLE  *NewObjectNode        OPTIONAL
>     );
>
> +/** AML code generation for a method returning an Integer.
> +
> +  AmlCodeGenMethodRetInteger (
> +    "_CBA", 0, 1, TRUE, 3, ParentNode, NewObjectNode
> +    );
> +  is equivalent of the following ASL code:
> +    Method(_CBA, 1, Serialized, 3) {
> +      Return (0)
> +    }
> +
> +  The ASL parameters "ReturnType" and "ParameterTypes" are not asked
> +  in this function. They are optional parameters in ASL.
> +
> +  @param [in]  MethodNameString     The new Method's name.
> +                                    Must be a NULL-terminated ASL NameString
> +                                    e.g.: "MET0", "_SB.MET0", etc.
> +                                    The input string is copied.
> +  @param [in]  ReturnedInteger      The value of the integer returned by the
> +                                    method.
> +  @param [in]  NumArgs              Number of arguments.
> +                                    Must be 0 <= NumArgs <= 6.
> +  @param [in]  IsSerialized         TRUE is equivalent to Serialized.
> +                                    FALSE is equivalent to NotSerialized.
> +                                    Default is NotSerialized in ASL spec.
> +  @param [in]  SyncLevel            Synchronization level for the method.
> +                                    Must be 0 <= SyncLevel <= 15.
> +                                    Default is 0 in ASL.
> +  @param [in]  ParentNode           If provided, set ParentNode as the parent
> +                                    of the node created.
> +  @param [out] NewObjectNode        If success, contains the created node.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenMethodRetInteger (
> +  IN  CONST CHAR8                   *MethodNameString,
> +  IN        UINT64                  ReturnedInteger,
> +  IN        UINT8                   NumArgs,
> +  IN        BOOLEAN                 IsSerialized,
> +  IN        UINT8                   SyncLevel,
> +  IN        AML_NODE_HANDLE         ParentNode           OPTIONAL,
> +  OUT       AML_OBJECT_NODE_HANDLE  *NewObjectNode        OPTIONAL
> +  );
> +
>   /** Create a _LPI name.
>
>     AmlCreateLpiNode ("_LPI", 0, 1, ParentNode, &LpiNode) is
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
> index 838a892c6b58..07822ead5b70 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
> @@ -1685,6 +1685,61 @@ exit_handler:
>     return Status;
>   }
>
> +/** AML code generation for a Return object node,
> +    returning an Integer.
> +
> +  AmlCodeGenReturn (0), ParentNode, NewObjectNode) is
> +  equivalent of the following ASL code:
> +    Return (0)
> +
> +  The ACPI 6.3 specification, 20.2.8 "Statement Opcodes Encoding" states:
> +    DefReturn := ReturnOp ArgObject
> +    ReturnOp := 0xA4
> +    ArgObject := TermArg => DataRefObject
> +
> +  Thus, the ReturnNode must be evaluated as a DataRefObject.
> +
> +  The ReturnNode must be generated inside a Method body scope.
> +
> +  @param [in]  Integer        The integer is returned by the Return
> +                              ASL statement.
> +  @param [in]  ParentNode     If provided, set ParentNode as the parent
> +                              of the node created.
> +                              Must be a MethodOp node.
> +  @param [out] NewObjectNode  If success, contains the created node.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenReturnInteger (
> +  IN        UINT64           Integer,
> +  IN        AML_NODE_HEADER  *ParentNode      OPTIONAL,
> +  OUT       AML_OBJECT_NODE  **NewObjectNode  OPTIONAL
> +  )
> +{
> +  EFI_STATUS       Status;
> +  AML_OBJECT_NODE  *IntNode;
> +
> +  IntNode = NULL;
> +
> +  Status = AmlCodeGenInteger (Integer, &IntNode);
> +  ASSERT_EFI_ERROR (Status);
[SAMI] In release builds, ASSERT_EFI_ERROR() would vanish and we could
end up accessing an invalid pointer.
If you agree, I will change this to check the status and return the
error code, before merging.
[/SAMI]
> +
> +  // AmlCodeGenReturn() deletes DataNode if error.
> +  Status = AmlCodeGenReturn (
> +             (AML_NODE_HEADER *)IntNode,
> +             ParentNode,
> +             NewObjectNode
> +             );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return Status;
> +}
> +
>   /** AML code generation for a method returning a NameString.
>
>     AmlCodeGenMethodRetNameString (
> @@ -1793,6 +1848,107 @@ error_handler:
>     return Status;
>   }
>
> +/** AML code generation for a method returning an Integer.
> +
> +  AmlCodeGenMethodRetInteger (
> +    "_CBA", 0, 1, TRUE, 3, ParentNode, NewObjectNode
> +    );
> +  is equivalent of the following ASL code:
> +    Method(_CBA, 1, Serialized, 3) {
> +      Return (0)
> +    }
> +
> +  The ASL parameters "ReturnType" and "ParameterTypes" are not asked
> +  in this function. They are optional parameters in ASL.
> +
> +  @param [in]  MethodNameString     The new Method's name.
> +                                    Must be a NULL-terminated ASL NameString
> +                                    e.g.: "MET0", "_SB.MET0", etc.
> +                                    The input string is copied.
> +  @param [in]  ReturnedInteger      The value of the integer returned by the
> +                                    method.
> +  @param [in]  NumArgs              Number of arguments.
> +                                    Must be 0 <= NumArgs <= 6.
> +  @param [in]  IsSerialized         TRUE is equivalent to Serialized.
> +                                    FALSE is equivalent to NotSerialized.
> +                                    Default is NotSerialized in ASL spec.
> +  @param [in]  SyncLevel            Synchronization level for the method.
> +                                    Must be 0 <= SyncLevel <= 15.
> +                                    Default is 0 in ASL.
> +  @param [in]  ParentNode           If provided, set ParentNode as the parent
> +                                    of the node created.
> +  @param [out] NewObjectNode        If success, contains the created node.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenMethodRetInteger (
> +  IN  CONST CHAR8                   *MethodNameString,
> +  IN        UINT64                  ReturnedInteger,
> +  IN        UINT8                   NumArgs,
> +  IN        BOOLEAN                 IsSerialized,
> +  IN        UINT8                   SyncLevel,
> +  IN        AML_NODE_HANDLE         ParentNode      OPTIONAL,
> +  OUT       AML_OBJECT_NODE_HANDLE  *NewObjectNode  OPTIONAL
> +  )
> +{
> +  EFI_STATUS              Status;
> +  AML_OBJECT_NODE_HANDLE  MethodNode;
> +
> +  if ((MethodNameString == NULL)  ||
> +      ((ParentNode == NULL) && (NewObjectNode == NULL)))
> +  {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // Create a Method named MethodNameString.
> +  Status = AmlCodeGenMethod (
> +             MethodNameString,
> +             NumArgs,
> +             IsSerialized,
> +             SyncLevel,
> +             NULL,
> +             &MethodNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  Status = AmlCodeGenReturnInteger (
> +             ReturnedInteger,
> +             (AML_NODE_HANDLE)MethodNode,
> +             NULL
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    goto error_handler;
> +  }
> +
> +  Status = LinkNode (
> +             MethodNode,
> +             ParentNode,
> +             NewObjectNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    goto error_handler;
> +  }
> +
> +  return Status;
> +
> +error_handler:
> +  if (MethodNode != NULL) {
> +    AmlDeleteTree ((AML_NODE_HANDLE)MethodNode);
> +  }
> +
> +  return Status;
> +}
> +
>   /** Create a _LPI name.
>
>     AmlCreateLpiNode ("_LPI", 0, 1, ParentNode, &LpiNode) is

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.