x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

On ACPI hardware reduced platform, the legacy PIC and PIT may not be initialized even though they may be present in silicon. Touching these legacy components causes unexpected result on system.

On Bay Trail-T(ASUS-T100) platform, touching these legacy components blocks platform hardware low idle power state(S0ix) during system suspend. So we should bypass them on ACPI hardware reduced platform.

Suggested-by Arjan van de Ven <[email protected]>

Signed-off-by Li Aubrey <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/irqinit.c | 6 +++++-
 arch/x86/kernel/time.c    | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/kernel/irqinit.c +5 -1

index 70e181e..9a64cc3 100644
@@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	init_bsp_APIC();
 #endif
-	legacy_pic->init(0);
+	if (acpi_gbl_reduced_hardware) {
+		pr_info("Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+	} else
+		legacy_pic->init(0);
 
 	for (i = 0; i < nr_legacy_irqs(); i++)
 		irq_set_chip_and_handler(i, chip, handle_level_irq);

arch/x86/kernel/time.c +1 -0

index 25adc0e..5ba94fa 100644
@@ -14,6 +14,7 @@
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
+#include <linux/acpi.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>

Quoted 47 lines · Li, Aubrey

On ACPI hardware reduced platform, the legacy PIC and PIT may not be initialized even though they may be present in silicon. Touching these legacy components causes unexpected result on system.

On Bay Trail-T(ASUS-T100) platform, touching these legacy components blocks platform hardware low idle power state(S0ix) during system suspend. So we should bypass them on ACPI hardware reduced platform.

Suggested-by Arjan van de Ven <[email protected]>

Signed-off-by Li Aubrey <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/irqinit.c | 6 +++++-
 arch/x86/kernel/time.c    | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/kernel/irqinit.c +5 -1

index 70e181e..9a64cc3 100644
@@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	init_bsp_APIC();
 #endif
-	legacy_pic->init(0);
+	if (acpi_gbl_reduced_hardware) {
+		pr_info("Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+	} else
+		legacy_pic->init(0);
 
 	for (i = 0; i < nr_legacy_irqs(); i++)
 		irq_set_chip_and_handler(i, chip, handle_level_irq);

arch/x86/kernel/time.c +1 -0

index 25adc0e..5ba94fa 100644
@@ -14,6 +14,7 @@
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
+#include <linux/acpi.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>

arch/x86/kernel/time.c void __init setup_default_timer_irq(void) · line 83

Quoted 8 lines

arch/x86/kernel/time.c +1 -1

@@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
 /* Default timer init function */
 void __init hpet_time_init(void)
 {
-	if (!hpet_enable())
+	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
 		setup_pit_timer();
 	setup_default_timer_irq();
 }

So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes various hardware drivers that have little relation to each other...

Instead of having a proper platform init this flag hooks into various drivers and generic code, such as the efi reboot and shutdown code, and now the generic irq init code.

For this IRQ init problem, why not add a proper callback to x86_platform_ops, define your own IRQ init function, initialize it in your platform init sequence and let it be called? That solves it without creating an ugly mix of different platform methods.

For the EFI shutdown case, what's wrong with setting your own pm_power_off handler like most of the other platforms are doing? Plus the EFI code in drivers/firmware/efi/reboot.c should probably only set the shutdown handler if pm_power_off is still NULL.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 71 lines · Ingo Molnar

Quoted 47 lines · Li, Aubrey

On ACPI hardware reduced platform, the legacy PIC and PIT may not be initialized even though they may be present in silicon. Touching these legacy components causes unexpected result on system.

On Bay Trail-T(ASUS-T100) platform, touching these legacy components blocks platform hardware low idle power state(S0ix) during system suspend. So we should bypass them on ACPI hardware reduced platform.

Suggested-by Arjan van de Ven <[email protected]>

Signed-off-by Li Aubrey <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/irqinit.c | 6 +++++-
 arch/x86/kernel/time.c    | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/kernel/irqinit.c +5 -1

index 70e181e..9a64cc3 100644
@@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	init_bsp_APIC();
 #endif
-	legacy_pic->init(0);
+	if (acpi_gbl_reduced_hardware) {
+		pr_info("Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+	} else
+		legacy_pic->init(0);
 
 	for (i = 0; i < nr_legacy_irqs(); i++)
 		irq_set_chip_and_handler(i, chip, handle_level_irq);

arch/x86/kernel/time.c +1 -0

index 25adc0e..5ba94fa 100644
@@ -14,6 +14,7 @@
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
+#include <linux/acpi.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>

arch/x86/kernel/time.c void __init setup_default_timer_irq(void) · line 83

Quoted 8 lines

arch/x86/kernel/time.c +1 -1

@@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
 /* Default timer init function */
 void __init hpet_time_init(void)
 {
-	if (!hpet_enable())
+	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
 		setup_pit_timer();
 	setup_default_timer_irq();
 }

So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes various hardware drivers that have little relation to each other...

Instead of having a proper platform init this flag hooks into various drivers and generic code, such as the efi reboot and shutdown code, and now the generic irq init code.

For this IRQ init problem, why not add a proper callback to x86_platform_ops, define your own IRQ init function, initialize it in your platform init sequence and let it be called? That solves it without creating an ugly mix of different platform methods.

Quoted 4 lines

For the EFI shutdown case, what's wrong with setting your own pm_power_off handler like most of the other platforms are doing? Plus the EFI code in drivers/firmware/efi/reboot.c should probably only set the shutdown handler if pm_power_off is still NULL.

I think our goal is to make the code as generic as possible for all x86 platform, rather than creating a new x86 branch, I added Alan Cox for this strategy discussion.

Do you have any inputs for the patch itself?

Quoted 77 lines · Li, Aubrey

Quoted 75 lines · Ingo Molnar

Quoted 47 lines · Li, Aubrey

On ACPI hardware reduced platform, the legacy PIC and PIT may not be initialized even though they may be present in silicon. Touching these legacy components causes unexpected result on system.

On Bay Trail-T(ASUS-T100) platform, touching these legacy components blocks platform hardware low idle power state(S0ix) during system suspend. So we should bypass them on ACPI hardware reduced platform.

Suggested-by Arjan van de Ven <[email protected]>

Signed-off-by Li Aubrey <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/irqinit.c | 6 +++++-
 arch/x86/kernel/time.c    | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/kernel/irqinit.c +5 -1

index 70e181e..9a64cc3 100644
@@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	init_bsp_APIC();
 #endif
-	legacy_pic->init(0);
+	if (acpi_gbl_reduced_hardware) {
+		pr_info("Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+	} else
+		legacy_pic->init(0);
 
 	for (i = 0; i < nr_legacy_irqs(); i++)
 		irq_set_chip_and_handler(i, chip, handle_level_irq);

arch/x86/kernel/time.c +1 -0

index 25adc0e..5ba94fa 100644
@@ -14,6 +14,7 @@
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
+#include <linux/acpi.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>

arch/x86/kernel/time.c void __init setup_default_timer_irq(void) · line 83

Quoted 8 lines

arch/x86/kernel/time.c +1 -1

@@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
 /* Default timer init function */
 void __init hpet_time_init(void)
 {
-	if (!hpet_enable())
+	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
 		setup_pit_timer();
 	setup_default_timer_irq();
 }

So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes various hardware drivers that have little relation to each other...

Instead of having a proper platform init this flag hooks into various drivers and generic code, such as the efi reboot and shutdown code, and now the generic irq init code.

For this IRQ init problem, why not add a proper callback to x86_platform_ops, define your own IRQ init function, initialize it in your platform init sequence and let it be called? That solves it without creating an ugly mix of different platform methods.

For the EFI shutdown case, what's wrong with setting your own pm_power_off handler like most of the other platforms are doing? Plus the EFI code in drivers/firmware/efi/reboot.c should probably only set the shutdown handler if pm_power_off is still NULL.

Quoted 5 lines

I think our goal is to make the code as generic as possible for all x86 platform, rather than creating a new x86 branch, I added Alan Cox for this strategy discussion.

Do you have any inputs for the patch itself?

Other than that the patch is unacceptable for an upstream merge in its current form for the reason I mentioned? No.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 84 lines · Ingo Molnar

Quoted 81 lines · Li, Aubrey

Quoted 71 lines · Ingo Molnar

Quoted 47 lines · Li, Aubrey

On ACPI hardware reduced platform, the legacy PIC and PIT may not be initialized even though they may be present in silicon. Touching these legacy components causes unexpected result on system.

On Bay Trail-T(ASUS-T100) platform, touching these legacy components blocks platform hardware low idle power state(S0ix) during system suspend. So we should bypass them on ACPI hardware reduced platform.

Suggested-by Arjan van de Ven <[email protected]>

Signed-off-by Li Aubrey <[email protected]>

Cc Len Brown <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---
 arch/x86/kernel/irqinit.c | 6 +++++-
 arch/x86/kernel/time.c    | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/kernel/irqinit.c +5 -1

index 70e181e..9a64cc3 100644
@@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	init_bsp_APIC();
 #endif
-	legacy_pic->init(0);
+	if (acpi_gbl_reduced_hardware) {
+		pr_info("Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+	} else
+		legacy_pic->init(0);
 
 	for (i = 0; i < nr_legacy_irqs(); i++)
 		irq_set_chip_and_handler(i, chip, handle_level_irq);

arch/x86/kernel/time.c +1 -0

index 25adc0e..5ba94fa 100644
@@ -14,6 +14,7 @@
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
+#include <linux/acpi.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>

arch/x86/kernel/time.c void __init setup_default_timer_irq(void) · line 83

Quoted 8 lines

arch/x86/kernel/time.c +1 -1

@@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
 /* Default timer init function */
 void __init hpet_time_init(void)
 {
-	if (!hpet_enable())
+	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
 		setup_pit_timer();
 	setup_default_timer_irq();
 }

So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes various hardware drivers that have little relation to each other...

Instead of having a proper platform init this flag hooks into various drivers and generic code, such as the efi reboot and shutdown code, and now the generic irq init code.

For this IRQ init problem, why not add a proper callback to x86_platform_ops, define your own IRQ init function, initialize it in your platform init sequence and let it be called? That solves it without creating an ugly mix of different platform methods.

Quoted 4 lines

For the EFI shutdown case, what's wrong with setting your own pm_power_off handler like most of the other platforms are doing? Plus the EFI code in drivers/firmware/efi/reboot.c should probably only set the shutdown handler if pm_power_off is still NULL.

I think our goal is to make the code as generic as possible for all x86 platform, rather than creating a new x86 branch, I added Alan Cox for this strategy discussion.

Quoted 4 lines

Quoted 1 line

Do you have any inputs for the patch itself?

Other than that the patch is unacceptable for an upstream merge in its current form for the reason I mentioned? No.

So you are suggesting we extend a new x86 platform branch and override the x86_platform and pm_power_off and reboot, like what intel_mid does?

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Quoted 9 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

Hmmm.

Sort of. What we need is a "do not touch PIC/PIT" bit for the code that tries to fall back to them in some cases (which may appear to work if the hardware is physically there, but it may confuse the platform).

Quoted 3 lines · Rafael J. Wysocki

Sort of. What we need is a "do not touch PIC/PIT" bit for the code that tries to fall back to them in some cases (which may appear to work if the hardware is physically there, but it may confuse the platform).

Can "some cases" detection be nicely put into a x86_platform platform-specific method?

Quoted 3 lines · Rafael J. Wysocki

Sort of. What we need is a "do not touch PIC/PIT" bit for the code that tries to fall back to them in some cases (which may appear to work if the hardware is physically there, but it may confuse the platform).

Can "some cases" detection be nicely put into a x86_platform platform-specific method?

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Rafael J. Wysocki

Sort of. What we need is a "do not touch PIC/PIT" bit for the code that tries to fall back to them in some cases (which may appear to work if the hardware is physically there, but it may confuse the platform).

Can "some cases" detection be nicely put into a x86_platform platform-specific method?

In some cases they don't belong in x86, ACPI is also used for ARM64.

However

	if ( has_8259_pic() )

is trivally 0, 1 or some platform or acpi provided method.

Quoted 15 lines · Alan Cox

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Rafael J. Wysocki

Sort of. What we need is a "do not touch PIC/PIT" bit for the code that tries to fall back to them in some cases (which may appear to work if the hardware is physically there, but it may confuse the platform).

Can "some cases" detection be nicely put into a x86_platform platform-specific method?

In some cases they don't belong in x86, ACPI is also used for ARM64.

However

	if ( has_8259_pic() )

Quoted 1 line

is trivally 0, 1 or some platform or acpi provided method.

And which is how that should have been implemented to start with IMO.

Besides, the "ACPI reduced hardware" case is kind of a red herring here, because it most likely is not the only case when we'll want has_8259_pic() to return 0 (quite likely, we'll want that on all BayTrail-based systems, for example).

Besides, the "ACPI reduced hardware" case is kind of a red herring here, because it most likely is not the only case when we'll want has_8259_pic() to return 0 (quite likely, we'll want that on all BayTrail-based systems, for example).

Quoted 7 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

Quoted 2 lines

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

Quoted 14 lines · Borislav Petkov

Quoted 9 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

Quoted 2 lines

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

more like

platform_has(X86_PLATFORM_PIT)

etc, one for each legacy io item

so we can clear it on hw reduced, but also in other cases. hw reduced is one way, but I'd be surprised if there weren't other ways (like quirks) where we'd want to do the same things

Quoted 15 lines · Arjan van de Ven

Quoted 14 lines · Borislav Petkov

Quoted 9 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

Quoted 8 lines

Quoted 2 lines

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

more like

platform_has(X86_PLATFORM_PIT)

etc, one for each legacy io item

Precisely. The main problem is the generic, 'lumps everything together' nature of the acpi_gbl_reduced_hardware flag.

(Like the big kernel lock lumped together all sorts of locking rules and semantics.)

Properly split out, feature-ish or driver-ish interfaces for PIT and other legacy details are the proper approach to 'turn them off'.

- x86_platform is a function pointer driven, driver-ish interface.

- platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish interface.

Both are fine - for something as separate as the PIT (or the PIC) it might make more sense to go towards a 'driver' interface though, as modern drivers are (and will be) much different from the legacy PIT.

Whichever method is used, low level platforms can just switch them on/off in their enumeration/detection routines, while the generic code will have them enabled by default.

Quoted 4 lines

so we can clear it on hw reduced, but also in other cases. hw reduced is one way, but I'd be surprised if there weren't other ways (like quirks) where we'd want to do the same things

Exactly. The key step is the proper, clean separation out of hardware interfaces.

Thanks,

Ingo

Quoted 41 lines · Ingo Molnar

Quoted 15 lines · Arjan van de Ven

Quoted 14 lines · Borislav Petkov

Quoted 9 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

Quoted 8 lines

Quoted 2 lines

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

more like

platform_has(X86_PLATFORM_PIT)

etc, one for each legacy io item

Precisely. The main problem is the generic, 'lumps everything together' nature of the acpi_gbl_reduced_hardware flag.

(Like the big kernel lock lumped together all sorts of locking rules and semantics.)

Properly split out, feature-ish or driver-ish interfaces for PIT and other legacy details are the proper approach to 'turn them off'.

- x86_platform is a function pointer driven, driver-ish interface.

- platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish interface.

Quoted 8 lines

Both are fine - for something as separate as the PIT (or the PIC) it might make more sense to go towards a 'driver' interface though, as modern drivers are (and will be) much different from the legacy PIT.

Whichever method is used, low level platforms can just switch them on/off in their enumeration/detection routines, while the generic code will have them enabled by default.

Whichever method is used, we will face a problem how to determine PIT exists or not.

When we enabled Bay Trail-T platform at the beginning, we were trying to make the code as generic as possible, and it works properly up to now. So we don't have a SUBARCH like X86_SUBARCH_INTEL_MID to use the platform specific functions. And for now I'm not quite sure it's a good idea to create one.

If we make it as a flag driven, I don't know there is a flag in firmware better than ACPI HW reduced flag(Of course it's not good enough to cover all the cases). Or if we want to use platform info to turn on/off this flag, we'll have to maintain a platform list, which may be longer and more complicated than worth doing that.

Quoted 60 lines · Li, Aubrey

Quoted 41 lines · Ingo Molnar

Quoted 15 lines · Arjan van de Ven

Quoted 14 lines · Borislav Petkov

Quoted 9 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

Quoted 8 lines

Quoted 2 lines

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

more like

platform_has(X86_PLATFORM_PIT)

etc, one for each legacy io item

Precisely. The main problem is the generic, 'lumps everything together' nature of the acpi_gbl_reduced_hardware flag.

(Like the big kernel lock lumped together all sorts of locking rules and semantics.)

Properly split out, feature-ish or driver-ish interfaces for PIT and other legacy details are the proper approach to 'turn them off'.

- x86_platform is a function pointer driven, driver-ish interface.

- platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish interface.

Quoted 8 lines

Both are fine - for something as separate as the PIT (or the PIC) it might make more sense to go towards a 'driver' interface though, as modern drivers are (and will be) much different from the legacy PIT.

Whichever method is used, low level platforms can just switch them on/off in their enumeration/detection routines, while the generic code will have them enabled by default.

Whichever method is used, we will face a problem how to determine PIT exists or not.

When we enabled Bay Trail-T platform at the beginning, we were trying to make the code as generic as possible, and it works properly up to now. So we don't have a SUBARCH like X86_SUBARCH_INTEL_MID to use the platform specific functions. And for now I'm not quite sure it's a good idea to create one.

Quoted 5 lines

If we make it as a flag driven, I don't know there is a flag in firmware better than ACPI HW reduced flag(Of course it's not good enough to cover all the cases). Or if we want to use platform info to turn on/off this flag, we'll have to maintain a platform list, which may be longer and more complicated than worth doing that.

Well, it's not nearly so difficult, because you already have a platform flag: acpi_gbl_reduced_hardware.

What I object against is to infest generic codepaths with unreadable, unrobust crap like:

(quoted diff) +5 -0

+       if (acpi_gbl_reduced_hardware) {
+               pr_info("Using NULL legacy PIC\n");
+               legacy_pic = &null_legacy_pic;
+       } else
+               legacy_pic->init(0);

To solve that, add a small (early) init function (say 'x86_reduced_hw_init()') that sets up the right driver selections if acpi_gbl_reduced_hardware is set:

- in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'

- clean up 'global_clock_event' handling: instead of a global variable, move its management into x86_platform_ops::get_clockevent() and set the method to hpet/pit/abp/etc. specific handlers that return the right clockevent device.

- in your x86_reduced_hw_init() function add the hpet clockevent device to x86_platform_ops::get_clockevent, overriding the default PIT.

- in x86_reduced_hw_init() set pm_power_off.

- set 'reboot_type' and remove the acpi_gbl_reduced_hardware hack from efi_reboot_required().

etc.

Just keep the generic init codepaths free of those random selections based on global flags, okay?

Thanks,

Ingo

I'll do more investigation above items but I want to leave at least these two as the quirk today unless I am convinced I can do that because from my understanding, UEFI runtime services should not be supported in reduced hw mode.

Quoted 92 lines · Ingo Molnar

Quoted 60 lines · Li, Aubrey

Quoted 41 lines · Ingo Molnar

Quoted 15 lines · Arjan van de Ven

Quoted 14 lines · Borislav Petkov

Quoted 9 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

Quoted 8 lines

Quoted 2 lines

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..

more like

platform_has(X86_PLATFORM_PIT)

etc, one for each legacy io item

Precisely. The main problem is the generic, 'lumps everything together' nature of the acpi_gbl_reduced_hardware flag.

(Like the big kernel lock lumped together all sorts of locking rules and semantics.)

Properly split out, feature-ish or driver-ish interfaces for PIT and other legacy details are the proper approach to 'turn them off'.

- x86_platform is a function pointer driven, driver-ish interface.

- platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish interface.

Quoted 8 lines

Both are fine - for something as separate as the PIT (or the PIC) it might make more sense to go towards a 'driver' interface though, as modern drivers are (and will be) much different from the legacy PIT.

Whichever method is used, low level platforms can just switch them on/off in their enumeration/detection routines, while the generic code will have them enabled by default.

Whichever method is used, we will face a problem how to determine PIT exists or not.

When we enabled Bay Trail-T platform at the beginning, we were trying to make the code as generic as possible, and it works properly up to now. So we don't have a SUBARCH like X86_SUBARCH_INTEL_MID to use the platform specific functions. And for now I'm not quite sure it's a good idea to create one.

Quoted 5 lines

If we make it as a flag driven, I don't know there is a flag in firmware better than ACPI HW reduced flag(Of course it's not good enough to cover all the cases). Or if we want to use platform info to turn on/off this flag, we'll have to maintain a platform list, which may be longer and more complicated than worth doing that.

Well, it's not nearly so difficult, because you already have a platform flag: acpi_gbl_reduced_hardware.

What I object against is to infest generic codepaths with unreadable, unrobust crap like:

(quoted diff) +5 -0

+       if (acpi_gbl_reduced_hardware) {
+               pr_info("Using NULL legacy PIC\n");
+               legacy_pic = &null_legacy_pic;
+       } else
+               legacy_pic->init(0);

To solve that, add a small (early) init function (say 'x86_reduced_hw_init()') that sets up the right driver selections if acpi_gbl_reduced_hardware is set:

- in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'

- clean up 'global_clock_event' handling: instead of a global variable, move its management into x86_platform_ops::get_clockevent() and set the method to hpet/pit/abp/etc. specific handlers that return the right clockevent device.

Quoted 4 lines

- in your x86_reduced_hw_init() function add the hpet clockevent device to x86_platform_ops::get_clockevent, overriding the default PIT.

Quoted 6 lines

- in x86_reduced_hw_init() set pm_power_off.

- set 'reboot_type' and remove the acpi_gbl_reduced_hardware hack from efi_reboot_required().

I'll do more investigation above items but I want to leave at least these two as the quirk today unless I am convinced I can do that because from my understanding, UEFI runtime services should not be supported in reduced hw mode.

Quoted 10 lines

Quoted 10 lines

- in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'

- clean up 'global_clock_event' handling: instead of a global variable, move its management into x86_platform_ops::get_clockevent() and set the method to hpet/pit/abp/etc. specific handlers that return the right clockevent device.

- in your x86_reduced_hw_init() function add the hpet clockevent device to x86_platform_ops::get_clockevent, overriding the default PIT.

how about this one?

arch/x86/kernel/acpi/boot.c +0 -0

index b9e30da..70955d6 100644
@@ -1541,6 +1541,16 @@ int __init early_acpi_boot_init(void)
 	 */
 	early_acpi_process_madt();

If the above makes sense, I'll send poweroff and reboot change together in a seperate patch.

Quoted 21 lines · Li, Aubrey

Quoted 10 lines

Quoted 10 lines

- in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'

- clean up 'global_clock_event' handling: instead of a global variable, move its management into x86_platform_ops::get_clockevent() and set the method to hpet/pit/abp/etc. specific handlers that return the right clockevent device.

- in your x86_reduced_hw_init() function add the hpet clockevent device to x86_platform_ops::get_clockevent, overriding the default PIT.

how about this one?

arch/x86/kernel/acpi/boot.c +0 -0

index b9e30da..70955d6 100644
@@ -1541,6 +1541,16 @@ int __init early_acpi_boot_init(void)
 	 */
 	early_acpi_process_madt();
 

arch/x86/kernel/acpi/boot.c int __init early_acpi_boot_init(void) · line 1552

Quoted 9 lines

arch/x86/kernel/acpi/boot.c +9 -0

+	/*
+	 * Override x86_init functions and bypass legacy pic
+	 * in hardware-reduced ACPI mode
+	 */
+	if (acpi_gbl_reduced_hardware) {
+		x86_init.timers.timer_init = x86_init_noop;
+		x86_init.irqs.pre_vector_init = x86_init_noop;
+		legacy_pic = &null_legacy_pic;
+	}

Sounds good to me, assuming it builds, boots and works! :-)

A couple of extra suggestions:

1)

I'd suggest moving it into its own dedicated, appropriately named static function, to make it clear that 'ACPI Reduced Hardware' init happens there.

2)

I'd also initialize it like this:

		x86_init.timers.timer_init	= x86_init_noop;
		x86_init.irqs.pre_vector_init	= x86_init_noop;
		legacy_pic			= &null_legacy_pic;

to make the noop patterns stand out better.

3)

Once all is said and done, please also make acpi_gbl_reduced_hardware a flag internal to the ACPI code, not exposed to and used by other bits of x86 code.

Quoted 2 lines

If the above makes sense, I'll send poweroff and reboot change together in a seperate patch.

Yeah, please send them in a single series though, so they form a logical group.

Quoted 7 lines · Arjan van de Ven

Quoted 3 lines

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings, for example, having that flag should cause the 8042 input code to not probe for the 8042.

Quoted 2 lines

for interrupts, there really ought to be a "apic first/only" mode, which is then used on all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too, similar to cpu_has()? I.e., platform_has()...

if (platform_has(X86_PLATFORM_REDUCED_HW))
do stuff..