From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.5916.1662978997220745380 for ; Mon, 12 Sep 2022 03:36:37 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3A4A3113E; Mon, 12 Sep 2022 03:36:43 -0700 (PDT) Received: from [192.168.1.11] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 684D93F71A; Mon, 12 Sep 2022 03:36:35 -0700 (PDT) Message-ID: <65068b03-c53b-6230-8f81-884445a90534@arm.com> Date: Mon, 12 Sep 2022 12:36:08 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [edk2-devel] [PATCH 2/3] DynamicTablesPkg: AML Code generation to add _CPC entries To: devel@edk2.groups.io, jbrasen@nvidia.com Cc: ardb+tianocore@kernel.org, Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com References: From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > > --- > > .../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; > > +} >