[Cryptech-Commits] [sw/stm32] 01/01: Change RPC UART to have a high-priority thread monitoring a large(ish) DMA buffer, because we've observed out-of-order receives under load.

[git at cryptech.is]

This is an automated email from the git hooks/post-receive script.

paul at psgd.org pushed a commit to branch uart_rx_thread
in repository sw/stm32.

commit 2b6b9f89cc83ee2412166045e839da61be976564
Author: Paul Selkirk <paul at psgd.org>
AuthorDate: Fri Mar 31 21:55:42 2017 -0400

    Change RPC UART to have a high-priority thread monitoring a large(ish) DMA
    buffer, because we've observed out-of-order receives under load.
---
 .../TARGET_CRYPTECH_ALPHA/stm32f4xx_it.c           |   6 ++
 projects/hsm/hsm.c                                 | 106 ++++++++++++---------
 projects/hsm/mgmt-thread.c                         |   3 +
 3 files changed, 69 insertions(+), 46 deletions(-)

diff --git a/libraries/mbed/targets/cmsis/TARGET_STM/TARGET_STM32F4/TARGET_CRYPTECH_ALPHA/stm32f4xx_it.c b/libraries/mbed/targets/cmsis/TARGET_STM/TARGET_STM32F4/TARGET_CRYPTECH_ALPHA/stm32f4xx_it.c
index f69e790..cbfff2d 100644
--- a/libraries/mbed/targets/cmsis/TARGET_STM/TARGET_STM32F4/TARGET_CRYPTECH_ALPHA/stm32f4xx_it.c
+++ b/libraries/mbed/targets/cmsis/TARGET_STM/TARGET_STM32F4/TARGET_CRYPTECH_ALPHA/stm32f4xx_it.c
@@ -253,4 +253,10 @@ __weak void HAL_UART2_RxHalfCpltCallback(UART_HandleTypeDef *huart)
    */
 }
 
+void HAL_UART_ErrorCallback(UART_HandleTypeDef *huart)
+{
+    /* I dunno, just trap it for now */
+    Error_Handler();
+}
+
 /************************ (C) COPYRIGHT STMicroelectronics *****END OF FILE****/
diff --git a/projects/hsm/hsm.c b/projects/hsm/hsm.c
index 60e35fc..c5af86d 100644
--- a/projects/hsm/hsm.c
+++ b/projects/hsm/hsm.c
@@ -117,59 +117,71 @@ void hal_ks_lock(void)   { osMutexWait(ks_mutex, osWaitForever); }
 void hal_ks_unlock(void) { osMutexRelease(ks_mutex); }
 #endif
 
-static uint8_t uart_rx[2];	/* current character received from UART */
-
-/* Callback for HAL_UART_Receive_DMA().
- * With multiple worker threads, we can't do a blocking receive, because
- * that prevents other threads from sending RPC responses (because they
- * both want to lock the UART - see stm32f4xx_hal_uart.c). So we have to
- * do a non-blocking receive with a callback routine.
- * Even with only one worker thread, context-switching to the CLI thread
- * causes HAL_UART_Receive to miss input.
+/* A ring buffer for the UART DMA receiver. In theory, it should get at most
+ * 92 characters per 1ms tick, but we're going to up-size it for safety.
  */
-static void RxCallback(uint8_t c)
-{
-    /* current RPC input buffer */
-    static rpc_buffer_t *ibuf = NULL;
-    int complete;
-
-    if (ibuf == NULL) {
-        if ((ibuf = (rpc_buffer_t *)osMailAlloc(ibuf_queue, 0)) == NULL)
-            /* This could happen if all dispatch threads are busy, and
-             * there are NUM_RPC_TASK requests already queued. We'd like
-             * to to send a "server busy" error, but we've just received
-             * the first byte of the request, so we don't yet have enough
-             * context to craft a response.
-             */
-            return;
-        ibuf->len = 0;
-    }
+#ifndef RPC_UART_RECVBUF_SIZE
+#define RPC_UART_RECVBUF_SIZE  256  /* must be a power of 2 */
+#endif
+#define RPC_UART_RECVBUF_MASK  (RPC_UART_RECVBUF_SIZE - 1)
 
-    if (hal_slip_process_char(c, ibuf->buf, &ibuf->len, sizeof(ibuf->buf), &complete) != LIBHAL_OK)
-        Error_Handler();
+typedef struct {
+    uint32_t ridx;
+    uint8_t buf[RPC_UART_RECVBUF_SIZE];
+} uart_ringbuf_t;
 
-    if (complete) {
-	if (osMailPut(ibuf_queue, (void *)ibuf) != osOK)
-            Error_Handler();
-        ibuf = NULL;
-    }
-}
+volatile uart_ringbuf_t uart_ringbuf = {0, {0}};
 
