[edk2-devel] [PATCH 2/3] DynamicTablesPkg: AML Code generation to add _CPC entries
PierreGondois
pierre.gondois at arm.com
Mon Sep 12 10:36:08 UTC 2022
Hello Jeff,
Please find some remarks inline:
On 9/7/22 17:15, Jeff Brasen via groups.io 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 at nvidia.com>
>
> ---
>
> .../Include/Library/AmlLib/AmlLib.h | 155 +++++
>
> .../Common/AmlLib/CodeGen/AmlCodeGen.c | 549 ++++++++++++++++++
>
> 2 files changed, 704 insertions(+)
>
>
>
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
>
> index 39968660f2..65f6cd7583 100644
>
[snip]
>
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
>
> index 5fb39d077b..dfc3015baf 100644
>
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
>
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c
>
> @@ -2850,3 +2850,552 @@ error_handler:
>
>
>
> return Status;
>
> }
>
> +
>
> +/** Adds a register to the package
>
> +
>
> + @ingroup CodeGenApis
>
> +
>
> + @param [in] Register If provided, register that will be added to package.
>
> + otherwise NULL register will be added
>
> + @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
>
> +AmlAddRegisterToPackage (
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *Register OPTIONAL,
>
> + IN AML_OBJECT_NODE_HANDLE PackageNode
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + AML_DATA_NODE_HANDLE RdNode;
>
> + AML_OBJECT_NODE_HANDLE ResourceTemplateNode;
>
> +
>
> + RdNode = NULL;
>
> + ResourceTemplateNode = NULL;
I don't think it would be necessary to set ResourceTemplateNode
if the 'goto error_handler;' just above was replaced by a 'return Status;'.
>
> +
>
> + Status = AmlCodeGenResourceTemplate (&ResourceTemplateNode);
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
I know this is not consistent in the file, but would it be possible
to use:
ASSERT_EFI_ERROR (Status);
when a Status is available ? This comment can be applied to other places.
For this specifc error handling, I think we can just return without
going to the error hangler.
>
> + goto error_handler;
>
> + }
>
> +
>
> + if (Register != NULL) {
>
> + Status = AmlCodeGenRdRegister (
>
> + Register->AddressSpaceId,
>
> + Register->RegisterBitWidth,
>
> + Register->RegisterBitOffset,
>
> + Register->Address,
>
> + Register->AccessSize,
>
> + NULL,
>
> + &RdNode
>
> + );
>
> + } else {
>
> + Status = AmlCodeGenRdRegister (
>
> + EFI_ACPI_6_4_SYSTEM_MEMORY,
>
> + 0,
>
> + 0,
>
> + 0,
>
> + 0,
>
> + NULL,
>
> + &RdNode
>
> + );
>
> + }
>
> +
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
> + Status = AmlAppendRdNode (ResourceTemplateNode, RdNode);
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
> + RdNode = NULL;
>
> +
>
> + Status = AmlVarListAddTail (
>
> + (AML_NODE_HANDLE)PackageNode,
>
> + (AML_NODE_HANDLE)ResourceTemplateNode
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
> + ResourceTemplateNode = NULL;
I don't think it's necessary.
>
> +
>
> + return Status;
>
> +
>
> +error_handler:
>
> + if (RdNode != NULL) {
>
> + AmlDeleteTree ((AML_NODE_HANDLE)RdNode);
>
> + }
>
> +
>
> + if (ResourceTemplateNode != NULL) {
>
> + AmlDeleteTree ((AML_NODE_HANDLE)ResourceTemplateNode);
>
> + }
>
> +
>
> + return Status;
>
> +}
>
> +
>
> +/** 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_3_GENERIC_ADDRESS_STRUCTURE *Register OPTIONAL,
>
> + IN UINT32 Integer,
>
> + IN AML_OBJECT_NODE_HANDLE PackageNode
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + AML_OBJECT_NODE_HANDLE IntegerNode;
>
> +
>
> + IntegerNode = NULL;
>
> +
>
> + if (Register != NULL) {
>
> + Status = AmlAddRegisterToPackage (Register, PackageNode);
>
> + } else {
>
> + Status = AmlCodeGenInteger (Integer, &IntegerNode);
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
I think this could just be a 'return Status;'
>
> + }
>
> +
>
> + Status = AmlVarListAddTail (
>
> + (AML_NODE_HANDLE)PackageNode,
>
> + (AML_NODE_HANDLE)IntegerNode
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
This should be out of the 'else' statement to assert if
AmlAddRegisterToPackage() fails, as:
if () {
Status = AmlAddRegisterToPackage ();
} else {
...
Status = AmlVarListAddTail ();
}
if (EFI_ERROR (Status)) {
}
>
> +
>
> + IntegerNode = NULL;
I don't think it's necessary.
>
> + }
>
> +
>
> + return Status;
>
> +
>
> +error_handler:
>
> + 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] HighestPerformanceBuffer If provided, buffer that indicates the highest level
>
> + of performance the processor.
>
> + @param [in] HighestPerformanceInteger Indicates the highest level of performance the processor,
>
> + used if buffer is NULL.
>
> + @param [in] NominalPerformanceBuffer If provided buffer that indicates the highest sustained
>
> + performance level of the processor.
>
> + @param [in] NominalPerformanceInteger Indicates the highest sustained performance level
>
> + of the processor, used if buffer is NULL.
>
> + @param [in] LowestNonlinearPerformanceBuffer If provided, buffer that indicates the lowest performance level
>
> + of the processor with non-linear power savings.
>
> + @param [in] LowestNonlinearPerformanceInteger Indicates the lowest performance level of the processor with
>
> + non-linear power savings, used if buffer is NULL.
>
> + @param [in] LowestPerformanceBuffer If provided, buffer that indicates the
>
> + lowest performance level of the processor.
>
> + @param [in] LowestPerformanceInteger Indicates the lowest performance level of the processor,
>
> + used if buffer is NULL.
>
> + @param [in] GuaranteedPerformanceRegister If provided, Guaranteed Performance Register Buffer.
>
> + @param [in] DesiredPerformanceRegister If provided, Desired Performance Register Buffer.
>
> + @param [in] MinimumPerformanceRegister If provided, Minimum Performance Register Buffer.
>
> + @param [in] MaximumPerformanceRegister If provided, Maximum Performance Register Buffer.
>
> + @param [in] PerformanceReductionToleranceRegister If provided, Performance Reduction Tolerance Register.
>
> + @param [in] TimeWindowRegister If provided, Time Window Register.
>
> + @param [in] CounterWraparoundTimeBuffer If provided, Counter Wraparound Time buffer.
>
> + @param [in] CounterWraparoundTimeInteger Counter Wraparound Time, used if buffer is NULL.
>
> + @param [in] ReferencePerformanceCounterRegister Reference Performance Counter Register.
>
> + @param [in] DeliveredPerformanceCounterRegister Delivered Performance Counter Register.
>
> + @param [in] PerformanceLimitedRegister Performance Limited Register.
>
> + @param [in] CPPCEnableRegister If provided, CPPC EnableRegister.
>
> + @param [in] AutonomousSelectionEnableBuffer If provided, Autonomous Selection Enable buffer.
>
> + @param [in] AutonomousSelectionEnableInteger Autonomous Selection Enable, used if buffer is NULL.
>
> + @param [in] AutonomousActivityWindowRegister If provided, AutonomousActivity-WindowRegister.
>
> + @param [in] EnergyPerformancePreferenceRegister If provided, EnergyPerformance-PreferenceRegister.
>
> + @param [in] ReferencePerformanceBuffer If provided, Reference Performance buffer.
>
> + @param [in] ReferencePerformanceInteger Reference Performance, used if buffer is NULL.
>
> + @param [in] LowestFrequencyBuffer If provided, Lowest Frequency buffer.
>
> + @param [in] LowestFrequencyInteger Lowest Frequency, used if buffer is NULL.
>
> + @param [in] NominalFrequencyBuffer If provided, NominalFrequencyBuffer buffer.
>
> + @param [in] NominalFrequencyInteger NominalFrequencyBuffer, used if buffer is NULL.
>
> + @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 (
>
> + /// Indicates the highest level of performance the processor
>
> + /// is theoretically capable of achieving.
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *HighestPerformanceBuffer OPTIONAL,
>
> + IN UINT32 HighestPerformanceInteger,
>
> + /// Indicates the highest sustained performance level of the processor.
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *NominalPerformanceBuffer OPTIONAL,
>
> + IN UINT32 NominalPerformanceInteger,
>
> + /// Indicates the lowest performance level of the processor with non-linear power savings.
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *LowestNonlinearPerformanceBuffer OPTIONAL,
>
> + IN UINT32 LowestNonlinearPerformanceInteger,
>
> + /// Indicates the lowest performance level of the processor..
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *LowestPerformanceBuffer OPTIONAL,
>
> + IN UINT32 LowestPerformanceInteger,
>
> + /// Guaranteed Performance Register Buffer.
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *GuaranteedPerformanceRegister OPTIONAL,
>
> + /// Desired Performance Register Buffer.
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *DesiredPerformanceRegister OPTIONAL,
>
> + /// Minimum Performance Register Buffer.
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *MinimumPerformanceRegister OPTIONAL,
>
> + /// Maximum Performance Register Buffer.
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *MaximumPerformanceRegister OPTIONAL,
>
> + /// Performance Reduction Tolerance Register.
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *PerformanceReductionToleranceRegister OPTIONAL,
>
> + /// Time Window Register.
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *TimeWindowRegister OPTIONAL,
>
> + /// Counter Wraparound Time
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *CounterWraparoundTimeBuffer OPTIONAL,
>
> + IN UINT32 CounterWraparoundTimeInteger,
>
> + /// Reference Performance Counter Register
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *ReferencePerformanceCounterRegister,
>
> + /// Delivered Performance Counter Register
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *DeliveredPerformanceCounterRegister,
>
> + /// Performance Limited Register
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *PerformanceLimitedRegister,
>
> + /// CPPC EnableRegister
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *CPPCEnableRegister OPTIONAL,
>
> + /// Autonomous Selection Enable
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *AutonomousSelectionEnableBuffer OPTIONAL,
>
> + IN UINT32 AutonomousSelectionEnableInteger,
>
> + /// AutonomousActivity-WindowRegister
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *AutonomousActivityWindowRegister OPTIONAL,
>
> + /// EnergyPerformance-PreferenceRegister
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *EnergyPerformancePreferenceRegister OPTIONAL,
>
> + /// Reference Performance
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *ReferencePerformanceBuffer OPTIONAL,
>
> + IN UINT32 ReferencePerformanceInteger,
>
> + /// Lowest Frequency
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *LowestFrequencyBuffer OPTIONAL,
>
> + IN UINT32 LowestFrequencyInteger,
>
> + /// Nominal Frequency
>
> + /// Optional
>
> + IN EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE *NominalFrequencyBuffer OPTIONAL,
>
> + IN UINT32 NominalFrequencyInteger,
>
> + 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;
>
> +
>
> + if ((ReferencePerformanceCounterRegister == NULL) ||
>
> + (PerformanceLimitedRegister == NULL) ||
>
> + ((ParentNode == NULL) && (NewCpcNode == NULL)))
Aren't other fields mandatory like the Nominal Performance ?
I believe there should be other checks like:
((NominalPerformanceBuffer == NULL) && (NominalFrequencyInteger == 0))
>
> + {
>
> + ASSERT (0);
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + CpcNode = NULL;
I think CpcNode shouldn't need to be set if the above call to
AmlCodeGenNamePackage() was handling the error with a
'return Status' instead of a 'goto error_handler'
>
> + CpcPackage = NULL;
>
> +
>
> + Status = AmlCodeGenNamePackage ("_CPC", NULL, &CpcNode);
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
> + // Get the Package object node of the _CPC node,
>
> + // which is the 2nd fixed argument (i.e. index 1).
>
> + CpcPackage = (AML_OBJECT_NODE_HANDLE)AmlGetFixedArgument (
>
> + CpcNode,
>
> + EAmlParseIndexTerm1
>
> + );
>
> + if ((CpcPackage == NULL) ||
>
> + (AmlGetNodeType ((AML_NODE_HANDLE)CpcPackage) != EAmlNodeObject) ||
>
> + (!AmlNodeHasOpCode (CpcPackage, AML_PACKAGE_OP, 0)))
>
> + {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
> + // NumEntries 23 per ACPI 6.4 specification
>
> + Status = AmlAddRegisterOrIntegerToPackage (
>
> + NULL,
>
> + 23,
>
> + CpcPackage
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
> + // Revision 3 per ACPI 6.4 specification
>
> + Status = AmlAddRegisterOrIntegerToPackage (
>
> + NULL,
>
> + 3,
>
> + CpcPackage
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
If a Revision field is added to the CM_ARM_CPC_INFO object,
the generation of NumEntries and Revision will have to be modified.
>
> +
>
> + Status = AmlAddRegisterOrIntegerToPackage (
>
> + HighestPerformanceBuffer,
>
> + HighestPerformanceInteger,
>
> + CpcPackage
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> +
>
[snip]
>
> +
>
> + if (ParentNode != NULL) {
>
> + Status = AmlVarListAddTail (
>
> + (AML_NODE_HANDLE)ParentNode,
>
> + (AML_NODE_HANDLE)CpcNode
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT (0);
>
> + goto error_handler;
>
> + }
>
> + }
>
> +
>
> + if (NewCpcNode != NULL) {
>
> + *NewCpcNode = CpcNode;
>
> + }
I think the handling of ParentNode and NewCpcNode can be done
with LinkNode()
>
> +
>
> + return Status;
>
> +
>
> +error_handler:
>
> + if (CpcNode != NULL) {
>
> + AmlDeleteTree ((AML_NODE_HANDLE)CpcNode);
>
> + }
>
> +
>
> + return Status;
>
> +}
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93647): https://edk2.groups.io/g/devel/message/93647
Mute This Topic: https://groups.io/mt/93527136/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list