Skip to content
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

FPGA: Add CPU instruction address SPI access control #243

Closed
wants to merge 16 commits into from

Conversation

secworks
Copy link
Contributor

  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
Copy link
Contributor Author

This PR is a test implementation of a solution for #234

@dehanj
Copy link
Member

dehanj commented Jun 25, 2024

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
Copy link
Member

dehanj commented Jun 25, 2024

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
Copy link
Member

dehanj commented Jun 25, 2024

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

Now the ready_spi is blocked.

      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.

Signed-off-by: Joachim Strömbergson <[email protected]>
@dehanj dehanj force-pushed the pc_based_access_control_spi branch from e569645 to ac8bf9f Compare July 4, 2024 12:16
@secworks
Copy link
Contributor Author

secworks commented Jul 8, 2024

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
Copy link
Contributor Author

secworks commented Jul 9, 2024

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.

Copy link

github-actions bot commented Jul 9, 2024

Max Frequency Info: 23.36 MHz (PASS at 18.00 MHz)

Copy link

github-actions bot commented Jul 9, 2024

Max Frequency Info:

Copy link

github-actions bot commented Jul 9, 2024

Max Frequency Info: 21.85 MHz (PASS at 18.00 MHz)

1 similar comment
Copy link

github-actions bot commented Jul 9, 2024

Max Frequency Info: 21.85 MHz (PASS at 18.00 MHz)

Copy link

github-actions bot commented Jul 9, 2024

Max Frequency Info: 22.59 MHz (PASS at 18.00 MHz)

1 similar comment
Copy link

github-actions bot commented Jul 9, 2024

Max Frequency Info: 22.59 MHz (PASS at 18.00 MHz)

@dehanj dehanj force-pushed the pc_based_access_control_spi branch from e51cdde to f308447 Compare July 9, 2024 13:43
dehanj and others added 6 commits July 9, 2024 16:00
      Add access stateful control register that toggles if access to a
      resources is granted based on if code is excuted from ROM or RAM.
      The register is used to enable or block access to SPI but
      potentially other HW resources.

Signed-off-by: Joachim Strömbergson <[email protected]>
      Use the access_ok_reg, not obsolete spi_acces_ok wire
      Remove now obsolete ROM_PREFIX define

Signed-off-by: Joachim Strömbergson <[email protected]>
Signed-off-by: Joachim Strömbergson <[email protected]>
Signed-off-by: Joachim Strömbergson <[email protected]>
Signed-off-by: Joachim Strömbergson <[email protected]>
@dehanj dehanj force-pushed the pc_based_access_control_spi branch from f308447 to e0a32da Compare July 9, 2024 14:00
Copy link

github-actions bot commented Jul 9, 2024

FPGA (SPI): Max Frequency Info: 22.59 MHz (PASS at 18.00 MHz)

      New logic looks at instruction execution from a defined
      trampoline address to enable stateful SPI access.

      The access is disabled as soon as an instruction is executed
      from any address in RAM.

Signed-off-by: Joachim Strömbergson <[email protected]>
      Add testcase that checks that access control
      is enabled and disabled as expected.

Signed-off-by: Joachim Strömbergson <[email protected]>
@dehanj dehanj mentioned this pull request Aug 22, 2024
9 tasks
@dehanj dehanj closed this Aug 26, 2024
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 this pull request may close these issues.

2 participants