mbox series

[v2,00/12] treewide: break dependencies on x86's RM header

Message ID 20191126165417.22423-1-sean.j.christopherson@intel.com (mailing list archive)
Headers show
Series treewide: break dependencies on x86's RM header | expand

Message

Sean Christopherson Nov. 26, 2019, 4:54 p.m. UTC
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(-)

Comments

Ingo Molnar Nov. 27, 2019, 7:20 a.m. UTC | #1
* 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
Sean Christopherson Nov. 27, 2019, 2:47 p.m. UTC | #2
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.
Ingo Molnar Nov. 27, 2019, 5:25 p.m. UTC | #3
* 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