FPGA: Add CPU instruction address SPI access control

tillitis / tillitis-key1

Board designs, FPGA verilog, firmware for TKey, the flexible and open USB security key 🔑

https://www.tillitis.se

394 stars 24 forks source link

FPGA: Add CPU instruction address SPI access control #243

Closed secworks closed 1 month ago

secworks commented 3 months ago

  Add logic that checks if the CPU is reading an instruction
  to execute from ROM or not. If instructions are read
  from ROM, access to the SPI from the API is granted, and
  signals between the SPI master and a slave are allowed.

  If instructions are not read from ROM, any API access
  is blocked. and between the SPI master and a
  slave are disabled.

secworks commented 3 months ago

This PR is a test implementation of a solution for https://github.com/tillitis/tillitis-key1/issues/234

dehanj commented 3 months ago

The linter shows a warning:

%Warning-UNUSEDSIGNAL: /__w/tillitis-key1/tillitis-key1/hw/application_fpga/core/tk1/rtl/tk1.v:161:16: Signal is not used: 'spi_access_ok'
                                                                                                     : ... In instance application_fpga.tk1_inst
  161 |   reg          spi_access_ok;
      |                ^~~~~~~~~~~~~
                       ... For warning description see https://verilator.org/warn/UNUSEDSIGNAL?v=5.012
                       ... Use "/* verilator lint_off UNUSEDSIGNAL */" and lint_on around source to disable this message.
%Error: Exiting due to 1 warning(s)
make: *** [Makefile:196: lint] Error 1

dehanj commented 3 months ago

Testing: works in general, blocks SPI access. However spi_ready is still available, which I would argue should not. Since it is a nice API to see if one have an SPI master available.

dehanj commented 3 months ago

Noticed that the frequency is down to 23.00 MHz on this branch.

Now the ready_spi is blocked.

secworks commented 2 months ago

It seems the condition to set spi_access_ok signal is too hard, which blocks SPI access from both FW and app address space.

The condition is if ((cpu_valid & cpu_instr) & (cpu_addr[31 : 30] == ROM_PREFIX))

If the CPU is in fact waiting for a load or store to happen, we may not be in a cpu_instr. And then spi_access_ok is not set when it should be. The last commit removes the complete condition to see that SPI access is possible from SPI and APP_MODE.

secworks commented 2 months ago

As long as an access comes from the ROM prefix we should be good. Possibly that it also is a valid (i.e active) access. Since the actual access is performed by a R/W access against API registers, we now that it is a valid access. So the condition should not need to also check that it is an instruction, nor if it is a valid access. A simple prefix check should be sufficient.