[PATCH 0/3] arch/x86: Remove unnecessary dependencies on bootparam.h

[PATCH 0/3] arch/x86: Remove unnecessary dependencies on bootparam.h

[Thomas Zimmermann]

Reduce built time in some cases by removing unnecessary include statements
for <asm/bootparam.h>. Reorganize some header files accordingly.

While working on the kernel's boot-up graphics, I noticed that touching
include/linux/screen_info.h triggers a complete rebuilt of the kernel
on x86. It turns out that the architecture's PCI and EFI headers include
<asm/bootparam.h>, which depends on <linux/screen_info.h>. But none of
the drivers have any business with boot parameters or the screen_info
state.

The patchset moves a few limes from pci.h and efi.h into separate header
files and then removes the obsolete include statements on x86. I did

  make allmodconfig
  make -j28
  touch include/linus/screen_info.h
  time -j28 make

to measure the time it takes to rebuild. Results without the patchset
are around 20 minutes.

  real    20m46,705s
  user    354m29,166s
  sys     28m27,359s

And with the patchset applied it goes down to about a minute.

  real    0m58,232s
  user    4m37,617s
  sys     0m34,993s

The test system was an Intel i5-13500.

Thomas Zimmermann (3):
  arch/x86: Move struct pci_setup_rom into pci_setup.h
  arch/x86: Add <asm/ima-efi.h> for arch_ima_efi_boot_mode
  arch/x86: Do not include <asm/bootparam.h> in several header files

 arch/x86/include/asm/efi.h              |  3 ---
 arch/x86/include/asm/ima-efi.h          | 12 ++++++++++++
 arch/x86/include/asm/kexec.h            |  1 -
 arch/x86/include/asm/mem_encrypt.h      |  2 +-
 arch/x86/include/asm/pci.h              | 13 -------------
 arch/x86/include/asm/pci_setup.h        | 19 +++++++++++++++++++
 arch/x86/include/asm/sev.h              |  3 ++-
 arch/x86/include/asm/x86_init.h         |  2 --
 arch/x86/pci/common.c                   |  1 +
 drivers/firmware/efi/libstub/x86-stub.c |  1 +
 include/asm-generic/Kbuild              |  1 +
 include/asm-generic/ima-efi.h           | 16 ++++++++++++++++
 security/integrity/ima/ima_efi.c        |  5 +----
 13 files changed, 54 insertions(+), 25 deletions(-)
 create mode 100644 arch/x86/include/asm/ima-efi.h
 create mode 100644 arch/x86/include/asm/pci_setup.h
 create mode 100644 include/asm-generic/ima-efi.h


base-commit: a9d99261a978835b02e248fe18af3026416af3e8
-- 
2.43.0

[PATCH 1/3] arch/x86: Move struct pci_setup_rom into pci_setup.h

[Thomas Zimmermann]

The type definition of struct pci_setup_rom in <asm/pci.h> requires
struct setup_data from <asm/bootparam.h>. Many drivers include
<linux/pci.h>, but do not use boot parameters. Changes to bootparam.h
or its included header files could easily trigger a large, unnecessary
rebuild of the kernel.

Moving struct pci_setup_rom into its own header file avoid including
<asm/bootparam.h> in <asm/pci.h>. Update the only two users of the
struct in the x86 PCI code and in the EFI code. Also remove the include
statement for x86_init.h, which is unnecessary but pulls in bootparams.h.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 arch/x86/include/asm/pci.h              | 13 -------------
 arch/x86/include/asm/pci_setup.h        | 19 +++++++++++++++++++
 arch/x86/pci/common.c                   |  1 +
 drivers/firmware/efi/libstub/x86-stub.c |  1 +
 4 files changed, 21 insertions(+), 13 deletions(-)
 create mode 100644 arch/x86/include/asm/pci_setup.h

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index b40c462b4af3..b3ab80a03365 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -10,7 +10,6 @@
 #include <linux/numa.h>
 #include <asm/io.h>
 #include <asm/memtype.h>
-#include <asm/x86_init.h>
 
 struct pci_sysdata {
 	int		domain;		/* PCI domain */
@@ -124,16 +123,4 @@ cpumask_of_pcibus(const struct pci_bus *bus)
 }
 #endif
 
-struct pci_setup_rom {
-	struct setup_data data;
-	uint16_t vendor;
-	uint16_t devid;
-	uint64_t pcilen;
-	unsigned long segment;
-	unsigned long bus;
-	unsigned long device;
-	unsigned long function;
-	uint8_t romdata[];
-};
-
 #endif /* _ASM_X86_PCI_H */
diff --git a/arch/x86/include/asm/pci_setup.h b/arch/x86/include/asm/pci_setup.h
new file mode 100644
index 000000000000..b4b246ef6f2b
--- /dev/null
+++ b/arch/x86/include/asm/pci_setup.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_PCI_SETUP_H
+#define _ASM_X86_PCI_SETUP_H
+
+#include <asm/bootparam.h>
+
+struct pci_setup_rom {
+	struct setup_data data;
+	uint16_t vendor;
+	uint16_t devid;
+	uint64_t pcilen;
+	unsigned long segment;
+	unsigned long bus;
+	unsigned long device;
+	unsigned long function;
+	uint8_t romdata[];
+};
+
+#endif /* _ASM_X86_PCI_SETUP_H */
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index ddb798603201..c6cbb9182160 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -17,6 +17,7 @@
 #include <asm/segment.h>
 #include <asm/io.h>
 #include <asm/smp.h>
+#include <asm/pci_setup.h>
 #include <asm/pci_x86.h>
 #include <asm/setup.h>
 #include <asm/irqdomain.h>
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 1bfdae34df39..0c878ebe5257 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -17,6 +17,7 @@
 #include <asm/boot.h>
 #include <asm/kaslr.h>
 #include <asm/sev.h>
+#include <asm/pci_setup.h>
 
 #include "efistub.h"
 #include "x86-stub.h"
-- 
2.43.0

[PATCH 2/3] arch/x86: Add <asm/ima-efi.h> for arch_ima_efi_boot_mode

[Thomas Zimmermann]

The header file <asm/efi.h> contains the macro arch_ima_efi_boot_mode,
which expands to use struct boot_params from <asm/bootparams.h>. Many
drivers include <linux/efi.h>, but do not use boot parameters. Changes
to bootparam.h or its included headers can easily trigger large,
unnessary rebuilds of the kernel.

Moving x86's arch_ima_efi_boot_mode to <asm/ima-efi.h> and including
<asm/bootparam.h> separates that dependency from the rest of the EFI
interfaces. The only user is in ima_efi.c. As the file already declares
a default value for arch_ima_efi_boot_mode, move this define into
asm-generic for all other architectures.

With arch_ima_efi_boot_mode removed from efi.h, <asm/bootparam.h> can
later be removed from further x86 header files.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 arch/x86/include/asm/efi.h       |  3 ---
 arch/x86/include/asm/ima-efi.h   | 12 ++++++++++++
 include/asm-generic/Kbuild       |  1 +
 include/asm-generic/ima-efi.h    | 16 ++++++++++++++++
 security/integrity/ima/ima_efi.c |  5 +----
 5 files changed, 30 insertions(+), 7 deletions(-)
 create mode 100644 arch/x86/include/asm/ima-efi.h
 create mode 100644 include/asm-generic/ima-efi.h

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index c4555b269a1b..99f31176c892 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -418,9 +418,6 @@ extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
 extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
 				     void *buf, struct efi_mem_range *mem);
 
-#define arch_ima_efi_boot_mode	\
-	({ extern struct boot_params boot_params; boot_params.secure_boot; })
-
 #ifdef CONFIG_EFI_RUNTIME_MAP
 int efi_get_runtime_map_size(void);
 int efi_get_runtime_map_desc_size(void);
diff --git a/arch/x86/include/asm/ima-efi.h b/arch/x86/include/asm/ima-efi.h
new file mode 100644
index 000000000000..3fe054937077
--- /dev/null
+++ b/arch/x86/include/asm/ima-efi.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_IMA_EFI_H
+#define _ASM_X86_IMA_EFI_H
+
+#include <asm/bootparam.h>
+
+#define arch_ima_efi_boot_mode	\
+	({ extern struct boot_params boot_params; boot_params.secure_boot; })
+
+#include <asm-generic/ima-efi.h>
+
+#endif /* _ASM_X86_IMA_EFI_H */
diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
index def242528b1d..4fd16e71e8cd 100644
--- a/include/asm-generic/Kbuild
+++ b/include/asm-generic/Kbuild
@@ -26,6 +26,7 @@ mandatory-y += ftrace.h
 mandatory-y += futex.h
 mandatory-y += hardirq.h
 mandatory-y += hw_irq.h
+mandatory-y += ima-efi.h
 mandatory-y += io.h
 mandatory-y += irq.h
 mandatory-y += irq_regs.h
diff --git a/include/asm-generic/ima-efi.h b/include/asm-generic/ima-efi.h
new file mode 100644
index 000000000000..f87f5edef440
--- /dev/null
+++ b/include/asm-generic/ima-efi.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef __ASM_GENERIC_IMA_EFI_H_
+#define __ASM_GENERIC_IMA_EFI_H_
+
+#include <linux/efi.h>
+
+/*
+ * Only include this header file from your architecture's <asm/ima-efi.h>.
+ */
+
+#ifndef arch_ima_efi_boot_mode
+#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
+#endif
+
+#endif /* __ASM_GENERIC_FB_H_ */
diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
index 138029bfcce1..56bbee271cec 100644
--- a/security/integrity/ima/ima_efi.c
+++ b/security/integrity/ima/ima_efi.c
@@ -6,10 +6,7 @@
 #include <linux/module.h>
 #include <linux/ima.h>
 #include <asm/efi.h>
