Re: [PATCH v4 2/3] DynamicTablesPkg: AML Code generation to add _CPC entries

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

Hello Jeff,
Just 2 minors comments. Maybe Sami will have more.
Otherwise, for the 3 patches:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Regards,
Pierre

On 9/20/22 00:01, Jeff Brasen wrote:
> _CPC entries can describe CPU performance information.
> The object is described in ACPI 6.4 s8.4.7.1.
> "_CPC (Continuous Performance Control)".
> 
> Add AmlCreateCpcNode() helper function to add _CPC entries to an
> existing CPU object.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>   .../Include/Library/AmlLib/AmlLib.h           |  54 ++
>   .../Common/AmlLib/CodeGen/AmlCodeGen.c        | 476 ++++++++++++++++++
>   2 files changed, 530 insertions(+)
> 
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> index 39968660f2..ebaccba811 100644
> --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h

[snip]

> +
> +/** Adds an integer or register to the package
> +
> +  @ingroup CodeGenApis
> +
> +  @param [in]  Register     If provided, register that will be added to package
> +  @param [in]  Integer      If Register is NULL, integer that will be added to the package
> +  @param [in]  PackageNode  Package to add value to
> +
> +  @retval EFI_SUCCESS             The function completed successfully.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +AmlAddRegisterOrIntegerToPackage (
> +  IN EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE  *Register OPTIONAL,
> +  IN UINT32                                  Integer,
> +  IN AML_OBJECT_NODE_HANDLE                  PackageNode
> +  )
> +{
> +  EFI_STATUS              Status;
> +  AML_OBJECT_NODE_HANDLE  IntegerNode;
> +
> +  IntegerNode = NULL;
> +

I think that with IsNullGenericAddress(), the first
  (Register != NULL)
is not necessary.

> +  if ((Register != NULL) && !IsNullGenericAddress (Register)) {
> +    Status = AmlAddRegisterToPackage (Register, PackageNode);
> +  } else {
> +    Status = AmlCodeGenInteger (Integer, &IntegerNode);
> +    if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);
> +      return Status;
> +    }
> +
> +    Status = AmlVarListAddTail (
> +               (AML_NODE_HANDLE)PackageNode,
> +               (AML_NODE_HANDLE)IntegerNode
> +               );
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    if (IntegerNode != NULL) {
> +      AmlDeleteTree ((AML_NODE_HANDLE)IntegerNode);
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +/** Create a _CPC node.
> +
> +  Creates and optionally adds the following node
> +   Name(_CPC, Package()
> +   {
> +    NumEntries,                              // Integer
> +    Revision,                                // Integer
> +    HighestPerformance,                      // Integer or Buffer (Resource Descriptor)
> +    NominalPerformance,                      // Integer or Buffer (Resource Descriptor)
> +    LowestNonlinearPerformance,              // Integer or Buffer (Resource Descriptor)
> +    LowestPerformance,                       // Integer or Buffer (Resource Descriptor)
> +    GuaranteedPerformanceRegister,           // Buffer (Resource Descriptor)
> +    DesiredPerformanceRegister ,             // Buffer (Resource Descriptor)
> +    MinimumPerformanceRegister ,             // Buffer (Resource Descriptor)
> +    MaximumPerformanceRegister ,             // Buffer (Resource Descriptor)
> +    PerformanceReductionToleranceRegister,   // Buffer (Resource Descriptor)
> +    TimeWindowRegister,                      // Buffer (Resource Descriptor)
> +    CounterWraparoundTime,                   // Integer or Buffer (Resource Descriptor)
> +    ReferencePerformanceCounterRegister,     // Buffer (Resource Descriptor)
> +    DeliveredPerformanceCounterRegister,     // Buffer (Resource Descriptor)
> +    PerformanceLimitedRegister,              // Buffer (Resource Descriptor)
> +    CPPCEnableRegister                       // Buffer (Resource Descriptor)
> +    AutonomousSelectionEnable,               // Integer or Buffer (Resource Descriptor)
> +    AutonomousActivityWindowRegister,        // Buffer (Resource Descriptor)
> +    EnergyPerformancePreferenceRegister,     // Buffer (Resource Descriptor)
> +    ReferencePerformance                     // Integer or Buffer (Resource Descriptor)
> +    LowestFrequency,                         // Integer or Buffer (Resource Descriptor)
> +    NominalFrequency                         // Integer or Buffer (Resource Descriptor)
> +  })
> +
> +  If resource buffer is NULL then integer will be used.
> +
> +  Cf. ACPI 6.4, s8.4.7.1 _CPC (Continuous Performance Control)
> +
> +  @ingroup CodeGenApis
> +
> +  @param [in]  CpcInfo               CpcInfo object
> +  @param [in]  ParentNode            If provided, set ParentNode as the parent
> +                                     of the node created.
> +  @param [out] NewCpcNode            If success and provided, contains the created node.
> +
> +  @retval EFI_SUCCESS             The function completed successfully.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +  @retval EFI_OUT_OF_RESOURCES    Failed to allocate memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AmlCreateCpcNode (
> +  IN  AML_CPC_INFO            *CpcInfo,
> +  IN  AML_NODE_HANDLE         ParentNode   OPTIONAL,
> +  OUT AML_OBJECT_NODE_HANDLE  *NewCpcNode   OPTIONAL
> +  )
> +{
> +  EFI_STATUS              Status;
> +  AML_OBJECT_NODE_HANDLE  CpcNode;
> +  AML_OBJECT_NODE_HANDLE  CpcPackage;
> +  UINT32                  NumberOfEntries;
> +
> +  if ((CpcInfo == NULL) ||
> +      ((ParentNode == NULL) && (NewCpcNode == NULL)))
> +  {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // Revision 3 per ACPI 6.4 specification
> +  if (CpcInfo->Revision == 3) {
> +    // NumEntries 23 per ACPI 6.4 specification
> +    NumberOfEntries = 23;
> +  } else {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +

It seems CpcInfo is already checked earlier. It seems the checks below could
be part of the other checks above (where ParentNode is checked).

> +  if ((CpcInfo == NULL) ||
> +      (IsNullGenericAddress (&CpcInfo->HighestPerformanceBuffer) &&
> +       (CpcInfo->HighestPerformanceInteger == 0)) ||
> +      (IsNullGenericAddress (&CpcInfo->NominalPerformanceBuffer) &&
> +       (CpcInfo->NominalPerformanceInteger == 0)) ||
> +      (IsNullGenericAddress (&CpcInfo->LowestNonlinearPerformanceBuffer) &&
> +       (CpcInfo->LowestNonlinearPerformanceInteger == 0)) ||
> +      (IsNullGenericAddress (&CpcInfo->LowestPerformanceBuffer) &&
> +       (CpcInfo->LowestPerformanceInteger == 0)) ||
> +      IsNullGenericAddress (&CpcInfo->DesiredPerformanceRegister) ||
> +      IsNullGenericAddress (&CpcInfo->ReferencePerformanceCounterRegister) ||
> +      IsNullGenericAddress (&CpcInfo->DeliveredPerformanceCounterRegister) ||
> +      IsNullGenericAddress (&CpcInfo->PerformanceLimitedRegister))
> +  {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  CpcPackage = NULL;
> +
> +  Status = AmlCodeGenNamePackage ("_CPC", NULL, &CpcNode);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
> +  }
> +

[snip]