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

Only generate a dtb and bus devices if dtb_enabled #1244

Merged
merged 6 commits into from
Feb 7, 2023
Merged

Conversation

jerryz123
Copy link
Collaborator

Previously, spike would generate a dts/dtb even if !dtb_enabled, and add devices based on what was generated. This breaks the use case where a spike-as-library user wants to both:

  • provide a custom bootrom+dtb
  • provide a custom set of bus devices

!dtb_enabled now skips both dts/dtb generation and adding the default PLIC/CLINT/UART devices.

@jerryz123 jerryz123 changed the title Only generate a dtb if dtb_enabled Only generate a dtb and bus devices if dtb_enabled Feb 5, 2023
Copy link
Contributor

@scottj97 scottj97 left a comment

Choose a reason for hiding this comment

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

I don't have time to investigate right now, but b19eb27 changes the way my custom tests (proprietary, sorry) run the reset sequence. The log diff:

 core   0: 0x0000000000001008 (0xf1402573) csrr    a0, mhartid
 core   0: 3 0x0000000000001008 (0xf1402573) x10 0x0000000000000000
 core   0: 0x000000000000100c (0x0182b283) ld      t0, 24(t0)
-core   0: 3 0x000000000000100c (0x0182b283) x5  0x0000000080002000 mem 0x0000000000001018
+core   0: 3 0x000000000000100c (0x0182b283) x5  0x0000000080000000 mem 0x0000000000001018
 core   0: 0x0000000000001010 (0x00028067) jr      t0
 core   0: 3 0x0000000000001010 (0x00028067)

So the memory value at 0x1018 is different, which changes the destination of the jr. I believe this code is coming from Spike's built-in reset vector.

@jerryz123
Copy link
Collaborator Author

I think I see the issue. I'll update this in a moment.

@jerryz123
Copy link
Collaborator Author

I've removed the offending commit from this PR.

riscv/sim.h Show resolved Hide resolved
riscv/sim.cc Show resolved Hide resolved
@jerryz123 jerryz123 force-pushed the dtb-omit branch 3 times, most recently from 9ef6a83 to ac6009e Compare February 5, 2023 21:41
Copy link
Contributor

@scottj97 scottj97 left a comment

Choose a reason for hiding this comment

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

I think the commit messages for 6e1169b and 5c69bf9 need to explain the rationale. Imagine someone bisecting a regression and they land on one of those commits. The message needs to help them understand the why.

Before 3b26740, the reset was necessary because sim_t::get_dts would be
called before sim_t::make_dtb was called in reset(). This is no longer
necessary since now `make_dtb` is called in the constructor of sim_t
This makes proc_t respect cfg->pmpregions even if no dtb parsing
is performed
The DTS-based configuration already switched the default to sv57,
This just changes the processor_t constructor to match
!dtb_enabled will now result in the following behavior:
 * sim_t.dts and sim_t.dtb will be empty
 * the dtb_file passed to sim_t will be ignored
 * The default bootrom will not be instantiated
 * Bus devices normally configured by parsing the dtb will not be added
   This includes the CLINT/PLIC/UART
@aswaterman aswaterman merged commit 140bae1 into master Feb 7, 2023
@aswaterman aswaterman deleted the dtb-omit branch February 7, 2023 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.

None yet

3 participants