Skip to content

DynamicTablesPkg: Add CEDT table generation #11010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

DynamicTablesPkg: Add CEDT table generation #11010

wants to merge 3 commits into from

Conversation

NicholasGraves
Copy link
Contributor

Description

  • Add namespace objects for CEDT table generation.

  • Add generator to create CEDT table

  • Add unit tests for generator.

  • 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?

    • unit tests

How This Was Tested

  • unit tests
  • qemu test

Integration Instructions

N/A

@github-actions github-actions bot added the impact:testing This contribution includes tests such as unit and/or integration tests. label Apr 25, 2025
@NicholasGraves NicholasGraves force-pushed the cxl-namespace-obj-upstream branch 13 times, most recently from 0e2342c to 1a65675 Compare May 1, 2025 21:14
pierregondois
pierregondois reviewed May 5, 2025
View reviewed changes
Copy link
Contributor

@pierregondois pierregondois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Nick,
Thanks for the PR, I added a few comments.
I haven't checked the tests yet, but would it be possible to place them in another patch/commit ?

Regards,
Pierre

DynamicTablesPkg/Include/ArchCommonNameSpaceObjects.h
UINT64 ComponentRegisterBase;
} CM_ARCH_COMMON_CXL_HOST_BRIDGE_INFO;

#define CFMWS_MAX_INTERLEAVE_WAYS (16)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should not need to have fixed-length arrays. Would it be possible to replace InterleaveTargetTokens by a token to a CM_ARCH_COMMON_OBJ_REF instead ?

The CM_ARCH_COMMON_OBJ_REF array would then contain tokens referencing CM_ARCH_COMMON_CXL_HOST_BRIDGE_INFO objects

Copy link
Contributor Author

@NicholasGraves NicholasGraves May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max size is given in the CXL spec, see section 8.2.4.19.7. What is the advantage of declaring the array out-of-band with respect to the parent struct?

DynamicTablesPkg/DynamicTables.dsc.inc
@@ -129,4 +132,3 @@
NULL|DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyLib.inf
NULL|DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtPcieLib/SsdtPcieLib.inf
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need I think

DynamicTablesPkg/Library/Acpi/Common/AcpiCedtLib/CedtGenerator.c
InterleaveTargetIndex < WindowList[Index].NumberOfInterleaveWays;
InterleaveTargetIndex++)
{
UINT32 HostBridgeCount;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: unfortunately there is a coding standard requesting to declare variables at the beginning of functions. So would it be possible to move these variable declarations up ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

section 5.4.1.1 allows for block scope declaration in the spec here: https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification/release-2.20/edk2-CCodingStandardsSpecification-release-2.20.pdf

DynamicTablesPkg/Library/Acpi/Common/AcpiCedtLib/CedtGenerator.c
// Also validate that the provided interleave ways is compliant.
TotalInterleaveTargets = 0;
for (Index = 0; Index < WindowCount; Index++) {
UINT8 InterleaveWays = WindowList[Index].NumberOfInterleaveWays;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about declaring variables at the beginning of functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to where this convention is defined?

DynamicTablesPkg/Library/Acpi/Common/AcpiCedtLib/CedtGenerator.c
return Status;
}

Cfmws->InterleaveTargetList[InterleaveTargetIndex] = HostBridge->Uid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm reading the following in the spec:

a.
If ENIW=0,
N=0
b.
If ENIW=1,
N= HPA[8+HBIG]

[...]

I'm not sure to interpret this correctly, but shouldn't this be a list of conditions to check ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the section you're referencing; in my opinion it is not written clearly and would be better placed elsewhere.

This field is simply an ordered list of the targets involved with the interleaving.

The formula provided in the table prescribes the HDM decoder (Host-managed Device Memory) settings for each interleave target (i.e. CHBS entry). Configuring the HDM decoders correctly is out of scope for CEDT table generation and is delegated to the specific CXL implementation.

@NicholasGraves
DynamicTablesPkg: Add CXL CEDT namespace objects
4b464e8 
- Add objects for CEDT CHBS and CEDT CFMWS. These describe CXL host
  bridges and CXL fixed memory windows, respectively.

Signed-off-by: Nick Graves <[email protected]>
@NicholasGraves NicholasGraves force-pushed the cxl-namespace-obj-upstream branch 3 times, most recently from 72344f2 to 7ce03b2 Compare May 6, 2025 19:46
@NicholasGraves
DynamicTablesPkg: Add CEDT generator
8e8234d 
- Generate CEDT table from ConfigurationManager objects.
- Only CHBS and CFMWS sub-tables are supported currently.

Signed-off-by: Nick Graves <[email protected]>
@NicholasGraves
DynamicTablesPkg: Add CEDT generator tests
79ea2ab 
- Add unit tests for CEDT generator.

Signed-off-by: Nick Graves <[email protected]>
@NicholasGraves NicholasGraves force-pushed the cxl-namespace-obj-upstream branch from 7ce03b2 to 79ea2ab Compare May 6, 2025 19:54
@NicholasGraves
Copy link
Contributor Author

Hello Nick, Thanks for the PR, I added a few comments. I haven't checked the tests yet, but would it be possible to place them in another patch/commit ?

Regards, Pierre

Done, moved tests to a separate commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierregondois pierregondois pierregondois left review comments

@samimujawar samimujawar Awaiting requested review from samimujawar

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
impact:testing This contribution includes tests such as unit and/or integration tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NicholasGraves @pierregondois