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

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

[Pan, Jacob jun]

>From 41685ae1e8b8e77fdacbdf8e8155f1e54624cbef Mon Sep 17 00:00:00 2001
From: Jacob Pan <jacob.jun.pan@intel.com>
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 <jacob.jun.pan@intel.com>
---
 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 (jacob.jun.pan@intel.com)
+ *
+ * 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

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

[Ingo Molnar]

* Pan, Jacob jun <jacob.jun.pan@intel.com> wrote:

> >From 41685ae1e8b8e77fdacbdf8e8155f1e54624cbef Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> 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 <jacob.jun.pan@intel.com>
> ---
>  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 (jacob.jun.pan@intel.com)
> + *
> + * 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

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

[Alan Cox]

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

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

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> 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

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

[Alan Cox]

> 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

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

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> 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

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

[Alan Cox]

On Fri, 26 Jun 2009 12:47:03 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Alan Cox <alan@lxorguk.ukuu.org.uk> 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 ?

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

[Ingo Molnar]

* Pan, Jacob jun <jacob.jun.pan@intel.com> 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

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

[H. Peter Anvin]

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

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

[Ingo Molnar]

* H. Peter Anvin <hpa@linux.intel.com> 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

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

[Pavel Machek]

> +#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

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

[Pan, Jacob jun]

>> +#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?

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

[H. Peter Anvin]

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