search

lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.56k stars 762 forks source link

[rom/rom_ext] Audit and remove access to AON peripherals in deep sleep resume path #22614

Open jettr opened 6 months ago

jettr commented 6 months ago

Description

In #22204 we would an issue with the ROM trying to reset the watchdog count down on the deep sleep resume path; this is causing an exception because the owner firmware had already locked the watchdog registers by clearing REGWEN for watchdog.

We need to audit and fix the ROM/ROM_EXT code for the deep sleep resume path for all places where it tries to write to registers that may have already been locked. @a-will has already provided a [first pass analysis] for ROM (https://github.com/lowRISC/opentitan/issues/22204#issuecomment-2060028889):

  • [x] adc_ctrl: No accesses
  • [ ] aon_timer
    • Writes to various values that could be locked.
  • [ ] ast: Not generally writable by app firmware, but should take care in ROM and/or ROM_EXT

  • [ ] clkmgr

    • Writes to JITTER_ENABLE, which could be locked by app firmware

  • [ ] pinmux

    • Writes to mux configs for UART and strapping, but could be locked
  • [x] pwm: No accesses

  • [ ] pwrmgr

    • RESET_EN written, but could be locked

  • [ ] rstmgr

    • ROM has drivers that could write ALERT_INFO_CTRL and CPU_INFO_CTRL
    • However, the functions do not appear to be used...

  • [x] sensor_ctrl

    • No apparent writes. Only have reads.

  • [ ] sram_ctrl_ret

    • Writes to CTRL, which could be locked
  • [x] sysrst_ctrl: No accesses

We still need to audit ROM_EXT and then fix all potential issues.

matutem commented 5 months ago

IMO low power exit reset is quite different from all other resets:

This means upon reset we will need to read the reset_info CSR, and regarding a comment above, for non-lowpower resets that are not intended (like software reset) we would also need to read the alert and cpu_info registers to determine what happened. The reset_info value can be used to take the low-power vs other resets path.

It seems to me there is no need to write to the CSRs mentioned above in low power exit.

a-will commented 5 months ago

Writes to the retention SRAM's CTRL CSR also shouldn't happen after low-power exit--Either bit would wipe data that owner firmware intended to keep.

a-will commented 5 months ago

For pinmux, I feel like ROM should be allowed to try to connect the ROM UART TX but not fail if the CSR is locked. Straps, on the other hand, should probably be left alone.

jettr commented 5 months ago

For pinmux, I feel like ROM should be allowed to try to connect the ROM UART TX but not fail if the CSR is locked

Seems reasonable to me either way. But yes to not failing if locked :)

Straps, on the other hand, should probably be left alone.

+1

cfrantz commented 4 months ago

I've added a target in #23381 to capture the ROM disassembly. The disassembly (with source) almost makes it easy enough to find all relevant register accesses, but not quite. Because of function inlining, sometimes the source emitted into the assembly is incomplete and lacks the named register. Target addresses are not always show in sw instructions, so its also not possible to create a list of register addresses in the ROM instruction stream.

We can, however, use the disassembly to aggregate the entire list of sources that contribute code to the ROM. We can then grep those sources for register names of AON peripherals.

aggregate_srcs.sh will find source filenames in the disassembly and create a single output which is the aggregation of all source files that contributed at least one line of code to the ROM:

#!/bin/bash

DISASSEMBLY=$1
SRCS=("sw/device/silicon_creator/rom/rom_start.S")
SRCS+=($(grep "/proc/self/cwd" ${DISASSEMBLY} | cut -f1 -d: | sed -e "s,/proc/self/cwd/,,g" -e "s,\./,,g"  | sort | uniq))

grep -n . ${SRCS[@]}

Then, we can find all lines of code in the aggregated source that reference one of the AON peripherals:

./aggregate_srcs.sh bazel-out/k8-fastbuild-ST-2cc462681f62/bin/sw/device/silicon_creator/rom/mask_rom_sim_dv.dis \
    | grep _REG_OFFSET \
    | egrep "AON_TIMER|AST|CLKMGR|PINMUX|PWRMGR|RSTMGR|SENSOR_CTRL|SRAM_CTRL|SYSRST_CTRL"

This produces 50 lines of code that reference an AON peripheral. Not all of these lines of code necessarily contribute to the ROM, but its a constrained set of code to audit. At 1459afe, I see the following:

cfrantz commented 4 months ago

Audit Log

My conclusions after manually reviewing all of the items above.

Clearly reads

Const calculation:

Main SRAM is not an AON peripheral:

Writable registers that don't have a REGWEN

Writes that are not of concern to the ROM

Unsure

REGWENs to be aware of

CLKMGR_JITTER_REGWEN

PINMUX_MIO_PAD_ATTR_REGWEN_n

PWRMGR_RESET_EN_REGWEN

RET-RAM SRAM_CTRL_REGWEN

RSTMGR_ALERT_REGWEN

AON_TIMER_WDOG_REGWEN

cfrantz commented 4 months ago

I believe the ROM has the low-power wakeup issues already addressed:

a-will commented 4 months ago

I believe the ROM has the low-power wakeup issues already addressed:

  • PINMUX: we skip re-initializing the pinmux from a low-power wakeup.
  • WDOG: we skip re-initializing the watchdog from a low-power wakeup.
  • RETRAM_SRAM_CTRL: we only initialize this from power-on-reset (and additional OTP defined conditions).
  • CLKMGR_JITTER: we ought to lock this in the ROM or ROM_EXT to prevent it from being changed from the SKU-defined value.
  • PWRMGR_RESET_EN: we enable all reset sources in the ROM. I'm not sure that we care if the owner changes it later.
  • RSTMGR_ALERT_INFO: we enable this in the ROM to allow capturing alert causes in the rstmgr crashdump registers, but I'm not sure we care if the owner changes it later.

Awesome, LGTM. As long as there isn't a sec_mmio_write32() (or similar checking that the value was actually written) for the CSRs we don't care about, then that'll do. And if I'm just comparing the two comments, then it looks like we're good!

The remaining bit would be documenting the interfaces between boot stages (including any requirements for mutable CSRs), but that's technically not part of this issue. :smile: