Skip to content

RFC: Even higher-level cache maintenance operations #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
adamgreig opened this issue Jun 15, 2017 · 3 comments · Fixed by #192
Closed

RFC: Even higher-level cache maintenance operations #47

adamgreig opened this issue Jun 15, 2017 · 3 comments · Fixed by #192

Comments

@adamgreig
Copy link
Member

#40 landed CMSIS-style DCache cleaning and invalidating by address and size, both usize. This maps well to the actual hardware operations but is inconvenient to use.

We could add some convenience methods that operate on normal Rust objects:

  • clean_dcache_by_slice<T>(slice: &[T]) which passes slice.as_ptr() as usize and core::mem::size_of<T>() * slice.len() to the underlying methods,
  • clean_dcache_by_ref<T>(obj: &T), which passes T as *const _ as usize and core::mem::size_of<T>() to the underlying methods.
  • As above for invalidate_dcache and clean_invalidate_dcache

Big safety issue: all the cache methods operate on cache lines of 32 bytes. If you pass something whose size is not a multiple of 32, then some extra bytes after that object will be cleaned or invalidated. In general this is definitely unsafe. Perhaps these methods could panic on non-32-byte-multiple sizes?

@jonas-schievink
Copy link
Contributor

If you pass something whose size is not a multiple of 32, then some extra bytes after that object will be cleaned or invalidated. In general this is definitely unsafe.

Why is this unsafe?

@adamgreig
Copy link
Member Author

The basic problem is that invalidating an object you didn't expect to can lead to writes "going missing" if they hadn't yet been cleaned to main memory, which we can abuse to read uninitialised memory and/or produce invalid values, both of which are explicitly UB.

Really I think this means the existing D-cache invalidate operations, as well as any new invalidate by ref/slice operations, should probably be unsafe. Possibly also I-cache and maybe also clean operations. I'm not sure if you can technically trigger UB with only clean, though.

It's been a few years so I made a quick example. First we clean the cache, writing whatever is leftover in our stack through to main memory. Next we stack allocate a small buffer followed by a bool, but the writes to initialise them haven't been cleaned to main memory, they're only in D-cache. Then we invalidate the buffer, causing the bool to also be invalidated, so on next read it contains an invalid value for a bool (32 in this case), which was leftover in the stack.

#![no_std]
#![no_main]

extern crate panic_halt;
extern crate stm32f7;
use cortex_m_rt::entry;
use cortex_m_semihosting::dbg;

// Proposed new invalidate function
fn invalidate_dcache_by_ref<T>(scb: &mut cortex_m::peripheral::SCB, obj: &T) {
    scb.invalidate_dcache_by_address(obj as *const _ as usize, core::mem::size_of::<T>());
}

// Demonstrate undefined behaviour
fn trigger_ub(scb: &mut cortex_m::peripheral::SCB) {
    // Create a buffer and a bool flag in the stack.
    // Zeroing these variables has gone through to the D-cache but not
    // been cleaned to the main memory.
    let buf = [0u8; 16];
    let flag: bool = false;

    // Printing out their addresses prevents optimiser removing them
    dbg!(&buf as *const _ as usize);
    dbg!(&flag as *const _ as usize);

    // Invalidate the cache line containing `buf`.
    // This _also_ invalidates `flag`, since buf only uses half the cache line.
    // Both buf and flag will be fetched from main memory on next access.
    invalidate_dcache_by_ref(scb, &buf);

    // Cast underlying flag value to a u8 and print its value.
    // It "must" be 0 or 1...
    let flag_value = unsafe { *(&flag as *const bool as *const u8) };
    dbg!(flag_value);
}

#[entry]
fn main() -> ! {

    // Enable D-cache
    let peripherals = cortex_m::peripheral::Peripherals::take().unwrap();
    let mut scb = peripherals.SCB;
    let mut cpuid = peripherals.CPUID;
    scb.enable_dcache(&mut cpuid);

    // Clean current stack through to main memory
    scb.clean_dcache(&mut cpuid);
    cortex_m::asm::dmb();

    // Trigger UB
    trigger_ub(&mut scb);

    loop {
        cortex_m::asm::nop();
    }
}
[src/main.rs:23] &buf as *const _ as usize = 537067432
[src/main.rs:24] &flag as *const _ as usize = 537067451
[src/main.rs:34] flag_value = 32

🤐

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jan 13, 2020

Wow, thanks for that demonstration! I thought the invalidation would ensure to also flush modified cache lines back to RAM, but if it doesn't it makes sense that things might break.

bors bot added a commit that referenced this issue Feb 4, 2020
192: Update and improve cache operations. r=jonas-schievink a=adamgreig

Closes #47, #188.

I've implemented the proposed methods from #47 and marked all d-cache invalidation functions as unsafe. It's not unsafe to invalidate i-cache or branch predictor as they are read-only caches. The clean and clean+invalidate operations do not alter memory from the executing core's point of view so are also safe.

It wasn't possible to remove the requirement to pass in `&mut CPUID` as you require synchronized access to `CPUID` to read the number of sets and ways in the cache, which is required to fully clean or invalidate them, which is required to enable or disable them. So it goes.

Breaking change due to changing safety of d-cache invalidation functions.

Co-authored-by: Adam Greig <[email protected]>
bors bot added a commit that referenced this issue Feb 5, 2020
192: Update and improve cache operations. r=jonas-schievink a=adamgreig

Closes #47, #188.

I've implemented the proposed methods from #47 and marked all d-cache invalidation functions as unsafe. It's not unsafe to invalidate i-cache or branch predictor as they are read-only caches. The clean and clean+invalidate operations do not alter memory from the executing core's point of view so are also safe.

It wasn't possible to remove the requirement to pass in `&mut CPUID` as you require synchronized access to `CPUID` to read the number of sets and ways in the cache, which is required to fully clean or invalidate them, which is required to enable or disable them. So it goes.

Breaking change due to changing safety of d-cache invalidation functions.

Co-authored-by: Adam Greig <[email protected]>
@bors bors bot closed this as completed in 4b4f825 Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants