2009-06-26 00:14:32

by Jacob Pan

[permalink] [raw]
Subject: [PATCH 2/9] x86: introduce a set of platform feature flags

>From 41685ae1e8b8e77fdacbdf8e8155f1e54624cbef Mon Sep 17 00:00:00 2001
From: Jacob Pan <[email protected]>
Date: Thu, 11 Jun 2009 09:37:26 -0700
Subject: [PATCH] x86: introduce a set of platform feature flags

This patch introduces a set of x86 pc platform feature flags. the intention is
to clean up setup code based on the availability of patform features.

With the introduction of non-PC x86 platforms, these flags will also pave
the way for cleaner integration.

The current feature flags are not a complete set of all possible PC features
only the ones relavent to system setup, such as resource allocation, are
included.

Signed-off-by: Jacob Pan <[email protected]>
---
arch/x86/include/asm/platform_feature.h | 65 ++++++++++++++++++
arch/x86/kernel/.gitignore | 1 +
arch/x86/kernel/Makefile | 11 +++
arch/x86/kernel/mkx86pcflags.pl | 32 +++++++++
arch/x86/kernel/platform_info.c | 111 +++++++++++++++++++++++++++++++
5 files changed, 220 insertions(+), 0 deletions(-)
create mode 100644 arch/x86/include/asm/platform_feature.h
create mode 100644 arch/x86/kernel/mkx86pcflags.pl
create mode 100644 arch/x86/kernel/platform_info.c

diff --git a/arch/x86/include/asm/platform_feature.h b/arch/x86/include/asm/platform_feature.h
new file mode 100644
index 0000000..bcadda5
--- /dev/null
+++ b/arch/x86/include/asm/platform_feature.h
@@ -0,0 +1,65 @@
+/*
+ * platform_feature.h - Defines x86 platform feature bits
+ *
+ * (C) Copyright 2008 Intel Corporation
+ * Author: Jacob Pan ([email protected])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ *
+ * Note: platform feature flags allow kernel to identify hardware capabilities
+ * at boottime and runtime. It enables binary compaitiblity between standard
+ * X86 PC and non-standard X86 platforms such as MID. These flags are default
+ * to X86 PC standard, at boot time, they will be overwritten by system tables
+ * provided by firmware.
+ *
+ */
+#ifndef _ASM_X86_PLATFORM_FEATURE_H
+#define _ASM_X86_PLATFORM_FEATURE_H
+
+#ifndef __ASSEMBLY__
+#include <linux/bitops.h>
+#endif
+#include <asm/required-features.h>
+
+#define N_PLATFORM_CAPINTS 2 /* N 32-bit words worth of info */
+/* X86 base platform features, include PC, legacy free MID devices, etc.
+ * This list provides early and important information to the kernel in a
+ * centralized place such that kernel can make a decision on the best
+ * choice of which system devices to use. e.g. timers or interrupt
+ * controllers.
+ */
+#define X86_PLATFORM_FEATURE_8259 (0*32+0) /* i8259A PIC */
+#define X86_PLATFORM_FEATURE_8254 (0*32+1) /* i8253/4 PIT */
+#define X86_PLATFORM_FEATURE_IOAPIC (0*32+2) /* IO-APIC */
+#define X86_PLATFORM_FEATURE_HPET (0*32+3) /* HPET timer */
+#define X86_PLATFORM_FEATURE_RTC (0*32+4) /* real time clock*/
+#define X86_PLATFORM_FEATURE_FLOPPY (0*32+5) /* ISA floppy */
+#define X86_PLATFORM_FEATURE_ISA (0*32+6) /* ISA/LPC bus */
+#define X86_PLATFORM_FEATURE_BIOS (0*32+7) /* BIOS service,
+ * e.g. int calls
+ * EBDA, etc.
+ */
+#define X86_PLATFORM_FEATURE_ACPI (0*32+8) /* has ACPI support */
+#define X86_PLATFORM_FEATURE_SFI (0*32+9) /* has SFI support */
+#define X86_PLATFORM_FEATURE_8042 (0*32+10) /* i8042 KBC */
+
+extern __u32 platform_feature[N_PLATFORM_CAPINTS];
+extern const char *const
+ x86_platform_available_feature[N_PLATFORM_CAPINTS * 32];
+#define platform_has(bit) \
+ test_bit(bit, (unsigned long *)platform_feature)
+
+#define platform_feature_set_flag(bit) \
+ set_bit(bit, (unsigned long *)platform_feature)
+
+#define clear_platform_feature(bit) \
+ clear_bit(bit, (unsigned long *)platform_feature)
+
+void platform_feature_init_default(int);
+void clear_all_platform_feature(void);
+
+
+#endif /* _ASM_X86_PLATFORM_FEATURE_H */
diff --git a/arch/x86/kernel/.gitignore b/arch/x86/kernel/.gitignore
index 08f4fd7..0943d5e 100644
--- a/arch/x86/kernel/.gitignore
+++ b/arch/x86/kernel/.gitignore
@@ -1,3 +1,4 @@
vsyscall.lds
vsyscall_32.lds
vmlinux.lds
+x86pcflags.c
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 6c327b8..8d1ae61 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_IA32_EMULATION) += tls.o
obj-y += step.o
obj-$(CONFIG_STACKTRACE) += stacktrace.o
obj-y += cpu/
+obj-y += platform_info.o x86pcflags.o
obj-y += acpi/
obj-y += reboot.o
obj-$(CONFIG_MCA) += mca_32.o
@@ -112,6 +113,16 @@ obj-$(CONFIG_X86_CHECK_BIOS_CORRUPTION) += check.o

