Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume

From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Myron Stowe" <myron.stowe@redhat.com>,
	"Juha-Pekka Heikkila" <juhapekka.heikkila@gmail.com>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Borislav Petkov" <bp@alien8.de>,
	linux-acpi <linux-acpi@vger.kernel.org>,
	"Linux PCI" <linux-pci@vger.kernel.org>,
	x86@kernel.org,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Benoit Grégoire" <benoitg@coeus.ca>,
	"Hui Wang" <hui.wang@canonical.com>
Subject: Re: [5.17 regression] "x86/PCI: Ignore E820 reservations for bridge windows on newer systems" breaks suspend/resume
Date: Wed, 9 Feb 2022 18:01:07 +0200	[thread overview]
Message-ID: <YgPlQ6UK3+4/yzLk@lahna> (raw)
In-Reply-To: <02994528-aaad-5259-1774-19aeacdd18fc@redhat.com>

Hi Hans,

Thanks for looking into this!

On Wed, Feb 09, 2022 at 04:12:32PM +0100, Hans de Goede wrote:
> An alternative, much more elaborate fix would be to:
> 
> 1. Add E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW types to
>    enum e820_type and modify the 2 places which check for type == reserved
>    to treat these both as reserved too, so as to not have any functional
>    changes there
> 
> 2. Modify the code building e820 tables from the EFI memmap to use
>    E820_TYPE_MMIO for MMIO EFI memmap entries and make e820_nomerge()
>    treat E820_TYPE_MMIO as non mergable to retain these as is
> 
> 3. Modify pci_acpi_root_prepare_resources() to compare ACPI provided
>    bridge mmio windows against E820_TYPE_MMIO e820_Table entries and
>    if there is an exact match (as is the case on the Yoga C940)
>    then change the e820 type to E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
> 
>    This means that we are now very narrowly treating EFI MMIO marked
>    regions special, in the special case where they are a 1:1 map
>    to an ACPI PCI bridge window resource.
> 
>    Note since we treat both E820_TYPE_MMIO and E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
>    identical to the old RESERVED everywhere there is no functional change
>    here.
> 
> 4. Modify arch/x86/kernel/resource.c: remove_e820_regions() to skip
>    e820 table entries with a type of E820_TYPE_MMIO_PCI_BRIDGE_WINDOW,
>    this would actually be a functional change and should fix the
>    issues we are trying to fix.
> 
> Note an alternative would be to skip having the extra E820_TYPE_MMIO_PCI_BRIDGE_WINDOW
> type and to skip step 3. above. That would boil down to doing the same
> as your original patch in a somewhat cleaner manner.

I agree and my vote goes for this last alternative (e.g skipping step 3).
That would also make it slightly easier to follow the logic if someone
in the future needs to investigate possible issues around this, I think.