Creates C-State, P-State and CPC objects under processor for X64 arch #6484

abdattar · 2024-11-29T13:30:44Z

Description

This PR creates below ACPI objects for processor device for X64 arch
C-State: Creates _CST and _CSD objects.
P-State: Create _PCT, _PSS objects plus _PPC method
_PSD object for domain dependency
_CPC continue performance control object
Update AML library to generate _CST, _CSD, _PCT and _PSS node.
Generates _STA method based on configuration data.

Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- Examples: Does it add a new library class or move a module to a different repo.
Impacts security?
- Security - Does this PR have a direct security impact?
- Examples: Crypto algorithm change or buffer overflow fix.
Includes tests?
- Tests - Does this PR include any explicit test code?
- Examples: Unit tests or integration tests.

How This Was Tested

Tested on AMD Platform

Integration Instructions

N/A

DynamicTablesPkg/Include/AcpiObjects.h

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

DynamicTablesPkg/Include/AcpiObjects.h

DynamicTablesPkg/Include/ArchCommonNameSpaceObjects.h

Add _CSD version and number of entries definition. These were introduced in the ACPI 3.0 specification. Reference: ACPI 6.5 specification, section 8.4.1.2, Table 8.3: C-State Dependency Package Values. Signed-off-by: Abdul Lateef Attar <[email protected]>

Introduces functions to create C-State and P-State objects. Includes code generation functions for _CST, _CSD, _PCT, and _PSS ACPI processor objects. Signed-off-by: Abdul Lateef Attar <[email protected]>

Introduces configuration manager objects for C-State and P-State. These objects are used to parse and store the configuration data for C-State and P-State tables. Signed-off-by: Abdul Lateef Attar <[email protected]>

Add ACPI processor _CST, _CSD, _PCT, and _PSS objects for X64 CPU SSDT table creation. Also, add the _PPC method. Signed-off-by: Abdul Lateef Attar <[email protected]>

Introduce _PSD and _CPC ACPI objects for X64 platforms. Signed-off-by: Abdul Lateef Attar <[email protected]>

Adds _STA device status bit definations. Reference: ACPI 6.5 specification, section 6.3.7 Signed-off-by: Abdul Lateef Attar <[email protected]>

Implement the _STA method for the CPU object based on the value provided by the configuration manager. Signed-off-by: Abdul Lateef Attar <[email protected]>

abdattar · 2024-12-09T13:10:03Z

Hi @pierregondois,
I have addressed the review comments. Could you please review it again?
Thanks,
Abdul

pierregondois · 2024-12-09T13:21:34Z

DynamicTablesPkg/Include/AcpiObjects.h

+/** A structure that describes a
+    processor power state control package Info.
+
+  Cf. ACPI 6.1, s8.4.1.1 _CST (Processor Power State Control).


Please move to 6.5 spec

pierregondois · 2024-12-09T13:22:06Z

DynamicTablesPkg/Include/AcpiObjects.h

+  /// Worst case latency for entering and exiting the C-State, in milliseconds.
+  UINT16                                    Latency;
+
+  /// Power consumption in the C-State.


NIT: is it possible to add that this is in mW ?

pierregondois · 2024-12-09T13:35:32Z

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

+EFIAPI
+AmlCreatePssNode (
+  IN  AML_PSS_INFO            *PssInfo,
+  IN  UINT32                  NumPackages,


IIRC, PssInfo is an array of NumPackages elements. Is it possible to mention it in the parameter description.

pierregondois · 2024-12-09T13:36:58Z

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

+    goto error_handler;
+  }
+
+  for (Index = 0; Index < NumPackages; Index++) {


I think you need to iterate over the different PssInfo elements, it seems you are using the first entry for all the iterations.
I.e. use PssInfo[Index].XXX in the loop.

pierregondois · 2024-12-09T13:37:18Z

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

+  AML_OBJECT_NODE_HANDLE  PssMainPackage;
+  UINT32                  Index;
+
+  if ((PssInfo == NULL) ||


Would it be possible to forbid calling the function with NumPackages == 0 ?

pierregondois · 2024-12-09T13:46:50Z

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

+    Status            // Integer (DWORD)
+  })
+
+  Cf. ACPI 6.4, s8.4.5.2 _PSS (Processor Supported Performance States).


Should update to 6.5 spec (maybe same comment at other places)

pierregondois · 2024-12-09T13:50:18Z

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

+/** Create _PSS node
+
+  Generates and optionally appends the following node:
+  Name (_PSS, Package()


I think the description of what is generated is incorrect: each PState is in a package and _PSS returns a package of PState, so it should be as in the spec:

Package { PState [0] // Package - Performance state 0 .... PState [n] // Package - Performance state n } Each Pstate sub-Package contains the elements described below: Package { CoreFrequency, // Integer (DWORD) Power, // Integer (DWORD) Latency, // Integer (DWORD) BusMasterLatency, // Integer (DWORD) Control, // Integer (DWORD) Status, // Integer (DWORD) }

I think some comments for AmlCreatePssNode() can be applied to AmlCreateCsdNode() and AmlCreateCstNode(). Is it possible to check please ?

Ack, agree will update the description.

pierregondois · 2024-12-09T14:09:01Z

DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c

+  }
+
+  for (Index = 0; Index < NumEntries; Index++) {
+    if ((CsdInfo[Index].NumEntries != EFI_ACPI_6_4_AML_CSD_NUM_ENTRIES) ||


Should be 6.5 spec

pierregondois · 2024-12-09T14:20:35Z

MdePkg/Include/IndustryStandard/Acpi65.h

@@ -19,6 +19,15 @@
 //
 #pragma pack(1)

+///
+/// _STA bit definitions ACPI 6.5 s6.3.7


Not here, but typo in the commit message: definations -> definitions

pierregondois · 2024-12-09T14:53:34Z

DynamicTablesPkg/Include/X64NameSpaceObjects.h

+  /** Optional field: Reference Token for the Cst info of this processor.
+      i.e. a token referencing a CM_ARCH_COMMON_CST_INFO object.
+  */
+  CM_OBJECT_TOKEN    CstToken;