obj-$(CONFIG_SWIOTLB) += pci-swiotlb.o

+quiet_cmd_mkx86pcflags = MKFEATURE $@
+ cmd_mkx86pcflags = $(PERL) $(srctree)/$(src)/mkx86pcflags.pl $< $@
+
+platform_feature = $(src)/../include/asm/platform_feature.h
+
+
+targets += x86pcflags.c
+$(obj)/x86pcflags.c: $(platform_feature) $(src)/mkx86pcflags.pl FORCE
+ $(call if_changed,mkx86pcflags)
+
###
# 64 bit specific files
ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/mkx86pcflags.pl b/arch/x86/kernel/mkx86pcflags.pl
new file mode 100644
index 0000000..19c13aa
--- /dev/null
+++ b/arch/x86/kernel/mkx86pcflags.pl
@@ -0,0 +1,32 @@
+#!/usr/bin/perl
+#
+# Generate the x86_platform_available_feature[] array from arch/x86/include/asm/platform_feature.h
+#
+
+($in, $out) = @ARGV;
+
+open(IN, "< $in\0") or die "$0: cannot open: $in: $!\n";
+open(OUT, "> $out\0") or die "$0: cannot create: $out: $!\n";
+
+print OUT "#include <asm/platform_feature.h>\n\n";
+print OUT "const char * const x86_platform_available_feature[N_PLATFORM_CAPINTS*32] = {\n";
+
+while (defined($line = <IN>)) {
+ if ($line =~ /^\s*\#\s*define\s+(X86_PLATFORM_FEATURE_(\S+))\s+(.*)$/) {
+ $macro = $1;
+ $feature = $2;
+ $tail = $3;
+ if ($tail =~ /\/\*\s*\"([^"]*)\".*\*\//) {
+ $feature = $1;
+ }
+
+ if ($feature ne '') {
+ printf OUT "\t%-32s = \"%s\",\n",
+ "[$macro]", "\L$feature";
+ }
+ }
+}
+print OUT "};\n";
+
+close(IN);
+close(OUT);
diff --git a/arch/x86/kernel/platform_info.c b/arch/x86/kernel/platform_info.c
new file mode 100644
index 0000000..b39898a
--- /dev/null
+++ b/arch/x86/kernel/platform_info.c
@@ -0,0 +1,111 @@
+#include <linux/smp.h>
+#include <linux/timex.h>
+#include <linux/string.h>
+#include <linux/seq_file.h>
+#include <linux/sysdev.h>
+#include <linux/cpu.h>
+#include <asm/bootparam.h>
+#include <asm/platform_feature.h>
+#include <asm/setup.h>
+
+/* Set of default platform features of standard X86 PC. May be overwritten by
+ * information found during boot, such as boot parameters, SFI, ACPI tables,
+ * etc. Not every X86PC feature is listed here, only include useful ones that
+ * can not be safely detected at runtime.
+ */
+__u32 platform_feature[N_PLATFORM_CAPINTS] =
+{
+ (1UL << X86_PLATFORM_FEATURE_8259) |
+ (1UL << X86_PLATFORM_FEATURE_8042) |
+ (1UL << X86_PLATFORM_FEATURE_IOAPIC) |
+ (1UL << X86_PLATFORM_FEATURE_BIOS) |
+ (1UL << X86_PLATFORM_FEATURE_HPET) |
+ (1UL << X86_PLATFORM_FEATURE_8254) |
+ (1UL << X86_PLATFORM_FEATURE_RTC) |
+ (1UL << X86_PLATFORM_FEATURE_ISA) |
+ (1UL << X86_PLATFORM_FEATURE_FLOPPY),
+ 0
+};
+EXPORT_SYMBOL_GPL(platform_feature);
+
+inline void clear_all_platform_feature(void)
+{
+ memset(platform_feature, 0, sizeof(platform_feature));
+}
+
+static ssize_t
+sysfs_show_available_platform_feature(struct sys_device *dev,
+ struct sysdev_attribute *attr, char *buf)
+{
+ ssize_t count = 0;
+ int i;
+
+ for (i = 0; i < 32*N_PLATFORM_CAPINTS; i++) {
+ if (x86_platform_available_feature[i] != NULL) {
+ count += snprintf(buf + count,
+ max((ssize_t)PAGE_SIZE - count,
+ (ssize_t)0), "%s ",
+ x86_platform_available_feature[i]);
+ }
+ }
+ count += snprintf(buf + count,
+ max((ssize_t)PAGE_SIZE - count, (ssize_t)0), "\n");
+
+ return count;
+}
+
+static ssize_t
+sysfs_show_current_platform_feature(struct sys_device *dev,
+ struct sysdev_attribute *attr,
+ char *buf)
+{
+ ssize_t count = 0;
+ int i;
+
+ for (i = 0; i < 32*N_PLATFORM_CAPINTS; i++) {
+ if (platform_has(i))
+ count += snprintf(buf + count,
+ max((ssize_t)PAGE_SIZE - count, (ssize_t)0),
+ "%s ", x86_platform_available_feature[i]);
+ }
+ count += snprintf(buf + count,
+ max((ssize_t)PAGE_SIZE - count, (ssize_t)0), "\n");
+
+ return count;
+}
+
+static SYSDEV_ATTR(current_platform_feature, 0444,
+ sysfs_show_current_platform_feature,
+ NULL);
+
+static SYSDEV_ATTR(available_platform_feature, 0444,
+ sysfs_show_available_platform_feature, NULL);
+static struct sysdev_class platform_feature_sysclass = {
+ .name = "platform_feature",
+};
+static struct sys_device device_platform_feature = {
+ .id = 0,
+ .cls = &platform_feature_sysclass,
+};
+static int __init sysfs_platforminfo_init(void)
+{
+ int error = sysdev_class_register(&platform_feature_sysclass);
+
+ if (!error)
+ error = sysdev_register(&device_platform_feature);
+ if (!error)
+ error = sysdev_create_file(
+ &device_platform_feature,
+ &attr_current_platform_feature);
+ if (!error)
+ error = sysdev_create_file(
+ &device_platform_feature,
+ &attr_available_platform_feature);
+ return error;
+}
+arch_initcall(sysfs_platforminfo_init);
+
+void platform_feature_init_default(int subarch_id)
+{
+ printk(KERN_INFO "Use default X86 platform feature set\n");
+}
--
1.5.6.5


