search

firecracker-microvm / firecracker

Secure and fast microVMs for serverless computing.
http://firecracker-microvm.io
Apache License 2.0
25.07k stars 1.75k forks source link

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

Open roypat opened 1 month ago

roypat commented 1 month ago

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 https://github.com/firecracker-microvm/firecracker/pull/4687#discussion_r1686789518

jackabald commented 1 month ago

Hey, @roypat!

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

Thanks!

roypat commented 1 month ago

Hi @jackabald,

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

jackabald commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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.

jackabald commented 1 month ago

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!