[fpga,bazel] Replace the updatemem bitstream splicing flow for FPGA with loading of memories via JTAG & stub out MMI generation#30543
Conversation
updatemem bitstream splicing flow for FPGA with loading of memories via JTAGupdatemem bitstream splicing flow for FPGA with loading of memories via JTAG & stub out MMI generation
f1e195b to
fd62d42
Compare
dcab4b9 to
b48becf
Compare
| let mut words = Vec::new(); | ||
|
|
||
| for word_idx in start..(start + count) { | ||
| self.dmi_write(regs::INDEX_REG_OFFSET, word_idx) |
There was a problem hiding this comment.
Is an auto-increment feature planned to avoid those writes?
There was a problem hiding this comment.
I don't know if there is an explicit plan for it but we should definitely capture it in an issue, as it should help speed things up a bit.
Note however that reading from the bkdr_loader is not on the hot path for FPGA tests (we only write & clear), and reads are always going to be slower as we cannot so easily optimize batched reads to DMI-exposed CSRs - so the performance in this case isn't as much of a concern.
| # Read in the extra arguments from the opentitan_test. | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--bitstream", type=str) | ||
| parser.add_argument("--rom", type=str) |
There was a problem hiding this comment.
I don't have the context but I am seeing a lot of duplication here in those pentests, surely some of it can refactored? Not your fault of course.
There was a problem hiding this comment.
Completely agree, yes this whole thing looks like something that needs refactoring.
(But definitely not a target for this PR 😅)
jwnrt
left a comment
There was a problem hiding this comment.
Reviewed in a meeting with @luismarques and @AlexJones0, LGTM
b48becf to
682d8fb
Compare
|
CHANGE AUTHORIZED: hw/ip/prim_xilinx/rtl/prim_rom.sv This removes the temporary deactivation of the ROM endpoint, which is as intended now that the tooling has been adapted. |
1 similar comment
|
CHANGE AUTHORIZED: hw/ip/prim_xilinx/rtl/prim_rom.sv This removes the temporary deactivation of the ROM endpoint, which is as intended now that the tooling has been adapted. |
682d8fb to
c7febb3
Compare
af44455 to
118aac3
Compare
The muxing of the jtag was previously dependent on the state. This can lead to glitches after implementation because the state is a multibit signal. Vivado uses one-hot encodings for FSM states. Hence, the bkdr_loader mux signal depends on three registers (despite being declared as a 2b signal). The state transition from PRELOAD to SWITCHING happens right when we write to the done signal and can lead to a glitch if the three state bits don't change at the same time. If it happens on the JTAG clock, it will upset the JTAG statemachine and lead to transmission errors. A simple fix is to register the mux signal. Ideally the mux-signal would be synchronized to the jtag clock domain. But this is tricky because we cannot rely on the JTAG clock being active. The already implemented delayed switching should be sufficient if we can guarantee that the jtag is inactive when the switch is performed. Signed-off-by: Michael Gautschi <mgautschi@lowrisc.org> Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This *should* be solved by the RTL fix to the backdoor loader that stops the JTAG glitch due to the mux that is dependent upon multiple registers - however this behaviour manifests depending on the state of the RTL and the build seed. So to be safe and derisk this transition a bit, we explicitly ignore errors on this write only, and loudly log a warning. If there is instead some error in another way such that the write is not completed, the subsequent FPGA test will fail and CI will fail much more noisily. The intention is to monitor the results for a few days/weeks across multiple bitstream builds, and revert this change when we have confidence that this issue is completely fixed. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
There could potentially be some side effects if OpenOCD is still "talking" to the downstream JTAG TAP at the moment we transition into mission mode and the JTAG is muxed back to the rv_dm and lc_ctrl. To eliminate problems here, explicitly drop self (and thus drop the JTAG connection, and hence shut down OpenOCD), *before* we wait for the transition to complete. We must still wait for the transition afterwards to prevent accidental use of the FPGA whilst it is still transitioning, which could cause unexpected errors. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Previously, we pruned anything that has a dependency on the spliced bitstream for the list of software targets to be built. This is because the CI runner does not have support for bitstream splicing, but also acts as a bit of a "catch-all" way of filtering out anything that needs a bitstream, and generally any OpenTitan tests which support FPGA environments - since those will be built in the relevant FPGA jobs anyhow. Since the spliced bitstream will be removed, we should be a bit more explicit about what we're trying to exclude here. For tests, it makes a lot more sense to use appropriate Bazel test tag filters and tags on the test. This will let us catch anything that depends on Verilator, a CW340 bitstream, or QEMU specifically. However, tests that are aliased (e.g. with `opentitan_alias_test`) will not inherit all their parent tags, and will appear as valid build & test targets (to prevent issues with duplicate test appearances when querying). To stop us building these aliased tests, we filter out any targets tagged with 'FPGA', and the set of targets which (transitively) depend on such targets. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
We're no longer filtering out all OpenTitanTest targets since we removed the filter on the bitstream splice in the CI "Software Build & Test" job - so now we need to be careful to explicitly exclude QEMU from the list of test tag filters. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This was a simple shell script that wrapped some Bazel commands for verifying on-target coverage measurement infrastructure. It has been broken in `master` for a while as the `gcp_spliced_test_rom` target does not actually exist, and has instead been replaced by a much more comprehensive coverage infrastructure integrated with our build system & tooling. Thus, remove this deprecated test script. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
These docs are outdated - the CW310 target has been removed, there is no more "hyper3X0" naming style (Bitstreams are always built for HyperDebug targets), the `--ast-cfg-version` option was removed, and the emulation SKU must be used instead of the SiVal SKU for testing (at least, as the default in the documentation). Also, the `runfiles/sc_hsm/*` pattern for unzipping not needed, as it should be available after unzipping via `runfiles/+hsm+sc_hsm/sc_hsm/`. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Since most tests are run in the SiVal ROM_EXT job, the additional overhead that will be added per test by the bkdr_loader flow (when using the CMSIS_DAP adapter) is about 8-10s per test, which causes this job to far exceed the allocated hour. Split this job into two to accommodate the change in runtime (at least, whilst we are using the CMSIS_DAP adapter). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
We're still getting the cached bitstream from the bucket, but we're not longer splicing it, so update the name to reflect this change. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Add rules to allow for retrieving the bitstream and VMEMs (e.g. ROM/OTP) of an execution environment, with the ability to override the file that is provided using `src`. This is intended as a partial replacement for the bitstream splicing flow, which is currently overloaded - it is being used for both bitstream splicing (as the name suggests), and as a hook for build options to be able to modify the OTP and ROM that is spliced in and used for test execution. Bitstreams will no longer be spliced when using the backdoor loader flow, which means the OTP and the ROM can be directly overwritten. However, we still want the option to have build options hook into and overload the ROM or OTP (e.g. on a Bazel transition) - so replace them with these two rules, which targets can use accordingly. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
When FPGA bitstream splicing is replaced by backdoor loading, the orchestrator will no longer work (as it directly takes as input a spliced bitstream, which it then passes to the test to load). Modify the orchestrator source to package the ROM and OTP files separately into the Orchestrator zip, and to extract and load them when running the CP stage of FPGA provisioning flows. The underlying `bitstream_splice` target will likewise no longer exist after the Bazel transition to the fpga_bkdr loader are merged - as such, we define new targets and build options to transition on, which are just local to the orchestrator now. Also update the docs accordingly. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
The FPGA Python test binaries have the ability to provide a bitstream. In such cases, the test logic will not work, as the FPGA will also need to be loaded with the ROM and OTP images. Provide the plumbing necessary to pass these as arguments through to opentitantool commands so that we can load these memories via the bkdr_loader on the FPGA. Note that there is no Bazel logic here to actually provide these arguments - that will need to be fixed separately in the future. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
In preparation for moving from the bitstream splicing flow to the FPGA bkdr_loader flow, we must remove dependencies on the spliced bitstream target. One such target is the dev bundle for releases. Now, it makes more sense to package together the standard CW340 ROM_EXT VMEM images for the ROM and the OTP, so that they can be directly loaded onto a CW340 target via OpenTitanTool. We likewise now package a patched version of OpenOCD in the dev bundle, since this will be needed to load the FPGA memories over JTAG. OpenOCD will include the CMSIS-DAP adapter config by default, but we also include it here for completeness to allow users to make local modifications to the flow. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
The bkdr_loader re-uses the DFT TAP, so the rv_dm JTAG tap sel has to account for this new logic and include FPGA-specific logic for proceeding through the bkdr_loader after a reset with the TAP strap applied. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This doc comment was outdated anyhow, as it referenced the CW310 which has now been deprecated from CI. Update the comment to reflect the original intent, but now updated to suit the fpga bkdr_loader workflow, which is more involved (as you need to build OpenOCD and run OpenTitanTool - there is no more single "packaged" spliced bitstream). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
With the appropriate supporting infrastructure for the backdoor loader in place, we can now re-enable the ROM endpoint as a target that is exposed via the bkdr_loader interface. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Instead of using a bitstream spliced with the ROM and OTP, we always use the same base CW340 bitstream when running on FPGA. The scrambled ROM and OTP images are then loaded over the FPGA-unique bkdr_loader IP. To reduce the difference from the previous behaviour and avoid side effects being built up in NVM (e.g. flash info pages) across test runs, we zero other memories at the start of each test. This importantly encompasses retention SRAM and the flash info pages, but we also clear the flash data pages for good measure, to be as close to the previous spliced bitstream flow as possible. The SRAM of a loaded bitstream would be initialized to some scrambled value, and so we opt not to zero it instead (it is also unlikely that any tests would be meaningfully impacted by state carried across in SRAM). Since the backdoor loader requires JTAG, JTAG is now made a dependency of every test. All other consumers of the spliced bitstream target have been converted, so we now simply remove this target and the logic for splicing the bitstreams, as this will not be supported going forward. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Replace the existing MMI file generation logic with some simple commands that write some text to a dummy MMI file. The MMI file is not removed completely at this staged since there is still various pieces of infrastructure (Bazel, CI bitstream cache) that assume it exists. However, it will not be needed as we are now switching from the bitstream splicing flow to the bkdr_loader flow on FPGA, and this logic is in fact complicating the increase of the size of the ROM. As such, for now we stub this out with some simple textual contents. This file should be completely removed in the future. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
We no longer splice bitstreams in CI, so it no longer makes sense for the `.orig` and `.splice` references to be in the test. Still, it's useful to have the functionality offered by these tests, so we just rename the bitstreams that are being used in this test. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
These scripts were being used to generate e.g. ROM images that were specifically compatible with the VMEM format that Vivado's Updatemem tool expects. Since we no longer use the bitstream splicing flow and therefore no longer use `updatemem`, this script (and its relevant test & simulator) will no longer be usable with the larger ROM (which will not have an MMI file with the BRAM mapping needed for splicing). As this script will not be useful going forward and has been effectively replaced by the bkdr_loader flow on FPGA, remove it completely. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
As with many manual test targets, this has become broken over time. Most importantly, we cannot execute in the rom_with_fake_keys environments because these are signed with RMA keys, while we need DEV - and it is not trivial to change the key for a single env. Also remove the stale manual tag that came from when this test previously used a manual bitstream override. This is no longer relevant given the build system (exec_env and FPGA bkdr_loader) improvements. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
These docs had not yet been updated to note the jumper cables necessary for running the SPI TPM tests. Note them and give an example of a test that can be run to confirm that they have been configured correctly. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Correct a couple of outdated parts of these setup docs - particularly with regards to the fact that we do not really support using the CW340 without the Hyperdebug at all. Make the separation between the board and the interface (as given to OpenTitanTool) clearer and note the requirement of Hyperdebug for various parts of the OpenTitan test flow. Then, perform a larger overhaul to cover the new backdoor loading flow, which replaces the old bitstream splicing workflow for setting up the test environment of an FPGA. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This documentation was already out of date. Update it to match the new FPGA bkdr_loader flow and remove the references to splicing. Update the example test to match the current configuration, and make note of the current `changes_otp` flag which is used (and why it is used!) Make some small changes to the example of overriding the bitstream since we no longer have any tests that do that - it is still supported, but heavily dissuaded. Also, change the formatting to recommend current OpenTitan style guides (one sentence per line). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
This `splice_rom` script is outdated and out-of-use, and had been replaced by the Bazel splicing flows. This is now even further replaced by the new FPGA backdoor loading flow, and so the script no longer makes sense to keep, and should be entirely removed. Similarly, the FPGA reference documentation is somewhat out of date and also needs updating for the new flow. Make these changes to ensure that the doc is (relatively) up-to-date. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Remove references to splicing bitstreams and update docs where relevant. Also address some smaller areas of documentation that have fallen out of date with the current practices. Where necessary, apply relevant OpenTitan Markdown styling for consistencfy (one sentence per line). Additionally, add a small note for a needed FIXUP with old CW310 references, which should be replaced with CW340 references (but is deferred to a separate commit). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
118aac3 to
5610834
Compare
Addresses #30294.
This PR attempts to completely replace the existing bitstream splicing flow with the new FPGA backdoor loading flow (using the unique bkdr_loader IP only instantiated in the CW340 top-level). Unfortunately, there are two assumptions that are repeated throughout various parts of the codebase:
This necessitates changing a wide variety of FPGA software flows. It might be possible to split this PR up further, but will be difficult unless we opt to leave the documentation and perhaps some SW flows in a partially broken state across PRs.
JTAG Speed & Test Overheads
Note that this is currently expected to be quite slow (~8 seconds or so for ROM & OTP splicing, on every single test) because loading goes through JTAG operations to a DMI via the bkdr TAP, meaning that each write requires individual
drscanoperations to the loader's registers. In terms of speed:earlgrey_1.0.0which is running on the same CI infrastructure.Even when using FTDI @ 15,000 kHz, there is still approximately 30 μs overhead between each
drscanoperation, where each operation itself takes about 4 to 5 μs. This needs further profiling - it may be due to USB transaction latency (lots of small writes), but it also could be overhead from OpenOCD and a motivation for migrating to e.g. probe-rs. Regardless, this reduces overhead to around 1-2 seconds per test.Notes
See the comment here. Due to the second assumption listed above, I was facing issues where side effects could build up in non-volatile memory which would then only cause failures after multiple test runs, and were sometimes dependent upon the order of the tests run. Long-term, it would be good to both (a) implement flash info splicing & (b) clean up test definitions to more rigorously define what they do and do not change. For now we take a more conservative approach of clearing all non-volatile memory (flash info & data pages, SRAM and retention SRAM) to "emulate" the behaviour previously provided by loading a bitstream. I envision that in the future we would always want to clear retention SRAM, splice NVM info, and then zeroing NVM data & SRAM may be relevant but should not be required for any targets to function properly.
The orchestrator and devbundle (release) flows both assume that they can package a spliced bitstream, which no longer exists. These have been replaced by packaging the base bitstream along with the ROM and OTP images (and OpenOCD + OpenTitanTool for loading these), which should be a sufficient replacement. But this will be a breaking change to any downstream users of these targets - hopefully most users are currently following
earlgrey_1.0.0, but otherwise there may be some churn there.The Python host penetration tests have likewise been updated so that they can take a
--romand--otpargument alongside their--bitstream, although this has not been fully tested and it is not clear if/how these are even being used in the current version. This likewise probably does not matter as much onmasterat the moment, but necessitates some further investigation. When we need to get these working properly again, these flags should make doing so easier, as it will just require some additional Bazel plumbing.One potential concern - the Debug infrastructure (DMI, DTM) will now not be reset across environment changes as they were previously by bitstream clears. This could potentially cause an increase in the JTAG test flakiness we see in CI, as failures from one test cascade into another. Luckily, it seems that the majority of these tests are already clearing the bitstream before they run - but we should keep an eye on the impact that this change has on CI stability (and ideally aim to diagnose and fix the underlying JTAG issue!)
Other things to note:
USR_ACCESSis exposed via the bkdr_loader, but with this PR this flow still goes via the ROM (and thus Ibex), to reduce the amount of changes in this PR.