Skip to content
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.

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

DynamicTablesPkg: Adds SPMI table generator #6236

Merged
merged 3 commits into from
Oct 3, 2024
Merged

Conversation

abdattar
Copy link
Contributor

Adds ACPI SPMI table generator library.
Updates acpi standard table enum with spmi.
Updates arch common namespace object and parser.
Updates the Readme.

Cc: Sami Mujawar [email protected]
Cc: Pierre Gondois [email protected]

Description

Adds ACPI SPMI table generator library.

  • 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 and dumped the acpi tables.

Integration Instructions

N/A

@samimujawar
Copy link
Contributor

The Acpiview PR for this table is "ShellPkg: Add support to parse SPMI table in acpiview #5980" (#5980).

@abdattar
Copy link
Contributor Author

Hi @pierregondois,
Thanks for the quick review, I have updated the patch after addressing the review comments.
Thanks
AbduL

@lgao4
Copy link
Contributor

lgao4 commented Sep 30, 2024

The change in MdePkg is good to me.

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.

@samimujawar @abdattar I assume there should only be only one IPMI per platform, so linking the two objects with a Token should not be necessary right ?


The SPMI table might require the following objects in an SSDT table:

  • _IFT (Interface type)
  • _SRV (spec. revision)
    in prevision of creating an associated SSDT table generator, would it be possible to add a field for the spec. revision in the CM_ARCH_COMMON_SPMI_INTERFACE_INFO object ?
    This would allow not to have a hard coded value in the 2 table generators

@abdattar
Copy link
Contributor Author

@samimujawar @abdattar I assume there should only be only one IPMI per platform, so linking the two objects with a Token should not be necessary right ?

The SPMI table might require the following objects in an SSDT table:

  • _IFT (Interface type)
  • _SRV (spec. revision)
    in prevision of creating an associated SSDT table generator, would it be possible to add a field for the spec. revision in the CM_ARCH_COMMON_SPMI_INTERFACE_INFO object ?
    This would allow not to have a hard coded value in the 2 table generators

Revision value is obtained from IPMI library call, if the call fails then default version 2 is applied.
I dont think we need to provide revision number in configution object.

@pierregondois
Copy link
Contributor

@samimujawar @abdattar I assume there should only be only one IPMI per platform, so linking the two objects with a Token should not be necessary right ?

The SPMI table might require the following objects in an SSDT table:

    _IFT (Interface type)
    _SRV (spec. revision)
    in prevision of creating an associated SSDT table generator, would it be possible to add a field for the spec. revision in the CM_ARCH_COMMON_SPMI_INTERFACE_INFO object ?
    This would allow not to have a hard coded value in the 2 table generators

Revision value is obtained from IPMI library call, if the call fails then default version 2 is applied.
I dont think we need to provide revision number in configution object.

Yes right, sorry I forgot it was done like that. Ok for me then

@abdattar
Copy link
Contributor Author

Hi @pierregondois ,
Missed the below comment.
"@samimujawar @abdattar I assume there should only be only one IPMI per platform, so linking the two objects with a Token should not be necessary right ?"

[Abdul]
Yes, there will be only single IPMI per platform.
each object has its own purpose.

  1. EArchCommonObjSpmiInterfaceInfo: Gives information about abstract interface type and how the resources are mapped.
  2. EArchCommonObjSpmiInterruptDeviceInfo: Gives information about what type of interrupt it uses and device location.
    Current/modern platform do not have BMC/IPMI devices on the host board.
    Usually it will be outside of the host.
    EArchCommonObjSpmiInterruptDeviceInfo is legacy information, current platform dont need this information.
    To simplify(minimize the configuration fields) the configuration I have separated it.

Adds macro which defines SPMI table revision
and interface type as per the specification.

Cc: Michael D Kinney <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Zhiguang Liu <[email protected]>
Cc: Sami Mujawar <[email protected]>
Cc: Pierre Gondois <[email protected]>
Signed-off-by: Abdul Lateef Attar <[email protected]>
Adds ACPI SPMI table generator library.
Updates acpi standard table enum with spmi.
Updates arch common namespace object and parser.
Updates the Readme.

Cc: Michael D Kinney <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Zhiguang Liu <[email protected]>
Cc: Sami Mujawar <[email protected]>
Cc: Pierre Gondois <[email protected]>
Signed-off-by: Abdul Lateef Attar <[email protected]>
@abdattar
Copy link
Contributor Author

abdattar commented Oct 3, 2024

Hi @pierregondois , @samimujawar , @lgao4
Addressed the review comments, please review and approve the PR.
Thanks
AbduL

@samimujawar samimujawar added the push Auto push patch series in PR if all checks pass label Oct 3, 2024
@mergify mergify bot merged commit f962adc into tianocore:master Oct 3, 2024
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants