From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f45.google.com ([209.85.220.45]:36467 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751467AbcBNIeD (ORCPT ); Sun, 14 Feb 2016 03:34:03 -0500 Received: by mail-pa0-f45.google.com with SMTP id yy13so69294996pab.3 for ; Sun, 14 Feb 2016 00:34:02 -0800 (PST) Subject: Re: [PATCH v2] PCI: ACPI: IA64: fix IO port generic range check To: Lorenzo Pieralisi , linux-pci@vger.kernel.org References: <1455212827-9382-1-git-send-email-lorenzo.pieralisi@arm.com> Cc: linux-acpi@vger.kernel.org, linux-ia64@vger.kernel.org, Bjorn Helgaas , Jiang Liu , Tony Luck , Tomasz Nowicki , Mark Salter , "Rafael J. Wysocki" From: Hanjun Guo Message-ID: <56C03BF0.6020505@linaro.org> Date: Sun, 14 Feb 2016 16:33:52 +0800 MIME-Version: 1.0 In-Reply-To: <1455212827-9382-1-git-send-email-lorenzo.pieralisi@arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Sender: linux-pci-owner@vger.kernel.org List-ID: On 2016/2/12 1:47, Lorenzo Pieralisi wrote: > The [0 - 64k] ACPI PCI IO port resource boundary check in: > > acpi_dev_ioresource_flags() > > is currently applied blindly in the ACPI resource parsing to all > architectures, but only x86 suffers from that IO space limitation. > > On arches (ie IA64 and ARM64) where IO space is memory mapped, > the PCI root bridges IO resource windows are firstly initialized from > the _CRS (in acpi_decode_space()) and contain the CPU physical address > at which a root bridge decodes IO space in the CPU physical address > space with the offset value representing the offset required to translate > the PCI bus address into the CPU physical address. > > The IO resource windows are then parsed and updated in arch code > before creating and enumerating PCI buses (eg IA64 add_io_space()) > to map in an arch specific way the obtained CPU physical address range > to a slice of virtual address space reserved to map PCI IO space, > ending up with PCI bridges resource windows containing IO > resources like the following on a working IA64 configuration: > > PCI host bridge to bus 0000:00 > pci_bus 0000:00: root bus resource [io 0x1000000-0x100ffff window] (bus > address [0x0000-0xffff]) > pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000fffff window] > pci_bus 0000:00: root bus resource [mem 0x80000000-0x8fffffff window] > pci_bus 0000:00: root bus resource [mem 0x80004000000-0x800ffffffff window] > pci_bus 0000:00: root bus resource [bus 00] > > This implies that the [0 - 64K] check in acpi_dev_ioresource_flags() > leaves platforms with memory mapped IO space (ie IA64) broken (ie kernel > can't claim IO resources since the host bridge IO resource is disabled > and discarded by ACPI core code, see log on IA64 with missing root bridge > IO resource, silently filtered by current [0 - 64k] check in > acpi_dev_ioresource_flags()): > > PCI host bridge to bus 0000:00 > pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000fffff window] > pci_bus 0000:00: root bus resource [mem 0x80000000-0x8fffffff window] > pci_bus 0000:00: root bus resource [mem 0x80004000000-0x800ffffffff window] > pci_bus 0000:00: root bus resource [bus 00] > > [...] > > pci 0000:00:03.0: [1002:515e] type 00 class 0x030000 > pci 0000:00:03.0: reg 0x10: [mem 0x80000000-0x87ffffff pref] > pci 0000:00:03.0: reg 0x14: [io 0x1000-0x10ff] > pci 0000:00:03.0: reg 0x18: [mem 0x88020000-0x8802ffff] > pci 0000:00:03.0: reg 0x30: [mem 0x88000000-0x8801ffff pref] > pci 0000:00:03.0: supports D1 D2 > pci 0000:00:03.0: can't claim BAR 1 [io 0x1000-0x10ff]: no compatible > bridge window > > For this reason, the IO port resources boundaries check in generic ACPI > parsing code should be moved to x86 arch code so that more arches (ie > ARM64) can benefit from the generic ACPI resources parsing interface > without incurring in unexpected resource filtering, fixing at the same > time current breakage on IA64. > > This patch moves the IO ports boundary [0 - 64k] check to x86 arch code > code that validates the PCI host bridge resources. > > Fixes: 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing > interface for host bridge") > Signed-off-by: Lorenzo Pieralisi > Tested-by: Hanjun Guo > Cc: Bjorn Helgaas > Cc: Hanjun Guo > Cc: Jiang Liu > Cc: Tony Luck > Cc: Tomasz Nowicki > Cc: Mark Salter > Cc: "Rafael J. Wysocki" > --- > v1 -> v2 > > - Updated commit log to report missing IO resources > - Fixed function ioport_valid() comment 16k/64k typo > > v1: https://lkml.org/lkml/2016/2/1/157 > > arch/x86/pci/acpi.c | 18 +++++++++++++----- > drivers/acpi/resource.c | 3 --- > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c > index 3cd6983..cec68e7 100644 > --- a/arch/x86/pci/acpi.c > +++ b/arch/x86/pci/acpi.c > @@ -275,11 +275,14 @@ static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci) > * to access PCI configuration space. > * > * So explicitly filter out PCI CFG IO ports[0xCF8-0xCFF]. > + * > + * Furthermore, IO ports address space is limited to 64k on x86, > + * any IO resource exceeding the boundary must therefore be discarded. > */ > -static bool resource_is_pcicfg_ioport(struct resource *res) > +static bool ioport_valid(struct resource *res) > { > - return (res->flags & IORESOURCE_IO) && > - res->start == 0xCF8 && res->end == 0xCFF; > + return !(res->start == 0xCF8 && res->end == 0xCFF) && > + !(res->end >= 0x10003); > } > > static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci) > @@ -287,13 +290,18 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci) > struct acpi_device *device = ci->bridge; > int busnum = ci->root->secondary.start; > struct resource_entry *entry, *tmp; > + struct resource *res; > int status; > > status = acpi_pci_probe_root_resources(ci); > if (pci_use_crs) { > - resource_list_for_each_entry_safe(entry, tmp, &ci->resources) > - if (resource_is_pcicfg_ioport(entry->res)) > + resource_list_for_each_entry_safe(entry, tmp, &ci->resources) { > + res = entry->res; > + > + if (res->flags & IORESOURCE_IO && !ioport_valid(res)) > resource_list_destroy_entry(entry); > + } > + > return status; > } > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index d02fd53..c112e1d 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -127,9 +127,6 @@ static void acpi_dev_ioresource_flags(struct resource *res, u64 len, > if (!acpi_dev_resource_len_valid(res->start, res->end, len, true)) > res->flags |= IORESOURCE_DISABLED | IORESOURCE_UNSET; > > - if (res->end >= 0x10003) > - res->flags |= IORESOURCE_DISABLED | IORESOURCE_UNSET; > - > if (io_decode == ACPI_DECODE_16) > res->flags |= IORESOURCE_IO_16BIT_ADDR; > if (translation_type == ACPI_SPARSE_TRANSLATION) Reviewed-by: Hanjun Guo Thanks Hanjun