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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.