| Message ID | 1455212827-9382-1-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 <lorenzo.pieralisi@arm.com> > Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Hanjun Guo <hanjun.guo@linaro.org> > Cc: Jiang Liu <jiang.liu@linux.intel.com> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Tomasz Nowicki <tn@semihalf.com> > Cc: Mark Salter <msalter@redhat.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > --- > 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 <hanjun.guo@linaro.org> Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 11, 2016 at 05:47:07PM +0000, 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. I definitely agree with moving this check out of the generic ACPI code, so while I have a minor question below, Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Fixes: 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing > interface for host bridge") 3772aea7d6f3 was merged via the ACPI tree. Does it make sense to have this fix for it merged the same way? I'll assume so unless Rafael thinks otherwise. > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Hanjun Guo <hanjun.guo@linaro.org> > Cc: Jiang Liu <jiang.liu@linux.intel.com> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Tomasz Nowicki <tn@semihalf.com> > Cc: Mark Salter <msalter@redhat.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > --- > 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); Is the "res->end >= 0x10003" test actually fixing a problem? I think 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support PCI host bridge") is the x86 change corresponding to 3772aea7d6f3. I took a quick look through it, and I didn't see a res->end test before 4d6b4e69a245, but maybe I missed it. The reason I'm asking is because there's no reason in principle that x86 couldn't support multiple host bridges, one with a 0-64K I/O space accessible via the x86 inb/outb instructions, and others with more I/O space accessible only via the in-kernel inb()/outb() functions, which would use an MMIO region that the host bridge converts to I/O accesses on the PCI side. This is what ia64 does, and x86 could do something similar. If it did, it would be fine for res->end to be above 0x10003 for those memory-mapped I/O spaces. > } > > 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) > -- > 2.5.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Thu, Feb 11, 2016 at 05:47:07PM +0000, 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. > > I definitely agree with moving this check out of the generic ACPI > code, so while I have a minor question below, > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > >> Fixes: 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing >> interface for host bridge") > > 3772aea7d6f3 was merged via the ACPI tree. Does it make sense to > have this fix for it merged the same way? I'll assume so unless > Rafael thinks otherwise. I'll apply it, thanks! >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: Hanjun Guo <hanjun.guo@linaro.org> >> Cc: Jiang Liu <jiang.liu@linux.intel.com> >> Cc: Tony Luck <tony.luck@intel.com> >> Cc: Tomasz Nowicki <tn@semihalf.com> >> Cc: Mark Salter <msalter@redhat.com> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> --- >> 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); > > Is the "res->end >= 0x10003" test actually fixing a problem? > > I think 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support > PCI host bridge") is the x86 change corresponding to 3772aea7d6f3. I > took a quick look through it, and I didn't see a res->end test before > 4d6b4e69a245, but maybe I missed it. > > The reason I'm asking is because there's no reason in principle that > x86 couldn't support multiple host bridges, one with a 0-64K I/O space > accessible via the x86 inb/outb instructions, and others with more I/O > space accessible only via the in-kernel inb()/outb() functions, which > would use an MMIO region that the host bridge converts to I/O accesses > on the PCI side. This is what ia64 does, and x86 could do something > similar. If it did, it would be fine for res->end to be above > 0x10003 for those memory-mapped I/O spaces. Interesting, but I guess quite theoretical. :-) In any case I think that may be fixed up on top of the $subject patch. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 11:27:01PM +0100, Rafael J. Wysocki wrote: > On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Feb 11, 2016 at 05:47:07PM +0000, 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. > > > > I definitely agree with moving this check out of the generic ACPI > > code, so while I have a minor question below, > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > >> Fixes: 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing > >> interface for host bridge") > > > > 3772aea7d6f3 was merged via the ACPI tree. Does it make sense to > > have this fix for it merged the same way? I'll assume so unless > > Rafael thinks otherwise. > > I'll apply it, thanks! > > >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > >> Cc: Bjorn Helgaas <bhelgaas@google.com> > >> Cc: Hanjun Guo <hanjun.guo@linaro.org> > >> Cc: Jiang Liu <jiang.liu@linux.intel.com> > >> Cc: Tony Luck <tony.luck@intel.com> > >> Cc: Tomasz Nowicki <tn@semihalf.com> > >> Cc: Mark Salter <msalter@redhat.com> > >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > >> --- > >> 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); > > > > Is the "res->end >= 0x10003" test actually fixing a problem? > > > > I think 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support > > PCI host bridge") is the x86 change corresponding to 3772aea7d6f3. I > > took a quick look through it, and I didn't see a res->end test before > > 4d6b4e69a245, but maybe I missed it. > > > > The reason I'm asking is because there's no reason in principle that > > x86 couldn't support multiple host bridges, one with a 0-64K I/O space > > accessible via the x86 inb/outb instructions, and others with more I/O > > space accessible only via the in-kernel inb()/outb() functions, which > > would use an MMIO region that the host bridge converts to I/O accesses > > on the PCI side. This is what ia64 does, and x86 could do something > > similar. If it did, it would be fine for res->end to be above > > 0x10003 for those memory-mapped I/O spaces. > > Interesting, but I guess quite theoretical. :-) > > In any case I think that may be fixed up on top of the $subject patch. Agreed on both counts. Thanks for taking this. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 11:27:01PM +0100, Rafael J. Wysocki wrote: > On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Feb 11, 2016 at 05:47:07PM +0000, 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. > > > > I definitely agree with moving this check out of the generic ACPI > > code, so while I have a minor question below, > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > >> Fixes: 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing > >> interface for host bridge") > > > > 3772aea7d6f3 was merged via the ACPI tree. Does it make sense to > > have this fix for it merged the same way? I'll assume so unless > > Rafael thinks otherwise. > > I'll apply it, thanks! > > >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > >> Cc: Bjorn Helgaas <bhelgaas@google.com> > >> Cc: Hanjun Guo <hanjun.guo@linaro.org> > >> Cc: Jiang Liu <jiang.liu@linux.intel.com> > >> Cc: Tony Luck <tony.luck@intel.com> > >> Cc: Tomasz Nowicki <tn@semihalf.com> > >> Cc: Mark Salter <msalter@redhat.com> > >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > >> --- > >> 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); > > > > Is the "res->end >= 0x10003" test actually fixing a problem? > > > > I think 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support > > PCI host bridge") is the x86 change corresponding to 3772aea7d6f3. I > > took a quick look through it, and I didn't see a res->end test before > > 4d6b4e69a245, but maybe I missed it. > > > > The reason I'm asking is because there's no reason in principle that > > x86 couldn't support multiple host bridges, one with a 0-64K I/O space > > accessible via the x86 inb/outb instructions, and others with more I/O > > space accessible only via the in-kernel inb()/outb() functions, which > > would use an MMIO region that the host bridge converts to I/O accesses > > on the PCI side. This is what ia64 does, and x86 could do something > > similar. If it did, it would be fine for res->end to be above > > 0x10003 for those memory-mapped I/O spaces. > > Interesting, but I guess quite theoretical. :-) > > In any case I think that may be fixed up on top of the $subject patch. Wait a minute, this doesn't seem right to me. The problem we're trying to fix is that on ia64, we incorrectly discard the PCI host bridge window [io 0x1000000-0x100ffff window]. That window is currently discarded by the generic acpi_dev_ioresource_flags() function, where we're removing this code: - if (res->end >= 0x10003) - res->flags |= IORESOURCE_DISABLED | IORESOURCE_UNSET; and we're adding the "res->end >= 0x10003" check to arch/x86/pci/acpi.c. But the removal also affects other users of acpi_dev_ioresource_flags(), and that's broader than the scope of this patch. We might want to remove the check, but if we do, it should be in a separate patch by itself so it isn't a hidden side-effect of fixing this ia64 problem. The other users of acpi_dev_ioresource_flags() include: {acpi_lpss_create_device,acpi_create_platform_device,acpi_pci_probe_root_resources,acpi_default_enumeration,bcm_acpi_probe,tpm_tis_acpi_init,acpi_dma_parse_resource_group,acpi_dma_request_slave_chan_by_index,acpi_gpio_resource_lookup,acpi_gpio_count,acpi_i2c_add_device,inv_mpu_process_acpi_config,acpi_spi_add_device} acpi_dev_get_resources acpi_dev_process_resource acpi_dev_resource_io acpi_dev_get_ioresource acpi_dev_ioresource_flags {pnpacpi_add_device,resources_store} pnpacpi_parse_allocated_resource pnpacpi_allocated_resource acpi_dev_resource_io acpi_dev_get_ioresource acpi_dev_ioresource_flags I think the original test in acpi_dev_ioresource_flags() isn't quite correct because it's generic code, but it enforces an arch-specific 64K limit. Maybe acpi_dev_ioresource_flags() should instead check res->end against ioport_resource.end, as we already do in acpi_pci_root_validate_resources()? Each arch already sets its own ioport_resource.end using IO_SPACE_LIMIT: arch/x86/include/asm/io.h #define IO_SPACE_LIMIT 0xffff arch/ia64/include/asm/io.h #define IO_SPACE_LIMIT 0xffffffffffffffffUL Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 11:33:32PM -0600, Bjorn Helgaas wrote: > On Tue, Mar 08, 2016 at 11:27:01PM +0100, Rafael J. Wysocki wrote: > > On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Thu, Feb 11, 2016 at 05:47:07PM +0000, 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. > > > > > > I definitely agree with moving this check out of the generic ACPI > > > code, so while I have a minor question below, > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > >> Fixes: 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing > > >> interface for host bridge") > > > > > > 3772aea7d6f3 was merged via the ACPI tree. Does it make sense to > > > have this fix for it merged the same way? I'll assume so unless > > > Rafael thinks otherwise. > > > > I'll apply it, thanks! > > > > >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > > >> Cc: Bjorn Helgaas <bhelgaas@google.com> > > >> Cc: Hanjun Guo <hanjun.guo@linaro.org> > > >> Cc: Jiang Liu <jiang.liu@linux.intel.com> > > >> Cc: Tony Luck <tony.luck@intel.com> > > >> Cc: Tomasz Nowicki <tn@semihalf.com> > > >> Cc: Mark Salter <msalter@redhat.com> > > >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > >> --- > > >> 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); > > > > > > Is the "res->end >= 0x10003" test actually fixing a problem? > > > > > > I think 4d6b4e69a245 ("x86/PCI/ACPI: Use common interface to support > > > PCI host bridge") is the x86 change corresponding to 3772aea7d6f3. I > > > took a quick look through it, and I didn't see a res->end test before > > > 4d6b4e69a245, but maybe I missed it. > > > > > > The reason I'm asking is because there's no reason in principle that > > > x86 couldn't support multiple host bridges, one with a 0-64K I/O space > > > accessible via the x86 inb/outb instructions, and others with more I/O > > > space accessible only via the in-kernel inb()/outb() functions, which > > > would use an MMIO region that the host bridge converts to I/O accesses > > > on the PCI side. This is what ia64 does, and x86 could do something > > > similar. If it did, it would be fine for res->end to be above > > > 0x10003 for those memory-mapped I/O spaces. > > > > Interesting, but I guess quite theoretical. :-) > > > > In any case I think that may be fixed up on top of the $subject patch. > > Wait a minute, this doesn't seem right to me. > > The problem we're trying to fix is that on ia64, we incorrectly > discard the PCI host bridge window [io 0x1000000-0x100ffff window]. > > That window is currently discarded by the generic > acpi_dev_ioresource_flags() function, where we're removing this code: > > - if (res->end >= 0x10003) > - res->flags |= IORESOURCE_DISABLED | IORESOURCE_UNSET; > > and we're adding the "res->end >= 0x10003" check to > arch/x86/pci/acpi.c. > > But the removal also affects other users of acpi_dev_ioresource_flags(), > and that's broader than the scope of this patch. We might want to > remove the check, but if we do, it should be in a separate patch by > itself so it isn't a hidden side-effect of fixing this ia64 problem. > > The other users of acpi_dev_ioresource_flags() include: > > {acpi_lpss_create_device,acpi_create_platform_device,acpi_pci_probe_root_resources,acpi_default_enumeration,bcm_acpi_probe,tpm_tis_acpi_init,acpi_dma_parse_resource_group,acpi_dma_request_slave_chan_by_index,acpi_gpio_resource_lookup,acpi_gpio_count,acpi_i2c_add_device,inv_mpu_process_acpi_config,acpi_spi_add_device} > acpi_dev_get_resources > acpi_dev_process_resource > acpi_dev_resource_io > acpi_dev_get_ioresource > acpi_dev_ioresource_flags > > {pnpacpi_add_device,resources_store} > pnpacpi_parse_allocated_resource > pnpacpi_allocated_resource > acpi_dev_resource_io > acpi_dev_get_ioresource > acpi_dev_ioresource_flags > > I think the original test in acpi_dev_ioresource_flags() isn't quite > correct because it's generic code, but it enforces an arch-specific > 64K limit. Yes, I was about to write to you I noticed the same issue. That (>=0x10003) check in generic code is an x86ism, it is wrong but it is there and given that there are other potential users of acpi_dev_ioresource_flags() this patch should be dropped I do not want to trigger x86 regressions because some IO resources are not filtered. I am travelling, so can't have a proper look till next week, Rafael, please drop this patch. > Maybe acpi_dev_ioresource_flags() should instead check res->end > against ioport_resource.end, as we already do in > acpi_pci_root_validate_resources()? Each arch already sets its own > ioport_resource.end using IO_SPACE_LIMIT: > > arch/x86/include/asm/io.h #define IO_SPACE_LIMIT 0xffff > arch/ia64/include/asm/io.h #define IO_SPACE_LIMIT 0xffffffffffffffffUL We can't do that, it may work on IA64 but the ioport_resource is a chunk of address space on IA64/ARM64 that has nothing to do with the physical address at which the root bridges decode IO space (which is what's contained in the resource). I will have a look next week, please drop this patch. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 9, 2016 at 8:14 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Tue, Mar 08, 2016 at 11:33:32PM -0600, Bjorn Helgaas wrote: >> On Tue, Mar 08, 2016 at 11:27:01PM +0100, Rafael J. Wysocki wrote: >> > On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > > On Thu, Feb 11, 2016 at 05:47:07PM +0000, Lorenzo Pieralisi wrote: >> > >> The [0 - 64k] ACPI PCI IO port resource boundary check in: >> > >> [cut] >> Wait a minute, this doesn't seem right to me. >> >> The problem we're trying to fix is that on ia64, we incorrectly >> discard the PCI host bridge window [io 0x1000000-0x100ffff window]. >> >> That window is currently discarded by the generic >> acpi_dev_ioresource_flags() function, where we're removing this code: >> >> - if (res->end >= 0x10003) >> - res->flags |= IORESOURCE_DISABLED | IORESOURCE_UNSET; >> >> and we're adding the "res->end >= 0x10003" check to >> arch/x86/pci/acpi.c. >> >> But the removal also affects other users of acpi_dev_ioresource_flags(), >> and that's broader than the scope of this patch. We might want to >> remove the check, but if we do, it should be in a separate patch by >> itself so it isn't a hidden side-effect of fixing this ia64 problem. >> >> The other users of acpi_dev_ioresource_flags() include: >> >> {acpi_lpss_create_device,acpi_create_platform_device,acpi_pci_probe_root_resources,acpi_default_enumeration,bcm_acpi_probe,tpm_tis_acpi_init,acpi_dma_parse_resource_group,acpi_dma_request_slave_chan_by_index,acpi_gpio_resource_lookup,acpi_gpio_count,acpi_i2c_add_device,inv_mpu_process_acpi_config,acpi_spi_add_device} >> acpi_dev_get_resources >> acpi_dev_process_resource >> acpi_dev_resource_io >> acpi_dev_get_ioresource >> acpi_dev_ioresource_flags >> >> {pnpacpi_add_device,resources_store} >> pnpacpi_parse_allocated_resource >> pnpacpi_allocated_resource >> acpi_dev_resource_io >> acpi_dev_get_ioresource >> acpi_dev_ioresource_flags >> >> I think the original test in acpi_dev_ioresource_flags() isn't quite >> correct because it's generic code, but it enforces an arch-specific >> 64K limit. > > Yes, I was about to write to you I noticed the same issue. > > That (>=0x10003) check in generic code is an x86ism, it is wrong but > it is there and given that there are other potential > users of acpi_dev_ioresource_flags() this patch should > be dropped I do not want to trigger x86 regressions because > some IO resources are not filtered. > > I am travelling, so can't have a proper look till next week, > Rafael, please drop this patch. > >> Maybe acpi_dev_ioresource_flags() should instead check res->end >> against ioport_resource.end, as we already do in >> acpi_pci_root_validate_resources()? Each arch already sets its own >> ioport_resource.end using IO_SPACE_LIMIT: >> >> arch/x86/include/asm/io.h #define IO_SPACE_LIMIT 0xffff >> arch/ia64/include/asm/io.h #define IO_SPACE_LIMIT 0xffffffffffffffffUL > > We can't do that, it may work on IA64 but the ioport_resource is a > chunk of address space on IA64/ARM64 that has nothing to do with > the physical address at which the root bridges decode IO space (which > is what's contained in the resource). > > I will have a look next week, please drop this patch. OK, dropping. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 07:14:06AM +0000, Lorenzo Pieralisi wrote: > On Tue, Mar 08, 2016 at 11:33:32PM -0600, Bjorn Helgaas wrote: > > Maybe acpi_dev_ioresource_flags() should instead check res->end > > against ioport_resource.end, as we already do in > > acpi_pci_root_validate_resources()? Each arch already sets its own > > ioport_resource.end using IO_SPACE_LIMIT: > > > > arch/x86/include/asm/io.h #define IO_SPACE_LIMIT 0xffff > > arch/ia64/include/asm/io.h #define IO_SPACE_LIMIT 0xffffffffffffffffUL I suspect we're saying the same thing with slightly different words, so I apologize if I'm beating a dead horse. ioport_resource defines the universe of Linux ioport space. Every IORESOURCE_IO struct resource must be contained in ioport_resource. Such a resource contains Linux ioport numbers, i.e., things that could appear in /proc/ioports and could be passed as an argument to request_region() and the inb()/outb() functions. > We can't do that, it may work on IA64 but the ioport_resource is a > chunk of address space on IA64/ARM64 that has nothing to do with > the physical address at which the root bridges decode IO space (which > is what's contained in the resource). I'm stumbling over "the physical address at which the root bridges decode IO space (which is what's contained in the resource)". x86 host bridges typically just forward CPU-generated I/O port transactions to PCI with no address translation, so it sounds like you're talking about ia64/ARM64 bridges that decode CPU MMIO space and turn accesses into PCI I/O transactions. If we start from a driver calling inb(), the driver supplies a Linux ioport number "X". On ia64 (and I assume ARM64 as well), inb() performs an MMIO access to memory address "Y". A host bridge consumes that MMIO access and generates a corresponding PCI I/O transaction to PCI ioport "Z". It is indeed true that ioport_resource has nothing to do with memory addresses like "Y". But the driver (and this part of the ACPI infrastructure) are dealing with ioport address "X", and that is definitely in an IORESOURCE_IO struct resource. The IORESOURCE_MEM struct resource that contains "Y" is internal to the host bridge driver and invisible to the device driver that's using inb(). For example, you could have something like this (with made-up addresses): [io 0x0000-0xffff] # Linux ioport space [mem 0x100000000-0x10000ffff] # MMIO space consumed by host bridge [bus 0x0000-0xffff] # PCI bus I/O transaction space [io 0x10000-0x1ffff] # Linux ioport space ("X") [mem 0x200000000-0x20000ffff] # MMIO space consumed by host bridge ("Y") [bus 0x0000-0xffff] # PCI bus I/O transaction space ("Z") Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 09:35:56AM -0600, Bjorn Helgaas wrote: > On Wed, Mar 09, 2016 at 07:14:06AM +0000, Lorenzo Pieralisi wrote: > > On Tue, Mar 08, 2016 at 11:33:32PM -0600, Bjorn Helgaas wrote: > > > > Maybe acpi_dev_ioresource_flags() should instead check res->end > > > against ioport_resource.end, as we already do in > > > acpi_pci_root_validate_resources()? Each arch already sets its own > > > ioport_resource.end using IO_SPACE_LIMIT: > > > > > > arch/x86/include/asm/io.h #define IO_SPACE_LIMIT 0xffff > > > arch/ia64/include/asm/io.h #define IO_SPACE_LIMIT 0xffffffffffffffffUL > > I suspect we're saying the same thing with slightly different words, > so I apologize if I'm beating a dead horse. > > ioport_resource defines the universe of Linux ioport space. Every > IORESOURCE_IO struct resource must be contained in ioport_resource. > Such a resource contains Linux ioport numbers, i.e., things that could > appear in /proc/ioports and could be passed as an argument to > request_region() and the inb()/outb() functions. That's agreed but that's not where the problem is on IA64/ARM64 IO ACPI descriptors parsing. > > We can't do that, it may work on IA64 but the ioport_resource is a > > chunk of address space on IA64/ARM64 that has nothing to do with > > the physical address at which the root bridges decode IO space (which > > is what's contained in the resource). > > I'm stumbling over "the physical address at which the root bridges > decode IO space (which is what's contained in the resource)". > > x86 host bridges typically just forward CPU-generated I/O port > transactions to PCI with no address translation, so it sounds like > you're talking about ia64/ARM64 bridges that decode CPU MMIO space and > turn accesses into PCI I/O transactions. > > If we start from a driver calling inb(), the driver supplies a Linux > ioport number "X". On ia64 (and I assume ARM64 as well), inb() > performs an MMIO access to memory address "Y". A host bridge consumes > that MMIO access and generates a corresponding PCI I/O transaction to > PCI ioport "Z". > > It is indeed true that ioport_resource has nothing to do with memory > addresses like "Y". But the driver (and this part of the ACPI > infrastructure) are dealing with ioport address "X", and that is > definitely in an IORESOURCE_IO struct resource. The IORESOURCE_MEM_ > struct resource that contains "Y" is internal to the host bridge > driver and invisible to the device driver that's using inb(). The problem is, that's the address like "Y" that are checked in acpi_dev_ioresource_flags() with current PCI initialization flow. IO ports number "X" are created in (IA64) add_io_space() that is executed after acpi_pci_probe_root_resources() is called (see arch/ia64/pci/pci.c), that's the code that, IIUC, translates an IO resource containing a CPU physical address (which is the address that is ioremapped) to an IO resource containing a port number *and* a corresponding MEM resource. The check that causes failures (>=0x10003) is carried out on the "raw" IO resource parsed from ACPI, not the one crafted in add_io_space(). The resource checked in acpi_dev_ioresource_flags() is created from ACPI IO descriptor and does contain CPU physical MMIO addresses, comparing it to a port number is bound to fail, they do NOT represent the same thing, happy be corrected. That's the reason why currently IA64 IO space is not working, and this patch fixes it, please correct me if I am wrong. I will write back next week with the commits sequence and logic that led to the current failure, sorry for not being able to respond more promptly and in a comprehensive way I need sometime to write up my understanding of the problem and commit logs, I will do that next week. Thanks, Lorenzo > > For example, you could have something like this (with made-up > addresses): > > [io 0x0000-0xffff] # Linux ioport space > [mem 0x100000000-0x10000ffff] # MMIO space consumed by host bridge > [bus 0x0000-0xffff] # PCI bus I/O transaction space > > [io 0x10000-0x1ffff] # Linux ioport space ("X") > [mem 0x200000000-0x20000ffff] # MMIO space consumed by host bridge ("Y") > [bus 0x0000-0xffff] # PCI bus I/O transaction space ("Z") > > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 10, 2016 at 07:42:36AM +0000, Lorenzo Pieralisi wrote: [...] > > > We can't do that, it may work on IA64 but the ioport_resource is a > > > chunk of address space on IA64/ARM64 that has nothing to do with > > > the physical address at which the root bridges decode IO space (which > > > is what's contained in the resource). > > > > I'm stumbling over "the physical address at which the root bridges > > decode IO space (which is what's contained in the resource)". > > > > x86 host bridges typically just forward CPU-generated I/O port > > transactions to PCI with no address translation, so it sounds like > > you're talking about ia64/ARM64 bridges that decode CPU MMIO space and > > turn accesses into PCI I/O transactions. > > > > If we start from a driver calling inb(), the driver supplies a Linux > > ioport number "X". On ia64 (and I assume ARM64 as well), inb() > > performs an MMIO access to memory address "Y". A host bridge consumes > > that MMIO access and generates a corresponding PCI I/O transaction to > > PCI ioport "Z". > > > > It is indeed true that ioport_resource has nothing to do with memory > > addresses like "Y". But the driver (and this part of the ACPI > > infrastructure) are dealing with ioport address "X", and that is > > definitely in an IORESOURCE_IO struct resource. The IORESOURCE_MEM_ > > struct resource that contains "Y" is internal to the host bridge > > driver and invisible to the device driver that's using inb(). > > The problem is, that's the address like "Y" that are checked in > acpi_dev_ioresource_flags() with current PCI initialization flow. > > IO ports number "X" are created in (IA64) add_io_space() that is > executed after acpi_pci_probe_root_resources() is called (see > arch/ia64/pci/pci.c), that's the code that, IIUC, translates an > IO resource containing a CPU physical address (which is the address > that is ioremapped) to an IO resource containing a port number *and* > a corresponding MEM resource. > > The check that causes failures (>=0x10003) is carried out > on the "raw" IO resource parsed from ACPI, not the one crafted > in add_io_space(). > > The resource checked in acpi_dev_ioresource_flags() is created from > ACPI IO descriptor and does contain CPU physical MMIO addresses, > comparing it to a port number is bound to fail, they do NOT represent > the same thing, happy be corrected. > > That's the reason why currently IA64 IO space is not working, and > this patch fixes it, please correct me if I am wrong. > > I will write back next week with the commits sequence and logic that > led to the current failure, sorry for not being able to respond > more promptly and in a comprehensive way I need sometime to write up > my understanding of the problem and commit logs, I will do that > next week. I had a further look and went through commit history and I think my explanation above is correct, current code for IA64 PCI IO space is wrong IMO and the failures started with: commit 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing interface for host bridge") because that's the commit that added IA64 PCI code to use acpi_dev_get_resources(), let me add to that. For the records, and for the same reason, let's imagine we can fix the (>=0x10003) check in: acpi_dev_ioresource_flags() the code in acpi_pci_probe_root_resources() that checks the PCI IO resources (ie acpi_pci_root_validate_resources()) is wrong, in that it validates the PCI IO resources obtained from ACPI IO descriptors against ioport_resource, and that's not correct, they do not represent the same thing. Code in acpi_dev_get_resources() walks the device (PCI host bridge in this case) resources (ie _CRS) with acpi_dev_process_resource(). On IA64 IO space is memory mapped, therefore the CPU physical address at which the PCI host bridge maps the IO space has to come from ACPI IO descriptors. IIUC, those descriptors are parsed through: acpi_dev_resource_address_space() acpi_dev_resource_ext_address_space() which in turn call: acpi_decode_space() where the resource window is actually created, in particular the acpi_address64_attribute value contains (on IA64, I verified with ACPI spec working group through actual ACPI tables): attr->minimum = PCI IO port min value attr->translation_offset = offset to add to attr->minimum to obtain the CPU physical address at which the PCI IO bridge decodes IO space. Translation offset, according to the ACPI specification has to be used when resources on primary and secondary sides of the PCI bridge are different (IIUC secondary bus represents PCI IO ports, primary bus CPU physical addresses mapping IO ports in CPU physical address space). This is the physical address that is remapped in IA64 new_space(), to map the bridge CPU physical address into the virtual address space, and it has always been so, even before 3772aea7d6f3. Now, the resource window, in acpi_decode_space() is created as: res->start = attr->minimum + attr->translation_offset; res->end = attr->maximum + attr->translation_offset; and that's the resource we have in acpi_dev_ioresource_flags(), if we are comparing it against ioport_resource that's not correct since as I already mentioned, they represent *different* things. There is something we can do, which is, checking translation_type in acpi_dev_ioresource_flags(), if it is == TypeTranslation (which basically means that the IO space is MMIO) the >=10003 can be skipped, but that's hackish (also because I am not sure IA64 platforms set translation_type consistently in ACPI tables). Even if we do that, call to acpi_pci_root_validate_resources() for IO space is wrong on IA64, we are comparing an IO resource against ioport_resource there, but the Linux IO port space for the host bridge is created in IA64 pci_acpi_root_prepare_resources() with the call to add_io_space() which happens *after* the sequence above is executed. On IA64: pci_acpi_root_prepare_resources() -> acpi_pci_probe_root_resources() -> acpi_dev_get_resources() -> acpi_pci_root_validate_resources() -> add_io_space() # this is where the Linux IO port space for the bridge is created and that's when we can validate the IO resource against ioport_resource I have no experience/knowledge of IA64 systems so I may be totally wrong, I just want to understand. Comments welcome, Hanjun proved my understanding by testing on IA64 and current mainline just does not work (see commit log for failure messages), feedback from IA64 people is really needed here, we have to get this fixed please (and through that fix, getting it to work on ARM64 too). Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 02:53:22PM +0000, Lorenzo Pieralisi wrote: > On Thu, Mar 10, 2016 at 07:42:36AM +0000, Lorenzo Pieralisi wrote: > > [...] > > > > > We can't do that, it may work on IA64 but the ioport_resource is a > > > > chunk of address space on IA64/ARM64 that has nothing to do with > > > > the physical address at which the root bridges decode IO space (which > > > > is what's contained in the resource). > > > > > > I'm stumbling over "the physical address at which the root bridges > > > decode IO space (which is what's contained in the resource)". > > > > > > x86 host bridges typically just forward CPU-generated I/O port > > > transactions to PCI with no address translation, so it sounds like > > > you're talking about ia64/ARM64 bridges that decode CPU MMIO space and > > > turn accesses into PCI I/O transactions. > > > > > > If we start from a driver calling inb(), the driver supplies a Linux > > > ioport number "X". On ia64 (and I assume ARM64 as well), inb() > > > performs an MMIO access to memory address "Y". A host bridge consumes > > > that MMIO access and generates a corresponding PCI I/O transaction to > > > PCI ioport "Z". > > > > > > It is indeed true that ioport_resource has nothing to do with memory > > > addresses like "Y". But the driver (and this part of the ACPI > > > infrastructure) are dealing with ioport address "X", and that is > > > definitely in an IORESOURCE_IO struct resource. The IORESOURCE_MEM_ > > > struct resource that contains "Y" is internal to the host bridge > > > driver and invisible to the device driver that's using inb(). > > > > The problem is, that's the address like "Y" that are checked in > > acpi_dev_ioresource_flags() with current PCI initialization flow. > > > > IO ports number "X" are created in (IA64) add_io_space() that is > > executed after acpi_pci_probe_root_resources() is called (see > > arch/ia64/pci/pci.c), that's the code that, IIUC, translates an > > IO resource containing a CPU physical address (which is the address > > that is ioremapped) to an IO resource containing a port number *and* > > a corresponding MEM resource. > > > > The check that causes failures (>=0x10003) is carried out > > on the "raw" IO resource parsed from ACPI, not the one crafted > > in add_io_space(). > > > > The resource checked in acpi_dev_ioresource_flags() is created from > > ACPI IO descriptor and does contain CPU physical MMIO addresses, > > comparing it to a port number is bound to fail, they do NOT represent > > the same thing, happy be corrected. > > > > That's the reason why currently IA64 IO space is not working, and > > this patch fixes it, please correct me if I am wrong. > > > > I will write back next week with the commits sequence and logic that > > led to the current failure, sorry for not being able to respond > > more promptly and in a comprehensive way I need sometime to write up > > my understanding of the problem and commit logs, I will do that > > next week. > > I had a further look and went through commit history and I think > my explanation above is correct, current code for IA64 PCI IO space > is wrong IMO and the failures started with: > > commit 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing > interface for host bridge") > > because that's the commit that added IA64 PCI code to use > acpi_dev_get_resources(), let me add to that. > > For the records, and for the same reason, let's imagine we can fix the > (>=0x10003) check in: > > acpi_dev_ioresource_flags() > > the code in acpi_pci_probe_root_resources() that checks the PCI IO > resources (ie acpi_pci_root_validate_resources()) is wrong, in that it > validates the PCI IO resources obtained from ACPI IO descriptors against > ioport_resource, and that's not correct, they do not represent the > same thing. > > Code in acpi_dev_get_resources() walks the device (PCI host bridge in > this case) resources (ie _CRS) with acpi_dev_process_resource(). > > On IA64 IO space is memory mapped, therefore the CPU physical address > at which the PCI host bridge maps the IO space has to come from ACPI > IO descriptors. > > IIUC, those descriptors are parsed through: > > acpi_dev_resource_address_space() > acpi_dev_resource_ext_address_space() > > which in turn call: > > acpi_decode_space() > > where the resource window is actually created, in particular the > acpi_address64_attribute value contains (on IA64, I verified with > ACPI spec working group through actual ACPI tables): > > attr->minimum = PCI IO port min value > attr->translation_offset = offset to add to attr->minimum to obtain the > CPU physical address at which the PCI IO > bridge decodes IO space. Translation offset, > according to the ACPI specification has to > be used when resources on primary and > secondary sides of the PCI bridge are > different (IIUC secondary bus represents PCI > IO ports, primary bus CPU physical addresses > mapping IO ports in CPU physical address > space). This is the physical address that is > remapped in IA64 new_space(), to map the > bridge CPU physical address into the virtual > address space, and it has always been so, even > before 3772aea7d6f3. > > Now, the resource window, in acpi_decode_space() is created as: > > res->start = attr->minimum + attr->translation_offset; > res->end = attr->maximum + attr->translation_offset; > > and that's the resource we have in acpi_dev_ioresource_flags(), if we > are comparing it against ioport_resource that's not correct since as I > already mentioned, they represent *different* things. > > There is something we can do, which is, checking translation_type > in acpi_dev_ioresource_flags(), if it is == TypeTranslation (which > basically means that the IO space is MMIO) the >=10003 can be skipped, > but that's hackish (also because I am not sure IA64 platforms set > translation_type consistently in ACPI tables). > > Even if we do that, call to acpi_pci_root_validate_resources() for > IO space is wrong on IA64, we are comparing an IO resource against > ioport_resource there, but the Linux IO port space for the host > bridge is created in IA64 pci_acpi_root_prepare_resources() with > the call to add_io_space() which happens *after* the sequence above > is executed. > > On IA64: > > pci_acpi_root_prepare_resources() > -> acpi_pci_probe_root_resources() > -> acpi_dev_get_resources() > -> acpi_pci_root_validate_resources() > -> add_io_space() # this is where the Linux IO port space for the bridge is > created and that's when we can validate the IO > resource against ioport_resource > > I have no experience/knowledge of IA64 systems so I may be totally wrong, > I just want to understand. > > Comments welcome, Hanjun proved my understanding by testing on IA64 and > current mainline just does not work (see commit log for failure messages), > feedback from IA64 people is really needed here, we have to get this fixed > please (and through that fix, getting it to work on ARM64 too). I/O port translations makes my head hurt, but I think you're right. The raw I/O resources from ACPI _CRS are definitely different from the Linux ioport spaces constructed by add_io_space(), so we shouldn't compare the two. If we need to validate the raw I/O ports from _CRS, I suppose in theory we should be able to check that they're contained in the raw I/O port range of an upstream window. I think the problem is that 3772aea7d6f3 started applying the x86-specific "[0-0xffff]" restriction to all arches, so if you want to back that out so it applies only on x86 (but to all devices, not just PNP0A03), I think that would make sense. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 02:27:08PM -0500, Bjorn Helgaas wrote: > On Mon, Mar 14, 2016 at 02:53:22PM +0000, Lorenzo Pieralisi wrote: [...] > > Even if we do that, call to acpi_pci_root_validate_resources() for > > IO space is wrong on IA64, we are comparing an IO resource against > > ioport_resource there, but the Linux IO port space for the host > > bridge is created in IA64 pci_acpi_root_prepare_resources() with > > the call to add_io_space() which happens *after* the sequence above > > is executed. > > > > On IA64: > > > > pci_acpi_root_prepare_resources() > > -> acpi_pci_probe_root_resources() > > -> acpi_dev_get_resources() > > -> acpi_pci_root_validate_resources() > > -> add_io_space() # this is where the Linux IO port space for the bridge is > > created and that's when we can validate the IO > > resource against ioport_resource > > > > I have no experience/knowledge of IA64 systems so I may be totally wrong, > > I just want to understand. > > > > Comments welcome, Hanjun proved my understanding by testing on IA64 and > > current mainline just does not work (see commit log for failure messages), > > feedback from IA64 people is really needed here, we have to get this fixed > > please (and through that fix, getting it to work on ARM64 too). > > I/O port translations makes my head hurt, but I think you're right. > > The raw I/O resources from ACPI _CRS are definitely different from the > Linux ioport spaces constructed by add_io_space(), so we shouldn't > compare the two. > > If we need to validate the raw I/O ports from _CRS, I suppose in > theory we should be able to check that they're contained in the raw > I/O port range of an upstream window. > > I think the problem is that 3772aea7d6f3 started applying the > x86-specific "[0-0xffff]" restriction to all arches, so if you want to > back that out so it applies only on x86 (but to all devices, not just > PNP0A03), I think that would make sense. I noticed there is already an X86 specific check in: drivers/acpi/resource.c (ie valid_IRQ) I can add something like code below there and be done with it: #ifdef CONFIG_X86 static inline bool io_space_valid(struct resource *res) { return res->end < 0x10003; } #else static inline bool io_space_valid(struct resource *res) { return true; } #endif if Rafael is ok with that. Whatever I do requires an arch specific hook (empty/always-true on !CONFIG_X86) to be called from acpi_dev_ioresource_flags(), otherwise removing that check really becomes a minefield. Other solution is reverting back to not using acpi_dev_get_resources() on IA64 and ARM64 which defeats the whole purpose of Jiang's consolidation, so I won't go there. Next step is removing (or refactoring) acpi_pci_root_validate_resources() for IORESOURCE_IO from acpi_pci_probe_root_resources(), it is a bogus check as our discussion above highlighted and does not work on ARM64 (it probably does on IA64 since IO_SPACE_LIMIT == 0xffffffffffffffffUL, but that's conceptually wrong regardless). Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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 <lorenzo.pieralisi@arm.com> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Hanjun Guo <hanjun.guo@linaro.org> Cc: Jiang Liu <jiang.liu@linux.intel.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Tomasz Nowicki <tn@semihalf.com> Cc: Mark Salter <msalter@redhat.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> --- 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(-)