GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — DynamicTablesPkg: Adds SPMI table generator by abdattar · Pull Request #6236 · tianocore/edk2

Conversation

@abdattar

@abdattar abdattar commented Sep 25, 2024

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tianocore-assign-reviewers
tianocore-assign-reviewers Bot requested review from pierregondois and samimujawar September 25, 2024 07:30
@abdattar
abdattar force-pushed the Spmi branch from 1ca29a0 to 1d0807a Compare September 25, 2024 07:34
@samimujawar

samimujawar commented Sep 25, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pierregondois
pierregondois reviewed Sep 25, 2024
View reviewed changes
Comment thread DynamicTablesPkg/Include/AcpiTableGenerator.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread DynamicTablesPkg/Library/Acpi/Common/AcpiSpmiLib/SpmiGenerator.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread DynamicTablesPkg/Library/Acpi/Common/AcpiSpmiLib/SpmiGenerator.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread DynamicTablesPkg/Library/Acpi/Common/AcpiSpmiLib/SpmiGenerator.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread DynamicTablesPkg/Include/ArchCommonNameSpaceObjects.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread DynamicTablesPkg/Library/Acpi/Common/AcpiSpmiLib/SpmiGenerator.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@abdattar
abdattar force-pushed the Spmi branch from 1d0807a to 9b7b2e9 Compare September 26, 2024 08:53
@tianocore-assign-reviewers
tianocore-assign-reviewers Bot requested review from LiuZhiguang001, lgao4 and mdkinney September 26, 2024 08:53
@abdattar

abdattar commented Sep 26, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lgao4

lgao4 commented Sep 30, 2024

Copy link
Copy Markdown
Contributor

The change in MdePkg is good to me.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pierregondois
pierregondois reviewed Sep 30, 2024
View reviewed changes

@pierregondois pierregondois left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread MdePkg/Include/IndustryStandard/ServiceProcessorManagementInterfaceTable.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread DynamicTablesPkg/DynamicTables.dsc.inc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@abdattar

abdattar commented Sep 30, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdattar
abdattar force-pushed the Spmi branch from 9b7b2e9 to 981e5e4 Compare September 30, 2024 10:14
@pierregondois

pierregondois commented Sep 30, 2024

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdattar

abdattar commented Sep 30, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@abdattar
abdattar force-pushed the Spmi branch 2 times, most recently from 1355e82 to 7deb641 Compare October 3, 2024 04:15
@abdattar
MdePkg/IndustryStandard: Adds SPMI macros
75c587a 
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]>
@abdattar
abdattar force-pushed the Spmi branch from 7deb641 to cc1c1b4 Compare October 3, 2024 05:28
@abdattar
DynamicTablesPkg: Adds SPMI table generator
00f89e5 
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
abdattar force-pushed the Spmi branch from cc1c1b4 to 00f89e5 Compare October 3, 2024 05:40
@abdattar

abdattar commented Oct 3, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pierregondois
pierregondois approved these changes Oct 3, 2024
View reviewed changes
samimujawar
samimujawar approved these changes Oct 3, 2024
View reviewed changes
@samimujawar samimujawar added the push Auto push patch series in PR if all checks pass label Oct 3, 2024
@mergify
Merge branch 'master' into Spmi
d6ddcea
@mergify
mergify Bot merged commit f962adc into tianocore:master Oct 3, 2024
@abdattar
abdattar deleted the Spmi branch January 20, 2025 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lgao4 lgao4 Awaiting requested review from lgao4
@LiuZhiguang001 LiuZhiguang001 Awaiting requested review from LiuZhiguang001
@mdkinney mdkinney Awaiting requested review from mdkinney
2 more reviewers
@samimujawar samimujawar samimujawar approved these changes
@pierregondois pierregondois pierregondois approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

push Auto push patch series in PR if all checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@abdattar @samimujawar @lgao4 @pierregondois
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.