[Pdbg] [RFC PATCH] libpdbg: Handle cooked/raw CFAM addressing modes

[Andrew Jeffery]

On Mon, 26 Aug 2019, at 21:09, Alistair Popple wrote:
> In general I'm not a fan of using environment variables to configure user 
> options as it's too easy to forget about them. I would rather this be a 
> command line option (eg. --byte-addressing). Obviously this mean you have to 
> specify this option every time you run, but it is no harder to create command 
> aliases to fix this by always passing the option than it is to create an 
> environment variable.

Fair points! Using an environment variable was a bit of a back-door method to
avoid some API plumbing work, so the criticism is warranted :D

> 
> Sadly I also doubt we will ever be able to switch the default as hardware 
> procedures are written assuming "cooked" FSI addresses and most people who use 
> get/putcfam expect to use them as well. However having an option to use byte 
> addressing would be really nice, I just think we need to tweak the approach.

Right, I liked your point about aliases, I hadn't considered them.

> 
> On Monday, 26 August 2019 3:36:58 PM AEST Andrew Jeffery wrote:
> > Currently pdbg implements a "cooked" CFAM addressing mode where we have to
> > unscramble the address provided on the commandline before accessing the
> > CFAM itself. Introduce an environment variable, PDBG_CFAM_ADDRESS_MODE,
> > that allows switching between "cooked" addressing and "raw" addressing,
> > where "raw" is the intuitive byte-offset address of registers inside the
> > CFAM address space. If the environment variable is undefined libpdbg
> > will output a warning then continue on the assumption that the address
> > is passed in cooked form. The assumption of cooked mode may change in
> > future releases once adoption of the environment variable is high.
> > 
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> >  Makefile.am          |  2 ++
> >  libpdbg/addressing.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
> >  libpdbg/addressing.h | 23 ++++++++++++++
> >  libpdbg/bmcfsi.c     |  5 ++-
> >  libpdbg/kernel.c     |  5 +--
> >  5 files changed, 105 insertions(+), 3 deletions(-)
> >  create mode 100644 libpdbg/addressing.c
> >  create mode 100644 libpdbg/addressing.h
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 011e686f1bd8..23e51bb67ef3 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -144,6 +144,8 @@ libcronus_la_SOURCES = \
> >  
> >  libpdbg_la_SOURCES = \
> >  	$(DT_sources) \
> > +	libpdbg/addressing.c \
> > +	libpdbg/addressing.h \
> >  	libpdbg/adu.c \
> >  	libpdbg/backend.h \
> >  	libpdbg/bitutils.h \
> > diff --git a/libpdbg/addressing.c b/libpdbg/addressing.c
> > new file mode 100644
> > index 000000000000..14aec2347be2
> > --- /dev/null
> > +++ b/libpdbg/addressing.c
> > @@ -0,0 +1,73 @@
> > +/* Copyright 2019 IBM Corp.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + * 	http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > + * implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +#include <stdlib.h>
> > +#include <strings.h>
> > +
> > +#include "addressing.h"
> > +#include "debug.h"
> > +
> > +#define PDBG_CFAM_ADDRESS_MODE "PDBG_CFAM_ADDRESS_MODE"
> > +
> > +static void cfam_addr_help(void)
> > +{
> > +	pdbg_log(PDBG_ERROR,
> > +"The following CFAM addressing modes are supported:\n"
> > +"\n"
> > +"'"PDBG_CFAM_ADDRESS_MODE"=help': Print this help and exit\n"
> > +"'"PDBG_CFAM_ADDRESS_MODE"=raw': Byte-based engine addressing\n"
> > +"'"PDBG_CFAM_ADDRESS_MODE"=cooked': Register-indexed engine addressing\n"
> > +"\n"
> > +"Each engine is allocated 1kiB of address space in the CFAM. 'cooked'\n"
> > +"addressing mode requires the base address of the engine in the CFAM's\n"
> > +"address space be OR'ed with the 4-byte index of the register of 
> interest\n");
> > +}
> > +
> > +static int cfam_addr_warned;
> > +
> > +uint32_t cfam_addr(uint32_t addr)
> > +{
> > +	char *mode;
> > +
> > +	/* PDBG_CFAM_ADDRESS_MODE=raw|cooked|help */
> > +	mode = getenv("PDBG_CFAM_ADDRESS_MODE");
> 
> This needs to happen in the client application (ie. pdbg). Otherwise an 
> application such as istep0 that is written assuming raw addresses will end up 
> doing totally the wrong thing if cooked addresses are selected from the 
> environment variable, and it won't have any way of figuring that out.

Right, it's easy to wind up with addressing mode confusion and break things -
that's a fair point. However we do have setenv() :trollface: (not seriously
suggesting that).

On a bit of a tangent, I don't see how istep0 could currently be written using
raw addresses as the cooked processing was unconditional?

> 
> > +	if (!mode) {
> > +		if (!cfam_addr_warned) {
> > +			pdbg_log(PDBG_ERROR,
> > +"Please specify an explicit CFAM addressing mode using the\n"
> > +PDBG_CFAM_ADDRESS_MODE " environment variable. Defaulting to 'cooked'.\n"
> > +"The default may change to 'raw' in future releases\n");
> > +			pdbg_log(PDBG_ERROR, "\n");
> > +			cfam_addr_help();
> > +			cfam_addr_warned = 1;
> > +		}
> > +		mode = "cooked";
> > +	}
> > +
> > +	if (!strcasecmp("raw", mode)) {
> > +		return addr;
> > +	} else if (!strcasecmp("cooked", mode)) {
> > +		return ((addr & 0x7ffc00) | ((addr & 0x3ff) << 2));
> > +	} else if (!strcasecmp("help", mode)) {
> > +		cfam_addr_help();
> > +		/* A little rude in library code... */
> > +		exit(EXIT_SUCCESS);
> > +	} else {
> > +		cfam_addr_help();
> > +		/* A little rude in library code... */
> > +		exit(EXIT_FAILURE);
> > +	}
> > +}
> 
> I think this is the wrong layer for this to be in. The addressing mode should 
> be part of the API exported, which also helps avoids the problem of doing rude 
> things in library code :-)
> 
> In other words I think we need to add two new functions - 
> fsi_read_byte_address() and fsi_write_byte_address() for example. The client 
> application can then call the correct one based on an environment or command-
> line option.

Yep, I think that's a better approach, much more explicit than environment variable
magic.

Andrew