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

Clean up create_fdt function in src/vmm/src/arch/aarch64/fdt.rs #4700

Open
roypat opened this issue Jul 23, 2024 · 6 comments
Open

Clean up create_fdt function in src/vmm/src/arch/aarch64/fdt.rs #4700

roypat opened this issue Jul 23, 2024 · 6 comments
Assignees
Labels
Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later

Comments

@roypat
Copy link
Contributor

roypat commented Jul 23, 2024

The create_fdt function has two eccentricities that can be cleaned up:

  1. It is unnecessarily parametrized by std::hash::HashBuilder, and
  2. It both writes the flattened device tree to guest memory, as well as returns it (however this return value is ignored the only call site). In the spirit of separating concerns, it should not write the FDT to guest memory - instead the caller should use the return value and write it to memory. This might also simplify our unittesting a little bit, as we no longer need to construct GuestMemoryMmap in the tests.

See also #4687 (comment)

@roypat roypat added Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later labels Jul 23, 2024
@jackabald
Copy link
Contributor

Hey, @roypat!

I'd like to work on this issue, can you assign me?

Thanks!

@roypat
Copy link
Contributor Author

roypat commented Jul 24, 2024

Hi @jackabald,

I've assigned you, thanks for having a look!

@jackabald
Copy link
Contributor

@roypat, I have made changes to suit your needs regarding this issue. However, I am having trouble configuring the test cases in fdt.rs. I don't want to open a pull request without fully testing this code, but I am having trouble running these test cases.

Should I just send a pull request for you to take a look at?

@roypat
Copy link
Contributor Author

roypat commented Jul 26, 2024

@roypat, I have made changes to suit your needs regarding this issue. However, I am having trouble configuring the test cases in fdt.rs. I don't want to open a pull request without fully testing this code, but I am having trouble running these test cases.

Should I just send a pull request for you to take a look at?

Yes, opening a draft PR for early feedback is always welcome :)

@jackabald
Copy link
Contributor

@roypat linked the pull request. please let me know if I am going in the right direction. I had created a whole new function for writing to guest memory but it seems it's not needed if we just want callers to assign it to memory based on its return value.

@roypat roypat linked a pull request Jul 29, 2024 that will close this issue
@jackabald jackabald mentioned this issue Jul 31, 2024
9 tasks
@jackabald
Copy link
Contributor

Please review my new pull request. If you would like me to work on cleaning up some of the unit testing I will happily work on this further :)

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants