Re: [PATCH v1 03/13] DynamicTablesPkg: AML Code generation for Resource data EndTag

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

Hi Pierre,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 23/06/2021 12:40 PM, Pierre.Gondois@arm.com wrote:
> From: Pierre Gondois <Pierre.Gondois@arm.com>
>
> Add a helper function AmlCodeGenEndTag() to generate AML Resource Data
> EndTag. The EndTag resource data is automatically generated by the ASL
> compiler at the end of a list of resource data elements. Therefore, an
> equivalent function is not present in ASL.
>
> However, AmlCodeGenEndTag() is useful when generating AML code for the
> ResourceTemplate() macro.
>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>   .../AmlLib/CodeGen/AmlResourceDataCodeGen.c   | 104 ++++++++++++++++++
>   .../AmlLib/CodeGen/AmlResourceDataCodeGen.h   |  34 ++++++
>   2 files changed, 138 insertions(+)
>
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> index 07a96725a4ef..78910cc5d4b4 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> @@ -274,6 +274,110 @@ AmlCodeGenRdRegister (
>     return LinkRdNode (RdNode, NameOpNode, NewRdNode);
>   }
>   
> +/** Code generation for the EndTag resource data.
> +
> +  The EndTag resource data is automatically generated by the ASL compiler
> +  at the end of a list of resource data elements. Thus, it doesn't have
> +  a corresponding ASL function.
> +
> +  This function allocates memory to create a data node. It is the caller's
> +  responsibility to either:
> +   - attach this node to an AML tree;
> +   - delete this node.
> +
> +  @param [in]  CheckSum        CheckSum to store in the EndTag.
> +                               Optional: can be let to 0. It is not
> +                               updated when new resource data elements
> +                               are added/removed/modified in the list.
[SAMI] Can you rephrase the description, please? If I understand 
correctly, the current implementation does not compute the checksum. 
However, if the checksum value provided is not zero then an update to 
the resource nodes would need to recompute the checksum, right?
> +  @param [in]  ParentNode      If not NULL, add the generated node
> +                               to the end of the variable list of
> +                               argument of the ParentNode.
> +                               The ParentNode must not initially contain
> +                               an EndTag resource data element.
> +  @param  [out] NewRdNode      If success, contains the generated node.
> +
> +  @retval EFI_SUCCESS             The function completed successfully.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenEndTag (
> +  IN  UINT8               CheckSum,   OPTIONAL
> +  IN  AML_OBJECT_NODE   * ParentNode, OPTIONAL
> +  OUT AML_DATA_NODE    ** NewRdNode   OPTIONAL
> +  )
> +{
> +  EFI_STATUS                      Status;
> +  AML_DATA_NODE                 * RdNode;
> +  EFI_ACPI_END_TAG_DESCRIPTOR     EndTag;
> +  ACPI_SMALL_RESOURCE_HEADER      SmallResHdr;
> +
> +  if ((ParentNode == NULL) && (NewRdNode == NULL)) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  RdNode = NULL;
> +
> +  // Header
> +  SmallResHdr.Bits.Length = sizeof (EFI_ACPI_END_TAG_DESCRIPTOR) -
> +                              sizeof (ACPI_SMALL_RESOURCE_HEADER);
> +  SmallResHdr.Bits.Name = ACPI_SMALL_END_TAG_DESCRIPTOR_NAME;
> +  SmallResHdr.Bits.Type = ACPI_SMALL_ITEM_FLAG;
> +
> +  // Body
> +  EndTag.Desc = SmallResHdr.Byte;
> +  EndTag.Checksum = CheckSum;
> +
> +  Status = AmlCreateDataNode (
> +             EAmlNodeDataTypeResourceData,
> +             (UINT8*)&EndTag,
> +             sizeof (EFI_ACPI_END_TAG_DESCRIPTOR),
> +             &RdNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  if (NewRdNode != NULL) {
> +    *NewRdNode = RdNode;
> +  }
> +
> +  if (ParentNode != NULL) {
> +    // Check the BufferOp doesn't contain any resource data yet.
> +    // This is a hard check: do not allow to add an EndTag if the BufferNode
> +    // already has resource data elements attached. Indeed, the EndTag should
> +    // have already been added.
> +    if (AmlGetNextVariableArgument ((AML_NODE_HEADER*)ParentNode, NULL) !=
> +          NULL) {
> +      ASSERT (0);
> +      Status = EFI_INVALID_PARAMETER;
> +      goto error_handler;
> +    }
> +
> +    // Manually add the EndTag RdNode. Indeed, the AmlAppendRdNode function
> +    // is looking for an EndTag, which we are adding here.
> +    Status = AmlVarListAddTail (
> +               (AML_NODE_HEADER*)ParentNode,
> +               (AML_NODE_HEADER*)RdNode
> +               );
> +    if (EFI_ERROR (Status)) {
> +      ASSERT (0);
> +      goto error_handler;
> +    }
> +  }
> +
> +  return Status;
> +
> +error_handler:
> +  if (RdNode != NULL) {
> +    AmlDeleteTree ((AML_NODE_HEADER*)RdNode);
> +  }
> +  return Status;
> +}
> +
>   // DEPRECATED APIS
>   #ifndef DISABLE_NEW_DEPRECATED_INTERFACES
>   
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h
> index 3c9217d9ddab..0b464305da40 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h
> @@ -104,4 +104,38 @@ AmlCodeGenRdRegister (
>     OUT AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
>     );
>   
> +/** Code generation for the EndTag resource data.
> +
> +  The EndTag resource data is automatically generated by the ASL compiler
> +  at the end of a list of resource data elements. Thus, it doesn't have
> +  a corresponding ASL function.
> +
> +  This function allocates memory to create a data node. It is the caller's
> +  responsibility to either:
> +   - attach this node to an AML tree;
> +   - delete this node.
> +
> +  @param [in]  CheckSum        CheckSum to store in the EndTag.
> +                               Optional: can be let to 0. It is not
> +                               updated when new resource data elements
> +                               are added/removed/modified in the list.
> +  @param [in]  ParentNode      If not NULL, add the generated node
> +                               to the end of the variable list of
> +                               argument of the ParentNode.
> +                               The ParentNode must not initially contain
> +                               an EndTag resource data element.
> +  @param  [out] NewRdNode      If success, contains the generated node.
> +
> +  @retval EFI_SUCCESS             The function completed successfully.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AmlCodeGenEndTag (
> +  IN  UINT8               CheckSum,   OPTIONAL
> +  IN  AML_OBJECT_NODE   * ParentNode, OPTIONAL
> +  OUT AML_DATA_NODE    ** NewRdNode   OPTIONAL
> +  );
> +
>   #endif // AML_RESOURCE_DATA_CODE_GEN_H_