[RFC PATCH 0/4] wire up CPU features to udev based module loading

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[Ard Biesheuvel]

This series implements automatic module loading based on optional CPU features,
and tries to do so in a generic way. Currently, 32 feature bits are supported,
and how they map to actual CPU features is entirely up to the architecture.

There is some GCC attribute foo in here which people may object to, so any
suggestions on how to implement this in a cleaner way are more than welcome.

Typical usage would look like this:

static struct cpu_feature mod_cpu_feature[] = {
	{ HWCAP_NEON },
	{}
};
MODULE_DEVICE_TABLE(cpu, mod_cpu_feature);

where (on the ARM arch) the module in question would be loaded automatically if
a NEON-capable CPU is detected (and advertised).

Ard Biesheuvel (4):
  x86: move arch_cpu_uevent() to generic code
  cpu: advertise CPU features over udev in a generic way
  scripts/mod: add generic CPU features as module alias
  arm64: advertise CPU features using module aliases

 arch/arm64/Kconfig                |  3 +++
 arch/arm64/kernel/setup.c         |  2 ++
 arch/x86/kernel/cpu/match.c       | 11 -----------
 drivers/base/cpu.c                | 39 ++++++++++++++++++++++++++++++++++++++-
 include/linux/cpu.h               |  1 -
 include/linux/mod_devicetable.h   | 11 +++++++++++
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 10 ++++++++++
 8 files changed, 67 insertions(+), 13 deletions(-)

-- 
1.8.3.2

[RFC PATCH 1/4] x86: move arch_cpu_uevent() to generic code

[Ard Biesheuvel]

Only x86 implements arch_cpu_uevent(), and there is nothing arch
specific about it, so move it to drivers/base/cpu.c.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/kernel/cpu/match.c | 11 -----------
 drivers/base/cpu.c          | 15 ++++++++++++++-
 include/linux/cpu.h         |  1 -
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
index 3656537..ab6082a 100644
--- a/arch/x86/kernel/cpu/match.c
+++ b/arch/x86/kernel/cpu/match.c
@@ -78,14 +78,3 @@ ssize_t arch_print_cpu_modalias(struct device *dev,
 	*buf++ = '\n';
 	return buf - bufptr;
 }
-
-int arch_cpu_uevent(struct device *dev, struct kobj_uevent_env *env)
-{
-	char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	if (buf) {
-		arch_print_cpu_modalias(NULL, NULL, buf);
-		add_uevent_var(env, "MODALIAS=%s", buf);
-		kfree(buf);
-	}
-	return 0;
-}
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 848ebbd..49c6f4b 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -275,6 +275,19 @@ static void cpu_device_release(struct device *dev)
 	 */
 }
 
