From cd04a5f1aadc8e2ec4e787f5ba4cc8c38a579314 Mon Sep 17 00:00:00 2001 From: Marcin Bukat Date: Tue, 18 Nov 2014 23:27:26 +0100 Subject: 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 | 10 ++++- utils/hwstub/lib/hwstub.c | 40 +++++++++++++++---- utils/hwstub/lib/hwstub.h | 5 ++- utils/hwstub/stub/SOURCES | 2 + utils/hwstub/stub/asm/arm/atomic_rw.S | 58 +++++++++++++++++++++++++++ utils/hwstub/stub/asm/mips/atomic_rw.S | 61 ++++++++++++++++++++++++++++ utils/hwstub/stub/main.c | 72 +++++++++++++++++++++++++++++++++- utils/hwstub/stub/protocol.h | 2 +- utils/hwstub/stub/target.h | 8 ++++ utils/hwstub/tools/hwstub_shell.cpp | 12 +++--- utils/hwstub/tools/init.lua | 4 +- 11 files changed, 252 insertions(+), 22 deletions(-) create mode 100644 utils/hwstub/stub/asm/arm/atomic_rw.S create mode 100644 utils/hwstub/stub/asm/mips/atomic_rw.S (limited to 'utils/hwstub') diff --git a/utils/hwstub/hwstub_protocol.h b/utils/hwstub/hwstub_protocol.h index 33081d3ca2..1fe982323d 100644 --- a/utils/hwstub/hwstub_protocol.h +++ b/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 { diff --git a/utils/hwstub/lib/hwstub.c b/utils/hwstub/lib/hwstub.c index 6ae0400a66..d3908585da 100644 --- a/utils/hwstub/lib/hwstub.c +++ b/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 - - sizeof(struct hwstub_write_req_t))); + int xfer = _hwstub_write(dev, HWSTUB_WRITE, addr, buf, + 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; diff --git a/utils/hwstub/lib/hwstub.h b/utils/hwstub/lib/hwstub.h index d7d6ceb8ef..4d12de8eda 100644 --- a/utils/hwstub/lib/hwstub.h +++ b/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); diff --git a/utils/hwstub/stub/SOURCES b/utils/hwstub/stub/SOURCES index 1b0b56072d..c91580c966 100644 --- a/utils/hwstub/stub/SOURCES +++ b/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 diff --git a/utils/hwstub/stub/asm/arm/atomic_rw.S b/utils/hwstub/stub/asm/arm/atomic_rw.S new file mode 100644 index 0000000000..cb6272b4dc --- /dev/null +++ b/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 + diff --git a/utils/hwstub/stub/asm/mips/atomic_rw.S b/utils/hwstub/stub/asm/mips/atomic_rw.S new file mode 100644 index 0000000000..47c4213a5d --- /dev/null +++ b/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 diff --git a/utils/hwstub/stub/main.c b/utils/hwstub/stub/main.c index 4e54b8fd89..3604cfdae0 100644 --- a/utils/hwstub/stub/main.c +++ b/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,13 +420,30 @@ static void handle_read(struct usb_ctrlrequest *req) { if(id != last_id) return usb_drv_stall(EP_CONTROL, true, true); - memcpy(usb_buffer, (void *)last_addr, req->wLength); + 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); usb_drv_recv(EP_CONTROL, NULL, 0); } }; +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,7 +452,13 @@ 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); - memcpy((void *)write->dAddress, usb_buffer + sz_hdr, size - sz_hdr); + 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); diff --git a/utils/hwstub/stub/protocol.h b/utils/hwstub/stub/protocol.h index e358bb0a36..d551342d4b 100644 --- a/utils/hwstub/stub/protocol.h +++ b/utils/hwstub/stub/protocol.h @@ -1,3 +1,3 @@ #include "../hwstub_protocol.h" -#define HWSTUB_VERSION_REV 1 \ No newline at end of file +#define HWSTUB_VERSION_REV 0 \ No newline at end of file diff --git a/utils/hwstub/stub/target.h b/utils/hwstub/stub/target.h index cb17401a9c..5cd049d04f 100644 --- a/utils/hwstub/stub/target.h +++ b/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; diff --git a/utils/hwstub/tools/hwstub_shell.cpp b/utils/hwstub/tools/hwstub_shell.cpp index b2838ebed0..30d1ac3b3f 100644 --- a/utils/hwstub/tools/hwstub_shell.cpp +++ b/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); } diff --git a/utils/hwstub/tools/init.lua b/utils/hwstub/tools/init.lua index 1fe0e0d734..aaca8b6c82 100644 --- a/utils/hwstub/tools/init.lua +++ b/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("* write8/16/32(a, v) writes the 8/16/32-bit integer v 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 atomically"); h:add("* print_log() prints the device log"); h = HELP:create_topic("HW"); -- cgit v1.2.3