Conversation
src/arch.rs
Outdated
| /// | ||
| /// * The register is readable at the current Exception Level. | ||
| /// * Reading the register does not destructively alter hardware state (e.g., | ||
| /// acknowledging an interrupt by reading `ICC_IAR1_EL1`). |
There was a problem hiding this comment.
How would acknowledging an interrupt cause undefined behaviour?
There was a problem hiding this comment.
Fair, altering hardware state sounded more serious, but after reading about this more, seems like this specifically shouldn't cause any memory issues/UBs. Also, since we control the list of registers we wrap with this helper anyway, I guess there's no harm in making this function safe.
src/arch.rs
Outdated
| sctlr &= !sctlr_el2::C; // Data Cache Enable | ||
| sctlr &= !sctlr_el2::I; // Instruction Cache Enable | ||
| // SAFETY: We assume we have an identity mapped pagetables for the currently running | ||
| // code, so disabling MMU is safe. |
There was a problem hiding this comment.
Even if so, it's not really safe. The compiler is free to emit atomic memory accesses in safe Rust code, but these have undefined behaviour when the data cache is disabled. So it's not really safe to run arbitrary Rust code with the MMU disabled.
I think the best solution would be to disable the MMU and caches in the same inline assembly block as you jump to the payload image.
There was a problem hiding this comment.
Fair - I've moved the cache disable code to a separate assembly function.
qwandor
left a comment
qwandor
left a comment
There was a problem hiding this comment.
Please make sure someone else looks over the cache maintenance side of things, I'm not super familiar with that.
src/arch.rs
Outdated
| sys_reg!(cnthctl_el2); | ||
| sys_reg!(spsr_el2); | ||
| sys_reg!(elr_el2); | ||
| sys_reg!(sp_el1); |
There was a problem hiding this comment.
Consider using the arm-sysregs crate which we've recently published.
There was a problem hiding this comment.
Done, although probably could be better. ccsidr_el1 seems to be missing support for Associativity and NumSets fields, for instance. I'm happy to fix this, but is there any specific process for Googlers to contribute to Trusted Firmware-A repositories, like there is for GitHub, for instance?
There was a problem hiding this comment.
You should be able to login to https://review.trustedfirmware.org/ with your GitHub account and then add your SSH key at https://review.trustedfirmware.org/settings/#SSHKeys to upload patches.
For the missing fields, this is probably because they are conditional and clash with other fields. In this case the size and location of the Associativity and NumSets fields depends on whether FEAT_CCIDX is implemented.
| let mut sctlr = read_sctlr_el2(); | ||
| sctlr.remove(SctlrEl2::M); // MMU Enable | ||
| sctlr.remove(SctlrEl2::C); // Data Cache Enable | ||
| sctlr.remove(SctlrEl2::I); // Instruction Cache Enable |
There was a problem hiding this comment.
Optional: you can use the -= operator if you like.
2 participants
No description provided.