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

feat: Add OpenTitan Earl Grey ROM_EXT board to KNOWN_BOARDS #116

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

pqcfox
Copy link
Contributor

@pqcfox pqcfox commented Jul 19, 2024

This PR adds a new board, opentitan_earlgrey_rom_ext, to KNOWN_BOARDS in order to allow Tockloader to load applications in execution environments where OpenTitan uses the ROM_EXT ROM extension boot stage.

ROM_EXT is a section in flash occupying addresses 0x20000000 to 0x20010000 which acts as an intermediate boot stage between the actual ROM and any kernel running on OpenTitan.

By creating a new board in KNOWN_BOARDS whose address_translator indicates that flash starts at 0x20010000, we can avoid Tockloader clobbering ROM_EXT when loading the kernel and apps, and allow Tockloader to be used with the full boot stack in OpenTitan.

For reverse compatibility with branches of OpenTitan which don't use ROM_EXT but do use Tockloader, a new board was added instead of modifying the existing KNOWN_BOARDS entry.

"address_translator": lambda addr: addr - 0x20010000,
"flash_file": {
# Set to the maximum flash size.
"max_size": 0x00100000,
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to adjust the flash-file max size to the size that's actually available for Tockloader to use now? Assuming I added this correctly for opentitan_earlgrey, this should probably be 0x000F0000 now?

This makes sure that Tockloader errors out when it would overrun the actual device flash for the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, thank you so much. Let me double check the flash map to confirm, then will amend the commit!

Copy link
Contributor Author

@pqcfox pqcfox Jul 19, 2024

Choose a reason for hiding this comment

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

Just finished my check, it looks like from this issue the eFlash has a maximum storage of 512kB (-> 0x00080000) at 256 pages and 2 banks. I just looked at top_earlgrey.hjson in the OpenTitan repo which confirms that the page and bank numbers are still current.

From that, it sounds like 0x00070000 would be the right size--I'll tentatively push a commit based on that, but if there's any details I'm missing let me know and I'll amend right away :)

Edit, the PR I was referencing was out of date, my dearest apologies! Added max_size as 0x000F0000 in the commit below.

Copy link
Member

Choose a reason for hiding this comment

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

I think 0x00070000 makes sense in this case!

The impact of setting this value too small is that you might get a spurious error, and setting it too large might cause Tockloader to produce a huge image file (which happened to me in development once, hence introducing this flag 😅) and producing an image that overflow's the actual device flash. Neither's a subtle bug, so this should be a fairly low-risk (hah!) change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point! I'll go ahead and set it low for now, thanks so much!

@pqcfox pqcfox force-pushed the feat/add_opentitan_rom_ext_board branch 3 times, most recently from fe5af7c to 3f5da04 Compare July 19, 2024 21:26
Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

This looks good to me. In case the flash max size is too low after this change we can always bump it back. But, assuming that the kernel + apps should fit on a single flash bank, this looks correct to me.

@lschuermann lschuermann requested a review from bradjc July 19, 2024 21:28
Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

If the proper start address is 0x20010000, why is tockloader being asked to write before that? We have many similar boards that use a bootloader and the address translator is not the mechanism to handle that. The translator should only be for discrepancies between the chips memory map and the addresses that a tool (eg openocd) uses.

@lschuermann
Copy link
Member

lschuermann commented Jul 19, 2024

The translator should only be for discrepancies between the chips memory map and the addresses that a tool (eg openocd) uses.

Hm, in this case the "tool" that Tockloader uses to write to the board is the flash-file backend, where the beginning of that file is placed at the start of flash of the board. That's pretty much universal across boards that use the flash-file backend (e.g., for LiteX Arty, the flash-file is loaded at 0x40000000).

Maybe we should instead replace the address translator with a backend-specific offset? If a board has flash mapped at some address X, then the flash-file backend will always require an offset of X, whereas OpenOCD or JLink may expose the flash at address X and doesn't require an offset.

Then again, other boards do sometimes expose the flash at a different address in the debugger vs. the on-chip memory map, so even for those backends we need the ability to specify an offset.

In any case, I do think that this is the correct fix for now, and we should re-think address translation generally.

@bradjc
Copy link
Contributor

bradjc commented Jul 19, 2024

