Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20231206125433.18420-1-tzimmermann@suse.de> <20231206125433.18420-2-tzimmermann@suse.de>
In-Reply-To: <20231206125433.18420-2-tzimmermann@suse.de>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Thu, 7 Dec 2023 16:35:03 +0100
Message-ID: <CAMj1kXEOXh10v7dz-Y4hVM0y1VxR3YFxSxuE9a3wE0LbMsy2UA@mail.gmail.com>
Subject: Re: [PATCH 1/3] arch/x86: Move struct pci_setup_rom into pci_setup.h
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
        bhelgaas@google.com, arnd@arndb.de, zohar@linux.ibm.com,
        dmitry.kasatkin@gmail.com, paul@paul-moore.com, jmorris@namei.org,
        serge@hallyn.com, javierm@redhat.com, linux-arch@vger.kernel.org,
        linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>