ARM: PCI: implement generic PCI host controller

This patch moves bios32 over to using the generic code for enabling PCI resources. Since the core code takes care of bridge resources too, we can also drop the explicit IO and MEMORY enabling for them in the arch code.

A side-effect of this change is that we no longer explicitly enable devices when running in PCI_PROBE_ONLY mode. This stays closer to the meaning of the option and prevents us from trying to enable devices without any assigned resources (the core code refuses to enable resources without parents).

Tested-by Jason Gunthorpe <[email protected]>

Tested-by Jingoo Han <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 11 lines · Will Deacon

This patch moves bios32 over to using the generic code for enabling PCI resources. Since the core code takes care of bridge resources too, we can also drop the explicit IO and MEMORY enabling for them in the arch code.

A side-effect of this change is that we no longer explicitly enable devices when running in PCI_PROBE_ONLY mode. This stays closer to the meaning of the option and prevents us from trying to enable devices without any assigned resources (the core code refuses to enable resources without parents).

Quoted 3 lines

Tested-by Jason Gunthorpe <[email protected]>

Tested-by Jingoo Han <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Tested acceptably fine here with crudbus-from-hell.

Tested-by Russell King <[email protected]>

Quoted 11 lines · Will Deacon

This patch moves bios32 over to using the generic code for enabling PCI resources. Since the core code takes care of bridge resources too, we can also drop the explicit IO and MEMORY enabling for them in the arch code.

A side-effect of this change is that we no longer explicitly enable devices when running in PCI_PROBE_ONLY mode. This stays closer to the meaning of the option and prevents us from trying to enable devices without any assigned resources (the core code refuses to enable resources without parents).

Quoted 3 lines

Tested-by Jason Gunthorpe <[email protected]>

Tested-by Jingoo Han <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Tested acceptably fine here with crudbus-from-hell.

Tested-by Russell King <[email protected]>

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines · Will Deacon

This patch moves bios32 over to using the generic code for enabling PCI resources. Since the core code takes care of bridge resources too, we can also drop the explicit IO and MEMORY enabling for them in the arch code.

A side-effect of this change is that we no longer explicitly enable devices when running in PCI_PROBE_ONLY mode. This stays closer to the meaning of the option and prevents us from trying to enable devices without any assigned resources (the core code refuses to enable resources without parents).

Quoted 7 lines

Quoted 3 lines

Tested-by Jason Gunthorpe <[email protected]>

Tested-by Jingoo Han <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Tested acceptably fine here with crudbus-from-hell.

Tested-by Russell King <[email protected]>

Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.

The problem is that bios32.c won't create a resource hierarchy for the firmware-initialised resources, so we have a bunch of orphaned resources that we can't pass to pci_enable_resources (since it checks r->parent).

This means that, unless firmware *enables* all of the resources, Linux won't be able to enable them. I think this is stronger than simply not re-assigning devices like the PCI_PROBE_ONLY flag intends.

I can see two solutions:

(1) Just drop this patch and stick with the old code (which doesn't check r->parent). The downside is we still don't have any resource hierarchy, so /proc/iomem doesn't contain bus information.

(2) Walk over the bus claiming the resources that we find. Patch below.

Any thoughts?

commit 802062290eee961362d3090a50cf851412a63314

Author: Will Deacon <[email protected]>
Date:   Wed Feb 19 23:29:30 2014 +0000

ARM: bios32: claim firmware-initialised resources when PCI_PROBE_ONLY

When initialising a PCI bus with the PCI_PROBE_ONLY flag set, we don't (re-)assign any firmware-initialised resources, leading to an incomplete resource hierarchy and devices with orphaned resources.

If we try to enable these orphaned resources using pci_enable_resources, we will get back -EINVAL since a valid ->parent pointer is required. Consequently, pcibios_enable_device doesn't attempt to enable devices when PCI_PROBE_ONLY is set. This has the unfortunate effect of requiring the firmware to *enable* all of the devices!

This patch fixes the problem by building up a resource hierarchy from the firmware-initialised resources when PCI_PROBE_ONLY is set.

Signed-off-by Will Deacon <[email protected]>

arch/arm/kernel/bios32.c +31 -3

index 91f48804e3bb..d65c76a4d7ad 100644
@@ -514,6 +514,35 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
        }
 }
 
