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

[Alistair Popple]

On Thursday, 15 November 2018 12:05:52 PM AEDT Amitay Isaacs wrote:
> > 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?

Yep, I should probably have been clearer about the motivation. In general when 
debugging systems I've found it to be good not to hide magic - specifying a 
path+offset is really just a convenient way of specifying a specific SCOM 
operation, but it should always report the exact operation that was done (ie. 
getscom <xlate-addr>)

It may also help new users figure out what specifying a path actually does.
 
> 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)

Actually I kinda like the combined output. I don't think it's too verbose or 
any harder to read as the full paths should be aligned in a column anyway.

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

pdbg_address_absolute()? pdbg_address_translate() works as well though. 
relative seems confusing to me because your passing in a relative address.

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

Excellent point, I agree. So basically just an automagic version of the 
existing get_class_target_addr(). For the moment we can keep the 
implementation simple. Something like the below psuedo-code should work:

struct target *pdbg_address_absolute(tgt, offset, addr) {
	assert(pdbg_target_class(tgt) == "pib", "chiplet", "xbus", <rest of the 
scommable classes>, ...);
	tgt = get_class_target_addr(tgt, "pib", &addr);
	return tgt;
}

I can start working on more generalised "magic".

- Alistair

> Amitay.