Enhance MmioAddress safety and fix AArch64 instruction emission

[phip1611]

This PR improves the robustness of the MMIO backend and fixes a correctness issue on aarch64.

Safety hardening (MmioAddress): Replace the internal *mut u8 with NonNull<u8>, enforcing the non-null invariant at the type level rather than via debug_assert. A dedicated InvalidAddressError::Null variant makes null-pointer rejections explicit in the public API. There was no UB as none of this is public API, it just makes the internal API more robust.

aarch64 fix: read_volatile/write_volatile can cause LLVM to emit post-increment addressing instructions (e.g. str w9, [x0], #4) that do not populate ESR_EL2, preventing hypervisor MMIO emulation in protected VMs. The new mod arch replaces these with explicit ldrb/strb inline assembly on aarch64, falling back to read/write_volatile on all other targets. See rust-lang/rust#131894.

No user-visible behavior changes on x86. There was no UB in the previous code — these are robustness and correctness improvements.

This fixes #47.

[mkroening]

Looks good to me, generally. 👍

[mkroening]

Both RegisterAddress and MmioAddress are public, though, no?

[phip1611]

I rephrased the comment.

/// All implementations and instances of this trait are created within this
/// crate and do follow all safety invariants. API users don't get access to the
/// underlying register addresses, nor can they construct one themselves, as this
/// type et al. are sealed.

Better?

[mkroening]

I think I understand it a bit better now, yes.

Now that I have taken a closer look at it, though, I think the interface is still problematic as is:

let ptr_max = ptr::with_exposed_provenance_mut(usize::MAX);
let Err(InvalidAddressError::InvalidBaseAddress(addr)) =
    (unsafe { Uart16550::new_mmio(ptr_max, 1) })
else {
    return;
};

// UB
addr.add_offset(10);

Is there a reason for making RegisterAddress public instead of having MmioBackend::Address = NonNull<u8> and PioBackend::Address = u16?

Moreover, even if RegisterAccess were internal, add_offset would still be internally unsound if add_offset is not made unsafe or wrapping_add is used for MMIO addresses.

Lastly, why do new_mmio and new_port return MmioAddress and PortIoAddress on error instead of the provided argument or nothing at all since the arguments are Copy?

[mkroening]

Perhaps it might make sense to put NonNull<u8> into the API instead of *mut u8. That way, this requirement is more explicit for users. Users who already have a NonNull can also use that directly instead of removing and recreating the type invariant.

[phip1611]

Hm, I see the value add. However, if one has to construct a NonNull, construction is more cumbersome.

Instead of passing new_mmio(0x1000 as *mut _) one has to write new_mmio(NonNull::new(0x1000 as *mut _).unwrap()).

I'm not sure if this additional boilerplate code is a win for users 🤔 what do you think?

[mkroening]

I would personally use the following:

let ptr = ptr::with_exposed_provenance_mut::<u8>(0x1000);
let ptr = NonNull::new(ptr).unwrap();
let uart = unsafe { Uart16550::new_mmio(ptr, 1) };

I have also opened #52 about integer-to-pointer casts in general.

I can see why you would want to be able to do it in a single, unconvoluted line, but in my opinion, a lot is happening here, and being explicit helps readability and simplifies the API. I'd be fine with either one in the end, though.

[phip1611]

I didn't know the with_exposed_provenance_mut() helper - interesting