[multitop] Implement HW rules RFC #25580

pamaury · 2024-12-10T15:52:04Z

This PR implements the Multitop HW desc rule RFC. It replaces #24791 which is the original implementation.

There is one notable difference between this implementation and the one suggested in the RFC.

RFC suggested implementation

The RFC suggested to build the top description entire out of rules and providers:

# In hw/ip/uart/BUILD:
# This target exports an OpenTitanIpInfo provider
opentitan_ip(
  name = "uart",
  hjson = "//path/to/uart.hjson"
)

# In hw/top_earlgrey/BUILD:
# This target exports an OpenTitanTopInfo provider
opentitan_top(
  name = "top_earlgrey_desc",
  hjson = "//path/to/top_earlgrey.gen.hjson",
  ips = [
    "//hw/ip/uart",
   ...
  ]
)

The information is then used to e.g. build headers:

# In hw/top:
opentitan_ip_c_headers(
  name = "uart_c_regs",
  top = "//hw/top_earlgrey:top_earlgrey_desc",
  ip = "uart",
)

Problem with this approach

While this approach works fine, we need to consider what happens when we mix this multitop. Suppose that we have a way to selecting a top though a bazel config and we use that to point to the relevant top description:

# In hw/top
alias(
  name = "top_desc",
  actual = select({
    "//hw/top:is_earlgrey": "//hw/top_earlgrey:top_earlgrey_desc",
    "//hw/top:is_darjeeling": "//hw/top_earlgrey:top_darjeeling_desc",
  })
)

opentitan_ip_c_headers(
  name = "uart_c_regs",
  top = "//hw/top:top_desc",   # <-- NOTE: not pointing to earlgrey anymore
  ip = "uart",
)

This works fine because both Earlgrey and Darjeeling have a UART. Now consider an IP block that only exists in Earlgrey or Darjeeling:

opentitan_ip_c_headers(
  name = "dma_c_regs",
  top = "//hw/top:top_desc", 
  ip = "dma",
)

opentitan_ip_c_headers(
  name = "usb_c_regs",
  top = "//hw/top:top_desc", 
  ip = "usb",
)

We run into an issue: the rule opentitan_ip_c_headers cannot work for ip = dma if run on earlgrey because Earlgrey does not have a DMA and therefore does not even know where the DMA hjson file is. One solution could be to output an empty header file but this would just push the problem onto the users: if they try to use it and depend on DMA register offsets, they will fail to compile. Therefore, the proper definition should be:

opentitan_ip_c_headers(
  name = "dma_c_regs",
  top = "//hw/top:top_desc", 
  ip = "dma",
  target_compatible_with = opentitan_require_top("darjeeling") # Express that we want darjeeling
)

opentitan_ip_c_headers(
  name = "usb_c_regs",
  top = "//hw/top:top_desc", 
  ip = "usb",
  target_compatible_with = opentitan_require_top("earlgrey") # Express that we want earlgrey
)

Where opentitan_require_top is some unspecified-as-of-yet macro, for example:

def opentitan_require_top(topname):
  return select({
    "//hw/top:is_{}".format(topname): [],
    "//conditions:default": ["@platforms//:incompatible"],
  })

But now run into another issue: we have to manually specify for each IP the list of tops that it is compatible with! This means that adding a top require adding 35+ conditions everywhere! Worse, if we change the definition of a top (e.g. Darjeeling which is supposed to be easily tweakable) then we have to change this as well. This defeats the entire point of have a top description generated by bazel.

Formalizing the problem

Thinking back, what we really would like to write is this:

opentitan_ip_c_headers(
  name = "dma_c_regs",
  top = "//hw/top:top_desc", 
  ip = "dma",
  target_compatible_with = opentitan_require_ip("dma") # Express that we only accept tops with the DMA IP
)

where opentitan_require_ip is a magical macro that somehow expands to the list of tops with the DMA block. In order to write such a macro, we need to remember that macros are expanded during Bazel's loading phase, i.e. before the build graph is even constructed. During the loading phase, the only "symbols" available to macros are:

other macros defined in .bzl files
variables defines in .bzl files

It is clear that in order to work, opentitan_require_ip needs access to a (possibly scaled-down) description of every top. Therefore the conclusion is that the top descriptions needs to be available in the loading phase. With the rule+provider approach from the RFC, it is only available in the analysis phase.

Solution

The solution proposed in the RFC is therefore to still use the idea of the RFC but at the loading phase. Here opentitan_ip and opentitan_top become macros returning struct. Those need to be placed in newly created .bzl files to be usable in the loading phase:

# In hw/ip/uart/defs.bzl:  (not a BUILD file)

UART = opentitan_ip(
  name = "uart",
  hjson = "//path/to/uart.hjson"
)

# In hw/top_earlgrey/defs.bzl:  (not a BUILD file)
load("//hw/ip/uart:defz.bzl", "UART")
EARLGREY = opentitan_top(
  name = "top_earlgrey_desc",
  hjson = "//path/to/top_earlgrey.gen.hjson",
  ips = [
    UART,
   ...
  ]
)

# In hw/top/defs.bzl:
load("//hw/top_earlgrey:defz.bzl", "EARLGREY")
load("//hw/top_darjeeling:defz.bzl", "DARJEELING")

ALL_TOPS = [EARLGREY, DARJEELING]

A simple implementation of those macros is to simply wrap the information in a struct:

