Message ID | 20220314041735.542867-15-npiggin@gmail.com |
---|---|
State | New |
Headers | show |
Series | gdbserver fixes and POWER10 support | expand |
On Mon, 14 Mar 2022 at 04:18, Nicholas Piggin <npiggin@gmail.com> wrote: > > Not all targets have memory access that can support arbitrary byte > writes. Use RMW for put_mem commands that are not 8-byte aligned. Do we know which targets can/can't support unaligned byte writes? > > This should really either be done in the target accessor, or at least > an alignment capability should be exposed to the caller. But for now > this will allow sbefifo mem ops to work to set breakpoints (4-byte > writes). > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > src/pdbgproxy.c | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) > > diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c > index a37e997..e8aab70 100644 > --- a/src/pdbgproxy.c > +++ b/src/pdbgproxy.c > @@ -14,6 +14,7 @@ > #include <assert.h> > #include <getopt.h> > #include <errno.h> > +#include <byteswap.h> > #include <ccan/array_size/array_size.h> > > #include <bitutils.h> > @@ -281,15 +282,15 @@ out: > static void put_mem(uint64_t *stack, void *priv) > { > uint64_t addr, len; > + uint64_t start_addr, end_addr; > + uint64_t align = 8; > + uint32_t *insn; > uint8_t *data; > + uint8_t *buf; > uint8_t attn_opcode[] = {0x00, 0x00, 0x02, 0x00}; > + uint8_t gdb_break_opcode[] = {0x7d, 0x82, 0x10, 0x08}; > int err = 0; > > - if (littleendian) { > - attn_opcode[1] = 0x02; > - attn_opcode[2] = 0x00; > - } > - > addr = stack[0]; > len = stack[1]; > data = (uint8_t *) &stack[2]; > @@ -301,8 +302,7 @@ static void put_mem(uint64_t *stack, void *priv) > goto out; > } > > - > - if (len == 4 && stack[2] == 0x0810827d) { > + if (len == 4 && !memcmp(data, gdb_break_opcode, 4)) { > /* According to linux-ppc-low.c gdb only uses this > * op-code for sw break points so we replace it with > * the correct attn opcode which is what we need for > @@ -318,17 +318,27 @@ static void put_mem(uint64_t *stack, void *priv) > err = 2; > goto out; > } > - } else { > - stack[2] = __builtin_bswap64(stack[2]) >> 32; > } > > - PR_INFO("put_mem 0x%016" PRIx64 " = 0x%016" PRIx64 "\n", addr, stack[2]); > + if (len == 4 && littleendian) { > + insn = (uint32_t *)data; > + *insn = bswap_32(*insn); > + } > > - if (mem_write(adu_target, addr, data, len, 0, false)) { > + start_addr = addr & ~(align - 1); > + end_addr = (addr + len + (align - 1)) & ~(align - 1); Would some ALIGN_UP / ALIGN_DOWN macros make sense here? > + if (addr != start_addr || (addr + len) != end_addr) { > + buf = malloc(end_addr - start_addr); > + mem_read(adu_target, start_addr, buf, end_addr - start_addr, 0, false); > + memcpy(buf + (addr - start_addr), data, len); > + } else { > + buf = data; > + } > + > + if (mem_write(adu_target, start_addr, buf, end_addr - start_addr, 0, false)) { > PR_ERROR("Unable to write memory\n"); > err = 3; > } Needs to free buf if it was malloc'd. > - > out: > if (err) > send_response(fd, ERROR(EPERM)); > -- > 2.23.0 > > -- > Pdbg mailing list > Pdbg@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/pdbg
Excerpts from Joel Stanley's message of March 16, 2022 9:46 am: > On Mon, 14 Mar 2022 at 04:18, Nicholas Piggin <npiggin@gmail.com> wrote: >> >> Not all targets have memory access that can support arbitrary byte >> writes. Use RMW for put_mem commands that are not 8-byte aligned. > > Do we know which targets can/can't support unaligned byte writes? sbefifo chip ops at least can only do 8 byte aligned / sized writes. >> @@ -318,17 +318,27 @@ static void put_mem(uint64_t *stack, void *priv) >> err = 2; >> goto out; >> } >> - } else { >> - stack[2] = __builtin_bswap64(stack[2]) >> 32; >> } >> >> - PR_INFO("put_mem 0x%016" PRIx64 " = 0x%016" PRIx64 "\n", addr, stack[2]); >> + if (len == 4 && littleendian) { >> + insn = (uint32_t *)data; >> + *insn = bswap_32(*insn); >> + } >> >> - if (mem_write(adu_target, addr, data, len, 0, false)) { >> + start_addr = addr & ~(align - 1); >> + end_addr = (addr + len + (align - 1)) & ~(align - 1); > > Would some ALIGN_UP / ALIGN_DOWN macros make sense here? Yeah that could improve things. > >> + if (addr != start_addr || (addr + len) != end_addr) { >> + buf = malloc(end_addr - start_addr); >> + mem_read(adu_target, start_addr, buf, end_addr - start_addr, 0, false); >> + memcpy(buf + (addr - start_addr), data, len); >> + } else { >> + buf = data; >> + } >> + >> + if (mem_write(adu_target, start_addr, buf, end_addr - start_addr, 0, false)) { >> PR_ERROR("Unable to write memory\n"); >> err = 3; >> } > > Needs to free buf if it was malloc'd. Good catch. Thanks, Nick
diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c index a37e997..e8aab70 100644 --- a/src/pdbgproxy.c +++ b/src/pdbgproxy.c @@ -14,6 +14,7 @@ #include <assert.h> #include <getopt.h> #include <errno.h> +#include <byteswap.h> #include <ccan/array_size/array_size.h> #include <bitutils.h> @@ -281,15 +282,15 @@ out: static void put_mem(uint64_t *stack, void *priv) { uint64_t addr, len; + uint64_t start_addr, end_addr; + uint64_t align = 8; + uint32_t *insn; uint8_t *data; + uint8_t *buf; uint8_t attn_opcode[] = {0x00, 0x00, 0x02, 0x00}; + uint8_t gdb_break_opcode[] = {0x7d, 0x82, 0x10, 0x08}; int err = 0; - if (littleendian) { - attn_opcode[1] = 0x02; - attn_opcode[2] = 0x00; - } - addr = stack[0]; len = stack[1]; data = (uint8_t *) &stack[2]; @@ -301,8 +302,7 @@ static void put_mem(uint64_t *stack, void *priv) goto out; } - - if (len == 4 && stack[2] == 0x0810827d) { + if (len == 4 && !memcmp(data, gdb_break_opcode, 4)) { /* According to linux-ppc-low.c gdb only uses this * op-code for sw break points so we replace it with * the correct attn opcode which is what we need for @@ -318,17 +318,27 @@ static void put_mem(uint64_t *stack, void *priv) err = 2; goto out; } - } else { - stack[2] = __builtin_bswap64(stack[2]) >> 32; } - PR_INFO("put_mem 0x%016" PRIx64 " = 0x%016" PRIx64 "\n", addr, stack[2]); + if (len == 4 && littleendian) { + insn = (uint32_t *)data; + *insn = bswap_32(*insn); + } - if (mem_write(adu_target, addr, data, len, 0, false)) { + start_addr = addr & ~(align - 1); + end_addr = (addr + len + (align - 1)) & ~(align - 1); + if (addr != start_addr || (addr + len) != end_addr) { + buf = malloc(end_addr - start_addr); + mem_read(adu_target, start_addr, buf, end_addr - start_addr, 0, false); + memcpy(buf + (addr - start_addr), data, len); + } else { + buf = data; + } + + if (mem_write(adu_target, start_addr, buf, end_addr - start_addr, 0, false)) { PR_ERROR("Unable to write memory\n"); err = 3; } - out: if (err) send_response(fd, ERROR(EPERM));
Not all targets have memory access that can support arbitrary byte writes. Use RMW for put_mem commands that are not 8-byte aligned. This should really either be done in the target accessor, or at least an alignment capability should be exposed to the caller. But for now this will allow sbefifo mem ops to work to set breakpoints (4-byte writes). Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- src/pdbgproxy.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)