RFC: enhance kvm-binding to optionally support serde

[jiangliu]

This PR implements serde for kvm-binding based on the proposed vmm-serde crate (rust-vmm/community#76).
It's based on the @aghecenco work (#4), but optionally implementing de/serialization by using rust proc macro.
It implements the Serialize/Deserialize trait by:

1. post-process source files generated by the bindgen, adding `#[derive(SerializeFfi, DeserializeFfi/DeserializeFfiFam)]' to suitable structs.
2. the vmm_serde crate implements proc_macro_derive(SerializeFfi, DeseriailizeFfi, DeserializeFfiFam) by using the serde_bytes crate.
3. Implement trait SizeofFamStruct to support data structures with flexible array member.

The first patch remvoes Cargo.lock from the crate, the second patch imports add_serde.py from @aghecenco previous work with modification, the third patch builds the infrastructure to serde, and the last patch commits code generated by add_serde.py.

[yisun-git]

Can serde.rs be common, i.e. work for arm too?

[jiangliu]

Can serde.rs be common, i.e. work for arm too?
serde is platform independent, but I have no ARM env available so only enabled it fort x86.

[yisun-git]

On 19-09-24 18:57:26, Jiang Liu wrote: > Can serde.rs be common, i.e. work for arm too? serde is platform independent, but I have no ARM env available so only enabled it fort x86.

Ok. There are some data structures are common so I think we may propagate at least part of serde.rs to upper layer. We can do it when there is real requirement.

…

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #7 (comment)

[bjzhjing]

@jiangliu For kvm_nested_state, is it sensible to have serde_ffi_fam_impl implemented on it? The structure does not meet the condition to work with SizeofFamStruct trait. Cargo test --all-features has associated failures. Please help confirm this.

[alxiord]

Hey @jiangliu, I've been giving this extensive thought and come to the following conclusions:

• I no longer agree with the FFI byte-dump serialization approach. It's unsafe, it's nearly impossible to debug, and very tricky to work around in case of a corrupt snapshot. I much rather prefer the initial solution: let serde do its thing and serialize structs as structs, giving the crate consumer the highest granularity of the data and the possibility to dump it in a (machine and) human readable format like JSON.
• I also no longer agree with autogenerating serialization support. Snapshot deserialization essentially introduces a new attack surface, so I advocate for paying close attention to everything that's read from a file into process memory. That means manual inspection of every serializable item, and careful considering whether serialization is absolutely necessary for a binding / field. (For instance, padding should alway be skipped.)

I initially formulated these points in #11 but that didn't exactly generate traction so would you take a peek at #14 instead?

[jiangliu]

Hey @jiangliu, I've been giving this extensive thought and come to the following conclusions:

• I no longer agree with the FFI byte-dump serialization approach. It's unsafe, it's nearly impossible to debug, and very tricky to work around in case of a corrupt snapshot. I much rather prefer the initial solution: let serde do its thing and serialize structs as structs, giving the crate consumer the highest granularity of the data and the possibility to dump it in a (machine and) human readable format like JSON.
• I also no longer agree with autogenerating serialization support. Snapshot deserialization essentially introduces a new attack surface, so I advocate for paying close attention to everything that's read from a file into process memory. That means manual inspection of every serializable item, and careful considering whether serialization is absolutely necessary for a binding / field. (For instance, padding should alway be skipped.)

I initially formulated these points in #11 but that didn't exactly generate traction so would you take a peek at #14 instead?

Hi Alexandra,
Thanks for the great work. I'm work with hand-writing serde implementations for kvm-bindings, and it's step forward towards hand-writing kvm-binding itself:)
Apparently, snapshot has more strict requirements on kvm-binding serde implementation than live-upgrading. So it should ok ok for us to migrate to the new implementation.
Thanks for your great work again:)

[sboeuf]

Trying to revive this thread! I'd really like to see every project rely on the same kvm-bindings crate, hence I'd like to propose that we all decide to move to versionize crate. Cloud-Hypervisor does not rely on it yet, but I'd be totally fine making the change since I can see how versionize can help everybody handle versioning in the future.

This means we'd need to send a PR with this patch firecracker-microvm@b581698 to derive all KVM structures with Versionize. That would give us a kvm-bindings crate usable both from Firecracker and Cloud-Hypervisor (after we'd do the necessary changes).

That being said, if we agree on this move, I think it'd be important to move versionize to rust-vmm as it's kinda weird having a rust-vmm crate relying on a Firecracker crate :) (should be the other way around).

@sameo @rbradford @aghecenco @andreeaflorescu @jiangliu

[jiangliu]

Trying to revive this thread! I'd really like to see every project rely on the same kvm-bindings crate, hence I'd like to propose that we all decide to move to versionize crate. Cloud-Hypervisor does not rely on it yet, but I'd be totally fine making the change since I can see how versionize can help everybody handle versioning in the future.

This means we'd need to send a PR with this patch firecracker-microvm@b581698 to derive all KVM structures with Versionize. That would give us a kvm-bindings crate usable both from Firecracker and Cloud-Hypervisor (after we'd do the necessary changes).

That being said, if we agree on this move, I think it'd be important to move versionize to rust-vmm as it's kinda weird having a rust-vmm crate relying on a Firecracker crate :) (should be the other way around).

@sameo @rbradford @aghecenco @andreeaflorescu @jiangliu

I'm OK to migrate to versionize crate once it's moved to rust-vmm, so we could have common code base.
We can migrate our vmm-serde based implementation to versionize crate.

[andreeaflorescu]

We were also looking at using a proxy crate for the state instead of adding the serialization macros in the kvm-bindings structure directly. I find adding either serde or versionize macros directly in kvm-bindings as a bit too intrusive because there is not getting away from it :))

I believe @alexandruag is working on a prototype, and can probably give you some more details.

Are we actually okay with closing this PR or do we want to keep it open for the discussions?

[jiangliu]

It's OK to close this PR and open another issue for discussion.

[sboeuf]

We were also looking at using a proxy crate for the state instead of adding the serialization macros in the kvm-bindings structure directly. I find adding either serde or versionize macros directly in kvm-bindings as a bit too intrusive because there is not getting away from it :))

But if we add one more layer between versionize and kvm-bindings, this will introduce lots of duplication and maintenance, won't it? As long as we agree that versionize can live on rust-vmm, we can simply decide that deriving kvm-bindings structures with Versionize is acceptable.

I believe @alexandruag is working on a prototype, and can probably give you some more details.

Cool!

Are we actually okay with closing this PR or do we want to keep it open for the discussions?

Let's keep it open for the discussion.

[andreeaflorescu]

But if we add one more layer between versionize and kvm-bindings, this will introduce lots of duplication and maintenance, won't it?

@sboeuf I don't think that the cost is so high TBH, but then it's probably better to discuss it on the actual proposal :)) I have a few concerns with adding serialize support directly on the bindings. Over time it can add a non-negligible size overhead. Then the second thing is that bindings in their current form (with the padding definition and FAM structs) is not exactly the "KVM state" in a sens.

[sboeuf]

Closing to continue the discussion on #24