diff mbox series

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

Message ID 20231206125433.18420-2-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series arch/x86: Remove unnecessary dependencies on bootparam.h | expand

Commit Message

Thomas Zimmermann Dec. 6, 2023, 12:38 p.m. UTC 
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

Comments

Ard Biesheuvel Dec. 7, 2023, 3:35 p.m. UTC | #1 
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
>
Thomas Zimmermann Dec. 15, 2023, 12:15 p.m. UTC | #2 
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
>>
diff mbox series

Patch

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"