support keystone on CVA6

[asyarifstudio]

The pull request contains changes to support running keystone on CVA6 with Genesys 2 Board.

1. Support both CVA6 64bit and 32bit
2. Add dedicated patches for CVA6
3. Add dedicated configuration for CVA6
4. Update CVA6 documentation

On top of that, this pull request also

1. Rearrange the Makefile to automatically fetch the configuration based on the KEYSTONE_PLATFORM
2. Fix issue with Keystone 32 Bit on QEMU

Several notes in this Pull request

1. This PR does not include Root of Trust in CVA6. Please see the more details in the documentation
2. There is still runtime error in 32 bit but the hello-native can run till then end. possibly due to CVA6 32 does not support D extension

Let me know if you have any comment

[asyarifstudio]

I just merge the branch to the latest master. I also want to highlight the issue in the CI run beb530d

1. https://app.circleci.com/pipelines/github/keystone-enclave/keystone/371/workflows/36650ac7-2e29-44a1-aef8-344eae86b3a3/jobs/1649 error was because at that time, the fast-setup.sh has been removed from the repository. it was recently readded
2. https://app.circleci.com/pipelines/github/keystone-enclave/keystone/371/workflows/36650ac7-2e29-44a1-aef8-344eae86b3a3/jobs/1650 because fast-setup.sh is not executed, the SDK not built thus the CMakelist.txt throws error that the SDK folder is not available

it seems the flow in the CI is not yet updated to use the Makefile and buildroot toolchain.

[asyarifstudio]

Hi @dayeol @grg-haas

I ping you guys to check if you have any feedback regarding this PR. it would be nice if we can have this PR accepted so the CVA6 is fully supported. On top of that, I fixed the 32 bit target on the QEMU so it's working now.

[grg-haas]

Hi @asyarifstudio ! Thank you so much for your PR. I haven't had much spare capacity for reviewing these past few weeks, but things have eased up so I'll make sure to give this a good look in the next few days

[grg-haas]

Okay, I've taken a look at this patchset. Again, thank you very much for your contributions -- its exciting to have people participate in this project. A couple of high-level notes:

1. Following the landing of the unmatched patch series, could platform/cva6/ instead live under overlays/keystone/board for consistency?
2. A fair amount of the contents of platform/cva6 seem to be copied from here. While this is fine for board support right now, this is tough to maintain since we would manually have to diff the copies of the files in our repo against the ones from upstream if there are any changes. I instead would strongly prefer cva6-sdk to be integrated as a submodule.
   1. The rootfs overlay seems to be the same as in cva6-sdk so this can be reused directly
   2. Many of the linux patches are the same as in cva6-sdk. Looking closely at their repository, I see that most (all?) of them should be good to apply in our tree as well. You do have some extra patches on top of this, so we would need another patch directory to hold these (as well as your OpenSBI patches)
3. Could your CVA6 configuration files then also live in overlays/keystone/board/cva6? I realize I had these just in overlays/keystone/configs previously, but it does make sense to separate them into more specific directories.

One potential layout could be:

• overlays/keystone/
  • configs
    • riscv{32,64}_cva6_defconfig
  • board/cva6
    • configs
      • busybox32-cva6-defconfig
      • linux{32,64}-cva6-defconfig
    • patches
      • linux
        • relative symlinks for necessary patches from linux_patch in cva6-sdk
        • 0005-Fix-LowRisc-K-Config...
        • 0006-fix-netif_napi...
      • opensbi
        • ... your opensbi patches here
    • cva6-sdk (submodule)

Then, BR2_GLOBAL_PATCH_DIR would point into $(BR2_EXTERNAL_KEYSTONE_PATH}/{patches,board/cva6/patches}. Of course, please let me know your thoughts on this approach -- I'm open to other ideas as well, but this would be easier for us to maintain in the long run.

[asyarifstudio]

Hi @grg-haas
thanks a lot for the feedback

Regarding the point 1, 3 and layout in general, I think a platform or board specific folder has to be in the root for better visibility. putting them under overlays/keystone/ may misleads because one may assume overlays is generic term for additional FS on top of BuildRoot FS. OpenSBI has similar approach with platform folder, linux with arch folder, and Buildroot with arch folder. But I don't mind moving them under overlays/keystone as well.

