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

Support for HiFive Unmatched #386

Merged
merged 21 commits into from
Dec 13, 2023

Conversation

Nanamiiiii
Copy link
Contributor

@Nanamiiiii Nanamiiiii commented Nov 14, 2023

This PR adds board support for HiFive Unmatched. (#384)
You can generate SD card image for unmatched using buildroot by single make command.
It will not be available for production, but it will allow for easy testing.

I checked that almost all example runs correctly, but only attestor.ke throw runtime error bacause the package does not include firmware image (fw_jump.elf) correctly. Same thing is happening with qemu target.

New items/Changes

  • Add buildroot config for unmatched in overlays/keystone/configs
  • Add board specific configs/patches in overlays/keystone/board
    • Some patches in freedom-u-sdk & meta-sifive (2023.08) are included
    • Implement secureboot same as bootrom in u-boot. This is applied via a patch.
  • Unmatched's platform overrides in upstream OpenSBI is added to sm/plat
  • Add make target to flush SD card image
  • Other minor modifications

Known issue

Porting issue mentioned here is still remaining.
With default SMM_SIZE (0x200000), machine hangs when switching to S-Mode entering u-boot proper (when function sbi_hart_switch_mode is called).

In this PR, change SMM_SIZE to 0x80000 same as sbi domain region by passing compile flag for unmatched target. This is temporary fix.

Remarks

FU740 has waymasks same as FU540. Waymasking implemented for FU540 can be ported to FU740, I think.

Nanamiiiii and others added 14 commits November 9, 2023 00:15
Port buildroot & keystone sm to hifive unmatched.
Some patches were added from oe-layer, freedom-u-sdk and meta-sifive.
This can generate sdcard image for unmatched by single make command.
Buildroot toolchain is configured to use lp64d,
but opensbi will use lp64 by default.
By adding PLATFORM_RISCV_TOOLCHAIN_DEFAULT=1 to env,
opensbi will use compiler matched abi when compiling.
PLATFORM for opensbi and KEYSTONE_PLATFORM don't always coincide.
KEYSTONE_PLATFORM should be used when specifying sm special sources.
Changes for testing were mixed in.
Copy link
Collaborator

@grg-haas grg-haas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Nanamiiiii!

Thank you for this comprehensive PR -- you've done a great job adding support for this platform to Keystone under the new build system. Attached, please find a few comments/suggestions. We should address these in order to decrease future maintenance costs as Keystone continues to evolve. Of course, all are up for discussion -- please do let me know if you disagree with anything and we can find some common ground :)

Thanks again,
Gregor

Makefile Outdated Show resolved Hide resolved
mkutils/plat/unmatched/flush.mk Outdated Show resolved Hide resolved
sm/plat/generic/objects.mk Show resolved Hide resolved
sm/plat/generic/sifive/fu740.c Show resolved Hide resolved
sm/src/objects.mk Show resolved Hide resolved
New files related keystone secureboot are located in src/uboot.
They are copied into extracted u-boot source before building.
patches include only modification in original source.
@grg-haas
Copy link
Collaborator

@dayeol do you mind giving this a brief glance before approval? I think this looks pretty great, but just want to run it by you before merging

@grg-haas grg-haas requested a review from dayeol November 16, 2023 19:04
Copy link
Contributor

@dayeol dayeol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! This aligns well with the goal of the project to make it portable across many RISC-V platforms. Your contribution will really help others to use this project for theirs with more available unmatched boards.

I have a question and a minor concern regarding the added license, which we could resolve together. I'll get back really soon :D

sm/plat/generic/sifive/fu740.c Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe GPL2 is a copyleft license and inclusion of it would affect some requirements. I will consult this with CCC. Meanwhile, is there a way this license could be completely removed from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is attached to the systemd unit and udev rule for LED control of the board, and may be removed if they are not needed.
It is possible to download the sources directly from freedom-u-sdk at build time without including them in Keystone's repository, but I don't know if that would affect the license.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this code is completely separate from our repo, I would like to keep it excluded from our repo as it might be confusing & complicated to maintain.
Could you please make it download the sources directly from freedom-u-sdk?

Other than that, I'm good with the PR. Thanks in advance!

@Nanamiiiii
Copy link
Contributor Author

With default SMM_SIZE (0x200000), machine hangs when switching to S-Mode entering u-boot proper (when function sbi_hart_switch_mode is called).

In this PR, change SMM_SIZE to 0x80000 same as sbi domain region by passing compile flag for unmatched target. This is temporary fix.

I found the cause of this issue. While first board initialization process of u-boot, it use 0x80200000 as stack top. So stack attempts to stretch to the protected area that has lower address and cause access violation.
I solved this by changing U-Boot load address a little higher. 0x80210000 was accepted when I tried.

@dayeol dayeol self-requested a review December 2, 2023 22:02
Copy link
Contributor

@dayeol dayeol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please exclude GPL-licensed code from the PR and have it download the code at build time if necessary.

White early initialization, U-Boot stretch stack to lower addr space
from its load addr `0x80200000`. So the stack will touch pmp protected
space and U-Boot hangs due to access violation.
To avoid this, U-Boot load addr is changed to upper addr, `0x80210000`.
Some files imported from freedom-u-sdk are GPL licensed.
So they're removed from keystone repo and directly downloaded at build-time.
@dayeol dayeol merged commit a06b054 into keystone-enclave:master Dec 13, 2023
1 check failed
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.

3 participants