Hi Arnd,
On Tue, Feb 25, 2014 at 10:30:33PM +0000, Arnd Bergmann wrote:
> On Tuesday 25 February 2014, Will Deacon wrote:
> > I need the alignment code so I can handle being passed a CPU physical
> > address which isn't 64K aligned. In this case, pci_ioremap_io needs a 64K
> > aligned address so that the __io macro does the right thing.
>
> I still don't see why pci_ioremap_io needs 64K alignment. For all I can tell,
> it just needs a page-aligned address.
I misread __io as an '|' rather than a '+'. You're right, we just need page
alignment for the mapping.
> > For example, I can tell kvmtool to place I/O space at CPU physical 0x16000,
> > bus address 0x6000 (so ports 0x0-0x5fff are unavailable). In that case, I
> > still need to map those lower ports for the __io macro to offset into the
> > window correctly.
>
> I don't think so. pci_ioremap_io will still try to map 64K of address space,
> but you can map 0x16000-0x25fff to PCI_IO_VIRT_BASE. The io_offset in
> that case will be negative (-0x6000).
Yes, and that means that all this rounding is unnecessary; I can just fail
addresses that aren't page-aligned (which is much more reasonable).
> > of_pci_range_to_resource(range, dev->of_node, res);
> > res->start = window + range->pci_addr;
> > res->end = res->start + range->size - 1;
> > *offset = window - range->pci_addr;
> > return 0;
> > }
>
> The offset is good now, but the start is not: res->start refers to
> the linux I/O space and should just be 'window' here. Adding up
> window and pci_addr makes no sense. Think of the case where you
> have two host bridges, one using bus address 0x0-0xffff and the other
> one using 0x10000-0x1ffff. You really want the first one to
> have res->start=0 and the second one res->start=0x10000, and
> using offset=0. Instead the second one get start=0x20000,
> which is wrong in both CPU and bus space.
Aha, so res->start is the offset into our virtual I/O space at which the
window starts? That's what I've been getting wrong -- I thought res->start
was a PCI bus address for some reason.
> The round_down() also still gets you into trouble here, because you
> don't adjust the start or the size.
I'll just get rid of the rounding.
> > If I do:
> >
> > *offset = window;
> >
> > then things work as expected (this is all referring back to the kvmtool example
> > I mentioned earlier in this mail).
>
> Yes, but only only because you have two bugs that cancel each other out
> to some degree. It only works for the first bus though, as explained
> above: if offset is >0, the Linux I/O port number that gets assigned
> in the resource is no longer the one that maps to the one in your
> virtual address space pointing to the right physical address.
Ok, so taking all this on board I end up with the diff below (against v4).
It seems to work well with my kvmtool setup:
pci_bus 0000:00: root bus resource [io 0x0000-0x9fff] (bus address [0x6000-0xffff])
Will
--->8
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 71b0960772ec..b761f3e64a6a 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -133,30 +133,25 @@ static int gen_pci_calc_io_offset(struct device *dev,
{
static atomic_t wins = ATOMIC_INIT(0);
int err, idx, max_win;
- unsigned int io_offset;
+ unsigned int window;
- /* Ensure the CPU physical address is aligned to a 64K boundary */
- range->size += range->cpu_addr & (SZ_64K - 1);
- range->cpu_addr = round_down(range->cpu_addr, SZ_64K);
- range->pci_addr = round_down(range->pci_addr, SZ_64K);
-
- if (range->size > SZ_64K)
- return -E2BIG;
+ if (!PAGE_ALIGNED(range->cpu_addr))
+ return -EINVAL;
- max_win = IO_SPACE_LIMIT + 1 / SZ_64K;
+ max_win = (IO_SPACE_LIMIT + 1) / SZ_64K;
idx = atomic_inc_return(&wins);
if (idx >= max_win)
return -ENOSPC;
- io_offset = (idx - 1) * SZ_64K;
- err = pci_ioremap_io(io_offset, range->cpu_addr);
+ window = (idx - 1) * SZ_64K;
+ err = pci_ioremap_io(window, range->cpu_addr);
if (err)
return err;
of_pci_range_to_resource(range, dev->of_node, res);
- res->start = io_offset + range->pci_addr;
+ res->start = window;
res->end = res->start + range->size - 1;
- *offset = io_offset;
+ *offset = window - range->pci_addr;
return 0;
}
--
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