-
-#ifndef arch_ima_efi_boot_mode
-#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
-#endif
+#include <asm/ima-efi.h>
 
 static enum efi_secureboot_mode get_sb_mode(void)
 {
-- 
2.43.0

[PATCH 3/3] arch/x86: Do not include <asm/bootparam.h> in several header files

[Thomas Zimmermann]

Remove the include statement for <asm/bootparam.h> from several header
files that don't require it. Limits the exposure of the boot parameters
within the Linux kernel code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 arch/x86/include/asm/kexec.h       | 1 -
 arch/x86/include/asm/mem_encrypt.h | 2 +-
 arch/x86/include/asm/sev.h         | 3 ++-
 arch/x86/include/asm/x86_init.h    | 2 --
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index c9f6a6c5de3c..91ca9a9ee3a2 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -25,7 +25,6 @@
 
 #include <asm/page.h>
 #include <asm/ptrace.h>
-#include <asm/bootparam.h>
 
 struct kimage;
 
diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
index 359ada486fa9..c1a8a3408c18 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -15,7 +15,7 @@
 #include <linux/init.h>
 #include <linux/cc_platform.h>
 
-#include <asm/bootparam.h>
+struct boot_params;
 
 #ifdef CONFIG_X86_MEM_ENCRYPT
 void __init mem_encrypt_init(void);
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 5b4a1ce3d368..8dad8b1613bf 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -13,7 +13,6 @@
 
 #include <asm/insn.h>
 #include <asm/sev-common.h>
-#include <asm/bootparam.h>
 #include <asm/coco.h>
 
 #define GHCB_PROTOCOL_MIN	1ULL
@@ -22,6 +21,8 @@
 
 #define	VMGEXIT()			{ asm volatile("rep; vmmcall\n\r"); }
 
+struct boot_params;
+
 enum es_result {
 	ES_OK,			/* All good */
 	ES_UNSUPPORTED,		/* Requested operation not supported */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index c878616a18b8..f062715578a0 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -2,8 +2,6 @@
 #ifndef _ASM_X86_PLATFORM_H
 #define _ASM_X86_PLATFORM_H
 
-#include <asm/bootparam.h>
-
 struct ghcb;
 struct mpc_bus;
 struct mpc_cpu;
-- 
2.43.0

Re: [PATCH 1/3] arch/x86: Move struct pci_setup_rom into pci_setup.h

[Ard Biesheuvel]

Hello Thomas,

On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> The type definition of struct pci_setup_rom in <asm/pci.h> requires
> struct setup_data from <asm/bootparam.h>. Many drivers include
> <linux/pci.h>, but do not use boot parameters. Changes to bootparam.h
> or its included header files could easily trigger a large, unnecessary
> rebuild of the kernel.
>
> Moving struct pci_setup_rom into its own header file avoid including
> <asm/bootparam.h> in <asm/pci.h>. Update the only two users of the
> struct in the x86 PCI code and in the EFI code. Also remove the include
> statement for x86_init.h, which is unnecessary but pulls in bootparams.h.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  arch/x86/include/asm/pci.h              | 13 -------------
>  arch/x86/include/asm/pci_setup.h        | 19 +++++++++++++++++++
>  arch/x86/pci/common.c                   |  1 +
>  drivers/firmware/efi/libstub/x86-stub.c |  1 +
>  4 files changed, 21 insertions(+), 13 deletions(-)
>  create mode 100644 arch/x86/include/asm/pci_setup.h
>

Thanks for cleaning this up.

Would it be more appropriate to move all setup_data related
definitions into a separate header entirely?

- the SETUP_ defines
- struct setup_data
- struct pci_setup_rom
- struct   jailhouse_setup_data
etc etc

struct setup_header has a setup_data field which is the root of the
setup_data linked list, but it is typed as __u64 so it doesn't
actually need to know the real type of the associated structs.

That way, you can avoid creating a special asm/pci_setup.h that only
covers this one particular definition.



> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index b40c462b4af3..b3ab80a03365 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -10,7 +10,6 @@
>  #include <linux/numa.h>
>  #include <asm/io.h>
>  #include <asm/memtype.h>
> -#include <asm/x86_init.h>
>
>  struct pci_sysdata {
>         int             domain;         /* PCI domain */
> @@ -124,16 +123,4 @@ cpumask_of_pcibus(const struct pci_bus *bus)
>  }
>  #endif
>
> -struct pci_setup_rom {
> -       struct setup_data data;
> -       uint16_t vendor;
> -       uint16_t devid;
> -       uint64_t pcilen;
> -       unsigned long segment;
> -       unsigned long bus;
> -       unsigned long device;
> -       unsigned long function;
> -       uint8_t romdata[];
> -};
> -
>  #endif /* _ASM_X86_PCI_H */
> diff --git a/arch/x86/include/asm/pci_setup.h b/arch/x86/include/asm/pci_setup.h
> new file mode 100644
> index 000000000000..b4b246ef6f2b
> --- /dev/null
> +++ b/arch/x86/include/asm/pci_setup.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_PCI_SETUP_H
> +#define _ASM_X86_PCI_SETUP_H
> +
> +#include <asm/bootparam.h>
> +
> +struct pci_setup_rom {
> +       struct setup_data data;
> +       uint16_t vendor;
> +       uint16_t devid;
> +       uint64_t pcilen;
> +       unsigned long segment;
> +       unsigned long bus;
> +       unsigned long device;
> +       unsigned long function;
> +       uint8_t romdata[];
> +};
> +
> +#endif /* _ASM_X86_PCI_SETUP_H */
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index ddb798603201..c6cbb9182160 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -17,6 +17,7 @@
>  #include <asm/segment.h>
>  #include <asm/io.h>
>  #include <asm/smp.h>
> +#include <asm/pci_setup.h>
>  #include <asm/pci_x86.h>
>  #include <asm/setup.h>
>  #include <asm/irqdomain.h>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 1bfdae34df39..0c878ebe5257 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -17,6 +17,7 @@
>  #include <asm/boot.h>
>  #include <asm/kaslr.h>
>  #include <asm/sev.h>
> +#include <asm/pci_setup.h>
>
>  #include "efistub.h"
>  #include "x86-stub.h"
> --
> 2.43.0
>

Re: [PATCH 2/3] arch/x86: Add <asm/ima-efi.h> for arch_ima_efi_boot_mode

[Ard Biesheuvel]

On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> The header file <asm/efi.h> contains the macro arch_ima_efi_boot_mode,
> which expands to use struct boot_params from <asm/bootparams.h>. Many
> drivers include <linux/efi.h>, but do not use boot parameters. Changes
> to bootparam.h or its included headers can easily trigger large,
> unnessary rebuilds of the kernel.
>
> Moving x86's arch_ima_efi_boot_mode to <asm/ima-efi.h> and including
> <asm/bootparam.h> separates that dependency from the rest of the EFI
> interfaces. The only user is in ima_efi.c. As the file already declares
> a default value for arch_ima_efi_boot_mode, move this define into
> asm-generic for all other architectures.
>
> With arch_ima_efi_boot_mode removed from efi.h, <asm/bootparam.h> can
> later be removed from further x86 header files.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  arch/x86/include/asm/efi.h       |  3 ---
>  arch/x86/include/asm/ima-efi.h   | 12 ++++++++++++
>  include/asm-generic/Kbuild       |  1 +
>  include/asm-generic/ima-efi.h    | 16 ++++++++++++++++
>  security/integrity/ima/ima_efi.c |  5 +----
>  5 files changed, 30 insertions(+), 7 deletions(-)
>  create mode 100644 arch/x86/include/asm/ima-efi.h
>  create mode 100644 include/asm-generic/ima-efi.h
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index c4555b269a1b..99f31176c892 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -418,9 +418,6 @@ extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
>  extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
>                                      void *buf, struct efi_mem_range *mem);
>
> -#define arch_ima_efi_boot_mode \
> -       ({ extern struct boot_params boot_params; boot_params.secure_boot; })
> -
>  #ifdef CONFIG_EFI_RUNTIME_MAP
>  int efi_get_runtime_map_size(void);
>  int efi_get_runtime_map_desc_size(void);
> diff --git a/arch/x86/include/asm/ima-efi.h b/arch/x86/include/asm/ima-efi.h
> new file mode 100644
> index 000000000000..3fe054937077
> --- /dev/null
> +++ b/arch/x86/include/asm/ima-efi.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_IMA_EFI_H
> +#define _ASM_X86_IMA_EFI_H
> +
> +#include <asm/bootparam.h>
> +
> +#define arch_ima_efi_boot_mode \
> +       ({ extern struct boot_params boot_params; boot_params.secure_boot; })
> +

Could you please check whether this kludge is still needed now that we
no longer have conflicting declarations of boot_params? (i.e., drop
the ({ }) and the extern declaration)



> +#include <asm-generic/ima-efi.h>
> +
> +#endif /* _ASM_X86_IMA_EFI_H */
> diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
> index def242528b1d..4fd16e71e8cd 100644
> --- a/include/asm-generic/Kbuild
> +++ b/include/asm-generic/Kbuild
> @@ -26,6 +26,7 @@ mandatory-y += ftrace.h
>  mandatory-y += futex.h
>  mandatory-y += hardirq.h
>  mandatory-y += hw_irq.h
> +mandatory-y += ima-efi.h
>  mandatory-y += io.h
>  mandatory-y += irq.h
>  mandatory-y += irq_regs.h
> diff --git a/include/asm-generic/ima-efi.h b/include/asm-generic/ima-efi.h
> new file mode 100644
> index 000000000000..f87f5edef440
> --- /dev/null
> +++ b/include/asm-generic/ima-efi.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef __ASM_GENERIC_IMA_EFI_H_
> +#define __ASM_GENERIC_IMA_EFI_H_
> +
> +#include <linux/efi.h>
> +
> +/*
> + * Only include this header file from your architecture's <asm/ima-efi.h>.
> + */
> +
> +#ifndef arch_ima_efi_boot_mode
> +#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
> +#endif
> +
> +#endif /* __ASM_GENERIC_FB_H_ */
> diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
> index 138029bfcce1..56bbee271cec 100644
> --- a/security/integrity/ima/ima_efi.c
> +++ b/security/integrity/ima/ima_efi.c
> @@ -6,10 +6,7 @@
>  #include <linux/module.h>
>  #include <linux/ima.h>
>  #include <asm/efi.h>
> -
> -#ifndef arch_ima_efi_boot_mode
> -#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
> -#endif
> +#include <asm/ima-efi.h>
>
>  static enum efi_secureboot_mode get_sb_mode(void)
>  {
> --
> 2.43.0
>

Re: [PATCH 3/3] arch/x86: Do not include <asm/bootparam.h> in several header files

[Ard Biesheuvel]

On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Remove the include statement for <asm/bootparam.h> from several header
> files that don't require it. Limits the exposure of the boot parameters
> within the Linux kernel code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  arch/x86/include/asm/kexec.h       | 1 -
>  arch/x86/include/asm/mem_encrypt.h | 2 +-
>  arch/x86/include/asm/sev.h         | 3 ++-
>  arch/x86/include/asm/x86_init.h    | 2 --
>  4 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index c9f6a6c5de3c..91ca9a9ee3a2 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -25,7 +25,6 @@
>
>  #include <asm/page.h>
>  #include <asm/ptrace.h>
> -#include <asm/bootparam.h>
>
>  struct kimage;
>
> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> index 359ada486fa9..c1a8a3408c18 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -15,7 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/cc_platform.h>
>
> -#include <asm/bootparam.h>
> +struct boot_params;
>
>  #ifdef CONFIG_X86_MEM_ENCRYPT
>  void __init mem_encrypt_init(void);
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 5b4a1ce3d368..8dad8b1613bf 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -13,7 +13,6 @@
>
>  #include <asm/insn.h>
>  #include <asm/sev-common.h>
> -#include <asm/bootparam.h>
>  #include <asm/coco.h>
>
>  #define GHCB_PROTOCOL_MIN      1ULL
> @@ -22,6 +21,8 @@
>
>  #define        VMGEXIT()                       { asm volatile("rep; vmmcall\n\r"); }
>
> +struct boot_params;
> +
>  enum es_result {
>         ES_OK,                  /* All good */
>         ES_UNSUPPORTED,         /* Requested operation not supported */
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index c878616a18b8..f062715578a0 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -2,8 +2,6 @@
>  #ifndef _ASM_X86_PLATFORM_H
>  #define _ASM_X86_PLATFORM_H
>
> -#include <asm/bootparam.h>
> -
>  struct ghcb;
>  struct mpc_bus;
>  struct mpc_cpu;
> --
> 2.43.0
>

Re: [PATCH 1/3] arch/x86: Move struct pci_setup_rom into pci_setup.h

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5002 bytes --]

Hi Ard

Am 07.12.23 um 16:35 schrieb Ard Biesheuvel:
> Hello Thomas,
> 
> On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> The type definition of struct pci_setup_rom in <asm/pci.h> requires
>> struct setup_data from <asm/bootparam.h>. Many drivers include
>> <linux/pci.h>, but do not use boot parameters. Changes to bootparam.h
>> or its included header files could easily trigger a large, unnecessary
>> rebuild of the kernel.
>>
>> Moving struct pci_setup_rom into its own header file avoid including
>> <asm/bootparam.h> in <asm/pci.h>. Update the only two users of the
>> struct in the x86 PCI code and in the EFI code. Also remove the include
>> statement for x86_init.h, which is unnecessary but pulls in bootparams.h.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   arch/x86/include/asm/pci.h              | 13 -------------
>>   arch/x86/include/asm/pci_setup.h        | 19 +++++++++++++++++++
>>   arch/x86/pci/common.c                   |  1 +
>>   drivers/firmware/efi/libstub/x86-stub.c |  1 +
>>   4 files changed, 21 insertions(+), 13 deletions(-)
>>   create mode 100644 arch/x86/include/asm/pci_setup.h
>>
> 
> Thanks for cleaning this up.
> 
> Would it be more appropriate to move all setup_data related
> definitions into a separate header entirely?
> 
> - the SETUP_ defines
> - struct setup_data
> - struct pci_setup_rom
> - struct   jailhouse_setup_data
> etc etc
> 
> struct setup_header has a setup_data field which is the root of the
> setup_data linked list, but it is typed as __u64 so it doesn't
> actually need to know the real type of the associated structs.
> 
> That way, you can avoid creating a special asm/pci_setup.h that only
> covers this one particular definition.

Thanks for reviewing.

I've now moved everything from <asm/bootparam.h> except struct 
bootparams into its own header file <asm/setup_data.h>. struct 
pci_setup_rom remains in pci.h. And most headers now include 
setup_data.h, while a few source files still require bootparam.h. I'll 
send this out in the next iteration.

Time for recompiling goes down from 58 sec to 56 sec. It's mostly bound 
by the linker now.

Best regards
Thomas

> 
> 
> 
>> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
>> index b40c462b4af3..b3ab80a03365 100644
>> --- a/arch/x86/include/asm/pci.h
>> +++ b/arch/x86/include/asm/pci.h
>> @@ -10,7 +10,6 @@
>>   #include <linux/numa.h>
>>   #include <asm/io.h>
>>   #include <asm/memtype.h>
>> -#include <asm/x86_init.h>
>>
>>   struct pci_sysdata {
>>          int             domain;         /* PCI domain */
>> @@ -124,16 +123,4 @@ cpumask_of_pcibus(const struct pci_bus *bus)
>>   }
>>   #endif
>>
>> -struct pci_setup_rom {
>> -       struct setup_data data;
>> -       uint16_t vendor;
>> -       uint16_t devid;
>> -       uint64_t pcilen;
>> -       unsigned long segment;
>> -       unsigned long bus;
>> -       unsigned long device;
>> -       unsigned long function;
>> -       uint8_t romdata[];
>> -};
>> -
>>   #endif /* _ASM_X86_PCI_H */
>> diff --git a/arch/x86/include/asm/pci_setup.h b/arch/x86/include/asm/pci_setup.h
>> new file mode 100644
>> index 000000000000..b4b246ef6f2b
>> --- /dev/null
>> +++ b/arch/x86/include/asm/pci_setup.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_PCI_SETUP_H
>> +#define _ASM_X86_PCI_SETUP_H
>> +
>> +#include <asm/bootparam.h>
>> +
>> +struct pci_setup_rom {
>> +       struct setup_data data;
>> +       uint16_t vendor;
>> +       uint16_t devid;
>> +       uint64_t pcilen;
>> +       unsigned long segment;
>> +       unsigned long bus;
>> +       unsigned long device;
>> +       unsigned long function;
>> +       uint8_t romdata[];
>> +};
>> +
>> +#endif /* _ASM_X86_PCI_SETUP_H */
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index ddb798603201..c6cbb9182160 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -17,6 +17,7 @@
>>   #include <asm/segment.h>
>>   #include <asm/io.h>
>>   #include <asm/smp.h>
>> +#include <asm/pci_setup.h>
>>   #include <asm/pci_x86.h>
>>   #include <asm/setup.h>
>>   #include <asm/irqdomain.h>
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>> index 1bfdae34df39..0c878ebe5257 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -17,6 +17,7 @@
>>   #include <asm/boot.h>
>>   #include <asm/kaslr.h>
>>   #include <asm/sev.h>
>> +#include <asm/pci_setup.h>
>>
>>   #include "efistub.h"
>>   #include "x86-stub.h"
>> --
>> 2.43.0
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/3] arch/x86: Add <asm/ima-efi.h> for arch_ima_efi_boot_mode

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4880 bytes --]

