[topgen] Improve support for multiple address spaces #23122
Conversation
Razer6
requested review from
jwnrt and
jadephilipoom
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
2 times, most recently
from
6c23793 to
eaf64b5
Compare
Razer6
requested review from
a-will
and removed request for
msfschaffner,
jwnrt and
jadephilipoom
a-will
force-pushed
the
dev-multiple-addr-spaces
branch
2 times, most recently
from
81c0174 to
32ad6ff
Compare
There was a problem hiding this comment.
Hm, no real content. I suppose there isn't any reason to generate these C files at this time, but also... no harm leaving it.
There was a problem hiding this comment.
Yeah, these address spaces only have definitions in the header. Not sure if we can determine if there is nothing to generate. Probably the length of all internal structures is 0?
util/topgen/c_test.py
Outdated
| def _get_irq_peripherals(self): | ||
| if self.addr_space != self.default_addr_space: | ||
| return [] |
There was a problem hiding this comment.
I'm not totally sure whether this restriction would generally hold, but it doesn't hurt to start with it.
There was a problem hiding this comment.
No, in the future, I would like to determine if there is a PLIC in the address space and then yield the devices that are connected to that one - keyword interrupt_domain.
|
Looks like englishbreakfast may need similar attention :) |
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
2 times, most recently
from
4393271 to
7621c1f
Compare
|
CI is happy now :) @andreaskurth Who be eligible to review that PR form a python perspective? |
Razer6
added
Component:Tooling
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
from
7621c1f to
821e459
Compare
util/topgen/c.py
Outdated
| @@ -125,6 +132,28 @@ def __init__(self, top_info, name_to_block: Dict[str, IpBlock]): | |||
| self._init_clkmgr_clocks() | |||
| self._init_mmio_region() | |||
|
|
|||
| def all_device_regions(self) -> Dict[str, MemoryRegion]: | |||
There was a problem hiding this comment.
@a-will This function was newly added to capture the memory regions of all devices, not just in the current address space. This was needed since the single alert handler needs to capture all devices, not just the ones from the current address space.
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
from
821e459 to
595873f
Compare
Razer6
requested review from
rswarbrick,
mundaym,
a team,
vogelpi and
cfrantz
as code owners
util/topgen.py
Outdated
| rsformat_dir.mkdir(parents=True, exist_ok=True) | ||
| # The Rust file needs some complex information, so we initialize this | ||
| # object to store it. | ||
| rs_helper = TopGenRust(completecfg, name_to_block, version_stamp, addr_space['name']) |
There was a problem hiding this comment.
I know that the rest of this file has the same problem, but maybe it makes sense to do some sanity checking of addr_space['name'] here? (Make sure it's a string, maybe? And how does default_addr_space relate to the allowed values here? (Or maybe that sort of check belongs somewhere earlier, rather than being Rust-specific?)
There was a problem hiding this comment.
I agree, it might be checked that addr_space['name'] is a string. There is no custom validation for these kind of dicts, right? default_addr_space is a boolean, to indicate that address space is a the default address space. Maybe that should be checked somewhere else as part of the general validation phase.
There was a problem hiding this comment.
I don't think there is (not code I wrote...). And good point about default_addr_space: I totally misread your code!
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
from
a54d8d0 to
32e1390
Compare
Razer6
removed request for
andreaskurth,
hcallahan-lowrisc,
matutem,
mundaym,
vogelpi,
cfrantz and
milesdai
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
3 times, most recently
from
e6bc97d to
667e183
Compare
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
2 times, most recently
from
b76909f to
8cdfb81
Compare
Individually generate C and Rust collateral for all address spaces Signed-off-by: Robert Schilling <rschilling@rivosinc.com>
Razer6
force-pushed
the
dev-multiple-addr-spaces
branch
from
8cdfb81 to
58a1740
Compare
|
Closing since #25894 is merged |
This PR uses a different approach than #22961 to support multiple address spaces better and generate their C and Rust collateral.
In this PR,
topgeniterates over all address spaces available and individually callsTopGenCorTopGenRuston it. Consequently, all address spaces are treated separately, and all outputs are generated in separate files. Therefore, the address space becomes part of the file name as a suffix after the top name. One address space is a default address space (annotated in the HJSON, e.g., thehart) that doesn't face this change in file name. That means the currently existing file names stay the same (without suffixes) and describe thehartaddress space.The C and Rust headers generate the definition for alerts and interrupts. Right now, these definitions will only be generated for the default address space. This will be improved in the future since multiple address spaces with multiple CPUs might have their own PLIC. Consequently, the interrupt definitions for different address spaces or interrupt domains will be generated in the corresponding file.