mbox series

[00/17] rust/vmstate: Clean up, fix, enhance & test

Message ID 20250317151236.536673-1-zhao1.liu@intel.com (mailing list archive)
Headers show
Series rust/vmstate: Clean up, fix, enhance & test | expand

Message

Zhao Liu March 17, 2025, 3:12 p.m. UTC
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

Comments

Paolo Bonzini March 17, 2025, 5:20 p.m. UTC | #1
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
Zhao Liu March 18, 2025, 6:49 a.m. UTC | #2
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