+#ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE
+static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (buf) {
+		arch_print_cpu_modalias(NULL, NULL, buf);
+		add_uevent_var(env, "MODALIAS=%s", buf);
+		kfree(buf);
+	}
+	return 0;
+}
+#endif
+
 /*
  * register_cpu - Setup a sysfs device for a CPU.
  * @cpu - cpu->hotpluggable field set to 1 will generate a control file in
@@ -296,7 +309,7 @@ int register_cpu(struct cpu *cpu, int num)
 	cpu->dev.offline = !cpu_online(num);
 	cpu->dev.of_node = of_get_cpu_node(num, NULL);
 #ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE
-	cpu->dev.bus->uevent = arch_cpu_uevent;
+	cpu->dev.bus->uevent = cpu_uevent;
 #endif
 	cpu->dev.groups = common_cpu_attr_groups;
 	if (cpu->hotpluggable)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 801ff9e..c12ddd1 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -44,7 +44,6 @@ extern ssize_t arch_cpu_release(const char *, size_t);
 struct notifier_block;
 
 #ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE
-extern int arch_cpu_uevent(struct device *dev, struct kobj_uevent_env *env);
 extern ssize_t arch_print_cpu_modalias(struct device *dev,
 				       struct device_attribute *attr,
 				       char *bufptr);
-- 
1.8.3.2

[RFC PATCH 2/4] cpu: advertise CPU features over udev in a generic way

[Ard Biesheuvel]

This patch implements a generic modalias 'cpu:feature:...' which
enables CPU feature flag based module loading in a generic way.
All the arch needs to do is enable CONFIG_ARCH_HAS_CPU_AUTOPROBE
and export a u32 called 'cpu_features'. (What each bit actually
means is irrelevant on this level.)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/base/cpu.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 49c6f4b..a661d31 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -276,6 +276,30 @@ static void cpu_device_release(struct device *dev)
 }
 
 #ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE
+ssize_t print_cpu_modalias(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	extern u32 __weak cpu_features;
+	ssize_t n;
+	int i;
+	u32 f;
+
+	/*
+	 * With 32 features maximum (taking 3 bytes each to print), we don't
+	 * need to worry about overrunning the PAGE_SIZE sized buffer.
+	 */
+	n = sprintf(buf, "cpu:feature:");
+	for (f = cpu_features, i = 0; f; f >>= 1, i++)
+		if (f & 1)
+			n += sprintf(&buf[n], ",%02X", i);
+	buf[n++] = '\n';
+	return n;
+}
+
+ssize_t __attribute__((weak, alias("print_cpu_modalias")))
+arch_print_cpu_modalias(struct device *, struct device_attribute *, char *);
+
 static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
-- 
1.8.3.2

[RFC PATCH 2/4] cpu: advertise CPU features over udev in a generic way

[Dave Martin]

On Thu, Nov 07, 2013 at 06:17:35PM +0100, Ard Biesheuvel wrote:
> This patch implements a generic modalias 'cpu:feature:...' which
> enables CPU feature flag based module loading in a generic way.
> All the arch needs to do is enable CONFIG_ARCH_HAS_CPU_AUTOPROBE
> and export a u32 called 'cpu_features'. (What each bit actually
> means is irrelevant on this level.)

There seems to be an assumption here that a module is either a pure CPU
accelerator, or it is completely independent of CPU features.

I'm not sure that this is true.  A CPU feature isn't a "device".
Rather, it's a property of code (which might be a driver for something
different -- maybe we have a hardware crypto accelerator where key
scheduling must be done in software.  It's still a driver for the
crypto engine, but we might have different implementations of the key
scheduling, based on the CPU features avaiable).

It's also not obvious why we should blindly load all CPU-feature-
dependent helper modules on bgoot, regardless of whether the module(s)
that use them are being loaded.

Maybe the amount of CPU feature dependent code is small enough that
we don't really care about such subtleties, though.


It's also not clear how different optimised modules for the same
thing would get prioritised.  Suppose there we have v5E and NEON
optimised AES helper modules?  Both those CPU features are avaiable,
but which module should we load?

If all candidate modules get loaded, which one actually gets used?
Does the load order matter?

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/base/cpu.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 49c6f4b..a661d31 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -276,6 +276,30 @@ static void cpu_device_release(struct device *dev)
>  }
>  
>  #ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE
> +ssize_t print_cpu_modalias(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	extern u32 __weak cpu_features;

Why is this __weak?  Surely CONFIG_ARCH_HAS_CPU_AUTOPROBE=y makes no
sense if the arch code does not define either cpu_features or
arch_print_cpu_modalias()?  The build should be made to fail in that
case...

> +	ssize_t n;
> +	int i;
> +	u32 f;
> +
> +	/*
> +	 * With 32 features maximum (taking 3 bytes each to print), we don't
> +	 * need to worry about overrunning the PAGE_SIZE sized buffer.
> +	 */
> +	n = sprintf(buf, "cpu:feature:");
> +	for (f = cpu_features, i = 0; f; f >>= 1, i++)
> +		if (f & 1)
> +			n += sprintf(&buf[n], ",%02X", i);

Why can't this overflow buf?

modalias matching is pretty much based on string matching, so I wonder
whether we could use the human-readable feature strings instead.
Those are already a stable ABI.  Relying on numbers unnecessarily
encrypts the udev/modprobe config.

Otherwise, "%02X" seems to place an arbitrary limit of 256 features.
I'm not sure that padding these numbers to a particular width is
advantageous for the parser.

> +	buf[n++] = '\n';
> +	return n;
> +}
> +
> +ssize_t __attribute__((weak, alias("print_cpu_modalias")))
> +arch_print_cpu_modalias(struct device *, struct device_attribute *, char *);
> +

If an implementation of arch_print_cpu_modalias() is linked with this,
won't that result in the print_cpu_modalias() defined here just being
included as dead code?

i.e., we knowingly link into the kernel some code that the build-time
configuration tells us is dead.

Maybe I'm misunderstanding things here, but I think this weak-symbol
stuff is mainly useful when shipping a binary blob in which people can
override certain symbols at link time.

We build vmlinux in one go, so I'm not sure that's appropriate here (?)

>  static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 2/4] cpu: advertise CPU features over udev in a generic way

[Ard Biesheuvel]

> On 7 nov. 2013, at 20:33, Dave Martin <Dave.Martin@arm.com> wrote:
> 
>> On Thu, Nov 07, 2013 at 06:17:35PM +0100, Ard Biesheuvel wrote:
>> This patch implements a generic modalias 'cpu:feature:...' which
>> enables CPU feature flag based module loading in a generic way.
>> All the arch needs to do is enable CONFIG_ARCH_HAS_CPU_AUTOPROBE
>> and export a u32 called 'cpu_features'. (What each bit actually
>> means is irrelevant on this level.)
> 
> There seems to be an assumption here that a module is either a pure CPU
> accelerator, or it is completely independent of CPU features.
> 
> I'm not sure that this is true.  A CPU feature isn't a "device".
> Rather, it's a property of code (which might be a driver for something
> different -- maybe we have a hardware crypto accelerator where key
> scheduling must be done in software.  It's still a driver for the
> crypto engine, but we might have different implementations of the key
> scheduling, based on the CPU features avaiable).
> 

The use case I am targeting is dedicated instructions for AES, CRC, SHA1, SHA2, etc, all of which -on arm64- can be independently enabled by the implementer.  A single distro image should run as closely to optimal as possible right out of the box, especially in these cases.

> It's also not obvious why we should blindly load all CPU-feature-
> dependent helper modules on bgoot, regardless of whether the module(s)
> that use them are being loaded.
> 
> Maybe the amount of CPU feature dependent code is small enough that
> we don't really care about such subtleties, though.
> 
> It's also not clear how different optimised modules for the same
> thing would get prioritised.  Suppose there we have v5E and NEON
> optimised AES helper modules?  Both those CPU features are avaiable,
> but which module should we load?
> 
> If all candidate modules get loaded, which one actually gets used?
> Does the load order matter?
> 

In the cryptoapi case, each algorithm also has a priority assigned to it, which should be sufficient to break these kinds of ties. Other code exists (xor, raid6) that does a quick boot time benchmark of all available options. In general, though, this is undefined, so I agree that associating random bits of NEON or FP code with the CPU feature bit may make little sense, especially on v8 (which has feature bits for mandatory extensions like FP and ASIMD)

