diff mbox series

[02/10] Introduce hwprobe facility to avoid hard-coding probe functions

Message ID 20210626023824.1124164-3-npiggin@gmail.com
State Superseded
Headers show
Series hwprobe patches | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (9d52c580d3fabbea6b276b98f925ab0fceebb96c)
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present

Commit Message

Nicholas Piggin June 26, 2021, 2:38 a.m. UTC 
From: Stewart Smith <stewart@flamingspork.com>

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       |  3 ++
 include/skiboot.h | 39 +++++++++++++++++++++++++-
 skiboot.lds.S     |  6 ++++
 5 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 core/hwprobe.c

Comments

Dan Horák June 28, 2021, 3:11 p.m. UTC | #1 
On Sat, 26 Jun 2021 12:38:16 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> From: Stewart Smith <stewart@flamingspork.com>
> 
> 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.

I already commented in January, but the special linker section plus
start/end label style is not compatible with LTO and possibly relies on
undefined behaviour in the C language. I don't think LTO is hot topic
for system firmware, but it could change. So at minimum it should be
documented in the commit message.

https://lists.ozlabs.org/pipermail/skiboot/2021-January/017470.html
https://bugzilla.redhat.com/show_bug.cgi?id=1877485#c14
https://sigrok.org/bugzilla/show_bug.cgi?id=1433


		Dan

> 
> Signed-off-by: Stewart Smith <stewart@flamingspork.com>
> ---
>  core/Makefile.inc |  1 +
>  core/hwprobe.c    | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>  core/init.c       |  3 ++
>  include/skiboot.h | 39 +++++++++++++++++++++++++-
>  skiboot.lds.S     |  6 ++++
>  5 files changed, 118 insertions(+), 1 deletion(-)
>  create mode 100644 core/hwprobe.c
> 
> diff --git a/core/Makefile.inc b/core/Makefile.inc
> index 829800e5b..f80019b6a 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 000000000..de331af48
> --- /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 d5ba67edd..ed3229172 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -1333,6 +1333,9 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
>  	probe_npu2();
>  	probe_npu3();
>  
> +	/* Probe all HWPROBE hardware we have code linked for*/
> +	probe_hardware();
> +
>  	/* Initialize PCI */
>  	pci_init_slots();
>  
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 331aa9501..d104c7a89 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
> @@ -342,4 +344,39 @@ 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.
> + * This struct should not be defined directly but with the macros.
> + */
> +struct hwprobe {
> +	const char	*name;
> +	void		(*probe)(void);
> +
> +	bool		probed;
> +
> +	/* NULL or NULL-terminated array of strings */
> +	const char	**deps;
> +};
> +
> +#define DEFINE_HWPROBE(__name, __probe)		\
> +static const struct hwprobe __used __section(".hwprobes") hwprobe_##__name = { \
> +	.name = #__name,				\
> +	.probe = __probe,				\
> +	.deps = NULL,					\
> +}
> +
> +#define DEFINE_HWPROBE_DEPS(__name, __probe, ...)	\
> +static const struct hwprobe __used __section(".hwprobes") hwprobe_##__name = { \
> +	.name = #__name,				\
> +	.probe = __probe,				\
> +	.deps = (const char *[]){ __VA_ARGS__, NULL},	\
> +}
> +
> +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 5a7f9e316..c8e6e747c 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.23.0
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Nicholas Piggin June 28, 2021, 8:23 p.m. UTC | #2 
Excerpts from Dan Horák's message of June 29, 2021 1:11 am:
> On Sat, 26 Jun 2021 12:38:16 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
>> From: Stewart Smith <stewart@flamingspork.com>
>> 
>> 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.
> 
> I already commented in January, but the special linker section plus
> start/end label style is not compatible with LTO and possibly relies on
> undefined behaviour in the C language. I don't think LTO is hot topic
> for system firmware, but it could change. So at minimum it should be
> documented in the commit message.
> 
> https://lists.ozlabs.org/pipermail/skiboot/2021-January/017470.html
> https://bugzilla.redhat.com/show_bug.cgi?id=1877485#c14
> https://sigrok.org/bugzilla/show_bug.cgi?id=1433

