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

Ensure uniqueness of phandle values within FDT #47

Merged
merged 1 commit into from
Oct 25, 2021
Merged

Ensure uniqueness of phandle values within FDT #47

merged 1 commit into from
Oct 25, 2021

Conversation

gsserge
Copy link

@gsserge gsserge commented Oct 21, 2021

This implementation ensures the phandle values' uniqueness only when the property is set using FdtWriter::property_u32. It's still possible to misuse phandles using raw FdtWriter::property, or property setters for types other than u32. However, it's such an unlikely scenario and I thought it wasn't worth the trouble to change all of the setters.

Basic micro benchmarking of the changed FdtWriter::property_u32 demonstrated ~60 ns slowdown if the property name is "phandle".

src/writer.rs Outdated
pub fn property_u32(&mut self, name: &str, val: u32) -> Result<()> {
if name == "phandle" && !self.phandles.insert(val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems OK to me, but I wonder if it might be cleaner to just have a property_phandle() function instead. This would require the caller to actually use that function to get the guarantee of uniqueness, but it seems less surprising than special treatment of some names.

I'm not leaning toward either option in particular, but I'd be interested to hear arguments for one or the other.

I also wonder if we could make a function that generates and returns unique phandle numbers (perhaps in a strongly-typed way instead of just plain u32) rather than requiring the caller to provide them. That does restrict the caller somewhat, since they'd have to emit the phandle property first before being able to refer to it in other nodes, which could be problematic depending on the structure of the tree. I guess it could also be a separate function to allocate a phandle and then pass it to the property function later.

Copy link
Author

@gsserge gsserge Oct 21, 2021

Choose a reason for hiding this comment

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

Thanks for the review! These are good points; we were thinking about a separate method like property_phandle() and about the generator function as well.

I think that the main reason to stick with the current API is that firecracker is using property_u32 to set phandles explicitly, for example https://github.com/firecracker-microvm/firecracker/blob/830ee86678f6a7b1eea95dd70f170901c063c7e6/src/arch/src/aarch64/fdt.rs#L197

Thinking about this again, I actually like the idea of having property_phandle() with the guarantee of uniqueness. It wouldn't prevent incorrect usage of property_u32() or property(), but at least it's more explicit. @andreeaflorescu what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yap, I also like the property_phandle idea more. We can adapt the code in Firecracker, that should not be a problem. In the future we can also think about adding other property types as per the specification as well.

Signed-off-by: Sergii Glushchenko <gsserge@amazon.com>
@danielverkamp danielverkamp merged commit 88554cf into rust-vmm:main Oct 25, 2021
@gsserge gsserge deleted the duplicate_phandle branch October 26, 2021 07:51
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.

None yet

3 participants