Re: [PATCH v2] PCI: ACPI: IA64: fix IO port generic range check

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Mon, Mar 14, 2016 at 02:27:08PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 14, 2016 at 02:53:22PM +0000, Lorenzo Pieralisi wrote:

[...]

> > Even if we do that, call to acpi_pci_root_validate_resources() for
> > IO space is wrong on IA64, we are comparing an IO resource against
> > ioport_resource there, but the Linux IO port space for the host
> > bridge is created in IA64 pci_acpi_root_prepare_resources() with
> > the call to add_io_space() which happens *after* the sequence above
> > is executed.
> > 
> > On IA64:
> > 
> > pci_acpi_root_prepare_resources()
> >  -> acpi_pci_probe_root_resources()
> >     -> acpi_dev_get_resources()
> >     -> acpi_pci_root_validate_resources()
> >  -> add_io_space() # this is where the Linux IO port space for the bridge is
> >                      created and that's when we can validate the IO
> > 		     resource against ioport_resource
> > 
> > I have no experience/knowledge of IA64 systems so I may be totally wrong,
> > I just want to understand.
> > 
> > Comments welcome, Hanjun proved my understanding by testing on IA64 and
> > current mainline just does not work (see commit log for failure messages),
> > feedback from IA64 people is really needed here, we have to get this fixed
> > please (and through that fix, getting it to work on ARM64 too).
> 
> I/O port translations makes my head hurt, but I think you're right.
> 
> The raw I/O resources from ACPI _CRS are definitely different from the
> Linux ioport spaces constructed by add_io_space(), so we shouldn't
> compare the two.
> 
> If we need to validate the raw I/O ports from _CRS, I suppose in
> theory we should be able to check that they're contained in the raw
> I/O port range of an upstream window.
> 
> I think the problem is that 3772aea7d6f3 started applying the
> x86-specific "[0-0xffff]" restriction to all arches, so if you want to 
> back that out so it applies only on x86 (but to all devices, not just
> PNP0A03), I think that would make sense.

I noticed there is already an X86 specific check in:

drivers/acpi/resource.c (ie valid_IRQ)

I can add something like code below there and be done with it:

#ifdef CONFIG_X86
static inline bool io_space_valid(struct resource *res)
{
	return res->end < 0x10003;
}
#else
static inline bool io_space_valid(struct resource *res) { return true; }
#endif

if Rafael is ok with that. Whatever I do requires an arch specific hook
(empty/always-true on !CONFIG_X86) to be called from
acpi_dev_ioresource_flags(), otherwise removing that check really becomes
a minefield.

Other solution is reverting back to not using acpi_dev_get_resources()
on IA64 and ARM64 which defeats the whole purpose of Jiang's consolidation,
so I won't go there.

Next step is removing (or refactoring) acpi_pci_root_validate_resources()
for IORESOURCE_IO from acpi_pci_probe_root_resources(), it is a bogus check
as our discussion above highlighted and does not work on ARM64 (it
probably does on IA64 since IO_SPACE_LIMIT == 0xffffffffffffffffUL, but
that's conceptually wrong regardless).


Thanks,
Lorenzo