diff mbox

[V3,18/21] ACPI, PCI: Refine the way to handle translation_offset for ACPI resources

Message ID 1452691267-32240-19-git-send-email-tn@semihalf.com
State Superseded
Headers show

Commit Message

Tomasz Nowicki Jan. 13, 2016, 1:21 p.m. UTC
From: Liu Jiang <jiang.liu@linux.intel.com>

Some architectures, such as IA64 and ARM64, have no instructions to
directly access PCI IO ports, so they map PCI IO ports into PCI MMIO
address space. Typically PCI host bridges on those architectures take
the responsibility to map (translate) PCI IO port transactions into
Memory-Mapped IO transactions. ACPI specification provides support
of such a usage case by using resource translation_offset.

But current ACPI resource parsing interface isn't neutral enough,
it still has some special logic for IA64. So refine the ACPI resource
parsing interface and IA64 code to neutrally handle translation_offset
by:
1) ACPI resource parsing interface doesn't do any translation, it just
   save the translation_offset to be used by arch code.
2) Arch code will do the mapping(translation) based on arch specific
   information. Typically it does:
2.a) Translate per PCI domain IO port address space into system global
   IO port address space.
2.b) Setup MMIO address mapping for IO ports.
void handle_io_resource(struct resource_entry *io_entry)
{
	struct resource *mmio_res;

	mmio_res = kzalloc(sizeof(*mmio_res), GFP_KERNEL);
	mmio_res->flags = IORESOURCE_MEM;
	mmio_res->start = io_entry->offset + io_entry->res->start;
	mmio_res->end = io_entry->offset + io_entry->res->end;
	insert_resource(&iomem_resource, mmio_res)

	base = map_to_system_ioport_address(entry);
	io_entry->offset = base;
	io_entry->res->start += base;
	io_entry->res->end += base;
}

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/ia64/pci/pci.c     | 26 ++++++++++++++++----------
 drivers/acpi/resource.c | 12 +++++-------
 2 files changed, 21 insertions(+), 17 deletions(-)

Comments

Lorenzo Pieralisi Jan. 14, 2016, 12:13 p.m. UTC | #1
Gerry, Tomasz,

On Wed, Jan 13, 2016 at 02:21:04PM +0100, Tomasz Nowicki wrote:
> From: Liu Jiang <jiang.liu@linux.intel.com>
> 
> Some architectures, such as IA64 and ARM64, have no instructions to
> directly access PCI IO ports, so they map PCI IO ports into PCI MMIO
> address space. Typically PCI host bridges on those architectures take
> the responsibility to map (translate) PCI IO port transactions into
> Memory-Mapped IO transactions. ACPI specification provides support
> of such a usage case by using resource translation_offset.
> 
> But current ACPI resource parsing interface isn't neutral enough,
> it still has some special logic for IA64. So refine the ACPI resource
> parsing interface and IA64 code to neutrally handle translation_offset
> by:
> 1) ACPI resource parsing interface doesn't do any translation, it just
>    save the translation_offset to be used by arch code.
> 2) Arch code will do the mapping(translation) based on arch specific
>    information. Typically it does:
> 2.a) Translate per PCI domain IO port address space into system global
>    IO port address space.
> 2.b) Setup MMIO address mapping for IO ports.
> void handle_io_resource(struct resource_entry *io_entry)
> {
> 	struct resource *mmio_res;
> 
> 	mmio_res = kzalloc(sizeof(*mmio_res), GFP_KERNEL);
> 	mmio_res->flags = IORESOURCE_MEM;
> 	mmio_res->start = io_entry->offset + io_entry->res->start;
> 	mmio_res->end = io_entry->offset + io_entry->res->end;
> 	insert_resource(&iomem_resource, mmio_res)
> 
> 	base = map_to_system_ioport_address(entry);
> 	io_entry->offset = base;
> 	io_entry->res->start += base;
> 	io_entry->res->end += base;
> }
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  arch/ia64/pci/pci.c     | 26 ++++++++++++++++----------
>  drivers/acpi/resource.c | 12 +++++-------
>  2 files changed, 21 insertions(+), 17 deletions(-)

I still do not understand what TranslationType argument means in the
ACPI specifications Resource descriptors (if you do not check it in
generic code that info is not propagated to arches through the resources
so basically we ignore it - I do not think we can do anything else given
that on ia64 it is missing from ACPI tables), but this patch makes sense
to me (it needs testing/reviewing for ia64 though):

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index be4c9ef..c75356b 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -154,7 +154,7 @@ static int add_io_space(struct device *dev, struct pci_root_info *info,
>  	struct resource_entry *iospace;
>  	struct resource *resource, *res = entry->res;
>  	char *name;
> -	unsigned long base, min, max, base_port;
> +	unsigned long base_mmio, base_port;
>  	unsigned int sparse = 0, space_nr, len;
>  
>  	len = strlen(info->common.name) + 32;
> @@ -172,12 +172,10 @@ static int add_io_space(struct device *dev, struct pci_root_info *info,
>  		goto free_resource;
>  
>  	name = (char *)(iospace + 1);
> -	min = res->start - entry->offset;
> -	max = res->end - entry->offset;
> -	base = __pa(io_space[space_nr].mmio_base);
> +	base_mmio = __pa(io_space[space_nr].mmio_base);
>  	base_port = IO_SPACE_BASE(space_nr);
>  	snprintf(name, len, "%s I/O Ports %08lx-%08lx", info->common.name,
> -		 base_port + min, base_port + max);
> +		 base_port + res->start, base_port + res->end);
>  
>  	/*
>  	 * The SDM guarantees the legacy 0-64K space is sparse, but if the
> @@ -190,19 +188,27 @@ static int add_io_space(struct device *dev, struct pci_root_info *info,
>  	resource = iospace->res;
>  	resource->name  = name;
>  	resource->flags = IORESOURCE_MEM;
> -	resource->start = base + (sparse ? IO_SPACE_SPARSE_ENCODING(min) : min);
> -	resource->end   = base + (sparse ? IO_SPACE_SPARSE_ENCODING(max) : max);
> +	resource->start = base_mmio;
> +	resource->end = base_mmio;
> +	if (sparse) {
> +		resource->start += IO_SPACE_SPARSE_ENCODING(res->start);
> +		resource->end += IO_SPACE_SPARSE_ENCODING(res->end);
> +	} else {
> +		resource->start += res->start;
> +		resource->end += res->end;
> +	}
>  	if (insert_resource(&iomem_resource, resource)) {
>  		dev_err(dev,
>  			"can't allocate host bridge io space resource  %pR\n",
>  			resource);
>  		goto free_resource;
>  	}
> +	resource_list_add_tail(iospace, &info->io_resources);
>  
> +	/* Adjust base of original IO port resource descriptor */
>  	entry->offset = base_port;
> -	res->start = min + base_port;
> -	res->end = max + base_port;
> -	resource_list_add_tail(iospace, &info->io_resources);
> +	res->start += base_port;
> +	res->end += base_port;
>  
>  	return 0;
>  
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index cdc5c25..6578f68 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -190,8 +190,7 @@ static bool acpi_decode_space(struct resource_win *win,
>  {
>  	u8 iodec = attr->granularity == 0xfff ? ACPI_DECODE_10 : ACPI_DECODE_16;
>  	bool wp = addr->info.mem.write_protect;
> -	u64 len = attr->address_length;
> -	u64 start, end, offset = 0;
> +	u64 len = attr->address_length, offset = 0;
>  	struct resource *res = &win->res;
>  
>  	/*
> @@ -215,14 +214,13 @@ static bool acpi_decode_space(struct resource_win *win,
>  	else if (attr->translation_offset)
>  		pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",
>  			 attr->translation_offset);
> -	start = attr->minimum + offset;
> -	end = attr->maximum + offset;
>  
>  	win->offset = offset;
> -	res->start = start;
> -	res->end = end;
> +	res->start = attr->minimum;
> +	res->end = attr->maximum;
>  	if (sizeof(resource_size_t) < sizeof(u64) &&
> -	    (offset != win->offset || start != res->start || end != res->end)) {
> +	    (offset != win->offset || attr->minimum != res->start ||
> +	     attr->maximum != res->end)) {
>  		pr_warn("acpi resource window ([%#llx-%#llx] ignored, not CPU addressable)\n",
>  			attr->minimum, attr->maximum);
>  		return false;
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Jan. 19, 2016, 12:20 p.m. UTC | #2
Gerry,

On Wed, Jan 13, 2016 at 02:21:04PM +0100, Tomasz Nowicki wrote:
> From: Liu Jiang <jiang.liu@linux.intel.com>
> 
> Some architectures, such as IA64 and ARM64, have no instructions to
> directly access PCI IO ports, so they map PCI IO ports into PCI MMIO
> address space. Typically PCI host bridges on those architectures take
> the responsibility to map (translate) PCI IO port transactions into
> Memory-Mapped IO transactions. ACPI specification provides support
> of such a usage case by using resource translation_offset.
> 
> But current ACPI resource parsing interface isn't neutral enough,
> it still has some special logic for IA64. So refine the ACPI resource
> parsing interface and IA64 code to neutrally handle translation_offset
> by:
> 1) ACPI resource parsing interface doesn't do any translation, it just
>    save the translation_offset to be used by arch code.
> 2) Arch code will do the mapping(translation) based on arch specific
>    information. Typically it does:
> 2.a) Translate per PCI domain IO port address space into system global
>    IO port address space.
> 2.b) Setup MMIO address mapping for IO ports.

This patch fixes IO space handling on IA64 and should go in as a fix.

IA64 PCI IO space is currently broken (Hanjun tested this on an IA64 box).

The first broken commit is:

3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing interface for host bridge")

because acpi core code checks (in acpi_dev_ioresource_flags()) the
resource.end>=0x10003, which fails on ia64 - currently resource.end is
set in acpi_decode_space() to:

AddressMaximum + AddressTranslation

where AddressTranslation is the CPU physical address mapping IO space
on IA64, the >=0x10003 check in acpi_dev_ioresource_flags always
triggers and the IO resource is then disabled.

Do you want me to re-send this patch as a fix, with updated commit log ?

Thanks,
Lorenzo

> void handle_io_resource(struct resource_entry *io_entry)
> {
> 	struct resource *mmio_res;
> 
> 	mmio_res = kzalloc(sizeof(*mmio_res), GFP_KERNEL);
> 	mmio_res->flags = IORESOURCE_MEM;
> 	mmio_res->start = io_entry->offset + io_entry->res->start;
> 	mmio_res->end = io_entry->offset + io_entry->res->end;
> 	insert_resource(&iomem_resource, mmio_res)
> 
> 	base = map_to_system_ioport_address(entry);
> 	io_entry->offset = base;
> 	io_entry->res->start += base;
> 	io_entry->res->end += base;
> }
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  arch/ia64/pci/pci.c     | 26 ++++++++++++++++----------
>  drivers/acpi/resource.c | 12 +++++-------
>  2 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index be4c9ef..c75356b 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -154,7 +154,7 @@ static int add_io_space(struct device *dev, struct pci_root_info *info,
>  	struct resource_entry *iospace;
>  	struct resource *resource, *res = entry->res;
>  	char *name;
> -	unsigned long base, min, max, base_port;
> +	unsigned long base_mmio, base_port;
>  	unsigned int sparse = 0, space_nr, len;
>  
>  	len = strlen(info->common.name) + 32;
> @@ -172,12 +172,10 @@ static int add_io_space(struct device *dev, struct pci_root_info *info,
>  		goto free_resource;
>  
>  	name = (char *)(iospace + 1);
> -	min = res->start - entry->offset;
> -	max = res->end - entry->offset;
> -	base = __pa(io_space[space_nr].mmio_base);
> +	base_mmio = __pa(io_space[space_nr].mmio_base);
>  	base_port = IO_SPACE_BASE(space_nr);
>  	snprintf(name, len, "%s I/O Ports %08lx-%08lx", info->common.name,
> -		 base_port + min, base_port + max);
> +		 base_port + res->start, base_port + res->end);
>  
>  	/*
>  	 * The SDM guarantees the legacy 0-64K space is sparse, but if the
> @@ -190,19 +188,27 @@ static int add_io_space(struct device *dev, struct pci_root_info *info,
>  	resource = iospace->res;
>  	resource->name  = name;
>  	resource->flags = IORESOURCE_MEM;
> -	resource->start = base + (sparse ? IO_SPACE_SPARSE_ENCODING(min) : min);
> -	resource->end   = base + (sparse ? IO_SPACE_SPARSE_ENCODING(max) : max);
> +	resource->start = base_mmio;
> +	resource->end = base_mmio;
> +	if (sparse) {
> +		resource->start += IO_SPACE_SPARSE_ENCODING(res->start);
> +		resource->end += IO_SPACE_SPARSE_ENCODING(res->end);
> +	} else {
> +		resource->start += res->start;
> +		resource->end += res->end;
> +	}
>  	if (insert_resource(&iomem_resource, resource)) {
>  		dev_err(dev,
>  			"can't allocate host bridge io space resource  %pR\n",
>  			resource);
>  		goto free_resource;
>  	}
> +	resource_list_add_tail(iospace, &info->io_resources);
>  
> +	/* Adjust base of original IO port resource descriptor */
>  	entry->offset = base_port;
> -	res->start = min + base_port;
> -	res->end = max + base_port;
> -	resource_list_add_tail(iospace, &info->io_resources);
> +	res->start += base_port;
> +	res->end += base_port;
>  
>  	return 0;
>  
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index cdc5c25..6578f68 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -190,8 +190,7 @@ static bool acpi_decode_space(struct resource_win *win,
>  {
>  	u8 iodec = attr->granularity == 0xfff ? ACPI_DECODE_10 : ACPI_DECODE_16;
>  	bool wp = addr->info.mem.write_protect;
> -	u64 len = attr->address_length;
> -	u64 start, end, offset = 0;
> +	u64 len = attr->address_length, offset = 0;
>  	struct resource *res = &win->res;
>  
>  	/*
> @@ -215,14 +214,13 @@ static bool acpi_decode_space(struct resource_win *win,
>  	else if (attr->translation_offset)
>  		pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",
>  			 attr->translation_offset);
> -	start = attr->minimum + offset;
> -	end = attr->maximum + offset;
>  
>  	win->offset = offset;
> -	res->start = start;
> -	res->end = end;
> +	res->start = attr->minimum;
> +	res->end = attr->maximum;
>  	if (sizeof(resource_size_t) < sizeof(u64) &&
> -	    (offset != win->offset || start != res->start || end != res->end)) {
> +	    (offset != win->offset || attr->minimum != res->start ||
> +	     attr->maximum != res->end)) {
>  		pr_warn("acpi resource window ([%#llx-%#llx] ignored, not CPU addressable)\n",
>  			attr->minimum, attr->maximum);
>  		return false;
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Jan. 25, 2016, 9:52 a.m. UTC | #3
On Tue, Jan 19, 2016 at 12:20:26PM +0000, Lorenzo Pieralisi wrote:
> Gerry,
> 
> On Wed, Jan 13, 2016 at 02:21:04PM +0100, Tomasz Nowicki wrote:
> > From: Liu Jiang <jiang.liu@linux.intel.com>
> > 
> > Some architectures, such as IA64 and ARM64, have no instructions to
> > directly access PCI IO ports, so they map PCI IO ports into PCI MMIO
> > address space. Typically PCI host bridges on those architectures take
> > the responsibility to map (translate) PCI IO port transactions into
> > Memory-Mapped IO transactions. ACPI specification provides support
> > of such a usage case by using resource translation_offset.
> > 
> > But current ACPI resource parsing interface isn't neutral enough,
> > it still has some special logic for IA64. So refine the ACPI resource
> > parsing interface and IA64 code to neutrally handle translation_offset
> > by:
> > 1) ACPI resource parsing interface doesn't do any translation, it just
> >    save the translation_offset to be used by arch code.
> > 2) Arch code will do the mapping(translation) based on arch specific
> >    information. Typically it does:
> > 2.a) Translate per PCI domain IO port address space into system global
> >    IO port address space.
> > 2.b) Setup MMIO address mapping for IO ports.
> 
> This patch fixes IO space handling on IA64 and should go in as a fix.
> 
> IA64 PCI IO space is currently broken (Hanjun tested this on an IA64 box).
> 
> The first broken commit is:
> 
> 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing interface for host bridge")
> 
> because acpi core code checks (in acpi_dev_ioresource_flags()) the
> resource.end>=0x10003, which fails on ia64 - currently resource.end is
> set in acpi_decode_space() to:
> 
> AddressMaximum + AddressTranslation
> 
> where AddressTranslation is the CPU physical address mapping IO space
> on IA64, the >=0x10003 check in acpi_dev_ioresource_flags always
> triggers and the IO resource is then disabled.
> 
> Do you want me to re-send this patch as a fix, with updated commit log ?

Two more points to discuss here. IA64, in its prepare_resources() callback
calls acpi_pci_probe_root_resources() in turn. That function parses the
_CRS and validate the resources (acpi_pci_root_validate_resources()).

That does not make sense IMO, because IA64 changes the IO port resources
after calling acpi_pci_probe_root_resources(), hence the validation
carried out in acpi_pci_probe_root_resources(), at least for IO ports
seems wrong (what's the point of validating the CRS against
ioport_resource if we change the _CRS IO space resources afterwards ?).

Second point: we are aware that by removing the offset addition in
acpi_decode_space(), if for any reason on x86 or IA64 a resource has
that offset !=0 (speaking in terms of memory resources for instance)
we are in trouble. Jiang mentioned that on x86 and IA64 offset is always
0x0 for memory resources, but just want to make sure we are all aware
of this potential pitfall.

Comments appreciated, it is time to a) fix IA64 and b) get this _CRS
parsing consolidation done.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Salter Jan. 25, 2016, 4:57 p.m. UTC | #4
On Mon, 2016-01-25 at 09:52 +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 19, 2016 at 12:20:26PM +0000, Lorenzo Pieralisi wrote:
> > Gerry,
> > 
> > On Wed, Jan 13, 2016 at 02:21:04PM +0100, Tomasz Nowicki wrote:
> > > From: Liu Jiang <jiang.liu@linux.intel.com>
> > > 
> > > Some architectures, such as IA64 and ARM64, have no instructions to
> > > directly access PCI IO ports, so they map PCI IO ports into PCI MMIO
> > > address space. Typically PCI host bridges on those architectures take
> > > the responsibility to map (translate) PCI IO port transactions into
> > > Memory-Mapped IO transactions. ACPI specification provides support
> > > of such a usage case by using resource translation_offset.
> > > 
> > > But current ACPI resource parsing interface isn't neutral enough,
> > > it still has some special logic for IA64. So refine the ACPI resource
> > > parsing interface and IA64 code to neutrally handle translation_offset
> > > by:
> > > 1) ACPI resource parsing interface doesn't do any translation, it just
> > >    save the translation_offset to be used by arch code.
> > > 2) Arch code will do the mapping(translation) based on arch specific
> > >    information. Typically it does:
> > > 2.a) Translate per PCI domain IO port address space into system global
> > >    IO port address space.
> > > 2.b) Setup MMIO address mapping for IO ports.
> > 
> > This patch fixes IO space handling on IA64 and should go in as a fix.
> > 
> > IA64 PCI IO space is currently broken (Hanjun tested this on an IA64 box).
> > 
> > The first broken commit is:
> > 
> > 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing interface for host bridge")
> > 
> > because acpi core code checks (in acpi_dev_ioresource_flags()) the
> > resource.end>=0x10003, which fails on ia64 - currently resource.end is
> > set in acpi_decode_space() to:
> > 
> > AddressMaximum + AddressTranslation
> > 
> > where AddressTranslation is the CPU physical address mapping IO space
> > on IA64, the >=0x10003 check in acpi_dev_ioresource_flags always
> > triggers and the IO resource is then disabled.
> > 
> > Do you want me to re-send this patch as a fix, with updated commit log ?
> 
> Two more points to discuss here. IA64, in its prepare_resources() callback
> calls acpi_pci_probe_root_resources() in turn. That function parses the
> _CRS and validate the resources (acpi_pci_root_validate_resources()).
> 
> That does not make sense IMO, because IA64 changes the IO port resources
> after calling acpi_pci_probe_root_resources(), hence the validation
> carried out in acpi_pci_probe_root_resources(), at least for IO ports
> seems wrong (what's the point of validating the CRS against
> ioport_resource if we change the _CRS IO space resources afterwards ?).
> 

I agree that acpi_pci_root_validate_resources() is doing the wrong thing
generally. The current code (without this patch series) is checking CPU
bus address range for PCI IO window against ioport_resource which is based
on ioport cookies made up by the kernel. This patch takes out the addition
of the translation offset so the check is now PCI bus io address range
validated against the cookies in ioport_resource. I think that works by
chance because all ia64 PCI segments use the same PCI bus io address
range based at zero. So the PCI bus io addresses will always look valid
against ia64 IO space zero in the ioport_resource list. The PCI ioports
actually get installed after ia64 changes the resource to hold the
cookie. Interestingly, ia64 also installs an iomem resource for the CPU
bus address of the ioport range window. The generic ACPI PCI host 
code should probably do the same in pci_acpi_root_prepare_resource().
And at some point, it might make sense to consolidate the ia64 ioport
cookie handling with that done by the devicetree interfaces used by
the generic ACPI PCI host code.

> Second point: we are aware that by removing the offset addition in
> acpi_decode_space(), if for any reason on x86 or IA64 a resource has
> that offset !=0 (speaking in terms of memory resources for instance)
> we are in trouble. Jiang mentioned that on x86 and IA64 offset is always
> 0x0 for memory resources, but just want to make sure we are all aware
> of this potential pitfall.

I think that has always been the case. At least for x86 which doesn't
appear to have ever used the offset in _CRS. In any case, it still
leaves us with the same problem where acpi_pci_root_validate_resources()
is validating PCI bus addresses against something different (cookies/cpu
iospace for ioport, CPU bus for iomem).

> 
> Comments appreciated, it is time to a) fix IA64 and b) get this _CRS
> parsing consolidation done.
> 
> Thanks,
> Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo Jan. 28, 2016, 10:23 a.m. UTC | #5
Hi Lorenzo,

On 2016/1/19 20:20, Lorenzo Pieralisi wrote:
> Gerry,
>
> On Wed, Jan 13, 2016 at 02:21:04PM +0100, Tomasz Nowicki wrote:
>> From: Liu Jiang <jiang.liu@linux.intel.com>
>>
>> Some architectures, such as IA64 and ARM64, have no instructions to
>> directly access PCI IO ports, so they map PCI IO ports into PCI MMIO
>> address space. Typically PCI host bridges on those architectures take
>> the responsibility to map (translate) PCI IO port transactions into
>> Memory-Mapped IO transactions. ACPI specification provides support
>> of such a usage case by using resource translation_offset.
>>
>> But current ACPI resource parsing interface isn't neutral enough,
>> it still has some special logic for IA64. So refine the ACPI resource
>> parsing interface and IA64 code to neutrally handle translation_offset
>> by:
>> 1) ACPI resource parsing interface doesn't do any translation, it just
>>     save the translation_offset to be used by arch code.
>> 2) Arch code will do the mapping(translation) based on arch specific
>>     information. Typically it does:
>> 2.a) Translate per PCI domain IO port address space into system global
>>     IO port address space.
>> 2.b) Setup MMIO address mapping for IO ports.
>
> This patch fixes IO space handling on IA64 and should go in as a fix.
>
> IA64 PCI IO space is currently broken (Hanjun tested this on an IA64 box).
>
> The first broken commit is:
>
> 3772aea7d6f3 ("ia64/PCI/ACPI: Use common ACPI resource parsing interface for host bridge")
>
> because acpi core code checks (in acpi_dev_ioresource_flags()) the
> resource.end>=0x10003, which fails on ia64 - currently resource.end is
> set in acpi_decode_space() to:
>
> AddressMaximum + AddressTranslation
>
> where AddressTranslation is the CPU physical address mapping IO space
> on IA64, the >=0x10003 check in acpi_dev_ioresource_flags always
> triggers and the IO resource is then disabled.
>
> Do you want me to re-send this patch as a fix, with updated commit log ?

I talked to Gerry offline, he is busy these days, he said it's pretty
OK for you to resend this patch and fix the problem.

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
index be4c9ef..c75356b 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -154,7 +154,7 @@  static int add_io_space(struct device *dev, struct pci_root_info *info,
 	struct resource_entry *iospace;
 	struct resource *resource, *res = entry->res;
 	char *name;
-	unsigned long base, min, max, base_port;
+	unsigned long base_mmio, base_port;
 	unsigned int sparse = 0, space_nr, len;
 
 	len = strlen(info->common.name) + 32;
@@ -172,12 +172,10 @@  static int add_io_space(struct device *dev, struct pci_root_info *info,
 		goto free_resource;
 
 	name = (char *)(iospace + 1);
-	min = res->start - entry->offset;
-	max = res->end - entry->offset;
-	base = __pa(io_space[space_nr].mmio_base);
+	base_mmio = __pa(io_space[space_nr].mmio_base);
 	base_port = IO_SPACE_BASE(space_nr);
 	snprintf(name, len, "%s I/O Ports %08lx-%08lx", info->common.name,
-		 base_port + min, base_port + max);
+		 base_port + res->start, base_port + res->end);
 
 	/*
 	 * The SDM guarantees the legacy 0-64K space is sparse, but if the
@@ -190,19 +188,27 @@  static int add_io_space(struct device *dev, struct pci_root_info *info,
 	resource = iospace->res;
 	resource->name  = name;
 	resource->flags = IORESOURCE_MEM;
-	resource->start = base + (sparse ? IO_SPACE_SPARSE_ENCODING(min) : min);
-	resource->end   = base + (sparse ? IO_SPACE_SPARSE_ENCODING(max) : max);
+	resource->start = base_mmio;
+	resource->end = base_mmio;
+	if (sparse) {
+		resource->start += IO_SPACE_SPARSE_ENCODING(res->start);
+		resource->end += IO_SPACE_SPARSE_ENCODING(res->end);
+	} else {
+		resource->start += res->start;
+		resource->end += res->end;
+	}
 	if (insert_resource(&iomem_resource, resource)) {
 		dev_err(dev,
 			"can't allocate host bridge io space resource  %pR\n",
 			resource);
 		goto free_resource;
 	}
+	resource_list_add_tail(iospace, &info->io_resources);
 
+	/* Adjust base of original IO port resource descriptor */
 	entry->offset = base_port;
-	res->start = min + base_port;
-	res->end = max + base_port;
-	resource_list_add_tail(iospace, &info->io_resources);
+	res->start += base_port;
+	res->end += base_port;
 
 	return 0;
 
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index cdc5c25..6578f68 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -190,8 +190,7 @@  static bool acpi_decode_space(struct resource_win *win,
 {
 	u8 iodec = attr->granularity == 0xfff ? ACPI_DECODE_10 : ACPI_DECODE_16;
 	bool wp = addr->info.mem.write_protect;
-	u64 len = attr->address_length;
-	u64 start, end, offset = 0;
+	u64 len = attr->address_length, offset = 0;
 	struct resource *res = &win->res;
 
 	/*
@@ -215,14 +214,13 @@  static bool acpi_decode_space(struct resource_win *win,
 	else if (attr->translation_offset)
 		pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",
 			 attr->translation_offset);
-	start = attr->minimum + offset;
-	end = attr->maximum + offset;
 
 	win->offset = offset;
-	res->start = start;
-	res->end = end;
+	res->start = attr->minimum;
+	res->end = attr->maximum;
 	if (sizeof(resource_size_t) < sizeof(u64) &&
-	    (offset != win->offset || start != res->start || end != res->end)) {
+	    (offset != win->offset || attr->minimum != res->start ||
+	     attr->maximum != res->end)) {
 		pr_warn("acpi resource window ([%#llx-%#llx] ignored, not CPU addressable)\n",
 			attr->minimum, attr->maximum);
 		return false;