def opentitan_ip(name, hjson):
    return struct(
        name = name,
        hjson = hjson,
    )

def opentitan_top(name, hjson, ips):
    return struct(
        name = name,
        hjson = hjson,
        top_lib = top_lib,
        top_ld = top_ld,
        ips = ips,
    )

This allows use to implement opentitan_require_ip simply as follows:

def opentitan_require_ip(ip):
    compatible_tops = []
    for top in ALL_TOPS:
        for _ip in top.ips:
            if _ip.name == ip:
                compatible_tops.append(top.name)
                break

    return select({
        "//hw/top:is_{}".format(top): []
        for top in compatible_tops
    } | {
        "//conditions:default": ["@platform//:incompatible"],
    })

Of course, we do not want to carry those struct around for the analysis phase so we still want to create a target that exports an OpenTitanTopInfo provider as before. This is implemented in the PR and a simplified version of the code is the following:

# In hw/top/BUILD:

# Create one description per top
[
    describe_top(
        name = "top_{}_desc".format(top),
        all_tops = ALL_TOPS,
        top = top,
    )
    for top in ALL_TOP_NAMES
]

# In hw/top/defs.bzl:
def _describe_top(ctx):
    return [
        OpenTitanTopInfo(
            name = ctx.attr.topname,
            hjson = ctx.file.hjson,
            ip_hjson = ctx.attr.ip_hjson, # simplified: ctx.attr.ip_hjson has the wrong "type" and needs a bit of massaging...
        ),
    ]

describe_top_rule = rule(
    implementation = _describe_top,
    doc = """Create a target that provides the description of a top in the form of an OpenTitanTopInfo provider.""",
    attrs = {
        "hjson": attr.label(mandatory = True, allow_single_file = True, doc = "toplevel hjson file generated by topgen"),
        "ip_hjson": attr.label_keyed_string_dict(allow_files = True, doc = "mapping from hjson files to tops"),
        "topname": attr.string(mandatory = True, doc = "Name of the top"),
    },
)


def describe_top(name, all_tops, top):
    """
    Create a target that provides an OpenTitanTopInfo corresponding to the
    requested top.

    - all_tops: list of tops (created by opentitan_top).
    - top: name of the top to use.
    """

    # Details omitted: go through the all_tops struct and extract the relevant information

    describe_top_rule(
        name = name,
        hjson = top_hjson,
        ip_hjson = all_hjson,
        topname = top,
    )

cfrantz · 2024-12-16T18:57:39Z

util/rewrite_all_hw.py

+        is_template = info.get("is_template", False)
+
+        def_file_path = ippath / ("defs.bzl.tpl" if is_template else "defs.bzl")
+        # build_file_path = ippath / ("BUILD.tpl" if is_template else "BUILD")


Are we also templating BUILD files?

Yes some (all?) ipgen IP template BUILD file. Well technically they are not templates because their content does not depend on the template but we can't name them just BUILD because then bazel considers them as a package and this can cause all sorts of weird issues potentially. Therefore calling BUILD.tpl seems like an easy way to avoid this issue. Unless you have a better idea on how to handle this?

Will we be able to dispense with the "all_files" target? That may make these BUILD and BUILD.tpl files unnecessary.

Unfortunately, there was quite a lot of pushback against touching the "all_files" targets so I have given up on touching them for the time being.

cfrantz · 2024-12-16T18:58:37Z

util/rewrite_all_hw.py

+    else:
+        assert False, "unknown step"
+
+


nit:

if __name__ == '__main__': main()

Ah yes, although I should point out that this script will disappear at the end of the PR (I just forgot to add it yet). Still good practice though.

cfrantz · 2024-12-16T19:07:20Z

hw/top/defs.bzl

+    names = {}
+    for top in ALL_TOPS:
+        for ip in top.ips:
+            names[ip.name] = {}


Would this be better as a comprehension? (also, maybe use an integer as the value, rather than empty dict?)

names = {ip.name: 1 for ip in top.ips for top in ALL_TOPS}

Too bad the set is only in bazel nightly, hehe.

cfrantz · 2024-12-16T19:11:40Z

rules/host.bzl

+        # WARNING This is a horrible hack: when we transition to host, we pretend
+        # that this is earlgrey so opentitantool can compile...


Agree. Longer term, we probably need to distinguish between chip-specific constants in opentitantool, stick them into something like a hashmap by chip, and add a --chip=... cmdline argument to select which set of constants we care about.

Yes that's a big issue. I would like to raise it in the SW WG tomorrow, there is clearly some work to be done here. I think the multitop SW PR is a good starting point to generate such a data structure.

One route: I could revive my devicetree generator, and opentitantool could consume its output for a description of the system. That would also leave open support for out-of-tree tops (instead of hard-coding entire known chips).

a-will · 2024-12-16T21:28:31Z

Out of curiosity, did you already consider a bazel module extension to create a repo that fills out the information from the hjson files? From the description, this starts to sound rather similar to a "foreign build system" or "foreign packaging" sort of problem. The hjson files seem to contain critical information about the build graph, such that it exists outside of bazel's BUILD files.

It looks like this PR goes the route of requiring the user to generate the graph with a separate call, but could we do it on the fly (in a bazel module extension) with what we already have with topgen, reggen, and the hjson files?

pamaury · 2024-12-16T23:04:28Z

