[Skiboot] [PATCH 1/7] Add hwprobe: no longer hard code probe order
Nicholas Piggin
npiggin at gmail.com
Tue Feb 2 19:13:06 AEDT 2021
Excerpts from Stewart Smith's message of January 25, 2021 10:58 am:
> hwprobe is a little system to have different hardware probing modules
> run in the dependency order they choose rather than hard coding
> that order in core/init.c
This is pretty cool, I like it.
>
> Signed-off-by: Stewart Smith <stewart at flamingspork.com>
> ---
> core/Makefile.inc | 1 +
> core/hwprobe.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++
> core/init.c | 12 ++------
> hw/npu.c | 8 ++++++
> hw/npu2-common.c | 8 ++++++
> hw/npu3.c | 8 ++++++
> hw/phb3.c | 6 +++-
> hw/phb4.c | 6 ++++
> include/skiboot.h | 29 +++++++++++++++++++-
> skiboot.lds.S | 6 ++++
> 10 files changed, 142 insertions(+), 12 deletions(-)
> create mode 100644 core/hwprobe.c
>
> diff --git a/core/Makefile.inc b/core/Makefile.inc
> index 829800e5..f80019b6 100644
> --- a/core/Makefile.inc
> +++ b/core/Makefile.inc
> @@ -13,6 +13,7 @@ CORE_OBJS += timer.o i2c.o rtc.o flash.o sensor.o ipmi-opal.o
> CORE_OBJS += flash-subpartition.o bitmap.o buddy.o pci-quirk.o powercap.o psr.o
> CORE_OBJS += pci-dt-slot.o direct-controls.o cpufeatures.o
> CORE_OBJS += flash-firmware-versions.o opal-dump.o
> +CORE_OBJS += hwprobe.o
>
> ifeq ($(SKIBOOT_GCOV),1)
> CORE_OBJS += gcov-profiling.o
> diff --git a/core/hwprobe.c b/core/hwprobe.c
> new file mode 100644
> index 00000000..de331af4
> --- /dev/null
> +++ b/core/hwprobe.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +/* Copyright 2021 Stewart Smith */
> +
> +#define pr_fmt(fmt) "HWPROBE: " fmt
> +#include <skiboot.h>
> +#include <string.h>
> +
> +static bool hwprobe_deps_satisfied(const struct hwprobe *hwp)
> +{
> + struct hwprobe *hwprobe;
> + const char *dep;
> + unsigned int i;
> +
> + if (hwp->deps == NULL)
> + return true;
> +
> + dep = hwp->deps[0];
> +
> + prlog(PR_TRACE, "Checking deps for %s\n", hwp->name);
> +
> + while (dep != NULL) {
> + prlog(PR_TRACE, "Checking %s dep %s\n", hwp->name, dep);
> + hwprobe = &__hwprobes_start;
> + for (i = 0; &hwprobe[i] < &__hwprobes_end; i++) {
> + if(strcmp(hwprobe[i].name,dep) == 0 &&
> + !hwprobe[i].probed)
> + return false;
> + }
> + dep++;
> + }
> +
> + prlog(PR_TRACE, "deps for %s are satisfied!\n", hwp->name);
> + return true;
> +
> +}
> +
> +void probe_hardware(void)
> +{
> + struct hwprobe *hwprobe;
> + unsigned int i;
> + bool work_todo = true;
> + bool did_something = true;
> +
> + while (work_todo) {
> + work_todo = false;
> + did_something = false;
> + hwprobe = &__hwprobes_start;
> + prlog(PR_DEBUG, "Begin loop\n");
> + for (i = 0; &hwprobe[i] < &__hwprobes_end; i++) {
> + if (hwprobe[i].probed)
> + continue;
> + if (hwprobe_deps_satisfied(&hwprobe[i])) {
> + prlog(PR_DEBUG, "Probing %s...\n", hwprobe[i].name);
> + if (hwprobe[i].probe)
> + hwprobe[i].probe();
> + did_something = true;
> + hwprobe[i].probed = true;
> + } else {
> + prlog(PR_DEBUG, "Dependencies for %s not yet satisfied, skipping\n",
> + hwprobe[i].name);
> + work_todo = true;
> + }
> + }
> +
> + if (work_todo && !did_something) {
> + prlog(PR_ERR, "Cannot satisfy dependencies! Bailing out\n");
> + break;
> + }
You could actually call probe_hardware multiple times at different
stages of boot if you had core dependencies that needed to come up in
different orders too (e.g., console, memory allocator, SMP), so the
probe could stop when it runs out of work to do, until the core code
adds a new facility (handwaving and thinking big here, but starting
small and not over-engineered is the right way to start of course).
> + }
> +}
> diff --git a/core/init.c b/core/init.c
> index 4cb8f989..92811aa4 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -1325,16 +1325,8 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
> /* Init In-Memory Collection related stuff (load the IMC dtb into memory) */
> imc_init();
>
> - /* Probe PHB3 on P8 */
> - probe_phb3();
> -
> - /* Probe PHB4 on P9 */
> - probe_phb4();
> -
> - /* Probe NPUs */
> - probe_npu();
> - probe_npu2();
> - probe_npu3();
Split out adding users into a later patch after the infrastructure and
initial probe_hardware call?
> + /* Probe all hardware we have code linked for (PHBs, NPUs etc)*/
> + probe_hardware();
>
> /* Initialize PCI */
> pci_init_slots();
> diff --git a/hw/npu.c b/hw/npu.c
> index dba7ee50..b747df2d 100644
> --- a/hw/npu.c
> +++ b/hw/npu.c
> @@ -1691,3 +1691,11 @@ void probe_npu(void)
> dt_for_each_compatible(dt_root, np, "ibm,power8-npu-pciex")
> npu_create_phb(np);
> }
> +
> +static const char* npu_deps[] = { "phb3", NULL };
> +
> +DECLARE_HWPROBE(npu) = {
> + .name = "npu",
> + .probe = probe_npu,
> + .deps = npu_deps
> +};
You could do .deps = (const char *[]){ "phb3", NULL },
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index 3bc9bcee..aa71e16c 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -679,3 +679,11 @@ void probe_npu2(void)
> setup_devices(npu);
> }
> }
> +
> +static const char* npu2_deps[] = { "phb4", NULL };
> +
> +DECLARE_HWPROBE(npu2) = {
> + .name = "npu2",
> + .probe = probe_npu2,
> + .deps = npu2_deps
> +};
> diff --git a/hw/npu3.c b/hw/npu3.c
> index 03461373..b0b957a7 100644
> --- a/hw/npu3.c
> +++ b/hw/npu3.c
> @@ -547,3 +547,11 @@ void probe_npu3(void)
> npu3_init(npu);
> }
> }
> +
> +static const char* npu3_deps[] = { "phb4", NULL };
> +
> +DECLARE_HWPROBE(npu3) = {
> + .name = "npu3",
> + .probe = probe_npu3,
> + .deps = npu3_deps
> +};
> diff --git a/hw/phb3.c b/hw/phb3.c
> index 8af6b616..d1ce8b97 100644
> --- a/hw/phb3.c
> +++ b/hw/phb3.c
> @@ -5049,4 +5049,8 @@ void probe_phb3(void)
> phb3_create(np);
> }
>
> -
> +DECLARE_HWPROBE(phb3) = {
> + .name = "phb3",
> + .probe = probe_phb3,
> + .deps = NULL
> +};
> diff --git a/hw/phb4.c b/hw/phb4.c
> index e7758d34..9060335a 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -6082,3 +6082,9 @@ void probe_phb4(void)
> dt_for_each_compatible(dt_root, np, "ibm,power9-pciex")
> phb4_create(np);
> }
> +
> +DECLARE_HWPROBE(phb4) = {
> + .name = "phb4",
> + .probe = probe_phb4,
> + .deps = NULL
> +};
> diff --git a/include/skiboot.h b/include/skiboot.h
> index d33c0250..e2274041 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> -/* Copyright 2013-2019 IBM Corp. */
> +/* Copyright 2013-2019 IBM Corp.
> + * Copyright 2021 Stewart Smith
> + */
>
> #ifndef __SKIBOOT_H
> #define __SKIBOOT_H
> @@ -345,4 +347,29 @@ extern int fake_nvram_info(uint32_t *total_size);
> extern int fake_nvram_start_read(void *dst, uint32_t src, uint32_t len);
> extern int fake_nvram_write(uint32_t offset, void *src, uint32_t size);
>
> +/*
> + * A bunch of hardware needs to be probed, sometimes in a particular order.
> + * Very simple dependency graph, with a even simpler way to resolve it.
> + * But it means we can now at link time choose what hardware we support.
> + */
> +struct hwprobe {
> + const char *name;
> + void (*probe)(void);
> +
> + bool probed; /* Internal use only */
> +
> + /*
> + * Must be NULL terminated list of strings
> + */
> + const char **deps;
> +};
> +
> +#define DECLARE_HWPROBE(name)\
> +static const struct hwprobe __used __section(".hwprobes") name ##_hwprobe
How about this-ish?
#define DECLARE_HWPROBE(__name, __probe, ...) \
static const struct hwprobe __used __section(".hwprobes") hwprobe_##__name = { \
.name = #__name, \
.probe = __probe, \
.deps = (const char *[]){ __VA_ARGS__ , NULL }, \
}
DECLARE_HWPROBE(npu2, probe_npu2, "phb4");
Thanks,
Nick
More information about the Skiboot
mailing list