RegisterBlocks are not #[repr(C)]

[mciantyre]

Caveat: I've still not run anything on hardware 😛

I'm studying the RAL API, and I noticed that none of the peripheral RegisterBlocks are #[repr(C)]. Here's a snippet of the CCM peripheral:

// Expected #[repr(C)] here
pub struct RegisterBlock {
    /// CCM Control Register
    pub CCR: RWRegister<u32>,

    _reserved1: [u32; 1],

    /// CCM Status Register
    pub CSR: RORegister<u32>,

    /// CCM Clock Switcher Register
    pub CCSR: RWRegister<u32>,

    /// CCM Arm Clock Root Register
    pub CACRR: RWRegister<u32>,

    // Rest elided...
}

The block registers are written in order, according to the SVD and the processor reference manual. Its address is 0x400F_C000.

Given the RWRegister API, a hypothetical write to the CCSR register could resemble

ccm.CCSR.write(0xDEAD_BEEF)

The write will cast the the memory at 0x400F_C000 to a RegisterBlock, offset the pointer it by the number of registers between 0x400F_C000 and CCSR (expected to be the absolute address 0x400F_C00C), then store 0xDEAD_BEEF at that address.

Because the RegisterBlock is not #[repr(C)], we cannot guarantee it's layout, since Rust does not guarantee struct layout. Therefore, the store of 0xDEAD_BEEF might not necessarily happen on the actual CCSR register, address 0x400F_C00C; it could happen on another memory address between 0x400F_C000 and 0x400F_C000 + core::mem::size_of::<RegisterBlock>()

I think this is a bug, or could lead to a bug. It applies to all RegisterBlocks in imxrt-ral. It may also apply to the stm32ral crate, from which we've derived the RAL generation script. A study of the stm32ral crate also reveals no usage of #[repr(C)]. A fix could be to specify #[repr(C)] in the RAL generation script, then re-generate the RAL crate.

What do we think? Let me know if I'm missing something that makes this a non-issue!

[teburd]

@adamgreig might be interested in this one

That does seem like a potential issue to me as well. The docs do seem clear that field ordering and alignment are not guaranteed, though go into more details about some of things that repr(Rust) does. It seems like if all Register types are 4 bytes, we are just hitting that luckily it works scenario. I can say the clocking works fine on my evk board as I've tested that, granted that may not indicate if there are other issues.

That all said I think repr(C, align(4)) perhaps makes sense here to ensure field order and alignment rules are always obeyed the way we want. Just to make it clearer to ourselves and the compiler, no we really want fields in this memory order and of this alignment.

[adamgreig]

Thanks @mciantyre, you're quite right, they should be repr(C). I think in practice it would be unlikely for the compiler to reorder a struct of u32s but nevertheless it's not correct as-is. I've applied a fix to stm32ral here and I imagine something very similar would be fine in imxrt-rs.