| Message ID | 20231206125433.18420-2-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | arch/x86: Remove unnecessary dependencies on bootparam.h | expand |
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 >
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 --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"
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