On Wed, Feb 26, 2014 at 05:46:59PM +0000, Will Deacon wrote:
> 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.
No, res->start should be start of logical I/O space. I will send today the
patch that fixes the of_pci_range_to_resource() call and then you should
no longer have to worry about offsets (other than keeping a logical io_offset
in the bridge to account for multiple host bridges being active).
>
> > 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
>
--
-------------------
.oooO
( )
\ ( Oooo.
\_) ( )
) /
(_/
One small step
for me ...
--
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
