Skip to content

Windows: DeviceTree traversal/offset bugfixes #1683

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

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dgmcdona
Copy link
Contributor

@dgmcdona dgmcdona commented Mar 7, 2025

There are two issues with the current implementation of the windows.devicetree.DeviceTree plugin:

  • The traversal of the device tree is incomplete. The DEVICE_OBJECT.get_attached_devices extension method does not traverse the full breadth at each level of the device tree through the NextDevice linked-list.
  • Every entry uses the driver's virtual address as the offset. Device entries should yield their own virtual addresses in the generator.

This PR fixes the issue by implementing a recursive, private staticmethod on the DeviceTree plugin class that does a complete depth-first traversal of the tree, yielding many more device objects (using their virtual address as the Offset member instead of that of the parent driver).

@dgmcdona dgmcdona marked this pull request as draft March 7, 2025 23:41
Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

Looks ok, but please no static methods in classes that are versionable. Also, I'd tend to agree with the scanner and suggest that at least a comment explaining why the exception can be ignored should be included...

dgmcdona added 2 commits March 7, 2025 20:53
This updates the `devicetree` plugin to return the virtual offsets of
the device objects themselves instead of that of their parent driver.
Previously, the attached devices were being traversed once depth-wise,
but the horizontal levels of the device tree were not being properly
traversed, leading to tons of missing device entries. This fixes the
issue by implementing a recursive staticmethod for properly descending +
enumerating devices on each level of the tree.
@dgmcdona
Copy link
Contributor Author

dgmcdona commented Mar 8, 2025

@ikelos One consideration for this should be what to do about the get_attached_devices method on the DEVICE_OBJECT extension class. It's current implementation is flawed, which is why I moved away from it in this PR, but maybe we should deprecate + remove or fix it as part of this effort. I would have fixed it and used it here but it would have required a method signature change in order to also return the depth in the device tree that we need when constructing the TreeGrid.

@dgmcdona dgmcdona force-pushed the devicetree_traversal_and_offsets branch from ebf2d6f to a1091c8 Compare March 8, 2025 04:43
@dgmcdona dgmcdona force-pushed the devicetree_traversal_and_offsets branch 3 times, most recently from 1b024c0 to dea091a Compare March 8, 2025 05:23
Moves a lot of the code that was parsing information about device
objects out of the `DeviceTree` plugin logic and into the
`DEVICE_OBJECT` extension class. Also moved the dict mapping integer
values to device types into the extension module.

Fixes a bug within the extension class' `get_attached_devices` method -
now, immediate children of the device in the tree are yielded, instead
of the faulty deep traversal that was occuring before. Because of this
bugfix in an extension class as well as the addition of new methods in
the extension class, this also bumps the framework minor version number.
@dgmcdona dgmcdona force-pushed the devicetree_traversal_and_offsets branch from dea091a to 73e422f Compare March 8, 2025 08:05
@dgmcdona dgmcdona marked this pull request as ready for review March 8, 2025 08:08
@ikelos
Copy link
Member

ikelos commented Mar 8, 2025

I'd pick a different method name, and run them alongside each other (deprecating the old one)? Eventually, if it's really worth it, you can shift the new one back to the old name.

If the code belongs centrally, it's much better to have it live there than in a plugin where someone may not know about it and try to reimplement (possibly incorrectly). If it's only for the generator (ie, not to be used by other plugins) then it's fine. You could also consider a visitor design pattern, that takes a function and runs it on each node in the tree in a particular order, calling it with the node and the depth at each point?

@dgmcdona dgmcdona marked this pull request as draft March 8, 2025 20:42
@dgmcdona
Copy link
Contributor Author

dgmcdona commented Mar 8, 2025

Thanks, marking as draft again while I revisit this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants