[Pdbg] [PATCH v2 14/15] main: Convert getscom/putscom to use path based targeting

[Amitay Isaacs]

On Wed, 2018-11-14 at 17:30 +1100, Alistair Popple wrote:
> On Wednesday, 14 November 2018 4:56:29 PM AEDT Amitay Isaacs wrote:
> > On Tue, 2018-11-13 at 16:09 +1100, Alistair Popple wrote:
> > > On Friday, 9 November 2018 6:10:10 PM AEDT Amitay Isaacs wrote:
> > > > +static bool scommable(struct pdbg_target *target)
> > > > 
> > > >  {
> > > > 
> > > > -	uint64_t value;
> > > > -
> > > > -	if (pib_read(target, *addr, &value))
> > > > -		return 0;
> > > > +	char *classname;
> > > > 
> > > > -	printf("p%d:0x%" PRIx64 " = 0x%016" PRIx64 "\n", index,
> > > > *addr,
> > > > value);
> > > > +	classname = pdbg_target_class_name(target);
> > > > +	if (!strcmp(classname, "pib") ||
> > > > +	    !strcmp(classname, "core") ||
> > > > +	    !strcmp(classname, "thread"))
> > > > +		return true;
> > > 
> > > As we've discussed/agreed this is suboptimal but will work for
> > > the
> > > time being
> > > until we come up with a better solution.
> > 
> > Just to confirm, does getscom/putscom work on thread class?  Or
> > it's
> > only pib and core classes?
> 
> It depends :-)
> 
> On P8 it theorectically could, P9 not so much. In practice it's
> unlikely to be 
> ever used on P8 so until we come up with a better solution I think
> it's safe 
> to conclude that get/putscom won't work on a thread class.
> 
> > > > -	return 1;
> > > > +	return false;
> > > > 
> > > >  }
> > > 
> > > <snip>
> > > 
> > > > +		/* TODO: Restore the <mask> functionality */
> > > 
> > > We really should fix that at some point :-) I will take a look at
> > > it
> > > once this
> > > series is done.
> > > 
> > > > +		if (pib_write(target, addr, data)) {
> > > > +			printf("%s: failed\n", path);
> > > > +			free(path);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		count++;
> > > > +	}
> > > > +
> > > > +	return count;
> > > > 
> > > >  }
> > > >  OPTCMD_DEFINE_CMD_WITH_ARGS(putscom, putscom, (ADDRESS, DATA,
> > > > 
> > > > DEFAULT_DATA("0xffffffffffffffff"))); diff --git
> > > > a/tests/test_hw_bmc.sh
> > > > b/tests/test_hw_bmc.sh
> > > > index 9bc1deb..d656a3f 100755
> > > > --- a/tests/test_hw_bmc.sh
> > > > +++ b/tests/test_hw_bmc.sh
> > > > @@ -91,7 +91,7 @@ do_skip
> > > > 
> > > >  test_run $PDBG -P fsi getcfam 0xc09
> > > >  
> > > >  test_result 0 <<EOF
> > > > 
> > > > -p0:0xf000f = HEX16
> > > > +/kernelfsi at 0/pib at 1000: 0xf000f = HEX16
> > > 
> > > As mentioned in the previous patch I think for the time being we
> > > should try
> > > and maintain the same output if possible. I suspect printing the
> > > full
> > > path
> > > will just be confusing so perhaps we could add it as an option?
> > 
> > Sure.  This means that if the selected target is a core, then it
> > will
> > print "p0:c0:" and for a thread it will print "p0:c0:t0:".
> 
> Actually that's more complicated than what I'm thinking. I was
> thinking along 
> the lines of something like:
> 
> # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010
> p0:0x5001010 = 0xdeadbeef
> 
> Where 0x5001010 is the final translated address that is actually
> read/written. 
> However now that I've typed that out I can see a problem with that
> approach - 
> when multiple targets are selected it would be hard to correlate
> which was 
> which. So perhaps if you're specifying the path it would imply that
> you 
> shouldn't be confused by it in a print out. I'd still like to see the
> complete 
> translated address in the outut though.
> 
> So how about we do this:
> 1. If -P is specified print the full path and complete translated
> address. eg:
> 
> # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010
> /fsi0/pib0/chiplet5:0x5001010 = 0xdeadbeef
> 
> 2. If -p/-c is specified print the same short-hand notation
> (p0:c0:<addr> = 
> <data>)
> 

