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

[Philippe Mathieu-Daudé]

On 6/2/25 09:46, Paolo Bonzini wrote:
> On Thu, Feb 6, 2025 at 9:40 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > 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?
>
> Yes (for example build() could even fail to compile if you don't have
> impl_sizes/valid_sizes set), but why do you want that? I'm not even
> sure that all cases of .valid.max_access_size=4 are correct...

Exactly for that :) Not have implicit default values, so correct
values are reviewed when models are added.