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

risc-v: switch to hart1 #68

Merged
merged 1 commit into from
May 17, 2021
Merged

risc-v: switch to hart1 #68

merged 1 commit into from
May 17, 2021

Conversation

oliver-wm
Copy link

seL4 and librarys assume booting on hart1 while
Opensbi and U-boot pick a hart at random.
This patch uses an SBI call to start the elfloader on hart1
if a different hart has been choses.
Credits go to Peter Chubb for the straw-man example.

Signed-off-by: Oliver Scott Oliver.Scott@data61.csiro.au

@oliver-wm oliver-wm requested a review from yyshen April 13, 2021 04:19
@oliver-wm
Copy link
Author

revised from 59

@@ -24,7 +28,7 @@ _start:

/* OpenSBI starts all HARTs and sends them to this entry point. */
li s0, CONFIG_FIRST_HART_ID
bne a0, s0, secondary_harts
bne a0, s0, switch_heart
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bne a0, s0, switch_heart
bne a0, s0, switch_hart

Copy link
Author

Choose a reason for hiding this comment

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

👍

@@ -41,6 +45,14 @@ bootstack_secondary_cores:

.text

switch_heart:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
switch_heart:
switch_hart:

Copy link
Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@nomadeel nomadeel left a comment

Choose a reason for hiding this comment

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

Suggestions are mostly nits, looks good to me otherwise.

@kent-mcleod
Copy link
Member

What is supposed to happen if the bootloader only supports the legacy SBI extension?

@oliver-wm
Copy link
Author

oliver-wm commented Apr 15, 2021

What is supposed to happen if the bootloader only supports the legacy SBI extension?

Should we even support the legacy SBI extension?

I can add a check to call the existing code based on the SBI version or something like this.

@kent-mcleod
Copy link
Member

Should we even support the legacy SBI extension?

The only SBI calls that we currently use are legacy SBI extensions: https://github.com/seL4/seL4/blob/master/include/arch/riscv/arch/sbi.h#L38

This allows the kernel to still be used with older SBI implementations such as the BBL.

I can add a check to call the existing code based on the SBI version or something like this.

I think that is a good idea.

@oliver-wm
Copy link
Author

oliver-wm commented Apr 15, 2021

Sorry I should have been a bit more clear.

This allows the kernel to still be used with older SBI implementations such as the BBL

Do we want to support BBL? I suppose it wont hurt, but I don't see why people would be using it over more mature alternatives. I don't have that much context though.

That is the only reason they exist, but are now depreciated in favor of the listed exts here. I suppose our code could more accurately reflect that each call is a separate extension, if I understand it correctly.

@axel-h
Copy link
Member

axel-h commented Apr 15, 2021

I'd propose that you can select the SBI implementation. That is how I am using this at the moment. Depending what SBI you have, this defnes how the commands are actually implemented. Then we don'*t have to worry about the details now. I can make a PR with my changes for SBI selection.

@oliver-wm oliver-wm force-pushed the hart-1-riscv branch 2 times, most recently from 67da61b to 17a5432 Compare April 19, 2021 02:19
@oliver-wm
Copy link
Author

Ok @kent-mcleod I've added the check

otherwise rely on the legacy extension */
jal spi_probe_extension
li s0, 1
blt x1, s0, secondary_harts
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a bit info about what value is returned by spi_probe_extension and why the value is compared with 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

so if the extension is not supported, the current hart is not hart1, and SMP is not enabled, the hart just spins. Is that the expected results?

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's right. BBL worked like this. If using OPENSBI this code path shouldn't be used.

@@ -5,7 +5,7 @@
*/

#include <elfloader_common.h>
#include "sbi.h"
#include <sbi.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in another commit?

Copy link
Member

Choose a reason for hiding this comment

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

Or this change can be dropped as it doesn't affect the rest of the PR I believe.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah thought I would just fix it up. Not sure if it was intended.

@kent-mcleod
Copy link
Member

kent-mcleod commented Apr 21, 2021

@oliver-wm: Are you able to create some riscv SMP test configurations for the CI?

@DieracDelta
Copy link

DieracDelta commented Apr 21, 2021

Conceptually, how does this work with SMP? Suppose we're lucky and openSBI starts elfloader with hart 1/CONFIG_FIRST_HART_ID. Is elfloader supposed to start up all secondary cores and have them spin until sel4 gives a IPI? Or is that delegated to the seL4 kernel?

A couple of details about why I'm asking this: I'm trying to get SMP running on the hifive board with openSBI. My test application is sel4test with SMP support enabled (NUM_CORES is 4). When I have elfloader spin up only hart 1, sel4 hangs. I'm guessing sel4 is waiting for the other harts (which I have explicitly stopped) to register an IPI and start. On the other hand, when I have elfloader spin up all harts (with only hart 1 being passed through to elf loader's main), getCurrentCPUIndex comes out to 4051. I'm guessing the per-hart kernel stack pointer isn't initialized correctly as this is how getCurrentCPUIndex obtains the index. Perhaps someone can shed some light on what the boot control flow is supposed to look between sel4 and elfloader when SMP support is enabled.

@oliver-wm
Copy link
Author

Conceptually, how does this work with SMP? Suppose we're lucky and openSBI starts elfloader with hart 1/CONFIG_FIRST_HART_ID. Is elfloader supposed to start up all secondary cores and have them spin until sel4 gives a IPI? Or is that delegated to the seL4 kernel?

Yes, elfloader starts the other cores now once main has initialised from hart 1.

A couple of details about why I'm asking this: I'm trying to get SMP running on the hifive board with openSBI. My test application is sel4test with SMP support enabled (NUM_CORES is 4). When I have elfloader spin up only hart 1, sel4 hangs. I'm guessing sel4 is waiting for the other harts (which I have explicitly stopped) to register an IPI and start. On the other hand, when I have elfloader spin up all harts (with only hart 1 being passed through to elf loader's main), getCurrentCPUIndex comes out to 4051. I'm guessing the per-hart kernel stack pointer isn't initialized correctly as this is how getCurrentCPUIndex obtains the index. Perhaps someone can shed some light on what the boot control flow is supposed to look between sel4 and elfloader when SMP support is enabled.

I've just fixed a bug with the registers, some parameters were added to the elfloader code and the kernel code was never updated. So this should fix your issue. See seL4/seL4@baad619

seL4 and librarys assume booting on hart 1 while
Opensbi and U-boot pick a hart at random.

This patch uses the HSM SBI call (if it exisits)
to start the elfloader on hart1 if a different hart
has been chosen.

If SMP is enabled it now starts all harts with the HSM extension
from boot.c.

Credits go to Peter Chubb for initial spike.

Signed-off-by: Oliver Scott <Oliver.Scott@data61.csiro.au>
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

6 participants