| Message ID | 20250317151236.536673-1-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rust/vmstate: Clean up, fix, enhance & test | expand |
On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote: > Hi, > > This series is in preparation for HPET migration support (in particular, > to support varray and vmstate_validate), and it also cleans up and fixes > the current vmstate! However, there is still the gap from being a ‘stable’ > vmstate. Already a great improvement! I quickly went through things that I was planning to do in a slightly different way, but overall this is great work. Give me a day or two to finish reviewing and testing, most of it can already be applied to qemu-rust or even to 10.0. Thanks, Paolo
On Mon, Mar 17, 2025 at 06:20:15PM +0100, Paolo Bonzini wrote: > Date: Mon, 17 Mar 2025 18:20:15 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 00/17] rust/vmstate: Clean up, fix, enhance & test > > On Mon, Mar 17, 2025 at 3:52 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > Hi, > > > > This series is in preparation for HPET migration support (in particular, > > to support varray and vmstate_validate), and it also cleans up and fixes > > the current vmstate! However, there is still the gap from being a ‘stable’ > > vmstate. > > Already a great improvement! > > I quickly went through things that I was planning to do in a slightly > different way, but overall this is great work. Give me a day or two > to finish reviewing and testing, most of it can already be applied to > qemu-rust or even to 10.0. Thanks for your quick feedback! I think the main open now is about the best way to implement `field_exists`... pls see my patch 13 reply. I can quickly refresh v2 to help you apply! Regards, Zhao
Hi, This series is in preparation for HPET migration support (in particular, to support varray and vmstate_validate), and it also cleans up and fixes the current vmstate! However, there is still the gap from being a ‘stable’ vmstate. Patch Summary ============= Patch 1-11: Clean up & fix for vmstate_of & vmstate_struct, where the issues are catched by unit tests. Patch 12,13: Add versioned vmstate and vmstate_validate support, and vmstate_validate can accept safe "test" callback. Patch 13-17: Add unit test to cover as much as possible cases to be compatible with C version macros. * Note while in principle Rust's vmstate pattern doesn't have to match the C version, the C vmstate macros are rich enough to cover as much logic as possible. So checking against the C version is the most effective way to detect the error. With unit tests, the 2 vmstate gaps that come to mind right now are: * `test` parameter in vmstate_of/vmstate_struct. I think there's not too much difficulty, since referring to vmstate_validate makes it straightforward... * pointer to `struct`. assert_field_type() can't aware a inner type of the pointer. We may need another trait (different from impl_vmstate_pointer). Or, we may need to use the new vmstate_struct_ptr macro. But I haven't tried it in details yet. Thoughts about 'stable' vmstate =============================== To make vmstate 'stable', I think one key point is to make vmstate related things accept "safe" callbacks. vmstate_validate (and future `test` parameters) has achieved this. But VMStateDescription hasn't and it has the following callbacks: int (*pre_load)(void *opaque); int (*post_load)(void *opaque, int version_id); int (*pre_save)(void *opaque); int (*post_save)(void *opaque); bool (*needed)(void *opaque); bool (*dev_unplug_pending)(void *opaque); I find there would be 2 options to address this: 1. introduce macros (like vmstate_validate) to accept "safe" callback. For example, static VMSTATE_HPET: VMStateDescription = VMStateDescription { name: c_str!("hpet").as_ptr(), version_id: 2, minimum_version_id: 1, pre_save: pre_save!(hpet_pre_save), // the pre_save macro will convert "safe" hpet_pre_save() to C style callback. post_load: post_load!(hpet_post_load), // ditto fields: vmstate_fields! { vmstate_of!(HPETState, config), vmstate_of!(HPETState, int_status), vmstate_of!(HPETState, counter), vmstate_of!(HPETState, num_timers, version = 2), vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers), vmstate_struct!(HPETState, timers, [0 .. num_timers], VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, version = 0), }, subsections: vmstate_subsections! { VMSTATE_HPET_RTC_IRQ_LEVEL, VMSTATE_HPET_OFFSET, }, ..Zeroable::ZERO }; 2. introduce VMStateDescriptionBuilder (like MemoryRegionOpsBuilder) and use the call chain to accept callbacks and initialize VMStateField. About these 2 options, which one do you like? Best Regards, Zhao --- Zhao Liu (17): rust/vmstate: Remove unnecessary unsafe rust/vmstate: Fix num_offset in vmstate macros rust/vmstate: Add a prefix separator ", " for the array field in vmstate macros rust/vmstate: Use ident instead of expr to parse vmsd in vmstate_struct macro rust/vmstate: Fix num field when varray flags are set rust/vmstate: Fix size field of VMStateField with VMS_ARRAY_OF_POINTER flag rust/vmstate: Fix type check for varray in vmstate_struct rust/vmstate: Fix "cannot infer type" error in vmstate_struct rust/vmstate: Fix unnecessary VMState bound of with_varray_flag() rust/vmstate: Relax array check when build varray in vmstate_struct rust/vmstate: Re-implement VMState trait for timer binding rust/vmstate: Support version field in vmstate macros rust/vmstate: Support vmstate_validate rust/vmstate: Add unit test for vmstate_of macro rust/vmstate: Add unit test for vmstate_{of|struct} macro rust/vmstate: Add unit test for pointer case rust/vmstate: Add unit test for vmstate_validate rust/hw/char/pl011/src/device_class.rs | 2 +- rust/qemu-api/meson.build | 5 +- rust/qemu-api/src/assertions.rs | 15 + rust/qemu-api/src/vmstate.rs | 110 ++++-- rust/qemu-api/tests/tests.rs | 2 + rust/qemu-api/tests/vmstate_tests.rs | 467 +++++++++++++++++++++++++ 6 files changed, 575 insertions(+), 26 deletions(-) create mode 100644 rust/qemu-api/tests/vmstate_tests.rs