Out of curiosity, did you already consider a bazel module extension to create a repo that fills out the information from the hjson files? From the description, this starts to sound rather similar to a "foreign build system" or "foreign packaging" sort of problem. The hjson files seem to contain critical information about the build graph, such that it exists outside of bazel's BUILD files.

It looks like this PR goes the route of requiring the user to generate the graph with a separate call, but could we do it on the fly (in a bazel module extension) with what we already have with topgen, reggen, and the hjson files?

Everything top related in this PR is generated by topgen (+one defs.bzl file per IP that needs to be created manually). I agree is not ideal but since we already need to run topgen to get the top library, top linker file and so on, adding one more file doesn't sound too bad and definitely was the simplest option (not the most elegant).

When I started this, it didn't really look like that and then evolved into this. If I was starting from scratch now, I would write it as a bazel extension ^^ Eventually we should probably replace the various defs.bzl with an extension, maybe taking the top_<name>.gen.hsjon files as input. However, I think it's not critical in the sense that it doesn't change the logic and the way it's used. Also I would like to make more changes to topgen before doing so.

I can look at how much work it would be to "port" to an extension, if it's not too large, maybe it can be done in this PR.

a-will

Just a few quick, probably ill-considered comments. Sorry I haven't had much time to look yet!

a-will · 2024-12-17T02:45:55Z

hw/top/BUILD

+string_flag(
+    name = "top",
+    build_setting_default = "earlgrey",
+    values = ALL_TOP_NAMES,


As written, this may be harmful to out-of-tree tops. The moniker here could probably be reused, but at some point, we'd probably want a registration setup instead of hard-coded, known in-tree tops. Though... could possibly get away with making it future work (but not delay too long).

Technically short names are less robust against collisions than labels, but the probability of that seems quite low, even in a zany ecosystem where someone registers many subsystems. Many is probably no more than dozens, hehe.

I'll note that this "top" isn't actually the top descriptor needed to determine how to compile or test software. For that, we'd need that "board" or "machine" selection. For example, it might be earlgrey on the CW340+hyperdebug machine.

It might be that this is a separate attribute that is okay on its own, though!

I thought about out-of-tree tops and I think it is ok: behind the scene, everything is just labels, the top and/or IPs can be out of tree. Potentially the only issue is the name itself: as you said it seems unlikely to have a collision but technically not impossible. If we wanted to make it really really future proof, we could replace the "names" (earlgrey, darjeeling) by a full bazel path and turn that into a label_flag, and maybe intro some aliases in .bazelrc like "earlgrey" and "darjeeling" for well-known tops.
A bazel extension might help with the registration step.

Regarding the last point, yes that's on purpose: with @cfrantz we want to make the exec_env more modular. Eventually we envision that a full description will be the top, the platform (fpga, sim_dv, probably platform is not good name, maybe call it target) and the board (hyperdebug, CW310). Then the exec_env would be replaced by a set of toolchain to provide the tools for the various building steps (compile, sign, transform, dispatch test). It's still at the design stage however, we only have some vague notes.

The life-cycle stage may also be part of ingredients that together create the exec env.

a-will · 2024-12-17T02:57:08Z

hw/top/BUILD

+        target_compatible_with = opentitan_require_ip(ip),
+        top = ":top_desc",
+    )
+    for ip in ALL_IP_NAMES


This is another moniker where a registration setup would be good, at least at some point.

a-will · 2024-12-17T02:59:05Z

hw/top/defs.bzl

+    names = {}
+    for top in ALL_TOPS:
+        for ip in top.ips:
+            names[ip.name] = {}


Too bad the set is only in bazel nightly, hehe.

a-will · 2024-12-17T03:00:51Z

hw/top/defs.bzl

+    for top in ALL_TOPS:
+        for ip in top.ips:
+            names[ip.name] = {}
+    return names.keys()


Does this need to be sorted? Will it affect configuration hashes anywhere?

Sorting is not necessary for this PR but I guess it doesn't hurt to do it anyway if this ever gets used in a way where the order is important.

matutem · 2024-12-17T21:43:54Z

Regarding the creation of the ips for a given top, the info is in the top hjson file. Perhaps we can at least enable the creation of a test to make sure all required IPS match between bazel and hjson, prior to having bazel do it from the file itself?

matutem · 2024-12-17T22:14:36Z

util/rewrite_all_hw.py

+# Copyright lowRISC contributors (OpenTitan project).
+# Licensed under the Apache License, Version 2.0, see LICENSE for details.
+# SPDX-License-Identifier: Apache-2.0
+


Not clear what this script does. Could you add a file level doc string to describe it? The text can also be referred to via __doc__ and can be made to show up in the --help message. However, it is not clear from your comments if this script is here to stay.

This script was just used to create some large commits in this PR, it should be removed at the end of the PR as it will never be used anymore. I'll delete it when I update the PR.

You can alternatively split the script into 3 (for each stage) and then just include the script content into the commit message that applies its changes.

Since the script is removed at the end, I thought it was a bit too much to split it into three. I can still do it if you think it's really necessary though?

I meant to only have the script in the commit message so it's never added or removed as part of repository. This makes the script and what it produces atomic in git history, and cause less confusion when reviewing by commits.

Add and then remove is also fine to me, I don't have too strong of an opinion about this.

Ah I see, that makes a lot of sense. I will do that.

matutem · 2024-12-17T22:27:34Z

util/rewrite_all_hw.py

+
+
+def main():
+    parser = argparse.ArgumentParser()