-- 
Ard.

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> drivers/base/cpu.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>> 
>> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
>> index 49c6f4b..a661d31 100644
>> --- a/drivers/base/cpu.c
>> +++ b/drivers/base/cpu.c
>> @@ -276,6 +276,30 @@ static void cpu_device_release(struct device *dev)
>> }
>> 
>> #ifdef CONFIG_ARCH_HAS_CPU_AUTOPROBE
>> +ssize_t print_cpu_modalias(struct device *dev,
>> +               struct device_attribute *attr,
>> +               char *buf)
>> +{
>> +    extern u32 __weak cpu_features;
> 
> Why is this __weak?  Surely CONFIG_ARCH_HAS_CPU_AUTOPROBE=y makes no
> sense if the arch code does not define either cpu_features or
> arch_print_cpu_modalias()?  The build should be made to fail in that
> case...
> 
>> +    ssize_t n;
>> +    int i;
>> +    u32 f;
>> +
>> +    /*
>> +     * With 32 features maximum (taking 3 bytes each to print), we don't
>> +     * need to worry about overrunning the PAGE_SIZE sized buffer.
>> +     */
>> +    n = sprintf(buf, "cpu:feature:");
>> +    for (f = cpu_features, i = 0; f; f >>= 1, i++)
>> +        if (f & 1)
>> +            n += sprintf(&buf[n], ",%02X", i);
> 
> Why can't this overflow buf?
> 
> modalias matching is pretty much based on string matching, so I wonder
> whether we could use the human-readable feature strings instead.
> Those are already a stable ABI.  Relying on numbers unnecessarily
> encrypts the udev/modprobe config.
> 
> Otherwise, "%02X" seems to place an arbitrary limit of 256 features.
> I'm not sure that padding these numbers to a particular width is
> advantageous for the parser.
> 
>> +    buf[n++] = '\n';
>> +    return n;
>> +}
>> +
>> +ssize_t __attribute__((weak, alias("print_cpu_modalias")))
>> +arch_print_cpu_modalias(struct device *, struct device_attribute *, char *);
>> +
> 
> If an implementation of arch_print_cpu_modalias() is linked with this,
> won't that result in the print_cpu_modalias() defined here just being
> included as dead code?
> 
> i.e., we knowingly link into the kernel some code that the build-time
> configuration tells us is dead.
> 
> Maybe I'm misunderstanding things here, but I think this weak-symbol
> stuff is mainly useful when shipping a binary blob in which people can
> override certain symbols at link time.
> 
> We build vmlinux in one go, so I'm not sure that's appropriate here (?)
> 
>> static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env)
>> {
>>    char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> -- 
>> 1.8.3.2
>> 
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 2/4] cpu: advertise CPU features over udev in a generic way

[Ard Biesheuvel]

On 7 November 2013 20:33, Dave Martin <Dave.Martin@arm.com> wrote:
> On Thu, Nov 07, 2013 at 06:17:35PM +0100, Ard Biesheuvel wrote:
>> This patch implements a generic modalias 'cpu:feature:...' which
>> enables CPU feature flag based module loading in a generic way.
>> All the arch needs to do is enable CONFIG_ARCH_HAS_CPU_AUTOPROBE
>> and export a u32 called 'cpu_features'. (What each bit actually
>> means is irrelevant on this level.)

[ ... ]

> Why is this __weak?  Surely CONFIG_ARCH_HAS_CPU_AUTOPROBE=y makes no
> sense if the arch code does not define either cpu_features or
> arch_print_cpu_modalias()?  The build should be made to fail in that
> case...
>

If you define the latter but not the former, you will still get a link
error. But indeed, getting this stuff right is the main point of this
RFC series.
Just putting a '#define arch_print_cpu_modalias
arch_print_cpu_modalias' in the right place and testing for it here is
indeed a lot better.

>> +     ssize_t n;
>> +     int i;
>> +     u32 f;
>> +
>> +     /*
>> +      * With 32 features maximum (taking 3 bytes each to print), we don't
>> +      * need to worry about overrunning the PAGE_SIZE sized buffer.
>> +      */
>> +     n = sprintf(buf, "cpu:feature:");
>> +     for (f = cpu_features, i = 0; f; f >>= 1, i++)
>> +             if (f & 1)
>> +                     n += sprintf(&buf[n], ",%02X", i);
>
> Why can't this overflow buf?
>

Because there are only so many bits you can shift out of a u32 before
it becomes zero.

