linux-kernel - Re: [PATCH 1/3] arch/x86: Move struct pci_setup_rom into pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXEOXh10v7dz-Y4hVM0y1VxR3YFxSxuE9a3wE0LbMsy2UA@mail.gmail.com>
Date:   Thu, 7 Dec 2023 16:35:03 +0100
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Thomas Zimmermann <tzimmermann@...e.de>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        bhelgaas@...gle.com, arnd@...db.de, zohar@...ux.ibm.com,
        dmitry.kasatkin@...il.com, paul@...l-moore.com, jmorris@...ei.org,
        serge@...lyn.com, javierm@...hat.com, linux-arch@...r.kernel.org,
        linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH 1/3] arch/x86: Move struct pci_setup_rom into pci_setup.h

Hello Thomas,

On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@...e.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@...e.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
>