On Tue, Jul 8, 2025 at 11:35 AM Paolo Bonzini <pbonz...@redhat.com> wrote: > > > We have the ability to make memory accesses use a typesafe access width > > type in Rust, which the C API currently lacks as it does not use a > > newtype wrapper for specifying the amount of bytes a memory access has; > > it uses a plain 32-bit integer value instead. > > I find this both verbose and (ok, that's subjective) ugly due to the > extra import, the underscore.
Yep, I agree on that, but I wasn't sure what name would be better. Whatever type-level improvement this patch brings, it's small, though nice-to-have. We can drop it, no problem. > > There are two parts on the patches: > > 1) the extra checking on impl_sizes and valid_sizes. That's valuable, > what about just adding something like this: > > assert!(min == 1 || min == 2 || min == 4 || min == 8); > assert!(max == 1 || max == 2 || max == 4 || max == 8); > assert!(max >= min); > > It can be validated at compile time anyway, since the functions are > pretty much always used in const context (in fact, for C code there's a > scripts/checkpatch.pl check that they are declared as const). Yes sounds good! > > > 2) Passing Bits to the read and write callbacks. The argument is > ignored for pl011, and converted with "as u32" for HPET. I find this to > be worse than before, because it's very unobvious that _32 is defined to > 4 rather than 32. Maybe do a `Size` enum instead that has variants: 1 instead of 8, 2 instead of 16, 4 instead of 32, etc.? > > The main effect on generated code is to add an assert! to > memory_region_ops_read_cb() and memory_region_ops_write_cb() that's > similar to the above. I'm not sure of its value, either: if the size is > not 1/2/4/8, memory.c/physmem.c must have screwed up big. It's not a > safety concern, either, since the size is not used in any unsafe code. Yep it's more of a guard rail since we can't have refined integer types. -- Manos Pitsidianakis Emulation and Virtualization Engineer at Linaro Ltd