It looks like placing the section with the linker script as this patch 
does works okay, just not what they were doing before which seems to be
trying to define the section start/end symbols in C. AFAIKS.

The linker script method is the normal one including Linux kernel which
is being compiled with LTO AFAIK.

LTO might be interesting for skiboot for space reduction particularly.
We have a link time dead code elimination option (a basic kind of LTO)
already which can save quite a bit of space. A modern LTO might be able
to save even more.

Thanks,
Nick
Dan Horák July 1, 2021, 10:42 a.m. UTC | #3 
On Tue, 29 Jun 2021 06:23:19 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> Excerpts from Dan Horák's message of June 29, 2021 1:11 am:
> > On Sat, 26 Jun 2021 12:38:16 +1000
> > Nicholas Piggin <npiggin@gmail.com> wrote:
> > 
> >> From: Stewart Smith <stewart@flamingspork.com>
> >> 
> >> 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.
> > 
> > I already commented in January, but the special linker section plus
> > start/end label style is not compatible with LTO and possibly relies on
> > undefined behaviour in the C language. I don't think LTO is hot topic
> > for system firmware, but it could change. So at minimum it should be
> > documented in the commit message.
> > 
> > https://lists.ozlabs.org/pipermail/skiboot/2021-January/017470.html
> > https://bugzilla.redhat.com/show_bug.cgi?id=1877485#c14
> > https://sigrok.org/bugzilla/show_bug.cgi?id=1433
> 
> It looks like placing the section with the linker script as this patch 
> does works okay, just not what they were doing before which seems to be
> trying to define the section start/end symbols in C. AFAIKS.
> 
> The linker script method is the normal one including Linux kernel which
> is being compiled with LTO AFAIK.
> 
> LTO might be interesting for skiboot for space reduction particularly.
> We have a link time dead code elimination option (a basic kind of LTO)
> already which can save quite a bit of space. A modern LTO might be able
> to save even more.

thanks, Nick, this clears my concerns


		Dan
diff mbox series

Patch

diff --git a/core/Makefile.inc b/core/Makefile.inc
index 829800e5b..f80019b6a 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 000000000..de331af48
--- /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 d5ba67edd..ed3229172 100644
--- a/core/init.c
+++ b/core/init.c
@@ -1333,6 +1333,9 @@  void __noreturn __nomcount main_cpu_entry(const void *fdt)
 	probe_npu2();
 	probe_npu3();
 
+	/* Probe all HWPROBE hardware we have code linked for*/
+	probe_hardware();
+
 	/* Initialize PCI */
 	pci_init_slots();
 
diff --git a/include/skiboot.h b/include/skiboot.h
index 331aa9501..d104c7a89 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
@@ -342,4 +344,39 @@  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.
+ * This struct should not be defined directly but with the macros.
+ */
+struct hwprobe {
+	const char	*name;
+	void		(*probe)(void);
+
+	bool		probed;
+
+	/* NULL or NULL-terminated array of strings */
+	const char	**deps;
+};
+
+#define DEFINE_HWPROBE(__name, __probe)		\
+static const struct hwprobe __used __section(".hwprobes") hwprobe_##__name = { \
+	.name = #__name,				\
+	.probe = __probe,				\
+	.deps = NULL,					\
+}
+
+#define DEFINE_HWPROBE_DEPS(__name, __probe, ...)	\
+static const struct hwprobe __used __section(".hwprobes") hwprobe_##__name = { \
+	.name = #__name,				\
+	.probe = __probe,				\
+	.deps = (const char *[]){ __VA_ARGS__, NULL},	\
+}
+
+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 5a7f9e316..c8e6e747c 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 : {