rspq: Fix infinite loop with max command size

Previously, the maximum command size was 63 words, which is one less
word than the size of the command buffer in DMEM. Depending on
alignment within the RDRAM buffer, commands with this maximum size could
be DMA'd into the DMEM buffer at an offset of one word. In that case,
the command ended exactly at the end of the DMEM buffer. Due to an
optimization in rspq (which saves some IMEM), this lead to rspq
refetching the commands in an infinite loop. This is now fixed by
decreasing the maximum command size to 62 instead.
Additionally, this makes rspq more robust by moving some magic numbers
to macros and adding some (static) asserts.

include/rsp_queue.inc

@@ -159,7 +159,7 @@ _RSPQ_SAVED_STATE_END:
 #   function: Address of the function that will be jumped to
 #             when this command is executed.
 #   size:     The size of the command in bytes. Must be a 
-#             multiple of 4 and in the range [0, 252].
+#             multiple of 4 and in the range [0, RSPQ_DESCRIPTOR_MAX_SIZE].
 ########################################################
 .macro RSPQ_DefineCommand function size
     .ifne ((\size) % 4)
@@ -168,18 +168,18 @@ _RSPQ_SAVED_STATE_END:
     .endif
 
     .iflt (\size)
-        .error "Invalid size - valid range: [0, 252]"
+        .error "Invalid size - valid range: [0, RSPQ_DESCRIPTOR_MAX_SIZE]"
         .exitm
     .endif
 
-    .ifgt ((\size) - 252)
-        .error "Invalid size - valid range: [0, 252]"
+    .ifgt ((\size) - RSPQ_DESCRIPTOR_MAX_SIZE)
+        .error "Invalid size - valid range: [0, RSPQ_DESCRIPTOR_MAX_SIZE]"
         .exitm
     .endif
 
     # Put the command size (as number of 4 byte words) into the high 6 bits,
     # and the jump address shifted right by 2 bits into the lower 10.
-    .short (\function - _start) >> 2 | ((\size) & 0xFC) << 8
+    .short (\function - _start) >> 2 | ((\size) & RSPQ_DESCRIPTOR_SIZE_MASK) << 8
 .endm
 
 ########################################################
@@ -497,7 +497,7 @@ rspq_execute_command:
 
     # Command size
     srl rspq_cmd_size, cmd_desc, 8
-    andi rspq_cmd_size, 0xFC
+    andi rspq_cmd_size, RSPQ_DESCRIPTOR_SIZE_MASK
 
     # Check if the command is truncated because of buffer overflow (that is,
     # it finishes beyond the buffer end). If so, we must refetch the buffer

include/rspq.h

