| Message ID | 20191119002121.4107-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | treewide: break dependencies on x86's RM header | expand |
* 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. > > Build tested on x86 with allyesconfig and allmodconfig, so hopefully there > aren't more build issues lurking, but at this point it wouldn't surprise > me in the least if this somehow manages to break the build. > > Based on tip/master, commit ceceaf1f12ba ("Merge branch 'WIP.x86/cleanups'"). > > 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. Yeah, these kind of parasitic header dependencies are the main driving force behind kernel header spaghetti hell: it's super easy to add a new header, but very hard to remove them... Hence they practically only accumulate. As a result header removal patches get priority, from me at least. :-) > - 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. So if the ACPI maintainers are fine with -tip carrying patches #11 and #12 then I'd be glad to route these patches upstream. I've applied them to tip:WIP.core/headers as a work-in-progress tree, and I'm testing them on randconfigs to make sure there's no broken dependencies. I'll wait for the ACPI acks. I edited the title of patch 12 slightly, to: c8bceb321209: x86/ACPI/sleep: Move acpi_wakeup_address() definition into sleep.c, remove <asm/realmode.h> from <asm/acpi.h> to make sure the big header dependency change is obvious at first sight. Thanks, Ingo
On Tue, 19 Nov 2019 at 12:10, Ingo Molnar <mingo@kernel.org> 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. > > > > Build tested on x86 with allyesconfig and allmodconfig, so hopefully there > > aren't more build issues lurking, but at this point it wouldn't surprise > > me in the least if this somehow manages to break the build. > > > > Based on tip/master, commit ceceaf1f12ba ("Merge branch 'WIP.x86/cleanups'"). > > > > 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. > > Yeah, these kind of parasitic header dependencies are the main driving > force behind kernel header spaghetti hell: it's super easy to add a new > header, but very hard to remove them... > > Hence they practically only accumulate. > > As a result header removal patches get priority, from me at least. :-) > > > - 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. > > So if the ACPI maintainers are fine with -tip carrying patches #11 and #12 > then I'd be glad to route these patches upstream. > > I've applied them to tip:WIP.core/headers as a work-in-progress tree, and > I'm testing them on randconfigs to make sure there's no broken > dependencies. I'll wait for the ACPI acks. > > I edited the title of patch 12 slightly, to: > > c8bceb321209: x86/ACPI/sleep: Move acpi_wakeup_address() definition into sleep.c, remove <asm/realmode.h> from <asm/acpi.h> > > to make sure the big header dependency change is obvious at first sight. > I'm fine with the patches but can we drop the fixes headers please? This doesn't actually fix anything, and touching early boot stuff for no good reason should be avoided imo.
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On Tue, 19 Nov 2019 at 12:10, Ingo Molnar <mingo@kernel.org> 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. > > > > > > Build tested on x86 with allyesconfig and allmodconfig, so hopefully there > > > aren't more build issues lurking, but at this point it wouldn't surprise > > > me in the least if this somehow manages to break the build. > > > > > > Based on tip/master, commit ceceaf1f12ba ("Merge branch 'WIP.x86/cleanups'"). > > > > > > 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. > > > > Yeah, these kind of parasitic header dependencies are the main driving > > force behind kernel header spaghetti hell: it's super easy to add a new > > header, but very hard to remove them... > > > > Hence they practically only accumulate. > > > > As a result header removal patches get priority, from me at least. :-) > > > > > - 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. > > > > So if the ACPI maintainers are fine with -tip carrying patches #11 and #12 > > then I'd be glad to route these patches upstream. > > > > I've applied them to tip:WIP.core/headers as a work-in-progress tree, and > > I'm testing them on randconfigs to make sure there's no broken > > dependencies. I'll wait for the ACPI acks. > > > > I edited the title of patch 12 slightly, to: > > > > c8bceb321209: x86/ACPI/sleep: Move acpi_wakeup_address() definition into sleep.c, remove <asm/realmode.h> from <asm/acpi.h> > > > > to make sure the big header dependency change is obvious at first sight. > > > > I'm fine with the patches but can we drop the fixes headers please? > This doesn't actually fix anything, and touching early boot stuff for > no good reason should be avoided imo. Agreed and done. Thanks, Ingo
On Tue, 19 Nov 2019 at 13:22, Ingo Molnar <mingo@kernel.org> wrote: > > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > On Tue, 19 Nov 2019 at 12:10, Ingo Molnar <mingo@kernel.org> 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. > > > > > > > > Build tested on x86 with allyesconfig and allmodconfig, so hopefully there > > > > aren't more build issues lurking, but at this point it wouldn't surprise > > > > me in the least if this somehow manages to break the build. > > > > > > > > Based on tip/master, commit ceceaf1f12ba ("Merge branch 'WIP.x86/cleanups'"). > > > > > > > > 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. > > > > > > Yeah, these kind of parasitic header dependencies are the main driving > > > force behind kernel header spaghetti hell: it's super easy to add a new > > > header, but very hard to remove them... > > > > > > Hence they practically only accumulate. > > > > > > As a result header removal patches get priority, from me at least. :-) > > > > > > > - 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. > > > > > > So if the ACPI maintainers are fine with -tip carrying patches #11 and #12 > > > then I'd be glad to route these patches upstream. > > > > > > I've applied them to tip:WIP.core/headers as a work-in-progress tree, and > > > I'm testing them on randconfigs to make sure there's no broken > > > dependencies. I'll wait for the ACPI acks. > > > > > > I edited the title of patch 12 slightly, to: > > > > > > c8bceb321209: x86/ACPI/sleep: Move acpi_wakeup_address() definition into sleep.c, remove <asm/realmode.h> from <asm/acpi.h> > > > > > > to make sure the big header dependency change is obvious at first sight. > > > > > > > I'm fine with the patches but can we drop the fixes headers please? > > This doesn't actually fix anything, and touching early boot stuff for > > no good reason should be avoided imo. > > Agreed and done. > Thanks Ingo
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. Build tested on x86 with allyesconfig and allmodconfig, so hopefully there aren't more build issues lurking, but at this point it wouldn't surprise me in the least if this somehow manages to break the build. Based on tip/master, commit ceceaf1f12ba ("Merge branch 'WIP.x86/cleanups'"). 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_wakeup_address() definition into sleep.c 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 | 4 ++-- 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, 29 insertions(+), 8 deletions(-)