Support CoreMgmt over DRTIO #2559

occheung · 2024-08-27T14:00:21Z

ARTIQ Pull Request

Description of Changes

This PR implements the followings:

Flashing gateware/bootloader/firmware through artiq_coremgmt via config write similar to Zynq device.
Implement corresponding core-mgmt handling for satellites
Create new DRTIO messages for (2).

Item (1) should belong to its own PR, but I am not sure if we should toe Zynq's line on this.
Coremgmt now requires a DRTIO destination byte. It is currently implemented as mandatory, so it is a breaking change.

Related Issue

Closes #2498

Known Issues

~~Satellite will be unable to respond to AUX traffic while flashing. Taking down DRTIO link and reboot is WIP. We may want to rethink the new DRTIO messages for this.~~ Resolved.
~~Routing the new reply messages are probably missing.~~ Only needs to be controlled by the root core device. It should only forward packets, routing is unnecessary.

Tests

Remote flashing, rebooting, logging (via artiq_coremgmt and aqctl_corelog), and configuration via DRTIO between 2 core devices. Core devices ranges from Kasli 1.1, 2.0 and Kasli-SoC.

A corresponding PR will be opened to artiq-zynq soon.

Type of Changes

	Type
✓	✨ New feature

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

Use correct spelling and grammar.
Update RELEASE_NOTES.rst if there are noteworthy changes, especially if there are changes to existing APIs.
Close/update issues.

Code Changes

Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
Test your changes or have someone test them. Mention what was tested and how.
Add and check docstrings and comments
Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Git Logistics

Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
Write short & meaningful commit messages. Review each commit for messages (git show). Format:
```
topic: description. < 50 characters total.

Longer description. < 70 characters per line
```

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

Spaqin · 2024-08-28T02:14:53Z

artiq/firmware/runtime/mgmt.rs