+static void pcibios_claim_one_bus(struct pci_bus *bus)
+{
+       struct pci_dev *dev;
+       struct pci_bus *child_bus;
+
+       list_for_each_entry(dev, &bus->devices, bus_list) {
+               int i;
+
+               for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+                       struct resource *r = &dev->resource[i];
+
+                       if (r->parent || !r->start || !r->flags)
+                               continue;
+
+                       pr_debug("PCI: Claiming %s: "
+                                "Resource %d: %016llx..%016llx [%x]\n",
+                                pci_name(dev), i,
+                                (unsigned long long)r->start,
+                                (unsigned long long)r->end,
+                                (unsigned int)r->flags);
+
+                       pci_claim_resource(dev, i);
+               }
+       }
+
+       list_for_each_entry(child_bus, &bus->children, node)
+               pcibios_claim_one_bus(child_bus);
+}
+
 void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 {
        struct pci_sys_data *sys;

@@ -541,6 +570,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
                         * Assign resources.
                         */
                        pci_bus_assign_resources(bus);
+               } else {
+                       pcibios_claim_one_bus(bus);
                }
 
                /*

@@ -608,9 +639,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-       if (pci_has_flag(PCI_PROBE_ONLY))
-               return 0;
-
        return pci_enable_resources(dev, mask);
 }
 

Quoted 9 lines

Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.

The problem is that bios32.c won't create a resource hierarchy for the firmware-initialised resources, so we have a bunch of orphaned resources that we can't pass to pci_enable_resources (since it checks r->parent).

This means that, unless firmware *enables* all of the resources, Linux won't be able to enable them. I think this is stronger than simply not re-assigning devices like the PCI_PROBE_ONLY flag intends.

By "firmware enabling resources," do you mean firmware assigning addresses in the BARs and turning on the PCI_COMMAND_MEMORY (or _IO) bits?

I'd like to make the generic code ignore BAR values if PCI_COMMAND_MEMORY (or _IO) is cleared. When those bits are cleared, I don't think there's a good way to determine whether the address in a BAR is valid.

Quoted 14 lines · Bjorn Helgaas

Quoted 9 lines

Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.

The problem is that bios32.c won't create a resource hierarchy for the firmware-initialised resources, so we have a bunch of orphaned resources that we can't pass to pci_enable_resources (since it checks r->parent).

This means that, unless firmware *enables* all of the resources, Linux won't be able to enable them. I think this is stronger than simply not re-assigning devices like the PCI_PROBE_ONLY flag intends.

By "firmware enabling resources," do you mean firmware assigning addresses in the BARs and turning on the PCI_COMMAND_MEMORY (or _IO) bits?

Quoted 4 lines

I'd like to make the generic code ignore BAR values if PCI_COMMAND_MEMORY (or _IO) is cleared. When those bits are cleared, I don't think there's a good way to determine whether the address in a BAR is valid.

I think this is pretty smart, PROBE_ONLY really should mean 'everything is perfect, do not touch it' and if the device isn't enabled, well.. It isn't enabled, the firmware should have done it.

Will, this if for kvmtool right? Keeping the patch as is and instead changing kvmtool to enable the devices seems like a good option?

drivers/pci/host/pci-host-generic.c new file +9 -0

+static int gen_pci_alloc_io_offset(u32 sz, resource_size_t *offset)
+{
+	static DECLARE_BITMAP(io_map, (IO_SPACE_LIMIT + 1) / SZ_64K);
+	int idx, num_wins;
+
+	if (sz > SZ_64K)
+		return -ENOSPC;
+
+	num_wins = (IO_SPACE_LIMIT + 1) / SZ_64K;

Quoted 15 lines · On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:

drivers/pci/host/pci-host-generic.c new file +15 -0

+static int gen_pci_alloc_io_offset(u32 sz, resource_size_t *offset)
+{
+	static DECLARE_BITMAP(io_map, (IO_SPACE_LIMIT + 1) / SZ_64K);
+	int idx, num_wins;
+
+	if (sz > SZ_64K)
+		return -ENOSPC;
+
+	num_wins = (IO_SPACE_LIMIT + 1) / SZ_64K;
+	idx = 0;
+	do {
+		idx = find_next_zero_bit(io_map, num_wins, idx);
+		if (idx == num_wins)
+			return -ENOSPC;
+	} while (test_and_set_bit(idx, io_map));

drivers/pci/host/pci-host-generic.c line 177

Quoted 4 lines

drivers/pci/host/pci-host-generic.c new file +4 -0

+
+	*offset = idx * SZ_64K;
+	return 0;
+}

Sicne you're always starting the search at 0 and you never free the map, this is essentially the same as remembering the last number that was used and using the next one, right?

You should also pass the rang->pci_addr here to calculate the offset in case the pci_addr is not zero.

drivers/pci/host/pci-host-generic.c line 304

Quoted 8 lines

drivers/pci/host/pci-host-generic.c new file +8 -0

+	/* Register our I/O and Memory resources */
+	res_valid = 0;
+	list_for_each_entry(win, &pci->host.windows, list) {
+		struct resource *parent;
+
+		if (resource_type(win->res) == IORESOURCE_IO) {
+			parent = &ioport_resource;
+			err = pci_ioremap_io(win->offset, win->res->start);

and consequently pass the pci_addr rather than the offset here. How about moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?

Quoted 12 lines · Arnd Bergmann

drivers/pci/host/pci-host-generic.c line 304

Quoted 8 lines · On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:

drivers/pci/host/pci-host-generic.c new file +8 -0

+	/* Register our I/O and Memory resources */
+	res_valid = 0;
+	list_for_each_entry(win, &pci->host.windows, list) {
+		struct resource *parent;
+
+		if (resource_type(win->res) == IORESOURCE_IO) {
+			parent = &ioport_resource;
+			err = pci_ioremap_io(win->offset, win->res->start);

and consequently pass the pci_addr rather than the offset here. How about moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?

Quoted 11 lines · Will Deacon

Quoted 10 lines · Arnd Bergmann

Quoted 8 lines · On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:

+ /* Register our I/O and Memory resources */
+ res_valid = 0;
+ list_for_each_entry(win, &pci->host.windows, list) {

drivers/pci/host/pci-host-generic.c new file +5 -0

+         struct resource *parent;
+
+         if (resource_type(win->res) == IORESOURCE_IO) {
+                 parent = &ioport_resource;
+                 err = pci_ioremap_io(win->offset, win->res->start);

Quoted 2 lines

Quoted 2 lines

and consequently pass the pci_addr rather than the offset here. How about moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?

I've probably just confused myself, but passing the pci_addr to pci_ioremap_io doesn't make sense to me.

My understanding is that:

cpu = bus + offset

In the case of I/O, the offset is really:

offset = (PCI_IO_VIRT_BASE - bus) + window

where window is determined by the simple allocator I wrote.

Now, the __io macro takes care of PCI_IO_VIRT_BASE so we don't actually care about that when adding the PCI I/O resources, instead we'll just pass:

offset = window - bus

and then pci_ioremap_io will just want the window offset, since that's added directly on to PCI_IO_VIRT_BASE for the ioremap_page_range.

If I call pci_ioremap_io(range->pci_addr, ...) then I'm going to trip a BUG_ON unless the pci_addr is within IO_SPACE_LIMIT.

Quoted 2 lines · On Wednesday 19 February 2014 11:07:19 Will Deacon wrote:

I've probably just confused myself, but passing the pci_addr to pci_ioremap_io doesn't make sense to me.

I think the confusion is that there are two different things we call offset here. The calculation of the offset you pass into pci_ioremap_io() is correct AFAICT now, but it's not what you are supposed to pass into pci_add_resource_offset() or what we normally put into pci_host_bridge_window->offset.

Quoted 3 lines

My understanding is that:

cpu = bus + offset

Right. This would be the case for mem_offset.

Quoted 3 lines

In the case of I/O, the offset is really:

offset = (PCI_IO_VIRT_BASE - bus) + window

I can't seem to make sense of this calculation. PCI_IO_VIRT_BASE is a pointer to the start of the virtual window. You can add offsets to the pointer, but subtracting a number from it is not a well-defined operation.

Quoted 1 line

where window is determined by the simple allocator I wrote.

And your allocator calls it offset, which is what confused me.

Quoted 4 lines

Now, the __io macro takes care of PCI_IO_VIRT_BASE so we don't actually care about that when adding the PCI I/O resources, instead we'll just pass:

offset = window - bus

Yes, this would be the io_offset that you pass into pci_add_resource_offset().

Quoted 2 lines

and then pci_ioremap_io will just want the window offset, since that's added directly on to PCI_IO_VIRT_BASE for the ioremap_page_range.

Yes.

Quoted 2 lines

If I call pci_ioremap_io(range->pci_addr, ...) then I'm going to trip a BUG_ON unless the pci_addr is within IO_SPACE_LIMIT.

range->pci_addr is what you call 'bus' above. Since you want 'window', you have to pass 'bus'+'offset', or 'range->pci_addr + io_offset'. Normally, one of the two would be zero, while the other is equal to 'window'.

Documentation/devicetree/bindings/pci/host-generic-pci.txt line 74

Quoted 3 lines · Will Deacon

Documentation/devicetree/bindings/pci/host-generic-pci.txt new file +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Quoted 8 lines · Jason Gunthorpe

Documentation/devicetree/bindings/pci/host-generic-pci.txt line 74

Quoted 3 lines · Will Deacon

Documentation/devicetree/bindings/pci/host-generic-pci.txt new file +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since there's an 8250 down there. That means we have:

0x0 - 0x6200 : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Quoted 13 lines · On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:

Quoted 8 lines · Jason Gunthorpe

Documentation/devicetree/bindings/pci/host-generic-pci.txt line 74

Quoted 3 lines · Will Deacon

Documentation/devicetree/bindings/pci/host-generic-pci.txt new file +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since there's an 8250 down there. That means we have:

Quoted 4 lines

0x0 - 0x6200 : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Do you have a list of the stuff in there?

If that is actually port I/O, leaving it in there may be best, but I agree the example should be changed.

kvmtool provides an i8042, pci-shmem, an rtc, 8250 and vesa at the usual PC offsets. On ARM, I regularly use the 8250 (there are even DT bindings for it) and MarcZ got the rtc working for fun. Not tried the others though.

Quoted 4 lines · On Tuesday 18 February 2014 18:51:50 Will Deacon wrote:

kvmtool provides an i8042, pci-shmem, an rtc, 8250 and vesa at the usual PC offsets. On ARM, I regularly use the 8250 (there are even DT bindings for it) and MarcZ got the rtc working for fun. Not tried the others though.

Ah, interesting. But which ones are between 0x1000-0x6200? The ones you list should all be in the ISA range between 0x0-0xfff as far as I can tell, which we keep clear of by setting PCIBIOS_MIN_IO to 0x1000.

Quoted 8 lines · Jason Gunthorpe

Documentation/devicetree/bindings/pci/host-generic-pci.txt line 74

Quoted 3 lines · Will Deacon

Documentation/devicetree/bindings/pci/host-generic-pci.txt new file +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since there's an 8250 down there. That means we have:

0x0 - 0x6200 : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Quoted 13 lines · Will Deacon

Quoted 8 lines · Jason Gunthorpe

Documentation/devicetree/bindings/pci/host-generic-pci.txt line 74

Quoted 3 lines · Will Deacon

Documentation/devicetree/bindings/pci/host-generic-pci.txt new file +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since there's an 8250 down there. That means we have:

Quoted 4 lines

0x0 - 0x6200 : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Just to be clear, kvm has reserved the first 64k of physical address space for this IO window? The 0 is fine then, but it isn't typical of real hardware.

Regarding the 0x6200.. There are two conflicting issues there :(
- You really don't want to let the PCI core assign resources to that range, it probably won't work.

- You probably need that range mapped into the ARM IO window, and the PCI driver is the thing doing that

The PCI core already has a black out for low IO ports, I think it already covers the usual PC suspects so you are probably fine with what you have for KVM.

Quoted 4 lines

Regarding the 0x6200.. There are two conflicting issues there
- You really don't want to let the PCI core assign resources to that range, it probably won't work.

Right, with kvmtool we don't support resource assignment (the BARs are fixed) so everything is PCI_PROBE_ONLY.

Quoted 7 lines · On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:

Quoted 4 lines

Regarding the 0x6200.. There are two conflicting issues there
- You really don't want to let the PCI core assign resources to that range, it probably won't work.

Right, with kvmtool we don't support resource assignment (the BARs are fixed) so everything is PCI_PROBE_ONLY.

Ok, I looked at the source now and can confirm:

* 0x0-0x1000 are used for lots of legacy ISA devices.
* PCI devices get assigned IO addresses in 0x400 steps starting at 0x6200.
* There are three PCI drivers doing this: VESA, PCI-SHMEM and virtio-pci.

Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we have a standard DT property for that? On PowerPC we already specified "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems reasonable to use in architecture independent code as well.

Quoted 15 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:

Quoted 4 lines

Regarding the 0x6200.. There are two conflicting issues there
- You really don't want to let the PCI core assign resources to that range, it probably won't work.

Right, with kvmtool we don't support resource assignment (the BARs are fixed) so everything is PCI_PROBE_ONLY.

Ok, I looked at the source now and can confirm:

* 0x0-0x1000 are used for lots of legacy ISA devices.
* PCI devices get assigned IO addresses in 0x400 steps starting at 0x6200.
* There are three PCI drivers doing this: VESA, PCI-SHMEM and virtio-pci.

Quoted 4 lines

Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we have a standard DT property for that? On PowerPC we already specified "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems reasonable to use in architecture independent code as well.

For arch/arm/ it's done on the command line (pci=firmware) but yes, for the generic driver it sounds like a good idea to put this in the device-tree. I'll look at adding the ppc properties in the next version.

Quoted 10 lines · Will Deacon

Quoted 5 lines · Arnd Bergmann

On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we have a standard DT property for that? On PowerPC we already specified "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems reasonable to use in architecture independent code as well.

For arch/arm/ it's done on the command line (pci=firmware) but yes, for the generic driver it sounds like a good idea to put this in the device-tree. I'll look at adding the ppc properties in the next version.

Damn, I spoke too soon. PowerPC puts this information in the /chosen node, so it applies to all PCI host controllers in the system. That then maps nicely to pci_{add,clear}_flags which are global properties in Linux.

It sounds like this should really be per-controller. What do you think?

Quoted 12 lines · On Tuesday 18 February 2014 19:57:32 Will Deacon wrote:

Quoted 10 lines · Will Deacon

Quoted 5 lines · Arnd Bergmann

On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we have a standard DT property for that? On PowerPC we already specified "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems reasonable to use in architecture independent code as well.

For arch/arm/ it's done on the command line (pci=firmware) but yes, for the generic driver it sounds like a good idea to put this in the device-tree. I'll look at adding the ppc properties in the next version.

Quoted 5 lines

Damn, I spoke too soon. PowerPC puts this information in the /chosen node, so it applies to all PCI host controllers in the system. That then maps nicely to pci_{add,clear}_flags which are global properties in Linux.

It sounds like this should really be per-controller. What do you think?

I don't have a strong opinion either way. I think the reason for having it in the /chosen node is so that a second-stage boot loader can easily override it, which is not a concern for us. In particular, the properties are only used on rtas-pci, which means we access the config space to firmware runtime abstraction services rather than hardware registers.

Apparently the embedded powerpc platforms don't support these flags at all, which I also didn't notice at first.

Quoted 8 lines · Jason Gunthorpe

Review

Quoted 3 lines · Will Deacon

(quoted diff) +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since there's an 8250 down there. That means we have:

0x0 - 0x6200 : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Quoted 10 lines · On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:

Quoted 8 lines · Jason Gunthorpe

Review

Quoted 3 lines · Will Deacon

(quoted diff) +3 -0

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool output. For example purposes any value will do.

Quoted 7 lines

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since there's an 8250 down there. That means we have:

0x0 - 0x6200 : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Some more comments about this:

* After I looked at the kvmtool code some more, I think it would be nice to actually move the I/O space window to a nonzero location, e.g. between the PCI space and the AXI space. It's really confusing the way it is, and easy to introduce bugs if you have any code that accidentally relies on the numbers matching up. For all I can tell, kvmtool should just work fine with any definition of ARM_IOPORT_AREA, as long as you apply the trivial patch below.

* pci_get_io_space_block() is a very confusing name for a function that allocates a memory space block in kvmtool. It seems that KVM_IOPORT_AREA is a misnomer for the same reason. Note that PowerPC sets KVM_IOPORT_AREA to zero, but has the I/O space window at SPAPR_PCI_IO_WIN_ADDR, which is non-zero.

* It's interesting that you also support I/O space access to the PCI config space through PC-Style ports 0cf8-0cff. Not sure if that helps at all, but it is another standard way to probe the bus that could easily be implemented as an alternative to CAM and ECAM, in a generic PCI host driver.