On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx>
>
>> 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@xxxxxxx>
>> Tested-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>> Cc: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>> Cc: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx>
>> Cc: Tony Luck <tony.luck@xxxxxxxxx>
>> Cc: Tomasz Nowicki <tn@xxxxxxxxxxxx>
>> Cc: Mark Salter <msalter@xxxxxxxxxx>
>> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