> modalias matching is pretty much based on string matching, so I wonder
> whether we could use the human-readable feature strings instead.
> Those are already a stable ABI.  Relying on numbers unnecessarily
> encrypts the udev/modprobe config.
>

Introducing symbolic names would add an arch specific dependency to
the host tool that is used to generate the .ko metadata from the
custom ELF sections. This being a port of a similar feature on x86,
the comments in scripts/mod/file2alias.c were helpful in figuring out
that that was deemed too complicated at the time. And we don't resolve
PCI or USB VIDs to vendor names either, as this is just module
metadata for machine matching.

> Otherwise, "%02X" seems to place an arbitrary limit of 256 features.
> I'm not sure that padding these numbers to a particular width is
> advantageous for the parser.
>

Using a u32 already brings it down to 32, but it is sufficient for now
and I think changing it later if necessary should be doable.

>> +     buf[n++] = '\n';
>> +     return n;
>> +}
>> +
>> +ssize_t __attribute__((weak, alias("print_cpu_modalias")))
>> +arch_print_cpu_modalias(struct device *, struct device_attribute *, char *);
>> +
>
> If an implementation of arch_print_cpu_modalias() is linked with this,
> won't that result in the print_cpu_modalias() defined here just being
> included as dead code?
>
> i.e., we knowingly link into the kernel some code that the build-time
> configuration tells us is dead.
>

Absolutely true. See the above.

> Maybe I'm misunderstanding things here, but I think this weak-symbol
> stuff is mainly useful when shipping a binary blob in which people can
> override certain symbols at link time.
>
> We build vmlinux in one go, so I'm not sure that's appropriate here (?)
>

I agree, and (as I pointed out in my cover letter) I was prepared for
people perhaps having problems with that.
But yes, it seems just #defining arch_print_cpu_modalias in the
appropriate place and #ifndef'ing on it seems much bettter.

-- 
Ard.

[RFC PATCH 3/4] scripts/mod: add generic CPU features as module alias

[Ard Biesheuvel]

