hwstub/qeditor: add support for atomic read/writes

The current code assumed that READ/WRITE would produce atomic read/writes for
8/16/32-bit words, which in turned put assumption on the memcpy function.
Since some memcpy implementation do not always guarantee such strong assumption,
introduce two new operation READ/WRITE_ATOMIC which provide the necessary
tools to do correct read and write to register in a single memory access.

Change-Id: I37451bd5057bb0dcaf5a800d8aef8791c792a090

utils/hwstub/hwstub_protocol.h

@@ -26,7 +26,7 @@
  */
 
 #define HWSTUB_VERSION_MAJOR    4
-#define HWSTUB_VERSION_MINOR    0
+#define HWSTUB_VERSION_MINOR    1
 
 #define HWSTUB_VERSION__(maj, min) #maj"."#min
 #define HWSTUB_VERSION_(maj, min) HWSTUB_VERSION__(maj, min)
@@ -140,6 +140,8 @@ struct hwstub_device_desc_t
 #define HWSTUB_READ2    0x42
 #define HWSTUB_WRITE    0x43
 #define HWSTUB_EXEC     0x44
+#define HWSTUB_READ2_ATOMIC 0x45
+#define HWSTUB_WRITE_ATOMIC 0x46
 
 /**
  * HWSTUB_GET_LOG:
@@ -147,11 +149,14 @@ struct hwstub_device_desc_t
  */
 
 /**
- * HWSTUB_READ and HWSTUB_READ2:
+ * HWSTUB_READ and HWSTUB_READ2(_ATOMIC):
  * Read a range of memory. The request works in two steps: first the host
  * sends HWSTUB_READ with the parameters (address, length) and then
  * a HWSTUB_READ2 to retrieve the buffer. Both requests must use the same
  * ID in wValue, otherwise the second request will be STALLed.
+ * HWSTUB_READ2_ATOMIC behaves the same as HWSTUB_READ2 except that the read
+ * is guaranteed to be atomic (ie performed as a single memory access) and
+ * will be STALLed if atomicity can not be ensured.
  */
 
 struct hwstub_read_req_t
@@ -163,6 +168,7 @@ struct hwstub_read_req_t
  * HWSTUB_WRITE
  * Write a range of memory. The payload starts with the following header, everything
  * which follows is data.
+ * HWSTUB_WRITE_ATOMIC behaves the same except it is atomic. See HWSTUB_READ2_ATOMIC.
  */
 struct hwstub_write_req_t
 {

utils/hwstub/lib/hwstub.c

@@ -140,7 +140,8 @@ int hwstub_get_log(struct hwstub_device_t *dev, void *buf, size_t sz)
         HWSTUB_GET_LOG, 0, dev->intf, buf, sz, 1000);
 }
 
-static int _hwstub_read(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz)
+static int _hwstub_read(struct hwstub_device_t *dev, uint8_t breq, uint32_t addr,
+    void *buf, size_t sz)
 {
     struct hwstub_read_req_t read;
     read.dAddress = addr;
@@ -151,10 +152,11 @@ static int _hwstub_read(struct hwstub_device_t *dev, uint32_t addr, void *buf, s
         return -1;
     return libusb_control_transfer(dev->handle,
         LIBUSB_REQUEST_TYPE_CLASS | LIBUSB_RECIPIENT_INTERFACE | LIBUSB_ENDPOINT_IN,
-        HWSTUB_READ2, dev->id++, dev->intf, buf, sz, 1000);
+        breq, dev->id++, dev->intf, buf, sz, 1000);
 }
 
-static int _hwstub_write(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz)
+static int _hwstub_write(struct hwstub_device_t *dev, uint8_t breq, uint32_t addr,
+    const void *buf, size_t sz)
 {
     size_t hdr_sz = sizeof(struct hwstub_write_req_t);
     struct hwstub_write_req_t *req = malloc(sz + hdr_sz);
@@ -162,18 +164,34 @@ static int _hwstub_write(struct hwstub_device_t *dev, uint32_t addr, void *buf,
     memcpy(req + 1, buf, sz);
     int size = libusb_control_transfer(dev->handle,
         LIBUSB_REQUEST_TYPE_CLASS | LIBUSB_RECIPIENT_INTERFACE | LIBUSB_ENDPOINT_OUT,
-        HWSTUB_WRITE, dev->id++, dev->intf, (void *)req, sz + hdr_sz, 1000);
+        breq, dev->id++, dev->intf, (void *)req, sz + hdr_sz, 1000);
     free(req);
     return size - hdr_sz;
 }
 
