| Message ID | 20181210004954.15242-2-alistair@popple.id.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] optcmd: Add parser for 8-bit power of 2 integers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | master/apply_patch Patch failed to apply |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On Mon, 2018-12-10 at 11:49 +1100, Alistair Popple wrote: > Not all memory can be read with the default ADU block size of 8 > bytes. Specifically cache-inhibited access to some MMIO regions such > as PCIe BAR spaces requires 4 byte accesses to avoid check stopping > the machine. > > This patch adds an argument to the put/getmem commands to allow a > specific block size to be selected. Are you missing some more bits in the patch? $ git am \[Pdbg\]_\[PATCH_2_2\]_adu\:_Add_arugments_for_block_size.mbox Applying: adu: Add arugments for block size error: patch failed: src/pdbgproxy.c:233 error: src/pdbgproxy.c: patch does not apply Patch failed at 0001 adu: Add arugments for block size hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > --- > libpdbg/adu.c | 124 +++++++++++++++++++++++++++++++++++++------- > ---------- > libpdbg/libpdbg.h | 18 +++++--- > libpdbg/target.h | 4 +- > src/mem.c | 14 +++--- > src/pdbgproxy.c | 4 +- > src/thread.c | 2 +- > 6 files changed, 111 insertions(+), 55 deletions(-) > > diff --git a/libpdbg/adu.c b/libpdbg/adu.c > index b67a43e..3ab0d31 100644 > --- a/libpdbg/adu.c > +++ b/libpdbg/adu.c > @@ -84,20 +84,48 @@ > #define FBC_ALTD_DATA_DONE PPC_BIT(3) > #define FBC_ALTD_PBINIT_MISSING PPC_BIT(18) > > +/* There are more general implementations of this with a loop and > more > + * performant implementations using GCC builtins which aren't > + * portable. Given we only need a limited domain this is quick, easy > + * and portable. */ > +uint8_t blog2(uint8_t x) > +{ > + switch(x) { > + case 1: > + return 0; > + case 2: > + return 1; > + case 4: > + return 2; > + case 8: > + return 3; > + case 16: > + return 4; > + case 32: > + return 5; > + case 64: > + return 6; > + default: > + assert(0); > + } > +} > + > int adu_getmem(struct pdbg_target *adu_target, uint64_t start_addr, > - uint8_t *output, uint64_t size) > + uint8_t *output, uint64_t size, uint8_t block_size) > { > - return __adu_getmem(adu_target, start_addr, output, size, > false); > + return __adu_getmem(adu_target, start_addr, output, > + size, false, block_size); > } > > int adu_getmem_ci(struct pdbg_target *adu_target, uint64_t > start_addr, > - uint8_t *output, uint64_t size) > + uint8_t *output, uint64_t size, uint8_t block_size) > { > - return __adu_getmem(adu_target, start_addr, output, size, > true); > + return __adu_getmem(adu_target, start_addr, output, > + size, true, block_size); > } > > int __adu_getmem(struct pdbg_target *adu_target, uint64_t > start_addr, > - uint8_t *output, uint64_t size, bool ci) > + uint8_t *output, uint64_t size, bool ci, uint8_t > block_size) > { > struct adu *adu; > uint8_t *output0; > @@ -110,32 +138,33 @@ int __adu_getmem(struct pdbg_target > *adu_target, uint64_t start_addr, > output0 = output; > > /* Align start address to 8-byte boundary */ > - addr0 = 8 * (start_addr / 8); > + addr0 = block_size * (start_addr / block_size); The comment is now wrong. > > /* We read data in 8-byte aligned chunks */ > - for (addr = addr0; addr < start_addr + size; addr += 8) { > + for (addr = addr0; addr < start_addr + size; addr += > block_size) { The comment is now wrong. > uint64_t data; > > - if (adu->getmem(adu, addr, &data, ci)) > + if (adu->getmem(adu, addr, &data, ci, block_size)) > return -1; > > - /* ADU returns data in big-endian form in the register > */ > + /* ADU returns data in big-endian form in the register. > */ > data = __builtin_bswap64(data); > + data >>= (addr & 0x7ull)*8; > > if (addr < start_addr) { > size_t offset = start_addr - addr; > - size_t n = (size <= 8-offset ? size : 8- > offset); > + size_t n = (size <= block_size-offset ? size : > block_size-offset); > > memcpy(output, ((uint8_t *) &data) + offset, > n); > output += n; > - } else if (addr + 8 > start_addr + size) { > + } else if (addr + block_size > start_addr + size) { > uint64_t offset = start_addr + size - addr; > > memcpy(output, &data, offset); > output += offset; > } else { > - memcpy(output, &data, 8); > - output += 8; > + memcpy(output, &data, block_size); > + output += block_size; > } > > pdbg_progress_tick(output - output0, size); > @@ -147,19 +176,19 @@ int __adu_getmem(struct pdbg_target > *adu_target, uint64_t start_addr, > } > > int adu_putmem(struct pdbg_target *adu_target, uint64_t start_addr, > - uint8_t *output, uint64_t size) > + uint8_t *output, uint64_t size, uint8_t block_size) > { > - return __adu_putmem(adu_target, start_addr, output, size, > false); > + return __adu_putmem(adu_target, start_addr, output, size, > false, block_size); > } > > int adu_putmem_ci(struct pdbg_target *adu_target, uint64_t > start_addr, > - uint8_t *output, uint64_t size) > + uint8_t *output, uint64_t size, uint8_t block_size) > { > - return __adu_putmem(adu_target, start_addr, output, size, > true); > + return __adu_putmem(adu_target, start_addr, output, size, true, > block_size); > } > > int __adu_putmem(struct pdbg_target *adu_target, uint64_t > start_addr, > - uint8_t *input, uint64_t size, bool ci) > + uint8_t *input, uint64_t size, bool ci, uint8_t > block_size) > { > struct adu *adu; > int rc = 0, tsize; > @@ -169,7 +198,7 @@ int __adu_putmem(struct pdbg_target *adu_target, > uint64_t start_addr, > adu = target_to_adu(adu_target); > end_addr = start_addr + size; > for (addr = start_addr; addr < end_addr; addr += tsize, input > += tsize) { > - if ((addr % 8) || (addr + 8 > end_addr)) { > + if ((addr % block_size) || (addr + block_size > > end_addr)) { > /* If the address is not 64-bit aligned we > * copy in a byte at a time until it is. */ The above comment is now wrong. When handling non-aligned addresses, we are putting one byte at a time. Does that need to be aligned to block-size? If we still need to do block-size alignment, then the following code is wrong. > tsize = 1; > @@ -177,12 +206,13 @@ int __adu_putmem(struct pdbg_target > *adu_target, uint64_t start_addr, > /* Copy the input data in with correct > alignment */ > data = ((uint64_t) *input) << 8*(8 - (addr % 8) > - 1); The above code is still 8-byte aligned, not block-size aligned. > } else { > - tsize = 8; > - memcpy(&data, input, sizeof(data)); > + tsize = block_size; > + memcpy(&data, input, block_size); > data = __builtin_bswap64(data); > + data >>= (addr & 7ull)*8; > } > > - adu->putmem(adu, addr, data, tsize, ci); > + adu->putmem(adu, addr, data, tsize, ci, block_size); > pdbg_progress_tick(addr - start_addr, size); > } > > @@ -234,7 +264,8 @@ static int adu_reset(struct adu *adu) > return 0; > } > > -static int p8_adu_getmem(struct adu *adu, uint64_t addr, uint64_t > *data, int ci) > +static int p8_adu_getmem(struct adu *adu, uint64_t addr, uint64_t > *data, > + int ci, uint8_t block_size) > { > uint64_t ctrl_reg, cmd_reg, val; > int rc = 0; > @@ -242,12 +273,15 @@ static int p8_adu_getmem(struct adu *adu, > uint64_t addr, uint64_t *data, int ci) > CHECK_ERR(adu_lock(adu)); > > ctrl_reg = P8_TTYPE_TREAD; > - if (ci) > + if (ci) { > /* Do cache inhibited access */ > ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, > P8_TTYPE_CI_PARTIAL_READ); > - else > + block_size = (blog2(block_size) + 1) << 1; > + } else { > ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, > P8_TTYPE_DMA_PARTIAL_READ); > - ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, 8); > + block_size = 0; > + } > + ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, block_size); > > CHECK_ERR_GOTO(out, rc = pib_read(&adu->target, > P8_ALTD_CMD_REG, &cmd_reg)); > cmd_reg |= FBC_ALTD_START_OP; > @@ -292,19 +326,23 @@ out: > > } > > -int p8_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int > size, int ci) > +int p8_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int > size, > + int ci, uint8_t block_size) > { > int rc = 0; > uint64_t cmd_reg, ctrl_reg, val; > CHECK_ERR(adu_lock(adu)); > > ctrl_reg = P8_TTYPE_TWRITE; > - if (ci) > + if (ci) { > /* Do cache inhibited access */ > ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, > P8_TTYPE_CI_PARTIAL_WRITE); > - else > + block_size = (blog2(block_size) + 1) << 1; > + } else { > ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, > P8_TTYPE_DMA_PARTIAL_WRITE); > - ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, size); > + block_size <<= 1; > + } > + ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, block_size); > > CHECK_ERR_GOTO(out, rc = pib_read(&adu->target, > P8_ALTD_CMD_REG, &cmd_reg)); > cmd_reg |= FBC_ALTD_START_OP; > @@ -349,19 +387,23 @@ out: > return rc; > } > > -static int p9_adu_getmem(struct adu *adu, uint64_t addr, uint64_t > *data, int ci) > +static int p9_adu_getmem(struct adu *adu, uint64_t addr, uint64_t > *data, > + int ci, uint8_t block_size) > { > uint64_t ctrl_reg, cmd_reg, val; > > cmd_reg = P9_TTYPE_TREAD; > - if (ci) > + if (ci) { > /* Do cache inhibited access */ > cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, > P9_TTYPE_CI_PARTIAL_READ); > - else > + block_size = (blog2(block_size) + 1) << 1; > + } else { > cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, > P9_TTYPE_DMA_PARTIAL_READ); > + block_size = 0; > + } > > /* For a read size is apparently always 0 */ > - cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, 0); > + cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, block_size); The comment is now wrong. For ci, the size is not 0. > cmd_reg |= FBC_ALTD_START_OP; > cmd_reg = SETFIELD(FBC_ALTD_SCOPE, cmd_reg, SCOPE_REMOTE); > cmd_reg = SETFIELD(FBC_ALTD_DROP_PRIORITY, cmd_reg, > DROP_PRIORITY_LOW); > @@ -370,6 +412,8 @@ retry: > /* Clear status bits */ > CHECK_ERR(adu_reset(adu)); > > + CHECK_ERR(pib_write(&adu->target, P9_ALTD_DATA_REG, 0)); > + Was this a bug in the previous code? > /* Set the address */ > ctrl_reg = SETFIELD(P9_FBC_ALTD_ADDRESS, 0ULL, addr); > CHECK_ERR(pib_write(&adu->target, P9_ALTD_CONTROL_REG, > ctrl_reg)); > @@ -400,7 +444,8 @@ retry: > return 0; > } > > -static int p9_adu_putmem(struct adu *adu, uint64_t addr, uint64_t > data, int size, int ci) > +static int p9_adu_putmem(struct adu *adu, uint64_t addr, uint64_t > data, int size, > + int ci, uint8_t block_size) > { > uint64_t ctrl_reg, cmd_reg, val; > > @@ -409,12 +454,15 @@ static int p9_adu_putmem(struct adu *adu, > uint64_t addr, uint64_t data, int size > size <<= 1; > > cmd_reg = P9_TTYPE_TWRITE; > - if (ci) > + if (ci) { > /* Do cache inhibited access */ > cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, > P9_TTYPE_CI_PARTIAL_WRITE); > - else > + block_size = (blog2(block_size) + 1) << 1; > + } else { > cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, > P9_TTYPE_DMA_PARTIAL_WRITE); > - cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, size); > + block_size <<= 1; > + } > + cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, block_size); > cmd_reg |= FBC_ALTD_START_OP; > cmd_reg = SETFIELD(FBC_ALTD_SCOPE, cmd_reg, SCOPE_REMOTE); > cmd_reg = SETFIELD(FBC_ALTD_DROP_PRIORITY, cmd_reg, > DROP_PRIORITY_LOW); > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h > index 1977c75..0ba8508 100644 > --- a/libpdbg/libpdbg.h > +++ b/libpdbg/libpdbg.h > @@ -178,12 +178,18 @@ int htm_status(struct pdbg_target *target); > int htm_dump(struct pdbg_target *target, char *filename); > int htm_record(struct pdbg_target *target, char *filename); > > -int adu_getmem(struct pdbg_target *target, uint64_t addr, uint8_t > *ouput, uint64_t size); > -int adu_putmem(struct pdbg_target *target, uint64_t addr, uint8_t > *input, uint64_t size); > -int adu_getmem_ci(struct pdbg_target *target, uint64_t addr, uint8_t > *ouput, uint64_t size); > -int adu_putmem_ci(struct pdbg_target *target, uint64_t addr, uint8_t > *input, uint64_t size); > -int __adu_getmem(struct pdbg_target *target, uint64_t addr, uint8_t > *ouput, uint64_t size, bool ci); > -int __adu_putmem(struct pdbg_target *target, uint64_t addr, uint8_t > *input, uint64_t size, bool ci); > +int adu_getmem(struct pdbg_target *target, uint64_t addr, > + uint8_t *ouput, uint64_t size, uint8_t block_size); > +int adu_putmem(struct pdbg_target *target, uint64_t addr, > + uint8_t *input, uint64_t size, uint8_t block_size); > +int adu_getmem_ci(struct pdbg_target *target, uint64_t addr, > + uint8_t *ouput, uint64_t size, uint8_t block_size); > +int adu_putmem_ci(struct pdbg_target *target, uint64_t addr, > + uint8_t *input, uint64_t size, uint8_t block_size); > +int __adu_getmem(struct pdbg_target *target, uint64_t addr, uint8_t > *ouput, > + uint64_t size, bool ci, uint8_t block_size); > +int __adu_putmem(struct pdbg_target *target, uint64_t addr, uint8_t > *input, > + uint64_t size, bool ci, uint8_t block_size); > > int opb_read(struct pdbg_target *target, uint32_t addr, uint32_t > *data); > int opb_write(struct pdbg_target *target, uint32_t addr, uint32_t > data); > diff --git a/libpdbg/target.h b/libpdbg/target.h > index d1a6aec..96d0c73 100644 > --- a/libpdbg/target.h > +++ b/libpdbg/target.h > @@ -101,8 +101,8 @@ struct htm { > > struct adu { > struct pdbg_target target; > - int (*getmem)(struct adu *, uint64_t, uint64_t *, int); > - int (*putmem)(struct adu *, uint64_t, uint64_t, int, int); > + int (*getmem)(struct adu *, uint64_t, uint64_t *, int, > uint8_t); > + int (*putmem)(struct adu *, uint64_t, uint64_t, int, int, > uint8_t); > }; > #define target_to_adu(x) container_of(x, struct adu, target) > > diff --git a/src/mem.c b/src/mem.c > index ce099c2..b578f95 100644 > --- a/src/mem.c > +++ b/src/mem.c > @@ -39,8 +39,10 @@ struct mem_flags { > }; > > #define MEM_CI_FLAG ("--ci", ci, parse_flag_noarg, false) > +#define BLOCK_SIZE (parse_number8_pow2, "8") > > -static int getmem(uint64_t addr, uint64_t size, struct mem_flags > flags) > +static int getmem(uint64_t addr, uint64_t size, > + uint8_t block_size, struct mem_flags flags) > { > struct pdbg_target *target; > uint8_t *buf; > @@ -59,7 +61,7 @@ static int getmem(uint64_t addr, uint64_t size, > struct mem_flags flags) > > pdbg_set_progress_tick(progress_tick); > progress_init(); > - if (!__adu_getmem(target, addr, buf, size, flags.ci)) { > + if (!__adu_getmem(target, addr, buf, size, flags.ci, > block_size)) { > if (write(STDOUT_FILENO, buf, size) < 0) > PR_ERROR("Unable to write stdout.\n"); > else > @@ -74,10 +76,10 @@ static int getmem(uint64_t addr, uint64_t size, > struct mem_flags flags) > return rc; > > } > -OPTCMD_DEFINE_CMD_WITH_FLAGS(getmem, getmem, (ADDRESS, DATA), > +OPTCMD_DEFINE_CMD_WITH_FLAGS(getmem, getmem, (ADDRESS, DATA, > BLOCK_SIZE), > mem_flags, (MEM_CI_FLAG)); > > -static int putmem(uint64_t addr, struct mem_flags flags) > +static int putmem(uint64_t addr, uint8_t block_size, struct > mem_flags flags) > { > uint8_t *buf; > int read_size, rc = 0; > @@ -98,7 +100,7 @@ static int putmem(uint64_t addr, struct mem_flags > flags) > if (read_size <= 0) > break; > > - if (__adu_putmem(adu_target, addr, buf, read_size, > flags.ci)) { > + if (__adu_putmem(adu_target, addr, buf, read_size, > flags.ci, block_size)) { > rc = 0; > printf("Unable to write memory.\n"); > break; > @@ -111,5 +113,5 @@ static int putmem(uint64_t addr, struct mem_flags > flags) > free(buf); > return rc; > } > -OPTCMD_DEFINE_CMD_WITH_FLAGS(putmem, putmem, (ADDRESS), > +OPTCMD_DEFINE_CMD_WITH_FLAGS(putmem, putmem, (ADDRESS, BLOCK_SIZE), > mem_flags, (MEM_CI_FLAG)); > diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c > index 697e58e..64c1ae4 100644 > --- a/src/pdbgproxy.c > +++ b/src/pdbgproxy.c > @@ -233,7 +233,7 @@ static void get_mem(uint64_t *stack, void *priv) > > linear_map = get_real_addr(addr); > if (linear_map != -1UL) { > - if (adu_getmem(adu, linear_map, (uint8_t *) data, len)) > { > + if (adu_getmem(adu, linear_map, (uint8_t *) data, len, > 8)) { > PR_ERROR("Unable to read memory\n"); > err = 1; > } > @@ -315,7 +315,7 @@ static void put_mem(uint64_t *stack, void *priv) > > PR_INFO("put_mem 0x%016" PRIx64 " = 0x%016" PRIx64 "\n", addr, > stack[2]); > > - if (adu_putmem(adu, addr, data, len)) { > + if (adu_putmem(adu, addr, data, len, 8)) { > PR_ERROR("Unable to write memory\n"); > err = 3; > } > diff --git a/src/thread.c b/src/thread.c > index 4401384..02dd1c2 100644 > --- a/src/thread.c > +++ b/src/thread.c > @@ -97,7 +97,7 @@ static bool is_real_address(struct thread_regs > *regs, uint64_t addr) > > static int load8(struct pdbg_target *target, uint64_t addr, uint64_t > *value) > { > - if (adu_getmem(target, addr, (uint8_t *)value, 8)) { > + if (adu_getmem(target, addr, (uint8_t *)value, 8, 8)) { > pdbg_log(PDBG_ERROR, "Unable to read memory > address=%016" PRIx64 ".\n", addr); > return 0; > } > -- > 2.11.0 > Amitay.
On Monday, 10 December 2018 1:22:22 PM AEDT Amitay Isaacs wrote: > On Mon, 2018-12-10 at 11:49 +1100, Alistair Popple wrote: > > Not all memory can be read with the default ADU block size of 8 > > bytes. Specifically cache-inhibited access to some MMIO regions such > > as PCIe BAR spaces requires 4 byte accesses to avoid check stopping > > the machine. > > > > This patch adds an argument to the put/getmem commands to allow a > > specific block size to be selected. > > Are you missing some more bits in the patch? > > $ git am \[Pdbg\]_\[PATCH_2_2\]_adu\:_Add_arugments_for_block_size.mbox > Applying: adu: Add arugments for block size > error: patch failed: src/pdbgproxy.c:233 > error: src/pdbgproxy.c: patch does not apply > Patch failed at 0001 adu: Add arugments for block size > hint: Use 'git am --show-current-patch' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". Might need a rebase. I wrote this a little while ago for ecmd and it took a while to get tested. > > /* Align start address to 8-byte boundary */ > > > > - addr0 = 8 * (start_addr / 8); > > + addr0 = block_size * (start_addr / block_size); > > The comment is now wrong. True. Will fix all the comments. > > /* We read data in 8-byte aligned chunks */ > > > > - for (addr = addr0; addr < start_addr + size; addr += 8) { > > + for (addr = addr0; addr < start_addr + size; addr += > > block_size) { > > The comment is now wrong. > > > uint64_t data; > > > > - if (adu->getmem(adu, addr, &data, ci)) > > + if (adu->getmem(adu, addr, &data, ci, block_size)) > > > > return -1; > > > > - /* ADU returns data in big-endian form in the register > > */ > > + /* ADU returns data in big-endian form in the register. > > */ > > > > data = __builtin_bswap64(data); > > > > + data >>= (addr & 0x7ull)*8; > > > > if (addr < start_addr) { > > > > size_t offset = start_addr - addr; > > > > - size_t n = (size <= 8-offset ? size : 8- > > offset); > > + size_t n = (size <= block_size-offset ? size : > > block_size-offset); > > > > memcpy(output, ((uint8_t *) &data) + offset, > > > > n); > > > > output += n; > > > > - } else if (addr + 8 > start_addr + size) { > > + } else if (addr + block_size > start_addr + size) { > > > > uint64_t offset = start_addr + size - addr; > > > > memcpy(output, &data, offset); > > output += offset; > > > > } else { > > > > - memcpy(output, &data, 8); > > - output += 8; > > + memcpy(output, &data, block_size); > > + output += block_size; > > > > } > > > > pdbg_progress_tick(output - output0, size); > > > > @@ -147,19 +176,19 @@ int __adu_getmem(struct pdbg_target > > *adu_target, uint64_t start_addr, > > > > } > > > > int adu_putmem(struct pdbg_target *adu_target, uint64_t start_addr, > > > > - uint8_t *output, uint64_t size) > > + uint8_t *output, uint64_t size, uint8_t block_size) > > > > { > > > > - return __adu_putmem(adu_target, start_addr, output, size, > > false); > > + return __adu_putmem(adu_target, start_addr, output, size, > > false, block_size); > > > > } > > > > int adu_putmem_ci(struct pdbg_target *adu_target, uint64_t > > > > start_addr, > > - uint8_t *output, uint64_t size) > > + uint8_t *output, uint64_t size, uint8_t block_size) > > > > { > > > > - return __adu_putmem(adu_target, start_addr, output, size, > > true); > > + return __adu_putmem(adu_target, start_addr, output, size, true, > > block_size); > > > > } > > > > int __adu_putmem(struct pdbg_target *adu_target, uint64_t > > > > start_addr, > > - uint8_t *input, uint64_t size, bool ci) > > + uint8_t *input, uint64_t size, bool ci, uint8_t > > block_size) > > > > { > > > > struct adu *adu; > > int rc = 0, tsize; > > > > @@ -169,7 +198,7 @@ int __adu_putmem(struct pdbg_target *adu_target, > > uint64_t start_addr, > > > > adu = target_to_adu(adu_target); > > end_addr = start_addr + size; > > for (addr = start_addr; addr < end_addr; addr += tsize, input > > > > += tsize) { > > - if ((addr % 8) || (addr + 8 > end_addr)) { > > + if ((addr % block_size) || (addr + block_size > > > end_addr)) { > > > > /* If the address is not 64-bit aligned we > > > > * copy in a byte at a time until it is. */ > > The above comment is now wrong. When handling non-aligned addresses, > we are putting one byte at a time. Does that need to be aligned to > block-size? If we still need to do block-size alignment, then the > following code is wrong. Yes, that does seem a little suspect. It was mostly copied from the ekb so will dig a little bit more here and see what's happening. > > tsize = 1; > > > > @@ -177,12 +206,13 @@ int __adu_putmem(struct pdbg_target > > *adu_target, uint64_t start_addr, > > > > /* Copy the input data in with correct > > > > alignment */ > > > > data = ((uint64_t) *input) << 8*(8 - (addr % 8) > > > > - 1); > > The above code is still 8-byte aligned, not block-size aligned. Right, from what I understand based on reading the ekb (sadly documentation of the ADU at this level is lacking) it seems regardless of block size the ADU sources bytes from the same byte positions within the register. > > > > + CHECK_ERR(pib_write(&adu->target, P9_ALTD_DATA_REG, 0)); > > + > > Was this a bug in the previous code? No. That was left over from some debugging, will remove. Thanks! - Alistair
diff --git a/libpdbg/adu.c b/libpdbg/adu.c index b67a43e..3ab0d31 100644 --- a/libpdbg/adu.c +++ b/libpdbg/adu.c @@ -84,20 +84,48 @@ #define FBC_ALTD_DATA_DONE PPC_BIT(3) #define FBC_ALTD_PBINIT_MISSING PPC_BIT(18) +/* There are more general implementations of this with a loop and more + * performant implementations using GCC builtins which aren't + * portable. Given we only need a limited domain this is quick, easy + * and portable. */ +uint8_t blog2(uint8_t x) +{ + switch(x) { + case 1: + return 0; + case 2: + return 1; + case 4: + return 2; + case 8: + return 3; + case 16: + return 4; + case 32: + return 5; + case 64: + return 6; + default: + assert(0); + } +} + int adu_getmem(struct pdbg_target *adu_target, uint64_t start_addr, - uint8_t *output, uint64_t size) + uint8_t *output, uint64_t size, uint8_t block_size) { - return __adu_getmem(adu_target, start_addr, output, size, false); + return __adu_getmem(adu_target, start_addr, output, + size, false, block_size); } int adu_getmem_ci(struct pdbg_target *adu_target, uint64_t start_addr, - uint8_t *output, uint64_t size) + uint8_t *output, uint64_t size, uint8_t block_size) { - return __adu_getmem(adu_target, start_addr, output, size, true); + return __adu_getmem(adu_target, start_addr, output, + size, true, block_size); } int __adu_getmem(struct pdbg_target *adu_target, uint64_t start_addr, - uint8_t *output, uint64_t size, bool ci) + uint8_t *output, uint64_t size, bool ci, uint8_t block_size) { struct adu *adu; uint8_t *output0; @@ -110,32 +138,33 @@ int __adu_getmem(struct pdbg_target *adu_target, uint64_t start_addr, output0 = output; /* Align start address to 8-byte boundary */ - addr0 = 8 * (start_addr / 8); + addr0 = block_size * (start_addr / block_size); /* We read data in 8-byte aligned chunks */ - for (addr = addr0; addr < start_addr + size; addr += 8) { + for (addr = addr0; addr < start_addr + size; addr += block_size) { uint64_t data; - if (adu->getmem(adu, addr, &data, ci)) + if (adu->getmem(adu, addr, &data, ci, block_size)) return -1; - /* ADU returns data in big-endian form in the register */ + /* ADU returns data in big-endian form in the register. */ data = __builtin_bswap64(data); + data >>= (addr & 0x7ull)*8; if (addr < start_addr) { size_t offset = start_addr - addr; - size_t n = (size <= 8-offset ? size : 8-offset); + size_t n = (size <= block_size-offset ? size : block_size-offset); memcpy(output, ((uint8_t *) &data) + offset, n); output += n; - } else if (addr + 8 > start_addr + size) { + } else if (addr + block_size > start_addr + size) { uint64_t offset = start_addr + size - addr; memcpy(output, &data, offset); output += offset; } else { - memcpy(output, &data, 8); - output += 8; + memcpy(output, &data, block_size); + output += block_size; } pdbg_progress_tick(output - output0, size); @@ -147,19 +176,19 @@ int __adu_getmem(struct pdbg_target *adu_target, uint64_t start_addr, } int adu_putmem(struct pdbg_target *adu_target, uint64_t start_addr, - uint8_t *output, uint64_t size) + uint8_t *output, uint64_t size, uint8_t block_size) { - return __adu_putmem(adu_target, start_addr, output, size, false); + return __adu_putmem(adu_target, start_addr, output, size, false, block_size); } int adu_putmem_ci(struct pdbg_target *adu_target, uint64_t start_addr, - uint8_t *output, uint64_t size) + uint8_t *output, uint64_t size, uint8_t block_size) { - return __adu_putmem(adu_target, start_addr, output, size, true); + return __adu_putmem(adu_target, start_addr, output, size, true, block_size); } int __adu_putmem(struct pdbg_target *adu_target, uint64_t start_addr, - uint8_t *input, uint64_t size, bool ci) + uint8_t *input, uint64_t size, bool ci, uint8_t block_size) { struct adu *adu; int rc = 0, tsize; @@ -169,7 +198,7 @@ int __adu_putmem(struct pdbg_target *adu_target, uint64_t start_addr, adu = target_to_adu(adu_target); end_addr = start_addr + size; for (addr = start_addr; addr < end_addr; addr += tsize, input += tsize) { - if ((addr % 8) || (addr + 8 > end_addr)) { + if ((addr % block_size) || (addr + block_size > end_addr)) { /* If the address is not 64-bit aligned we * copy in a byte at a time until it is. */ tsize = 1; @@ -177,12 +206,13 @@ int __adu_putmem(struct pdbg_target *adu_target, uint64_t start_addr, /* Copy the input data in with correct alignment */ data = ((uint64_t) *input) << 8*(8 - (addr % 8) - 1); } else { - tsize = 8; - memcpy(&data, input, sizeof(data)); + tsize = block_size; + memcpy(&data, input, block_size); data = __builtin_bswap64(data); + data >>= (addr & 7ull)*8; } - adu->putmem(adu, addr, data, tsize, ci); + adu->putmem(adu, addr, data, tsize, ci, block_size); pdbg_progress_tick(addr - start_addr, size); } @@ -234,7 +264,8 @@ static int adu_reset(struct adu *adu) return 0; } -static int p8_adu_getmem(struct adu *adu, uint64_t addr, uint64_t *data, int ci) +static int p8_adu_getmem(struct adu *adu, uint64_t addr, uint64_t *data, + int ci, uint8_t block_size) { uint64_t ctrl_reg, cmd_reg, val; int rc = 0; @@ -242,12 +273,15 @@ static int p8_adu_getmem(struct adu *adu, uint64_t addr, uint64_t *data, int ci) CHECK_ERR(adu_lock(adu)); ctrl_reg = P8_TTYPE_TREAD; - if (ci) + if (ci) { /* Do cache inhibited access */ ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, P8_TTYPE_CI_PARTIAL_READ); - else + block_size = (blog2(block_size) + 1) << 1; + } else { ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, P8_TTYPE_DMA_PARTIAL_READ); - ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, 8); + block_size = 0; + } + ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, block_size); CHECK_ERR_GOTO(out, rc = pib_read(&adu->target, P8_ALTD_CMD_REG, &cmd_reg)); cmd_reg |= FBC_ALTD_START_OP; @@ -292,19 +326,23 @@ out: } -int p8_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int size, int ci) +int p8_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int size, + int ci, uint8_t block_size) { int rc = 0; uint64_t cmd_reg, ctrl_reg, val; CHECK_ERR(adu_lock(adu)); ctrl_reg = P8_TTYPE_TWRITE; - if (ci) + if (ci) { /* Do cache inhibited access */ ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, P8_TTYPE_CI_PARTIAL_WRITE); - else + block_size = (blog2(block_size) + 1) << 1; + } else { ctrl_reg = SETFIELD(P8_FBC_ALTD_TTYPE, ctrl_reg, P8_TTYPE_DMA_PARTIAL_WRITE); - ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, size); + block_size <<= 1; + } + ctrl_reg = SETFIELD(P8_FBC_ALTD_TSIZE, ctrl_reg, block_size); CHECK_ERR_GOTO(out, rc = pib_read(&adu->target, P8_ALTD_CMD_REG, &cmd_reg)); cmd_reg |= FBC_ALTD_START_OP; @@ -349,19 +387,23 @@ out: return rc; } -static int p9_adu_getmem(struct adu *adu, uint64_t addr, uint64_t *data, int ci) +static int p9_adu_getmem(struct adu *adu, uint64_t addr, uint64_t *data, + int ci, uint8_t block_size) { uint64_t ctrl_reg, cmd_reg, val; cmd_reg = P9_TTYPE_TREAD; - if (ci) + if (ci) { /* Do cache inhibited access */ cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, P9_TTYPE_CI_PARTIAL_READ); - else + block_size = (blog2(block_size) + 1) << 1; + } else { cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, P9_TTYPE_DMA_PARTIAL_READ); + block_size = 0; + } /* For a read size is apparently always 0 */ - cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, 0); + cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, block_size); cmd_reg |= FBC_ALTD_START_OP; cmd_reg = SETFIELD(FBC_ALTD_SCOPE, cmd_reg, SCOPE_REMOTE); cmd_reg = SETFIELD(FBC_ALTD_DROP_PRIORITY, cmd_reg, DROP_PRIORITY_LOW); @@ -370,6 +412,8 @@ retry: /* Clear status bits */ CHECK_ERR(adu_reset(adu)); + CHECK_ERR(pib_write(&adu->target, P9_ALTD_DATA_REG, 0)); + /* Set the address */ ctrl_reg = SETFIELD(P9_FBC_ALTD_ADDRESS, 0ULL, addr); CHECK_ERR(pib_write(&adu->target, P9_ALTD_CONTROL_REG, ctrl_reg)); @@ -400,7 +444,8 @@ retry: return 0; } -static int p9_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int size, int ci) +static int p9_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int size, + int ci, uint8_t block_size) { uint64_t ctrl_reg, cmd_reg, val; @@ -409,12 +454,15 @@ static int p9_adu_putmem(struct adu *adu, uint64_t addr, uint64_t data, int size size <<= 1; cmd_reg = P9_TTYPE_TWRITE; - if (ci) + if (ci) { /* Do cache inhibited access */ cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, P9_TTYPE_CI_PARTIAL_WRITE); - else + block_size = (blog2(block_size) + 1) << 1; + } else { cmd_reg = SETFIELD(P9_FBC_ALTD_TTYPE, cmd_reg, P9_TTYPE_DMA_PARTIAL_WRITE); - cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, size); + block_size <<= 1; + } + cmd_reg = SETFIELD(P9_FBC_ALTD_TSIZE, cmd_reg, block_size); cmd_reg |= FBC_ALTD_START_OP; cmd_reg = SETFIELD(FBC_ALTD_SCOPE, cmd_reg, SCOPE_REMOTE); cmd_reg = SETFIELD(FBC_ALTD_DROP_PRIORITY, cmd_reg, DROP_PRIORITY_LOW); diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h index 1977c75..0ba8508 100644 --- a/libpdbg/libpdbg.h +++ b/libpdbg/libpdbg.h @@ -178,12 +178,18 @@ int htm_status(struct pdbg_target *target); int htm_dump(struct pdbg_target *target, char *filename); int htm_record(struct pdbg_target *target, char *filename); -int adu_getmem(struct pdbg_target *target, uint64_t addr, uint8_t *ouput, uint64_t size); -int adu_putmem(struct pdbg_target *target, uint64_t addr, uint8_t *input, uint64_t size); -int adu_getmem_ci(struct pdbg_target *target, uint64_t addr, uint8_t *ouput, uint64_t size); -int adu_putmem_ci(struct pdbg_target *target, uint64_t addr, uint8_t *input, uint64_t size); -int __adu_getmem(struct pdbg_target *target, uint64_t addr, uint8_t *ouput, uint64_t size, bool ci); -int __adu_putmem(struct pdbg_target *target, uint64_t addr, uint8_t *input, uint64_t size, bool ci); +int adu_getmem(struct pdbg_target *target, uint64_t addr, + uint8_t *ouput, uint64_t size, uint8_t block_size); +int adu_putmem(struct pdbg_target *target, uint64_t addr, + uint8_t *input, uint64_t size, uint8_t block_size); +int adu_getmem_ci(struct pdbg_target *target, uint64_t addr, + uint8_t *ouput, uint64_t size, uint8_t block_size); +int adu_putmem_ci(struct pdbg_target *target, uint64_t addr, + uint8_t *input, uint64_t size, uint8_t block_size); +int __adu_getmem(struct pdbg_target *target, uint64_t addr, uint8_t *ouput, + uint64_t size, bool ci, uint8_t block_size); +int __adu_putmem(struct pdbg_target *target, uint64_t addr, uint8_t *input, + uint64_t size, bool ci, uint8_t block_size); int opb_read(struct pdbg_target *target, uint32_t addr, uint32_t *data); int opb_write(struct pdbg_target *target, uint32_t addr, uint32_t data); diff --git a/libpdbg/target.h b/libpdbg/target.h index d1a6aec..96d0c73 100644 --- a/libpdbg/target.h +++ b/libpdbg/target.h @@ -101,8 +101,8 @@ struct htm { struct adu { struct pdbg_target target; - int (*getmem)(struct adu *, uint64_t, uint64_t *, int); - int (*putmem)(struct adu *, uint64_t, uint64_t, int, int); + int (*getmem)(struct adu *, uint64_t, uint64_t *, int, uint8_t); + int (*putmem)(struct adu *, uint64_t, uint64_t, int, int, uint8_t); }; #define target_to_adu(x) container_of(x, struct adu, target) diff --git a/src/mem.c b/src/mem.c index ce099c2..b578f95 100644 --- a/src/mem.c +++ b/src/mem.c @@ -39,8 +39,10 @@ struct mem_flags { }; #define MEM_CI_FLAG ("--ci", ci, parse_flag_noarg, false) +#define BLOCK_SIZE (parse_number8_pow2, "8") -static int getmem(uint64_t addr, uint64_t size, struct mem_flags flags) +static int getmem(uint64_t addr, uint64_t size, + uint8_t block_size, struct mem_flags flags) { struct pdbg_target *target; uint8_t *buf; @@ -59,7 +61,7 @@ static int getmem(uint64_t addr, uint64_t size, struct mem_flags flags) pdbg_set_progress_tick(progress_tick); progress_init(); - if (!__adu_getmem(target, addr, buf, size, flags.ci)) { + if (!__adu_getmem(target, addr, buf, size, flags.ci, block_size)) { if (write(STDOUT_FILENO, buf, size) < 0) PR_ERROR("Unable to write stdout.\n"); else @@ -74,10 +76,10 @@ static int getmem(uint64_t addr, uint64_t size, struct mem_flags flags) return rc; } -OPTCMD_DEFINE_CMD_WITH_FLAGS(getmem, getmem, (ADDRESS, DATA), +OPTCMD_DEFINE_CMD_WITH_FLAGS(getmem, getmem, (ADDRESS, DATA, BLOCK_SIZE), mem_flags, (MEM_CI_FLAG)); -static int putmem(uint64_t addr, struct mem_flags flags) +static int putmem(uint64_t addr, uint8_t block_size, struct mem_flags flags) { uint8_t *buf; int read_size, rc = 0; @@ -98,7 +100,7 @@ static int putmem(uint64_t addr, struct mem_flags flags) if (read_size <= 0) break; - if (__adu_putmem(adu_target, addr, buf, read_size, flags.ci)) { + if (__adu_putmem(adu_target, addr, buf, read_size, flags.ci, block_size)) { rc = 0; printf("Unable to write memory.\n"); break; @@ -111,5 +113,5 @@ static int putmem(uint64_t addr, struct mem_flags flags) free(buf); return rc; } -OPTCMD_DEFINE_CMD_WITH_FLAGS(putmem, putmem, (ADDRESS), +OPTCMD_DEFINE_CMD_WITH_FLAGS(putmem, putmem, (ADDRESS, BLOCK_SIZE), mem_flags, (MEM_CI_FLAG)); diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c index 697e58e..64c1ae4 100644 --- a/src/pdbgproxy.c +++ b/src/pdbgproxy.c @@ -233,7 +233,7 @@ static void get_mem(uint64_t *stack, void *priv) linear_map = get_real_addr(addr); if (linear_map != -1UL) { - if (adu_getmem(adu, linear_map, (uint8_t *) data, len)) { + if (adu_getmem(adu, linear_map, (uint8_t *) data, len, 8)) { PR_ERROR("Unable to read memory\n"); err = 1; } @@ -315,7 +315,7 @@ static void put_mem(uint64_t *stack, void *priv) PR_INFO("put_mem 0x%016" PRIx64 " = 0x%016" PRIx64 "\n", addr, stack[2]); - if (adu_putmem(adu, addr, data, len)) { + if (adu_putmem(adu, addr, data, len, 8)) { PR_ERROR("Unable to write memory\n"); err = 3; } diff --git a/src/thread.c b/src/thread.c index 4401384..02dd1c2 100644 --- a/src/thread.c +++ b/src/thread.c @@ -97,7 +97,7 @@ static bool is_real_address(struct thread_regs *regs, uint64_t addr) static int load8(struct pdbg_target *target, uint64_t addr, uint64_t *value) { - if (adu_getmem(target, addr, (uint8_t *)value, 8)) { + if (adu_getmem(target, addr, (uint8_t *)value, 8, 8)) { pdbg_log(PDBG_ERROR, "Unable to read memory address=%016" PRIx64 ".\n", addr); return 0; }
Not all memory can be read with the default ADU block size of 8 bytes. Specifically cache-inhibited access to some MMIO regions such as PCIe BAR spaces requires 4 byte accesses to avoid check stopping the machine. This patch adds an argument to the put/getmem commands to allow a specific block size to be selected. Signed-off-by: Alistair Popple <alistair@popple.id.au> --- libpdbg/adu.c | 124 +++++++++++++++++++++++++++++++++++++----------------- libpdbg/libpdbg.h | 18 +++++--- libpdbg/target.h | 4 +- src/mem.c | 14 +++--- src/pdbgproxy.c | 4 +- src/thread.c | 2 +- 6 files changed, 111 insertions(+), 55 deletions(-)