2009-06-26 07:22:34

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


* Pan, Jacob jun <[email protected]> wrote:

> >From 41685ae1e8b8e77fdacbdf8e8155f1e54624cbef Mon Sep 17 00:00:00 2001
> From: Jacob Pan <[email protected]>
> Date: Thu, 11 Jun 2009 09:37:26 -0700
> Subject: [PATCH] x86: introduce a set of platform feature flags
>
> This patch introduces a set of x86 pc platform feature flags. the intention is
> to clean up setup code based on the availability of patform features.
>
> With the introduction of non-PC x86 platforms, these flags will also pave
> the way for cleaner integration.
>
> The current feature flags are not a complete set of all possible PC features
> only the ones relavent to system setup, such as resource allocation, are
> included.
>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> arch/x86/include/asm/platform_feature.h | 65 ++++++++++++++++++
> arch/x86/kernel/.gitignore | 1 +
> arch/x86/kernel/Makefile | 11 +++
> arch/x86/kernel/mkx86pcflags.pl | 32 +++++++++
> arch/x86/kernel/platform_info.c | 111 +++++++++++++++++++++++++++++++
> 5 files changed, 220 insertions(+), 0 deletions(-)
> create mode 100644 arch/x86/include/asm/platform_feature.h
> create mode 100644 arch/x86/kernel/mkx86pcflags.pl
> create mode 100644 arch/x86/kernel/platform_info.c
>
> diff --git a/arch/x86/include/asm/platform_feature.h b/arch/x86/include/asm/platform_feature.h
> new file mode 100644
> index 0000000..bcadda5
> --- /dev/null
> +++ b/arch/x86/include/asm/platform_feature.h
> @@ -0,0 +1,65 @@
> +/*
> + * platform_feature.h - Defines x86 platform feature bits
> + *
> + * (C) Copyright 2008 Intel Corporation
> + * Author: Jacob Pan ([email protected])
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + *
> + * Note: platform feature flags allow kernel to identify hardware capabilities
> + * at boottime and runtime. It enables binary compaitiblity between standard
> + * X86 PC and non-standard X86 platforms such as MID. These flags are default
> + * to X86 PC standard, at boot time, they will be overwritten by system tables
> + * provided by firmware.
> + *
> + */
> +#ifndef _ASM_X86_PLATFORM_FEATURE_H
> +#define _ASM_X86_PLATFORM_FEATURE_H
> +
> +#ifndef __ASSEMBLY__
> +#include <linux/bitops.h>
> +#endif
> +#include <asm/required-features.h>
> +
> +#define N_PLATFORM_CAPINTS 2 /* N 32-bit words worth of info */
> +/* X86 base platform features, include PC, legacy free MID devices, etc.
> + * This list provides early and important information to the kernel in a
> + * centralized place such that kernel can make a decision on the best
> + * choice of which system devices to use. e.g. timers or interrupt
> + * controllers.
> + */
> +#define X86_PLATFORM_FEATURE_8259 (0*32+0) /* i8259A PIC */
> +#define X86_PLATFORM_FEATURE_8254 (0*32+1) /* i8253/4 PIT */
> +#define X86_PLATFORM_FEATURE_IOAPIC (0*32+2) /* IO-APIC */
> +#define X86_PLATFORM_FEATURE_HPET (0*32+3) /* HPET timer */
> +#define X86_PLATFORM_FEATURE_RTC (0*32+4) /* real time clock*/
> +#define X86_PLATFORM_FEATURE_FLOPPY (0*32+5) /* ISA floppy */
> +#define X86_PLATFORM_FEATURE_ISA (0*32+6) /* ISA/LPC bus */
> +#define X86_PLATFORM_FEATURE_BIOS (0*32+7) /* BIOS service,
> + * e.g. int calls
> + * EBDA, etc.
> + */
> +#define X86_PLATFORM_FEATURE_ACPI (0*32+8) /* has ACPI support */
> +#define X86_PLATFORM_FEATURE_SFI (0*32+9) /* has SFI support */
> +#define X86_PLATFORM_FEATURE_8042 (0*32+10) /* i8042 KBC */

