Re: [Patch v7 4/7] PCI/ACPI: Add interface acpi_pci_root_create()

[Tomasz Nowicki]

On 12.11.2015 16:05, Jiang Liu wrote:
> On 2015/11/12 22:45, Tomasz Nowicki wrote:
> > On 12.11.2015 15:04, Jiang Liu wrote:
> > > On 2015/11/12 21:21, Tomasz Nowicki wrote:
> > > > On 12.11.2015 09:43, Jiang Liu wrote:
> > > > > On 2015/11/12 1:46, 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 ?
> > > > > Hi Lorenzo,
> > > > >       I may have found the divergence between us about the design
> > > > > here. You
> > > > > treat it as a one-stage translation but I treat it as a
> > > > > two-stage translation as below:
> > > > > stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into
> > > > > system global IO port address. Here system global IO port address is
> > > > > ioport_resource[0, IO_SPACE_LIMIT).
> > > > > stage 2: map system IO port address into system memory address.
> > > > >
> > > > > We need two objects of struct resource_win to support above two-stage
> > > > > translation. One object, type of IORESOURCE_IO, is used to support
> > > > > stage one, and it will also used to allocate IO port resources
> > > > > for PCI devices. Another object, type of IORESOURCE_MMIO, is used
> > > > > to allocate resource from iomem_resource and setup MMIO mapping
> > > > > to actually access IO ports.
> > > > >
> > > > > For ARM64, it doesn't support multiple per-PCI-domain(bus local)
> > > > > IO port address space yet, so stage one seems to be optional
> > > > > becomes the offset between bus local IO port address and system
> > > > > IO port address is always 0. But we still need two objects of
> > > > > struct resource_win. The first object is
> > > > >       {
> > > > >           offset:0,
> > > > >           start:AddressMinimum,
> > > > >           end:AddressMaximum,
> > > > >           flags:IORESOURCE_IO
> > > > >       }
> > > > > Here it's type of IORESOURCE_IO and offset must be zero because
> > > > > pcibios_resource_to_bus() will access it translate system IO
> > > > > port address into bus local IO port address. With my patch,
> > > > > the struct resource_win object created by the ACPI core will
> > > > > be reused for this.
> > > > >
> > > > > The second object is:
> > > > >       {
> > > > >           offset:Translation_Offset,
> > > > >           start:AddressMinimum + Translation_Offset,
> > > > >           end:AddressMaximum + Translation_Offset,
> > > > >           flags:IORESOURCE_MMIO
> > > > >       }
> > > > > Arch code need to create the second struct resource_win object
> > > > > and actually setup the MMIO mapping.
> > > > >
> > > > > But there's really another bug need to get fixed, funciton
> > > > > acpi_dev_ioresource_flags() assumes bus local IO port address
> > > > > space is size of 64K, which is wrong for IA64 and ARM64.
> > > >
> > > > So what would be the Translation_Offset meaning for two cases DWordIo
> > > > (....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not
> > > > use TypeTranslation for IA64 so far?
> > >
> > > IA64 actually ignores the translation type flag and just assume it's
> > > TypeTranslation, so there may be some IA64 BIOS implementations
> > > accidentally using TypeStatic. That's why we parsing SparseTranslation
> > > flag without checking TranslationType flag. I feel ARM64 may face the
> > > same situation as IA64:(
> > >
> > > We may expect (TypeStatic, 0-offset) and (TypeTranslation,
> > > non-0-offset) in real word. For other two combinations, I haven't
> > > found a real usage yet, though theoretically they are possible.
> >
> > I think we should not bend the generic code for IA64 only and expose
> > other platforms to the same issue. Instead, lets interpret spec
> > correctly and create IA64 quirk for the sake of backward compatibility.
> > Thoughts?
> I think there are at least two factors related to this issue.
>
> First we still lack of a way/framework to fix errors in ACPI resource
> descriptors. Recently we have refined ACPI resource parsing interfaces
> and enforced strictly sanity check. This brings us some regressions
> which are really BIOS flaws, but it used to work and now breaks:(
> I'm still struggling to get those regressions fixed. So we may run
> into the same situation if we enforce strict check for TranslationType:(
>
> Second enforcing strict check doesn't bring us too much benifits.
> Translation type is almost platform specific, and we haven't found a
> platform support both TypeTranslation and TypeStatic, so arch code
> may assume the correct translation type no matter what BIOS reports.
> So it won't hurt us even BIOS reports wrong translation type.

That is my point, lets pass down all we need from resource range 
descriptors to arch code, then archs with known quirks can whatever is 
needed to make it works. However, generic code like acpi_decode_space 
cannot play with offsets with silent IA64 assumption.

To sum it up, your last patch looks ok to me modulo Lorenzo's concern:
>>>>>> 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 ?
His concern is that your patch will cause:
acpi_pci_root_validate_resources(&device->dev, list,
				 IORESOURCE_MEM);
to fail now.

Regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/