Add serde support for bindings commonly used in migration/snapshotting

[roypat]

On crates.io, kvm-bindings has ~1.1M less downloads than kvm-ioctls, even though it is a non-optional dependency of the latter. The reason for this is that most project using kvm-ioctls do not actually depend rust-vmm/kvm-bindings, but instead use crates-io.patch directives in Cargo.toml to overwrite kvm-ioctl's dependency on kvm-bindings with a local fork. These forks usually serve a common need: supporting the (de)serialization of various structures define in kvm-bindings to allow for VM migration/snapshotting. As a result from this, these forks are also all nearly identical, making inclusion of the functionality upstream an obvious idea. Maintaining these forks also adds quite a bit of overhead for downstream rust-vmm users when it comes to consuming kvm-bindings updates, as all serialization-related functionality has to be rebased on top of upstream (which was particularly painful for the recent upgrade to 6.5 bindings).

There has been some discussion about including (de)serialization support in upstream rust-vmm a few years ago (#25, #24), with the conclusion as I understand it being that people were open to having a serialization solution upstream, but this seemingly never manifested. In this RFC, I am hoping to pick this thread up again.

The main difference between this PR and the old discussions is that this PR is serde-based instead of versionize-based. This is because all hypervisor projects I am aware of have switched to using serde for their migration/snapshotting needs (Firecracker itself switched earlier this year). There were also concerns about not wanting to take dependencies on crates outside the rust-vmm organization (as versionize is maintained by the Firecracker team), which are also alleviated by using serde (which is the de-facto industry standard for serialization in Rust).

This PR implements/derives Serialize and Deserialize for the minimum set of items needed by Firecracker and CloudHypervisor (e.g. both Firecracker and CloudHypervisor compile if their respective forks are replaced with this branch, both on x86_64 and aarch64). These impls are hidden behind a with-serde feature flag (name chosen to match the corresponding flag in vmm-sys-util). Thanks to how cargo's feature resolver works, we will not need to "feed" this flag through to kvm-ioctls or other reverse-dependencies. If a downstream crate enables the with-serde feature, it will get enabled globally (meaning crates that enable the feature can interoperate with those that do not).

Open Questions

There are a handful of open question that still need some thought before we can move forward with this:

• This PR adds serde support for a minimal set of items that was derived from Firecracker/CloudHypervisor needs. This might not match the needs of other users (and we might end up seeing a lot of PRs adding serde support for additional items). Should we try to add derives for more items from the get-go? We only add support for a minimal set needed by FC/CH. If needed, more implementations can be added in patch releases
• This PR modifies bindgen-generated code, meaning the derives for (De)Serialize would have to be manually readded every time the bindings are regenerated. This makes it easy to accidentally remove implementations. One solution to this would be to have a separate file that statically asserts the serde traits are implements (e.g. fn has_serde<T: Serialize + Deserialize>() {} and then a unit test that just does has_serde::<kvm_regs>() etc for every item we want to have serde support for). Another solution would be to instruct bindgen to automatically derive Serialize/Deserialize for every struct/enum, and have a separate file that contains manual implementations for items the derive fails (e.g. unions). This would also address point 1, although would significantly balloon the set of changes. We have implemented tests that ensure we do not regress on serde implementations.
  • Related to this is the question of whether we want to keep manual implementation of (De)Serializer that are required for various type (such as unions) to live in-line in bindings.rs, or inside a separate module (so that during bindings regeneration, we would really only need to readd derives and nothing more) Manual implementations are in a separate module, together with the tests.

[andreeaflorescu]

This is a great initiative! I think bindgen now supports adding custom derives to structures, so we might be able to add serialize/deserialize when generating the bindings: rust-lang/rust-bindgen#1089. I didn't try it myself to see how it works, but his might solve point 2. I would thus not be very concerned about the manual derives as long as we have tests in separate files (that are not autogenerated), in which we test that we don't regress on serialize/deserialize by mistake.

CC: @rbradford @sboeuf @jiangliu (as you were also involved in the initial discussions). Would this help with removing the need for a local fork for the projects you're working on?

[roypat]

This is a great initiative! I think bindgen now supports adding custom derives to structures, so we might be able to add serialize/deserialize when generating the bindings: rust-lang/rust-bindgen#1089. I didn't try it myself to see how it works, but his might solve point 2. I would thus not be very concerned about the manual derives as long as we have tests in separate files (that are not autogenerated), in which we test that we don't regress on serialize/deserialize by mistake.

Oh, neat, didn't know bindgen had support for that! Although it does seem like it does not support putting those derives behind a feature flag (the cli option --with-derive-custom says "Derive custom traits on any kind of type. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros") :(

I guess what would make the process the least painful is some comment in the test being like "if this test fails, make sure all the below structs/etc. have a #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize)] attribute", and all manual impls moved into some serialize.rs so that they dont get overwritten.

[roypat]

Alright, I've updated the PR to separate manual (De)Serialize implementations into their own serialize.rs files, and also added some compile-time tests to ensure we don't "loose" implementations when we regenerate the bindings. I've also brought the (De)Serialize implementations inline with CloudHypervisor's current fork, based on conversations with @rbradford about their live update needs

[rbradford]

@likebreath Ping! I think your feedback would be good on this!

[roypat]

Summarizing some comments from cloud-hypervisor/cloud-hypervisor#6272 (comment):

CloudHypervisor supports live upgrade across minor versions, so a serialization solution that would be backwards compatible across kernel upgrades would be ideal. One option to achieve this would be to just serialize all kvm-bindings structures as "byte array + len". They're all repr(C), so it would be sound, and the backwards compatibility of Linux's ABI would guarantee us that migrating the same serialized data across bindings generated from different kernel versions would not break:

• If inside some struct the kernel changes reserved bytes to actually have some meaning, this is completely transparent to a byte-dump of the structure
• If the kernel adds some fields to the end of a structure, we can detect by comparing the serialized length with size_of::<T>(), and appending zeros
• If we live-downgrade and to a kvm-bindings version where a field does not yet exist, we can truncate the extra bytes (although this probably would break the guest).

This would also have the advantage that we would not have to add derives to the bindgen generated code, as I envision the (De)Serialize implementations to be generated by a declarative macro defined in serialize.rs. This is not quite accurate, as the bindgen types will still need to implement a trait such as zerocopy::FromBytes.

I will probably have a go at implementing a prototype of this the next few days, but please let me know already if you have any comments/concerns!

/cc @likebreath

[rbradford]

Ideally we would like to support live migration across major versions too - we don't need to be able to support downgrading across versions though. I really like your proposal of treating the bindings (for the purpose of migration) as an opaque byte array which could be bigger at the destination vs the source.

[roypat]

Ideally we would like to support live migration across major versions too - we don't need to be able to support downgrading across versions though. I really like your proposal of treating the bindings (for the purpose of migration) as an opaque byte array which could be bigger at the destination vs the source.

I think with the byte array approach to serialization you'll achieve that goal, however migrating from the current (De)Serialize implementations to the byte-array approach will be a breaking change :(

That being said, I've updated this PR to use zerocopy for the serialization. The changes to bindings.rs are a bit uglier, so I guess we'll definitely need to write some documentation on how to regenerate bindings if we go forward with this.

[rbradford]

Ideally we would like to support live migration across major versions too - we don't need to be able to support downgrading across versions though. I really like your proposal of treating the bindings (for the purpose of migration) as an opaque byte array which could be bigger at the destination vs the source.

I think with the byte array approach to serialization you'll achieve that goal, however migrating from the current (De)Serialize implementations to the byte-array approach will be a breaking change :(

That being said, I've updated this PR to use zerocopy for the serialization. The changes to bindings.rs are a bit uglier, so I guess we'll definitely need to write some documentation on how to regenerate bindings if we go forward with this.

That's fine - we can switch CH to use this on a new major version. We don't advertise support for major version migration because there are still issues like this that need to be shaken out. Thank you very much for tackling this - it will be a massive improvement.

[roypat]

Hey all,
I've removed the "RFC" tag from the commit title, since I think we've aligned on a decent solution for serialization support (and all the original questions are answered). As such, I consider this PR ready for review :)