I want to fix this, but a board should be one board. Having address 0x20000000 (for example) mean different things on the same hardware is too likely to lead to confusion. I think we need a --trim-flash-file option or similar.

@lschuermann
Copy link
Member

lschuermann commented Jul 19, 2024

I want to fix this, but a board should be one board. Having address 0x20000000 (for example) mean different things on the same hardware is too likely to lead to confusion. I think we need a --trim-flash-file option or similar.

I don't think we disagree, maybe I can clarify what I'm proposing. Address X on the board means address X in this board's memory map, no other interpretation.

Loaders may expose address X on a board at address Y, which I'll call the "loader offset".

The flash-file loader will generally expose an address X on a board at an offset that is equal to the beginning of flash in a device's memory map. We should thus have a flash-file loader offset.

Other loaders (namely openocd for the non-LiteX Arty board) may also have an offset where they expose the device's flash at an address different from the device's memory-map view. Those loaders will thus also have their own offset.

We'd deprecate the address_translator generally. All it does is encode loader-specific offsets, and thus should not be a general loader-agnostic option.

Does that make sense?

@lschuermann
Copy link
Member

(I'd also be open to calling these offsets and limits something different for flash-file, like flash_start and flash_size, if that helps)

@pqcfox
Copy link
Contributor Author

pqcfox commented Jul 19, 2024

I'd be happy to draft something up along these lines--agree that having an offset flag (either for just the flash-file tool or more generally for loaders) seems like a preferable solution.

If there's consensus on whether to implement this for just flash-file or more generally, I'll go ahead and start on a new PR and we can close this one :)

@bradjc
Copy link
Contributor

bradjc commented Jul 22, 2024

We'd deprecate the address_translator generally. All it does is encode loader-specific offsets, and thus should not be a general loader-agnostic option.

I agree with this. In hindsight it does not make sense to assume that all loader tools will use the same deviations between the memory map address and the address they need to write to the correct location. I think we should support the same mechanism separately for each loader type.

What I'm still not clear on is how to support flash file for different assumptions about where the flash file will be placed. In this proposed context (where the translator is loader-specific) that would imply two different loaders (one that puts the flash file at address 0x20000000 and one that uses 0x20010000). But, having two different flash file loaders would be confusing, I think.

Do we have a flag for the flash loader that customizes a start address? Or an offset?

@lschuermann
Copy link
Member

lschuermann commented Jul 22, 2024

Okay, I'm beginning to understand the confusion. What this PR proposes is not two different flash-file loaders, but rather treating EarlGrey as two semantically different boards: one is where the user-exposed and usable flash region begins at 0x20000000, and one where it begins at 0x20010000.

This is not caused by switching loaders, but rather by (as far as I understand) new EarlGrey chips (or rather, ROM versions) implementing a signature check for the next stage booted after ROM, and thus needing an intermediate signed boot stage called ROM_EXT that then ends up completing the system boot and transferring over to "owner firmware" (i.e., Tock).

So, in essence, we're dealing with two different "boards" now: one that requires the ROM_EXT bootloader, and one that doesn't. The kernel is placed at different addresses on each of those "boards", but in both cases the flash-file loader base address always refers to the start of user-accessible flash.

@lschuermann
Copy link
Member

Pinging @pqcfox for confirmation on the above.

@lschuermann lschuermann requested a review from a team July 22, 2024 16:48
@bradjc
Copy link
Contributor

bradjc commented Jul 22, 2024

But those aren't two different boards. At least not in the way a board is conventionally defined. For example, the nrf52840dk is the nrf52840dk whether it has a bootloader or not. But the presence of a bootloader does change where the kernel needs to be located.

If tockloader was used to write the kernel to a flash file and then that file was loaded, either (assume kernel starts at 0x10000):

  • The loading tool needs to skip 0x10000 bytes from the flash file
  • Tockloader is configured to offset 0x10000 when writing the flash file and the file is written starting at 0x10000

I would like to avoid having different "boards" for different kernel starting addresses. The issue is that whatever is loading the flash file doesn't know to skip X bytes before loading. If one loader starts writing "address 0" (aka the start of the flash file) at 0x10000 and a different loader starts writing "address 0" (aka the start of the flash file) at 0x20000, then that is two different loaders with two different behaviors.