i think a better and cleaner approach would be to 'driverize' the
affected platform details - not sprinkle the code with
platform_has() calls.

Whether a given platform has a 'standard PC' or some custom driver
then depends on that driver's init sequence - it's not a central
thing.

Ingo

2009-06-26 09:13:47

by Alan

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


> Whether a given platform has a 'standard PC' or some custom driver
> then depends on that driver's init sequence - it's not a central
> thing.

How exactly do you propose to handle boot memory allocation (eg EBDA)
with a driver ? Perhaps you can explain "driverize" as it isn't in any
dictionary and I'm not at all clear what you are on about ?

2009-06-26 09:45:32

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


* Alan Cox <[email protected]> wrote:

> > Whether a given platform has a 'standard PC' or some custom
> > driver then depends on that driver's init sequence - it's not a
> > central thing.
>
> How exactly do you propose to handle boot memory allocation (eg
> EBDA) with a driver ? Perhaps you can explain "driverize" as it
> isn't in any dictionary and I'm not at all clear what you are on
> about ?

Which bit is not clear? We've been continuously 'driverizing' the
jumbled mess of x86 platform support code in the past ~1-2 years.
Things like x86_quirks or struct apic and other similar code
restructuring and cleanups. They are (of course) not classic 'driver
core' drivers (many of the core kernel facilities are not available
yet at this early stage), but properly abstracted, function vector
driven approaches are still a must.