[likebreath]

Thank you for driving this improvement. I guess this is the best we can do to have a seamless live upgrade across kernel versions, while I still worry about debugging issues caused by serialization as byte array. Hopefully, the kernel ABI backward compatibility ensures there is rarely such thing happening.

With that, I ran some live migration tests of Cloud Hypervisor with this PR, and it works well. It is also very easy to integrate thanks to the feature gate being used that is identical to what we have.

[likebreath]

This means that the kvm_xsave struct serialization is only good for the first 4K bytes, and VMM needs to maintain a vector of kvm_xsave for migration/upgrade (while relying on conversions to kvm_xsave2 for related KVM ioctl), right?

[roypat]

Yes, that's pretty much the idea. I was planning to add support for KVM_GET_XSAVE2 to kvm_ioctls using the Fam struct added in this PR (based on querying the KVM_CAP_XSAVE2 capability to determine the size of the FAM)

[roypat]

friendly ping for reviewers :)

[rbradford]

@likebreath Did you have a chance to try this against Cloud Hypervisor?

[likebreath]

@likebreath Did you have a chance to try this against Cloud Hypervisor?

Yes, I did. As mentioned in the comment above #101 (review), the switch to this fork was easy and our live migration tests were working fine.

[roypat]

(no functional changes since the last revision you reviewed, Rob, only documentation updates)

[rbradford]

@roypat Thanks so much for working on this - we're going to move CH over to use this soon!

[sboeuf]

Awesome, this is solving a long haul issue 👍