[alsa-devel] [PATCH v2 00/12] treewide: break dependencies on x86's RM header
x86's asm/realmode.h, which defines low level structures, variables and helpers used to bring up APs during SMP boot, ends up getting included in practically every nook and cranny of the kernel because the address used by ACPI for resuming from S3 also happens to be stored in the real mode header, and ACPI bleeds the dependency into its widely included headers.
As a result, modifying realmode.h for even the most trivial change to the boot code triggers a full kernel rebuild, which is frustrating to say the least as it some of the most difficult code to get exactly right *and* is also some of the most functionally isolated code in the kernel.
To break the kernel's widespread dependency on realmode.h, add a wrapper in the aforementioned ACPI S3 code to access the real mode header instead of derefencing the header directly in asm/acpi.h and thereby exposing it to the world via linux/acpi.h.
v2: - Rebased on tip/x86/cleanups, commit b74374fef924 ("x86/setup: Enhance the comments"). - Use acpi_get_wakeup_address() as new function name. [Boris and Pavel] - Capture acpi_get_wakeup_address() in a local address. [Pavel] - Collect acks. I didn't add Rafael's acks on patches 11 and 12 due to the above changes. - Explicitly call out the removal of <asm/realmode.h> from <asm/acpi.h> in patch 12. [Ingo] - Remove superfluous Fixes: tags. [Ard]
Patch Synopsis: - Patches 01-09 fix a variety of build errors that arise when patch 12 drops realmode.h from asm/acpi.h. Most of the errors are quite absurb as they have no relation whatsoever to x86's RM boot code, but occur because realmode.h happens to include asm/io.h.
- Patch 10 removes a spurious include of realmode.h from an ACPI header.
- Patches 11 and 12 implement the wrapper and move it out of acpi.h.
Sean Christopherson (12): x86/efi: Explicitly include realmode.h to handle RM trampoline quirk x86/boot: Explicitly include realmode.h to handle RM reservations x86/ftrace: Explicitly include vmalloc.h for set_vm_flush_reset_perms() x86/kprobes: Explicitly include vmalloc.h for set_vm_flush_reset_perms() perf/x86/intel: Explicitly include asm/io.h to use virt_to_phys() efi/capsule-loader: Explicitly include linux/io.h for page_to_phys() virt: vbox: Explicitly include linux/io.h to pick up various defs vmw_balloon: Explicitly include linux/io.h for virt_to_phys() ASoC: Intel: Skylake: Explicitly include linux/io.h for virt_to_phys() x86/ACPI/sleep: Remove an unnecessary include of asm/realmode.h ACPI/sleep: Convert acpi_wakeup_address into a function x86/ACPI/sleep: Move acpi_get_wakeup_address() into sleep.c, remove <asm/realmode.h> from <asm/acpi.h>
arch/ia64/include/asm/acpi.h | 5 ++++- arch/ia64/kernel/acpi.c | 2 -- arch/x86/events/intel/ds.c | 1 + arch/x86/include/asm/acpi.h | 3 +-- arch/x86/kernel/acpi/sleep.c | 11 +++++++++++ arch/x86/kernel/acpi/sleep.h | 2 +- arch/x86/kernel/ftrace.c | 1 + arch/x86/kernel/kprobes/core.c | 1 + arch/x86/kernel/setup.c | 1 + arch/x86/platform/efi/quirks.c | 1 + drivers/acpi/sleep.c | 3 +++ drivers/firmware/efi/capsule-loader.c | 1 + drivers/misc/vmw_balloon.c | 1 + drivers/virt/vboxguest/vboxguest_core.c | 1 + drivers/virt/vboxguest/vboxguest_utils.c | 1 + sound/soc/intel/skylake/skl-sst-cldma.c | 1 + 16 files changed, 30 insertions(+), 6 deletions(-)
Explicitly include asm/realmode.h, which is needed to handle a real mode trampoline quirk in efi_free_boot_services(), instead of picking it up by way of linux/acpi.h. acpi.h will soon stop including realmode.h so that changing realmode.h doesn't require a full kernel rebuild.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/platform/efi/quirks.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c index 3b9fd679cea9..f9ef5c5346ca 100644 --- a/arch/x86/platform/efi/quirks.c +++ b/arch/x86/platform/efi/quirks.c @@ -16,6 +16,7 @@ #include <asm/efi.h> #include <asm/uv/uv.h> #include <asm/cpu_device_id.h> +#include <asm/realmode.h> #include <asm/reboot.h>
#define EFI_MIN_RESERVE 5120
Explicitly include asm/realmode.h, which provides reserve_real_mode(), instead of picking it up by an indirect include of asm/acpi.h. acpi.h will soon stop including realmode.h so that changing realmode.h doesn't require a full kernel rebuild.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/kernel/setup.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index a87138186724..38dfb61cacb8 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -35,6 +35,7 @@ #include <asm/kaslr.h> #include <asm/mce.h> #include <asm/mtrr.h> +#include <asm/realmode.h> #include <asm/olpc_ofw.h> #include <asm/pci-direct.h> #include <asm/prom.h>
The inclusion of linux/vmalloc.h, which is required for its definition of set_vm_flush_reset_perms(), is somehow dependent on asm/realmode.h being included by asm/acpi.h. Explicitly include linux/vmalloc.h so that a future patch can drop the realmode.h include from asm/acpi.h without breaking the build.
Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/kernel/ftrace.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 024c3053dbba..2009047bb015 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -23,6 +23,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/memory.h> +#include <linux/vmalloc.h>
#include <trace/syscall.h>
The inclusion of linux/vmalloc.h, which is required for its definition of set_vm_flush_reset_perms(), is somehow dependent on asm/realmode.h being included by asm/acpi.h. Explicitly include linux/vmalloc.h so that a future patch can drop the realmode.h include from asm/acpi.h without breaking the build.
Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/kernel/kprobes/core.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 43fc13c831af..e7d4c89c9aba 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -40,6 +40,7 @@ #include <linux/frame.h> #include <linux/kasan.h> #include <linux/moduleloader.h> +#include <linux/vmalloc.h>
#include <asm/text-patching.h> #include <asm/cacheflush.h>
Through a labyrinthian sequence of includes, usage of virt_to_phys() is dependent on the include of asm/io.h in asm/realmode.h via asm/acpi.h. Explicitly include asm/io.h to break the dependency on realmode.h so that a future patch can remove the realmode.h include from acpi.h without breaking the build.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/events/intel/ds.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c index ce83950036c5..4b94ae4ae369 100644 --- a/arch/x86/events/intel/ds.c +++ b/arch/x86/events/intel/ds.c @@ -7,6 +7,7 @@ #include <asm/perf_event.h> #include <asm/tlbflush.h> #include <asm/insn.h> +#include <asm/io.h>
#include "../perf_event.h"
Through a labyrinthian sequence of includes, usage of page_to_phys() is dependent on the include of asm/io.h in x86's asm/realmode.h, which is included in x86's asm/acpi.h and thus by linux/acpi.h. Explicitly include linux/io.h to break the dependency on realmode.h so that a future patch can remove the realmode.h include from acpi.h without breaking the build.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- drivers/firmware/efi/capsule-loader.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c index b1395133389e..d3067cbd5114 100644 --- a/drivers/firmware/efi/capsule-loader.c +++ b/drivers/firmware/efi/capsule-loader.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/miscdevice.h> #include <linux/highmem.h> +#include <linux/io.h> #include <linux/slab.h> #include <linux/mutex.h> #include <linux/efi.h>
Through a labyrinthian sequence of includes, usage of page_to_phys(), virt_to_phys() and out*() is dependent on the include of asm/io.h in x86's asm/realmode.h, which is included in x86's asm/acpi.h and thus by linux/acpi.h. Explicitly include linux/io.h to break the dependency on realmode.h so that a future patch can remove the realmode.h include from acpi.h without breaking the build.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- drivers/virt/vboxguest/vboxguest_core.c | 1 + drivers/virt/vboxguest/vboxguest_utils.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/drivers/virt/vboxguest/vboxguest_core.c b/drivers/virt/vboxguest/vboxguest_core.c index 2307b0329aec..d823d558c0c4 100644 --- a/drivers/virt/vboxguest/vboxguest_core.c +++ b/drivers/virt/vboxguest/vboxguest_core.c @@ -6,6 +6,7 @@ */
#include <linux/device.h> +#include <linux/io.h> #include <linux/mm.h> #include <linux/sched.h> #include <linux/sizes.h> diff --git a/drivers/virt/vboxguest/vboxguest_utils.c b/drivers/virt/vboxguest/vboxguest_utils.c index 75fd140b02ff..80e0f1254476 100644 --- a/drivers/virt/vboxguest/vboxguest_utils.c +++ b/drivers/virt/vboxguest/vboxguest_utils.c @@ -7,6 +7,7 @@ */
#include <linux/errno.h> +#include <linux/io.h> #include <linux/kernel.h> #include <linux/mm.h> #include <linux/module.h>
Through a labyrinthian sequence of includes, usage of virt_to_phys() is dependent on the include of asm/io.h in x86's asm/realmode.h, which is included in x86's asm/acpi.h and thus by linux/acpi.h. Explicitly include linux/io.h to break the dependency on realmode.h so that a future patch can remove the realmode.h include from acpi.h without breaking the build.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- drivers/misc/vmw_balloon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c index 5e6be1527571..b837e7eba5f7 100644 --- a/drivers/misc/vmw_balloon.c +++ b/drivers/misc/vmw_balloon.c @@ -17,6 +17,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/types.h> +#include <linux/io.h> #include <linux/kernel.h> #include <linux/mm.h> #include <linux/vmalloc.h>
Through a labyrinthian sequence of includes, usage of virt_to_phys() is dependent on the include of asm/io.h in x86's asm/realmode.h, which is included in x86's asm/acpi.h and thus by linux/acpi.h. Explicitly include linux/io.h to break the dependency on realmode.h so that a future patch can remove the realmode.h include from acpi.h without breaking the build.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- sound/soc/intel/skylake/skl-sst-cldma.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/intel/skylake/skl-sst-cldma.c b/sound/soc/intel/skylake/skl-sst-cldma.c index 5a2c35f58fda..36f697c61074 100644 --- a/sound/soc/intel/skylake/skl-sst-cldma.c +++ b/sound/soc/intel/skylake/skl-sst-cldma.c @@ -8,6 +8,7 @@ */
#include <linux/device.h> +#include <linux/io.h> #include <linux/mm.h> #include <linux/delay.h> #include "../common/sst-dsp.h"
None of the declarations in x86's acpi/sleep.h are in any way dependent on the real mode boot code. Remove sleep.h's include of asm/realmode.h to limit the dependencies on realmode.h to code that actually interacts with the boot code.
Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/kernel/acpi/sleep.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/acpi/sleep.h b/arch/x86/kernel/acpi/sleep.h index fbb60ca4255c..d06c2079b6c1 100644 --- a/arch/x86/kernel/acpi/sleep.h +++ b/arch/x86/kernel/acpi/sleep.h @@ -3,7 +3,7 @@ * Variables and functions used by the code in sleep.c */
-#include <asm/realmode.h> +#include <linux/linkage.h>
extern unsigned long saved_video_mode; extern long saved_magic;
Convert acpi_wakeup_address from a raw variable into a function so that x86 can wrap its dereference of the real mode boot header in a function instead of broadcasting it to the world via a #define. This sets the stage for a future patch to move x86's definition of the new function, acpi_get_wakeup_address(), out of asm/acpi.h and thus break acpi.h's dependency on asm/realmode.h.
No functional change intended.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/ia64/include/asm/acpi.h | 5 ++++- arch/ia64/kernel/acpi.c | 2 -- arch/x86/include/asm/acpi.h | 5 ++++- drivers/acpi/sleep.c | 3 +++ 4 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h index f886d4dc9d55..b66ba907019c 100644 --- a/arch/ia64/include/asm/acpi.h +++ b/arch/ia64/include/asm/acpi.h @@ -38,7 +38,10 @@ int acpi_gsi_to_irq (u32 gsi, unsigned int *irq); /* Low-level suspend routine. */ extern int acpi_suspend_lowlevel(void);
-extern unsigned long acpi_wakeup_address; +static inline unsigned long acpi_get_wakeup_address(void) +{ + return 0; +}
/* * Record the cpei override flag and current logical cpu. This is diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c index 70d1587ddcd4..a5636524af76 100644 --- a/arch/ia64/kernel/acpi.c +++ b/arch/ia64/kernel/acpi.c @@ -42,8 +42,6 @@ int acpi_lapic; unsigned int acpi_cpei_override; unsigned int acpi_cpei_phys_cpuid;
-unsigned long acpi_wakeup_address = 0; - #define ACPI_MAX_PLATFORM_INTERRUPTS 256
/* Array to record platform interrupt vectors for generic interrupt routing. */ diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h index bc9693c9107e..23ffafd927a1 100644 --- a/arch/x86/include/asm/acpi.h +++ b/arch/x86/include/asm/acpi.h @@ -62,7 +62,10 @@ static inline void acpi_disable_pci(void) extern int (*acpi_suspend_lowlevel)(void);
/* Physical address to resume after wakeup */ -#define acpi_wakeup_address ((unsigned long)(real_mode_header->wakeup_start)) +static inline unsigned long acpi_get_wakeup_address(void) +{ + return ((unsigned long)(real_mode_header->wakeup_start)); +}
/* * Check if the CPU can handle C2 and deeper diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index 9fa77d72ef27..2e87ccf17ff6 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -61,8 +61,11 @@ static struct notifier_block tts_notifier = { static int acpi_sleep_prepare(u32 acpi_state) { #ifdef CONFIG_ACPI_SLEEP + unsigned long acpi_wakeup_address; + /* do we have a wakeup address for S2 and S3? */ if (acpi_state == ACPI_STATE_S3) { + acpi_wakeup_address = acpi_get_wakeup_address(); if (!acpi_wakeup_address) return -EFAULT; acpi_set_waking_vector(acpi_wakeup_address);
On Tue 2019-11-26 08:54:16, Sean Christopherson wrote:
Convert acpi_wakeup_address from a raw variable into a function so that x86 can wrap its dereference of the real mode boot header in a function instead of broadcasting it to the world via a #define. This sets the stage for a future patch to move x86's definition of the new function, acpi_get_wakeup_address(), out of asm/acpi.h and thus break acpi.h's dependency on asm/realmode.h.
No functional change intended.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com
Thanks!
Acked-by: Pavel Machek pavel@ucw.cz
Move the definition of acpi_get_wakeup_address() into sleep.c to break linux/acpi.h's dependency (by way of asm/acpi.h) on asm/realmode.h. Everyone and their mother includes linux/acpi.h, i.e. modifying realmode.h results in a full kernel rebuild, which makes the already inscrutable real mode boot code even more difficult to understand and is positively rage inducing when trying to make changes to x86's boot flow.
No functional change intended.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/include/asm/acpi.h | 6 +----- arch/x86/kernel/acpi/sleep.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h index 23ffafd927a1..ca0976456a6b 100644 --- a/arch/x86/include/asm/acpi.h +++ b/arch/x86/include/asm/acpi.h @@ -13,7 +13,6 @@ #include <asm/processor.h> #include <asm/mmu.h> #include <asm/mpspec.h> -#include <asm/realmode.h> #include <asm/x86_init.h>
#ifdef CONFIG_ACPI_APEI @@ -62,10 +61,7 @@ static inline void acpi_disable_pci(void) extern int (*acpi_suspend_lowlevel)(void);
/* Physical address to resume after wakeup */ -static inline unsigned long acpi_get_wakeup_address(void) -{ - return ((unsigned long)(real_mode_header->wakeup_start)); -} +unsigned long acpi_get_wakeup_address(void);
/* * Check if the CPU can handle C2 and deeper diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c index ca13851f0570..26b7256f590f 100644 --- a/arch/x86/kernel/acpi/sleep.c +++ b/arch/x86/kernel/acpi/sleep.c @@ -26,6 +26,17 @@ unsigned long acpi_realmode_flags; static char temp_stack[4096]; #endif
+/** + * acpi_get_wakeup_address - provide physical address for S3 wakeup + * + * Returns the physical address where the kernel should be resumed after the + * system awakes from S3, e.g. for programming into the firmware waking vector. + */ +unsigned long acpi_get_wakeup_address(void) +{ + return ((unsigned long)(real_mode_header->wakeup_start)); +} + /** * x86_acpi_enter_sleep_state - enter sleep state * @state: Sleep state to enter.
* Sean Christopherson sean.j.christopherson@intel.com wrote:
x86's asm/realmode.h, which defines low level structures, variables and helpers used to bring up APs during SMP boot, ends up getting included in practically every nook and cranny of the kernel because the address used by ACPI for resuming from S3 also happens to be stored in the real mode header, and ACPI bleeds the dependency into its widely included headers.
As a result, modifying realmode.h for even the most trivial change to the boot code triggers a full kernel rebuild, which is frustrating to say the least as it some of the most difficult code to get exactly right *and* is also some of the most functionally isolated code in the kernel.
To break the kernel's widespread dependency on realmode.h, add a wrapper in the aforementioned ACPI S3 code to access the real mode header instead of derefencing the header directly in asm/acpi.h and thereby exposing it to the world via linux/acpi.h.
v2:
- Rebased on tip/x86/cleanups, commit b74374fef924 ("x86/setup: Enhance the comments").
- Use acpi_get_wakeup_address() as new function name. [Boris and Pavel]
- Capture acpi_get_wakeup_address() in a local address. [Pavel]
- Collect acks. I didn't add Rafael's acks on patches 11 and 12 due to the above changes.
- Explicitly call out the removal of <asm/realmode.h> from <asm/acpi.h> in patch 12. [Ingo]
- Remove superfluous Fixes: tags. [Ard]
You didn't include every patch from v1 though, such us my fix to Quark:
[PATCH] x86/platform/intel/quark: Explicitly include linux/io.h for virt_to_phys()
I've applied that one too and your updated patches, and it's now all pushed out into tip:WIP.core/headers.
Thanks,
Ingo
On Wed, Nov 27, 2019 at 08:20:57AM +0100, Ingo Molnar wrote:
- Sean Christopherson sean.j.christopherson@intel.com wrote:
x86's asm/realmode.h, which defines low level structures, variables and helpers used to bring up APs during SMP boot, ends up getting included in practically every nook and cranny of the kernel because the address used by ACPI for resuming from S3 also happens to be stored in the real mode header, and ACPI bleeds the dependency into its widely included headers.
As a result, modifying realmode.h for even the most trivial change to the boot code triggers a full kernel rebuild, which is frustrating to say the least as it some of the most difficult code to get exactly right *and* is also some of the most functionally isolated code in the kernel.
To break the kernel's widespread dependency on realmode.h, add a wrapper in the aforementioned ACPI S3 code to access the real mode header instead of derefencing the header directly in asm/acpi.h and thereby exposing it to the world via linux/acpi.h.
v2:
- Rebased on tip/x86/cleanups, commit b74374fef924 ("x86/setup: Enhance the comments").
- Use acpi_get_wakeup_address() as new function name. [Boris and Pavel]
- Capture acpi_get_wakeup_address() in a local address. [Pavel]
- Collect acks. I didn't add Rafael's acks on patches 11 and 12 due to the above changes.
- Explicitly call out the removal of <asm/realmode.h> from <asm/acpi.h> in patch 12. [Ingo]
- Remove superfluous Fixes: tags. [Ard]
You didn't include every patch from v1 though, such us my fix to Quark:
[PATCH] x86/platform/intel/quark: Explicitly include linux/io.h for virt_to_phys()
I've applied that one too and your updated patches, and it's now all pushed out into tip:WIP.core/headers.
Sorry, it wasn't clear to me whether or not to include that one. Next time I'll ask.
* Sean Christopherson sean.j.christopherson@intel.com wrote:
You didn't include every patch from v1 though, such us my fix to Quark:
[PATCH] x86/platform/intel/quark: Explicitly include linux/io.h for virt_to_phys()
I've applied that one too and your updated patches, and it's now all pushed out into tip:WIP.core/headers.
Sorry, it wasn't clear to me whether or not to include that one. Next time I'll ask.
No problem - in general it's best to include all, because in general it's much easier for maintainers to leave out something than to remember to add it back in. ;-)
Thanks,
Ingo
participants (3)
-
Ingo Molnar -
Pavel Machek -
Sean Christopherson