-void HAL_UART2_RxHalfCpltCallback(UART_HandleTypeDef *huart)
-{
-    RxCallback(uart_rx[0]);
-}
+#define RINGBUF_RIDX(rb)       (rb.ridx & RPC_UART_RECVBUF_MASK)
+#define RINGBUF_WIDX(rb)       (sizeof(rb.buf) - __HAL_DMA_GET_COUNTER(huart_user.hdmarx))
+#define RINGBUF_COUNT(rb)      ((unsigned)(RINGBUF_WIDX(rb) - RINGBUF_RIDX(rb)))
+#define RINGBUF_READ(rb, dst)  {dst = rb.buf[RINGBUF_RIDX(rb)]; rb.ridx++;}
 
-void HAL_UART2_RxCpltCallback(UART_HandleTypeDef *huart)
+/* Thread entry point for the UART DMA monitor.
+ */
+void uart_rx_thread(void const *args)
 {
-    RxCallback(uart_rx[1]);
-}
+    /* current RPC input buffer */
+    rpc_buffer_t *ibuf = NULL;
 
-void HAL_UART_ErrorCallback(UART_HandleTypeDef *huart)
-{
-    /* I dunno, just trap it for now */
-    Error_Handler();
+    /* I wanted to call osThreadYield(), but the documentation is misleading,
+     * and it only yields to the next ready thread of the same priority, so
+     * this high-priority thread wouldn't let anything else run. osDelay(1)
+     * reschedules this thread for the next tick, which is what we want.
+     */
+    for ( ; ; osDelay(1)) {
+        if (ibuf == NULL) {
+            if ((ibuf = (rpc_buffer_t *)osMailAlloc(ibuf_queue, 0)) == NULL)
+                /* This could happen if all dispatch threads are busy, and
+                 * there are NUM_RPC_TASK requests already queued. We could
+                 * send a "server busy" error, or we could just try again on
+                 * the next tick.
+                 */
+                continue;
+            ibuf->len = 0;
+        }
+
+        while (RINGBUF_COUNT(uart_ringbuf)) {
+            uint8_t c;
+            int complete;
+
+            RINGBUF_READ(uart_ringbuf, c);
+            if (hal_slip_process_char(c, ibuf->buf, &ibuf->len, sizeof(ibuf->buf), &complete) != LIBHAL_OK)
+                Error_Handler();
+
+            if (complete) {
+                if (osMailPut(ibuf_queue, (void *)ibuf) != osOK)
+                    Error_Handler();
+                ibuf = NULL;
+                /* Yield, to allow one of the dispatch threads to pick up this
+                 * new request.
+                 */
+                break;
+            }
+        }
+    }
 }
+osThreadDef(uart_rx_thread, osPriorityHigh, DEFAULT_STACK_SIZE);
 
 hal_error_t hal_serial_send_char(uint8_t c)
 {
@@ -297,7 +309,9 @@ int main()
     }
 
     /* Start the UART receiver. */
-    if (HAL_UART_Receive_DMA(&huart_user, uart_rx, 2) != CMSIS_HAL_OK)
+    if (HAL_UART_Receive_DMA(&huart_user, (uint8_t *) uart_ringbuf.buf, sizeof(uart_ringbuf.buf)) != CMSIS_HAL_OK)
+        Error_Handler();
+    if (osThreadCreate(osThread(uart_rx_thread), NULL) == NULL)
         Error_Handler();
 
     /* Launch other threads (csprng warm-up thread?)
diff --git a/projects/hsm/mgmt-thread.c b/projects/hsm/mgmt-thread.c
index 82b8e72..72841b7 100644
--- a/projects/hsm/mgmt-thread.c
+++ b/projects/hsm/mgmt-thread.c
@@ -67,6 +67,7 @@ static int cmd_thread_show(struct cli_def *cli, const char *command, char *argv[
     extern void main(void);
     extern void dispatch_thread(void);
     extern void osTimerThread(void);
+    extern void uart_rx_thread(void);
 
     for (task_id = 1; task_id <= os_maxtaskrun; ++ task_id) {
         if ((task = os_active_TCB[task_id-1]) != NULL) {
@@ -76,6 +77,8 @@ static int cmd_thread_show(struct cli_def *cli, const char *command, char *argv[
                 name = "dispatch_thread";
             else if (task->ptask == osTimerThread)
                 name = "osTimerThread";
+            else if (task->ptask == uart_rx_thread)
+                name = "uart_rx_thread";
             else
                 name = "unknown";