Date:	Wed, 11 Nov 2015 18:12:07 +0000
From:	Liviu Dudau <Liviu.Dudau@....com>
To:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:	Jiang Liu <jiang.liu@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>,
	Tomasz Nowicki <tn@...ihalf.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
	Marc Zyngier <marc.zyngier@....com>,
	Hanjun Guo <hanjun.guo@...aro.org>, linux-acpi@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [Patch v7 4/7] PCI/ACPI: Add interface acpi_pci_root_create()

On Wed, Nov 11, 2015 at 05:46:47PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
> 
> [...]
> 
> > >> In particular, I would like to understand, for an eg DWordIO descriptor,
> > >> what Range Minimum, Range Maximum and Translation Offset represent,
> > >> they can't mean different things depending on the SW parsing them,
> > >> this totally defeats the purpose.
> > > 
> > > I have no clue about what those mean in ACPI though.
> > > 
> > > Generally speaking, each PCI domain is expected to have a (normally 64KB)
> > > range of CPU addresses that gets translated into PCI I/O space the same
> > > way that config space and memory space are handled.
> > > This is true for almost every architecture except for x86, which uses
> > > different CPU instructions for I/O space compared to the other spaces.
> > > 
> > >> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
> > >> basically that's the CPU physical address at which the PCI host bridge
> > >> map the IO space transactions), I do not think ia64 is any different from
> > >> arm64 in this respect, if it is please provide an HW description here from
> > >> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
> > >> tables would help).
> > > 
> > > The main difference between ia64 and a lot of the other architectures (e.g.
> > > sparc is different again) is that ia64 defines a logical address range
> > > in terms of having a small number for each I/O space followed by the
> > > offset within that space as a 'port number' and uses a mapping function
> > > that is defined as
> > > 
> > > static inline void *__ia64_mk_io_addr (unsigned long port)
> > > {
> > >         struct io_space *space = &io_space[IO_SPACE_NR(port)];
> > >         return (space->mmio_base | IO_SPACE_PORT(port););
> > > }
> > > static inline unsigned int inl(unsigned long port)
> > > {
> > >         return *__ia64_mk_io_addr(port);
> > > }
> > > 
> > > Most architectures allow only one I/O port range and put it at a fixed
> > > virtual address so that inl() simply becomes 
> > > 
> > > static inline u32 inl(unsigned long addr)
> > > {
> > >         return readl(PCI_IOBASE + addr);
> > > }
> > > 
> > > which noticeably reduces code size.
> > > 
> > > On some architectures (powerpc, arm, arm64), we then get the same simplified
> > > definition with a fixed virtual address, and use pci_ioremap_io() or
> > > something like that to to map a physical address range into this virtual
> > > address window at the correct io_offset;
> > Hi all,
> > 	Thanks for explanation, I found a way to make the ACPI resource
> > parsing interface arch neutral, it should help to address Lorenzo's
> > concern. Please refer to the attached patch. (It's still RFC, not tested
> > yet).
> 
> If we go with this approach though, you are not adding the offset to
> the resource when parsing the memory spaces in acpi_decode_space(), are we
> sure that's what we really want ?
> 
> In DT, a host bridge range has a:
> 
> - CPU physical address
> - PCI bus address
> 
> We use that to compute the offset between primary bus (ie CPU physical
> address) and secondary bus (ie PCI bus address).
> 
> The value ending up in the PCI resource struct (for memory space) is
> the CPU physical address, if you do not add the offset in acpi_decode_space
> that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
> am I wrong ?
> 
> Overall I think the point is related to ioport_resource and its check
> in acpi_pci_root_validate_resources() which basically that's the
> problem that started this thread.
> 
> On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not
> a HW one. 

You're right, it is a kernel limit. There is no HW limit for IO on arm64.

> Comparing the resources parsed from the PCI bridge _CRS against
> the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least
> not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT
> is bumped up to 4G, that's the reason why adding the offset to the ACPI IO
> resources work on ia64 as far as I understand.
> 
> And that's why I pulled Arnd in this discussion since he knows better
> than me: what does ioport_resource _really_ represent on ARM64 ? It seems
> to me that it is a range of IO ports values (ie a window that defines
> the allowed offset in the virtual address space allocated to PCI IO) that
> has _nothing_ to do with the CPU physical address at which the IO space is
> actually mapped.

Correct. The idea is that you can have any number of host bridges and what
you get back for an ioport_resource is a window inside the host bridge IO
space. That space is also offset-ed inside the kernel's IO port space
(0..IO_SPACE_LIMIT) by the amount of space already taken by preceeding
host bridges, so that two ioport_resources comming from two different
host bridges don't overlap in the CPU address space.

Best regards,
Liviu


> 
> To sum it up for a, say, DWordIo/Memory descriptor:
> 
> - AddressMinimum, AddressMaximum represent the PCI bus addresses defining
>   the resource start..end
> - AddressTranslation is the offset that has to be added to AddressMinimum
>   and AddressMaximum to get the window in CPU physical address space
> 
> So:
> 
> - Either we go with the patch attached (but please check my comment on
>   the memory spaces)
> - Or we patch acpi_pci_root_validate_resources() to amend the way
>   IORESOURCE_IO is currently checked against ioport_resource, it can't
>   work on arm64 at present, I described why above
> 
> Thoughts appreciated it is time we got this sorted and thanks for
> the patch.
> 
> Thanks,
> Lorenzo
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists