Re: [PATCH 10/10] rust: bindings for MemoryRegionOps

[Philippe Mathieu-Daudé]

Hi Paolo,

On 17/1/25 20:40, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs     |  43 +++---
>   rust/hw/char/pl011/src/lib.rs        |   1 -
>   rust/hw/char/pl011/src/memory_ops.rs |  36 -----
>   rust/qemu-api/meson.build            |   1 +
>   rust/qemu-api/src/lib.rs             |   1 +
>   rust/qemu-api/src/memory.rs          | 191 +++++++++++++++++++++++++++
>   rust/qemu-api/src/sysbus.rs          |   7 +-
>   rust/qemu-api/src/zeroable.rs        |  12 ++
>   8 files changed, 234 insertions(+), 58 deletions(-)
>   delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
>   create mode 100644 rust/qemu-api/src/memory.rs
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 259efacb046..294394c6e82 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 <manos.pitsidianakis@linaro.org>
>   // SPDX-License-Identifier: GPL-2.0-or-later
>   
> -use core::ptr::{addr_of_mut, NonNull};
> +use core::ptr::{addr_of, addr_of_mut, NonNull};
>   use std::{
>       ffi::CStr,
>       os::raw::{c_int, c_void},
> @@ -12,14 +12,14 @@
>       bindings::{self, *},
>       c_str, impl_vmstate_forward,
>       irq::InterruptSource,
> +    memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
>       prelude::*,
> -    qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
> +    qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
>       qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
>   };
>   
>   use crate::{
>       device_class,
> -    memory_ops::PL011_OPS,
>       registers::{self, Interrupt},
>       RegisterOffset,
>   };
> @@ -490,20 +490,24 @@ impl PL011State {
>       /// location/instance. All its fields are expected to hold unitialized
>       /// values with the sole exception of `parent_obj`.
>       unsafe fn init(&mut self) {
> +        static PL011_OPS: MemoryRegionOps<PL011State> = 
> MemoryRegionOpsBuilder::<PL011State>::new()
> +            .read(&PL011State::read)
> +            .write(&PL011State::write)
> +            .native_endian()

Could we always make .valid_sizes() explicit?

> +            .impl_sizes(4, 4)
> +            .build();
> +
>           // SAFETY:
>           //
>           // self and self.iomem are guaranteed to be valid at this point since 
> callers
>           // must make sure the `self` reference is valid.
> -        unsafe {
> -            memory_region_init_io(
> -                addr_of_mut!(self.iomem),
> -                addr_of_mut!(*self).cast::<Object>(),
> -                &PL011_OPS,
> -                addr_of_mut!(*self).cast::<c_void>(),
> -                Self::TYPE_NAME.as_ptr(),
> -                0x1000,
> -            );
> -        }
> +        MemoryRegion::init_io(
> +            unsafe { &mut *addr_of_mut!(self.iomem) },
> +            addr_of_mut!(*self),
> +            &PL011_OPS,
> +            "pl011",
> +            0x1000,
> +        );


> diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
> index 300c732ae1d..5622e974cbc 100644
> --- a/rust/hw/char/pl011/src/lib.rs
> +++ b/rust/hw/char/pl011/src/lib.rs
> @@ -29,7 +29,6 @@
>   
>   mod device;
>   mod device_class;
> -mod memory_ops;
>   
>   pub use device::pl011_create;
>   
> diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
> deleted file mode 100644
> index 95b4df794e4..00000000000
> --- a/rust/hw/char/pl011/src/memory_ops.rs
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -// Copyright 2024, Linaro Limited
> -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -
> -use core::ptr::NonNull;
> -use std::os::raw::{c_uint, c_void};
> -
> -use qemu_api::{bindings::*, zeroable::Zeroable};
> -
> -use crate::device::PL011State;
> -
> -pub static PL011_OPS: MemoryRegionOps = MemoryRegionOps {
> -    read: Some(pl011_read),
> -    write: Some(pl011_write),
> -    read_with_attrs: None,
> -    write_with_attrs: None,
> -    endianness: device_endian::DEVICE_NATIVE_ENDIAN,
> -    valid: Zeroable::ZERO,
> -    impl_: MemoryRegionOps__bindgen_ty_2 {
> -        min_access_size: 4,
> -        max_access_size: 4,
> -        ..Zeroable::ZERO
> -    },
> -};