If we print the translated address then it makes sense to print the
target relative to which the address is translated.  So even for
chipsets specified with -P, printing "p0:<xlate-addr>" makes sense.

Printing the translated address may be a way to confirm that the
address translation is working correctly, specially once we add weird
translation schemes.  Is that the motivation behind printing translated
addresses?  

To be consistent we have to use either of the following formats.

1. /fsi0/pib0/chiplet5: 0x1010 = 0xdeadbeef

This makes the explicit the target and the scom address specified.  

2. p0: 0x5001010 = 0xdeadbeef

This is more engineering output which tells what address space the scom
address falls under and the actual translated address relative to that
address space.  

The first format is advantageous if there are multiple targets printed:

   /fsi0/pib0/chiplet1: 0x1010 = 0xdeadbeef
   /fsi0/pib0/chiplet3: 0x1010 = 0xdeadbeef
   /fsi0/pib0/chiplet5: 0x1010 = 0xdeadbeef

Rather than,

   p0: 0x1001010 = 0xdeadbeef
   p0: 0x3001010 = 0xdeadbeef
   p0: 0x5001010 = 0xdeadbeef

3. May be we want both formats and can be specified by user.  We can
add an option (e.g. -x) to print translated address.

4. How about combined output?  May be too verbose.

   p0: 0x1001010 = 0xdeadbeef (/fsi0/pib0/chiplet1)
   p0: 0x3001010 =
0xdeadbeef (/fsi0/pib0/chiplet3)
   p0: 0x5001010 = 0xdeadbeef
(/fsi0/pib0/chiplet5)



> > Can we do getscom/putscom on non pib/core/thread targets?  If yes,
> > what
> > should be the prefix for those targets?
> 
> Yes, but lets not worry about prefixing those. Lets just print the
> path in that 
> case. My primary concern was changing the output of something like:
> 
> # pdbg -p0 getscom 0xf000f
> 
> From `p0:0xf000f = <data>` to `/fsi0/pib1000 = <data>` which I
> suspect may 
> confuse existing users. Anyone who figures out how to specify a path
> wont (or 
> at least shouldn't) be surprised to see it in the output.
> 
> > > Also I think we should print the complete translated SCOM address
> > > rather than
> > > the offset pdbg was called with. Thanks!
> > 
> > We need pdbg_target_address_translate() or something similar. IIRC,
> > there was a patch that added a public wrapper around
> > get_class_target_addr() but used a bad name.  :-)
> 
> Yeah. But "somebody" said it was a bad API and we shouldn't need it
> :-)
> 
> > I can add that patch back.
> 
> Turns out that somebody was correct though and we need a better name
> and API. 
> Instead of manually specifying the target address class the API
> should just 
> take target and address offset and magically return the translated
> address for 
> the address space it belongs to. Something like:
> 
> uint64_t target_parent_addr(struct pdbg_target *target, uint64_t
> offset)
>  
> Although happy to bike shed names. For the moment the "magic" could
> just be 

  pdbg_address_relative(), or
  pdbg_address_translate()

> hardcoded to check scommable() and use the existing
> get_class_target_addr(tgt, 
> "pib", offset). We can add better magic once this series is done.

If we do automagic translation relative to the correct address space, I
think the api should also return the base target whose address space 
the scom address resides in.


Amitay.
-- 

Pride makes us do things well. But it is love that makes us do them do 
perfection. - Dad