Add an option to disable checksumming if possible

* Introduce feature flags which allows the proxy and m1n1 to determine which
features they have in common.
* Add a feature flag that disables checksumming (by replacing checksums with
constant values) for the data packets exchanged by REQ_MEMREAD, REQ_MEMWRITE
and REQ_EVENT. The feature is enabled if m1n1 supports it; checksumming is kept
enabled for UART communication.
* To ensure no packet loss when checksumming is disabled, an extra sentinel
value is added after the exchanged data for memory read/write operations.

Signed-off-by: Vincent Duvert <vincent@duvert.net>

proxyclient/proxy.py

@@ -76,6 +76,18 @@ class UartChecksumError(UartError):
 class UartRemoteError(UartError):
     pass
 
+class Feature(IntFlag):
+    DISABLE_DATA_CSUMS = 0x01  # Data transfers don't use checksums
+
+    @classmethod
+    def get_all(cls):
+        return cls.DISABLE_DATA_CSUMS
+
+    def __str__(self):
+        return ", ".join(feature.name for feature in self.__class__
+            if feature & self) or "<none>"
+
+
 class START(IntEnum):
     BOOT = 0
     EXCEPTION = 1
@@ -119,6 +131,9 @@ class UartInterface:
     REQ_BOOT = 0x04AA55FF
     REQ_EVENT = 0x05AA55FF
 
+    CHECKSUM_SENTINEL = 0xD0DECADE
+    DATA_END_SENTINEL = 0xB0CACC10
+
     ST_OK = 0
     ST_BADCMD = -1
     ST_INVAL = -2
@@ -156,6 +171,7 @@ class UartInterface:
         self.tty_enable = True
         self.handlers = {}
         self.evt_handlers = {}
+        self.enabled_features = Feature(0)
 
     def checksum(self, data):
         sum = 0xDEADBEEF;
@@ -166,6 +182,12 @@ class UartInterface:
 
         return (sum ^ 0xADDEDBAD) & 0xFFFFFFFF
 
+    def data_checksum(self, data):
+        if self.enabled_features & Feature.DISABLE_DATA_CSUMS:
+            return self.CHECKSUM_SENTINEL
+
+        return self.checksum(data)
+
     def readfull(self, size):
         d = b''
         while len(d) < size:
@@ -255,7 +277,7 @@ class UartInterface:
                 if self.debug:
                     print(">>", hexdump(reply))
                 checksum = struct.unpack("<I", reply[-4:])[0]
-                ccsum = self.checksum(reply[:-4])
+                ccsum = self.data_checksum(reply[:-4])
                 if checksum != ccsum:
                     print("Event checksum error: Expected 0x%08x, got 0x%08x"%(checksum, ccsum))
                     raise UartChecksumError()
@@ -334,8 +356,21 @@ class UartInterface:
             print(" Connected")
 
     def nop(self):
-        self.cmd(self.REQ_NOP)
+        features = Feature.get_all()
-        self.reply(self.REQ_NOP)
+
+        # Send the supported feature flags in the NOP message (has no effect
+        # if the target does not support it)
+        self.cmd(self.REQ_NOP, struct.pack("<Q", features.value))
+        result = self.reply(self.REQ_NOP)
+
+        # Get the enabled feature flags from the message response (returns
+        # 0 if the target does not support it)
+        features = Feature(struct.unpack("<QQQ", result)[0])
+
+        if self.debug:
+            print(f"Enabled features: {features}")
+
+        self.enabled_features = features
 
     def proxyreq(self, req, reboot=False, no_reply=False, pre_reply=None):
         self.cmd(self.REQ_PROXY, req)
@@ -349,7 +384,7 @@ class UartInterface:
             return self.reply(self.REQ_PROXY)
 
     def writemem(self, addr, data, progress=False):
-        checksum = self.checksum(data)
+        checksum = self.data_checksum(data)
         size = len(data)
         req = struct.pack("<QQI", addr, size, checksum)
         self.cmd(self.REQ_MEMWRITE, req)
@@ -363,6 +398,10 @@ class UartInterface:
                 sys.stdout.flush()
         if progress:
             print()
+        if self.enabled_features & Feature.DISABLE_DATA_CSUMS:
+            # Extra sentinel after the data to make sure no data is lost
+            self.dev.write(struct.pack("<I", self.DATA_END_SENTINEL))
+
         # should automatically report a CRC failure
         self.reply(self.REQ_MEMWRITE)
 
@@ -375,9 +414,17 @@ class UartInterface:
         if self.debug:
             print(">> DATA:")
             chexdump(data)
-        ccsum = self.checksum(data)
+        ccsum = self.data_checksum(data)
         if checksum != ccsum:
             raise UartChecksumError("Reply data checksum error: Expected 0x%08x, got 0x%08x"%(checksum, ccsum))
+
+        if self.enabled_features & Feature.DISABLE_DATA_CSUMS:
+            # Extra sentinel after the data to make sure no data was lost
+            sentinel = struct.unpack("<I", self.readfull(4))[0]
+            if sentinel != self.DATA_END_SENTINEL:
+                raise UartChecksumError(f"Reply data sentinel error: Expected "
+                    f"{self.DATA_END_SENTINEL:#x}, got {sentinel:#x}")
+
         return data
 
     def readstruct(self, addr, stype):

src/uartproxy.c

@@ -21,6 +21,7 @@ typedef struct {
             u64 size;
             u32 dchecksum;
         } mrequest;
+        u64 features;
     };
     u32 checksum;
 } UartRequest;
@@ -36,6 +37,7 @@ typedef struct {
             u32 dchecksum;
         } mreply;
         struct uartproxy_msg_start start;
+        u64 features;
     };
     u32 checksum;
     u32 _dummy; // Not transferred
