Opentitan earlgrey changes for upstream#323
Opentitan earlgrey changes for upstream#323lexbaileylowrisc wants to merge 12 commits intolowRISC:masterfrom
Conversation
This is the start of efforts to contribute code from the lowRISC fork of QEMU to bring the QEMU support for OpenTitan machines in line with Earl Grey 1.0 by adding a new machine called ot-earlgrey. This work was done by Emmanuel Blot and Loïc Lefort from Rivos, and various people at lowRISC. This patch set in particular adds the basic machine definition, and one of the peripherals required for it: the alert handler device. I previously emailed asking for advice on how to test our changes in a way that is acceptable upstream, and how best to submit such a large set of changes, if anyone has more thoughts on that, please do reply to that email. (https://lists.nongnu.org/archive/html/qemu-riscv/2026-02/msg00139.html) There's lots more commits to come, I don't want to dump too much on the mailing list all at once. For now this is a small first step. This patch set should give a flavour of the style of what's to come, and I'm expecting there will be some changes required. We'd just like to understand what will be needed to make this acceptable as soon as we can before we try to upstream lots more of these changes. In particular, most of our new peripheral blocks are currently in the directory called hw/opentitan. In our fork we have about 60 blocks in this directory, we have put them there to keep them organised as opentitan-specific blocks. Is this a suitable place for them? would you prefer a different structure? Thanks! To: qemu-devel@nongnu.org Cc: Palmer Dabbelt <palmer@dabbelt.com> Cc: Alistair Francis <alistair.francis@wdc.com> Cc: Weiwei Li <liwei1518@gmail.com> Cc: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Cc: Liu Zhiwei <zhiwei_liu@linux.alibaba.com> Cc: qemu-riscv@nongnu.org Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Cc: Alistair Francis <alistair@alistair23.me> Cc: Pierrick Bouvier <pierrick.bouvier@linaro.org> Cc: Dr. David Alan Gilbert <dave@treblig.org> Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Philippe Mathieu-Daudé <philmd@linaro.org> Cc: lowRISC <qemu-maintainers@lowrisc.org> Cc: nabihestefan@google.com <nabihestefan@google.com> Cc: Amit Kumar-Hermosillo <amitkh@google.com> Signed-off-by: Lex Bailey <lex.bailey@lowrisc.org> --- b4-submit-tracking --- # This section is used internally by b4 prep for tracking purposes. { "series": { "revision": 1, "change-id": "20260217-opentitan-earlgrey-474a14215feb", "prefixes": [ "RFC" ] } }
Follow vendor-device syntax used with other RISCV cores Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
Use a separate Kconfig symbols for Ibex UART, Timer, and SPI devices: having an Ibex CPU does not imply usage of these specific implementations. Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
b25d9af to
3717ad8
Compare
hw/opentitan/meson.build
Outdated
| # TomCrypt dependency | ||
|
|
||
| libtomcrypt_dep = subproject('libtomcrypt').get_variable('libtomcrypt_dep') | ||
|
|
There was a problem hiding this comment.
I don't think we're submitting libtomcrypt
There was a problem hiding this comment.
yep, missed this, removed
hw/opentitan/ot_alert.c
Outdated
| * Note: for now, only a minimalist subset of Power Manager device is | ||
| * implemented in order to enable OpenTitan's ROM boot to progress |
There was a problem hiding this comment.
I think this comment is a copy-paste error anyway. removed.
There was a problem hiding this comment.
I've left a few comments.
One additional general comment I have is that I wonder if it is also easier just to recreate the OpenTitan machine from scratch (like we are with the alert handler peripheral here), rather than pulling commits from the history.
Otherwise, there might be a lot of review noise as we add later commits to e.g. change PLIC and alert mapping numbers (iirc, this happens a lot). It is also a bit strange from the perspective of upstream to introduce an OT Earlgrey machine based on RTL from an old commit and then continually update it over time.
At the same time, that would then also require effort to recreate the Earlgrey machine in new commits as well. But the benefit is that we would give up on rebasing and we then don't have to go through the process of getting all those intermediary change commits reviewed. I suspect this would become a problem anyway, because if review comments require us to change a lot of stuff, then the rebases would become even more painful anyway.
target/riscv/monitor.c
Outdated
|
|
||
| mem_info_svxx(mon, env); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we need this commit for this patch? (adding general RISC-V registers to the monitor). It seems like a convenience commit to me, and directly contradicts the existing comment in the code.
Even if we did want it, I suspect it would be better as a separate patch/contribution.
include/hw/ssi/ssi.h
Outdated
|
|
||
| typedef enum SSICSMode SSICSMode; | ||
|
|
||
| #define TYPE_SSI_BUS "SSI" |
There was a problem hiding this comment.
I suspect upstream will want us to motivate this change - it would be good to try and figure out why we need the SSI bus to be public (where are we using it, and why? Why is this the correct solution?)
There was a problem hiding this comment.
turns out it's not needed. removed.
docs/opentitan/index.md
Outdated
| @@ -0,0 +1,25 @@ | |||
| # Ibex SoC emulation | |||
There was a problem hiding this comment.
A few thoughts:
- I suspect upstream will desire
.rstfiles, not.mdfiles. - I suspect
docs/opentitan/is too non-specific, it is likely we will want docs to live somewhere likesystem/devices/opentitan/and / orsystem/riscv/opentitan/depending on what is being documented. - Maybe it's fine to leave it as is, but I wonder if upstream won't want us replicating QEMU build command docs here? Just a thought.
There was a problem hiding this comment.
I think I will leave this like this for now (for the sake of speed) if upstream complains then we'll change it
| uint64_t henvcfg; | ||
|
|
||
| /* Ibex custom CSRs */ | ||
| target_ulong cpuctrlsts; |
There was a problem hiding this comment.
I mentioned previously, I am not sure if this will be acceptable to upstream (and I'm not actually sure why it's needed, the MIPS and T-Head CSRs don't seem to need to add to the CPUArchState). Either way, it feels like if this is needed, it should be in a separate patch submission to the one adding the alert handler.
There was a problem hiding this comment.
agreed, this feels like it shouldn't be here, but I no longer have time to fix this :( we'll see what upstream says
hw/opentitan/meson.build
Outdated
| @@ -0,0 +1,6 @@ | |||
| # TomCrypt dependency | |||
|
|
|||
| libtomcrypt_dep = subproject('libtomcrypt').get_variable('libtomcrypt_dep') | |||
There was a problem hiding this comment.
We can't have the tomcrypt dependency for upstream, at least right now.
| { | ||
| MachineClass *mc = MACHINE_CLASS(oc); | ||
|
|
||
| mc->desc = "RISC-V Board compatible with OpenTitan EarlGrey FPGA platform"; |
There was a problem hiding this comment.
Nit - we probably don't want the "FPGA" references in this commit, but I also don't think that matters much at this stage, especially if it makes rebasing harder.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * QEMU RISC-V Helpers for LowRISC Ibex Demo System & OpenTitan EarlGrey | |||
| * QEMU RISC-V Helpers for LowRISC OpenTitan EarlGrey and related systems | |||
There was a problem hiding this comment.
Nit: if possible, it would be good to remove the Ibex demo systems on the original commits rather than the last commit, to reduce confusion when reviewing and polluting the QEMU history.
| @@ -0,0 +1,670 @@ | |||
| /* | |||
There was a problem hiding this comment.
I suspect upstream will want hw/riscv/opentitan/ or hw/misc/opentitan/ instead but I also think it's fine to leave it as is for now (no need to put a bunch of effort in now if they might be happy with it anyway), especially since you mention this in the initial commit.
There was a problem hiding this comment.
yeah, I'm gonna leave it like this and wait for them to complain
0ee3636 to
99617b6
Compare
Signed-off-by: Lex Bailey <lex.bailey@lowrisc.org>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com> Includes existing MIT licenced code (already published elsewhere)
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
1. Define a generic version of lowrisc-ibex core that can be used in several machines: - leave MISA empty so that generic properties can be defined for this core - remove all arbitrary default properties but ISA I,C,U which are mandatory for ibex - define default mtvec which is only support vectored mode - update privilege version (1.12) according to the Ibex documentation - define ibex architecture identifier - remove hart array (mostly useless, its definition is incoherent and prevent from applying properties to CPU cores) 2. Add an EarlGrey machine that uses this new definition Signed-off-by: Loïc Lefort <loic@rivosinc.com> Signed-off-by: Emmanuel Blot <eblot@rivosinc.com> Signed-off-by: Lex Bailey <lex.bailey@lowrisc.org>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
also adds property no_epmp_cfg on EarlGrey machine to disable default ePMP configuration. Usage: qemu-system-riscv32 -M ot-earlgrey,no-epmp-cfg=true [...] Signed-off-by: Emmanuel Blot <eblot@rivosinc.com> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com> Includes existing MIT licenced code (already published elsewhere)
Signed-off-by: Lex Bailey <lex.bailey@lowrisc.org> Includes existing MIT licenced code (already published elsewhere)
99617b6 to
9ed7dac
Compare
DO NOT MERGE
this is just so we know what we're proposing upstream