@lschuermann
Copy link
Member

lschuermann commented Jul 22, 2024

But the presence of a bootloader does change where the kernel needs to be located.

AFAIK, on EarlGrey, it does. That's my argument for why it's two different boards. The (non-)ROM_EXT variants also differ in other aspects, such as the handover ePMP configuration. On ROM_EXT variants, the first 0x10000 would be reserved for the ROM_EXT and thus don't count towards user/owner-accessible flash.

If my understanding is correct, the non-ROM_EXT variant will be phased out. Right now we have the situation where we need to support both targets.

If tockloader was used to write the kernel to a flash file and then that file was loaded, either (assume kernel starts at 0x10000):

* The loading tool needs to skip 0x10000 bytes from the flash file

* Tockloader is configured to offset 0x10000 when writing the flash file and the file is written starting at 0x10000

I don't think the above would work, given that the kernel doesn't always start at an offset of 0x10000.

Notwithstanding the above, I do agree that we should have a flash-file start offset / trim parameter configurable on the command line. When you want to flash a kernel using the upstream board definition right now, you'd then do --flash-file-offset 0x20010000 --address 0x2001000 because both the flash-file start address and the actual kernel start address are shifted by the same amount. That indicates to me that we're dealing with a different board here.

@lschuermann lschuermann removed the request for review from a team July 22, 2024 16:58
@lschuermann
Copy link
Member

(I'm confused, I could have sworn I requested the review of core-wg on tock/tock/pull/4075 instead, sorry for the noise!)

@bradjc
Copy link
Contributor

bradjc commented Jul 22, 2024

Leon and I chatted, and while I don't like what is happening here, Tockloader should make this eas(ier) for users.

Can we be opinionated on what is "opentitan_earlgrey" and what is something else? If this new thing is the future, isn't that "opentitan_earlgrey"? Or is it that there is no longer just one thing that is "opentitan_earlgrey" and then nothing should be called "opentitan_earlgrey"? How does the datasheet describe this new thing? A new starting address has to be known, right? Can we call it what the datasheet calls it?

By adding a board we are asking users to decide which they have. Ideally we would just auto-detect internally. We need to make sure users know how to choose the right one.

And for my own benefit, for posterity, I'm ok with calling this new thing a new board because it has a different hardware memory map from the old thing.

@pqcfox
Copy link
Contributor Author

pqcfox commented Jul 22, 2024

I think that's an excellent point!

Regarding being opinionated on naming, I do think the existing board whose flash start is at 0x20000000 should retain the name "opentitan_earlgrey." The Earl Grey datasheet leaves it ambiguous what goes into flash, so that both

  • Earl Grey which boots directly from ROM -> Tock @ 0x20000000
  • Earl Grey which boots from ROM -> ROM_EXT @ 0x20000000 -> Tock @ 0x20001000 ("secure boot")

are considered Earlgrey-M2.5.2-RC0, but the former appropriately reflects the datasheet's indifference to what goes at 0x20000000. (Also, this is what master currently uses in OpenTitan, so it's somewhat of the "canonical" choice).

I'd propose the name "opentitan_earlgrey_secure_boot" for a human readable name for the new, latter board, as it helps unambiguously denote that it should be used exactly when OpenTitan's secure boot infrastructure is being used (where ROM_EXT comes in).

Lastly, because the difference between the two is immutable and selected by the silicon manufacturer (i.e. can't be changed by the user of the board), I agree it's a good idea to distinguish these as separate boards. Ultimately, only "opentitan_earlgrey_secure_boot" will be used in production, but maintaining support for the current "opentitan_earlgrey" would be very useful given its extensive use in OpenTitan right now.

Let me know your thoughts--pushing an amended commit now with these changes just in case. Thanks!

@pqcfox pqcfox force-pushed the feat/add_opentitan_rom_ext_board branch from 3f5da04 to 8fb60c9 Compare July 22, 2024 21:36
@bradjc bradjc merged commit a347ba8 into tock:master Jul 23, 2024
2 checks passed
@pqcfox pqcfox deleted the feat/add_opentitan_rom_ext_board branch July 23, 2024 17:00
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.

4 participants