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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- 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 --]