Regarding point 2, the files from cva6-sdk. I agree with you. However some patches are not working with Keystone because different version of Linux. I will link the rest of the files to cva6-sdk

[grg-haas]

Hi @asyarifstudio,

That's a very good point with regard to discoverability. One of my long-term plans with this Buildroot build system is to split overlays/keystone into its own repository and then submodule it here. This would then effectively allow the Keystone sources and the Buildroot integration (reference distro) to live separately, allowing both for full-stack Keystone-focused builds (driven by the Makefile in this repo), as well as easy Keystone integration into other Buildroot-based project. All that to say, I believe in the next few months that stuff under overlays/keystone will become a lot more discoverable since the board directory there will actually be on the top-level then :)

For now though, I take your point. The overlays subdirectory was poorly named, and really would be more aptly named externals. For now, would it be okay to centralize your changes under overlays/keystone/board/cva6 as described above? Then, if you like, we can also rename overlays -> externals (or something else) in order to aid discoverability.

[asyarifstudio]

Hi @grg-haas

Understood, I will move the platform/cva6 to overlays/keystone/board/cva6 for now and I let you guys manage renaming the folder in the future.

[asyarifstudio]

hi @grg-haas

I also want to propose the naming convention for additional board buildroot configuration. I saw for sifive unmatched, an if statement has to be added

ifeq ($(KEYSTONE_PLATFORM),mpfs)
	EXTERNALS += microchip
else ifeq ($(KEYSTONE_PLATFORM),unmatched)
    BUILDROOT_CONFIGFILE = riscv64_hifive_unmatched_defconfig
endif

I think we can avoid this by generalize the buildroot config name

riscv$(KEYSTONE_BITS)_$(KEYSTONE_PLATFORM)_defconfig

with this, no changes is needed in the Makefile when new buildroot config is added. However we need to do two things for qemu target

1. rename the generic config to qemu and change qemu_riscv64_virt_defconfig to riscv64_qemu_defconfig
2. rename mkutils/plat/generic to mkutils/plat/qemu

I can implement this together in this PR. I already did actually but it seems to be removed after I rebase.

what do you think?

[grg-haas]

@asyarifstudio I love this idea! Please remember to also change KEYSTONE_PLATFORM ?= qemu in the main Makefile to make this a transparent change for anyone using this platform.

@dayeol , any opposition to renaming the generic platform to qemu?

[asyarifstudio]

great. I will include this in my PR. we can adjust later if necessary

[asyarifstudio]

Hi @grg-haas

few notes on the update

1. I kept the name generic for qemu, original I thought it's because the name but it was because 524e749 from unmatched. but generic is good enough I think
2. I changed the fw_jump to fw_payload in attestation dependency. I dont see why the fw_jump is needed there. in the archived SDK repo, attestion uses fw_payload

[grg-haas]

Hi @asyarifstudio

Thanks for these changes, they broadly look good. A few clarification questions:

1. Since you kept the name of the QEMU-based platform as generic, why is the change to the unmatched SM implementation in 524e749 needed?
2. I do not yet see the cva6-sdk submodule versioned here, although you refer to it in the post_build.sh script. Please make sure to add this.
3. We build different OpenSBI firmwares based on the specific platform being used. While the archived SDK did in fact use fw_payload, we've transitioned the generic platform at least to use fw_jump instead. Rather than changing the default firmware bundled with attestation, could you do something like this?

diff --git a/examples/attestation/CMakeLists.txt b/examples/attestation/CMakeLists.txt
index 710c4ec..a700e7f 100644
--- a/examples/attestation/CMakeLists.txt
+++ b/examples/attestation/CMakeLists.txt
@@ -3,7 +3,6 @@ set(eapp_src eapp/attestor.c)
 set(host_bin attestor-runner)
 set(host_src host/attestor-runner.cpp host/host.cpp host/verifier.cpp)
 set(package_name "attestor.ke")
-set(package_script "./attestor-runner attestor eyrie-rt loader.bin --sm-bin fw_jump.bin")
 set(eyrie_plugins "none")
 
 # eapp
@@ -41,6 +40,9 @@ if(NOT DEFINED fw_bin)
   set(fw_bin ../../../images/fw_jump.bin)
 endif()
 
+get_filename_component(fw_file ${fw_bin} NAME)
+set(package_script "./attestor-runner attestor eyrie-rt loader.bin --sm-bin ${fw_file}")
+
 # add target for packaging (see keystone.cmake)
 
 add_keystone_package(${eapp_bin}-package
diff --git a/overlays/keystone/package/keystone-examples/keystone-examples.mk b/overlays/keystone/package/keystone-examples/keystone-examples.mk
index c95c746..c887dc2 100644
--- a/overlays/keystone/package/keystone-examples/keystone-examples.mk
+++ b/overlays/keystone/package/keystone-examples/keystone-examples.mk
@@ -11,6 +11,10 @@ include $(KEYSTONE)/mkutils/pkg-keystone.mk
 endif
 
 KEYSTONE_EXAMPLES_DEPENDENCIES += host-keystone-sdk keystone-runtime opensbi
+ifeq ($(KEYSTONE_PLATFORM),cva6)
+KEYSTONE_EXAMPLES_CONF_OPTS += -Dfw_bin=$(BINARIES_DIR)/fw_payload.bin
+endif
+
 KEYSTONE_EXAMPLES_CONF_OPTS += -DKEYSTONE_SDK_DIR=$(HOST_DIR)/usr/share/keystone/sdk \
                                 -DKEYSTONE_EYRIE_RUNTIME=$(KEYSTONE_RUNTIME_BUILDDIR) \
                                 -DKEYSTONE_BITS=${KEYSTONE_BITS}

[asyarifstudio]

Hi @grg-haas

Thanks a lot for your feedback. to answer your question

1. I still revert it because it cause issue when the KEYSTONE_PLATFORM is set to cva6, then it will search for plat/cva6 folder in the SM instead of plat/fpga/ariane as set inside the buildroot configuration.
2. Sorry for that, added in the last pushed
3. Yes, I followed your suggestion.

[grg-haas]

Hi @asyarifstudio, thanks for these changes! This looks really good -- I am just about ready to merge this.

I've made a PR in your fork (ThalesGroup#1) to pull these changes for cva6 into the recently integrated CI. Once that PR is merged into your fork's dev-cva6-support branch, it should also reflect in this PR here. As we build up our development infrastructure, I'd love to run more regular tests against CVA6 so we can avoid accidentally breaking this platform in the future. Therefore, I think landing this CI change (which would at least check that everything compiles against CVA6) would be great.

I was trying to think of ways that we can also do full end-to-end system tests of Keystone on CVA6. For generic, we can run QEMU in a GitHub-hosted CI runner without too much issue. For the mpfs platform (landing soon), I have a physical board with a self-hosted runner that flashes the newest build onto it and runs some tests (see here). I was wondering how to do something similar for CVA6, and I thought we could maybe do so through simulation. I see in the main CVA6 readme that they support simulation through verilator. Do you have any experience with trying to (at least functionally) simulate Keystone, or is this too slow? Or, do you typically run this CVA6 Keystone build on a physical board (Genesys 2)?

[asyarifstudio]

Hi @grg-haas

Thanks a lot. It's nice that the CI is back so we can make sure that no build is broken.

I let the build run on your PR, but there seems to be issue on build generic 32

I'm not very familiar with the CI, perhaps if you can help to share the log so I can analyze it and fix the issue.

on the second point. We tested the keystone on Genesys2 Board. I personally never tried the verilator but I will check with my colleagues here what best strategy to test Keystone on CVA6 in the CI.

[grg-haas]

Hi @asyarifstudio,

That test is kinda flaky -- sometimes, just rerunning it will make it pass. I'm not necessarily too worried about it. I've approved CI for this PR to also run in this repo, so we'll see how that turns out -- I don't expect any issues. If it passes I am comfortable with merging these changes!

And sounds good on the CVA6 CI note. I was also just curious about this, so no need to ask around if it is a large effort.