Dummy PR for proxy bindings #3

alexandruag · 2020-09-07T16:49:48Z

No description provided.

Signed-off-by: Alexandru Agache <[email protected]>

jiangliu · 2020-09-08T03:00:56Z

Once we have run into a trap to separate data structs and their state structs.
The trap may get solved if we will group kvm-ioctls, kvm-bindings, kvm-states into a workspace.

acatangiu

Nice!

acatangiu · 2020-09-08T06:13:00Z

src/kvm_bindings_versionize/src/x86/bindings/tests.rs

+use super::*;
+
+#[test]
+fn bindgen_test_layout_kvm_pic_state() {


Since these structs are shadowing the upstream ones, I think the layout tests for them should compare size, alignment, etc against the upstream struct instead of hardcoded values.
This would improve on the mechanisms to keep them in sync.

Yeah, what wasn't clearly presented is the layout tests are actually the ones generated for the upstream bindings, and so we know the structs match.

The exact layout tests are generated by upstream, but they're now disconnected from upstream.

If we upgrade used upstream version, the values hardcoded here (generated with previous upstream version) might be stale.

If these tests would instead compare local structs against imported/upstream structs, the code here would not get stale and would work correctly for any upstream version.

acatangiu · 2020-09-08T06:20:24Z

src/kvm_bindings_versionize/src/lib.rs

+    assert_eq!(align_of::<T>(), align_of::<U>());
+    assert_eq!(size_of::<T>(), size_of::<U>());
+    // Safe because `src` is aligned, the two types have the same size and alignment,
+    // and they are both plain old data structs.


Is there a way to guarantee this? Maybe force them through a dummy FFI or something similar to make sure we don't accidentally break this assumption?

My worry is that the function safety is based on only using it for POD or repr(C) data, but that isn't in any way enforced, and we could accidentally misuse it.

The assumption must hold for kvm bindings, and the unit tests and sanity checks above verify everything except the presence of #[repr(C)], which is where the dummy FFI might come in handy for extra certainty.

acatangiu · 2020-09-08T06:25:00Z

src/kvm_bindings_versionize/src/lib.rs

+// macro (i.e. `use kvm_bindings as upstream;`). It also requires the `convert_bitwise`
+// function above to be in scope.
+#[macro_export]
+macro_rules! impl_conversions {


Cool how conversion is ultimately only casting and thus no/low overhead 👍

Unfortunately this is not just casting, but rather equivalent to a memcpy. My initial measurements didn't detect significant overhead, so maybe it's the compiler that in the end applies some more simplifications.

acatangiu · 2020-09-08T06:26:38Z

src/kvm_bindings_versionize/src/lib.rs

+
+/// Helper function for FamStructWrapper conversion. Can/should be replaced by implementing
+/// `From` for `FamStructWrapper` in `vmm-sys-util`.
+pub fn convert_fam_struct_wrapper<'a, T, U>(src: &'a FamStructWrapper<T>) -> FamStructWrapper<U>


This is reallocation with deep copy, is there any way to do some trickery here as well?

The extent of the copying is comparable to the conversion above, but it can be made to look simpler :D

acatangiu · 2020-09-08T07:18:39Z

How does the versioning-by-proxy work when adding a new field to one of the versioned structs?

I think an example for that would be very useful in getting a complete picture of how the proxy approach works.

alexandruag

Thanks for looking and the comments everyone! Gerry, can you share more details about the trap you mention? Sounds like an interesting situation. Regarding the example, I don't have anything at hand but it should be exactly the same as for the other approach; what's different here is where the Versionize-aware definitions are kept.

alexandruag · 2020-09-09T10:53:53Z

src/kvm_bindings_versionize/src/x86/bindings/tests.rs

+use super::*;
+
+#[test]
+fn bindgen_test_layout_kvm_pic_state() {


Yeah, what wasn't clearly presented is the layout tests are actually the ones generated for the upstream bindings, and so we know the structs match.

alexandruag · 2020-09-09T11:11:28Z

src/kvm_bindings_versionize/src/lib.rs

+    assert_eq!(align_of::<T>(), align_of::<U>());
+    assert_eq!(size_of::<T>(), size_of::<U>());
+    // Safe because `src` is aligned, the two types have the same size and alignment,
+    // and they are both plain old data structs.


The assumption must hold for kvm bindings, and the unit tests and sanity checks above verify everything except the presence of #[repr(C)], which is where the dummy FFI might come in handy for extra certainty.

alexandruag · 2020-09-09T11:12:51Z

src/kvm_bindings_versionize/src/lib.rs

+// macro (i.e. `use kvm_bindings as upstream;`). It also requires the `convert_bitwise`
+// function above to be in scope.
+#[macro_export]
+macro_rules! impl_conversions {


Unfortunately this is not just casting, but rather equivalent to a memcpy. My initial measurements didn't detect significant overhead, so maybe it's the compiler that in the end applies some more simplifications.

alexandruag · 2020-09-09T11:13:46Z

src/kvm_bindings_versionize/src/lib.rs

+
+/// Helper function for FamStructWrapper conversion. Can/should be replaced by implementing
+/// `From` for `FamStructWrapper` in `vmm-sys-util`.
+pub fn convert_fam_struct_wrapper<'a, T, U>(src: &'a FamStructWrapper<T>) -> FamStructWrapper<U>


The extent of the copying is comparable to the conversion above, but it can be made to look simpler :D

acatangiu

Regarding the example, I don't have anything at hand but it should be exactly the same as for the other approach; what's different here is where the Versionize-aware definitions are kept.

In theory it should work the same, but I'm having a hard time getting my head wrapped around those conversions when we have multiple versions/layouts of a struct.

The mechanisms here are quite complex and adding a practical example would greatly help understanding things better.

acatangiu · 2020-09-09T11:33:15Z

src/kvm_bindings_versionize/src/x86/bindings/tests.rs

+use super::*;
+
+#[test]
+fn bindgen_test_layout_kvm_pic_state() {


The exact layout tests are generated by upstream, but they're now disconnected from upstream.

If we upgrade used upstream version, the values hardcoded here (generated with previous upstream version) might be stale.

If these tests would instead compare local structs against imported/upstream structs, the code here would not get stale and would work correctly for any upstream version.

alexandruag · 2020-09-10T12:47:20Z

Hmm, can't reply to the individual comments for some reason. Yeah, the staleness aspect is supposed to be addressed when changing the upstream version, and it wasn't captured here. One way of doing it that's aligned with your proposal is to include! the same autogenerated test code in two separate modules. The first exports (i.e. pub use ...) the local bindings, while the second does the same for the upstream bindings. This kind of approach seemed a bit ugly initially, but it's quite nice on second thought.

An example is definitely useful, but until one will be available just wanted to clarify the similarity aspect a bit. In both approaches there is are structs annotated with Versionize (or with hand written logic here and there), and this remains exactly the same. What's different with proxy is the extra conversion step that takes place before/after serialization/deserialization, and which is completely agnostic to serialization details.

alexandruag added 3 commits September 7, 2020 19:48

add local kvm-bindings-versionize crate

25fd902

Signed-off-by: Alexandru Agache <[email protected]>

add conversion logic

186b717

Signed-off-by: Alexandru Agache <[email protected]>

switch back to upstream bindings and ioctls

3402af8

Signed-off-by: Alexandru Agache <[email protected]>

alexandruag mentioned this pull request Sep 7, 2020

Support for serializing/deserializing KVM state rust-vmm/kvm-bindings#24

Closed

acatangiu reviewed Sep 8, 2020

View reviewed changes

alexandruag commented Sep 9, 2020

View reviewed changes

acatangiu reviewed Sep 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dummy PR for proxy bindings #3

Dummy PR for proxy bindings #3

alexandruag commented Sep 7, 2020

jiangliu commented Sep 8, 2020

acatangiu left a comment

acatangiu Sep 8, 2020

alexandruag Sep 9, 2020

acatangiu Sep 9, 2020

acatangiu Sep 8, 2020

alexandruag Sep 9, 2020

acatangiu Sep 8, 2020

alexandruag Sep 9, 2020

acatangiu Sep 8, 2020

alexandruag Sep 9, 2020

acatangiu commented Sep 8, 2020

alexandruag left a comment

alexandruag Sep 9, 2020

alexandruag Sep 9, 2020

alexandruag Sep 9, 2020

alexandruag Sep 9, 2020

acatangiu left a comment

acatangiu Sep 9, 2020

alexandruag commented Sep 10, 2020

Dummy PR for proxy bindings #3

Are you sure you want to change the base?

Dummy PR for proxy bindings #3

Conversation

alexandruag commented Sep 7, 2020

jiangliu commented Sep 8, 2020

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu commented Sep 8, 2020

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandruag commented Sep 10, 2020