+int hwstub_read_atomic(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz)
+{
+    /* reject any read greater than the buffer, it makes no sense anyway */
+    if(sz > dev->buf_sz)
+        return -1;
+    return _hwstub_read(dev, HWSTUB_READ2_ATOMIC, addr, buf, sz);
+}
+
+int hwstub_write_atomic(struct hwstub_device_t *dev, uint32_t addr, const void *buf, size_t sz)
+{
+    /* reject any write greater than the buffer, it makes no sense anyway */
+    if(sz + sizeof(struct hwstub_write_req_t) > dev->buf_sz)
+        return -1;
+    return _hwstub_write(dev, HWSTUB_WRITE_ATOMIC, addr, buf, sz);
+}
+
 /* Intermediate function which make sure we don't overflow the device buffer */
 int hwstub_read(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz)
 {
     int cnt = 0;
     while(sz > 0)
     {
-        int xfer = _hwstub_read(dev, addr, buf, MIN(sz, dev->buf_sz));
+        int xfer = _hwstub_read(dev, HWSTUB_READ2, addr, buf, MIN(sz, dev->buf_sz));
         if(xfer <  0)
             return xfer;
         sz -= xfer;
@@ -185,13 +203,13 @@ int hwstub_read(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz
 }
 
 /* Intermediate function which make sure we don't overflow the device buffer */
-int hwstub_write(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz)
+int hwstub_write(struct hwstub_device_t *dev, uint32_t addr, const void *buf, size_t sz)
 {
     int cnt = 0;
     while(sz > 0)
     {
-        int xfer = _hwstub_write(dev, addr, buf, MIN(sz, dev->buf_sz -
+        int xfer = _hwstub_write(dev, HWSTUB_WRITE, addr, buf, 
-            sizeof(struct hwstub_write_req_t)));
+            MIN(sz, dev->buf_sz - sizeof(struct hwstub_write_req_t)));
         if(xfer <  0)
             return xfer;
         sz -= xfer;
@@ -207,6 +225,12 @@ int hwstub_rw_mem(struct hwstub_device_t *dev, int read, uint32_t addr, void *bu
     return read ? hwstub_read(dev, addr, buf, sz) : hwstub_write(dev, addr, buf, sz);
 }
 
+int hwstub_rw_mem_atomic(struct hwstub_device_t *dev, int read, uint32_t addr, void *buf, size_t sz)
+{
+    return read ? hwstub_read_atomic(dev, addr, buf, sz) :
+        hwstub_write_atomic(dev, addr, buf, sz);
+}
+
 int hwstub_exec(struct hwstub_device_t *dev, uint32_t addr, uint16_t flags)
 {
     struct hwstub_exec_req_t exec;

utils/hwstub/lib/hwstub.h

@@ -53,8 +53,11 @@ int hwstub_get_desc(struct hwstub_device_t *dev, uint16_t desc, void *info, size
 int hwstub_get_log(struct hwstub_device_t *dev, void *buf, size_t sz);
 /* Returns number of bytes written/read or <0 on error */
 int hwstub_read(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz);
-int hwstub_write(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz);
+int hwstub_read_atomic(struct hwstub_device_t *dev, uint32_t addr, void *buf, size_t sz);
+int hwstub_write(struct hwstub_device_t *dev, uint32_t addr, const void *buf, size_t sz);
+int hwstub_write_atomic(struct hwstub_device_t *dev, uint32_t addr, const void *buf, size_t sz);
 int hwstub_rw_mem(struct hwstub_device_t *dev, int read, uint32_t addr, void *buf, size_t sz);
+int hwstub_rw_mem_atomic(struct hwstub_device_t *dev, int read, uint32_t addr, void *buf, size_t sz);
 /* Returns <0 on error */
 int hwstub_exec(struct hwstub_device_t *dev, uint32_t addr, uint16_t flags);
 int hwstub_call(struct hwstub_device_t *dev, uint32_t addr);

utils/hwstub/stub/SOURCES

@@ -2,9 +2,11 @@
 asm/arm/memcpy.S
 asm/arm/memmove.S
 asm/arm/memset.S
+asm/arm/atomic_rw.S
 #elif defined(CPU_MIPS)
 asm/mips/memcpy.S
 asm/mips/memset.S
+asm/mips/atomic_rw.S
 #else
 #error "Unimplemented ISA"
 #endif

utils/hwstub/stub/asm/arm/atomic_rw.S

@@ -0,0 +1,58 @@
+/***************************************************************************
+ *             __________               __   ___.
+ *   Open      \______   \ ____   ____ |  | _\_ |__   _______  ___
+ *   Source     |       _//  _ \_/ ___\|  |/ /| __ \ /  _ \  \/  /
+ *   Jukebox    |    |   (  <_> )  \___|    < | \_\ (  <_> > <  <
+ *   Firmware   |____|_  /\____/ \___  >__|_ \|___  /\____/__/\_ \
+ *                     \/            \/     \/    \/            \/
+ *
+ * Copyright (C) 2014 by Marcin Bukat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
+ * KIND, either express or implied.
+ *
+ ****************************************************************************/
+
+    .section   .text, "ax", %progbits
+    .global    target_read8
+    .type      target_read8, %function
+    .global    target_read16
+    .type      target_read16, %function
+    .global    target_read32
+    .type      target_read32, %function
+    .global    target_write8
+    .type      target_write8, %function
+    .global    target_write16
+    .type      target_write16, %function
+    .global    target_write32
+    .type      target_write32, %function
+
+target_read8:
+    ldrb r0, [r0]
+    bx lr
+
+target_read16:
+    ldrh r0, [r0]
+    bx lr
+
+target_read32:
+    ldr r0, [r0]
+    bx lr
+
+target_write8:
+    strb r1, [r0]
+    bx lr
+
+target_write16:
+    strh r1, [r0]
+    bx lr
+
+target_write32:
+    str r1, [r0]
+    bx lr
+

utils/hwstub/stub/asm/mips/atomic_rw.S

@@ -0,0 +1,61 @@
+/***************************************************************************
+ *             __________               __   ___.
+ *   Open      \______   \ ____   ____ |  | _\_ |__   _______  ___
+ *   Source     |       _//  _ \_/ ___\|  |/ /| __ \ /  _ \  \/  /
+ *   Jukebox    |    |   (  <_> )  \___|    < | \_\ (  <_> > <  <
+ *   Firmware   |____|_  /\____/ \___  >__|_ \|___  /\____/__/\_ \
+ *                     \/            \/     \/    \/            \/
+ *
+ * Copyright (C) 2014 by Marcin Bukat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
+ * KIND, either express or implied.
+ *
+ ****************************************************************************/
+#include "mips.h"
+
+    .set noreorder
+    .section   .text, "ax", %progbits
+    .global    target_read8
+    .type      target_read8, %function
+    .global    target_read16
+    .type      target_read16, %function
+    .global    target_read32
+    .type      target_read32, %function
+    .global    target_write8
+    .type      target_write8, %function
+    .global    target_write16
+    .type      target_write16, %function
+    .global    target_write32
+    .type      target_write32, %function
+
+target_read8:
+    jr ra
+    lbu v0, 0(a0)
+
+target_read16:
+    jr ra
+    lhu v0, 0(a0)
+
+target_read32:
+    jr ra
+    lw v0, 0(a0)
+
+target_write8:
+    jr ra
+    sb a1, 0(a0)
+
+target_write16:
+    jr ra
+    sh a1, 0(a0)
+
+target_write32:
+    jr ra
+    sw a1, 0(a0)
+
+    .set reorder

utils/hwstub/stub/main.c

@@ -356,6 +356,49 @@ static void handle_get_log(struct usb_ctrlrequest *req)
     enable_logf(true);
 }
 
+/* default implementation, relying on the compiler to produce correct code,
+ * targets should reimplement this... */
+uint8_t __attribute__((weak)) target_read8(const void *addr)
+{
+    return *(volatile uint8_t *)addr;
+}
+
+uint16_t __attribute__((weak)) target_read16(const void *addr)
+{
+    return *(volatile uint16_t *)addr;
+}
+
+uint32_t __attribute__((weak)) target_read32(const void *addr)
+{
+    return *(volatile uint32_t *)addr;
+}
+
+void __attribute__((weak)) target_write8(void *addr, uint8_t val)
+{
+    *(volatile uint8_t *)addr = val;
+}
+
+void __attribute__((weak)) target_write16(void *addr, uint16_t val)
+{
+    *(volatile uint16_t *)addr = val;
+}
+
+void __attribute__((weak)) target_write32(void *addr, uint32_t val)
+{
+    *(volatile uint32_t *)addr = val;
+}
+
+static bool read_atomic(void *dst, void *src, size_t sz)
+{
+    switch(sz)
+    {
+        case 1: *(uint8_t *)dst = target_read8(src); return true;
+        case 2: *(uint16_t *)dst = target_read16(src); return true;
+        case 4: *(uint32_t *)dst = target_read32(src); return true;
+        default: return false;
+    }
+}
+
 static void handle_read(struct usb_ctrlrequest *req)
 {
     static uint32_t last_addr = 0;
@@ -377,6 +420,12 @@ static void handle_read(struct usb_ctrlrequest *req)
     {
         if(id != last_id)
             return usb_drv_stall(EP_CONTROL, true, true);
+        if(req->bRequest == HWSTUB_READ2_ATOMIC)
+        {
+            if(!read_atomic(usb_buffer, (void *)last_addr, req->wLength))
+                return usb_drv_stall(EP_CONTROL, true, true);
+        }
+        else
             memcpy(usb_buffer, (void *)last_addr, req->wLength);
         asm volatile("nop" : : : "memory");
         usb_drv_send(EP_CONTROL, usb_buffer, req->wLength);
@@ -384,6 +433,17 @@ static void handle_read(struct usb_ctrlrequest *req)
     }
 };
 
+static bool write_atomic(void *dst, void *src, size_t sz)
+{
+    switch(sz)
+    {
+        case 1: target_write8(dst, *(uint8_t *)src); return true;
+        case 2: target_write16(dst, *(uint16_t *)src); return true;
+        case 4: target_write32(dst, *(uint32_t *)src); return true;
+        default: return false;
+    }
+}
+
 static void handle_write(struct usb_ctrlrequest *req)
 {
     int size = usb_drv_recv(EP_CONTROL, usb_buffer, req->wLength);
@@ -392,6 +452,12 @@ static void handle_write(struct usb_ctrlrequest *req)
     int sz_hdr = sizeof(struct hwstub_write_req_t);
     if(size < sz_hdr)
         return usb_drv_stall(EP_CONTROL, true, true);
+    if(req->bRequest == HWSTUB_WRITE_ATOMIC)
+    {
+        if(!write_atomic((void *)write->dAddress, usb_buffer + sz_hdr, size - sz_hdr))
+            return usb_drv_stall(EP_CONTROL, true, true);
+    }
+    else
         memcpy((void *)write->dAddress, usb_buffer + sz_hdr, size - sz_hdr);
     usb_drv_send(EP_CONTROL, NULL, 0);
 }
@@ -451,8 +517,10 @@ static void handle_class_intf_req(struct usb_ctrlrequest *req)
             return handle_get_log(req);
         case HWSTUB_READ:
         case HWSTUB_READ2:
+        case HWSTUB_READ2_ATOMIC:
             return handle_read(req);
         case HWSTUB_WRITE:
+        case HWSTUB_WRITE_ATOMIC:
             return handle_write(req);
         case HWSTUB_EXEC:
             return handle_exec(req);

utils/hwstub/stub/protocol.h

@@ -1,3 +1,3 @@
 #include "../hwstub_protocol.h"
 
-#define HWSTUB_VERSION_REV  1
+#define HWSTUB_VERSION_REV  0

utils/hwstub/stub/target.h

@@ -33,6 +33,14 @@ void target_get_config_desc(void *buffer, int *size);
 void target_udelay(int us);
 /* Wait for a short time (ms <= 1000) */
 void target_mdelay(int ms);
+/* Read a n-bit word atomically */
+uint8_t target_read8(const void *addr);
+uint16_t target_read16(const void *addr);
+uint32_t target_read32(const void *addr);
+/* Write a n-bit word atomically */
+void target_write8(void *addr, uint8_t val);
+void target_write16(void *addr, uint16_t val);
+void target_write32(void *addr, uint32_t val);
 
 /* mandatory for all targets */
 extern struct hwstub_target_desc_t target_descriptor;

utils/hwstub/tools/hwstub_shell.cpp

@@ -144,7 +144,7 @@ typedef void (*hw_writen_fn_t)(lua_State *state, soc_addr_t addr, soc_word_t val
 soc_word_t hw_read8(lua_State *state, soc_addr_t addr)
 {
     uint8_t u;
-    if(hwstub_rw_mem(g_hwdev, 1, addr, &u, sizeof(u)) != sizeof(u))
+    if(hwstub_rw_mem_atomic(g_hwdev, 1, addr, &u, sizeof(u)) != sizeof(u))
         luaL_error(state, "fail to read8 @ %p", addr);
     return u;
 }
@@ -152,7 +152,7 @@ soc_word_t hw_read8(lua_State *state, soc_addr_t addr)
 soc_word_t hw_read16(lua_State *state, soc_addr_t addr)
 {
     uint16_t u;
-    if(hwstub_rw_mem(g_hwdev, 1, addr, &u, sizeof(u)) != sizeof(u))
+    if(hwstub_rw_mem_atomic(g_hwdev, 1, addr, &u, sizeof(u)) != sizeof(u))
         luaL_error(state, "fail to read16 @ %p", addr);
     return u;
 }
@@ -160,7 +160,7 @@ soc_word_t hw_read16(lua_State *state, soc_addr_t addr)
 soc_word_t hw_read32(lua_State *state, soc_addr_t addr)
 {
     uint32_t u;
-    if(hwstub_rw_mem(g_hwdev, 1, addr, &u, sizeof(u)) != sizeof(u))
+    if(hwstub_rw_mem_atomic(g_hwdev, 1, addr, &u, sizeof(u)) != sizeof(u))
         luaL_error(state, "fail to read32 @ %p", addr);
     return u;
 }
@@ -168,21 +168,21 @@ soc_word_t hw_read32(lua_State *state, soc_addr_t addr)
 void hw_write8(lua_State *state, soc_addr_t addr, soc_word_t val)
 {
     uint8_t u = val;
-    if(hwstub_rw_mem(g_hwdev, 0, addr, &u, sizeof(u)) != sizeof(u))
+    if(hwstub_rw_mem_atomic(g_hwdev, 0, addr, &u, sizeof(u)) != sizeof(u))
         luaL_error(state, "fail to write8 @ %p", addr);
 }
 
 void hw_write16(lua_State *state, soc_addr_t addr, soc_word_t val)
 {
     uint16_t u = val;
-    if(hwstub_rw_mem(g_hwdev, 0, addr, &u, sizeof(u)) != sizeof(u))
+    if(hwstub_rw_mem_atomic(g_hwdev, 0, addr, &u, sizeof(u)) != sizeof(u))
         luaL_error(state, "fail to write16 @ %p", addr);
 }
 
 void hw_write32(lua_State *state, soc_addr_t addr, soc_word_t val)
 {
     uint32_t u = val;
-    if(hwstub_rw_mem(g_hwdev, 0, addr, &u, sizeof(u)) != sizeof(u))
+    if(hwstub_rw_mem_atomic(g_hwdev, 0, addr, &u, sizeof(u)) != sizeof(u))
         luaL_error(state, "fail to write32 @ %p", addr);
 }
 

utils/hwstub/tools/init.lua

@@ -38,8 +38,8 @@ do
     h = HELP:create_topic("DEV");
     h:add("This variable redirects to hwstub.dev and provides direct access to the device.");
     h:add("It contains some information about the device and the following methods.");
-    h:add("* read8/16/32(a) reads a 8/16/32-bit integer at address a");
+    h:add("* read8/16/32(a) reads a 8/16/32-bit integer at address a atomically");
-    h:add("* write8/16/32(a, v) writes the 8/16/32-bit integer v at address a");
+    h:add("* write8/16/32(a, v) writes the 8/16/32-bit integer v at address a atomically");
     h:add("* print_log() prints the device log");
 
     h = HELP:create_topic("HW");

utils/regtools/qeditor/backend.cpp

@@ -266,7 +266,7 @@ bool HWStubDevice::ReadMem(soc_addr_t addr, size_t length, void *buffer)
 {
     if(!m_hwdev)
         return false;
-    int ret = hwstub_rw_mem(m_hwdev, 1, addr, buffer, length);
+    int ret = hwstub_rw_mem_atomic(m_hwdev, 1, addr, buffer, length);
     return ret >= 0 && (size_t)ret == length;
 }
 
@@ -274,7 +274,7 @@ bool HWStubDevice::WriteMem(soc_addr_t addr, size_t length, void *buffer)
 {
     if(!m_hwdev)
         return false;
-    int ret = hwstub_rw_mem(m_hwdev, 0, addr, buffer, length);
+    int ret = hwstub_rw_mem_atomic(m_hwdev, 0, addr, buffer, length);
     return ret >= 0 && (size_t)ret == length;
 }