[PATCH] powerpc/PCI: compute I/O space bus-to-resource offset consistently

[PATCH] powerpc/PCI: compute I/O space bus-to-resource offset consistently

[Bjorn Helgaas]

Make sure we compute CPU addresses (resource start/end) the same way both
when we set up the I/O aperture (hose->io_resource) and when we use
pcibios_bus_to_resource() to convert BAR values into resources.

CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/powerpc/include/asm/pci.h   |    1 +
 arch/powerpc/kernel/pci-common.c |    8 ++++++--
 arch/powerpc/kernel/pci_32.c     |    6 +++---
 arch/powerpc/kernel/pci_64.c     |    2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 201e352..6653f27 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -182,6 +182,7 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
 				 const struct resource *rsrc,
 				 resource_size_t *start, resource_size_t *end);
 
+extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
 extern void pcibios_setup_bus_devices(struct pci_bus *bus);
 extern void pcibios_setup_bus_self(struct pci_bus *bus);
 extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 910b9de..2efd52d 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1492,6 +1492,11 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 	return pci_enable_resources(dev, mask);
 }
 
+resource_size_t pcibios_io_space_offset(struct pci_controller *hose)
+{
+	return (unsigned long) hose->io_base_virt - _IO_BASE;
+}
+
 static void __devinit pcibios_setup_phb_resources(struct pci_controller *hose, struct list_head *resources)
 {
 	struct resource *res;
@@ -1516,8 +1521,7 @@ static void __devinit pcibios_setup_phb_resources(struct pci_controller *hose, s
 		 (unsigned long long)res->start,
 		 (unsigned long long)res->end,
 		 (unsigned long)res->flags);
-	pci_add_resource_offset(resources, res,
-			(resource_size_t) hose->io_base_virt - _IO_BASE);
+	pci_add_resource_offset(resources, res, pcibios_io_space_offset(hose));
 
 	/* Hookup PHB Memory resources */
 	for (i = 0; i < 3; ++i) {
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index fdd1a3d..4b06ec5 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -219,9 +219,9 @@ void __devinit pcibios_setup_phb_io_space(struct pci_controller *hose)
 	struct resource *res = &hose->io_resource;
 
 	/* Fixup IO space offset */
-	io_offset = (unsigned long)hose->io_base_virt - isa_io_base;
-	res->start = (res->start + io_offset) & 0xffffffffu;
-	res->end = (res->end + io_offset) & 0xffffffffu;
+	io_offset = pcibios_io_space_offset(hose);
+	res->start += io_offset;
+	res->end += io_offset;
 }
 
 static int __init pcibios_init(void)
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 75417fd..94a54f6 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -168,7 +168,7 @@ static int __devinit pcibios_map_phb_io_space(struct pci_controller *hose)
 		return -ENOMEM;
 
 	/* Fixup hose IO resource */
-	io_virt_offset = (unsigned long)hose->io_base_virt - _IO_BASE;
+	io_virt_offset = pcibios_io_space_offset(hose);
 	hose->io_resource.start += io_virt_offset;
 	hose->io_resource.end += io_virt_offset;
 

Re: [PATCH] powerpc/PCI: compute I/O space bus-to-resource offset consistently

[Benjamin Herrenschmidt]

On Mon, 2012-02-27 at 19:50 -0700, Bjorn Helgaas wrote:
> --- a/arch/powerpc/kernel/pci_32.c
> +++ b/arch/powerpc/kernel/pci_32.c
> @@ -219,9 +219,9 @@ void __devinit pcibios_setup_phb_io_space(struct
> pci_controller *hose)
>         struct resource *res = &hose->io_resource;
>  
>         /* Fixup IO space offset */
> -       io_offset = (unsigned long)hose->io_base_virt - isa_io_base;
> -       res->start = (res->start + io_offset) & 0xffffffffu;
> -       res->end = (res->end + io_offset) & 0xffffffffu;
> +       io_offset = pcibios_io_space_offset(hose);
> +       res->start += io_offset;
> +       res->end += io_offset;
>  } 

Well, you are losing the 0xffffffff mask... so basically, what happens
is that your offset, if negative, will be 0 extended by
pcibios_io_space_offset() instead of sign-extended, and thus the
resource will have crap stuff in the top 32-bit.

As I said on irc, it might be a non-issue because the inX/outX accessors
are going to take an unsigned int port number anyway, but anything that
tries to print out that resource (or expose it to userspace) will look
bad.

One way to fix that is to ensure that pcibios_io_space_offset() does a
full sign extension to 64-bit.

Cheers,
Ben.

Re: [PATCH] powerpc/PCI: compute I/O space bus-to-resource offset consistently

[Bjorn Helgaas]

On Mon, Feb 27, 2012 at 8:52 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2012-02-27 at 19:50 -0700, Bjorn Helgaas wrote:
>> --- a/arch/powerpc/kernel/pci_32.c
>> +++ b/arch/powerpc/kernel/pci_32.c
>> @@ -219,9 +219,9 @@ void __devinit pcibios_setup_phb_io_space(struct
>> pci_controller *hose)
>>         struct resource *res = &hose->io_resource;
>>
>>         /* Fixup IO space offset */
>> -       io_offset = (unsigned long)hose->io_base_virt - isa_io_base;
>> -       res->start = (res->start + io_offset) & 0xffffffffu;
>> -       res->end = (res->end + io_offset) & 0xffffffffu;
>> +       io_offset = pcibios_io_space_offset(hose);
>> +       res->start += io_offset;
>> +       res->end += io_offset;
>>  }
>
> Well, you are losing the 0xffffffff mask... so basically, what happens
> is that your offset, if negative, will be 0 extended by
> pcibios_io_space_offset() instead of sign-extended, and thus the
> resource will have crap stuff in the top 32-bit.

I don't think it will have random crap.  Here's what I think will
happen.  Assume typical 64KB I/O port spaces, PCI bus addresses in the
range 0x0-0xffff, with isa_io_base = 0x10000000.  If we compute the
offset as I did in the patch:

    (unsigned long) hose->io_base_virt - _IO_BASE

the subtraction result is 32-bits and is zero-extended to 64 bits.
Alternatively, we could do this:

    (unsigned long long) (unsigned long) hose->io_base_virt - _IO_BASE

Here the conversion to 64 bits happens before the subtraction, so the
upper 32 bits of the result are likely all ones.  Examples:

hose->io_base_virt      offset                  resource
0x10000000              0x00000000              [io 0x0000-0xffff]
0x11000000              0x01000000              [io 0x01000000-0x0100ffff]
0x0f000000              0xff000000              [io 0xff000000-0xff00ffff]

0x10000000              0x00000000              [io 0x0000-0xffff]
0x11000000              0x01000000              [io 0x01000000-0x0100ffff]
0x0f000000              0xffffffffff000000      [io
0xffffffffff000000-0xffffffffff00ffff]

Either way will probably work since you only care about the low 32
bits, which are the same either way.  I chose the first because
casting the 32-bit pointer to a 64-bit value seemed to require two
ugly casts and the resulting resource values look unwieldy when
printed.

Bjorn