[Pdbg] [RFC PATCH] libpdbg: Handle cooked/raw CFAM addressing modes
Alistair Popple
alistair at popple.id.au
Mon Aug 26 21:39:06 AEST 2019
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 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.
> + 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) {
>
More information about the Pdbg
mailing list