From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 491FDC43334 for ; Tue, 14 Jun 2022 08:15:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233203AbiFNIPn (ORCPT ); Tue, 14 Jun 2022 04:15:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35776 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232688AbiFNIPk (ORCPT ); Tue, 14 Jun 2022 04:15:40 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id CC5313FD84 for ; Tue, 14 Jun 2022 01:15:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1655194537; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=l9Zc9wCIpLbAd4jBfyMUl844wfhz2xkE3MXngp1dQqs=; b=TqzuosyWB/I4KbkVXt15IF71qTpV7skWb0kONcLyhK05DfBG1wDJCpnv/8JwhSlbXlawrF OAmi9HSAEN7J+Eod4C1XJsgdlqI7hgliIIGxjFEKEIyOTmb5YOgA1Bd6qmsBxP0kjHNfiE pkMI8HgkHApfa35USe0P2Ow1ASZXLpU= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-523-KQYCR_qSOOKQSJOGfB-opg-1; Tue, 14 Jun 2022 04:15:32 -0400 X-MC-Unique: KQYCR_qSOOKQSJOGfB-opg-1 Received: by mail-ed1-f71.google.com with SMTP id eh10-20020a0564020f8a00b0042dd9bf7c57so5667741edb.17 for ; Tue, 14 Jun 2022 01:15:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=l9Zc9wCIpLbAd4jBfyMUl844wfhz2xkE3MXngp1dQqs=; b=Jrw8ZiVxEW7kVXAPnfsvmHTJh0PE7RwdN2NFxvYNWnz2RJoy5Nql1894FX73pB/4IH 9wow7dBGx975XxChM93JkcO8btpQhVSZ1oZlWoqIXar6TOcPdyJ3QVFZY2JgaEco8gbW +AyGEmd4q6m/SV2mPQJEscuKJ+1hpZhRCI8yqbPR+jMUn6oYIOvMMXSrWXUHAk5PdC2I v2IOMRfGmyQjNmd3dL+/BBvAafZYbtw3OnZfnUdMZsCY+Eb+0aXXByCH4iRuE8/0oLDu +v/Nnb8SQeZRkot8s6fJShIkumtT7fNidPwMsrac1ZRkpIj747QZiHR+oeB5jakg/ORk 59vw== X-Gm-Message-State: AJIora/JBT5LWicU1WA9VAbI2pIyl8DZqBRiJzI+4MlHx+TiR9Hk6mVi GYc0xwpx9xHWPXnShGukeAjqIy0gii4au+LyTRHio3elS8esg0yKVa7QkKf8pyiKGEFq1UeHwk+ 8SJfampJHIaNhkuTJc3xB8Q== X-Received: by 2002:a17:906:7295:b0:712:2241:2c3f with SMTP id b21-20020a170906729500b0071222412c3fmr3225327ejl.523.1655194530767; Tue, 14 Jun 2022 01:15:30 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwfw5fLxxDEGhZzCdJd093Ss0BooyMXikguimh5E7whRCP9wkHVKSAv5g9muKWd8yhROhhnoA== X-Received: by 2002:a17:906:7295:b0:712:2241:2c3f with SMTP id b21-20020a170906729500b0071222412c3fmr3225303ejl.523.1655194530530; Tue, 14 Jun 2022 01:15:30 -0700 (PDT) Received: from ?IPV6:2001:1c00:c1e:bf00:d69d:5353:dba5:ee81? (2001-1c00-0c1e-bf00-d69d-5353-dba5-ee81.cable.dynamic.v6.ziggo.nl. [2001:1c00:c1e:bf00:d69d:5353:dba5:ee81]) by smtp.gmail.com with ESMTPSA id qh21-20020a170906ecb500b006feaa22e367sm4707525ejb.165.2022.06.14.01.15.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Jun 2022 01:15:29 -0700 (PDT) Message-ID: <5b1753ef-0bfb-f937-cab1-ad960bdf6772@redhat.com> Date: Tue, 14 Jun 2022 10:15:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH] x86/PCI: Revert: "Clip only host bridge windows for E820 regions" Content-Language: en-US To: Bjorn Helgaas Cc: "Rafael J . Wysocki" , Mika Westerberg , =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= , Bjorn Helgaas , Myron Stowe , Juha-Pekka Heikkila , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H . Peter Anvin" , =?UTF-8?Q?Benoit_Gr=c3=a9goire?= , Hui Wang , linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, "Guilherme G . Piccoli" References: <20220613231539.GA722481@bhelgaas> From: Hans de Goede In-Reply-To: <20220613231539.GA722481@bhelgaas> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org Hi, On 6/14/22 01:15, Bjorn Helgaas wrote: > On Sun, Jun 12, 2022 at 04:43:25PM +0200, Hans de Goede wrote: >> Clipping the bridge windows directly from pci_acpi_root_prepare_resources() >> instead of clipping from arch_remove_reservations(), has a number of >> unforseen consequences. >> >> If there is an e820 reservation in the middle of a bridge window, then >> the smallest of the 2 remaining parts of the window will be also clipped >> off. Where as the previous code would clip regions requested by devices, >> rather then the entire window, leaving regions which were either entirely >> above or below a reservation in the middle of the window alone. >> >> E.g. on the Steam Deck this leads to this log message: >> >> acpi PNP0A08:00: clipped [mem 0x80000000-0xf7ffffff window] to [mem 0xa0100000-0xf7ffffff window] >> >> which then gets followed by these log messages: >> >> pci 0000:00:01.2: can't claim BAR 14 [mem 0x80600000-0x806fffff]: no compatible bridge window >> pci 0000:00:01.3: can't claim BAR 14 [mem 0x80500000-0x805fffff]: no compatible bridge window >> >> and many more of these. Ultimately this leads to the Steam Deck >> no longer booting properly, so revert the change. >> >> Note this is not a clean revert, this revert keeps the later change >> to make the clipping dependent on a new pci_use_e820 bool, moving >> the checking of this bool to arch_remove_reservations(). > > 4c5e242d3e93 was definitely a mistake (my fault). My intent was to > mainly to improve logging of the clipping, but I didn't implement it > well. > > That said, I'd like to understand the connection between the messages > you mention and the failure. There are four bridges whose MMIO > windows were in the [mem 0x80000000-0x9fffffff] area that we clipped > out. The log shows that we moved all those windows and the devices in > them to the [mem 0xa0100000-0xf7ffffff] area that remained after > clipping. > > So I think this *should* have worked even though we moved things > around unnecessarily. What am I missing? I don't know? My guess is that maybe the ACPI table do MMIO accesses somewhere to hardcoded addresses and moving things breaks the ACPI tables. > > The E820 map reports [mem 0xa0000000-0xa00fffff] in the middle of the > _CRS, and we currently trim that out. We think this is a firmware > defect, so it's likely to break in 2023 if we stop clipping by > default. I'm concerned that there may be other things in _CRS that we > need to avoid, but firmware isn't telling us about them. > > Or there's some dependency in the devices that we moved on their > original addresses, e.g., firmware on the device latched the address > and didn't notice the reassignment. Right this is the most likely cause I believe. Regards, Hans > > [1] https://bugzilla.kernel.org/attachment.cgi?id=301154 > >> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216109 >> Fixes: 4c5e242d3e93 ("x86/PCI: Clip only host bridge windows for E820 regions") >> Reported-and-tested-by: Guilherme G. Piccoli >> Signed-off-by: Hans de Goede >> --- >> arch/x86/include/asm/e820/api.h | 5 ----- >> arch/x86/include/asm/pci_x86.h | 8 ++++++++ >> arch/x86/kernel/resource.c | 14 +++++++++----- >> arch/x86/pci/acpi.c | 8 +------- >> 4 files changed, 18 insertions(+), 17 deletions(-) >> >> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h >> index 5a39ed59b6db..e8f58ddd06d9 100644 >> --- a/arch/x86/include/asm/e820/api.h >> +++ b/arch/x86/include/asm/e820/api.h >> @@ -4,9 +4,6 @@ >> >> #include >> >> -struct device; >> -struct resource; >> - >> extern struct e820_table *e820_table; >> extern struct e820_table *e820_table_kexec; >> extern struct e820_table *e820_table_firmware; >> @@ -46,8 +43,6 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn); >> >> extern int e820__get_entry_type(u64 start, u64 end); >> >> -extern void remove_e820_regions(struct device *dev, struct resource *avail); >> - >> /* >> * Returns true iff the specified range [start,end) is completely contained inside >> * the ISA region. >> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h >> index f52a886d35cf..70533fdcbf02 100644 >> --- a/arch/x86/include/asm/pci_x86.h >> +++ b/arch/x86/include/asm/pci_x86.h >> @@ -69,6 +69,8 @@ void pcibios_scan_specific_bus(int busn); >> >> /* pci-irq.c */ >> >> +struct pci_dev; >> + >> struct irq_info { >> u8 bus, devfn; /* Bus, device and function */ >> struct { >> @@ -246,3 +248,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) >> +extern bool pci_use_e820; >> +#else >> +#define pci_use_e820 false >> +#endif >> diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c >> index db2b350a37b7..bba1abd05bfe 100644 >> --- a/arch/x86/kernel/resource.c >> +++ b/arch/x86/kernel/resource.c >> @@ -1,7 +1,8 @@ >> // SPDX-License-Identifier: GPL-2.0 >> -#include >> #include >> +#include >> #include >> +#include >> >> static void resource_clip(struct resource *res, resource_size_t start, >> resource_size_t end) >> @@ -24,14 +25,14 @@ static void resource_clip(struct resource *res, resource_size_t start, >> res->start = end + 1; >> } >> >> -void remove_e820_regions(struct device *dev, struct resource *avail) >> +static void remove_e820_regions(struct resource *avail) >> { >> int i; >> struct e820_entry *entry; >> u64 e820_start, e820_end; >> struct resource orig = *avail; >> >> - if (!(avail->flags & IORESOURCE_MEM)) >> + if (!pci_use_e820) >> return; >> >> for (i = 0; i < e820_table->nr_entries; i++) { >> @@ -41,7 +42,7 @@ void remove_e820_regions(struct device *dev, struct resource *avail) >> >> resource_clip(avail, e820_start, e820_end); >> if (orig.start != avail->start || orig.end != avail->end) { >> - dev_info(dev, "clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n", >> + pr_info("clipped %pR to %pR for e820 entry [mem %#010Lx-%#010Lx]\n", >> &orig, avail, e820_start, e820_end); >> orig = *avail; >> } >> @@ -55,6 +56,9 @@ void arch_remove_reservations(struct resource *avail) >> * the low 1MB unconditionally, as this area is needed for some ISA >> * cards requiring a memory range, e.g. the i82365 PCMCIA controller. >> */ >> - if (avail->flags & IORESOURCE_MEM) >> + if (avail->flags & IORESOURCE_MEM) { >> resource_clip(avail, BIOS_ROM_BASE, BIOS_ROM_END); >> + >> + remove_e820_regions(avail); >> + } >> } >> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c >> index a4f43054bc79..2f82480fd430 100644 >> --- a/arch/x86/pci/acpi.c >> +++ b/arch/x86/pci/acpi.c >> @@ -8,7 +8,6 @@ >> #include >> #include >> #include >> -#include >> >> struct pci_root_info { >> struct acpi_pci_root_info common; >> @@ -20,7 +19,7 @@ struct pci_root_info { >> #endif >> }; >> >> -static bool pci_use_e820 = true; >> +bool pci_use_e820 = true; >> static bool pci_use_crs = true; >> static bool pci_ignore_seg; >> >> @@ -387,11 +386,6 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci) >> >> status = acpi_pci_probe_root_resources(ci); >> >> - if (pci_use_e820) { >> - resource_list_for_each_entry(entry, &ci->resources) >> - remove_e820_regions(&device->dev, entry->res); >> - } >> - >> if (pci_use_crs) { >> resource_list_for_each_entry_safe(entry, tmp, &ci->resources) >> if (resource_is_pcicfg_ioport(entry->res)) >> -- >> 2.36.0 >> >