Hi

Am 07.12.23 um 16:37 schrieb Ard Biesheuvel:
> On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> The header file <asm/efi.h> contains the macro arch_ima_efi_boot_mode,
>> which expands to use struct boot_params from <asm/bootparams.h>. Many
>> drivers include <linux/efi.h>, but do not use boot parameters. Changes
>> to bootparam.h or its included headers can easily trigger large,
>> unnessary rebuilds of the kernel.
>>
>> Moving x86's arch_ima_efi_boot_mode to <asm/ima-efi.h> and including
>> <asm/bootparam.h> separates that dependency from the rest of the EFI
>> interfaces. The only user is in ima_efi.c. As the file already declares
>> a default value for arch_ima_efi_boot_mode, move this define into
>> asm-generic for all other architectures.
>>
>> With arch_ima_efi_boot_mode removed from efi.h, <asm/bootparam.h> can
>> later be removed from further x86 header files.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   arch/x86/include/asm/efi.h       |  3 ---
>>   arch/x86/include/asm/ima-efi.h   | 12 ++++++++++++
>>   include/asm-generic/Kbuild       |  1 +
>>   include/asm-generic/ima-efi.h    | 16 ++++++++++++++++
>>   security/integrity/ima/ima_efi.c |  5 +----
>>   5 files changed, 30 insertions(+), 7 deletions(-)
>>   create mode 100644 arch/x86/include/asm/ima-efi.h
>>   create mode 100644 include/asm-generic/ima-efi.h
>>
>> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
>> index c4555b269a1b..99f31176c892 100644
>> --- a/arch/x86/include/asm/efi.h
>> +++ b/arch/x86/include/asm/efi.h
>> @@ -418,9 +418,6 @@ extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
>>   extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
>>                                       void *buf, struct efi_mem_range *mem);
>>
>> -#define arch_ima_efi_boot_mode \
>> -       ({ extern struct boot_params boot_params; boot_params.secure_boot; })
>> -
>>   #ifdef CONFIG_EFI_RUNTIME_MAP
>>   int efi_get_runtime_map_size(void);
>>   int efi_get_runtime_map_desc_size(void);
>> diff --git a/arch/x86/include/asm/ima-efi.h b/arch/x86/include/asm/ima-efi.h
>> new file mode 100644
>> index 000000000000..3fe054937077
>> --- /dev/null
>> +++ b/arch/x86/include/asm/ima-efi.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_IMA_EFI_H
>> +#define _ASM_X86_IMA_EFI_H
>> +
>> +#include <asm/bootparam.h>
>> +
>> +#define arch_ima_efi_boot_mode \
>> +       ({ extern struct boot_params boot_params; boot_params.secure_boot; })
>> +
> 
> Could you please check whether this kludge is still needed now that we
> no longer have conflicting declarations of boot_params? (i.e., drop
> the ({ }) and the extern declaration)