I think these should be token identifying a CM_ARCH_COMMON_OBJ_REF structure instead.

Indeed, it is likely that most of the CPUs on the platform will have the same set of C-States or P-States (or other). So instead of having all functions returning the same set of values for each CPU, it would be better to factorize these values in one object. For instance, all CPUs on the platform could implement the _CST method as:

Scope (\_SB) { Name (CST0, Package() { 0, // Count Package() { Register, Type, Latency, Power }, } // Name CST0 } // and for each CPU: Scope(\_PR.CPU0) { Method(_CST,0) { Return(\_SB.CST0) } } Scope(\_PR.CPU1) { Method(_CST,0) { Return(\_SB.CST0) } } Scope(\_PR.CPU2) { Method(_CST,0) { Return(\_SB.CST0) } } ...

This seems to be also how platform vendors have implemented their _PSS, _PCT methods.

Please check the GenerateLpiStates() and CreateAmlLpiMethod() functions

Didnt opted the factorization for below reason.

to generate the code similar to what ACPI spec says and close to the acpi spec examples.

There are possibilities(hypothetical scenario) that each processor might support different set of C-State and P-State.
Also different processor might have different values(in package) to put the processor into different C-State.

Using a CM_ARCH_COMMON_OBJ_REF would still allow CPUs to have different C-States/P-states. However they would have to be written only once. Each CPU can then aggregate these C/P-states how they desire.

Please have a look at the Lpi-States configuration at [1]. The Juno platform has an asymmetric topology, so for most platforms Lpi-States configuration would be simpler to configure (and so would it be for the C/P-States).

[1] https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c

abdattar force-pushed the CpuObjects branch from 121f9ae to 2d088fa Compare December 2, 2024 04:46

niruiyu requested a review from lzeng14 December 2, 2024 05:40

abdattar force-pushed the CpuObjects branch from 2d088fa to 1665476 Compare December 6, 2024 14:19

abdattar marked this pull request as ready for review December 6, 2024 14:25

tianocore-assign-reviewers bot requested review from lgao4, LiuZhiguang001, mdkinney, pierregondois and samimujawar December 6, 2024 14:25

pierregondois requested changes Dec 6, 2024

View reviewed changes

abdattar force-pushed the CpuObjects branch 2 times, most recently from 2817556 to a13b8af Compare December 9, 2024 07:05

Abdul Lateef Attar added 7 commits December 9, 2024 09:02

DynamicTablesPkg/AmlLib: Create C-State and P-State objects

c9c05e8

Introduces functions to create C-State and P-State objects. Includes code generation functions for _CST, _CSD, _PCT, and _PSS ACPI processor objects. Signed-off-by: Abdul Lateef Attar <[email protected]>

DynamicTablesPkg: Introduces C-state and P-state namespace objects

c25f395

Introduces configuration manager objects for C-State and P-State. These objects are used to parse and store the configuration data for C-State and P-State tables. Signed-off-by: Abdul Lateef Attar <[email protected]>

DynamicTablesPkg: Add _CST, _CSD, _PCT, and _PSS objects

86f65bd

Add ACPI processor _CST, _CSD, _PCT, and _PSS objects for X64 CPU SSDT table creation. Also, add the _PPC method. Signed-off-by: Abdul Lateef Attar <[email protected]>

DynamicTablesPkg: Add _PSD and _CPC support for X64

ccc46ee

Introduce _PSD and _CPC ACPI objects for X64 platforms. Signed-off-by: Abdul Lateef Attar <[email protected]>

MdePkg/IndustryStandard: Add _STA bit definations

280c68a

Adds _STA device status bit definations. Reference: ACPI 6.5 specification, section 6.3.7 Signed-off-by: Abdul Lateef Attar <[email protected]>

DynamicTablesPkg: Add _STA method to CPU object

8e23166

Implement the _STA method for the CPU object based on the value provided by the configuration manager. Signed-off-by: Abdul Lateef Attar <[email protected]>

abdattar force-pushed the CpuObjects branch from a13b8af to 8e23166 Compare December 9, 2024 09:03

pierregondois requested changes Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creates C-State, P-State and CPC objects under processor for X64 arch #6484

Creates C-State, P-State and CPC objects under processor for X64 arch #6484

abdattar commented Nov 29, 2024 •

edited

Loading

abdattar commented Dec 9, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 9, 2024

abdattar Dec 10, 2024

pierregondois Dec 10, 2024

Creates C-State, P-State and CPC objects under processor for X64 arch #6484

Are you sure you want to change the base?

Creates C-State, P-State and CPC objects under processor for X64 arch #6484

Conversation

abdattar commented Nov 29, 2024 • edited Loading

Description

How This Was Tested

Integration Instructions

abdattar commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdattar commented Nov 29, 2024 •

edited

Loading