+        ddma_mutex: &Mutex, subkernel_mutex: &Mutex, 
+        routing_table: &drtio_routing::RoutingTable, linkno: u8,
+        destination: u8, stream: &mut TcpStream, key: &String, value: &Vec<u8>) -> Result<(), Error<SchedError>> {
+        let mut message = Cursor::new(Vec::with_capacity(key.len() + value.len() + 4 * 2));


It will transmit both key and value through the DRTIO. Both key and value has a length parameter.

Oh yeah, forgot it's not C-strings we're dealing with.

Technically we can derive one of them using the size of total payload, but there is already string and bytes IO in the protocol, so I decided against it

Spaqin · 2024-08-28T02:22:56Z

artiq/firmware/satman/main.rs

+        drtioaux::Packet::CoreMgmtSetLogLevelRequest {destination: _destination, .. } => {
+            forward!(router, _routing_table, _destination, *rank, *self_destination, _repeaters, &packet);
+
+            error!("RISC-V satellite devices do not support buffered logging");


How difficult would it be to implement that? I think it would be useful to have remote log access on RISC-V too.

I think we just need to change the UARTLogger to BufferLogger, should be straight-forward.

Spaqin · 2024-08-28T02:26:33Z

artiq/firmware/satman/main.rs

+                coremgr.clear_data();
+            }
+
+            if succeeded {


Why not have a CoreMgmtReply { succeeded: bool } packet instead? Just for consistency with other aux packets.

I just found it less symmetric when we deal with requests that has a return payload (e.g. Config read, then it is whatever ConfigReadReply{ .. } and CoreMgmtReply{ false }, never would we see CoreMgmtReply{ true }).

I will change it for consistency.

occheung · 2024-08-29T04:43:38Z

Satellite now disables transceiver TX before flashing, and reboot on its own after. The log from master device while the satellite is flashing looks like this:

[    19.307458s]  INFO(runtime::mgmt): new connection from <host ip:port>
[    22.075730s] ERROR(runtime::rtio_mgt::drtio): [DEST#1] communication failed (LinkDown)
[    22.282018s]  INFO(runtime::rtio_mgt::drtio): [LINK#0] link is down
[    22.286957s]  INFO(runtime::rtio_mgt::drtio): [DEST#1] destination is down
[    33.694013s]  INFO(runtime::rtio_mgt::drtio): [LINK#0] link RX became up, pinging
[    37.659062s]  INFO(runtime::rtio_mgt::drtio): [LINK#0] remote replied after 21 packets
[    37.877679s]  INFO(runtime::rtio_mgt::drtio): [LINK#0] link initialization completed
[    37.885080s]  INFO(runtime::rtio_mgt::drtio): [DEST#1] destination is up
[    37.890785s]  INFO(runtime::rtio_mgt::drtio): [DEST#1] buffer space is 128

Spaqin · 2024-08-29T05:05:45Z

artiq/firmware/satman/mgmt.rs

+                drtioaux::send(0, &drtioaux::Packet::CoreMgmtAck)?;
+                #[cfg(not(soc_platform = "efc"))]
+                unsafe {
+                    clock::spin_us(10000);


Is the wait there to make sure the message is sent out?

Yes. There are no specific reason for picking 10ms.

10ms sounds like a lot, doesn't drtioaux::send wait until all data is out of the BRAM buffer, and then the encoder/transceiver latency is just a few hundred nanoseconds at most (and some of it might be in parallel with the txenable path)?

The delay also compensates the time the master devices need to receive the message.
With a delay of 10us, the master device has a large chance to detect link down before processing the message, so the remote_coremgmt module would report aux packet error (link down).
100us seems to work reliably in a 2 RISC-V devices setup (master <-> satellite).

Just implement a reliable handshake then, instead of "seems to work".
I.e. satellite waits for "going down ACK" from master before bringing down the link.

Spaqin · 2024-08-29T05:09:31Z

artiq/firmware/satman/mgmt.rs

+        let value = self.current_payload.read_bytes().unwrap();
+
+        match key.as_str() {
+            "gateware" | "bootloader" | "firmware" => {


I would assume that all three elements would get flashed at once, like they would if calling artiq_flash or on Zynq with the whole boot.bin. Should it reboot after flashing each part, actually? Or should it allow writing to flash without reboot, and the system would have to be rebooted in a separate step?

Especially since it seems master will not do that.

Yes, just flash everything at once. Developers who need to write only one part or perform other unusual actions would use JTAG.

Having some other instruction to prompt the flash looks hacky, especially when we implement this under config write. The config isn't really written in this case.

And there are 3 binaries we need to flash. Maybe we make a new coremgmt action to specifically deal with this instead?

Definitely less hacky than a magic delay. The magic delay is also suboptimal and unreliable and when it fails it'll be difficult to debug.

Yes, replacing the firmware is different from config write, and having a separate protocol would be OK.

dnadlinger · 2024-08-29T13:09:41Z

Is there a sufficient amount of checksumming on the binary payloads before they are written to flash? We have been (very occasionally, but still) seeing what looks like data corruption on the DRTIO links (aux packet error log messages), and potentially writing out corrupted gateware/firmware sounds like a debugging nightmare.

hartytp · 2024-08-29T14:26:26Z

seeing what looks like data corruption on the DRTIO links (aux packet error log messages), and potentially writing out corrupted gateware/firmware sounds like a debugging nightmare.

Side note: I suspect this may be a widespread issue which people just aren't noticing. I do wonder whether having a checksum on DRTIO would be a good idea to make this kind of thing more obvious and easier to debug?

sbourdeauducq · 2024-08-30T00:59:15Z

Just checksum the entire binaries before starting to flash, regardless of what DRTIO does.

sbourdeauducq · 2024-09-30T07:33:38Z

artiq/firmware/satman/main.rs

+        drtioaux::Packet::CoreMgmtDropLinkAck { destination: _destination } => {
+            forward!(router, _routing_table, _destination, *rank, *self_destination, _repeaters, &packet);
+
+            #[cfg(not(soc_platform = "efc"))]


not(has_drtio_eem) ?

artiq/artiq/firmware/satman/main.rs

Lines 755 to 758 in 333623e

#[cfg(not(soc_platform = "efc"))]

unsafe {

csr::gt_drtio::txenable_write(0xffffffffu32 as _);

}

This checks not EFC, though I do agree it should check for DRTIO EEM as well.
Probably not an intended change in #2410, as pointed out in #2134. I will make a PR to revert this.

sbourdeauducq · 2024-09-30T07:34:48Z

artiq/firmware/satman/mgmt.rs

+            }
+
+        } else {
+            panic!("CRC failed in SDRAM (actual {:08x}, expected {:08x})", actual_crc, expected_crc);


"in SDRAM" makes it sound like this is a SDRAM issue. Just write something like "CRC failed, images have not been written to flash".

sbourdeauducq · 2024-09-30T07:37:20Z

artiq/frontend/aqctl_corelog.py

@@ -25,6 +25,9 @@ def get_argparser():
                        help="Simulation - does not connect to device")
    parser.add_argument("core_addr", metavar="CORE_ADDR",
                        help="hostname or IP address of the core device")
+    parser.add_argument("-s", "--satellite", default=0,
+                        metavar="DRTIO_ID", type=int,


metavar="SATELLITE"
It should match the argument.

And it should be DRTIO_DEST or similar since this does not apply only to satellites.

sbourdeauducq · 2024-09-30T07:37:46Z

artiq/frontend/aqctl_corelog.py

@@ -25,6 +25,9 @@ def get_argparser():
                        help="Simulation - does not connect to device")
    parser.add_argument("core_addr", metavar="CORE_ADDR",
                        help="hostname or IP address of the core device")
+    parser.add_argument("-s", "--satellite", default=0,
+                        metavar="DRTIO_ID", type=int,
+                        help="the logged DRTIO destination")


"logged" is not very appropriate.

sbourdeauducq · 2024-09-30T07:41:31Z

artiq/frontend/artiq_coremgmt.py

+            def artifact_path(this_binary_dir, *path_filename):
+                if args.srcbuild:
+                    # source tree - use path elements to locate file
+                    return os.path.join(this_binary_dir, *path_filename)
+                else:
+                    # flat tree
+                    # all files in the same directory, discard path elements
+                    *_, filename = path_filename
+                    return os.path.join(this_binary_dir, filename)
+
+            def convert_gateware(bit_filename):
+                bin_handle, bin_filename = tempfile.mkstemp(
+                    prefix="artiq_",
+                    suffix="_" + os.path.basename(bit_filename))
+                with open(bit_filename, "rb") as bit_file, \
+                        open(bin_handle, "wb") as bin_file:
+                    bit2bin(bit_file, bin_file)
+                atexit.register(lambda: os.unlink(bin_filename))
+                return bin_filename
+
+            gateware = convert_gateware(artifact_path(
+                args.directory, "gateware", "top.bit"))
+            bootloader = artifact_path(
+                args.directory, "software", "bootloader", "bootloader.bin")


This stuff is copied from artiq_flash, can it be shared?
It's also kinda messy, this way we'd only have to clean it up once later.

Spaqin · 2024-09-30T08:39:37Z

artiq/frontend/artiq_coremgmt.py

+    t_flash = tools.add_parser("flash",
+                               help="flash the running system")
+
+    p_zynq = t_flash.add_argument("-z", "--zynq", default=False,


any chance for auto detection?

I guess we can simply try both riscv and zynq binaries, then go for the one that succeed.

Just like runtime/satman is currently detected.

architeuthidae · 2024-09-30T13:32:18Z

RELEASE_NOTES.rst

@@ -19,6 +19,8 @@ ARTIQ-9 (Unreleased)
 * Python 3.12 support.
 * Compiler can now give automatic suggestions for ``kernel_invariants``. 
 * Idle kernels now restart when written with ``artiq_coremgmt`` and stop when erased/removed from config.
+* Support for coredevice reflashing through new the ``flash`` tool in ``artiq_coremgmt``.


typo: 'through new the'

occheung · 2024-10-10T09:44:54Z

The runtime firmware currently does not support the termination of EFC's DRTIO connection, we may need to implement reconnect for DRTIO-over-EEM as well.

sbourdeauducq · 2024-10-10T12:14:33Z

The runtime firmware currently does not support the termination of EFC's DRTIO connection

Why not? Sounds like a bug/oversight/design issue.

sbourdeauducq · 2024-10-21T07:53:45Z

artiq/frontend/fetch_bin.py

+    if component == "gateware":
+        path = convert_gateware(path)
+
+    return path


Merge this file and bit2bin into one new file with an appropriate name.

sbourdeauducq · 2024-10-21T07:59:43Z

artiq/frontend/fetch_bin.py

+            prefix="artiq_", suffix="_" + os.path.basename(bit_filename))
+        with open(bit_filename, "rb") as bit_file, open(bin_handle, "wb") as bin_file:
+            bit2bin(bit_file, bin_file)
+        atexit.register(lambda: os.unlink(bin_filename))


It's a bit hacky that functions would have random atexit side effects like that.

sbourdeauducq · 2024-10-21T08:01:16Z

artiq/frontend/fetch_bin.py

+        atexit.register(lambda: os.unlink(bin_filename))
+        return bin_filename
+
+    if type(component) == list:


Would it be cleaner if the function only accepted lists, potentially containing only one element?

architeuthidae · 2024-10-24T09:43:57Z

artiq/frontend/artiq_coremgmt.py

+                pass
+
+        if retrieved_bins is None:
+            raise FileNotFoundError("both risc-v and zynq binaries not found")


nitpick: probably 'neither risc-v nor zynq'

This avoids reallocation when transfering binaries. Reallocation during flash handshakes could cause DRTIO timeouts.

sbourdeauducq · 2024-11-20T00:09:57Z

artiq/frontend/flash_tools.py

+# after
+# https://github.com/mfischer/fpgadev-zynq/blob/master/top/python/bit_to_zynq_bin.py
+
+import struct


Move import to top with the others.

sbourdeauducq requested a review from Spaqin August 27, 2024 14:41

Spaqin reviewed Aug 28, 2024

View reviewed changes

Spaqin reviewed Aug 29, 2024

View reviewed changes

architeuthidae mentioned this pull request Aug 29, 2024

doc: config over DRTIO / coremgmt flash #2562

Merged

2 tasks

occheung force-pushed the drtio-coremgmt branch from 038135a to 9990012 Compare September 19, 2024 09:19

occheung marked this pull request as ready for review September 20, 2024 09:38

sbourdeauducq reviewed Sep 30, 2024

View reviewed changes

Spaqin reviewed Sep 30, 2024

View reviewed changes

architeuthidae reviewed Sep 30, 2024

View reviewed changes

architeuthidae mentioned this pull request Oct 10, 2024

doc: Additonal EBAZ4205 documentation #2588

Merged

2 tasks

occheung mentioned this pull request Oct 14, 2024

Handle EFC DRTIO Disconnection #2594

Merged

1 task

sbourdeauducq reviewed Oct 21, 2024

View reviewed changes

architeuthidae reviewed Oct 24, 2024

View reviewed changes

occheung added 2 commits November 18, 2024 09:53

config: impl flashing over mgmt

12682a2

drtio: add new messages for remote mgmt

b60a616

occheung added 25 commits November 18, 2024 09:53

drtop-proto: rearrange packet assignment

e85b640

satman coremgmt: impl config erase

a56294f

runtime mgmt: remove cursor

f2f27e2

runtime mgmt: reorganize uses

4a54241

drtio-proto: avoid expecting drop link ack response

2b73d5a

frontend: make coremgmt flash zynq-compatible

5c21649

satman mgmt: get_slice_sat -> get_slice_satellite

a2d341f

runtime mgmt: minor fix

2865450

runtime mgmt: avoid passing incomplete message to corelog

6b6bcdb

runtim mgmt: fix pull log message relaying

9d3204d

update RELEASE_NOTES

2e9622b

flake8

cc40313

satman: uart_logger -> buffer_logger

ea61d9b

satman: support coremgmt logging

ffcf79b

RELEASE_NOTES: fix typo

9882e16

coremgmt flash: update crc mismatch message

21944ff

coremgmt frontend: fix arg name

e8bd990

artiq_flash/coremgmt flash -> fetch_bin

f5ff908

fix frontend args reference

a643da2

feature gate txenable with has_drtio_eem

1583deb

drtio_proto: add allocate step for flashing

f2c13a5

This avoids reallocation when transfering binaries. Reallocation during flash handshakes could cause DRTIO timeouts.

bit2bin, fetch_bin -> flash_tools

de349e4

coremgmt flashing: detect risc-v or zynq targets

e36916b

board_misoc: enable efc rebooting

6251e73

both ... not -> neither ... nor

90764b0

occheung force-pushed the drtio-coremgmt branch from ec4327a to 90764b0 Compare November 18, 2024 08:08

sbourdeauducq reviewed Nov 20, 2024

View reviewed changes

sbourdeauducq added 2 commits November 20, 2024 08:22

Merge branch 'master' into drtio-coremgmt

1ed04eb

flash_tools: cleanup import

df1a389

sbourdeauducq merged commit 8b335c4 into m-labs:master Nov 20, 2024

	#[cfg(not(soc_platform = "efc"))]
	unsafe {
	csr::gt_drtio::txenable_write(0xffffffffu32 as _);
	}

Support CoreMgmt over DRTIO #2559

Support CoreMgmt over DRTIO #2559

Uh oh!

Conversation

occheung commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ARTIQ Pull Request

Description of Changes

Related Issue

Known Issues

Tests

Type of Changes

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

Code Changes

Git Logistics

Licensing

Uh oh!

Spaqin Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

occheung commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spaqin Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger commented Aug 29, 2024

Uh oh!

hartytp commented Aug 29, 2024

Uh oh!

sbourdeauducq commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

occheung commented Aug 27, 2024 •

edited

Loading

Spaqin Aug 28, 2024 •

edited

Loading

Spaqin Aug 29, 2024 •

edited

Loading

sbourdeauducq commented Oct 10, 2024 •

edited

Loading