I dropped this and include setup.h instead. Appears to work.

Best regards
Thomas

> 
> 
> 
>> +#include <asm-generic/ima-efi.h>
>> +
>> +#endif /* _ASM_X86_IMA_EFI_H */
>> diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
>> index def242528b1d..4fd16e71e8cd 100644
>> --- a/include/asm-generic/Kbuild
>> +++ b/include/asm-generic/Kbuild
>> @@ -26,6 +26,7 @@ mandatory-y += ftrace.h
>>   mandatory-y += futex.h
>>   mandatory-y += hardirq.h
>>   mandatory-y += hw_irq.h
>> +mandatory-y += ima-efi.h
>>   mandatory-y += io.h
>>   mandatory-y += irq.h
>>   mandatory-y += irq_regs.h
>> diff --git a/include/asm-generic/ima-efi.h b/include/asm-generic/ima-efi.h
>> new file mode 100644
>> index 000000000000..f87f5edef440
>> --- /dev/null
>> +++ b/include/asm-generic/ima-efi.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +
>> +#ifndef __ASM_GENERIC_IMA_EFI_H_
>> +#define __ASM_GENERIC_IMA_EFI_H_
>> +
>> +#include <linux/efi.h>
>> +
>> +/*
>> + * Only include this header file from your architecture's <asm/ima-efi.h>.
>> + */
>> +
>> +#ifndef arch_ima_efi_boot_mode
>> +#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
>> +#endif
>> +
>> +#endif /* __ASM_GENERIC_FB_H_ */
>> diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
>> index 138029bfcce1..56bbee271cec 100644
>> --- a/security/integrity/ima/ima_efi.c
>> +++ b/security/integrity/ima/ima_efi.c
>> @@ -6,10 +6,7 @@
>>   #include <linux/module.h>
>>   #include <linux/ima.h>
>>   #include <asm/efi.h>
>> -
>> -#ifndef arch_ima_efi_boot_mode
>> -#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
>> -#endif
>> +#include <asm/ima-efi.h>
>>
>>   static enum efi_secureboot_mode get_sb_mode(void)
>>   {
>> --
>> 2.43.0
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]