Converting AtomicAccess to u64

[blitz]

I'm trying to implement the Bytes trait for the abstractions of our hypervisor and I'm really struggling. My current problem is that Bytes has a store method that takes the value to store as an AtomicAccess. Now our internal abstractions really want an u64 to store data. That didn't seem much of an issue until I tried converting it:

    fn store<T: AtomicAccess>(
        &self,
        val: T,
        addr: MemoryRegionAddress,
        order: Ordering,
    ) -> Result<(), Self::E> {
        let actual_value: <<T as AtomicAccess>::A as AtomicInteger>::V = val.into();

        // At this point actual_value has type V that is unconstrained and thus we can't do anything with it?
        let an_u64: u64 = actual_value as u64; // Error: an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

So the initially innocent looking problem of getting an actual integer I can use out of AtomicAccess appears really complex now. Am I missing something obvious? How do others solve this?

@bonzini

[blitz]

In C++ this would be solved by specializing template functions, but this is not possible in Rust:

struct U64Converter {
    value: u64,
}

macro_rules! as_plain_integer_or_fail {
    ($T:ty) => {
        impl From<$T> for U64Converter {
            fn from(val: $T) -> Self {
                Self { value: val as u64 }
            }
        }
    };
}

as_plain_integer_or_fail!(u8);
as_plain_integer_or_fail!(u16);
// ...

impl<T> From<T> for U64Converter { // Error: Conflicting implementations
    fn from(_val: T) -> Self {
        panic!("Trying to convert unsupported type to u64");
    }
}

[blitz]

So this solution gets somewhat closer:

unsafe fn any_int_to_u64<T>(val: T) -> u64 {
    use std::mem::{size_of_val, transmute};

    match size_of_val(&val) {
        1 => transmute::<T, u8>(val) as u64,
        2 => transmute::<T, u16>(val) as u64,
        4 => transmute::<T, u16>(val) as u64,
        8 => transmute::<T, u16>(val) as u64,
        unsupported => unimplemented!("Integer size {} cannot be converted to u64", unsupported),
    }
}

but fails due to:

31 |         1 => transmute::<T, u8>(val) as u64,
   |              ^^^^^^^^^^^^^^^^^^
   |
   = note: source type: `T` (this type does not have a fixed size)
   = note: target type: `u8` (8 bits)

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types

See also rust-lang/rust#61956.

[jiangliu]

What does this means?

Now our internal abstractions really want an u64 to store data.

When storing an AtomicU8 value to an address, the u8 value will be converted to u64 internally?

[blitz]

What does this means?

Now our internal abstractions really want an u64 to store data.

When storing an AtomicU8 value to an address, the u8 value will be converted to u64 internally?

When we've designed our internal memory bus we considered using generics, but abandoned the idea, because Rust has no good support for generic programming over integer types so far. So we ended up with a trait BusDevice (a virtual device or piece of RAM) that has (among other things) the following methods:

    fn read(&self, req: Request) -> u64;
    fn write(&self, req: Request, value: u64);

Request is just an address/size pair. This has worked pretty ok so far, because it's really easy to work with. No macros and no generics required. Reading a byte is:

let value = some_device.read(Request::new(addr, RequestSize::Size1))

Yes, this value is u64 and it means that we treat all "immediate" writes as u64 internally. There is some convenience in places to just be able to say read_byte where it makes sense. For writing it's the same:

let a_byte: u8 = 17;
some_device.write(Request::new(addr, RequestSize::Size1), a_byte.into());

But as for reading, there can just be helpers for write_byte on top, but they don't complicate the trait implementation.

We make the distinction between "immediate" accesses (1,2,4,8 byte writes that happen "atomically") and "bulk" accesses where you don't really care about whether data written as bytes or words. So there are additional methods for bulk memory access. This is loosely modelled around how PCI works. The rationale is if people manage to implement it in hardware, it is sufficient to implement device models. ;)

So while there is probably a very interesting discussion about the merits of each approach, it seems that at least the GuestMemory trait makes quite a lot of implicit assumptions about the types it can be implemented on. This results in very sad glue code.

I have a working (but still extremely sad and unsafe) workaround for now:

fn atomic_int_to_u64<T: AtomicAccess>(val: T) -> u64 {
    use std::mem::{size_of_val, transmute};

    let val: <<T as AtomicAccess>::A as AtomicInteger>::V = val.into();

    unsafe {
        // We cannot transmute unknown types directly due to
        // https://github.com/rust-lang/rust/issues/61956. The workaround is to transmute pointer
        // types and dereference them.
        match size_of_val(&val) {
            1 => *transmute::<*const _, *const u8>(&val) as u64,
            2 => *transmute::<*const _, *const u16>(&val) as u64,
            4 => *transmute::<*const _, *const u32>(&val) as u64,
            8 => *transmute::<*const _, *const u64>(&val) as u64,

            unsupported => {
                unimplemented!("Integer size {} cannot be converted to u64", unsupported)
            }
        }
    }
}

[blitz]

It seems the way from any concrete integer type to AtomicAccess / AtomicInteger is even worse, The above approach of transmuting pointer types cannot be used because AtomicAccess does not implement Copy and thus the pointer cannot be dereferenced.

[blitz]

Maybe a concrete problem. Consider this method from the Bytes trait:

    fn load<T: AtomicAccess>(
        &self,
        addr: MemoryRegionAddress,
        order: std::sync::atomic::Ordering,
    )

How would you implement a device that always returns 23?

The only way seems to somehow restrict the type of T (i.e. by introducing a where T::A::V: From<u8>), but that does not implement the trait anymore ("impl has extra requirement").

[gsserge]

Still unsafe, but might be prettier than using transmute: AtomicAccess has ByteValued as its supertrait. ByteValued has a method fn as_slice(&self) -> &[u8], which can be used to build up a value of u64 using u64::from_le_bytes().

Edit: I think the call to .as_slice() is actually safe, it's the impl ByteValued is unsafe.

[blitz]

Still unsafe, but might be prettier than using transmute: AtomicAccess has ByteValued as its supertrait. ByteValued has a method fn as_slice(&self) -> &[u8], which can be used to build up a value of u64 using u64::from_le_bytes().

Interesting! I wonder whether the compiler can then still remove all the cruft. The "nice" thing about the transmute solution is that the ugliness is completely optimized away. Will try that out.

Is there any example code of tests that implement MMIO devices or memory chunks that interact with these interfaces? It would be interesting to see how that is solved.

[andreeaflorescu]

Some examples are available in vm-virtio:

• https://github.com/rust-vmm/vm-virtio/blob/c5ec5126a2f2c55a6d03015ac39078f35944a7be/crates/devices/virtio-blk/src/request.rs#L207
• https://github.com/rust-vmm/vm-virtio/blob/main/crates/devices/virtio-blk/src/stdio_executor.rs#L366

We're not using Bytes directly afaik, but I haven't checked the code in a while so I might as well be wrong 😆

[blitz]

@andreeaflorescu Thanks for the pointers. Indeed trying to implement VolatileMemory and then implementing Bytes on top of it has yielded much better results. So remaining unsafes I think we can fix by improving our own abstractions.

Thanks for the rubber ducking here. 👍

[blitz]

Maybe one additional thought for the guest memory abstraction: Besides converting our RAM abstraction to &mut [u8] nothing else I wrote to implement GuestMemory has been really related to our system. Yet I wrote 240 lines of code so far to interface everything. We'll eventually share our code, but until then I'm also interested in any future redesigns of this code. I'm convinced there is a simpler abstraction hiding in here.