diff mbox series

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

Message ID 20190826053658.23879-1-andrew@aj.id.au
State New
Headers show
Series [RFC] libpdbg: Handle cooked/raw CFAM addressing modes | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (e005e0863a44f04c931b09804f258fb1c0b6f14c)
snowpatch_ozlabs/build-multiarch success Test build-multiarch on branch master

Commit Message

Andrew Jeffery Aug. 26, 2019, 5:36 a.m. UTC 
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@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

Comments

Alistair Popple Aug. 26, 2019, 11:39 a.m. UTC | #1 
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.

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.

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

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

- Alistair

> diff --git a/libpdbg/addressing.h b/libpdbg/addressing.h
> new file mode 100644
> index 000000000000..56a5fcc93a06
> --- /dev/null
> +++ b/libpdbg/addressing.h
> @@ -0,0 +1,23 @@
> +/* 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.
> + */
> +#ifndef __ADDRESSING_H
> +#define __ADDRESSING_H
> +
> +#include <stdint.h>
> +
> +uint32_t cfam_addr(uint32_t addr);
> +
> +#endif
> diff --git a/libpdbg/bmcfsi.c b/libpdbg/bmcfsi.c
> index 1d2e304ea2ef..011da9a62222 100644
> --- a/libpdbg/bmcfsi.c
> +++ b/libpdbg/bmcfsi.c
> @@ -25,6 +25,7 @@
>  #include <assert.h>
>  #include <inttypes.h>
>  
> +#include "addressing.h"
>  #include "bitutils.h"
>  #include "operations.h"
>  #include "hwunit.h"
> @@ -206,10 +207,12 @@ static uint64_t fsi_abs_ar(uint32_t addr, int read)
>  {
>  	uint32_t slave_id = (addr >> 21) & 0x3;
>  
> +	addr = cfam_addr(addr & 0x1fffff);
> +
>  	/* Reformat the address. I'm not sure I fully understand this
>  	 * yet but we basically shift the bottom byte and add 0b01
>  	 * (for the write word?) */
> -       	addr = ((addr & 0x1ffc00) | ((addr & 0x3ff) << 2)) << 1;
> +	addr <<= 1;
>  	addr |= 0x3;
>  	addr |= slave_id << 26;
>  	addr |= (0x8ULL | !!(read)) << 22;
> diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> index 4cc0334d7eb3..c71486ed0a85 100644
> --- a/libpdbg/kernel.c
> +++ b/libpdbg/kernel.c
> @@ -25,6 +25,7 @@
>  #include <inttypes.h>
>  #include <endian.h>
>  
> +#include "addressing.h"
>  #include "bitutils.h"
>  #include "operations.h"
>  #include "hwunit.h"
> @@ -37,7 +38,7 @@ int fsi_fd;
>  static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t 
*value)
>  {
>  	int rc;
> -	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
> +	uint32_t tmp, addr = cfam_addr(addr64);
>  
>  	rc = lseek(fsi_fd, addr, SEEK_SET);
>  	if (rc < 0) {
> @@ -64,7 +65,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t 
addr64, uint32_t *value)
>  static int kernel_fsi_putcfam(struct fsi *fsi, uint32_t addr64, uint32_t 
data)
>  {
>  	int rc;
> -	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
> +	uint32_t tmp, addr = cfam_addr(addr64);
>  
>  	rc = lseek(fsi_fd, addr, SEEK_SET);
>  	if (rc < 0) {
>
Andrew Jeffery Aug. 26, 2019, 11:52 p.m. UTC | #2 
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@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
Alistair Popple Aug. 28, 2019, 3:46 a.m. UTC | #3 
> > 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).

Ha. I thought of that when I wrote this, I was just waiting for you to suggest 
it :-P

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

Sorry, as you it's easy to confuse these. I should have said cooked/fsi 
addresses.

Jeremy had a off handed idea that I think I kind of like though. Rather than 
adding a command line option to change the addressing, why don't we make it 
explicit to the address?

In other words we could keep 0x.... to mean cooked addresses and introduce 
something like 0n.... to mean normal byte addressing. I like that it forces 
users to think about what addressing they really want and removes to 
possibility of forgetting about some alias without making it too much of a 
pain to type everytime. Would appreciate other peoples thoughts though.

- Alistair

> > 
> > > +	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
>
Andrew Jeffery Aug. 29, 2019, 12:16 a.m. UTC | #4 
On Wed, 28 Aug 2019, at 13:17, Alistair Popple wrote:
>  
> > > 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).
> 
> Ha. I thought of that when I wrote this, I was just waiting for you to suggest 
> it :-P
> 
> > 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?
> 
> Sorry, as you it's easy to confuse these. I should have said cooked/fsi 
> addresses.
> 
> Jeremy had a off handed idea that I think I kind of like though. Rather than 
> adding a command line option to change the addressing, why don't we make it 
> explicit to the address?
> 
> In other words we could keep 0x.... to mean cooked addresses and introduce 
> something like 0n.... to mean normal byte addressing. I like that it forces 
> users to think about what addressing they really want and removes to 
> possibility of forgetting about some alias without making it too much of a 
> pain to type everytime. Would appreciate other peoples thoughts though.
> 

Hmm, that is an interesting idea. Pity we wouldn't be able to swap the modes
without breaking things. The purist in me feels sketchy about hijacking the
base prefix, so what about doing suffixes? Addresses aren't floating point so
we can e.g. use '.' to terminate the value as it's not a digit in any base, and then
append e.g. 'c' or 'r' for cooked or raw respectively. From there we can use 
strtoul()'s endptr to capture the suffix while still using arbitrary bases. Some
examples:

# 1. Cooked address mode for compatibility, access the second word of peek
$ pdbg -a getcfam 0x401

# 2. Explicit cooked mode, same access as above
$ pdbg -a getcfam 0x401.c

# 3. Raw mode, same access as above, base 10 because we can
$ pdbg -a getcfam 1028.r

Maybe we could still do an option/env var for managing case 1? Could be
over-doing it though.

Andrew
diff mbox series

Patch

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");
+
+	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);
+	}
+}
diff --git a/libpdbg/addressing.h b/libpdbg/addressing.h
new file mode 100644
index 000000000000..56a5fcc93a06
--- /dev/null
+++ b/libpdbg/addressing.h
@@ -0,0 +1,23 @@ 
+/* 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.
+ */
+#ifndef __ADDRESSING_H
+#define __ADDRESSING_H
+
+#include <stdint.h>
+
+uint32_t cfam_addr(uint32_t addr);
+
+#endif
diff --git a/libpdbg/bmcfsi.c b/libpdbg/bmcfsi.c
index 1d2e304ea2ef..011da9a62222 100644
--- a/libpdbg/bmcfsi.c
+++ b/libpdbg/bmcfsi.c
@@ -25,6 +25,7 @@ 
 #include <assert.h>
 #include <inttypes.h>
 
