[13/14] gdbserver: use read-modify-write for put_mem that is not 8-byte aligned

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(-)

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