[1/7] Add hwprobe: no longer hard code probe order

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

Signed-off-by: Stewart Smith <stewart@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

On Sun, 24 Jan 2021 16:58:00 -0800
Stewart Smith <stewart@flamingspork.com> wrote:

> 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

not sure if skiboot will ever switch to use LTO, but using the ELF
sections for storing data like the hwprobes here together with start/end
markers is not LTO-safe. The solution is to add an explicit "no_reorder"
attribute to the markers, please see for example
https://sigrok.org/bugzilla/show_bug.cgi?id=1433 for more details.


		Dan


> Signed-off-by: Stewart Smith <stewart@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;
> +		}
> +	}
> +}
> 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();
> +	/* 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
> +};
> 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
> +
> +extern struct hwprobe __hwprobes_start;
> +extern struct hwprobe __hwprobes_end;
> +
> +extern void probe_hardware(void);
> +
>  #endif /* __SKIBOOT_H */
> diff --git a/skiboot.lds.S b/skiboot.lds.S
> index 5a7f9e31..c8e6e747 100644
> --- a/skiboot.lds.S
> +++ b/skiboot.lds.S
> @@ -164,6 +164,12 @@ SECTIONS
>  		__platforms_end = .;
>  	}
>  
> +	.hwprobes : {
> +		__hwprobes_start = .;
> +		KEEP(*(.hwprobes))
> +		__hwprobes_end = .;
> +	}
> +
>  	/* Relocations */
>  	. = ALIGN(0x10);
>  	.dynamic : {
> -- 
> 2.29.2
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

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