@@ -62,10 +64,17 @@ static_assert(sizeof(UartReply) == (REPLY_SIZE + 4), "Invalid UartReply size");
 #define ST_XFRERR  -3
 #define ST_CSUMERR -4
 
+#define PROXY_FEAT_DISABLE_DATA_CSUMS 0x01
+#define PROXY_FEAT_ALL                (PROXY_FEAT_DISABLE_DATA_CSUMS)
+
 static u32 iodev_proxy_buffer[IODEV_MAX];
 
-#define CHECKSUM_INIT  0xDEADBEEF
+#define CHECKSUM_INIT     0xDEADBEEF
-#define CHECKSUM_FINAL 0xADDEDBAD
+#define CHECKSUM_FINAL    0xADDEDBAD
+#define CHECKSUM_SENTINEL 0xD0DECADE
+#define DATA_END_SENTINEL 0xB0CACC10
+
+static bool disable_data_csums = false;
 
 // I just totally pulled this out of my arse
 // Noinline so that this can be bailed out by exc_guard = EXC_RETURN
@@ -102,6 +111,15 @@ static inline u32 checksum(void *start, u32 length)
     return checksum_finish(checksum_start(start, length));
 }
 
+static u64 data_checksum(void *start, u32 length)
+{
+    if (disable_data_csums) {
+        return CHECKSUM_SENTINEL;
+    }
+
+    return checksum(start, length);
+}
+
 iodev_id_t uartproxy_iodev;
 
 int uartproxy_run(struct uartproxy_msg_start *start)
@@ -110,6 +128,7 @@ int uartproxy_run(struct uartproxy_msg_start *start)
     int running = 1;
     size_t bytes;
     u64 checksum_val;
+    u64 enabled_features = 0;
 
     iodev_id_t iodev = IODEV_MAX;
 
@@ -180,6 +199,14 @@ int uartproxy_run(struct uartproxy_msg_start *start)
 
         switch (request.type) {
             case REQ_NOP:
+                enabled_features = request.features & PROXY_FEAT_ALL;
+                if (iodev == IODEV_UART) {
+                    // Don't allow disabling checksums on UART
+                    enabled_features &= ~PROXY_FEAT_DISABLE_DATA_CSUMS;
+                }
+
+                disable_data_csums = enabled_features & PROXY_FEAT_DISABLE_DATA_CSUMS;
+                reply.features = enabled_features;
                 break;
             case REQ_PROXY:
                 ret = proxy_process(&request.prequest, &reply.preply);
@@ -193,7 +220,7 @@ int uartproxy_run(struct uartproxy_msg_start *start)
                     break;
                 exc_count = 0;
                 exc_guard = GUARD_RETURN;
-                checksum_val = checksum((void *)request.mrequest.addr, request.mrequest.size);
+                checksum_val = data_checksum((void *)request.mrequest.addr, request.mrequest.size);
                 exc_guard = GUARD_OFF;
                 if (exc_count)
                     reply.status = ST_XFRERR;
@@ -218,21 +245,43 @@ int uartproxy_run(struct uartproxy_msg_start *start)
                     reply.status = ST_XFRERR;
                     break;
                 }
-                checksum_val = checksum((void *)request.mrequest.addr, request.mrequest.size);
+                checksum_val = data_checksum((void *)request.mrequest.addr, request.mrequest.size);
                 reply.mreply.dchecksum = checksum_val;
-                if (reply.mreply.dchecksum != request.mrequest.dchecksum)
+                if (reply.mreply.dchecksum != request.mrequest.dchecksum) {
                     reply.status = ST_XFRERR;
+                    break;
+                }
+                if (disable_data_csums) {
+                    // Check the sentinel that should be present after the data
+                    u32 sentinel = 0;
+                    bytes = iodev_read(iodev, &sentinel, sizeof(sentinel));
+                    if (bytes != sizeof(sentinel) || sentinel != DATA_END_SENTINEL) {
+                        reply.status = ST_XFRERR;
+                        break;
+                    }
+                }
                 break;
             default:
                 reply.status = ST_BADCMD;
                 break;
         }
         reply.checksum = checksum(&reply, REPLY_SIZE - 4);
-        iodev_write(iodev, &reply, REPLY_SIZE);
+        iodev_queue(iodev, &reply, REPLY_SIZE);
 
         if ((request.type == REQ_MEMREAD) && (reply.status == ST_OK)) {
-            iodev_write(iodev, (void *)request.mrequest.addr, request.mrequest.size);
+            iodev_queue(iodev, (void *)request.mrequest.addr, request.mrequest.size);
+
+            if (disable_data_csums) {
+                // Since there is no checksum, put a sentinel after the data so the receiver
+                // can check that no packets were lost.
+                u32 sentinel = DATA_END_SENTINEL;
+
+                iodev_queue(iodev, &sentinel, sizeof(sentinel));
+            }
         }
+
+        // Flush all queued data
+        iodev_write(iodev, NULL, 0);
     }
 
     return ret;
@@ -247,8 +296,12 @@ void uartproxy_send_event(u16 event_type, void *data, u16 length)
     hdr.len = length;
     hdr.event_type = event_type;
 
-    csum = checksum_start(&hdr, sizeof(UartEventHdr));
+    if (disable_data_csums) {
-    csum = checksum_finish(checksum_add(data, length, csum));
+        csum = CHECKSUM_SENTINEL;
+    } else {
+        csum = checksum_start(&hdr, sizeof(UartEventHdr));
+        csum = checksum_finish(checksum_add(data, length, csum));
+    }
     iodev_queue(uartproxy_iodev, &hdr, sizeof(UartEventHdr));
     iodev_queue(uartproxy_iodev, data, length);
     iodev_write(uartproxy_iodev, &csum, sizeof(csum));