This series of patches increases the mess in that area, and that's a
step backwards. It sprinkles the code with a fair amount of 'if
(feature_x)' conditions instead of cleanly separating out proper
abstractions.

Ingo

2009-06-26 10:16:23

by Alan

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags

> Which bit is not clear? We've been continuously 'driverizing' the

What do you mean by "driverizing". You keep using your made up word but
since nobody else knows what you mean its not productive to have the
discussion using invented terms like that.

> core' drivers (many of the core kernel facilities are not available
> yet at this early stage), but properly abstracted, function vector
> driven approaches are still a must.

So like the PPC machine vector ?

> This series of patches increases the mess in that area, and that's a
> step backwards. It sprinkles the code with a fair amount of 'if
> (feature_x)' conditions instead of cleanly separating out proper
> abstractions.

That makes sense

2009-06-26 10:47:21

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


* Alan Cox <[email protected]> wrote:

> > Which bit is not clear? We've been continuously 'driverizing'
> > the
>
> What do you mean by "driverizing". You keep using your made up
> word but since nobody else knows what you mean its not productive
> to have the discussion using invented terms like that.

Yeah, it's a made up shortcut for "turning previously monolithic
code into structured, driver-alike code with a driver data
structure, possible data fields and function callbacks". See the x86
examples i cited.

Ingo

2009-06-26 11:40:59

by Alan

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags

On Fri, 26 Jun 2009 12:47:03 +0200
Ingo Molnar <[email protected]> wrote:

>
> * Alan Cox <[email protected]> wrote:
>
> > > Which bit is not clear? We've been continuously 'driverizing'
> > > the
> >
> > What do you mean by "driverizing". You keep using your made up
> > word but since nobody else knows what you mean its not productive
> > to have the discussion using invented terms like that.
>
> Yeah, it's a made up shortcut for "turning previously monolithic
> code into structured, driver-alike code with a driver data
> structure, possible data fields and function callbacks". See the x86
> examples i cited.

ITYM Object orientation ?

So do you want a PPC like platform object or some kind of platform object
that creates timer and other objects which are separate ?

2009-06-26 15:47:53

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


* Pan, Jacob jun <[email protected]> wrote:

> +/* X86 base platform features, include PC, legacy free MID devices, etc.
> + * This list provides early and important information to the kernel in a
> + * centralized place such that kernel can make a decision on the best
> + * choice of which system devices to use. e.g. timers or interrupt
> + * controllers.
> + */
> +#define X86_PLATFORM_FEATURE_8259 (0*32+0) /* i8259A PIC */
> +#define X86_PLATFORM_FEATURE_8254 (0*32+1) /* i8253/4 PIT */
> +#define X86_PLATFORM_FEATURE_IOAPIC (0*32+2) /* IO-APIC */
> +#define X86_PLATFORM_FEATURE_HPET (0*32+3) /* HPET timer */
> +#define X86_PLATFORM_FEATURE_RTC (0*32+4) /* real time clock*/
> +#define X86_PLATFORM_FEATURE_FLOPPY (0*32+5) /* ISA floppy */
> +#define X86_PLATFORM_FEATURE_ISA (0*32+6) /* ISA/LPC bus */
> +#define X86_PLATFORM_FEATURE_BIOS (0*32+7) /* BIOS service,
> + * e.g. int calls
> + * EBDA, etc.
> + */
> +#define X86_PLATFORM_FEATURE_ACPI (0*32+8) /* has ACPI support */
> +#define X86_PLATFORM_FEATURE_SFI (0*32+9) /* has SFI support */
> +#define X86_PLATFORM_FEATURE_8042 (0*32+10) /* i8042 KBC */

Btw., this enumeration of basic PC features isnt bad in itself - and
if there's a boot-flag based detection method (like on MRST) then
this can convey a 'should this platform driver attempt to
initialize' information to the driver, and rather cleanly so.

But there's bad uses of this as well, and those bad uses seem to
dominate this patch-set. For example:

@@ -504,7 +514,11 @@ static void add_pin_to_irq_node(struct irq_cfg *cfg, int node, int apic, int pin

entry = cfg->irq_2_pin;
if (!entry) {
- entry = get_one_free_irq_2_pin(node);
+ /* Setup APB timer 0 is prior to kzalloc() becomes ready */
+ if (platform_has(X86_PLATFORM_FEATURE_APBT) && (!pin)) {
+ entry = get_one_free_irq_2_pin_early(node);
+ } else
+ entry = get_one_free_irq_2_pin(node);

static inline void mach_prepare_counter(void)
{
- /* Set the Gate high, disable speaker */
- outb((inb(0x61) & ~0x02) | 0x01, 0x61);
+ if (platform_has(X86_PLATFORM_FEATURE_APBT)) {
+ apbt_prepare_count(CALIBRATE_TIME_MSEC);
+ return;
+ }
+ /* Set the Gate high, disable speaker */
+ if (platform_has(X86_PLATFORM_FEATURE_8254))
+ outb((inb(0x61) & ~0x02) | 0x01, 0x61);


static inline void mach_countup(unsigned long *count_p)
{
unsigned long count = 0;
+ if (platform_has(X86_PLATFORM_FEATURE_APBT)) {
+ apbt_countup(count_p);
+ return;
+ }
do {
count++;
} while ((inb_p(0x61) & 0x20) == 0);


Basically, if you see two different pieces of hardware used in the
same function, next to each other, separated by some 'if
(platform_has)' (or other flaggery) that's a clear sign that this
bit should be abstracted out.

Bits that delimit initialization:

@@ -29,7 +31,9 @@ void __init i386_start_kernel(void)
reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
}
#endif
- reserve_ebda_region();
+
+ if (platform_has(X86_PLATFORM_FEATURE_BIOS))
+ reserve_ebda_region();

Should probably be pushed out into x86_quirks, to give other
subarchitectures a chance to turn off EBDA scanning as well.

and generally, instead of open-coding the check:

#ifdef CONFIG_X86_32
- probe_roms();
+ if (platform_has(X86_PLATFORM_FEATURE_BIOS))
+ probe_roms();
#endif

it would be cleaner to add a:

if (!platform_has(X86_PLATFORM_FEATURE_BIOS))
return;

early into probe_roms().

Ingo

2009-06-26 19:04:17

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags

Ingo Molnar wrote:
>
> Btw., this enumeration of basic PC features isnt bad in itself - and
> if there's a boot-flag based detection method (like on MRST) then
> this can convey a 'should this platform driver attempt to
> initialize' information to the driver, and rather cleanly so.
>

That is the whole point of this, yes. Furthermore, it is expected that
we *also* would be able to set platform flags based on early probes.

> But there's bad uses of this as well, and those bad uses seem to
> dominate this patch-set.

Not entirely surprising, since this stuff is more or less randomly
sprinkled through the existing code. We have a huge bunch of legacy
code and yes, it needs cleanup.

This is a huge opportunity, of course, but it's going to be a lot of
work. A lot of this is likely to overlap directly with the needs of
Xen, too.

-hpa

2009-06-26 19:14:10

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


* H. Peter Anvin <[email protected]> wrote:

> Ingo Molnar wrote:
> >
> > Btw., this enumeration of basic PC features isnt bad in itself - and
> > if there's a boot-flag based detection method (like on MRST) then
> > this can convey a 'should this platform driver attempt to
> > initialize' information to the driver, and rather cleanly so.
> >
>
> That is the whole point of this, yes. [...]

Except that about half of all the actual uses of platform_has() are
all but related to the "should this driver initialize" question ;-)

They are more used as "fudge platform code a bit here and there to
meet the limited constraints of the platform".

And that kind of fudging is a problem, obviously.

> [...] Furthermore, it is expected that we *also* would be able to
> set platform flags based on early probes.

Yeah.

> > But there's bad uses of this as well, and those bad uses seem to
> > dominate this patch-set.
>
> Not entirely surprising, since this stuff is more or less randomly
> sprinkled through the existing code. We have a huge bunch of
> legacy code and yes, it needs cleanup.
>
> This is a huge opportunity, of course, but it's going to be a lot
> of work. A lot of this is likely to overlap directly with the
> needs of Xen, too.

Correct.

Ingo

2009-07-01 09:01:17

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags


> +#define X86_PLATFORM_FEATURE_FLOPPY (0*32+5) /* ISA floppy */
> +#define X86_PLATFORM_FEATURE_ISA (0*32+6) /* ISA/LPC bus */
> +#define X86_PLATFORM_FEATURE_BIOS (0*32+7) /* BIOS service,
> + * e.g. int calls
> + * EBDA, etc.
> + */
> +#define X86_PLATFORM_FEATURE_ACPI (0*32+8) /* has ACPI support */
> +#define X86_PLATFORM_FEATURE_SFI (0*32+9) /* has SFI support */
> +#define X86_PLATFORM_FEATURE_8042 (0*32+10) /* i8042 KBC */
> +
> +extern __u32 platform_feature[N_PLATFORM_CAPINTS];
> +extern const char *const
> + x86_platform_available_feature[N_PLATFORM_CAPINTS * 32];
> +#define platform_has(bit) \
> + test_bit(bit, (unsigned long *)platform_feature)

test_bit and friends imply synchronization you probably don't want or
need...

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2009-07-02 23:09:19

by Jacob Pan

[permalink] [raw]
Subject: RE: [PATCH 2/9] x86: introduce a set of platform feature flags

>> +#define platform_has(bit) \
>> + test_bit(bit, (unsigned long *)platform_feature)
>
>test_bit and friends imply synchronization you probably don't want or
>need...
>
[[JPAN]] could you explain a little more? The disassembly shows the test_bit
ends up as a testb instruction and a jump. Why there is synchronization
involved?
Perhaps on some architectures?

2009-07-02 23:25:43

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 2/9] x86: introduce a set of platform feature flags

Pan, Jacob jun wrote:
>>> +#define platform_has(bit) \
>>> + test_bit(bit, (unsigned long *)platform_feature)
>> test_bit and friends imply synchronization you probably don't want or
>> need...
>>
> [[JPAN]] could you explain a little more? The disassembly shows the test_bit
> ends up as a testb instruction and a jump. Why there is synchronization
> involved?
> Perhaps on some architectures?

test_bit() doesn't imply synchronization; set_bit() and clear_bit() do
(but not __set_bit() and __clear_bit(), just to really be confusing.)

-hpa