This implements the changes needed to turn CPU features
declared as being depended upon by modules into module aliases
in the .ko metadata.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 include/linux/mod_devicetable.h   | 11 +++++++++++
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 10 ++++++++++
 3 files changed, 24 insertions(+)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 45e9214..e481796 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -564,6 +564,17 @@ struct x86_cpu_id {
 #define X86_MODEL_ANY  0
 #define X86_FEATURE_ANY 0	/* Same as FPU, you can't test for that */
 
+/*
+ * Generic table type for tracking CPU features.
+ * @feature:	the bit number of the feature (0 - 31)
+ *
+ * How the bit numbers map to actual CPU features is entirely up to the arch
+ */
+
+struct cpu_feature {
+	__u8	feature;
+};
+
 #define IPACK_ANY_FORMAT 0xff
 #define IPACK_ANY_ID (~0)
 struct ipack_device_id {
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index bb5d115..f282516 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -174,6 +174,9 @@ int main(void)
 	DEVID_FIELD(x86_cpu_id, model);
 	DEVID_FIELD(x86_cpu_id, vendor);
 
+	DEVID(cpu_feature);
+	DEVID_FIELD(cpu_feature, feature);
+
 	DEVID(mei_cl_device_id);
 	DEVID_FIELD(mei_cl_device_id, name);
 
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2370863..209cdb2 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1135,6 +1135,16 @@ static int do_x86cpu_entry(const char *filename, void *symval,
 }
 ADD_TO_DEVTABLE("x86cpu", x86_cpu_id, do_x86cpu_entry);
 
+/* LOOKS like cpu:feature:*FEAT* */
+static int do_cpu_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD(symval, cpu_feature, feature);
+
+	sprintf(alias, "cpu:feature:*%02X*", feature);
+	return 1;
+}
+ADD_TO_DEVTABLE("cpu", cpu_feature, do_cpu_entry);
+
 /* Looks like: mei:S */
 static int do_mei_entry(const char *filename, void *symval,
 			char *alias)
-- 
1.8.3.2

[RFC PATCH 4/4] arm64: advertise CPU features using module aliases

[Ard Biesheuvel]

This enables the generic implementation in drivers/base/cpu.c
that allows modules to be loaded automatically based on the
optional features supported (and advertised) by the CPU.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/Kconfig        | 3 +++
 arch/arm64/kernel/setup.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c044548..50cd97f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -202,6 +202,9 @@ config ARCH_WANT_HUGE_PMD_SHARE
 config HAVE_ARCH_TRANSPARENT_HUGEPAGE
 	def_bool y
 
+config ARCH_HAS_CPU_AUTOPROBE
+	def_bool y
+
 source "mm/Kconfig"
 
 config XEN_DOM0
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 780a7aa..90d5277 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -403,3 +403,5 @@ const struct seq_operations cpuinfo_op = {
 	.stop	= c_stop,
 	.show	= c_show
 };
+
+extern u32 cpu_features __attribute__((alias("elf_hwcap")));
-- 
1.8.3.2

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[H. Peter Anvin]

On 11/07/2013 09:17 AM, Ard Biesheuvel wrote:
> This series implements automatic module loading based on optional CPU features,
> and tries to do so in a generic way. Currently, 32 feature bits are supported,
> and how they map to actual CPU features is entirely up to the architecture.

NAK.

We in the x86 world already left 32 bits way behind; we currently have
320 bit feature masks.

If you're aiming at doing this in a generic way, it needs to be able to
accommodate the current x86cpu feature stuff as a subset, which this
doesn't.

	-hpa

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[Andi Kleen]

On Thu, Nov 07, 2013 at 01:09:41PM -0800, H. Peter Anvin wrote:
> On 11/07/2013 09:17 AM, Ard Biesheuvel wrote:
> > This series implements automatic module loading based on optional CPU features,
> > and tries to do so in a generic way. Currently, 32 feature bits are supported,
> > and how they map to actual CPU features is entirely up to the architecture.
> 
> NAK.
> 
> We in the x86 world already left 32 bits way behind; we currently have
> 320 bit feature masks.
> 
> If you're aiming at doing this in a generic way, it needs to be able to
> accommodate the current x86cpu feature stuff as a subset, which this
> doesn't.

They can just use the exact same code/macros as x86cpu, just need a different 
prefix and use wildcards if they miss something (e.g. family)

-Andi

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[Ard Biesheuvel]

On 7 November 2013 22:39, Andi Kleen <ak@linux.intel.com> wrote:
> On Thu, Nov 07, 2013 at 01:09:41PM -0800, H. Peter Anvin wrote:
>> On 11/07/2013 09:17 AM, Ard Biesheuvel wrote:
>> > This series implements automatic module loading based on optional CPU features,
>> > and tries to do so in a generic way. Currently, 32 feature bits are supported,
>> > and how they map to actual CPU features is entirely up to the architecture.
>>
>> NAK.
>>
>> We in the x86 world already left 32 bits way behind; we currently have
>> 320 bit feature masks.
>>
>> If you're aiming at doing this in a generic way, it needs to be able to
>> accommodate the current x86cpu feature stuff as a subset, which this
>> doesn't.
>
> They can just use the exact same code/macros as x86cpu, just need a different
> prefix and use wildcards if they miss something (e.g. family)
>

That would involve repurposing/generalizing a bit more of the existing
x86-only code than I did the first time around, but if you (as x86
maintainers) are happy with that, I'm all for it.

I do have a couple of questions then
- the module aliases host tool has no arch specific dependencies at
all except having x86cpu as one of the entries: would you mind
dropping the x86 prefix there? Or rather add dependencies on $ARCH?
(If we drop it there, we basically end up with 'cpu:' everywhere)
- in the vendor/family/model case, it may be preferable to drop these
fields entirely from certain modules' aliases if they match on 'any'
(provided that the module tools permit this) rather than add
architecture, variant, revision, etc  fields for all architectures if
they can only ever match on one
- some of the X86_ macros would probable be redefined in terms of the
generic macros rather than the other way around, which would result in
some changes under arch/x86 as well, is that acceptable for you?

Thanks,
Ard.

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[Andi Kleen]

> - the module aliases host tool has no arch specific dependencies at
> all except having x86cpu as one of the entries: would you mind
> dropping the x86 prefix there? Or rather add dependencies on $ARCH?
> (If we drop it there, we basically end up with 'cpu:' everywhere)

Should be fine.

> - in the vendor/family/model case, it may be preferable to drop these
> fields entirely from certain modules' aliases if they match on 'any'
> (provided that the module tools permit this) rather than add
> architecture, variant, revision, etc  fields for all architectures if
> they can only ever match on one

The module tools require everything matching with the same wild cards.

So I don't know how "any" would work.

-Andi

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[H. Peter Anvin]

I assumed he was referring to just dropping those fields of they didn't make sense for the architecture.  That would make sense to do.

Andi Kleen <ak@linux.intel.com> wrote:
>> - the module aliases host tool has no arch specific dependencies at
>> all except having x86cpu as one of the entries: would you mind
>> dropping the x86 prefix there? Or rather add dependencies on $ARCH?
>> (If we drop it there, we basically end up with 'cpu:' everywhere)
>
>Should be fine.
>
>> - in the vendor/family/model case, it may be preferable to drop these
>> fields entirely from certain modules' aliases if they match on 'any'
>> (provided that the module tools permit this) rather than add
>> architecture, variant, revision, etc  fields for all architectures if
>> they can only ever match on one
>
>The module tools require everything matching with the same wild cards.
>
>So I don't know how "any" would work.
>
>-Andi

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[H. Peter Anvin]

On 11/07/2013 02:15 PM, Ard Biesheuvel wrote:
> 
> That would involve repurposing/generalizing a bit more of the existing
> x86-only code than I did the first time around, but if you (as x86
> maintainers) are happy with that, I'm all for it.
> 
> I do have a couple of questions then
> - the module aliases host tool has no arch specific dependencies at
> all except having x86cpu as one of the entries: would you mind
> dropping the x86 prefix there? Or rather add dependencies on $ARCH?
> (If we drop it there, we basically end up with 'cpu:' everywhere)

I think it makes sense to indicate what kind of CPU the string refers
to, as the top-level indicator of what is going on.  This might be
possible to macroize the generation of this prefix, though.

> - in the vendor/family/model case, it may be preferable to drop these
> fields entirely from certain modules' aliases if they match on 'any'
> (provided that the module tools permit this) rather than add
> architecture, variant, revision, etc  fields for all architectures if
> they can only ever match on one

I think that can be CPU dependent.

> - some of the X86_ macros would probable be redefined in terms of the
> generic macros rather than the other way around, which would result in
> some changes under arch/x86 as well, is that acceptable for you?

If you are talking about X86_FEATURE_* then almost certainly no,
although I'm willing to listen to what you have in mind.

	-hpa

[RFC PATCH 0/4] wire up CPU features to udev based module loading

[Ard]

On 11/07/2013 11:49 PM, H. Peter Anvin wrote:
> On 11/07/2013 02:15 PM, Ard Biesheuvel wrote:
>>
>> That would involve repurposing/generalizing a bit more of the existing
>> x86-only code than I did the first time around, but if you (as x86
>> maintainers) are happy with that, I'm all for it.
>>
>> I do have a couple of questions then - the module aliases host tool has
>> no arch specific dependencies at all except having x86cpu as one of the
>> entries: would you mind dropping the x86 prefix there? Or rather add
>> dependencies on $ARCH? (If we drop it there, we basically end up with
>> 'cpu:' everywhere)
>
> I think it makes sense to indicate what kind of CPU the string refers to,
> as the top-level indicator of what is going on.  This might be possible to
> macroize the generation of this prefix, though.
>
>> - in the vendor/family/model case, it may be preferable to drop these
>> fields entirely from certain modules' aliases if they match on 'any'
>> (provided that the module tools permit this) rather than add
>> architecture, variant, revision, etc  fields for all architectures if
>> they can only ever match on one
>
> I think that can be CPU dependent.
>
>> - some of the X86_ macros would probable be redefined in terms of the
>> generic macros rather than the other way around, which would result in
>> some changes under arch/x86 as well, is that acceptable for you?
>
> If you are talking about X86_FEATURE_* then almost certainly no, although
> I'm willing to listen to what you have in mind.
>

Actually, this should not be necessary at all, if we are happy to put up with
distinct types for x86_cpu_id, generic_cpu_id, and perhaps other arch based
ones in the future.

So what I have in mind for x86 is this (and this way, no other changes are
needed to the existing x86 bits)

diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
index ab6082a..82e92b2 100644
--- a/arch/x86/kernel/cpu/match.c
+++ b/arch/x86/kernel/cpu/match.c
@@ -56,8 +56,7 @@ ssize_t arch_print_cpu_modalias(struct device *dev,
         int i, n;
         char *buf = bufptr;

-       n = snprintf(buf, size, "x86cpu:vendor:%04X:family:%04X:"
-                    "model:%04X:feature:",
+       n = snprintf(buf, size, "cpu:type:x86,ven%04Xfam%04Xmod%04X:feature:",
                 boot_cpu_data.x86_vendor,
                 boot_cpu_data.x86,
                 boot_cpu_data.x86_model);
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2370863..dea263a 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1110,7 +1110,7 @@ static int do_amba_entry(const char *filename,
  }
  ADD_TO_DEVTABLE("amba", amba_id, do_amba_entry);

-/* LOOKS like x86cpu:vendor:VVVV:family:FFFF:model:MMMM:feature:*,FEAT,*
+/* LOOKS like cpu:type:x86,venVVVVfamFFFFmodMMMM:feature:*,FEAT,*
   * All fields are numbers. It would be nicer to use strings for vendor
   * and feature, but getting those out of the build system here is too
   * complicated.
@@ -1124,10 +1124,10 @@ static int do_x86cpu_entry(const char *filename, void
*symval,
         DEF_FIELD(symval, x86_cpu_id, model);
         DEF_FIELD(symval, x86_cpu_id, vendor);

-       strcpy(alias, "x86cpu:");
-       ADD(alias, "vendor:",  vendor != X86_VENDOR_ANY, vendor);
-       ADD(alias, ":family:", family != X86_FAMILY_ANY, family);
-       ADD(alias, ":model:",  model  != X86_MODEL_ANY,  model);
+       strcpy(alias, "cpu:type:x86,");
+       ADD(alias, "ven", vendor != X86_VENDOR_ANY, vendor);
+       ADD(alias, "fam", family != X86_FAMILY_ANY, family);
+       ADD(alias, "mod", model  != X86_MODEL_ANY,  model);
         strcat(alias, ":feature:*");
         if (feature != X86_FEATURE_ANY)
                 sprintf(alias + strlen(alias), "%04X*", feature);

The way I intend this to work is that, for instance, arm64 will emit something
like

cpu:type:arm64:features:,0000,0001,0002

when the cpu uevent is raised, but as it is unclear whether we will want to
match on variant, revision etc on arm64, we could use a generic
MODULE_DEVICE_TABLE(cpu, ...) [as in patch #2 of this series] which would
produce something like

cpu:type:*:features:*00xx*

as a modalias to match on feature xx, whereas a plain X86_FEATURE_MATCH() will
produce something like

cpu:type:x86,ven*fam*mod*:features:*00xx*

Does this look like a reasonable approach to you?

Regards,
Ard.