[Pdbg] [PATCH v2 1/2] libpdbg: Add default device-tree selection to libpdbg
Amitay Isaacs
amitay at ozlabs.org
Tue Jul 2 17:52:26 AEST 2019
Hi Alistair,
I could not apply the patch to the current master.
Few comments inline below.
On Tue, 2019-07-02 at 16:02 +1000, Alistair Popple wrote:
> For portability reasons libpdbg intentionally did not deal with the
> selection and retrieval of the device tree for a given system. This
> means that by design each application was responsible for selecting
> and loading the correct device tree for the system it was on.
>
> This is convenient for embedded systems as it removes the dependency
> on some libc functions such as open. However there are currently no
> embedded users of libpdbg and forcing end user applications to select
> the correct device tree is likely to be error prone and inconsistent.
>
> Instead this patch adds the ability for applications to call
> pdbg_targets_init(NULL) and have libpdbg attempt to detect and load
> the correct device tree for the current system. This doesn't change
> any existing behaviour as applications may still pass an explicit
> device-tree to pdbg_targets_init() which will always be used.
>
> An environment variable is provided so users may override the
> auto-detected device-tree when the application does not provide one.
>
> This patch will bloat the overall size of the library as it will now
> include all the device-trees. It also likely makes things harder for
> embedded users. However if either of these is an issue we should be
> able to easily add a build time option to exclude this and/or only
> build in specific device-trees.
>
> Signed-off-by: Alistair Popple <alistair at popple.id.au>
> ---
> .gitignore | 1 +
> Makefile.am | 24 ++++-
> configure.ac | 1 +
> libpdbg/device.c | 10 ++
> libpdbg/dtb.c | 270
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> libpdbg/i2c.c | 6 +-
> libpdbg/libpdbg.h | 5 +
> libpdbg/target.h | 2 +
> 8 files changed, 314 insertions(+), 5 deletions(-)
> create mode 100644 libpdbg/dtb.c
>
> diff --git a/.gitignore b/.gitignore
> index 79f1ef9..13f21ff 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -35,6 +35,7 @@ m4
> *.dtsi
> *.dts
> *.dt.h
> +*.dtb.S
> optcmd_test
> *.trs
> *.log
> diff --git a/Makefile.am b/Makefile.am
> index 10c56b2..5cb0b6f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -55,20 +55,23 @@ EXTRA_DIST = \
> tests/test_driver.sh \
> $(PDBG_TESTS)
>
> -if TARGET_ARM
> DT_ARM = p8-fsi.dts p8-i2c.dts p8-kernel.dts \
> p9w-fsi.dts p9r-fsi.dts p9z-fsi.dts p9-kernel.dts
> +
> +if TARGET_ARM
> ARCH_FLAGS="-DTARGET_ARM=1"
> endif
>
> -if TARGET_PPC
> DT_PPC = p8-host.dts p9-host.dts
> +
> +if TARGET_PPC
> ARCH_FLAGS="-DTARGET_PPC=1"
> endif
>
> DT = fake.dts $(DT_ARM) $(DT_PPC)
>
> -DT_objects = $(DT:.dts=.dtb.o)
> +DT_objects = $(DT:.dts=.dtb.lo)
> +DT_sources = $(DT:.dts=.dtb.S)
> DT_headers = $(DT:.dts=.dt.h)
We don't need DT_objects anymore.
>
> optcmd_test_SOURCES = src/optcmd.c src/parsers.c
> src/tests/optcmd_test.c
> @@ -127,6 +130,7 @@ pdbg_CFLAGS += -DDISABLE_GDBSERVER
> endif
>
> src/main.c: $(DT_headers)
> +libpdbg/dtb.c: $(DT_headers)
>
> src/pdbg-gdb_parser.$(OBJEXT): CFLAGS+=-Wno-unused-const-variable
>
> @@ -149,6 +153,7 @@ libpdbg_la_SOURCES = \
> libpdbg/debug.c \
> libpdbg/debug.h \
> libpdbg/device.c \
> + libpdbg/dtb.c \
> libpdbg/fake.c \
> libpdbg/host.c \
> libpdbg/htm.c \
> @@ -166,10 +171,16 @@ libpdbg_la_SOURCES = \
> libpdbg/target.h \
> libpdbg/xbus.c
>
> +libpdbg_la_LIBADD = $(DT_objects)
> +
> if BUILD_LIBFDT
> -libpdbg_la_LIBADD = libfdt/libfdt.la
> +libpdbg_la_LIBADD += libfdt/libfdt.la
> endif
Instead of adding $(DT_objects) to libpdbg_la_LIBADD, better way is to
add $(DT_sources) to libpdbg_la_SOURCES.
>
> +noinst_LTLIBRARIES = libdtb.la
> +
> +libdtb_la_SOURCES = $(DT_sources)
> +
This is not really required. (In the sample patch I sent, I did not
want to change the build for libpdbg, so added a temporary new library
for testing.)
> include_HEADERS = libpdbg/libpdbg.h
>
> noinst_LIBRARIES = libccan.a
> @@ -242,6 +253,11 @@ p9z-fsi.dts: p9z-fsi.dts.m4 p9-fsi.dtsi
> %.dt.h: %.dts
> $(GEN_V)$(srcdir)/generate_dt_header.sh $< > $@
>
> +%.dtb.S: %.dtb
> + $(AM_V_CPPAS)$(CPP) $(srcdir)/template.S \
> + -DSYMBOL_PREFIX=$(shell basename $< .S | tr '.-' '_')_o
> -DFILENAME=\"$<\" -o \
> + $@
> +
> %.dtb.o: %.dtb
> $(AM_V_CC)$(CC) -c $(srcdir)/template.S -DSYMBOL_PREFIX=$(shell
> basename $@ | tr '.-' '_') -DFILENAME=\"$<\" -o $@
We can drop the rule for %.dtb.o, as automake will generate the rules
for building $(DT_sources).
>
> diff --git a/configure.ac b/configure.ac
> index c62ec05..c41e13a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -3,6 +3,7 @@ AM_INIT_AUTOMAKE([subdir-objects])
> AM_SILENT_RULES([yes])
>
> AC_PROG_CC
> +AM_PROG_AS
> AC_PROG_LIBTOOL
>
> AC_PATH_PROG([M4], [m4])
> diff --git a/libpdbg/device.c b/libpdbg/device.c
> index b7fd49f..ba9e05e 100644
> --- a/libpdbg/device.c
> +++ b/libpdbg/device.c
> @@ -638,7 +638,17 @@ uint64_t pdbg_target_address(struct pdbg_target
> *target, uint64_t *out_size)
>
> void pdbg_targets_init(void *fdt)
> {
> + /* Root node needs to be valid when this function returns */
> pdbg_dt_root = dt_new_node("", NULL, 0);
> +
> + if (!fdt)
> + fdt = pdbg_default_dtb();
> +
> + if (!fdt) {
> + pdbg_log(PDBG_ERROR, "Could not find a system device
> tree\n");
> + return;
> + }
> +
> dt_expand(fdt);
> }
>
> diff --git a/libpdbg/dtb.c b/libpdbg/dtb.c
> new file mode 100644
> index 0000000..725fbb3
> --- /dev/null
> +++ b/libpdbg/dtb.c
> @@ -0,0 +1,270 @@
> +/* 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 <stdio.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/mman.h>
> +#include <assert.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +
> +#include "libpdbg.h"
> +
> +#include "fake.dt.h"
> +
> +#include "p8-i2c.dt.h"
> +#include "p8-fsi.dt.h"
> +#include "p8-kernel.dt.h"
> +#include "p9w-fsi.dt.h"
> +#include "p9r-fsi.dt.h"
> +#include "p9z-fsi.dt.h"
> +#include "p9-kernel.dt.h"
> +#include "p8-host.dt.h"
> +#include "p9-host.dt.h"
> +
> +#define AMI_BMC "/proc/ractrends/Helper/FwInfo"
> +#define OPENFSI_BMC "/sys/bus/platform/devices/gpio-fsi/fsi0/"
> +#define FSI_CFAM_ID "/sys/devices/platform/gpio-fsi/fsi0/slave at 00:00
> /cfam_id"
> +#define XSCOM_BASE_PATH "/sys/kernel/debug/powerpc/scom"
> +
> +#define CHIP_ID_P8 0xea
> +#define CHIP_ID_P8P 0xd3
> +#define CHIP_ID_P9 0xd1
> +
> +static enum pdbg_backend pdbg_backend = PDBG_DEFAULT_BACKEND;
> +static const char *pdbg_backend_option;
> +
> +/* Determines the most appropriate backend for the host system we
> are
> + * running on. */
> +static enum pdbg_backend default_backend(void)
> +{
> + int rc;
> +
> + rc = access(XSCOM_BASE_PATH, F_OK);
> + if (rc == 0) /* PowerPC Host System */
> + return PDBG_BACKEND_HOST;
> +
> + rc = access(AMI_BMC, F_OK);
> + if (rc == 0) /* AMI BMC */
> + return PDBG_BACKEND_I2C;
> +
> + rc = access(OPENFSI_BMC, F_OK);
> + if (rc == 0) /* Kernel interface. OpenBMC */
> + return PDBG_BACKEND_KERNEL;
> +
> + return PDBG_BACKEND_FAKE;
> +}
> +
> +/* Try and determine what system type we are on by reading
> + * /proc/cpuinfo */
> +static void *ppc_target(void)
> +{
> + const char *pos = NULL;
> + char line[256];
> + FILE *cpuinfo;
> +
> + if (!strcmp(pdbg_backend_option, "p8"))
> + return &_binary_p8_host_dtb_o_start;
> + else if (!strcmp(pdbg_backend_option, "p9"))
> + return &_binary_p9_host_dtb_o_start;
> +
> + cpuinfo = fopen("/proc/cpuinfo", "r");
> + if (!cpuinfo)
> + return NULL;
> +
> + while ((pos = fgets(line, sizeof(line), cpuinfo)))
> + if (strncmp(line, "cpu", 3) == 0)
> + break;
> + fclose(cpuinfo);
> +
> + if (!pos) {
> + /* Got to EOF without a break */
> + pdbg_log(PDBG_ERROR, "Unable to parse
> /proc/cpuinfo\n");
> + return NULL;
> + }
> +
> + pos = strchr(line, ':');
> + if (!pos || (*(pos + 1) == '\0')) {
> + pdbg_log(PDBG_ERROR, "Unable to parse
> /proc/cpuinfo\n");
> + return NULL;
> + }
> +
> + pos += 2;
> +
> + if (strncmp(pos, "POWER8", 6) == 0) {
> + pdbg_log(PDBG_INFO, "Found a POWER8 PPC host
> system\n");
> + return &_binary_p8_host_dtb_o_start;
> + }
> +
> + if (strncmp(pos, "POWER9", 6) == 0) {
> + pdbg_log(PDBG_INFO, "Found a POWER9 PPC host
> system\n");
> + return &_binary_p9_host_dtb_o_start;
> + }
> +
> + pdbg_log(PDBG_ERROR, "Unsupported CPU type '%s'\n", pos);
> + return NULL;
> +}
> +
> +static void *bmc_target(void)
> +{
> + FILE *cfam_id_file;
> + uint32_t cfam_id = 0;
> + uint32_t chip_id = 0;
> + int rc;
> +
> + if (!pdbg_backend_option) {
> + /* Try and determine the correct device type */
> + cfam_id_file = fopen(FSI_CFAM_ID, "r");
> + if (!cfam_id_file) {
> + pdbg_log(PDBG_ERROR, "Unabled to open CFAM ID
> file\n");
> + return NULL;
> + }
> +
> + rc = fscanf(cfam_id_file, "0x%" PRIx32, &cfam_id);
> + if (rc != 1) {
> + pdbg_log(PDBG_ERROR, "Unable to read CFAM ID:
> %s", strerror(errno));
> + }
> + fclose(cfam_id_file);
> + chip_id = (cfam_id >> 4) & 0xff;
> + } else {
> + if (!strcmp(pdbg_backend_option, "p9"))
> + chip_id = CHIP_ID_P9;
> + else if (!strcmp(pdbg_backend_option, "p8"))
> + chip_id = CHIP_ID_P8;
> + else
> + pdbg_log(PDBG_WARNING, "Invalid OpenBMC system
> type '%s' specified\n",
> + pdbg_backend_option);
> + }
> +
> + switch(chip_id) {
> + case CHIP_ID_P9:
> + pdbg_log(PDBG_INFO, "Found a POWER9 OpenBMC based
> system\n");
> + return &_binary_p9_kernel_dtb_o_start;
> + break;
> +
> + case CHIP_ID_P8:
> + case CHIP_ID_P8P:
> + pdbg_log(PDBG_INFO, "Found a POWER8/8+ OpenBMC based
> system\n");
> + return &_binary_p8_kernel_dtb_o_start;
> + break;
> +
> + default:
> + pdbg_log(PDBG_ERROR, "Unrecognised Chip ID register
> 0x%08" PRIx32 "\n", chip_id);
> + return NULL;
> + }
> +}
> +
> +/* Opens a dtb at the given path */
> +static void *mmap_dtb(char *file)
> +{
> + int fd;
> + void *dtb;
> + struct stat statbuf;
> +
> + fd = open(file, O_RDONLY);
> + if (fd < 0) {
> + pdbg_log(PDBG_ERROR, "Unable to open dtb file '%s'\n",
> file);
> + return NULL;
> + }
> +
> + if (fstat(fd, &statbuf) == -1) {
> + pdbg_log(PDBG_ERROR, "Failed to stat file '%s'\n",
> file);
> + goto fail;
> + }
> +
> + dtb = mmap(NULL, statbuf.st_size, PROT_READ, MAP_PRIVATE, fd,
> 0);
> + if (dtb == MAP_FAILED) {
> + pdbg_log(PDBG_ERROR, "Failed top mmap file '%s'\n",
> file);
> + goto fail;
> + }
> +
> + return dtb;
> +
> +fail:
> + close(fd);
> + return NULL;
> +}
> +
> +int pdbg_set_backend(enum pdbg_backend backend, const char
> *backend_option)
> +{
> + pdbg_backend = backend;
> + pdbg_backend_option = backend_option;
> +
> + return 0;
> +}
This can be a void function if we are not verifying the arguments.
> +
> +const char *pdbg_get_backend_option(void)
> +{
> + return pdbg_backend_option;
> +}
> +
> +/* Determines what platform we are running on and returns a pointer
> to
> + * the fdt that is most likely to work on the system. */
> +void *pdbg_default_dtb(void)
> +{
> + char *dtb = getenv("LPDBG_DTB");
Any reason why we don't just use PDBG_DTB? Or if you want to make it
explicit that it's a library DTB, then LIBPDBG_DTB might be better.
> +
> + if (!pdbg_backend && dtb)
> + return mmap_dtb(dtb);
> +
> + if (!pdbg_backend)
> + pdbg_backend = default_backend();
What happens if pdbg_backend and dtb, both are set? From the code it
just ignores dtb. May be we should log something, so user knows what
happened.
> +
> + switch(pdbg_backend) {
> + case PDBG_BACKEND_HOST:
> + return ppc_target();
> + break;
> +
> + case PDBG_BACKEND_I2C:
> + /* I2C is only supported on POWER8 */
> + pdbg_log(PDBG_INFO, "Found a POWER8 AMI BMC based
> system\n");
> + return &_binary_p8_i2c_dtb_o_start;
> + break;
> +
> + case PDBG_BACKEND_KERNEL:
> + return bmc_target();
> + break;
> +
> + case PDBG_BACKEND_FSI:
> + if (!strcmp(pdbg_backend_option, "p8"))
> + return &_binary_p8_fsi_dtb_o_start;
> + else if (!strcmp(pdbg_backend_option, "p9w"))
> + return &_binary_p9w_fsi_dtb_o_start;
> + else if (!strcmp(pdbg_backend_option, "p9r"))
> + return &_binary_p9r_fsi_dtb_o_start;
> + else if (!strcmp(pdbg_backend_option, "p9z"))
> + return &_binary_p9z_fsi_dtb_o_start;
> + else {
> + pdbg_log(PDBG_WARNING, "Invalid device type
> specified, using a fake one\n");
> + return &_binary_fake_dtb_o_start;
> + }
> +
> + break;
> +
> + default:
> + pdbg_log(PDBG_WARNING, "Unable to determine a valid
> default backend, using a fake one for testing purposes\n");
> + /* Fall through */
> +
> + case PDBG_BACKEND_FAKE:
> + return &_binary_fake_dtb_o_start;
> + break;
> + }
> +}
> diff --git a/libpdbg/i2c.c b/libpdbg/i2c.c
> index 227c8a0..1a5d089 100644
> --- a/libpdbg/i2c.c
> +++ b/libpdbg/i2c.c
> @@ -131,7 +131,11 @@ int i2c_target_probe(struct pdbg_target *target)
> const char *bus;
> int addr;
>
> - bus = pdbg_target_property(&pib->target, "bus", NULL);
> + bus = pdbg_get_backend_option();
> +
> + if (!bus)
> + bus = pdbg_target_property(&pib->target, "bus", NULL);
> +
> if (!bus)
> bus = "/dev/i2c4";
>
> diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> index 14a41ab..ae763e5 100644
> --- a/libpdbg/libpdbg.h
> +++ b/libpdbg/libpdbg.h
> @@ -52,6 +52,9 @@ enum pdbg_target_status {PDBG_TARGET_UNKNOWN = 0,
> PDBG_TARGET_ENABLED,
> PDBG_TARGET_DISABLED, PDBG_TARGET_MUSTEXIST,
> PDBG_TARGET_NONEXISTENT,
> PDBG_TARGET_RELEASED};
>
> +enum pdbg_backend { PDBG_DEFAULT_BACKEND = 0, PDBG_BACKEND_FSI,
> PDBG_BACKEND_I2C,
> + PDBG_BACKEND_KERNEL, PDBG_BACKEND_FAKE,
> PDBG_BACKEND_HOST };
> +
> #define pdbg_for_each_compatible(parent, target, compat)
> \
> for (target =
> NULL; \
> (target = __pdbg_next_compatible_node(parent, target,
> compat)) != NULL;)
> @@ -103,6 +106,8 @@ enum pdbg_target_status pdbg_target_probe(struct
> pdbg_target *target);
> void pdbg_target_release(struct pdbg_target *target);
> enum pdbg_target_status pdbg_target_status(struct pdbg_target
> *target);
> void pdbg_target_status_set(struct pdbg_target *target, enum
> pdbg_target_status status);
> +int pdbg_set_backend(enum pdbg_backend backend, const char
> *backend_option);
> +void *pdbg_default_dtb(void);
> uint32_t pdbg_target_index(struct pdbg_target *target);
> char *pdbg_target_path(const struct pdbg_target *target);
> struct pdbg_target *pdbg_target_from_path(struct pdbg_target
> *target, const char *path);
> diff --git a/libpdbg/target.h b/libpdbg/target.h
> index 233f085..193c0e0 100644
> --- a/libpdbg/target.h
> +++ b/libpdbg/target.h
> @@ -61,4 +61,6 @@ bool pdbg_target_is_class(struct pdbg_target
> *target, const char *class);
> extern struct list_head empty_list;
> extern struct list_head target_classes;
>
> +const char *pdbg_get_backend_option(void);
> +
> #endif
> --
> 2.11.0
>
Amitay.
--
Anyone who slaps a 'this page is best viewed with Browser X' label on
a Web page appears to be yearning for the bad old days, before the Web,
when you had very little chance of reading a document written on another
computer, another word processor, or another network. - Tim Berners-Lee
More information about the Pdbg
mailing list