@@ -177,7 +177,7 @@ extern "C" {
 #endif
 
 /** @brief Maximum size of a command (in 32-bit words). */
-#define RSPQ_MAX_COMMAND_SIZE          63
+#define RSPQ_MAX_COMMAND_SIZE          62
 
 /** @brief Maximum size of a command that it is writable with #rspq_write
  *         (in 32-bit words).
@@ -476,6 +476,8 @@ inline rspq_write_t rspq_write_begin(uint32_t ovl_id, uint32_t cmd_id, int size)
     extern volatile uint32_t *rspq_cur_pointer, *rspq_cur_sentinel;
     extern void rspq_next_buffer(void);
 
+    assertf(size <= RSPQ_MAX_COMMAND_SIZE, "The maximum command size is %d!", RSPQ_MAX_COMMAND_SIZE);
+
     if (__builtin_expect(rspq_cur_pointer > rspq_cur_sentinel - size, 0))
         rspq_next_buffer();
 

include/rspq_constants.h

@@ -14,6 +14,8 @@
 #define RSPQ_MAX_OVERLAY_COUNT         8
 #define RSPQ_OVERLAY_ID_COUNT          16
 #define RSPQ_MAX_OVERLAY_COMMAND_COUNT ((RSPQ_MAX_OVERLAY_COUNT - 1) * 16)
+#define RSPQ_DESCRIPTOR_SIZE_MASK      0xFC
+#define RSPQ_DESCRIPTOR_MAX_SIZE       RSPQ_DESCRIPTOR_SIZE_MASK
 
 /** Minimum / maximum size of a block's chunk (contiguous memory buffer) */
 #define RSPQ_BLOCK_MIN_SIZE            64

src/rspq/rspq.c

@@ -210,6 +210,16 @@
 // rsp_queue.S (see cmd_write_status there for an explanation).
 _Static_assert((RSPQ_CMD_WRITE_STATUS & 1) == 0);
 _Static_assert((RSPQ_CMD_TEST_WRITE_STATUS & 1) == 0);
+
+// Check that the DMEM buffer is sized at least for the largest command
+// that we can handle, plus some extra space that's required because
+// the RSP code won't run a command that ends exactly at the end of
+// the buffer (see rsp_queue.inc).
+_Static_assert(RSPQ_DMEM_BUFFER_SIZE >= (RSPQ_MAX_COMMAND_SIZE + 2) * 4);
+
+// Check that the maximum command size is actually supported by the 
+// internal command descriptor format.
+_Static_assert(RSPQ_MAX_COMMAND_SIZE * 4 <= RSPQ_DESCRIPTOR_MAX_SIZE);
 /// @endcond
 
 /** @brief Smaller version of rspq_write that writes to an arbitrary pointer */

tests/rsp_test.S

@@ -1,4 +1,5 @@
 #include <rsp_queue.inc>
+#include "test_rspq_constants.h"
 
 #define ASSERT_GP_BACKWARD           0xF001   // Also defined in test_rspq.c
 #define ASSERT_TOO_MANY_NOPS         0xF002
@@ -17,7 +18,7 @@
         RSPQ_DefineCommand command_reset,     4      # 0x05
         RSPQ_DefineCommand command_test_high, 4      # 0x06
         RSPQ_DefineCommand command_reset_log, 4      # 0x07
-        RSPQ_DefineCommand command_big,       132    # 0x08
+        RSPQ_DefineCommand command_big,       TEST_RSPQ_BIG_COMMAND_SIZE*4    # 0x08
         RSPQ_DefineCommand command_big_out,   8      # 0x09
         RSPQ_DefineCommand command_send_rdp,  8      # 0x0A
         RSPQ_DefineCommand command_send_rdp_many,  4 # 0x0B
@@ -40,7 +41,7 @@ BIG_LOG_PTR: .long 0
 BIG_LOG:  .ds.b BIG_LOG_SIZE
 
     .align 2
-TEST_BIG: .ds.b 128
+TEST_BIG: .ds.b TEST_RSPQ_BIG_PAYLOAD_SIZE
 
     .text
 
@@ -141,7 +142,7 @@ command_send_rdp_many:
 
 
 command_big:
-    addi s1, rspq_dmem_buf_ptr, -128
+    addi s1, rspq_dmem_buf_ptr, -TEST_RSPQ_BIG_PAYLOAD_SIZE
     move s2, zero
 command_big_loop:
     lw t0, %lo(RSPQ_DMEM_BUFFER)(s1)
@@ -158,6 +159,6 @@ command_big_out:
     move s0, a1
     li s4, %lo(TEST_BIG)
     j DMAOut
-    li t0, DMA_SIZE(128, 1)
+    li t0, DMA_SIZE(TEST_RSPQ_BIG_PAYLOAD_SIZE, 1)
 
 #include <rsp_rdpq.inc>

tests/test_rspq.c

@@ -5,6 +5,7 @@
 #include <rspq_constants.h>
 #include <rdp.h>
 #include <rdpq_constants.h>
+#include "test_rspq_constants.h"
 
 #define ASSERT_GP_BACKWARD           0xF001   // Also defined in rsp_test.S
 #define ASSERT_TOO_MANY_NOPS         0xF002
@@ -712,27 +713,27 @@ void test_rspq_big_command(TestContext *ctx)
     test_ovl_init();
     DEFER(test_ovl_close());
 
-    uint32_t values[32];
-    for (uint32_t i = 0; i < 32; i++)
+    uint32_t values[TEST_RSPQ_BIG_PAYLOAD_WORDS];
+    for (uint32_t i = 0; i < TEST_RSPQ_BIG_PAYLOAD_WORDS; i++)
     {
         values[i] = RANDN(0xFFFFFFFF);
     }
 
 
-    uint32_t output[32] __attribute__((aligned(16)));
-    data_cache_hit_writeback_invalidate(output, 128);
+    uint32_t output[TEST_RSPQ_BIG_PAYLOAD_WORDS] __attribute__((aligned(16)));
+    data_cache_hit_writeback_invalidate(output, TEST_RSPQ_BIG_PAYLOAD_SIZE);
 
-    rspq_write_t wptr = rspq_write_begin(test_ovl_id, 0x8, 33);
+    rspq_write_t wptr = rspq_write_begin(test_ovl_id, 0x8, TEST_RSPQ_BIG_COMMAND_SIZE);
     rspq_write_arg(&wptr, 0);
-    for (uint32_t i = 0; i < 32; i++)
+    for (uint32_t i = 0; i < TEST_RSPQ_BIG_PAYLOAD_WORDS; i++)
     {
         rspq_write_arg(&wptr, i | i << 8 | i << 16 | i << 24);
     }
     rspq_write_end(&wptr);
 
-    wptr = rspq_write_begin(test_ovl_id, 0x8, 33);
+    wptr = rspq_write_begin(test_ovl_id, 0x8, TEST_RSPQ_BIG_COMMAND_SIZE);
     rspq_write_arg(&wptr, 0);
-    for (uint32_t i = 0; i < 32; i++)
+    for (uint32_t i = 0; i < TEST_RSPQ_BIG_PAYLOAD_WORDS; i++)
     {
         rspq_write_arg(&wptr, values[i]);
     }
@@ -742,14 +743,14 @@ void test_rspq_big_command(TestContext *ctx)
 
     TEST_RSPQ_EPILOG(0, rspq_timeout);
 
-    uint32_t expected[32];
-    for (uint32_t i = 0; i < 32; i++)
+    uint32_t expected[TEST_RSPQ_BIG_PAYLOAD_WORDS];
+    for (uint32_t i = 0; i < TEST_RSPQ_BIG_PAYLOAD_WORDS; i++)
     {
         uint32_t x = i | i << 8 | i << 16 | i << 24;
         expected[i] = x ^ values[i];
     }
 
-    ASSERT_EQUAL_MEM((uint8_t*)output, (uint8_t*)expected, 128, "Output does not match!");
+    ASSERT_EQUAL_MEM((uint8_t*)output, (uint8_t*)expected, TEST_RSPQ_BIG_PAYLOAD_SIZE, "Output does not match!");
 }
 
 void test_rspq_rdp_dynamic(TestContext *ctx)

tests/test_rspq_constants.h

@@ -0,0 +1,8 @@
+#ifndef TEST_RSPQ_CONSTANTS
+#define TEST_RSPQ_CONSTANTS
+
+#define TEST_RSPQ_BIG_COMMAND_SIZE      62
+#define TEST_RSPQ_BIG_PAYLOAD_WORDS     (TEST_RSPQ_BIG_COMMAND_SIZE-1)
+#define TEST_RSPQ_BIG_PAYLOAD_SIZE      (TEST_RSPQ_BIG_PAYLOAD_WORDS*4)
+
+#endif