Skip to content

Conversation

@jbrasen
Copy link
Contributor

@jbrasen jbrasen commented Mar 7, 2025

Description

This commit adds support for building non-serial DBG2 devices

  1. Add new common namespace objects:

    • EArchCommonObjMemoryRangeDescriptor
    • EArchCommonObjDbg2DeviceInfo
  2. Enhance DBG2 table generator:

    • Add support for non-serial DBG2 devices
    • Support multiple debug devices
    • Improve memory handling with proper allocation checks
    • Add better cleanup in error paths
  3. Update configuration manager:

    • Add parsers for new common namespace objects
  • Breaking change?
  • Impacts security?
  • Includes tests?

How This Was Tested

Created config manager object for a PCIe NIC and validated the DBG2 node. Added HostBasedTest for this library

Integration Instructions

N/A

@jbrasen jbrasen force-pushed the Upstream/Dbg2Generator branch 2 times, most recently from de93e47 to ddc4c23 Compare March 7, 2025 20:14
@jbrasen jbrasen force-pushed the Upstream/Dbg2Generator branch from ddc4c23 to 050383a Compare March 20, 2025 19:13
@github-actions github-actions bot added the impact:testing This contribution includes tests such as unit and/or integration tests. label Mar 20, 2025
@jbrasen jbrasen force-pushed the Upstream/Dbg2Generator branch 4 times, most recently from 91d3453 to 4eeeb14 Compare March 24, 2025 15:44
@jbrasen
Copy link
Contributor Author

jbrasen commented Mar 24, 2025

@pierregondois I think all the review feedback was updated. I also added a host based test for this.

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 Jeff,
Thanks for adding the test file.

@samimujawar as this is new to the package, would it be ok to check it the GoogleTest file if you're ok with it ?

Regards,
Pierre

@samimujawar
Copy link
Contributor

Hello Jeff, Thanks for adding the test file.

@samimujawar as this is new to the package, would it be ok to check it the GoogleTest file if you're ok with it ?

Regards, Pierre

@jbrasen Thanks a lot for introducing these tests. This is indeed a good initiative.
Unfortunately, I have not looked into detail on how these tests are supposed to be designed/created.
If you can add a section to the DynamicTablesPkg/Readme.md on how these tests can be built & run, it would be of great help for us to explore further.
For now I think we could go ahead with your implementation, but we would be keen to build on this further and improve the testing for this package.

@jbrasen jbrasen force-pushed the Upstream/Dbg2Generator branch from 4eeeb14 to dd8913e Compare April 15, 2025 02:54
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.

Just one minor comment, otherwise lgtm.
The serial port information generated for Juno/kvmtool didn't change. I didn't try to create non-serial ports but the code looked correct.

Just need to check with @samimujawar if this is ok to include GoogleTests

@samimujawar
Copy link
Contributor

samimujawar commented Apr 16, 2025

Just one minor comment, otherwise lgtm. The serial port information generated for Juno/kvmtool didn't change. I didn't try to create non-serial ports but the code looked correct.

Just need to check with @samimujawar if this is ok to include GoogleTests

I think adding GoogleTests is good. We need to see how GoogleTests can be supported other generators as well.

@NicholasGraves
Copy link
Contributor

Just one minor comment, otherwise lgtm. The serial port information generated for Juno/kvmtool didn't change. I didn't try to create non-serial ports but the code looked correct.
Just need to check with @samimujawar if this is ok to include GoogleTests

I think adding GoogleTests is good. We need to see how GoogleTests can be supported other generators as well.

I'm adding a DynamicTables generator for CEDT right now and I'm using the same pattern as this PR for gtests. Intercepting the table registration makes implementing the rest of the test mostly straightforward.

@ardbiesheuvel ardbiesheuvel added the push Auto push patch series in PR if all checks pass label Apr 24, 2025
jbrasen added 3 commits April 24, 2025 08:27
Add support for X64 Build needed for HostBasedTests

Signed-off-by: Jeff Brasen <[email protected]>
This commit adds support for building non-serial DBG2 devices

1. Add new common namespace objects:
   - EArchCommonObjMemoryRangeDescriptor
   - EArchCommonObjDbg2DeviceInfo

2. Enhance DBG2 table generator:
   - Add support for non-serial DBG2 devices
   - Support multiple debug devices
   - Improve memory handling with proper allocation checks
   - Add better cleanup in error paths

3. Update configuration manager:
   - Add parsers for new common namespace objects

Signed-off-by: Jeff Brasen <[email protected]>
This commit adds a hostbased test for the
Dbg2Generator library. The test is designed to
be run on a host machine against a UEFI image
that has been built with the DynamicTablesPkg.

The test exercises the Dbg2Generator library by
building an ACPI DBG2 from a list of devices.

Signed-off-by: Jeff Brasen <[email protected]>
@ardbiesheuvel ardbiesheuvel force-pushed the Upstream/Dbg2Generator branch from ed1db0c to 66b0cf4 Compare April 24, 2025 06:28
@tianocore tianocore deleted a comment from mergify bot Apr 24, 2025
@ardbiesheuvel ardbiesheuvel merged commit 7711e8a into tianocore:master Apr 24, 2025
124 checks passed
@mergify
Copy link

mergify bot commented Apr 24, 2025

queue

🟠 Waiting for conditions to match

Details
  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • base~=(^main|^master|^stable/)
      • label=push
      • any of: [🛡 GitHub branch protection]
        • check-success = tianocore.PatchCheck
        • check-neutral = tianocore.PatchCheck
        • check-skipped = tianocore.PatchCheck
      • any of: [🛡 GitHub branch protection]
        • check-success = ArmVirtPkg - Ubuntu GCC - PR
        • check-neutral = ArmVirtPkg - Ubuntu GCC - PR
        • check-skipped = ArmVirtPkg - Ubuntu GCC - PR
      • any of: [🛡 GitHub branch protection]
        • check-success = EmulatorPkg - Ubuntu GCC - PR
        • check-neutral = EmulatorPkg - Ubuntu GCC - PR
        • check-skipped = EmulatorPkg - Ubuntu GCC - PR
      • any of: [🛡 GitHub branch protection]
        • check-success = EmulatorPkg - Windows VS - PR
        • check-neutral = EmulatorPkg - Windows VS - PR
        • check-skipped = EmulatorPkg - Windows VS - PR
      • any of: [🛡 GitHub branch protection]
        • check-success = OvmfPkg - Ubuntu GCC - PR
        • check-neutral = OvmfPkg - Ubuntu GCC - PR
        • check-skipped = OvmfPkg - Ubuntu GCC - PR
      • any of: [🛡 GitHub branch protection]
        • check-success = OvmfPkg - Windows VS - PR
        • check-neutral = OvmfPkg - Windows VS - PR
        • check-skipped = OvmfPkg - Windows VS - PR
      • any of: [🛡 GitHub branch protection]
        • check-success = Windows VS - PR
        • check-neutral = Windows VS - PR
        • check-skipped = Windows VS - PR
      • any of: [🛡 GitHub branch protection]
        • check-success = Ubuntu GCC - PR
        • check-neutral = Ubuntu GCC - PR
        • check-skipped = Ubuntu GCC - PR
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

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

Labels

impact:testing This contribution includes tests such as unit and/or integration tests. push Auto push patch series in PR if all checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants