rust-vmm / vm-fdt Goto Github PK

We can improve the vm-fdt abstractions after we publish an initial version of the crate so that we do not block the first release.

The current interface was built on purpose to be similar to the libfdt one, but we can optimize it (slightly) for Rust.

Things that can be improved are better abstractions for nodes. For example, we can have the property_* functions on the FdtWriterNode instead of FdtWriter so that we can disallow properties to be added in non valid locations (see #19).

In this section we can specify how the vm-fdt can be used and why. For example, we can write that the vm-fdt provides a way for exporting the VM topology.

I was thinking about making this crate no_std either by default or behind a feature for now (at least until the error is stabilized in core). I gave it a try and it was pretty easy to convert it to no_std: https://github.com/andreeaflorescu/vm-fdt/tree/no_std. This should make the crate available to be used in embeded as well.

Use checked arithmetic for operations that can fail. In debug mode these cause panics, in release mode they just wrap around.

For example, this operation should use checked_sub.

Below it's a regression test that triggers a panic in debug mode:

    #[test]
    fn test_overflow_subtract() {
        let overflow_size = u32::MAX / size_of::<FdtReserveEntry>() as u32 - 3;
        let too_large_mem_reserve = vec![FdtReserveEntry::default(); overflow_size as usize];
        let mut fdt = FdtWriter::new(&too_large_mem_reserve);
        let root_node = fdt.begin_node("").unwrap();
        fdt.end_node(root_node).unwrap();
        fdt.finish(0x48).unwrap();
    }

In the current implementation of the FdtWriter, the maximum size of the DTB is checked at the end. If the size of the DTB is larger than the specified max_size an error is returned, otherwise DTB is padded such that the total size is max_size.

Chatted with @danielverkamp and this is a leftover from when they were using the C-based libfdt in crosvm, and it does not make a lot of sense to keep it this way.

The options we currently have are:

1. removing this check altogether
2. move the initialization of max_size to the new function, and in finish just check that the size is less then max_size without padding the structure with zeroes (as this is not actually a requirement).

Since the caller of the FDT interface is trusted, there is not a lot of sense to add runtime checks for each function that increases the DTB size (the DTB size is directly proportional to the number of function calls in the public interface) as this would introduce delays.

I am fine with either approach (1 or 2), but I was wondering if anyone else has some strong preferences here.

Pinging a few more people: @jiangliu @alexandruag @lauralt @slp @dianpopa

Running the regression tests for things such as overflows (see:

One option might be to have these kind of tests under a feature since the CI runs the tests with all features enabled. This might be a bit awkward since it would a feature only used in tests. Having them with the ignore macro might not be a good option because it requires adding a specialized CI pipeline to run the ignored tests (or maybe this is a good option?). Are there any options here?

Right now the following code works:

let mut fdt = FdtWriter::new().unwrap();
fdt.property_string("bla", "blabla").unwrap();

But I think that properties can only be added to nodes, and thus we should return an error here instead. To do that, we can check that node_depth != 0, which means that at least one node is created, and the property thus belongs to that node.

The Devicetree spec says that the memory reservations regions must not overlap.

Should we add a check for this as well to be compliant with the specification? I am not sure if the kernel is checking that as well, but it sounds like we can exit early on errors.

@danielverkamp what do you think?

• Update README.md (fill in or remove the existing TODOs)
• Check if we need to update the public interface documentation
• Fix unchecked conversions from usize to u32
• Fix unchecked arithmetic conversions which might lead panics in debug mode
• Check that the memory reservations are valid
• Update interface to remove libfdt-related leftorver
• #19

This conversion from usize to u32 can fail when passing large arrays in the mem_reservations parameter.

The user of this library is considered trusted, but we can still check it and return a Result<Self> in the new function.

The FDT specification describes a string length of e.g. the begin_node of 1 to 31 chars and enforces a strict character limitation described in https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#node-name-requirements.

Enforcing the character limitation would mean adding a dependency on regex, which is undesirable. We could instead enforce the size limitation, and document the character limitation as something that is not currently validated.

@danielverkamp what do you think?

The phandle property can be used to reference another node within the FDT uniquely: https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html?#phandle

Right now we are not checking the uniqueness of the phandle, and I think there are a few things that we can do:

• create a public API for retrieving a unique phandle, similar to what is implemented in libfdt: https://github.com/torvalds/linux/blob/5bfc75d92efd494db37f5c4c173d3639d4772966/scripts/dtc/libfdt/fdt_ro.c#L114
• add support for the phandle property; we could for example offer an API for setting the phandle of a node (once we clean up the abstractions to have a clear ownership between nodes & properties), and in the set function we can check for uniqueness. This would imply denying adding properties with the name phandle, or just check the name of the property, and in case it is phandle, check for uniqueness. It looks like this adds branching in the code, and we should discuss if we just want to go ahead with documenting this, or enforcing it.