Add support for SiFive Premier P550 platform #1397
Conversation
src/plat/p550/config.cmake
Outdated
| else() | ||
| unset(KernelPlatformFirstHartID CACHE) | ||
| endif() |
There was a problem hiding this comment.
I know all other riscv platforms except qemu-riscv-virt also have this, but why?
There was a problem hiding this comment.
I'm not sure, I don't know enough about CMake.
There was a problem hiding this comment.
Looks like it got added in 4ede700, I don't know what's specific about KernelPlatformFirstHartID that means it has to be unset. @kent-mcleod do you know?
There was a problem hiding this comment.
This is something I was wondering about for a long time and I think I had a PR somewhere that removes all these else-blocks, as they don't make sense. This was somehow useful when there was only one RISC-V platform. But now that we have multiple platforms, it is just wrong - it basically deletes the actual target configuration, because there is not guarantee about the order the CMake file are processed. Luckily, all this does not matter, as the flag KernelPlatformFirstHartID is not used anywhere. Just the define FIRST_HART_ID is used, it's needed for the setup of the PLIC interrupt routing.
There was a problem hiding this comment.
Axel's right.
It's intention is clearing the variable from the cache when the platform isn't selected. When building projects like sel4test, camkes and sel4bench, changing platforms is supported within the same build directory by just changing the KernelPlatform cmake config.
Because most platforms declare a platform-named boolean config with config_set, this would end up in the CMake variable cache and in the generated header file. When switching away from a platform the config would still be in the cache unless it was explicitly cleared. So there's this pattern of clearing config options in the else branches of each platform configuration.
But it's only safe to clear the config option in the platform config file if it's the only platform config file to define the config option, otherwise it could delete an option set by another config file.
So it's wrong for all of these unset(KernelPlatformFirstHartID CACHE) to exist here.
A potential fix would be to just have platforms set regular variables for the KernelPlatformFirstHartID setting, and then set it at the seL4Config.cmake level once all of the platforms have been enumerated. And unsetting it if the option hasn't been set.
A larger fix would be not using the CMakeCache for configuration variables and instead exporting and importing the configuration between CMake scopes via configuration files or targets.
1b13a6e to
7bf30f4
Compare
|
I think it should be called either ESWIN EIC7700X or HiFive Premier P550, to match Linux and SiFive's website, or SiFive P550. I'm all for reducing the number of platforms, but I suspect the DTS file won't be compatible with other P550 based SoCs, so calling it P550 may give trouble if other P550 based SoCs pop up in the future. Or make it a sub-platform of hifive? Only difference are the DTS files and the number of max IRQs. |
|
Ah right. I called it P550 because of the development board but I guess P550 is also the CPU it is based on. I'll go with |
|
You can't rename without breaking and confusing things, that's why we have to get it right now for this one to prevent such problems in the future. |
|
Okay so what do you suggest? |
|
For Arm, I would call it according to the SoC, as that is what matters most for a platform port, with UARTS and memory being the only variation on different boards, which can easily be tweaked with custom DTS overlays. Pine64 also has a ESWIN EIC7700X SoC based StarPro64 board, so I guess calling it EIC7700X or EIC7700 would make sense to me. That said, as the clock speed is hard-coded in the config, sometimes we need a platform per board instead of per SoC. For the imx93 port I did the input clock was fixed, so there will be no variation between boards. The same seems to be true for the EIC7700X, it has one fixed 24MHz clock input. |
7bf30f4 to
91dc412
Compare
|
sel4test passes in normal configuration and for MCS but on SMP I get output like this: a couple dozen |
Typo of "spurious". SIP 0x20 seems to be the timer interrupt, but that shouldn't lead to a spurious interrupt, so I'm slightly confused. Riscv seems to expect SSIP for IPI handling, maybe you are getting different IRQs for IPIs? You could try with #871 applied to rule out some locking bugs. |
91dc412 to
a603103
Compare
No difference it seems.
Hmm this part should be platform independent in RISC-V so I am also confused. |
|
Another problem I've run into that I'm not quite sure how to solve is that both the elfloader and Microkit loader expect the previous bootloading stage to provide the hart ID in the OpenSBI does do this, but on the P550 the bootflow is OpenSBI -> U-Boot -> seL4 and it looks like U-Boot does not actually pass anything in a0. On the P550, software does not seem to consistently start on the same hart ID which means we do need to know at run-time what the current hart is. I could patch U-Boot but I'd really prefer not to do that. |
I guess it's solved in Linux by U-Boot rewriting the DTB passed to Linux? This FOSDEM talkmight be somewhat relevant, I only clicked on the sheets. It's 5 years old, no idea what the current state is though. Isn't there an OpenSBI call to retrieve the hart ID? A bit hacky, but perhaps sbi_hart_get_status could be used, but that only works if there is only one hart running. |
There might be I wonder, do you really need U-Boot in the boot flow? I assume it's mostly for for convenience reasons? Assuming that U-Boot usually boots linux then, is Linux using UEFI then somehow, because it has an API: https://github.com/riscv-non-isa/riscv-uefi/blob/main/boot_protocol.adoc and this seems to be a way suggestion in a linux kernel mailing lists threads. |
From what I understood, they use U-Boot to load OpenSBI and second stage U-Boot, and then Linux/seL4. It seems to replace their own low-level bootup code, so it's an integral part of the bootup now. |
I think the riscv elfloader entrypoint is structured like that because there were other platforms that would pick a non-deterministic hart to boot on. I'm not sure whether this is an assumption that should be made for all riscv platforms though. The reason for the constant was because sometimes the first hart doesn't have smode implemented. So if this platform guarantees to boot into uboot on a known smode capable core, then maybe we could just make a config for switching CPU to boot CPU for the platforms that need it. Then this plat wouldn't need to be passed the hartid by u-boot. |
Ah okay. Yes this platform doesn't have any M-mode only cores like the Star64 or HiFive Unleashed. Although I thought |
The FDT fixups only seem to happen when booting an EFI image or Linux.
Without U-Boot you can't load off of MMC or tftpboot.
No there's no SBI interface for getting the current hart, riscv-non-isa/riscv-sbi-doc#25. |
|
Looking at https://docs.u-boot.org/en/latest/usage/os/vxworks.html, it says: And this comment confirms that hartid is passed via
Have you tried |
Was able to get that working yes. Thanks for the suggestion. |
I just stumbled upon the RISC-V Advanced Interrupt Architecture (AVA) extension (overview). In particular it says: It seems unlikely you're running into this, but perhaps it's related. |
src/plat/p550/config.cmake
Outdated
| # "riscv,ndev = <0x208>". | ||
| declare_default_headers( | ||
| TIMER_FREQUENCY 1000000 | ||
| MAX_IRQ 520 |
There was a problem hiding this comment.
Can you check what the kernel.elf alignment requirements are for it's PT_LOAD section?
There's potentially a max IRQ limit of 128 IRQs for riscv at the moment.
There was a problem hiding this comment.
I just checked:
LOAD off 0x0000000000008000 vaddr 0xffffffff84000000 paddr 0x0000000084000000 align 2**15
filesz 0x000000000000b52c memsz 0x000000000002e000 flags rwx
a603103 to
f98d31f
Compare
9e604a9 to
e4ad877
Compare
|
@Indanz if I do a base DTS for the EIC7700X and then add platform names for So that way people can still specify the board with Not sure how easy that is to do with the build system but I think something similar is done for the Rocketchip. |
That sounds fine, yes. I'm okay with most solutions, as long as you're aware of the consequences of your choice. I'll just leave it up to you to explain it to people that might get confused in the future. ;-) |
If it's confusing for people then I'm happy to do something else, I just don't know what... |
7fb37ab to
5ad54aa
Compare
|
I've moved the There could be more to generalise (such as the base DTS) but I would rather not do that prematurely. I think I'm done making all the changes to the PR. I think the only problem left is the spurious interrupts on SMP. After rebasing on the latest kernel, I no longer see sel4test locking up but I do still see the spurious interrupts logs. all seem to pass, with |
I guess this happens when Might be solved by setting ipiIrq to something else than irqInvalid, and ignore it in handleInterrupt. But clearing |
Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
The SiFive Premier P550 [1] is a new development board from SiFive that is based on the ESWIN EIC7700X SoC. The platform is interesting to the seL4 community as it implements the RISC-V hypervisor extension meaning we now have real hardware to evaluate RISC-V hypervisor changes to seL4. It also implements the Sscofpmf extension and so we'll be able to get more experiment with proper profiling on RISC-V. Unfortunately, it seems we still do not have proper ASID suppor according to [2]. The board comes in two configurations, 16GB and 32GB of memory. This adds support for the 16GB model. The A9AB DTS comes from SiFive's fork of Linux [3]. No modifications were made, any extra things are in the overlay. [1]: https://www.sifive.com/boards/hifive-premier-p550 [2]: https://forums.sifive.com/t/asid-vmid-support-in-p550-eic7700x/6887 [3]: https://github.com/sifive/riscv-linux/tree/dev/kernel/hifive-premier-p550 Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
The SiFive Premier P550 1 is a new development board from SiFive
that is based on the ESWIN EIC7700X SoC.
The platform is interesting to the seL4 community as it implements
the RISC-V hypervisor extension meaning we now have real hardware to
evaluate RISC-V hypervisor changes to seL4. It also implements the
Sscofpmf extension and so we'll be able to get more experiment with
proper profiling on RISC-V.
Unfortunately, it seems we still do not have proper ASID suppor
according to 2.
The board comes in two configurations, 16GB and 32GB of memory.
This adds support for the 16GB model.
The DTS comes from SiFive's fork of Linux 3.
No modifications were made, any extra things are in the overlay.