gdb_server: add async-notif keep-alive during memory read/write

To avoid gdb to timeout, OpenOCD implements a keep-alive mechanism
that consists in sending periodically to gdb empty strings embedded
in the "O" remote reply packet.

The main purpose of "O" packets is to forward in the gdb console
the output of the remote execution; the gdb-remote puts in the "O"
packet the string that gdb will print. It's use is restricted to
few "running/execution" contexts listed in
http://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html
and this currently limits the keep-alive capabilities of OpenOCD.

Long data transfer (memory R/W) can also cause gdb to timeout if
the interface is too slow. In this case the usual keep-alive based
on "O" packet cannot be used and, if used, would trigger a protocol
error that causes the transfer to be dropped.
The slow transfer rate can be simulated by adding some delay in the
main loop of mem_ap_write() and mem_ap_read(), then using the gdb
commands "dump" and "restore".

In the wait loop during a memory R/W, gdb drops any extra character
received from the gdb-remote that is not recognized as a valid
reply to the memory command. Every dropped character re-initializes
the timeout counter and could be used as keep-alive.

From gdb 7.0 (released 2009-10-06), an asynchronous notification
can also be received from gdb-remote during a memory R/W and has
the effect to reset the timeout counter, thus can be used as
keep-alive.
The notification would be treated as "junk" extra characters by any
gdb older than 7.0, being still valid as keep-alive.
Check putpkt_binary() and getpkt_sane() in gdb commit
74531fed1f2d662debc2c209b8b3faddceb55960

Currently, only one notification packet ("Stop") is recognized by
gdb, and gdb documentation reports that notification packets that
are not recognized should be silently dropped.
Use 'set debug remote 1' in gdb to dump the received notifications
and the junk extra characters.

Add a new level in enum gdb_output_flag for using the asynchronous
notifications.
Activate this new level during memory transfers.
Send a custom "oocd_keepalive" notification packet as keep_alive.
While there, drop a useless return in the switch/case, already
managed in case of break.

After this commit, the proper calls to keep_alive() have to be
added in the loops that code the memory transfers. Of course, the
keep_alive() should be placed during the wait for JTAG flush, not
while locally queuing the JTAG elementary transfers.

Change-Id: I9ca8e78630611597d15984bd0e8634c8fc3c32b9
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Reviewed-on: https://review.openocd.org/c/openocd/+/8165
Tested-by: jenkins

diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c
index ae288de1cf6741b4ad2efb2dc34357807ecf6e59..00e43cfe957bca175f906afe82b7c38caadd8817 100644 (file)
--- a/src/server/gdb_server.c
+++ b/src/server/gdb_server.c
@@ -53,6 +53,8 @@
 enum gdb_output_flag {
        /* GDB doesn't accept 'O' packets */
        GDB_OUTPUT_NO,
+       /* GDB doesn't accept 'O' packets but accepts notifications */
+       GDB_OUTPUT_NOTIF,
        /* GDB accepts 'O' packets */
        GDB_OUTPUT_ALL,
 };
@@ -1486,9 +1488,6 @@ static int gdb_error(struct connection *connection, int retval)
        return ERROR_OK;
 }
 
-/* We don't have to worry about the default 2 second timeout for GDB packets,
- * because GDB breaks up large memory reads into smaller reads.
- */
 static int gdb_read_memory_packet(struct connection *connection,
                char const *packet, int packet_size)
 {
@@ -3477,7 +3476,7 @@ static void gdb_log_callback(void *priv, const char *file, unsigned line,
        struct connection *connection = priv;
        struct gdb_connection *gdb_con = connection->priv;
 
-       if (gdb_con->output_flag == GDB_OUTPUT_NO)
+       if (gdb_con->output_flag != GDB_OUTPUT_ALL)
                /* No out allowed */
                return;
 
@@ -3562,10 +3561,14 @@ static int gdb_input_inner(struct connection *connection)
                                        retval = gdb_set_register_packet(connection, packet, packet_size);
                                        break;
                                case 'm':
+                                       gdb_con->output_flag = GDB_OUTPUT_NOTIF;
                                        retval = gdb_read_memory_packet(connection, packet, packet_size);
+                                       gdb_con->output_flag = GDB_OUTPUT_NO;
                                        break;
                                case 'M':
+                                       gdb_con->output_flag = GDB_OUTPUT_NOTIF;
                                        retval = gdb_write_memory_packet(connection, packet, packet_size);
+                                       gdb_con->output_flag = GDB_OUTPUT_NO;
                                        break;
                                case 'z':
                                case 'Z':
@@ -3656,9 +3659,9 @@ static int gdb_input_inner(struct connection *connection)
                                        retval = gdb_detach(connection);
                                        break;
                                case 'X':
+                                       gdb_con->output_flag = GDB_OUTPUT_NOTIF;
                                        retval = gdb_write_memory_binary_packet(connection, packet, packet_size);
-                                       if (retval != ERROR_OK)
-                                               return retval;
+                                       gdb_con->output_flag = GDB_OUTPUT_NO;
                                        break;
                                case 'k':
                                        if (gdb_con->extended_protocol) {
@@ -3754,6 +3757,39 @@ static int gdb_input(struct connection *connection)
        return ERROR_OK;
 }
 
+/*
+ * Send custom notification packet as keep-alive during memory read/write.
+ *
+ * From gdb 7.0 (released 2009-10-06) an unknown notification received during
+ * memory read/write would be silently dropped.
+ * Before gdb 7.0 any character, with exclusion of "+-$", would be considered
+ * as junk and ignored.
+ * In both cases the reception will reset the timeout counter in gdb, thus
+ * working as a keep-alive.
+ * Check putpkt_binary() and getpkt_sane() in gdb commit
+ * 74531fed1f2d662debc2c209b8b3faddceb55960
+ *
+ * Enable remote debug in gdb with 'set debug remote 1' to either dump the junk
+ * characters in gdb pre-7.0 and the notification from gdb 7.0.
+ */
+static void gdb_async_notif(struct connection *connection)
+{
+       static unsigned char count;
+       unsigned char checksum = 0;
+       char buf[22];
+
+       int len = sprintf(buf, "%%oocd_keepalive:%2.2x", count++);
+       for (int i = 1; i < len; i++)
+               checksum += buf[i];
+       len += sprintf(buf + len, "#%2.2x", checksum);
+
+#ifdef _DEBUG_GDB_IO_
+       LOG_DEBUG("sending packet '%s'", buf);
+#endif
+
+       gdb_write(connection, buf, len);
+}
+
 static void gdb_keep_client_alive(struct connection *connection)
 {
        struct gdb_connection *gdb_con = connection->priv;
@@ -3767,6 +3803,10 @@ static void gdb_keep_client_alive(struct connection *connection)
        case GDB_OUTPUT_NO:
                /* no need for keep-alive */
                break;
+       case GDB_OUTPUT_NOTIF:
+               /* send asynchronous notification */
+               gdb_async_notif(connection);
+               break;
        case GDB_OUTPUT_ALL:
                /* send an empty O packet */
                gdb_output_con(connection, "");