+#include "addressing.h"
 #include "bitutils.h"
 #include "operations.h"
 #include "hwunit.h"
@@ -206,10 +207,12 @@  static uint64_t fsi_abs_ar(uint32_t addr, int read)
 {
 	uint32_t slave_id = (addr >> 21) & 0x3;
 
+	addr = cfam_addr(addr & 0x1fffff);
+
 	/* Reformat the address. I'm not sure I fully understand this
 	 * yet but we basically shift the bottom byte and add 0b01
 	 * (for the write word?) */
-       	addr = ((addr & 0x1ffc00) | ((addr & 0x3ff) << 2)) << 1;
+	addr <<= 1;
 	addr |= 0x3;
 	addr |= slave_id << 26;
 	addr |= (0x8ULL | !!(read)) << 22;
diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
index 4cc0334d7eb3..c71486ed0a85 100644
--- a/libpdbg/kernel.c
+++ b/libpdbg/kernel.c
@@ -25,6 +25,7 @@ 
 #include <inttypes.h>
 #include <endian.h>
 
+#include "addressing.h"
 #include "bitutils.h"
 #include "operations.h"
 #include "hwunit.h"
@@ -37,7 +38,7 @@  int fsi_fd;
 static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
 {
 	int rc;
-	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
+	uint32_t tmp, addr = cfam_addr(addr64);
 
 	rc = lseek(fsi_fd, addr, SEEK_SET);
 	if (rc < 0) {
@@ -64,7 +65,7 @@  static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
 static int kernel_fsi_putcfam(struct fsi *fsi, uint32_t addr64, uint32_t data)
 {
 	int rc;
-	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
+	uint32_t tmp, addr = cfam_addr(addr64);
 
 	rc = lseek(fsi_fd, addr, SEEK_SET);
 	if (rc < 0) {