Adding args like this to ArgumentParser should print the doc string in the help message:
description=doc, formatter_class=RawTextHelpFormatter

Those macros record the IPs/top description in a struct. This struct can then be used to implement various build system rules and macros that need access to the top description. The main reason why these are macros and not rules is that some of this information needs to be available during bazel's loading phase, where only macro and variable expansion are available. Signed-off-by: Amaury Pouly <[email protected]>

pamaury · 2024-12-18T17:25:32Z

Out of curiosity, did you already consider a bazel module extension to create a repo that fills out the information from the hjson files? From the description, this starts to sound rather similar to a "foreign build system" or "foreign packaging" sort of problem. The hjson files seem to contain critical information about the build graph, such that it exists outside of bazel's BUILD files.

It looks like this PR goes the route of requiring the user to generate the graph with a separate call, but could we do it on the fly (in a bazel module extension) with what we already have with topgen, reggen, and the hjson files?

I have looked into using a bazel extension and I think I can make it work with #25697. However, I think that for this particular PR, it is better to not use it. Indeed, the only file which gets autogenerated in the per-top defs.bzl containing the opentitan_top call. This file is generated by topgen when the top changes which is something that needs to be done anyway, so this does not change the status quo. The extra per-IP defs.bzl files calling opentitan_ip are created manually but are expected to not change, or very rarely so again I don't think there is a huge drawback to checking them for the moment.

Longer term, I have some changes to topgen that will lead us to checking less software files into the repository. It might be a good point to revisit this choice.

The most important aspect is that even if we used a bazel extension to generated them, it would not change anything to how this information is used, i.e. it would generated the same things except that you would not see it. The only difference it would make is that the registration step is made a little bit easier.

Also regarding the string_flag versus label_flag, I don't think a label_flag is the best solution here. It's not really like a choosing a top is just "pointing something at something": it changes a lot of things in the build graph. From that perspective, selecting top is much closer to being a constraint value than a label flag. Unfortunately, constraint values cannot be specified on the command line which is why I went for a string flag. While it is true that this might create issues in case of name collision, I am not convinced that name collisions are a real issue here: it's trivial to just add any prefix/suffix/version to make the name different, and avoid the issue in the first place.

a-will · 2024-12-18T18:27:58Z

I have looked into using a bazel extension and I think I can make it work with #25697. However, I think that for this particular PR, it is better to not use it. Indeed, the only file which gets autogenerated in the per-top defs.bzl containing the opentitan_top call. This file is generated by topgen when the top changes which is something that needs to be done anyway, so this does not change the status quo. The extra per-IP defs.bzl files calling opentitan_ip are created manually but are expected to not change, or very rarely so again I don't think there is a huge drawback to checking them for the moment.

Longer term, I have some changes to topgen that will lead us to checking less software files into the repository. It might be a good point to revisit this choice.

The most important aspect is that even if we used a bazel extension to generated them, it would not change anything to how this information is used, i.e. it would generated the same things except that you would not see it. The only difference it would make is that the registration step is made a little bit easier.

I'd quibble with that point when we start to support out-of-tree tops. Right now, the topgen templates assume the definitions's labels are at particular paths in the main upstream bazel repo. At some point, the .bzl files (or whatever is used to assemble the graph) are going to need cross-repo paths to be filled in, and I suspect that will strongly indicate a desire to write the module extension.

The function of data_defs.tpl would end up getting absorbed into it--Otherwise, we're asking out-of-tree users to write those by hand, since that template likely can't handle the paths.

We don't have to add this support right now, though! But we might want to get that on a roadmap.

Also regarding the string_flag versus label_flag, I don't think a label_flag is the best solution here. It's not really like a choosing a top is just "pointing something at something": it changes a lot of things in the build graph. From that perspective, selecting top is much closer to being a constraint value than a label flag. Unfortunately, constraint values cannot be specified on the command line which is why I went for a string flag. While it is true that this might create issues in case of name collision, I am not convinced that name collisions are a real issue here: it's trivial to just add any prefix/suffix/version to make the name different, and avoid the issue in the first place.

Yeah, that seems fine to me. I was "thinking aloud" and figured I'd raise the point for discussion. Thanks for considering it! 🙂

pamaury · 2024-12-19T09:20:21Z

I'd quibble with that point when we start to support out-of-tree tops. Right now, the topgen templates assume the definitions's labels are at particular paths in the main upstream bazel repo. At some point, the .bzl files (or whatever is used to assemble the graph) are going to need cross-repo paths to be filled in, and I suspect that will strongly indicate a desire to write the module extension.

The function of data_defs.tpl would end up getting absorbed into it--Otherwise, we're asking out-of-tree users to write those by hand, since that template likely can't handle the paths.

We don't have to add this support right now, though! But we might want to get that on a roadmap.

Agreed and thanks for your comments, it's definitely helpful to think about all that. One issue with the paths is that topgen can access a CLI particular to search for IPs outside of the repo but it has no way to know what bazel repo they are in (if any!). So yeah, I agree that generally speaking, bazel modules and extensions are the better way to go to support out of tree stuff, and avoid use of ugly environment variables. It might actually be worth thinking about this more generally: we have a similar issue for hooks and other things that could benefit from bazel extensions.

