linux-kernel - Re: [PATCH v2] x86/PCI: Ignore E820 reservations for bridge windows on newer systems

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: <YWdaJVhoPDrg3Tsd@rocinante>
Date:   Thu, 14 Oct 2021 00:13:57 +0200
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Myron Stowe <myron.stowe@...hat.com>,
        Juha-Pekka Heikkila <juhapekka.heikkila@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>, linux-pci@...r.kernel.org,
        x86@...nel.org, linux-kernel@...r.kernel.org,
        Benoit Grégoire <benoitg@...us.ca>,
        Hui Wang <hui.wang@...onical.com>
Subject: Re: [PATCH v2] x86/PCI: Ignore E820 reservations for bridge windows
 on newer systems

Hi Hans,

Thank you for sending the patch over!

[...]
> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> [    0.557473] pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]

A very small nitpick: we usually remove time/date stamps from kernel ring
buffer outputs keeping only the relevant message parts left.

[...]
> Old systems are defined here as BIOS year < 2018, this was chosen to
> make sure that pci_use_e820 will not be set on the currently affected
> systems, while at the same time also taking into account that the
> systems for which the E820 checking was orignally added may have

A tiny typo of "originally" in the sentence above.

[...]
> @@ -232,3 +236,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
>  # define x86_default_pci_init_irq	NULL
>  # define x86_default_pci_fixup_irqs	NULL
>  #endif
> +
> +#if defined CONFIG_PCI && defined CONFIG_ACPI

I know that Mika already asked about this, and you responded, so I can only
added: brackets, let's add brackets, most definitely. :)

[...]
> +/* Consumed in arch/x86/kernel/resource.c */
> +bool pci_use_e820 = false;

A small nitpick: not sure if this comment is needed as probably most people
working with this code would use "git grep" and likes to list occurrences
where the variables is used.  But, this is highly subjective, thus there is
probably nothing to change here.

> +	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
> +	       pci_use_e820 ? "Honoring" : "Ignoring");

I know you followed the existing style, which is very much appreciated, but
if and where possible, we should move to newer API/style and replace the
printk() above with pr_info().  New code should not be adding old style if
it can be helped (checkpatch.pl would warn about this too).  What do yo you
think?

	Krzysztof