[Pdbg] [PATCH 2/2] adu: Add arugments for block size

[Alistair Popple]

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