From f88c9cd804804360fa4b3586d7d2f84505ab8c26 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Wed, 19 Mar 2025 19:31:09 +0000 Subject: [PATCH 01/24] rust: Kconfig: Factor out whether PL011 is Rust or C MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently every board that uses the PL011 duplicates the logic that selects the Rust implementation if Rust was enabled and the C implementation if it does not. Factor this out into a separate Kconfig stanza, so that boards can go back to simply doing "select PL011" and get whichever implementation is correct for the build. This fixes a compilation failure if CONFIG_VMAPPLE is enabled in a Rust build, because hw/vmapple/Kconfig didn't have the "pick the Rust PL011 if Rust is enabled" logic in it. Fixes: 59f4d65584bd33 ("hw/vmapple/vmapple: Add vmapple machine type") Reported-by: Tanish Desai Analyzed-by: Tanish Desai Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Link: https://lore.kernel.org/r/20250319193110.1565578-2-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- hw/arm/Kconfig | 30 ++++++++++-------------------- hw/char/Kconfig | 6 ++++++ hw/char/meson.build | 2 +- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index 15200a2d7e..a55b44d7bd 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -21,8 +21,7 @@ config ARM_VIRT select PCI_EXPRESS select PCI_EXPRESS_GENERIC_BRIDGE select PFLASH_CFI01 - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL031 # RTC select PL061 # GPIO select GPIO_PWR @@ -75,8 +74,7 @@ config HIGHBANK select AHCI_SYSBUS select ARM_TIMER # sp804 select ARM_V7M - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL022 # SPI select PL031 # RTC select PL061 # GPIO @@ -89,8 +87,7 @@ config INTEGRATOR depends on TCG && ARM select ARM_TIMER select INTEGRATOR_DEBUG - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL031 # RTC select PL041 # audio select PL050 # keyboard/mouse @@ -108,8 +105,7 @@ config MUSCA default y depends on TCG && ARM select ARMSSE - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL031 select SPLIT_IRQ select UNIMP @@ -173,8 +169,7 @@ config REALVIEW select WM8750 # audio codec select LSI_SCSI_PCI select PCI - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL031 # RTC select PL041 # audio codec select PL050 # keyboard/mouse @@ -199,8 +194,7 @@ config SBSA_REF select PCI_EXPRESS select PCI_EXPRESS_GENERIC_BRIDGE select PFLASH_CFI01 - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL031 # RTC select PL061 # GPIO select USB_XHCI_SYSBUS @@ -224,8 +218,7 @@ config STELLARIS select ARM_V7M select CMSDK_APB_WATCHDOG select I2C - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL022 # SPI select PL061 # GPIO select SSD0303 # OLED display @@ -285,8 +278,7 @@ config VEXPRESS select ARM_TIMER # sp804 select LAN9118 select PFLASH_CFI01 - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select PL041 # audio codec select PL181 # display select REALVIEW @@ -371,8 +363,7 @@ config RASPI default y depends on TCG && ARM select FRAMEBUFFER - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select SDHCI select USB_DWC2 select BCM2835_SPI @@ -448,8 +439,7 @@ config XLNX_VERSAL select ARM_GIC select CPU_CLUSTER select DEVICE_TREE - select PL011 if !HAVE_RUST # UART - select X_PL011_RUST if HAVE_RUST # UART + select PL011 # UART select CADENCE select VIRTIO_MMIO select UNIMP diff --git a/hw/char/Kconfig b/hw/char/Kconfig index 3f702565e6..9d517f3e28 100644 --- a/hw/char/Kconfig +++ b/hw/char/Kconfig @@ -11,6 +11,12 @@ config PARALLEL config PL011 bool + # The PL011 has both a Rust and a C implementation + select PL011_C if !HAVE_RUST + select X_PL011_RUST if HAVE_RUST + +config PL011_C + bool config SERIAL bool diff --git a/hw/char/meson.build b/hw/char/meson.build index 86ee808cae..4e439da8b9 100644 --- a/hw/char/meson.build +++ b/hw/char/meson.build @@ -9,7 +9,7 @@ system_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('parallel-isa.c')) system_ss.add(when: 'CONFIG_ISA_DEBUG', if_true: files('debugcon.c')) system_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_uart.c')) system_ss.add(when: 'CONFIG_PARALLEL', if_true: files('parallel.c')) -system_ss.add(when: 'CONFIG_PL011', if_true: files('pl011.c')) +system_ss.add(when: 'CONFIG_PL011_C', if_true: files('pl011.c')) system_ss.add(when: 'CONFIG_SCLPCONSOLE', if_true: files('sclpconsole.c', 'sclpconsole-lm.c')) system_ss.add(when: 'CONFIG_SERIAL', if_true: files('serial.c')) system_ss.add(when: 'CONFIG_SERIAL_ISA', if_true: files('serial-isa.c')) From d1368344bc9bb251080507940f2bad16048d2687 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Wed, 19 Mar 2025 19:31:10 +0000 Subject: [PATCH 02/24] rust: Kconfig: Factor out whether HPET is Rust or C Currently we require everywhere that wants to know if there is an HPET device to check for "CONFIG_HPET || CONFIG_X_HPET_RUST". Factor out whether the HPET device is Rust or C into a separate Kconfig stanza, so that CONFIG_HPET means "there is an HPET", and whether this has pulled in CONFIG_X_HPET_RUST or CONFIG_HPET_C is something the rest of QEMU can ignore. Signed-off-by: Peter Maydell Link: https://lore.kernel.org/r/20250319193110.1565578-3-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- configs/devices/i386-softmmu/default.mak | 1 - hw/i386/fw_cfg.c | 2 +- hw/i386/pc.c | 2 +- hw/timer/Kconfig | 8 +++++++- hw/timer/meson.build | 2 +- rust/hw/timer/Kconfig | 1 - tests/qtest/meson.build | 3 +-- 7 files changed, 11 insertions(+), 8 deletions(-) diff --git a/configs/devices/i386-softmmu/default.mak b/configs/devices/i386-softmmu/default.mak index 9ef343cace..4faf2f0315 100644 --- a/configs/devices/i386-softmmu/default.mak +++ b/configs/devices/i386-softmmu/default.mak @@ -6,7 +6,6 @@ #CONFIG_APPLESMC=n #CONFIG_FDC=n #CONFIG_HPET=n -#CONFIG_X_HPET_RUST=n #CONFIG_HYPERV=n #CONFIG_ISA_DEBUG=n #CONFIG_ISA_IPMI_BT=n diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c index a7f1b60b98..5c0bcd5f8a 100644 --- a/hw/i386/fw_cfg.c +++ b/hw/i386/fw_cfg.c @@ -26,7 +26,7 @@ #include CONFIG_DEVICES #include "target/i386/cpu.h" -#if !defined(CONFIG_HPET) && !defined(CONFIG_X_HPET_RUST) +#if !defined(CONFIG_HPET) struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX}; #endif diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 63a96cd23f..01d0581f62 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1704,7 +1704,7 @@ static void pc_machine_initfn(Object *obj) pcms->sata_enabled = true; pcms->i8042_enabled = true; pcms->max_fw_size = 8 * MiB; -#if defined(CONFIG_HPET) || defined(CONFIG_X_HPET_RUST) +#if defined(CONFIG_HPET) pcms->hpet_enabled = true; #endif pcms->fd_bootchk = true; diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig index 9ac0084534..b3d823ce2c 100644 --- a/hw/timer/Kconfig +++ b/hw/timer/Kconfig @@ -11,7 +11,13 @@ config A9_GTIMER config HPET bool - default y if PC && !HAVE_RUST + default y if PC + # The HPET has both a Rust and a C implementation + select HPET_C if !HAVE_RUST + select X_HPET_RUST if HAVE_RUST + +config HPET_C + bool config I8254 bool diff --git a/hw/timer/meson.build b/hw/timer/meson.build index f5f9eed2d0..178321c029 100644 --- a/hw/timer/meson.build +++ b/hw/timer/meson.build @@ -13,7 +13,7 @@ system_ss.add(when: 'CONFIG_DIGIC', if_true: files('digic-timer.c')) system_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_mct.c')) system_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_pwm.c')) system_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_gptimer.c')) -system_ss.add(when: 'CONFIG_HPET', if_true: files('hpet.c')) +system_ss.add(when: 'CONFIG_HPET_C', if_true: files('hpet.c')) system_ss.add(when: 'CONFIG_I8254', if_true: files('i8254_common.c', 'i8254.c')) system_ss.add(when: 'CONFIG_IMX', if_true: files('imx_epit.c')) system_ss.add(when: 'CONFIG_IMX', if_true: files('imx_gpt.c')) diff --git a/rust/hw/timer/Kconfig b/rust/hw/timer/Kconfig index 42e421317a..afd9803350 100644 --- a/rust/hw/timer/Kconfig +++ b/rust/hw/timer/Kconfig @@ -1,3 +1,2 @@ config X_HPET_RUST bool - default y if PC && HAVE_RUST diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 5a8c1f102c..3136d15e0f 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -103,8 +103,7 @@ qtests_i386 = \ config_all_devices.has_key('CONFIG_VIRTIO_PCI') and \ slirp.found() ? ['virtio-net-failover'] : []) + \ (unpack_edk2_blobs and \ - (config_all_devices.has_key('CONFIG_HPET') or \ - config_all_devices.has_key('CONFIG_X_HPET_RUST')) and \ + config_all_devices.has_key('CONFIG_HPET') and \ config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) + \ qtests_pci + \ qtests_cxl + \ From 7bda68e8e2b0b836639557ddb13d761bdd15a104 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 28 Feb 2025 14:02:03 +0100 Subject: [PATCH 03/24] qdev, rust/hpet: fix type of HPET "timers" property Signed-off-by: Paolo Bonzini --- hw/core/qdev-properties.c | 37 ++++++++++++++++++++++++++++++++++ include/hw/qdev-properties.h | 1 + rust/hw/timer/hpet/src/hpet.rs | 6 +++--- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index c04df3b337..147b3ffd16 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -442,6 +442,43 @@ const PropertyInfo qdev_prop_uint64_checkmask = { .set = set_uint64_checkmask, }; +/* --- pointer-size integer --- */ + +static void get_usize(Object *obj, Visitor *v, const char *name, void *opaque, + Error **errp) +{ + const Property *prop = opaque; + +#if HOST_LONG_BITS == 32 + uint32_t *ptr = object_field_prop_ptr(obj, prop); + visit_type_uint32(v, name, ptr, errp); +#else + uint64_t *ptr = object_field_prop_ptr(obj, prop); + visit_type_uint64(v, name, ptr, errp); +#endif +} + +static void set_usize(Object *obj, Visitor *v, const char *name, void *opaque, + Error **errp) +{ + const Property *prop = opaque; + +#if HOST_LONG_BITS == 32 + uint32_t *ptr = object_field_prop_ptr(obj, prop); + visit_type_uint32(v, name, ptr, errp); +#else + uint64_t *ptr = object_field_prop_ptr(obj, prop); + visit_type_uint64(v, name, ptr, errp); +#endif +} + +const PropertyInfo qdev_prop_usize = { + .type = "usize", + .get = get_usize, + .set = set_usize, + .set_default_value = qdev_propinfo_set_default_value_uint, +}; + /* --- string --- */ static void release_string(Object *obj, const char *name, void *opaque) diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 15fcec5260..2c99856caa 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -52,6 +52,7 @@ extern const PropertyInfo qdev_prop_bool; extern const PropertyInfo qdev_prop_uint8; extern const PropertyInfo qdev_prop_uint16; extern const PropertyInfo qdev_prop_uint32; +extern const PropertyInfo qdev_prop_usize; extern const PropertyInfo qdev_prop_int32; extern const PropertyInfo qdev_prop_uint64; extern const PropertyInfo qdev_prop_uint64_checkmask; diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs index 20e0afdfca..63c1971f0b 100644 --- a/rust/hw/timer/hpet/src/hpet.rs +++ b/rust/hw/timer/hpet/src/hpet.rs @@ -12,7 +12,7 @@ use std::{ use qemu_api::{ bindings::{ address_space_memory, address_space_stl_le, qdev_prop_bit, qdev_prop_bool, - qdev_prop_uint32, qdev_prop_uint8, + qdev_prop_uint32, qdev_prop_usize, }, c_str, cell::{BqlCell, BqlRefCell}, @@ -859,8 +859,8 @@ qemu_api::declare_properties! { c_str!("timers"), HPETState, num_timers, - unsafe { &qdev_prop_uint8 }, - u8, + unsafe { &qdev_prop_usize }, + usize, default = HPET_MIN_TIMERS ), qemu_api::define_property!( From 9d116f42a38cb95a33da837e0b0b50d91e28906b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 21 Mar 2025 11:25:21 +0000 Subject: [PATCH 04/24] rust: assertions: add static_assert Add a new assertion that is similar to "const { assert!(...) }" but can be used outside functions and with older versions of Rust. A similar macro is found in Linux, whereas the "static_assertions" crate has a const_assert macro that produces worse error messages. Suggested-by: Peter Maydell Signed-off-by: Paolo Bonzini Reviewed-by: Zhao Liu Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell Link: https://lore.kernel.org/r/20250321112523.1774131-2-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/assertions.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs index 104dec3977..bba38cfda1 100644 --- a/rust/qemu-api/src/assertions.rs +++ b/rust/qemu-api/src/assertions.rs @@ -120,3 +120,25 @@ macro_rules! assert_match { ); }; } + +/// Assert at compile time that an expression is true. This is similar +/// to `const { assert!(...); }` but it works outside functions, as well as +/// on versions of Rust before 1.79. +/// +/// # Examples +/// +/// ``` +/// # use qemu_api::static_assert; +/// static_assert!("abc".len() == 3); +/// ``` +/// +/// ```compile_fail +/// # use qemu_api::static_assert; +/// static_assert!("abc".len() == 2); // does not compile +/// ``` +#[macro_export] +macro_rules! static_assert { + ($x:expr) => { + const _: () = assert!($x); + }; +} From 5b87a07e76816ed61e5968eb370859a5901b8516 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 21 Mar 2025 11:25:22 +0000 Subject: [PATCH 05/24] hw/char/pl011: Pad PL011State struct to same size as Rust impl We have some users of the PL011 struct which embed it directly into their own state structs. This means that the Rust version of the device must have a state struct that is the same size or smaller than the C struct. In commit 9b642097d6b7 ("rust: pl011: switch to safe chardev operation") the Rust PL011 state struct changed from having a bindings::CharBackend to a chardev::CharBackend, which made it grow larger than the C version. This results in an assertion at startup when QEMU was built with Rust enabled: $ qemu-system-arm -M raspi2b -display none ERROR:../../qom/object.c:562:object_initialize_with_type: assertion failed: (size >= type->instance_size) The long-term better approach to this problem would be to move our C device code patterns away from "embed a struct" and (back) to "have a pointer to the device", so we can make the C PL011State struct a private implementation detail rather than exposed to its users. For the short term, add a padding field at the end of the C struct so it's big enough that the Rust state struct can fit. Fixes: 9b642097d6b7 ("rust: pl011: switch to safe chardev operation") Reviewed-by: Zhao Liu Signed-off-by: Peter Maydell Link: https://lore.kernel.org/r/20250321112523.1774131-3-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- include/hw/char/pl011.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h index 4fcaf3d7d3..299ca9b18b 100644 --- a/include/hw/char/pl011.h +++ b/include/hw/char/pl011.h @@ -52,6 +52,11 @@ struct PL011State { Clock *clk; bool migrate_clk; const unsigned char *id; + /* + * Since some users embed this struct directly, we must + * ensure that the C struct is at least as big as the Rust one. + */ + uint8_t padding_for_rust[16]; }; DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr); From cc3d262aa93a42e19c38f6acb6d0f6012a71eb4b Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 21 Mar 2025 11:25:23 +0000 Subject: [PATCH 06/24] rust: pl011: Check size of state struct at compile time The PL011 device's C implementation exposes its PL011State struct to users of the device, and one common usage pattern is to embed that struct into the user's own state struct. (The internals of the struct are technically visible to the C user of the device, but in practice are treated as implementation details.) This means that the Rust version of the state struct must not be larger than the C version's struct; otherwise it will trip a runtime assertion in object_initialize_type() when the C user attempts to in-place initialize the type. Add a compile-time assertion on the Rust side, so that if we accidentally make the Rust device state larger we know immediately that we need to expand the padding in the C version of the struct. Reviewed-by: Zhao Liu Signed-off-by: Peter Maydell Link: https://lore.kernel.org/r/20250321112523.1774131-4-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 9 ++++++++- rust/wrapper.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index f137b49fea..bf88e0b00a 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -2,7 +2,7 @@ // Author(s): Manos Pitsidianakis // SPDX-License-Identifier: GPL-2.0-or-later -use std::{ffi::CStr, ptr::addr_of_mut}; +use std::{ffi::CStr, mem::size_of, ptr::addr_of_mut}; use qemu_api::{ chardev::{CharBackend, Chardev, Event}, @@ -12,6 +12,7 @@ use qemu_api::{ prelude::*, qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl}, qom::{ObjectImpl, Owned, ParentField}, + static_assert, sysbus::{SysBusDevice, SysBusDeviceImpl}, vmstate::VMStateDescription, }; @@ -124,6 +125,12 @@ pub struct PL011State { pub migrate_clock: bool, } +// Some C users of this device embed its state struct into their own +// structs, so the size of the Rust version must not be any larger +// than the size of the C one. If this assert triggers you need to +// expand the padding_for_rust[] array in the C PL011State struct. +static_assert!(size_of::() <= size_of::()); + qom_isa!(PL011State : SysBusDevice, DeviceState, Object); #[repr(C)] diff --git a/rust/wrapper.h b/rust/wrapper.h index d927ad6799..d4fec54657 100644 --- a/rust/wrapper.h +++ b/rust/wrapper.h @@ -65,3 +65,4 @@ typedef enum memory_order { #include "exec/memattrs.h" #include "qemu/timer.h" #include "exec/address-spaces.h" +#include "hw/char/pl011.h" From 134ab17fffb32a3f86debb4eec9df12f7f833a3b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 19 Mar 2025 12:54:31 +0100 Subject: [PATCH 07/24] load_aout: replace bswap_needed with big_endian Targets know whether they are big-endian more than they know if the endianness is different from the host: the former is mostly a constant, at least in machine creation code, while the latter has to be computed with TARGET_BIG_ENDIAN != HOST_BIG_ENDIAN or something like that. load_aout, however, takes a "bswap_needed" argument. Replace it with a "big_endian" argument; even though all users are big-endian, it is cheap enough to keep the optional swapping functionality even for little-endian boards. Signed-off-by: Paolo Bonzini --- hw/core/loader.c | 4 ++-- hw/ppc/mac_newworld.c | 7 +------ hw/ppc/mac_oldworld.c | 7 +------ hw/sparc/sun4m.c | 9 +-------- hw/sparc64/sun4u.c | 9 +-------- include/hw/loader.h | 2 +- 6 files changed, 7 insertions(+), 31 deletions(-) diff --git a/hw/core/loader.c b/hw/core/loader.c index ce6ff1b52e..2e35f0aa90 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -226,7 +226,7 @@ static void bswap_ahdr(struct exec *e) ssize_t load_aout(const char *filename, hwaddr addr, int max_sz, - int bswap_needed, hwaddr target_page_size) + bool big_endian, hwaddr target_page_size) { int fd; ssize_t size, ret; @@ -241,7 +241,7 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz, if (size < 0) goto fail; - if (bswap_needed) { + if (big_endian != HOST_BIG_ENDIAN) { bswap_ahdr(&e); } diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c index cb3dc3ab48..2d5309d6f5 100644 --- a/hw/ppc/mac_newworld.c +++ b/hw/ppc/mac_newworld.c @@ -197,11 +197,6 @@ static void ppc_core99_init(MachineState *machine) } if (machine->kernel_filename) { - int bswap_needed = 0; - -#ifdef BSWAP_NEEDED - bswap_needed = 1; -#endif kernel_base = KERNEL_LOAD_ADDR; kernel_size = load_elf(machine->kernel_filename, NULL, translate_kernel_address, NULL, NULL, NULL, @@ -209,7 +204,7 @@ static void ppc_core99_init(MachineState *machine) if (kernel_size < 0) { kernel_size = load_aout(machine->kernel_filename, kernel_base, machine->ram_size - kernel_base, - bswap_needed, TARGET_PAGE_SIZE); + true, TARGET_PAGE_SIZE); } if (kernel_size < 0) { kernel_size = load_image_targphys(machine->kernel_filename, diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c index 0dbcea035c..b5814690f5 100644 --- a/hw/ppc/mac_oldworld.c +++ b/hw/ppc/mac_oldworld.c @@ -153,11 +153,6 @@ static void ppc_heathrow_init(MachineState *machine) } if (machine->kernel_filename) { - int bswap_needed = 0; - -#ifdef BSWAP_NEEDED - bswap_needed = 1; -#endif kernel_base = KERNEL_LOAD_ADDR; kernel_size = load_elf(machine->kernel_filename, NULL, translate_kernel_address, NULL, NULL, NULL, @@ -165,7 +160,7 @@ static void ppc_heathrow_init(MachineState *machine) if (kernel_size < 0) { kernel_size = load_aout(machine->kernel_filename, kernel_base, machine->ram_size - kernel_base, - bswap_needed, TARGET_PAGE_SIZE); + true, TARGET_PAGE_SIZE); } if (kernel_size < 0) { kernel_size = load_image_targphys(machine->kernel_filename, diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c index a48d3622c5..5aaafb40da 100644 --- a/hw/sparc/sun4m.c +++ b/hw/sparc/sun4m.c @@ -233,20 +233,13 @@ static unsigned long sun4m_load_kernel(const char *kernel_filename, kernel_size = 0; if (linux_boot) { - int bswap_needed; - -#ifdef BSWAP_NEEDED - bswap_needed = 1; -#else - bswap_needed = 0; -#endif kernel_size = load_elf(kernel_filename, NULL, translate_kernel_address, NULL, NULL, NULL, NULL, NULL, ELFDATA2MSB, EM_SPARC, 0, 0); if (kernel_size < 0) kernel_size = load_aout(kernel_filename, KERNEL_LOAD_ADDR, - RAM_size - KERNEL_LOAD_ADDR, bswap_needed, + RAM_size - KERNEL_LOAD_ADDR, true, TARGET_PAGE_SIZE); if (kernel_size < 0) kernel_size = load_image_targphys(kernel_filename, diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c index 8ab5cf0461..d3cb7270ff 100644 --- a/hw/sparc64/sun4u.c +++ b/hw/sparc64/sun4u.c @@ -168,13 +168,6 @@ static uint64_t sun4u_load_kernel(const char *kernel_filename, kernel_size = 0; if (linux_boot) { - int bswap_needed; - -#ifdef BSWAP_NEEDED - bswap_needed = 1; -#else - bswap_needed = 0; -#endif kernel_size = load_elf(kernel_filename, NULL, NULL, NULL, kernel_entry, kernel_addr, &kernel_top, NULL, ELFDATA2MSB, EM_SPARCV9, 0, 0); @@ -182,7 +175,7 @@ static uint64_t sun4u_load_kernel(const char *kernel_filename, *kernel_addr = KERNEL_LOAD_ADDR; *kernel_entry = KERNEL_LOAD_ADDR; kernel_size = load_aout(kernel_filename, KERNEL_LOAD_ADDR, - RAM_size - KERNEL_LOAD_ADDR, bswap_needed, + RAM_size - KERNEL_LOAD_ADDR, true, TARGET_PAGE_SIZE); } if (kernel_size < 0) { diff --git a/include/hw/loader.h b/include/hw/loader.h index 784a93d6c1..d280dc33e9 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -190,7 +190,7 @@ ssize_t load_elf(const char *filename, void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp); ssize_t load_aout(const char *filename, hwaddr addr, int max_sz, - int bswap_needed, hwaddr target_page_size); + bool big_endian, hwaddr target_page_size); #define LOAD_UIMAGE_LOADADDR_INVALID (-1) From e16354b7f2e70d5ece97854d00141e3df4625cf6 Mon Sep 17 00:00:00 2001 From: Pierrick Bouvier Date: Thu, 20 Mar 2025 15:29:33 -0700 Subject: [PATCH 08/24] exec/cpu-all: remove BSWAP_NEEDED This identifier is poisoned, so it can't be used from common code anyway. We replace all occurrences with its definition directly. Signed-off-by: Pierrick Bouvier Link: https://lore.kernel.org/r/20250320223002.2915728-2-pierrick.bouvier@linaro.org Signed-off-by: Paolo Bonzini --- bsd-user/elfload.c | 6 +++--- include/exec/cpu-all.h | 12 ------------ include/exec/poison.h | 1 - linux-user/elfload.c | 8 ++++---- linux-user/syscall_defs.h | 2 +- 5 files changed, 8 insertions(+), 21 deletions(-) diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c index 833fa3bd05..3bca0cc9ed 100644 --- a/bsd-user/elfload.c +++ b/bsd-user/elfload.c @@ -44,7 +44,7 @@ static inline void memcpy_fromfs(void *to, const void *from, unsigned long n) memcpy(to, from, n); } -#ifdef BSWAP_NEEDED +#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN static void bswap_ehdr(struct elfhdr *ehdr) { bswap16s(&ehdr->e_type); /* Object file type */ @@ -111,7 +111,7 @@ static void bswap_note(struct elf_note *en) bswap32s(&en->n_type); } -#else /* ! BSWAP_NEEDED */ +#else static void bswap_ehdr(struct elfhdr *ehdr) { } static void bswap_phdr(struct elf_phdr *phdr, int phnum) { } @@ -119,7 +119,7 @@ static void bswap_shdr(struct elf_shdr *shdr, int shnum) { } static void bswap_sym(struct elf_sym *sym) { } static void bswap_note(struct elf_note *en) { } -#endif /* ! BSWAP_NEEDED */ +#endif /* HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN */ #include "elfcore.c" diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 8cd6c00cf8..47b14446b8 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -26,18 +26,6 @@ #include "exec/tswap.h" #include "hw/core/cpu.h" -/* some important defines: - * - * HOST_BIG_ENDIAN : whether the host cpu is big endian and - * otherwise little endian. - * - * TARGET_BIG_ENDIAN : same for the target cpu - */ - -#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN -#define BSWAP_NEEDED -#endif - /* Target-endianness CPU memory access functions. These fit into the * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h. */ diff --git a/include/exec/poison.h b/include/exec/poison.h index 8ed04b3108..2c151fd1e0 100644 --- a/include/exec/poison.h +++ b/include/exec/poison.h @@ -37,7 +37,6 @@ #pragma GCC poison TARGET_NAME #pragma GCC poison TARGET_SUPPORTS_MTTCG #pragma GCC poison TARGET_BIG_ENDIAN -#pragma GCC poison BSWAP_NEEDED #pragma GCC poison TARGET_LONG_BITS #pragma GCC poison TARGET_FMT_lx diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 8799e4ea27..fa83d78667 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -2121,7 +2121,7 @@ static inline void memcpy_fromfs(void * to, const void * from, unsigned long n) memcpy(to, from, n); } -#ifdef BSWAP_NEEDED +#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN static void bswap_ehdr(struct elfhdr *ehdr) { bswap16s(&ehdr->e_type); /* Object file type */ @@ -3143,7 +3143,7 @@ static bool parse_elf_properties(const ImageSource *src, * The contents of a valid PT_GNU_PROPERTY is a sequence of uint32_t. * Swap most of them now, beyond the header and namesz. */ -#ifdef BSWAP_NEEDED +#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN for (int i = 4; i < n / 4; i++) { bswap32s(note.data + i); } @@ -3999,7 +3999,7 @@ struct target_elf_prpsinfo { char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */ }; -#ifdef BSWAP_NEEDED +#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN static void bswap_prstatus(struct target_elf_prstatus *prstatus) { prstatus->pr_info.si_signo = tswap32(prstatus->pr_info.si_signo); @@ -4038,7 +4038,7 @@ static void bswap_note(struct elf_note *en) static inline void bswap_prstatus(struct target_elf_prstatus *p) { } static inline void bswap_psinfo(struct target_elf_prpsinfo *p) {} static inline void bswap_note(struct elf_note *en) { } -#endif /* BSWAP_NEEDED */ +#endif /* HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN */ /* * Calculate file (dump) size of given memory region. diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h index 86d773add7..5d22759992 100644 --- a/linux-user/syscall_defs.h +++ b/linux-user/syscall_defs.h @@ -462,7 +462,7 @@ typedef struct { abi_ulong sig[TARGET_NSIG_WORDS]; } target_sigset_t; -#ifdef BSWAP_NEEDED +#if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN static inline void tswap_sigset(target_sigset_t *d, const target_sigset_t *s) { int i; From ea8a7ceba3aafe4de9e7306df663698809e8381a Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:05 +0800 Subject: [PATCH 09/24] rust/vmstate: Remove unnecessary unsafe Remove the `unsafe` block of vmsd, because vmsd (passed to vmstate_struct) is defined in Rust side now, and it doesn't need `unsafe`. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-2-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index f0510ae769..6698dfe7ae 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -447,7 +447,7 @@ macro_rules! vmstate_struct { }, size: ::core::mem::size_of::<$type>(), flags: $crate::bindings::VMStateFlags::VMS_STRUCT, - vmsd: unsafe { $vmsd }, + vmsd: $vmsd, ..$crate::zeroable::Zeroable::ZERO $( .with_varray_flag($crate::call_func_with_field!( $crate::vmstate::vmstate_varray_flag, From 6ca5c3bedf3265358cf9033577dd30ed865d1cb3 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:06 +0800 Subject: [PATCH 10/24] rust/vmstate: Fix num_offset in vmstate macros `num_offset` is a member of `VMStateField`, and there's no need to use "." to access this field in a `VMStateField` instance. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-3-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 6698dfe7ae..9533b1250f 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -208,7 +208,7 @@ macro_rules! vmstate_of { .as_bytes() .as_ptr() as *const ::std::os::raw::c_char, offset: $crate::offset_of!($struct_name, $field_name), - $(.num_offset: $crate::offset_of!($struct_name, $num),)? + $(num_offset: $crate::offset_of!($struct_name, $num),)? // The calls to `call_func_with_field!` are the magic that // computes most of the VMStateField from the type of the field. info: $crate::info_enum_to_ref!($crate::call_func_with_field!( @@ -440,7 +440,7 @@ macro_rules! vmstate_struct { name: ::core::concat!(::core::stringify!($field_name), "\0") .as_bytes() .as_ptr() as *const ::std::os::raw::c_char, - $(.num_offset: $crate::offset_of!($struct_name, $num),)? + $(num_offset: $crate::offset_of!($struct_name, $num),)? offset: { $crate::assert_field_type!($struct_name, $field_name, $type); $crate::offset_of!($struct_name, $field_name) From c3d80af5ecf4f00db4a8d3ac5eca6edc0c9f061e Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:07 +0800 Subject: [PATCH 11/24] rust/vmstate: Fix num field when varray flags are set Array type vmstate has the VMStateField with `num` equals its length. When the varray vmstate is built based a array type, the `num` field should be cleaned to 0, because varray uses `num_offset` instead of `num` to store elements number information. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-4-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 9533b1250f..e3233303f2 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -275,6 +275,7 @@ impl VMStateField { assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0); self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_ARRAY.0); self.flags = VMStateFlags(self.flags.0 | flag.0); + self.num = 0; // varray uses num_offset instead of num. self } From 20797069c71a90582078448b81de28f227a8403b Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:08 +0800 Subject: [PATCH 12/24] rust/vmstate: Fix size field of VMStateField with VMS_ARRAY_OF_POINTER flag The `size` field of the VMStateField with VMS_ARRAY_OF_POINTER flag should stores the size of pointer, which depends on platform. Currently, `*const`, `*mut`, `NonNull`, `Box<>` and their wrapper are supported, and they have the same size as `usize`. Store the size (of `usize`) when VMS_ARRAY_OF_POINTER flag is set. The size may be changed when more smart pointers are supported, but now the size of "usize" is enough. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-5-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index e3233303f2..e2a1f7a97a 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -256,6 +256,10 @@ impl VMStateField { if (self.flags.0 & VMStateFlags::VMS_POINTER.0) != 0 { self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_POINTER.0); self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0); + // VMS_ARRAY_OF_POINTER flag stores the size of pointer. + // FIXME: *const, *mut, NonNull and Box<> have the same size as usize. + // Resize if more smart pointers are supported. + self.size = std::mem::size_of::(); } self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_SINGLE.0); self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_ARRAY.0); From 618258256e6c60957100c5a01ab70f5473020cb9 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:09 +0800 Subject: [PATCH 13/24] rust/vmstate: Fix type check for varray in vmstate_struct When pass a varray to vmstate_struct, the `type` parameter should be the type of the element in the varray, for example: vmstate_struct!(HPETState, timers, [0 .. num_timers], VMSTATE_HPET_TIMER, BqlRefCell).with_version_id(0) But this breaks current type check, because it checks the type of `field`, which is an array type (for the above example, type of timers is [BqlRefCell; 32], not BqlRefCell). But the current assert_field_type() can no longer be extended to include new arguments, so a variant of it (a second macro containing the `num = $num:ident` parameter) had to be added to handle array cases. In this new macro, it not only checks the type of element, but also checks whether the `num` (number of elements in varray) is out of range. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-6-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/assertions.rs | 15 +++++++++++++++ rust/qemu-api/src/vmstate.rs | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs index bba38cfda1..eb12e9499a 100644 --- a/rust/qemu-api/src/assertions.rs +++ b/rust/qemu-api/src/assertions.rs @@ -91,6 +91,21 @@ macro_rules! assert_field_type { } }; }; + + ($t:ty, $i:tt, $ti:ty, num = $num:ident) => { + const _: () = { + #[allow(unused)] + fn assert_field_type(v: $t) { + fn types_must_be_equal(_: T) + where + T: $crate::assertions::EqType, + { + } + let index: usize = v.$num.try_into().unwrap(); + types_must_be_equal::<_, &$ti>(&v.$i[index]); + } + }; + }; } /// Assert that an expression matches a pattern. This can also be diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index e2a1f7a97a..9d9cdda993 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -447,7 +447,7 @@ macro_rules! vmstate_struct { .as_ptr() as *const ::std::os::raw::c_char, $(num_offset: $crate::offset_of!($struct_name, $num),)? offset: { - $crate::assert_field_type!($struct_name, $field_name, $type); + $crate::assert_field_type!($struct_name, $field_name, $type $(, num = $num)?); $crate::offset_of!($struct_name, $field_name) }, size: ::core::mem::size_of::<$type>(), From 42c814b1395c39659270248d205deaaaa47a84f2 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:10 +0800 Subject: [PATCH 14/24] rust/vmstate: Fix "cannot infer type" error in vmstate_struct Rust cannot infer the type (it should be VMStateField) after Zeroable::ZERO, which cause the compiling error. To fix this error, call with_varray_flag() after VMStateField's initialization. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-7-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 9d9cdda993..3be3a7260f 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -453,13 +453,15 @@ macro_rules! vmstate_struct { size: ::core::mem::size_of::<$type>(), flags: $crate::bindings::VMStateFlags::VMS_STRUCT, vmsd: $vmsd, - ..$crate::zeroable::Zeroable::ZERO $( - .with_varray_flag($crate::call_func_with_field!( - $crate::vmstate::vmstate_varray_flag, - $struct_name, - $num)) - $(.with_varray_multiply($factor))?)? - } + ..$crate::zeroable::Zeroable::ZERO + } $(.with_varray_flag( + $crate::call_func_with_field!( + $crate::vmstate::vmstate_varray_flag, + $struct_name, + $num + ) + ) + $(.with_varray_multiply($factor))?)? }; } From e5655e92a8b984129aed12f24fc50d6e3f63429d Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:11 +0800 Subject: [PATCH 15/24] rust/vmstate: Fix unnecessary VMState bound of with_varray_flag() The VMState type bound is not used in with_varray_flag(). And for vmstate_struct, Rust cannot infer the type of `num` from the call_func_with_field(), so this causes the compiling error because it complains "cannot satisfy `_: VMState`" in with_varray_flag(). Note Rust can infer the type in vmstate_of macro so that with_varray_flag() can work at there. It is possible that the different initialization ways in the two macros cause differences in Rust's type inference. But in fact, the VMState type bound is not used in with_varray_flag() and vmstate_varray_flag() has already checked the VMState type, it's safe to drop VMState bound of with_varray_flag(), which can fix the above compiling error. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-8-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 3be3a7260f..792a74fdfc 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -275,7 +275,7 @@ impl VMStateField { } #[must_use] - pub const fn with_varray_flag(mut self, flag: VMStateFlags) -> VMStateField { + pub const fn with_varray_flag(mut self, flag: VMStateFlags) -> VMStateField { assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0); self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_ARRAY.0); self.flags = VMStateFlags(self.flags.0 | flag.0); From 5006e39cfacbf37e6925239059ae6945e36cf74e Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:12 +0800 Subject: [PATCH 16/24] rust/vmstate: Relax array check when build varray in vmstate_struct The varry of structure created by vmstate_struct is different with vmstate_of. This is because vmstate_struct uses the `vmsd` to traverse the vmstates of structure's fields, rather than treating the structure directly as a well-defined vmstate. Therefore, there's no need to check array flag when building varray by vmstate_struct. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-9-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 792a74fdfc..0b5af1c90b 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -275,14 +275,20 @@ impl VMStateField { } #[must_use] - pub const fn with_varray_flag(mut self, flag: VMStateFlags) -> VMStateField { - assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0); + pub const fn with_varray_flag_unchecked(mut self, flag: VMStateFlags) -> VMStateField { self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_ARRAY.0); self.flags = VMStateFlags(self.flags.0 | flag.0); self.num = 0; // varray uses num_offset instead of num. self } + #[must_use] + #[allow(unused_mut)] + pub const fn with_varray_flag(mut self, flag: VMStateFlags) -> VMStateField { + assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0); + self.with_varray_flag_unchecked(flag) + } + #[must_use] pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField { assert!(num <= 0x7FFF_FFFFu32); @@ -454,7 +460,7 @@ macro_rules! vmstate_struct { flags: $crate::bindings::VMStateFlags::VMS_STRUCT, vmsd: $vmsd, ..$crate::zeroable::Zeroable::ZERO - } $(.with_varray_flag( + } $(.with_varray_flag_unchecked( $crate::call_func_with_field!( $crate::vmstate::vmstate_varray_flag, $struct_name, From 3baf82e0a17bc037c9c564958a8b90814119d738 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:13 +0800 Subject: [PATCH 17/24] rust/vmstate: Re-implement VMState trait for timer binding At present, Rust side has a timer binding "timer::Timer", so the vmstate for timer should base on that binding instead of the raw "binding::QEMUTimer". It's possible to apply impl_vmstate_transparent for cell::Opaque and then impl_vmstate_forward for timer::Timer. But binding::QEMUTimer shouldn't be used directly, so that vmstate for such raw timer type is useless. Thus, apply impl_vmstate_scalar for timer::Timer. And since Opaque<> is useful, apply impl_vmstate_transparent for cell::Opaque as well. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-10-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 0b5af1c90b..01f06ed737 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -27,12 +27,7 @@ use core::{marker::PhantomData, mem, ptr::NonNull}; pub use crate::bindings::{VMStateDescription, VMStateField}; -use crate::{ - bindings::{self, VMStateFlags}, - prelude::*, - qom::Owned, - zeroable::Zeroable, -}; +use crate::{bindings::VMStateFlags, prelude::*, qom::Owned, zeroable::Zeroable}; /// This macro is used to call a function with a generic argument bound /// to the type of a field. The function must take a @@ -344,6 +339,7 @@ impl_vmstate_transparent!(std::cell::UnsafeCell where T: VMState); impl_vmstate_transparent!(std::pin::Pin where T: VMState); impl_vmstate_transparent!(crate::cell::BqlCell where T: VMState); impl_vmstate_transparent!(crate::cell::BqlRefCell where T: VMState); +impl_vmstate_transparent!(crate::cell::Opaque where T: VMState); #[macro_export] macro_rules! impl_vmstate_bitsized { @@ -390,7 +386,7 @@ impl_vmstate_scalar!(vmstate_info_uint8, u8, VMS_VARRAY_UINT8); impl_vmstate_scalar!(vmstate_info_uint16, u16, VMS_VARRAY_UINT16); impl_vmstate_scalar!(vmstate_info_uint32, u32, VMS_VARRAY_UINT32); impl_vmstate_scalar!(vmstate_info_uint64, u64); -impl_vmstate_scalar!(vmstate_info_timer, bindings::QEMUTimer); +impl_vmstate_scalar!(vmstate_info_timer, crate::timer::Timer); // Pointer types using the underlying type's VMState plus VMS_POINTER // Note that references are not supported, though references to cells From b13100372180fdb052aa6bbce663eea0c59e5db4 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:14 +0800 Subject: [PATCH 18/24] rust/vmstate: Support vmstate_validate In C version, VMSTATE_VALIDATE accepts the function pointer, which is used to check if some conditions of structure could meet, although the C version macro doesn't accept any structure as the opaque type. But it's hard to integrate VMSTATE_VALIDAE into vmstate_struct, a new macro has to be introduced to specifically handle the case corresponding to VMSTATE_VALIDATE. One of the difficulties is inferring the type of a callback by its name `test_fn`. We can't directly use `test_fn` as a parameter of test_cb_builder__() to get its type "F", because in this way, Rust compiler will be too conservative on drop check and complain "the destructor for this type cannot be evaluated in constant functions". Fortunately, PhantomData could help in this case, because it is considered to never have a destructor, no matter its field type [*]. The `phantom__()` in the `call_func_with_field` macro provides a good example of using PhantomData to infer type. So copy this idea and apply it to the `vmstate_validate` macro. [*]: https://doc.rust-lang.org/std/ops/trait.Drop.html#drop-check Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-11-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 52 +++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 01f06ed737..9740931fb1 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -25,9 +25,12 @@ //! functionality that is missing from `vmstate_of!`. use core::{marker::PhantomData, mem, ptr::NonNull}; +use std::os::raw::{c_int, c_void}; pub use crate::bindings::{VMStateDescription, VMStateField}; -use crate::{bindings::VMStateFlags, prelude::*, qom::Owned, zeroable::Zeroable}; +use crate::{ + bindings::VMStateFlags, callbacks::FnCall, prelude::*, qom::Owned, zeroable::Zeroable, +}; /// This macro is used to call a function with a generic argument bound /// to the type of a field. The function must take a @@ -508,6 +511,53 @@ macro_rules! vmstate_fields { }} } +pub extern "C" fn rust_vms_test_field_exists FnCall<(&'a T, u8), bool>>( + opaque: *mut c_void, + version_id: c_int, +) -> bool { + let owner: &T = unsafe { &*(opaque.cast::()) }; + let version: u8 = version_id.try_into().unwrap(); + // SAFETY: the opaque was passed as a reference to `T`. + F::call((owner, version)) +} + +pub type VMSFieldExistCb = unsafe extern "C" fn( + opaque: *mut std::os::raw::c_void, + version_id: std::os::raw::c_int, +) -> bool; + +#[doc(alias = "VMSTATE_VALIDATE")] +#[macro_export] +macro_rules! vmstate_validate { + ($struct_name:ty, $test_name:expr, $test_fn:expr $(,)?) => { + $crate::bindings::VMStateField { + name: ::std::ffi::CStr::as_ptr($test_name), + field_exists: { + const fn test_cb_builder__< + T, + F: for<'a> $crate::callbacks::FnCall<(&'a T, u8), bool>, + >( + _phantom: ::core::marker::PhantomData, + ) -> $crate::vmstate::VMSFieldExistCb { + let _: () = F::ASSERT_IS_SOME; + $crate::vmstate::rust_vms_test_field_exists:: + } + + const fn phantom__(_: &T) -> ::core::marker::PhantomData { + ::core::marker::PhantomData + } + Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn))) + }, + flags: $crate::bindings::VMStateFlags( + $crate::bindings::VMStateFlags::VMS_MUST_EXIST.0 + | $crate::bindings::VMStateFlags::VMS_ARRAY.0, + ), + num: 0, // 0 elements: no data, only run test_fn callback + ..$crate::zeroable::Zeroable::ZERO + } + }; +} + /// A transparent wrapper type for the `subsections` field of /// [`VMStateDescription`]. /// From 1998502196ad81fde58a48aac2256731bf6d0022 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:15 +0800 Subject: [PATCH 19/24] rust/vmstate: Add unit test for vmstate_of macro The vmstate has too many combinations of VMStateFlags and VMStateField. Currently, the best way to test is to ensure that the Rust vmstate definition is consistent with the (possibly corresponding) C version. Add a unit test to cover some patterns accepted by vmstate_of macro, which correspond to the following C version macros: * VMSTATE_U16 * VMSTATE_UNUSED * VMSTATE_VARRAY_UINT16_UNSAFE * VMSTATE_VARRAY_MULTIPLY Note: Because vmstate_info_* are defined in vmstate-types.c, it's necessary to link libmigration to rust unit tests. In the future, maybe it's possible to spilt libmigration from rust_qemu_api_objs. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-12-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/meson.build | 5 +- rust/qemu-api/tests/tests.rs | 2 + rust/qemu-api/tests/vmstate_tests.rs | 134 +++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 rust/qemu-api/tests/vmstate_tests.rs diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index a3f226ccc2..858685ddd4 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -58,7 +58,8 @@ rust_qemu_api_objs = static_library( libchardev.extract_all_objects(recursive: false), libcrypto.extract_all_objects(recursive: false), libauthz.extract_all_objects(recursive: false), - libio.extract_all_objects(recursive: false)]) + libio.extract_all_objects(recursive: false), + libmigration.extract_all_objects(recursive: false)]) rust_qemu_api_deps = declare_dependency( dependencies: [ qom_ss.dependencies(), @@ -71,7 +72,7 @@ rust_qemu_api_deps = declare_dependency( test('rust-qemu-api-integration', executable( 'rust-qemu-api-integration', - 'tests/tests.rs', + files('tests/tests.rs', 'tests/vmstate_tests.rs'), override_options: ['rust_std=2021', 'build.rust_std=2021'], rust_args: ['--test'], install: false, diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 269122e7ec..99a7aab6fe 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -17,6 +17,8 @@ use qemu_api::{ zeroable::Zeroable, }; +mod vmstate_tests; + // Test that macros can compile. pub static VMSTATE: VMStateDescription = VMStateDescription { name: c_str!("name").as_ptr(), diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs new file mode 100644 index 0000000000..eb1bb2f8a0 --- /dev/null +++ b/rust/qemu-api/tests/vmstate_tests.rs @@ -0,0 +1,134 @@ +// Copyright (C) 2025 Intel Corporation. +// Author(s): Zhao Liu +// SPDX-License-Identifier: GPL-2.0-or-later + +use std::{ffi::CStr, mem::size_of, slice}; + +use qemu_api::{ + bindings::{vmstate_info_int8, vmstate_info_uint8, vmstate_info_unused_buffer, VMStateFlags}, + c_str, + vmstate::{VMStateDescription, VMStateField}, + vmstate_fields, vmstate_of, vmstate_unused, + zeroable::Zeroable, +}; + +const FOO_ARRAY_MAX: usize = 3; + +// =========================== Test VMSTATE_FOOA =========================== +// Test the use cases of the vmstate macro, corresponding to the following C +// macro variants: +// * VMSTATE_FOOA: +// - VMSTATE_U16 +// - VMSTATE_UNUSED +// - VMSTATE_VARRAY_UINT16_UNSAFE +// - VMSTATE_VARRAY_MULTIPLY +#[repr(C)] +#[derive(qemu_api_macros::offsets)] +struct FooA { + arr: [u8; FOO_ARRAY_MAX], + num: u16, + arr_mul: [i8; FOO_ARRAY_MAX], + num_mul: u32, + elem: i8, +} + +static VMSTATE_FOOA: VMStateDescription = VMStateDescription { + name: c_str!("foo_a").as_ptr(), + version_id: 1, + minimum_version_id: 1, + fields: vmstate_fields! { + vmstate_of!(FooA, elem), + vmstate_unused!(size_of::()), + vmstate_of!(FooA, arr[0 .. num]).with_version_id(0), + vmstate_of!(FooA, arr_mul[0 .. num_mul * 16]), + }, + ..Zeroable::ZERO +}; + +#[test] +fn test_vmstate_uint16() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) }; + + // 1st VMStateField ("elem") in VMSTATE_FOOA (corresponding to VMSTATE_UINT16) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[0].name) }.to_bytes_with_nul(), + b"elem\0" + ); + assert_eq!(foo_fields[0].offset, 16); + assert_eq!(foo_fields[0].num_offset, 0); + assert_eq!(foo_fields[0].info, unsafe { &vmstate_info_int8 }); + assert_eq!(foo_fields[0].version_id, 0); + assert_eq!(foo_fields[0].size, 1); + assert_eq!(foo_fields[0].num, 0); + assert_eq!(foo_fields[0].flags, VMStateFlags::VMS_SINGLE); + assert!(foo_fields[0].vmsd.is_null()); + assert!(foo_fields[0].field_exists.is_none()); +} + +#[test] +fn test_vmstate_unused() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) }; + + // 2nd VMStateField ("unused") in VMSTATE_FOOA (corresponding to VMSTATE_UNUSED) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[1].name) }.to_bytes_with_nul(), + b"unused\0" + ); + assert_eq!(foo_fields[1].offset, 0); + assert_eq!(foo_fields[1].num_offset, 0); + assert_eq!(foo_fields[1].info, unsafe { &vmstate_info_unused_buffer }); + assert_eq!(foo_fields[1].version_id, 0); + assert_eq!(foo_fields[1].size, 8); + assert_eq!(foo_fields[1].num, 0); + assert_eq!(foo_fields[1].flags, VMStateFlags::VMS_BUFFER); + assert!(foo_fields[1].vmsd.is_null()); + assert!(foo_fields[1].field_exists.is_none()); +} + +#[test] +fn test_vmstate_varray_uint16_unsafe() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) }; + + // 3rd VMStateField ("arr") in VMSTATE_FOOA (corresponding to + // VMSTATE_VARRAY_UINT16_UNSAFE) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[2].name) }.to_bytes_with_nul(), + b"arr\0" + ); + assert_eq!(foo_fields[2].offset, 0); + assert_eq!(foo_fields[2].num_offset, 4); + assert_eq!(foo_fields[2].info, unsafe { &vmstate_info_uint8 }); + assert_eq!(foo_fields[2].version_id, 0); + assert_eq!(foo_fields[2].size, 1); + assert_eq!(foo_fields[2].num, 0); + assert_eq!(foo_fields[2].flags, VMStateFlags::VMS_VARRAY_UINT16); + assert!(foo_fields[2].vmsd.is_null()); + assert!(foo_fields[2].field_exists.is_none()); +} + +#[test] +fn test_vmstate_varray_multiply() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) }; + + // 4th VMStateField ("arr_mul") in VMSTATE_FOOA (corresponding to + // VMSTATE_VARRAY_MULTIPLY) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[3].name) }.to_bytes_with_nul(), + b"arr_mul\0" + ); + assert_eq!(foo_fields[3].offset, 6); + assert_eq!(foo_fields[3].num_offset, 12); + assert_eq!(foo_fields[3].info, unsafe { &vmstate_info_int8 }); + assert_eq!(foo_fields[3].version_id, 0); + assert_eq!(foo_fields[3].size, 1); + assert_eq!(foo_fields[3].num, 16); + assert_eq!( + foo_fields[3].flags.0, + VMStateFlags::VMS_VARRAY_UINT32.0 | VMStateFlags::VMS_MULTIPLY_ELEMENTS.0 + ); + assert!(foo_fields[3].vmsd.is_null()); + assert!(foo_fields[3].field_exists.is_none()); + + // The last VMStateField in VMSTATE_FOOA. + assert_eq!(foo_fields[4].flags, VMStateFlags::VMS_END); +} From 57c327f3a044ebd6da9efda07e4d264996688110 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:16 +0800 Subject: [PATCH 20/24] rust/vmstate: Add unit test for vmstate_{of|struct} macro Add a unit test to cover some patterns accepted by vmstate_of and vmstate_struct macros, which correspond to the following C version macros: * VMSTATE_BOOL_V * VMSTATE_U64 * VMSTATE_STRUCT_VARRAY_UINT8 * (no C version) MULTIPLY variant of VMSTATE_STRUCT_VARRAY_UINT32 * VMSTATE_ARRAY Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-13-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/tests/vmstate_tests.rs | 158 ++++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 2 deletions(-) diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs index eb1bb2f8a0..2c3301e02a 100644 --- a/rust/qemu-api/tests/vmstate_tests.rs +++ b/rust/qemu-api/tests/vmstate_tests.rs @@ -5,10 +5,14 @@ use std::{ffi::CStr, mem::size_of, slice}; use qemu_api::{ - bindings::{vmstate_info_int8, vmstate_info_uint8, vmstate_info_unused_buffer, VMStateFlags}, + bindings::{ + vmstate_info_bool, vmstate_info_int64, vmstate_info_int8, vmstate_info_uint64, + vmstate_info_uint8, vmstate_info_unused_buffer, VMStateFlags, + }, c_str, + cell::BqlCell, vmstate::{VMStateDescription, VMStateField}, - vmstate_fields, vmstate_of, vmstate_unused, + vmstate_fields, vmstate_of, vmstate_struct, vmstate_unused, zeroable::Zeroable, }; @@ -132,3 +136,153 @@ fn test_vmstate_varray_multiply() { // The last VMStateField in VMSTATE_FOOA. assert_eq!(foo_fields[4].flags, VMStateFlags::VMS_END); } + +// =========================== Test VMSTATE_FOOB =========================== +// Test the use cases of the vmstate macro, corresponding to the following C +// macro variants: +// * VMSTATE_FOOB: +// - VMSTATE_BOOL_V +// - VMSTATE_U64 +// - VMSTATE_STRUCT_VARRAY_UINT8 +// - (no C version) MULTIPLY variant of VMSTATE_STRUCT_VARRAY_UINT32 +// - VMSTATE_ARRAY +#[repr(C)] +#[derive(qemu_api_macros::offsets)] +struct FooB { + arr_a: [FooA; FOO_ARRAY_MAX], + num_a: u8, + arr_a_mul: [FooA; FOO_ARRAY_MAX], + num_a_mul: u32, + wrap: BqlCell, + val: bool, + // FIXME: Use Timer array. Now we can't since it's hard to link savevm.c to test. + arr_i64: [i64; FOO_ARRAY_MAX], +} + +static VMSTATE_FOOB: VMStateDescription = VMStateDescription { + name: c_str!("foo_b").as_ptr(), + version_id: 2, + minimum_version_id: 1, + fields: vmstate_fields! { + vmstate_of!(FooB, val).with_version_id(2), + vmstate_of!(FooB, wrap), + vmstate_struct!(FooB, arr_a[0 .. num_a], &VMSTATE_FOOA, FooA).with_version_id(1), + vmstate_struct!(FooB, arr_a_mul[0 .. num_a_mul * 32], &VMSTATE_FOOA, FooA).with_version_id(2), + vmstate_of!(FooB, arr_i64), + }, + ..Zeroable::ZERO +}; + +#[test] +fn test_vmstate_bool_v() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) }; + + // 1st VMStateField ("val") in VMSTATE_FOOB (corresponding to VMSTATE_BOOL_V) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[0].name) }.to_bytes_with_nul(), + b"val\0" + ); + assert_eq!(foo_fields[0].offset, 136); + assert_eq!(foo_fields[0].num_offset, 0); + assert_eq!(foo_fields[0].info, unsafe { &vmstate_info_bool }); + assert_eq!(foo_fields[0].version_id, 2); + assert_eq!(foo_fields[0].size, 1); + assert_eq!(foo_fields[0].num, 0); + assert_eq!(foo_fields[0].flags, VMStateFlags::VMS_SINGLE); + assert!(foo_fields[0].vmsd.is_null()); + assert!(foo_fields[0].field_exists.is_none()); +} + +#[test] +fn test_vmstate_uint64() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) }; + + // 2nd VMStateField ("wrap") in VMSTATE_FOOB (corresponding to VMSTATE_U64) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[1].name) }.to_bytes_with_nul(), + b"wrap\0" + ); + assert_eq!(foo_fields[1].offset, 128); + assert_eq!(foo_fields[1].num_offset, 0); + assert_eq!(foo_fields[1].info, unsafe { &vmstate_info_uint64 }); + assert_eq!(foo_fields[1].version_id, 0); + assert_eq!(foo_fields[1].size, 8); + assert_eq!(foo_fields[1].num, 0); + assert_eq!(foo_fields[1].flags, VMStateFlags::VMS_SINGLE); + assert!(foo_fields[1].vmsd.is_null()); + assert!(foo_fields[1].field_exists.is_none()); +} + +#[test] +fn test_vmstate_struct_varray_uint8() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) }; + + // 3rd VMStateField ("arr_a") in VMSTATE_FOOB (corresponding to + // VMSTATE_STRUCT_VARRAY_UINT8) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[2].name) }.to_bytes_with_nul(), + b"arr_a\0" + ); + assert_eq!(foo_fields[2].offset, 0); + assert_eq!(foo_fields[2].num_offset, 60); + assert!(foo_fields[2].info.is_null()); // VMSTATE_STRUCT_VARRAY_UINT8 doesn't set info field. + assert_eq!(foo_fields[2].version_id, 1); + assert_eq!(foo_fields[2].size, 20); + assert_eq!(foo_fields[2].num, 0); + assert_eq!( + foo_fields[2].flags.0, + VMStateFlags::VMS_STRUCT.0 | VMStateFlags::VMS_VARRAY_UINT8.0 + ); + assert_eq!(foo_fields[2].vmsd, &VMSTATE_FOOA); + assert!(foo_fields[2].field_exists.is_none()); +} + +#[test] +fn test_vmstate_struct_varray_uint32_multiply() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) }; + + // 4th VMStateField ("arr_a_mul") in VMSTATE_FOOB (corresponding to + // (no C version) MULTIPLY variant of VMSTATE_STRUCT_VARRAY_UINT32) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[3].name) }.to_bytes_with_nul(), + b"arr_a_mul\0" + ); + assert_eq!(foo_fields[3].offset, 64); + assert_eq!(foo_fields[3].num_offset, 124); + assert!(foo_fields[3].info.is_null()); // VMSTATE_STRUCT_VARRAY_UINT8 doesn't set info field. + assert_eq!(foo_fields[3].version_id, 2); + assert_eq!(foo_fields[3].size, 20); + assert_eq!(foo_fields[3].num, 32); + assert_eq!( + foo_fields[3].flags.0, + VMStateFlags::VMS_STRUCT.0 + | VMStateFlags::VMS_VARRAY_UINT32.0 + | VMStateFlags::VMS_MULTIPLY_ELEMENTS.0 + ); + assert_eq!(foo_fields[3].vmsd, &VMSTATE_FOOA); + assert!(foo_fields[3].field_exists.is_none()); +} + +#[test] +fn test_vmstate_macro_array() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) }; + + // 5th VMStateField ("arr_i64") in VMSTATE_FOOB (corresponding to + // VMSTATE_ARRAY) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[4].name) }.to_bytes_with_nul(), + b"arr_i64\0" + ); + assert_eq!(foo_fields[4].offset, 144); + assert_eq!(foo_fields[4].num_offset, 0); + assert_eq!(foo_fields[4].info, unsafe { &vmstate_info_int64 }); + assert_eq!(foo_fields[4].version_id, 0); + assert_eq!(foo_fields[4].size, 8); + assert_eq!(foo_fields[4].num, FOO_ARRAY_MAX as i32); + assert_eq!(foo_fields[4].flags, VMStateFlags::VMS_ARRAY); + assert!(foo_fields[4].vmsd.is_null()); + assert!(foo_fields[4].field_exists.is_none()); + + // The last VMStateField in VMSTATE_FOOB. + assert_eq!(foo_fields[5].flags, VMStateFlags::VMS_END); +} From 8df1b0012aba2501bb1654cb4fbdf1b52ce22222 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:17 +0800 Subject: [PATCH 21/24] rust/vmstate: Add unit test for pointer case Add a unit test to cover some patterns accepted by vmstate_of macro, which correspond to the following C version macros: * VMSTATE_POINTER * VMSTATE_ARRAY_OF_POINTER Note: Currently, vmstate_struct can't handle the pointer to structure case. Leave this case as a FIXME and use vmstate_unused as a place holder. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-14-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/tests/vmstate_tests.rs | 119 ++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 4 deletions(-) diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs index 2c3301e02a..027b226675 100644 --- a/rust/qemu-api/tests/vmstate_tests.rs +++ b/rust/qemu-api/tests/vmstate_tests.rs @@ -2,15 +2,16 @@ // Author(s): Zhao Liu // SPDX-License-Identifier: GPL-2.0-or-later -use std::{ffi::CStr, mem::size_of, slice}; +use std::{ffi::CStr, mem::size_of, ptr::NonNull, slice}; use qemu_api::{ bindings::{ - vmstate_info_bool, vmstate_info_int64, vmstate_info_int8, vmstate_info_uint64, - vmstate_info_uint8, vmstate_info_unused_buffer, VMStateFlags, + vmstate_info_bool, vmstate_info_int32, vmstate_info_int64, vmstate_info_int8, + vmstate_info_uint64, vmstate_info_uint8, vmstate_info_unused_buffer, VMStateFlags, }, c_str, - cell::BqlCell, + cell::{BqlCell, Opaque}, + impl_vmstate_forward, vmstate::{VMStateDescription, VMStateField}, vmstate_fields, vmstate_of, vmstate_struct, vmstate_unused, zeroable::Zeroable, @@ -286,3 +287,113 @@ fn test_vmstate_macro_array() { // The last VMStateField in VMSTATE_FOOB. assert_eq!(foo_fields[5].flags, VMStateFlags::VMS_END); } + +// =========================== Test VMSTATE_FOOC =========================== +// Test the use cases of the vmstate macro, corresponding to the following C +// macro variants: +// * VMSTATE_FOOC: +// - VMSTATE_POINTER +// - VMSTATE_ARRAY_OF_POINTER +struct FooCWrapper([Opaque<*mut u8>; FOO_ARRAY_MAX]); // Though Opaque<> array is almost impossible. + +impl_vmstate_forward!(FooCWrapper); + +#[repr(C)] +#[derive(qemu_api_macros::offsets)] +struct FooC { + ptr: *const i32, + ptr_a: NonNull, + arr_ptr: [Box; FOO_ARRAY_MAX], + arr_ptr_wrap: FooCWrapper, +} + +static VMSTATE_FOOC: VMStateDescription = VMStateDescription { + name: c_str!("foo_c").as_ptr(), + version_id: 3, + minimum_version_id: 1, + fields: vmstate_fields! { + vmstate_of!(FooC, ptr).with_version_id(2), + // FIXME: Currently vmstate_struct doesn't support the pointer to structure. + // VMSTATE_STRUCT_POINTER: vmstate_struct!(FooC, ptr_a, VMSTATE_FOOA, NonNull) + vmstate_unused!(size_of::>()), + vmstate_of!(FooC, arr_ptr), + vmstate_of!(FooC, arr_ptr_wrap), + }, + ..Zeroable::ZERO +}; + +const PTR_SIZE: usize = size_of::<*mut ()>(); + +#[test] +fn test_vmstate_pointer() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.fields, 6) }; + + // 1st VMStateField ("ptr") in VMSTATE_FOOC (corresponding to VMSTATE_POINTER) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[0].name) }.to_bytes_with_nul(), + b"ptr\0" + ); + assert_eq!(foo_fields[0].offset, 0); + assert_eq!(foo_fields[0].num_offset, 0); + assert_eq!(foo_fields[0].info, unsafe { &vmstate_info_int32 }); + assert_eq!(foo_fields[0].version_id, 2); + assert_eq!(foo_fields[0].size, 4); + assert_eq!(foo_fields[0].num, 0); + assert_eq!( + foo_fields[0].flags.0, + VMStateFlags::VMS_SINGLE.0 | VMStateFlags::VMS_POINTER.0 + ); + assert!(foo_fields[0].vmsd.is_null()); + assert!(foo_fields[0].field_exists.is_none()); +} + +#[test] +fn test_vmstate_macro_array_of_pointer() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.fields, 6) }; + + // 3rd VMStateField ("arr_ptr") in VMSTATE_FOOC (corresponding to + // VMSTATE_ARRAY_OF_POINTER) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[2].name) }.to_bytes_with_nul(), + b"arr_ptr\0" + ); + assert_eq!(foo_fields[2].offset, 2 * PTR_SIZE); + assert_eq!(foo_fields[2].num_offset, 0); + assert_eq!(foo_fields[2].info, unsafe { &vmstate_info_uint8 }); + assert_eq!(foo_fields[2].version_id, 0); + assert_eq!(foo_fields[2].size, PTR_SIZE); + assert_eq!(foo_fields[2].num, FOO_ARRAY_MAX as i32); + assert_eq!( + foo_fields[2].flags.0, + VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0 + ); + assert!(foo_fields[2].vmsd.is_null()); + assert!(foo_fields[2].field_exists.is_none()); +} + +#[test] +fn test_vmstate_macro_array_of_pointer_wrapped() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.fields, 6) }; + + // 4th VMStateField ("arr_ptr_wrap") in VMSTATE_FOOC (corresponding to + // VMSTATE_ARRAY_OF_POINTER) + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[3].name) }.to_bytes_with_nul(), + b"arr_ptr_wrap\0" + ); + assert_eq!(foo_fields[3].offset, (FOO_ARRAY_MAX + 2) * PTR_SIZE); + assert_eq!(foo_fields[3].num_offset, 0); + assert_eq!(foo_fields[2].info, unsafe { &vmstate_info_uint8 }); + assert_eq!(foo_fields[3].version_id, 0); + assert_eq!(foo_fields[3].size, PTR_SIZE); + assert_eq!(foo_fields[3].num, FOO_ARRAY_MAX as i32); + assert_eq!( + foo_fields[2].flags.0, + VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0 + ); + assert!(foo_fields[3].vmsd.is_null()); + assert!(foo_fields[3].field_exists.is_none()); + + // The last VMStateField in VMSTATE_FOOC. + assert_eq!(foo_fields[4].flags, VMStateFlags::VMS_END); +} From 9bd7e6f7f2f0f97178fe6884b39f40e686567f52 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:18 +0800 Subject: [PATCH 22/24] rust/vmstate: Add unit test for vmstate_validate Add a unit test for vmstate_validate, which corresponds to the C version macro: VMSTATE_VALIDATE. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-15-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/tests/vmstate_tests.rs | 82 +++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs index 027b226675..b8d8b45b19 100644 --- a/rust/qemu-api/tests/vmstate_tests.rs +++ b/rust/qemu-api/tests/vmstate_tests.rs @@ -2,7 +2,7 @@ // Author(s): Zhao Liu // SPDX-License-Identifier: GPL-2.0-or-later -use std::{ffi::CStr, mem::size_of, ptr::NonNull, slice}; +use std::{ffi::CStr, mem::size_of, os::raw::c_void, ptr::NonNull, slice}; use qemu_api::{ bindings::{ @@ -13,7 +13,7 @@ use qemu_api::{ cell::{BqlCell, Opaque}, impl_vmstate_forward, vmstate::{VMStateDescription, VMStateField}, - vmstate_fields, vmstate_of, vmstate_struct, vmstate_unused, + vmstate_fields, vmstate_of, vmstate_struct, vmstate_unused, vmstate_validate, zeroable::Zeroable, }; @@ -397,3 +397,81 @@ fn test_vmstate_macro_array_of_pointer_wrapped() { // The last VMStateField in VMSTATE_FOOC. assert_eq!(foo_fields[4].flags, VMStateFlags::VMS_END); } + +// =========================== Test VMSTATE_FOOD =========================== +// Test the use cases of the vmstate macro, corresponding to the following C +// macro variants: +// * VMSTATE_FOOD: +// - VMSTATE_VALIDATE + +// Add more member fields when vmstate_of/vmstate_struct support "test" +// parameter. +struct FooD; + +impl FooD { + fn validate_food_0(&self, _version_id: u8) -> bool { + true + } + + fn validate_food_1(_state: &FooD, _version_id: u8) -> bool { + false + } +} + +fn validate_food_2(_state: &FooD, _version_id: u8) -> bool { + true +} + +static VMSTATE_FOOD: VMStateDescription = VMStateDescription { + name: c_str!("foo_d").as_ptr(), + version_id: 3, + minimum_version_id: 1, + fields: vmstate_fields! { + vmstate_validate!(FooD, c_str!("foo_d_0"), FooD::validate_food_0), + vmstate_validate!(FooD, c_str!("foo_d_1"), FooD::validate_food_1), + vmstate_validate!(FooD, c_str!("foo_d_2"), validate_food_2), + }, + ..Zeroable::ZERO +}; + +#[test] +fn test_vmstate_validate() { + let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOD.fields, 4) }; + let mut foo_d = FooD; + let foo_d_p = std::ptr::addr_of_mut!(foo_d).cast::(); + + // 1st VMStateField in VMSTATE_FOOD + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[0].name) }.to_bytes_with_nul(), + b"foo_d_0\0" + ); + assert_eq!(foo_fields[0].offset, 0); + assert_eq!(foo_fields[0].num_offset, 0); + assert!(foo_fields[0].info.is_null()); + assert_eq!(foo_fields[0].version_id, 0); + assert_eq!(foo_fields[0].size, 0); + assert_eq!(foo_fields[0].num, 0); + assert_eq!( + foo_fields[0].flags.0, + VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_MUST_EXIST.0 + ); + assert!(foo_fields[0].vmsd.is_null()); + assert!(unsafe { foo_fields[0].field_exists.unwrap()(foo_d_p, 0) }); + + // 2nd VMStateField in VMSTATE_FOOD + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[1].name) }.to_bytes_with_nul(), + b"foo_d_1\0" + ); + assert!(!unsafe { foo_fields[1].field_exists.unwrap()(foo_d_p, 1) }); + + // 3rd VMStateField in VMSTATE_FOOD + assert_eq!( + unsafe { CStr::from_ptr(foo_fields[2].name) }.to_bytes_with_nul(), + b"foo_d_2\0" + ); + assert!(unsafe { foo_fields[2].field_exists.unwrap()(foo_d_p, 2) }); + + // The last VMStateField in VMSTATE_FOOD. + assert_eq!(foo_fields[3].flags, VMStateFlags::VMS_END); +} From f7b87e464c8e9d30661fb9f519ed14fb053cd415 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 18 Mar 2025 21:02:19 +0800 Subject: [PATCH 23/24] rust/vmstate: Include complete crate path of VMStateFlags in vmstate_clock The use of "bindings::*" masks incomplete path of VMStateFlags. Include complete crate path of VMStateFlags in vmstate_clock, and clean up "bindings::*" in device_class.rs of pl011. Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20250318130219.1799170-16-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device_class.rs | 8 ++++++-- rust/qemu-api/src/vmstate.rs | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs index 0b2076ddaa..b4d4a7eb43 100644 --- a/rust/hw/char/pl011/src/device_class.rs +++ b/rust/hw/char/pl011/src/device_class.rs @@ -8,8 +8,12 @@ use std::{ }; use qemu_api::{ - bindings::*, c_str, prelude::*, vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct, - vmstate_subsections, vmstate_unused, zeroable::Zeroable, + bindings::{qdev_prop_bool, qdev_prop_chr}, + c_str, + prelude::*, + vmstate::VMStateDescription, + vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_unused, + zeroable::Zeroable, }; use crate::device::{PL011Registers, PL011State}; diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 9740931fb1..1b2b12eadd 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -487,7 +487,10 @@ macro_rules! vmstate_clock { $crate::offset_of!($struct_name, $field_name) }, size: ::core::mem::size_of::<*const $crate::qdev::Clock>(), - flags: VMStateFlags(VMStateFlags::VMS_STRUCT.0 | VMStateFlags::VMS_POINTER.0), + flags: $crate::bindings::VMStateFlags( + $crate::bindings::VMStateFlags::VMS_STRUCT.0 + | $crate::bindings::VMStateFlags::VMS_POINTER.0, + ), vmsd: unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_clock) }, ..$crate::zeroable::Zeroable::ZERO } From 64acc23c9793e86f2811345f3c122bf3ece8088b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 21 Mar 2025 14:17:52 +0100 Subject: [PATCH 24/24] rust: hpet: fix decoding of timer registers Due to a missing "& 0x18", timer registers are not decoded correctly. This breaks the tests/functional/test_x86_64_tuxrun.py functional test. Fixes: 519088b7cf6 ("rust: hpet: decode HPET registers into enums", 2025-03-06) Reported-by: Peter Maydell Tested-by: Peter Maydell Reviewed-by: Peter Maydell Signed-off-by: Paolo Bonzini --- rust/hw/timer/hpet/src/hpet.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs index 63c1971f0b..3ae3ec25f1 100644 --- a/rust/hw/timer/hpet/src/hpet.rs +++ b/rust/hw/timer/hpet/src/hpet.rs @@ -776,7 +776,7 @@ impl HPETState { let timer_id: usize = ((addr - 0x100) / 0x20) as usize; if timer_id <= self.num_timers.get() { // TODO: Add trace point - trace_hpet_ram_[read|write]_timer_id(timer_id) - TimerRegister::try_from(addr) + TimerRegister::try_from(addr & 0x18) .map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg)) } else { // TODO: Add trace point - trace_hpet_timer_id_out_of_range(timer_id)