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

[Alistair Popple]

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

> 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 
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.

- Alistair

> Amitay.