This commit was generated by the following script: from pathlib import Path import subprocess all_tops = ["darjeeling", "earlgrey"] all_ips = { "hw/ip/adc_ctrl": {}, "hw/ip/aes": {}, "hw/ip/aon_timer": {}, "hw/ip/csrng": {}, "hw/ip/dma": {}, "hw/ip/edn": {}, "hw/ip/entropy_src": {}, "hw/ip/gpio": {}, "hw/ip/hmac": {}, "hw/ip/i2c": {}, "hw/ip/keymgr": {}, "hw/ip/keymgr_dpe": {}, "hw/ip/kmac": {}, "hw/ip/lc_ctrl": {}, "hw/ip/otbn": {}, "hw/ip/otp_ctrl": {}, "hw/ip/mbx": {}, "hw/ip/pattgen": {}, "hw/ip/pwm": {}, "hw/ip/rom_ctrl": {}, "hw/ip/rv_core_ibex": {}, "hw/ip/rv_dm": {}, "hw/ip/rv_timer": {}, "hw/ip/soc_dbg_ctrl": {}, "hw/ip/spi_device": {}, "hw/ip/spi_host": {}, "hw/ip/sram_ctrl": {}, "hw/ip/sysrst_ctrl": {}, "hw/ip/uart": {}, "hw/ip/usbdev": {}, # templates "hw/ip_templates/alert_handler": {"is_template": True, "tops": all_tops}, "hw/ip_templates/clkmgr": {"is_template": True, "tops": all_tops}, "hw/ip_templates/rstmgr": {"is_template": True, "tops": all_tops}, "hw/ip_templates/pwrmgr": {"is_template": True, "tops": all_tops}, "hw/ip_templates/rv_plic": {"is_template": True, "tops": all_tops}, "hw/ip_templates/flash_ctrl": {"is_template": True, "tops": ["earlgrey"]}, "hw/ip_templates/pinmux": {"is_template": True, "tops": all_tops}, # top_earlgrey 'hw/top_earlgrey/ip/ast': {}, 'hw/top_earlgrey/ip/sensor_ctrl': {}, # top_darjeeling 'hw/top_darjeeling/ip/ast': {}, 'hw/top_darjeeling/ip/sensor_ctrl': {}, 'hw/top_darjeeling/ip/soc_proxy': {}, } project_root = Path(__file__).parents[1].resolve() def run_buildifier(project_root): subprocess.run( ["./bazelisk.sh", "run", "//quality:buildifier_fix"], check=True, cwd = project_root ) def run_topgen(project_root): for top in all_tops: subprocess.run( ["./util/topgen.py", "-t", f"hw/top_{top}/data/top_{top}.hjson"], check=True, cwd = project_root, ) subprocess.run( ["make", "-C", "hw", "cmdgen"], check=False, cwd = project_root, ) def step1(project_root): new_files = [] for (_ippath, info) in all_ips.items(): ippath = project_root / Path(_ippath) ip_name = ippath.name is_template = info.get("is_template", False) def_file_path = ippath / ("defs.bzl.tpl" if is_template else "defs.bzl") # build_file_path = ippath / ("BUILD.tpl" if is_template else "BUILD") # If file does not exist, create one. if def_file_path.exists(): print(f"File {def_file_path} already exists, will overwrite") new_files.append(def_file_path) if is_template: for top in info["tops"]: new_files.append(project_root / f"hw/top_{top}/ip_autogen" / ip_name / "defs.bzl") if is_template: hjson_bazel_target = f"//hw/top_${{topname}}/ip_autogen/{ip_name}:data/{ip_name}.hjson" # noqa: E231, E501 else: hjson_bazel_target = f"//{_ippath}/data:{ip_name}.hjson" # noqa: E231 print(hjson_bazel_target) def_file = [ '# Copyright lowRISC contributors (OpenTitan project).\n', '# Licensed under the Apache License, Version 2.0, see LICENSE for details.\n', '# SPDX-License-Identifier: Apache-2.0\n', 'load("//rules/opentitan:hw.bzl", "opentitan_ip")\n', '\n', '{} = opentitan_ip(\n'.format(ip_name.upper()), ' name = "{}",\n'.format(ip_name), ' hjson = "{}",\n'.format(hjson_bazel_target), ')\n', ] def_file_path.write_text(''.join(def_file)) # Run buildifier. run_buildifier(project_root) run_topgen(project_root) subprocess.run( ["git", "add"] + new_files, check = True, cwd = project_root, ) subprocess.run( [ "git", "commit", "-vas", "-m", "[bazel] Use new rules to describe IPs", # noqa: E231 "-m", "This commit was generated by the following script:", "-m", Path(__file__).read_text(), ], check=True, cwd = project_root ) step1(project_root) Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

For now this package contains nothing but a definition file that collects all tops. Signed-off-by: Amaury Pouly <[email protected]>

Now that the top description is available from the opentitan_top macro, we can create rules to extract it in a form that is more convenient for the build system. This commit introduces three rules: - two to extract the top's C library and linker files - one to construct a top description in the form of a provider This rules essentially reformats the opentitan_top's struct content as a provider. The reason for this indirection is that for rules that will depend on the top description in the analysis phase (and not the loading phase), getting the information via a provider is much cleaner and useful that via a struct. Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

The new rule takes an input a target created by opentitan_top and will eventually replace the old one that directly used an hjson file. Signed-off-by: Amaury Pouly <[email protected]>

This commit add a new ALL_IP_NAMES variable that collects the names of all IPs in all tops listed in ALL_TOPS. It also introduces two new macros opentitan_if_ip and opentitan_require_ip that can be used to conditional include starlark expression based on the availability of a particular IP for the selected top, or express compatibility requirements for targets. Signed-off-by: Amaury Pouly <[email protected]>

These new targets are multitop aware and will replace the existing ones. Note that those targets are marked with compatibility requirements on the top. For example, //hw/top:mbx_c_regs cannot be compiled with --//hw/top=earlgrey. This guarantees that by transitivity, targets that depends on, .e.g mbx headers, can only be compiled on relevant targets. Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by the following script: from pathlib import Path import subprocess import os all_tops = ["darjeeling", "earlgrey"] all_ips = { "hw/ip/adc_ctrl": {}, "hw/ip/aes": {}, "hw/ip/aon_timer": {}, "hw/ip/csrng": {}, "hw/ip/dma": {}, "hw/ip/edn": {}, "hw/ip/entropy_src": {}, "hw/ip/gpio": {}, "hw/ip/hmac": {}, "hw/ip/i2c": {}, "hw/ip/keymgr": {}, "hw/ip/keymgr_dpe": {}, "hw/ip/kmac": {}, "hw/ip/lc_ctrl": {}, "hw/ip/otbn": {}, "hw/ip/otp_ctrl": {}, "hw/ip/mbx": {}, "hw/ip/pattgen": {}, "hw/ip/pwm": {}, "hw/ip/rom_ctrl": {}, "hw/ip/rv_core_ibex": {}, "hw/ip/rv_dm": {}, "hw/ip/rv_timer": {}, "hw/ip/soc_dbg_ctrl": {}, "hw/ip/spi_device": {}, "hw/ip/spi_host": {}, "hw/ip/sram_ctrl": {}, "hw/ip/sysrst_ctrl": {}, "hw/ip/uart": {}, "hw/ip/usbdev": {}, # templates "hw/ip_templates/alert_handler": {"is_template": True, "tops": all_tops}, "hw/ip_templates/clkmgr": {"is_template": True, "tops": all_tops}, "hw/ip_templates/rstmgr": {"is_template": True, "tops": all_tops}, "hw/ip_templates/pwrmgr": {"is_template": True, "tops": all_tops}, "hw/ip_templates/rv_plic": {"is_template": True, "tops": all_tops}, "hw/ip_templates/flash_ctrl": {"is_template": True, "tops": ["earlgrey"]}, "hw/ip_templates/pinmux": {"is_template": True, "tops": all_tops}, # top_earlgrey 'hw/top_earlgrey/ip/ast': {}, 'hw/top_earlgrey/ip/sensor_ctrl': {}, # top_darjeeling 'hw/top_darjeeling/ip/ast': {}, 'hw/top_darjeeling/ip/sensor_ctrl': {}, 'hw/top_darjeeling/ip/soc_proxy': {}, } project_root = Path(__file__).parents[1].resolve() def find_all_files(project_root, search): # Use ripgrep to find all matching files res = subprocess.run( ["rg", "-l", search], capture_output = True, cwd = project_root, ) # ripgrep returns 1 if there are no matches, 2 on error if res.returncode == 1: return [] assert res.returncode == 0, "ripgrep command failed" return [Path(os.fsdecode(path)) for path in res.stdout.splitlines()] def global_replace(project_root, search, replace, verbose): print(f'global replace "{search}" by "{replace}"') for path in find_all_files(project_root, search): path = project_root / path if verbose: print(f"Patching {path}") # Read, patch, write f = path.read_text() f = f.replace(search, replace) path.write_text(f) def run_buildifier(project_root): subprocess.run( ["./bazelisk.sh", "run", "//quality:buildifier_fix"], check=True, cwd = project_root ) def run_topgen(project_root): for top in all_tops: subprocess.run( ["./util/topgen.py", "-t", f"hw/top_{top}/data/top_{top}.hjson"], check=True, cwd = project_root, ) subprocess.run( ["make", "-C", "hw", "cmdgen"], check=False, cwd = project_root, ) def step2(project_root): for (_ippath, info) in all_ips.items(): ippath = project_root / Path(_ippath) ip_name = ippath.name is_template = info.get("is_template", False) replacements = {} for typ in ["c", "rust"]: new_target = f"//hw/top:{ip_name}_{typ}_regs" # noqa: E231 if is_template: for top in info["tops"]: # Some exceptions if ip_name in ["rv_plic", "alert_handler"]: replacements[f"//hw/top_{top}:{ip_name}_{typ}_regs"] = new_target # noqa: E231, E501 else: replacements[f"//hw/top_{top}/ip_autogen/{ip_name}:{ip_name}_{typ}_regs"] = new_target # noqa: E231, E501 else: replacements[f"//{_ippath}/data:{ip_name}_{typ}_regs"] = new_target # noqa: E231 for (old, new) in replacements.items(): global_replace(project_root, old, new, verbose=False) # Run buildifier. run_buildifier(project_root) run_topgen(project_root) subprocess.run( [ "git", "commit", "-vas", "-m", "Replace old header targets by new ones", # noqa: E231 "-m", "This commit was generated by the following script:", "-m", Path(__file__).read_text(), ], check=True, cwd = project_root ) step2(project_root) Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by the following script: from pathlib import Path import subprocess import os all_tops = ["darjeeling", "earlgrey"] project_root = Path(__file__).parents[1].resolve() def find_all_files(project_root, search): # Use ripgrep to find all matching files res = subprocess.run( ["rg", "-l", search], capture_output = True, cwd = project_root, ) # ripgrep returns 1 if there are no matches, 2 on error if res.returncode == 1: return [] assert res.returncode == 0, "ripgrep command failed" return [Path(os.fsdecode(path)) for path in res.stdout.splitlines()] def run_buildifier(project_root): subprocess.run( ["./bazelisk.sh", "run", "//quality:buildifier_fix"], check=True, cwd = project_root ) def run_topgen(project_root): for top in all_tops: subprocess.run( ["./util/topgen.py", "-t", f"hw/top_{top}/data/top_{top}.hjson"], check=True, cwd = project_root, ) subprocess.run( ["make", "-C", "hw", "cmdgen"], check=False, cwd = project_root, ) def delete_rule(lines, rule_name, target_name, file_name): try: start_idx = 0 while start_idx < len(lines): start_idx = lines.index(f'{rule_name}(\n', start_idx) if target_name is None or lines[start_idx + 1] == f' {target_name},\n': # noqa: E231 break start_idx = start_idx + 1 except ValueError: assert False, \ f"did not find beginning of rule {rule_name} (target name {target_name}) in {file_name}" try: end_idx = lines.index(')\n', start_idx + 1) except ValueError: assert False, \ f"did not find end of rule {rule_name} (target name {target_name}) in {file_name}" if start_idx > 0 and lines[start_idx - 1] == '\n' and \ end_idx + 1 < len(lines) and lines[end_idx + 1] == '\n': end_idx += 1 return lines[:start_idx] + lines[end_idx + 1:], start_idx, lines[start_idx:end_idx + 1] def delete_all_rules(lines, rule_name, target_name, file_name): while True: try: lines, _, _ = delete_rule(lines, rule_name, target_name, file_name) except: # noqa: E722 break return lines def step3(project_root): files = list(set( find_all_files(project_root, "autogen_hjson_c_header\\(") + find_all_files(project_root, "autogen_hjson_rust_header\\(") )) print(files) for path in files: build_file_path = project_root / path build_file = build_file_path.read_text().splitlines(keepends=True) # Remove load to //rules:autogen.bzl build_file, _, _ = delete_rule( build_file, 'load', '"//rules:autogen.bzl"', build_file_path ) # Remove autogen_hjson_c_header and autogen_hjson_rust_header build_file = delete_all_rules( build_file, 'autogen_hjson_c_header', None, build_file_path ) build_file = delete_all_rules( build_file, 'autogen_hjson_rust_header', None, build_file_path ) build_file_path.write_text(''.join(build_file)) # Run buildifier. run_buildifier(project_root) run_topgen(project_root) subprocess.run( [ "git", "commit", "-vas", "-m", "Remove old header targets", # noqa: E231 "-m", "This commit was generated by the following script:", "-m", Path(__file__).read_text(), ], check=True, cwd = project_root ) step3(project_root) Signed-off-by: Amaury Pouly <[email protected]>

Those rules are now unused Signed-off-by: Amaury Pouly <[email protected]>

With the new rule, a very easy way to get the list of all Hjson files for IPs is to look at the `ip_hjson` attribute of the definition of //hw/top:top_<name>_desc. Since this script kind of assumes earlgrey anyway, explicitely point to the earlgrey description of now. Signed-off-by: Amaury Pouly <[email protected]>

Unfortunately, opentitantool is far from compiling with darjeeling due to many explicit dependencies on earlgrey. At the same time, opentitantool provides many operations which are not earlgrey-specific at all such as flash image generation, signing, modid checks, etc This commit introduces a necessary hack: in the host transition for the golden toolchain, always set the top to earlgrey. This way we can ensure that opentitantool will compile. For now this does not introduce any problem but this is not a proper fix. Signed-off-by: Amaury Pouly <[email protected]>

pamaury · 2024-12-19T16:02:36Z

@cfrantz @a-will are you happy with the current state? If so, can any of you approve?

a-will

Ran out of time, so I'll have to come back: I still need to look at the particular implementation of the rules. However, the general way that this setup works seems good for now.

Thanks for the hard work!

a-will · 2024-12-20T07:22:42Z

hw/top/defs.bzl

+                break
+
+    return select({
+        "//hw/top:is_{}".format(top): obj


This top matching may be okay, too, but we could also add constraint values for the IPs directly and define platforms for the tops. Since it's all hidden underneath opentitan_require_ip(), I think we can defer that decision as well, though.

The decision to hide the details behind macros is a deliberate one so we are not bound by the current implementation and could switch to platforms in the future. We had a long discussion about platforms with @cfrantz and I am not convinced there are the way to go for tops, at least in their current state (there were some improvements in bazel 8.0). It would require one constraint value per IP and top, and in the future we might want more (like "does the top have an embedded flash"). All of this for no obvious benefit over the current implementation with a string flag?

It seems to me that platforms are more useful for toolchain related decisions, and therefore we might want to use them to replace execution environment. Constraint value could be very useful to represent the test platform/board.

a-will · 2024-12-20T07:25:32Z

hw/top_darjeeling/data/autogen/defs.bzl

+    top_lib = "//hw/top_darjeeling/sw/autogen:top_darjeeling",
+    top_ld = "//hw/top_darjeeling/sw/autogen:top_darjeeling_memory",


This might get a bit more interesting with support for different address spaces (depending on how software uses them), but we can iterate. There aren't terribly many tops at this time, so it's easy enough to update.

This file is autogenerated by topgen so changing is indeed very simple. For addresses spaces, we don't seem to have any use case yet where the software would like to know about address spaces other than its own. In particular, I imagine that if there were two address spaces for SW (e.g. one per CPU?) then you would compile your software for either one or the other and that would become part of the "top selection". All other cases could be handled by adding information to the DT but again I haven't yet seen this requirement.
Maybe @Razer6 has more information on this topic?

a-will

LGTM, except I think the alias feature for C header generation may need to be preserved.

a-will · 2024-12-20T17:45:41Z

rules/autogen.bzl

-        "alias": attr.label(
-            mandatory = False,
-            allow_single_file = True,
-            doc = "A path to an alias file",
-        ),


The alias feature might be used out-of-tree, to replace some of those integrator-specific CSRs with the real definitions. Dropping support for it might be a problem. See #10982 for the motivations.

Probably the Rust generation should support it as well, but that's a separate problem, haha.

Thanks for the explanation, I had no idea what aliases were (I could not find any example or documentation). I can add it back to the rule and add an alias attribute to the opentitan_ip macros. This way integrators can just edit the defs.bzl files to add the alias if them want to.

This would be used in a separate header generation target out-of-tree, I think. In other words, in a separate bazel workspace, someone would write their own opentitan_ip_c_header target that uses our rule, and our rule's responsibility is simply to pass through the file to the command line option and into the sandbox.

I'll note that we have no uses of the alias attribute in our repo. One of our integrators might be able to help, but we probably ought to document this, haha.

pamaury mentioned this pull request Dec 10, 2024

[multitop] New bazel rules to describe HW #24791

Closed

1 task

pamaury force-pushed the multitop_hw_desc branch 11 times, most recently from 6a33b1c to 3ad7c24 Compare December 16, 2024 14:48

pamaury changed the title ~~[WIP][DO NOT MERGE][DO NOT COMMENT] HW rules reconstruction~~ [multitop] Implement HW rules RFC Dec 16, 2024

pamaury marked this pull request as ready for review December 16, 2024 18:00

pamaury requested review from msfschaffner, vogelpi, cfrantz and a team as code owners December 16, 2024 18:00

pamaury requested review from HU90m, Razer6, a-will, nbdd0121 and jwnrt and removed request for a team December 16, 2024 18:00

pamaury mentioned this pull request Dec 16, 2024

Miscellaneous changes in preparation for multitop #25667

Merged

pamaury requested review from timothytrippel, matutem and engdoreis December 16, 2024 18:16

cfrantz reviewed Dec 16, 2024

View reviewed changes

a-will reviewed Dec 17, 2024

View reviewed changes

matutem reviewed Dec 17, 2024

View reviewed changes

pamaury force-pushed the multitop_hw_desc branch from 3ad7c24 to b9f02aa Compare December 18, 2024 17:18

pamaury force-pushed the multitop_hw_desc branch from b9f02aa to da60fd9 Compare December 19, 2024 09:22

pamaury added 13 commits December 19, 2024 15:09

[topgen] Add new templates to generate the top's description

26ba32b

Signed-off-by: Amaury Pouly <[email protected]>

[bazel] Create //hw/top package to hold multitop HW items

026bcc8

For now this package contains nothing but a definition file that collects all tops. Signed-off-by: Amaury Pouly <[email protected]>

[hw/top] Create top desc and top library targets for all tops

b054dac

Signed-off-by: Amaury Pouly <[email protected]>

[bazel,autogen] Create new rule to generate C/rust headers from top

2989b2b

The new rule takes an input a target created by opentitan_top and will eventually replace the old one that directly used an hjson file. Signed-off-by: Amaury Pouly <[email protected]>

[rules,autogen] Remove autogen_hjson_{c,rust}_header

d31fc8f

Those rules are now unused Signed-off-by: Amaury Pouly <[email protected]>

pamaury force-pushed the multitop_hw_desc branch from da60fd9 to 943c94a Compare December 19, 2024 14:20

a-will reviewed Dec 20, 2024

View reviewed changes

a-will approved these changes Dec 20, 2024

View reviewed changes

		# WARNING This is a horrible hack: when we transition to host, we pretend
		# that this is earlgrey so opentitantool can compile...

		top_lib = "//hw/top_darjeeling/sw/autogen:top_darjeeling",
		top_ld = "//hw/top_darjeeling/sw/autogen:top_darjeeling_memory",

[multitop] Implement HW rules RFC #25580

Are you sure you want to change the base?

[multitop] Implement HW rules RFC #25580

Conversation

pamaury commented Dec 10, 2024 • edited Loading

RFC suggested implementation

Problem with this approach

Formalizing the problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-will commented Dec 16, 2024

pamaury commented Dec 16, 2024 • edited Loading

a-will left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matutem commented Dec 17, 2024

matutem Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamaury commented Dec 18, 2024 • edited Loading

a-will commented Dec 18, 2024

pamaury commented Dec 19, 2024

pamaury commented Dec 19, 2024

a-will left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-will left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamaury commented Dec 10, 2024 •

edited

Loading

pamaury commented Dec 16, 2024 •

edited

Loading

matutem Dec 17, 2024 •

edited

Loading

pamaury commented Dec 18, 2024 •

edited

Loading