x86: Fix the ACPI resource handling and range checking
Subsystems (4)
cover x86: Fix the ACPI resource handling and range checking
Yinghai noticed that Jiangs changes broke the resource range checks. I picked up his patch and addressed a few other things:
- Ignoring the IORESOURCE_DISABLED flag
- Range checking of IO resources against iomem_resource
There is more cleanup work to do in that area, but thats for later.
Please review and test ride it, so we can get x86/apic out of blocked state.
Thanks,
tglx
--- arch/x86/pci/acpi.c | 111 +++++++++++++++++++++++++++----------------------- drivers/acpi/ioapic.c | 38 +++++++++-------- 2 files changed, 83 insertions(+), 66 deletions(-)
PATCH 1/4 x86, pci, acpi: Redo sanity checks for root brigde probing
From: Yinghai Lu <[email protected]>
commit e22ce93870de ("x86, PCI, ACPI: Kill private function resource_to_addr() in arch/x86/pci/acpi.c") dropped the sanity checks for resources initialized via acpi_dev_resource_memory().
It further dropped a check for address_length = 0 for the resources which are initialized via acpi_dev_resource_address_space().
For translated addresses the range check operates on the non translated end address. Assign orig_end after adding the translation offset.
This causes that invalid resources are not dropped and the range check for translated addresses fails.
[ tglx: Rewrote changelog.
Added the address_length check to the x86 code and not into acpi_dev_resource_address_space(). While the acpi function should check this, we want to have a proper check which sets the IORESOURCE_DISABLED bit as it does for IO resources.
Removed the pointless memset of addr. acpi_resource_to_address64() either fails or fills it completely ]
--- arch/x86/pci/acpi.c | 63 ++++++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 31 deletions(-)
arch/x86/pci/acpi.c +32 -31
@@ -221,10 +221,9 @@ static void teardown_mcfg_map(struct pcistatic acpi_status count_resource (struct acpi_resource *acpi_res,void *data) {struct pci_root_info *info = data; -struct resource r = { - .flags =0 - }; +struct resource r; +memset (&r,0 ,sizeof (r));if (!acpi_dev_resource_memory (acpi_res, &r) && !acpi_dev_resource_address_space (acpi_res, &r))return AE_OK ;
@@ -239,14 +238,13 @@ static acpi_status setup_resource(struct {struct pci_root_info *info = data;u64 translation_offset =0 ; -struct resource r = { - .flags =0 - }; +struct resource r; +u64 orig_end; +memset (&r,0 ,sizeof (r));if (acpi_dev_resource_memory (acpi_res, &r)) { - r.flags &=IORESOURCE_MEM |IORESOURCE_IO ; + r.flags &=IORESOURCE_MEM ; }else if (acpi_dev_resource_address_space (acpi_res, &r)) { -u64 orig_end;struct acpi_resource_address64 addr; r.flags &=IORESOURCE_MEM |IORESOURCE_IO ;
@@ -256,40 +254,43 @@ static acpi_status setup_resource(structif (ACPI_FAILURE (acpi_resource_to_address64 (acpi_res, &addr)))return AE_OK ; +if (!addr.address_length) +return AE_OK ; +if (addr.resource_type ==ACPI_MEMORY_RANGE && addr.info.mem.caching ==ACPI_PREFETCHABLE_MEMORY ) r.flags |=IORESOURCE_PREFETCH ; translation_offset = addr.translation_offset; - orig_end = r.end; r.start += translation_offset; r.end += translation_offset; - -/* Exclude non-addressable range or non-addressable portion of range */ - r.end =min (r.end, iomem_resource.end); -if (r.end <= r.start) { -dev_info (&info->bridge->dev, -"host bridge window [%#llx-%#llx] (ignored, not CPU addressable)\n" , - (unsigned long long )r.start, orig_end); -return AE_OK ; - }else if (orig_end != r.end) { -dev_info (&info->bridge->dev, -"host bridge window [%#llx-%#llx] ([%#llx-%#llx] ignored, not CPU addressable)\n" , - (unsigned long long )r.start, orig_end, - (unsigned long long )r.end +1 , orig_end); - } + }else { +return AE_OK ; } -if (r.flags &&resource_size (&r)) { - r.name = info->name; - info->res[info->res_num] = r; - info->res_offset[info->res_num] = translation_offset; - info->res_num++; -if (!pci_use_crs) -dev_printk (KERN_DEBUG , &info->bridge->dev, -"host bridge window %pR (ignored)\n" , &r); +/* Exclude non-addressable range or non-addressable portion of range */ + orig_end = r.end; + r.end =min (r.end, iomem_resource.end); +if (r.end <= r.start) { +dev_info (&info->bridge->dev, +"host bridge window [%#llx-%#llx] (ignored, not CPU addressable)\n" , + (unsigned long long )r.start, orig_end); +return AE_OK ; + }else if (orig_end != r.end) { +dev_info (&info->bridge->dev, +"host bridge window [%#llx-%#llx] ([%#llx-%#llx] ignored, not CPU addressable)\n" , + (unsigned long long )r.start, orig_end, + (unsigned long long )r.end +1 , orig_end); } + r.name = info->name; + info->res[info->res_num] = r; + info->res_offset[info->res_num] = translation_offset; + info->res_num++; +if (!pci_use_crs) +dev_printk (KERN_DEBUG , &info->bridge->dev, +"host bridge window %pR (ignored)\n" , &r); +return AE_OK ; }
Borislav Petkov
maintainerRe: [patch 1/4] x86, pci, acpi: Redo sanity checks for root brigde probing
Quoted 56 lines · Thomas Gleixner
From: Yinghai Lu <[email protected]>
commit
e22ce93870de("x86, PCI, ACPI: Kill private functionresource_to_addr()inarch/x86/pci/acpi.c") dropped the sanity checks for resources initialized viaacpi_dev_resource_memory().It further dropped a check for
address_length= 0 for the resources which are initialized viaacpi_dev_resource_address_space().For translated addresses the range check operates on the non translated end address. Assign
orig_endafter adding the translation offset.This causes that invalid resources are not dropped and the range check for translated addresses fails.
[ tglx: Rewrote changelog.
Added the
address_lengthcheck to the x86 code and not intoacpi_dev_resource_address_space(). While the acpi function should check this, we want to have a proper check which sets theIORESOURCE_DISABLEDbit as it does for IO resources.Removed the pointless memset of addr.
acpi_resource_to_address64()either fails or fills it completely ]Fixes commit e22ce93870de ("x86, PCI, ACPI: Kill private function resource_to_addr() in arch/x86/pci/acpi.c").Signed-off-by Yinghai Lu <[email protected]>Link http://lkml.kernel.org/r/CAE9FiQWaL1Qu1eP-fQV784ucy+r-65AZ95VyiUrefn0dNtfLDw@mail.gmail.com open hereSigned-off-by Thomas Gleixner <[email protected]>--- arch/x86/pci/acpi.c | 63 ++++++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 31 deletions(-)Index:
tip/arch/x86/pci/acpi.c
===================================================================arch/x86/pci/acpi.c +2
-3@@ -221,10 +221,9 @@ static void teardown_mcfg_map(struct pcistatic acpi_status count_resource (struct acpi_resource *acpi_res,void *data) {struct pci_root_info *info = data; -struct resource r = { - .flags =0 - }; +struct resource r; +memset (&r,0 ,sizeof (r));if (!acpi_dev_resource_memory (acpi_res, &r) && !acpi_dev_resource_address_space (acpi_res, &r))return AE_OK ;@@ -239,14 +238,13 @@ static acpi_status setup_resource(struct {struct pci_root_info *info = data;u64 translation_offset =0 ;
arch/x86/pci/acpi.c static acpi_status setup_resource(struct · line 246
Quoted 10 lines
arch/x86/pci/acpi.c +4
-4-struct resource r = { - .flags =0 - }; +struct resource r; +u64 orig_end; +memset (&r,0 ,sizeof (r));if (acpi_dev_resource_memory (acpi_res, &r)) { - r.flags &=IORESOURCE_MEM |IORESOURCE_IO ; + r.flags &=IORESOURCE_MEM ;
As you pointed out on IRC, the &= is kinda ass-backwards logically but I don't understand the big picture yet to know what happens with this resource when it gets copied into the root a bit further down:
...
info->res[info->res_num] = r;and what a *cleared* IORESOURCE_MEM flag for a memory resource means...
arch/x86/pci/acpi.c static acpi_status setup_resource(struct · line 259
Quoted 12 lines
arch/x86/pci/acpi.c +3
-1}else if (acpi_dev_resource_address_space (acpi_res, &r)) { -u64 orig_end;struct acpi_resource_address64 addr; r.flags &=IORESOURCE_MEM |IORESOURCE_IO ;@@ -256,40 +254,43 @@ static acpi_status setup_resource(structif (ACPI_FAILURE (acpi_resource_to_address64 (acpi_res, &addr)))return AE_OK ; +if (!addr.address_length) +return AE_OK ; +
We have an
if (r.flags == 0)
test above this - might change it to
if (!r.flags)
for consistency.
Quoted 37 lines
arch/x86/pci/acpi.c +5
-23if (addr.resource_type ==ACPI_MEMORY_RANGE && addr.info.mem.caching ==ACPI_PREFETCHABLE_MEMORY ) r.flags |=IORESOURCE_PREFETCH ; translation_offset = addr.translation_offset; - orig_end = r.end; r.start += translation_offset; r.end += translation_offset; - -/* Exclude non-addressable range or non-addressable portion of range */ - r.end =min (r.end, iomem_resource.end); -if (r.end <= r.start) { -dev_info (&info->bridge->dev, -"host bridge window [%#llx-%#llx] (ignored, not CPU addressable)\n" , - (unsigned long long )r.start, orig_end); -return AE_OK ; - }else if (orig_end != r.end) { -dev_info (&info->bridge->dev, -"host bridge window [%#llx-%#llx] ([%#llx-%#llx] ignored, not CPU addressable)\n" , - (unsigned long long )r.start, orig_end, - (unsigned long long )r.end +1 , orig_end); - } + }else { +return AE_OK ; } -if (r.flags &&resource_size (&r)) { - r.name = info->name; - info->res[info->res_num] = r; - info->res_offset[info->res_num] = translation_offset; - info->res_num++; -if (!pci_use_crs) -dev_printk (KERN_DEBUG , &info->bridge->dev, -"host bridge window %pR (ignored)\n" , &r); +/* Exclude non-addressable range or non-addressable portion of range */ + orig_end = r.end; + r.end =min (r.end, iomem_resource.end);
arch/x86/pci/acpi.c static acpi_status setup_resource(struct · line 283
Quoted 10 lines
arch/x86/pci/acpi.c +10
-0+if (r.end <= r.start) { +dev_info (&info->bridge->dev, +"host bridge window [%#llx-%#llx] (ignored, not CPU addressable)\n" , + (unsigned long long )r.start, orig_end); +return AE_OK ; + }else if (orig_end != r.end) { +dev_info (&info->bridge->dev, +"host bridge window [%#llx-%#llx] ([%#llx-%#llx] ignored, not CPU addressable)\n" , + (unsigned long long )r.start, orig_end, + (unsigned long long )r.end +1 , orig_end);
This hunk could be made a little more readable (diff ontop):
---
arch/x86/pci/acpi.c +8 -5
@@ -270,26 +270,29 @@ static acpi_status setup_resource(struct/* Exclude non-addressable range or non-addressable portion of range */ orig_end = r.end; - r.end =min (r.end, iomem_resource.end); + r.end =min (r.end, iomem_resource.end); +if (r.end <= r.start) {dev_info (&info->bridge->dev,"host bridge window [%#llx-%#llx] (ignored, not CPU addressable)\n" , (unsigned long long )r.start, orig_end);return AE_OK ; - }else if (orig_end != r.end) { + } + +if (orig_end != r.end) {dev_info (&info->bridge->dev,"host bridge window [%#llx-%#llx] ([%#llx-%#llx] ignored, not CPU addressable)\n" , (unsigned long long )r.start, orig_end, (unsigned long long )r.end +1 , orig_end); } +if (!pci_use_crs) +dev_printk (KERN_DEBUG , &info->bridge->dev,"host bridge window %pR (ignored)\n" , &r); + r.name = info->name; info->res[info->res_num] = r; info->res_offset[info->res_num] = translation_offset; info->res_num++; -if (!pci_use_crs) -dev_printk (KERN_DEBUG , &info->bridge->dev, -"host bridge window %pR (ignored)\n" , &r);return AE_OK ; }
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--PATCH 2/4 x86: pci: acpi: Respect ioresource flags
In reply to Thomas Gleixnersetup_res() blindly clear all resource->flags except IO and MEM. So if a resource is marked disabled by the acpi code, setup_res() will use it nevertheless.
Preserve the flags and add proper checks to setup_res() and __release_pci_root_info(). The latter is simplified while at it.
--- arch/x86/pci/acpi.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)
arch/x86/pci/acpi.c +16 -19
@@ -218,6 +218,12 @@ static void teardown_mcfg_map(struct pci }#endif +static bool is_valid_resource (struct resource *res) +{ +return (res->flags & (IORESOURCE_MEM |IORESOURCE_IO )) && + !(res->flags &IORESOURCE_DISABLED ); +} +static acpi_status count_resource (struct acpi_resource *acpi_res,void *data) {struct pci_root_info *info = data;
@@ -228,7 +234,7 @@ static acpi_status count_resource(struct !acpi_dev_resource_address_space(acpi_res, &r))return AE_OK ; -if ((r.flags & (IORESOURCE_IO |IORESOURCE_MEM )) &&resource_size (&r)) +if (is_valid_resource (&r)) info->res_num++;return AE_OK ;
@@ -242,13 +248,10 @@ static acpi_status setup_resource(structu64 orig_end;memset (&r,0 ,sizeof (r)); -if (acpi_dev_resource_memory (acpi_res, &r)) { - r.flags &=IORESOURCE_MEM ; - }else if (acpi_dev_resource_address_space (acpi_res, &r)) { +if (!acpi_dev_resource_memory (acpi_res, &r)) {struct acpi_resource_address64 addr; - r.flags &=IORESOURCE_MEM |IORESOURCE_IO ; -if (r.flags ==0 ) +if (!acpi_dev_resource_address_space (acpi_res, &r))return AE_OK ;if (ACPI_FAILURE (acpi_resource_to_address64 (acpi_res, &addr)))
@@ -264,10 +267,11 @@ static acpi_status setup_resource(struct translation_offset = addr.translation_offset; r.start += translation_offset; r.end += translation_offset; - }else { -return AE_OK ; } +if (!is_valid_resource (&r)) +return AE_OK ; +/* Exclude non-addressable range or non-addressable portion of range */ orig_end = r.end; r.end =min (r.end, iomem_resource.end);
@@ -367,19 +371,12 @@ static void free_pci_root_info_res(strucstatic void __release_pci_root_info (struct pci_root_info *info) { +struct resource *res = info->res;int i; -struct resource *res; - -for (i =0 ; i < info->res_num; i++) { - res = &info->res[i]; - -if (!res->parent) -continue ; - -if (!(res->flags & (IORESOURCE_MEM |IORESOURCE_IO ))) -continue ; -release_resource (res); +for (i =0 ; i < info->res_num; i++, res++) { +if (res->parent &&is_valid_resource (res)) +release_resource (res); }free_pci_root_info_res (info);
Borislav Petkov
maintainerRe: [patch 2/4] x86: pci: acpi: Respect ioresource flags
Quoted 30 lines · Thomas Gleixner
setup_res()blindly clear allresource->flagsexcept IO and MEM. So if a resource is marked disabled by the acpi code,setup_res()will use it nevertheless.Preserve the flags and add proper checks to
setup_res()and__release_pci_root_info(). The latter is simplified while at it.Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/pci/acpi.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)Index:
tip/arch/x86/pci/acpi.c
===================================================================arch/x86/pci/acpi.c +6
-0@@ -218,6 +218,12 @@ static void teardown_mcfg_map(struct pci }#endif +static bool is_valid_resource (struct resource *res) +{ +return (res->flags & (IORESOURCE_MEM |IORESOURCE_IO )) && + !(res->flags &IORESOURCE_DISABLED ); +} +static acpi_status count_resource (struct acpi_resource *acpi_res,void *data) {struct pci_root_info *info = data;
arch/x86/pci/acpi.c static acpi_status count_resource(struct · line 240
Quoted 8 lines
arch/x86/pci/acpi.c +1
-1@@ -228,7 +234,7 @@ static acpi_status count_resource(struct !acpi_dev_resource_address_space(acpi_res, &r))return AE_OK ; -if ((r.flags & (IORESOURCE_IO |IORESOURCE_MEM )) &&resource_size (&r)) +if (is_valid_resource (&r)) info->res_num++;return AE_OK ;
Ok, this makes definitely more sense in getting rid of the *clearing* of the flags which is intuitively backwards to what it should do.
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--PATCH 3/4 x86: pci: acpi: Fix the range check for IO resources
In reply to Thomas GleixnerThe range check in setup_res() checks the IO range against iomem_resource. That's just wrong.
Move the check into a separate function and use ioport_resource for IO ranges.
--- arch/x86/pci/acpi.c | 51 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 18 deletions(-)
arch/x86/pci/acpi.c +33 -18
@@ -240,12 +240,41 @@ static acpi_status count_resource(structreturn AE_OK ; } +static bool valid_resource_range (struct resource *r,struct device *dev) +{ +struct resource *ior; +u64 end; + + ior = (r->flags &IORESOURCE_IO ) ? &ioport_resource : &iomem_resource; + +/* +* Exclude non-addressable range or non-addressable portion of +* range. We only check end because ior->start is always 0. +*/ + end = r->end; + r->end =min (r->end, ior->end); +if (r->end <= r->start) { +dev_info (dev,"host bridge %s window [%#llx-%#llx] invalid range\n" , + r->flags &IORESOURCE_IO ?"io" :"mem" , + (unsigned long long )r->start, end); +return false; + } + +if (r->end == end) +return true; + +dev_info (dev,"host bridge %s window [%#llx-%#llx] truncated to [%#llx-%#llx]\n" , + r->flags &IORESOURCE_IO ?"io" :"mem" , + (unsigned long long )r->start, end, + (unsigned long long )r->start, (unsigned long long )r->end); +return true; +} +static acpi_status setup_resource (struct acpi_resource *acpi_res,void *data) {struct pci_root_info *info = data;u64 translation_offset =0 ;struct resource r; -u64 orig_end;memset (&r,0 ,sizeof (r));if (!acpi_dev_resource_memory (acpi_res, &r)) {
@@ -269,31 +298,17 @@ static acpi_status setup_resource(struct r.end += translation_offset; } -if (!is_valid_resource (&r)) +if (!is_valid_resource (&r) || + !valid_resource_range (&r, &info->bridge->dev))return AE_OK ; -/* Exclude non-addressable range or non-addressable portion of range */ - orig_end = r.end; - r.end =min (r.end, iomem_resource.end); -if (r.end <= r.start) { -dev_info (&info->bridge->dev, -"host bridge window [%#llx-%#llx] (ignored, not CPU addressable)\n" , - (unsigned long long )r.start, orig_end); -return AE_OK ; - }else if (orig_end != r.end) { -dev_info (&info->bridge->dev, -"host bridge window [%#llx-%#llx] ([%#llx-%#llx] ignored, not CPU addressable)\n" , - (unsigned long long )r.start, orig_end, - (unsigned long long )r.end +1 , orig_end); - } - r.name = info->name; info->res[info->res_num] = r; info->res_offset[info->res_num] = translation_offset; info->res_num++;if (!pci_use_crs)dev_printk (KERN_DEBUG , &info->bridge->dev, -"host bridge window %pR (ignored)\n" , &r); +"host bridge window %pR ignored\n" , &r);return AE_OK ; }
Borislav Petkov
maintainerRe: [patch 3/4] x86: pci: acpi: Fix the range check for IO resources
Quoted 25 lines · Thomas Gleixner
The range check in
setup_res()checks the IO range againstiomem_resource. That's just wrong.Move the check into a separate function and use
ioport_resourcefor IO ranges.Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/pci/acpi.c | 51 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 18 deletions(-)Index:
tip/arch/x86/pci/acpi.c
===================================================================arch/x86/pci/acpi.c +6
-0@@ -240,12 +240,41 @@ static acpi_status count_resource(structreturn AE_OK ; } +static bool valid_resource_range (struct resource *r,struct device *dev) +{ +struct resource *ior; +u64 end; + + ior = (r->flags &IORESOURCE_IO ) ? &ioport_resource : &iomem_resource;
arch/x86/pci/acpi.c static acpi_status count_resource(struct · line 258
Quoted 10 lines
arch/x86/pci/acpi.c +10
-0+ +/* +* Exclude non-addressable range or non-addressable portion of +* range. We only check end because ior->start is always 0. +*/ + end = r->end; + r->end =min (r->end, ior->end); +if (r->end <= r->start) { + dev_info(dev, "host bridge %s window [%#llx-%#llx] invalid range\n", + r->flags & IORESOURCE_IO ? "io" : "mem",
I think you can simplify this and below even further as the check Iagainst ORESOURCE_IO is done already when selecting the ior thing (diff Iontop):
---
arch/x86/pci/acpi.c +2 -2
@@ -255,7 +255,7 @@ static bool valid_resource_range(struct r->end =min (r->end, ior->end);if (r->end <= r->start) {dev_info (dev,"host bridge %s window [%#llx-%#llx] invalid range\n" , - r->flags &IORESOURCE_IO ?"io" :"mem" , + ior->name, (unsigned long long )r->start, end);return false; }
@@ -264,7 +264,7 @@ static bool valid_resource_range(structreturn true;dev_info (dev,"host bridge %s window [%#llx-%#llx] truncated to [%#llx-%#llx]\n" , - r->flags &IORESOURCE_IO ?"io" :"mem" , + ior->name, (unsigned long long )r->start, end, (unsigned long long )r->start, (unsigned long long )r->end);return true;
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--PATCH 4/4 acpi: ioapic: Respect the resource flags
In reply to Thomas GleixnerThe acpi parser can set the DISABLED flag on a resource. In setup_res() we clear all flags except MEM, so we ignore the DISABLED flag. Add proper checks and preserve the flags.
--- drivers/acpi/ioapic.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)
drivers/acpi/ioapic.c +21 -17
@@ -40,29 +40,33 @@ struct acpi_pci_ioapic {static LIST_HEAD (ioapic_list);static DEFINE_MUTEX (ioapic_list_lock); +static inline bool is_valid_mem_resource (struct resource *res) +{ +return !(res->flags &IORESOURCE_DISABLED ) && + (res->flags &IORESOURCE_MEM ); +} +static acpi_status setup_res (struct acpi_resource *acpi_res,void *data) { +struct acpi_resource_address64 addr;struct resource *res = data;memset (res,0 ,sizeof (*res)); -if (acpi_dev_resource_memory (acpi_res, res)) { - res->flags &=IORESOURCE_MEM ; -if (res->flags) -return AE_OK ; - }else if (acpi_dev_resource_address_space (acpi_res, res)) { -struct acpi_resource_address64 addr; +if (acpi_dev_resource_memory (acpi_res, res)) +return AE_OK ; - res->flags &=IORESOURCE_MEM ; -if (res->flags && -ACPI_SUCCESS (acpi_resource_to_address64 (acpi_res, &addr)) && - addr.info.mem.caching !=ACPI_PREFETCHABLE_MEMORY ) { - res->start += addr.translation_offset; - res->end += addr.translation_offset; -return AE_OK ; - } +if (!acpi_dev_resource_address_space (acpi_res, res) || + !is_valid_mem_resource (res)) +return AE_OK ; +/* +* FIXME: This lacks a proper comment, why the resource +* address needs to be translated. +*/ +if (ACPI_SUCCESS (acpi_resource_to_address64 (acpi_res, &addr)) && + addr.info.mem.caching !=ACPI_PREFETCHABLE_MEMORY ) { + res->start += addr.translation_offset; + res->end += addr.translation_offset; } - res->flags =0 ; -return AE_OK ; }
@@ -150,7 +154,7 @@ static acpi_status handle_ioapic_add(acp res = &ioapic->res;acpi_walk_resources (handle,METHOD_NAME__CRS , setup_res, res); -if (res->flags ==IORESOURCE_UNSET ) { +if (!is_valid_mem_resource (res)) {acpi_handle_warn (handle,"failed to get resource\n" ); goto exit_free; }else if (request_resource (&iomem_resource, res)) {
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
Quoted 66 lines · Thomas Gleixner
The acpi parser can set the DISABLED flag on a resource. In
setup_res()we clear all flags except MEM, so we ignore the DISABLED flag. Add proper checks and preserve the flags.Signed-off-by Thomas Gleixner <[email protected]>--- drivers/acpi/ioapic.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)Index:
tip/drivers/acpi/ioapic.c
===================================================================drivers/acpi/ioapic.c +20
-16@@ -40,29 +40,33 @@ struct acpi_pci_ioapic {static LIST_HEAD (ioapic_list);static DEFINE_MUTEX (ioapic_list_lock); +static inline bool is_valid_mem_resource (struct resource *res) +{ +return !(res->flags &IORESOURCE_DISABLED ) && + (res->flags &IORESOURCE_MEM ); +} +static acpi_status setup_res (struct acpi_resource *acpi_res,void *data) { +struct acpi_resource_address64 addr;struct resource *res = data;memset (res,0 ,sizeof (*res)); -if (acpi_dev_resource_memory (acpi_res, res)) { - res->flags &=IORESOURCE_MEM ; -if (res->flags) -return AE_OK ; - }else if (acpi_dev_resource_address_space (acpi_res, res)) { -struct acpi_resource_address64 addr; +if (acpi_dev_resource_memory (acpi_res, res)) +return AE_OK ; - res->flags &=IORESOURCE_MEM ; -if (res->flags && -ACPI_SUCCESS (acpi_resource_to_address64 (acpi_res, &addr)) && - addr.info.mem.caching !=ACPI_PREFETCHABLE_MEMORY ) { - res->start += addr.translation_offset; - res->end += addr.translation_offset; -return AE_OK ; - } +if (!acpi_dev_resource_address_space (acpi_res, res) || + !is_valid_mem_resource (res)) +return AE_OK ; +/* +* FIXME: This lacks a proper comment, why the resource +* address needs to be translated. +*/ +if (ACPI_SUCCESS (acpi_resource_to_address64 (acpi_res, &addr)) && + addr.info.mem.caching !=ACPI_PREFETCHABLE_MEMORY ) { + res->start += addr.translation_offset; + res->end += addr.translation_offset; } - res->flags =0 ; -return AE_OK ; }
drivers/acpi/ioapic.c static acpi_status handle_ioapic_add(acp · line 160
Quoted 8 lines
drivers/acpi/ioapic.c +1
-1@@ -150,7 +154,7 @@ static acpi_status handle_ioapic_add(acp res = &ioapic->res;acpi_walk_resources (handle,METHOD_NAME__CRS , setup_res, res); -if (res->flags ==IORESOURCE_UNSET ) { +if (!is_valid_mem_resource (res)) {acpi_handle_warn (handle,"failed to get resource\n" ); goto exit_free; }else if (request_resource (&iomem_resource, res)) {
There is minor problem about mem pref handling, original code will ignore them.
with this patch will let it follow through.
should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
+static inline bool is_valid_mem_nonpref_resource(struct resource *res)
{
return !(res->flags & IORESOURCE_DISABLED) &&
- (res->flags & IORESOURCE_MEM);
+ (res->flags & IORESOURCE_MEM) &&
+ !(res->flags & IORESOURCE_PREFETCH);}
other than that for whole patch set:
Also please check attached following patch that will move code to acpi core.
Thanks
Yinghai
Subject: [PATCH] x86, PCI, ACPI: offset resource in acpi core
Also pass back via offset for pci root bus resource probing.
and set IORESOURCE_PREFETCH...
--- arch/x86/pci/acpi.c | 29 ++++++----------------------- drivers/acpi/ioapic.c | 22 ++++++---------------- drivers/acpi/resource.c | 16 +++++++++++----- drivers/pnp/pnpacpi/rsparser.c | 2 +- include/linux/acpi.h | 2 +- 5 files changed, 25 insertions(+), 46 deletions(-)
arch/x86/pci/acpi.c +6 -23
@@ -231,7 +231,7 @@ static acpi_status count_resource(structmemset (&r,0 ,sizeof (r));if (!acpi_dev_resource_memory (acpi_res, &r) && - !acpi_dev_resource_address_space (acpi_res, &r)) + !acpi_dev_resource_address_space (acpi_res, &r,NULL ))return AE_OK ;if (is_valid_resource (&r))
@@ -273,30 +273,13 @@ static bool valid_resource_range(structstatic acpi_status setup_resource (struct acpi_resource *acpi_res,void *data) {struct pci_root_info *info = data; -u64 translation_offset =0 ; +u64 offset =0 ;struct resource r;memset (&r,0 ,sizeof (r)); -if (!acpi_dev_resource_memory (acpi_res, &r)) { -struct acpi_resource_address64 addr; - -if (!acpi_dev_resource_address_space (acpi_res, &r)) -return AE_OK ; - -if (ACPI_FAILURE (acpi_resource_to_address64 (acpi_res, &addr))) -return AE_OK ; - -if (!addr.address_length) -return AE_OK ; - -if (addr.resource_type ==ACPI_MEMORY_RANGE && - addr.info.mem.caching ==ACPI_PREFETCHABLE_MEMORY ) - r.flags |=IORESOURCE_PREFETCH ; - - translation_offset = addr.translation_offset; - r.start += translation_offset; - r.end += translation_offset; - } +if (!acpi_dev_resource_memory (acpi_res, &r) && + !acpi_dev_resource_address_space (acpi_res, &r, &offset)) +return AE_OK ;if (!is_valid_resource (&r) || !valid_resource_range (&r, &info->bridge->dev))
@@ -304,7 +287,7 @@ static acpi_status setup_resource(struct r.name = info->name; info->res[info->res_num] = r; - info->res_offset[info->res_num] = translation_offset; + info->res_offset[info->res_num] = offset; info->res_num++;if (!pci_use_crs) dev_printk(KERN_DEBUG, &info->bridge->dev,
drivers/acpi/ioapic.c +6 -16
@@ -40,33 +40,23 @@ struct acpi_pci_ioapic {static LIST_HEAD (ioapic_list);static DEFINE_MUTEX (ioapic_list_lock); -static inline bool is_valid_mem_resource (struct resource *res) +static inline bool is_valid_mem_nonpref_resource (struct resource *res) {return !(res->flags &IORESOURCE_DISABLED ) && - (res->flags &IORESOURCE_MEM ); + (res->flags &IORESOURCE_MEM ) && + !(res->flags &IORESOURCE_PREFETCH ); }static acpi_status setup_res (struct acpi_resource *acpi_res,void *data) { -struct acpi_resource_address64 addr;struct resource *res = data;memset (res,0 ,sizeof (*res));if (acpi_dev_resource_memory (acpi_res, res))return AE_OK ; -if (!acpi_dev_resource_address_space (acpi_res, res) || - !is_valid_mem_resource (res)) -return AE_OK ; -/* -* FIXME: This lacks a proper comment, why the resource -* address needs to be translated. -*/ -if (ACPI_SUCCESS (acpi_resource_to_address64 (acpi_res, &addr)) && - addr.info.mem.caching !=ACPI_PREFETCHABLE_MEMORY ) { - res->start += addr.translation_offset; - res->end += addr.translation_offset; - } +acpi_dev_resource_address_space (acpi_res, res,NULL ); +return AE_OK ; }
@@ -154,7 +144,7 @@ static acpi_status handle_ioapic_add(acp res = &ioapic->res;acpi_walk_resources (handle,METHOD_NAME__CRS , setup_res, res); -if (!is_valid_mem_resource (res)) { +if (!is_valid_mem_nonpref_resource (res)) {acpi_handle_warn (handle,"failed to get resource\n" ); goto exit_free; }else if (request_resource (&iomem_resource, res)) {
drivers/acpi/resource.c +11 -5
@@ -175,13 +175,14 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_io);* acpi_dev_resource_address_space - Extract ACPI address space information. * @ares: Input ACPI resource object. * @res: Output generic resource object. +* @offset: Output for translation_offset. * * Check if the given ACPI resource object represents an address space resource * and if that's the case, use the information in it to populate the generic * resource object pointed to by @res. */ bool acpi_dev_resource_address_space (struct acpi_resource *ares, -struct resource *res) +struct resource *res,u64 *offset) {acpi_status status;struct acpi_resource_address64 addr;
@@ -199,12 +200,14 @@ bool acpi_dev_resource_address_space(str } status =acpi_resource_to_address64 (ares, &addr); -if (ACPI_FAILURE (status)) +if (ACPI_FAILURE (status) || !addr.address_length)return false; - res->start = addr.minimum; - res->end = addr.maximum; + res->start = addr.minimum + addr.translation_offset; + res->end = addr.maximum + addr.translation_offset; window = addr.producer_consumer ==ACPI_PRODUCER ; +if (offset) + *offset = addr.translation_offset;switch (addr.resource_type) {case ACPI_MEMORY_RANGE :
@@ -212,6 +215,9 @@ bool acpi_dev_resource_address_space(str res->flags =acpi_dev_memresource_flags (len, addr.info.mem.write_protect, window); +if (addr.info.mem.caching ==ACPI_PREFETCHABLE_MEMORY ) + res->flags |=IORESOURCE_PREFETCH ; +break ;case ACPI_IO_RANGE : io_decode = addr.granularity == 0xfff ?
@@ -471,7 +477,7 @@ static acpi_status acpi_dev_process_resoif (acpi_dev_resource_memory (ares, &r) ||acpi_dev_resource_io (ares, &r) - ||acpi_dev_resource_address_space (ares, &r) + ||acpi_dev_resource_address_space (ares, &r,NULL ) ||acpi_dev_resource_ext_address_space (ares, &r))return acpi_dev_new_resource_entry (&r, c);
drivers/pnp/pnpacpi/rsparser.c +1 -1
@@ -183,7 +183,7 @@ static acpi_status pnpacpi_allocated_resstruct resource r = {0 };int i, flags; -if (acpi_dev_resource_address_space (res, &r) +if (acpi_dev_resource_address_space (res, &r,NULL ) ||acpi_dev_resource_ext_address_space (res, &r)) {pnp_add_resource (dev, &r);return AE_OK ;
include/linux/acpi.h +1 -1
@@ -292,7 +292,7 @@ extern int pnpacpi_disabled;bool acpi_dev_resource_memory (struct acpi_resource *ares,struct resource *res);bool acpi_dev_resource_io (struct acpi_resource *ares,struct resource *res);bool acpi_dev_resource_address_space (struct acpi_resource *ares, -struct resource *res); +struct resource *res,u64 *offset);bool acpi_dev_resource_ext_address_space (struct acpi_resource *ares,struct resource *res);unsigned long acpi_dev_irq_flags (u8 triggering,u8 polarity,u8 shareable);
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
Quoted 8 lines · Yinghai Lu
Review
Quoted 6 lines · Thomas Gleixner
(quoted diff) +6
-0+static inline bool is_valid_mem_resource(struct resource *res) +{ + return !(res->flags & IORESOURCE_DISABLED) && + (res->flags & IORESOURCE_MEM); +} +There is minor problem about mem pref handling, original code will ignore them.
Bah. I missed that in that well documented function...
drivers/acpi/ioapic.c struct acpi_pci_ioapic { · line 48
Quoted 11 lines
with this patch will let it follow through.
should change
is_valid_mem_resourcetois_valid_mem_nonpref_resource()...drivers/acpi/ioapic.c +3
-1+static inline bool is_valid_mem_nonpref_resource (struct resource *res) {return !(res->flags &IORESOURCE_DISABLED ) && - (res->flags &IORESOURCE_MEM ); + (res->flags &IORESOURCE_MEM ) && + !(res->flags &IORESOURCE_PREFETCH ); }
Unfortunately that does not help, because nothing sets the IORESOURCE_PREFETCH flag. Will fix it proper.
I still have no explanation why the translation offset needs to be applied here.
Thanks,
tglx
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
Quoted 3 lines · Thomas Gleixner
I still have no explanation why the translation offset needs to be applied here.
That should be copied from pci root bus resource setup.
But looks like only ia64 use those translation_offset?
Thanks
Yinghai
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
In reply to Thomas GleixnerQuoted 24 lines · Thomas Gleixner
Quoted 8 lines · Yinghai Lu
Review
Quoted 6 lines · Thomas Gleixner
(quoted diff) +6
-0+static inline bool is_valid_mem_resource(struct resource *res) +{ + return !(res->flags & IORESOURCE_DISABLED) && + (res->flags & IORESOURCE_MEM); +} +There is minor problem about mem pref handling, original code will ignore them.
Bah. I missed that in that well documented function...
drivers/acpi/ioapic.c struct acpi_pci_ioapic { · line 48
Quoted 11 lines
with this patch will let it follow through.
should change
is_valid_mem_resourcetois_valid_mem_nonpref_resource()...drivers/acpi/ioapic.c +3
-1+static inline bool is_valid_mem_nonpref_resource (struct resource *res) {return !(res->flags &IORESOURCE_DISABLED ) && - (res->flags &IORESOURCE_MEM ); + (res->flags &IORESOURCE_MEM ) && + !(res->flags &IORESOURCE_PREFETCH ); }
Quoted 5 lines
Unfortunately that does not help, because nothing sets the
IORESOURCE_PREFETCHflag. Will fix it proper.I still have no explanation why the translation offset needs to be applied here.
Hi Thomas,
I have read related section in ACPI spec, seems the addition
of translation_offset is redundant here.
Quotation from ACPI spec 5a, 6.4.3.5.1
For bridges that translate addresses across the bridge, this is the offset that must be added to the address on the secondary side to obtain the address on the primary side. Non-bridge devices must list 0 for all Address Translation offset bits.
Quotation from ACPI spec 5, 9.17 I/O APIC Device:
It must also contain a _CRS object that reports the base address of the I/O APIC device. The _CRS object is required to contain only one resource, a memory resource pointing to the I/O APIC register base.
IO APIC is not a bridge, so translation_offset should always be zero. Thanks!
Gerry
Quoted 5 lines
Thanks,
tglx
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
Quoted 40 lines · Jiang Liu
Quoted 24 lines · Thomas Gleixner
Quoted 8 lines · Yinghai Lu
Review
Quoted 6 lines · Thomas Gleixner
(quoted diff) +6
-0+static inline bool is_valid_mem_resource(struct resource *res) +{ + return !(res->flags & IORESOURCE_DISABLED) && + (res->flags & IORESOURCE_MEM); +} +There is minor problem about mem pref handling, original code will ignore them.
Bah. I missed that in that well documented function...
drivers/acpi/ioapic.c struct acpi_pci_ioapic { · line 48
Quoted 11 lines
with this patch will let it follow through.
should change
is_valid_mem_resourcetois_valid_mem_nonpref_resource()...drivers/acpi/ioapic.c +3
-1+static inline bool is_valid_mem_nonpref_resource (struct resource *res) {return !(res->flags &IORESOURCE_DISABLED ) && - (res->flags &IORESOURCE_MEM ); + (res->flags &IORESOURCE_MEM ) && + !(res->flags &IORESOURCE_PREFETCH ); }Quoted 5 lines
Unfortunately that does not help, because nothing sets the
IORESOURCE_PREFETCHflag. Will fix it proper.I still have no explanation why the translation offset needs to be applied here.
Hi Thomas,
I have read related section in ACPI spec, seems the addition
oftranslation_offsetis redundant here.Quotation from ACPI spec 5a, 6.4.3.5.1
For bridges that translate addresses across the bridge, this is the offset that must be added to the address on the secondary side to obtain the address on the primary side. Non-bridge devices must list 0 for all Address Translation offset bits.
Quoted 6 lines
Quotation from ACPI spec 5, 9.17 I/O APIC Device:
It must also contain a_CRSobject that reports the base address of the I/O APIC device. The_CRSobject is required to contain only one resource, a memory resource pointing to the I/O APIC register base.IO APIC is not a bridge, so
translation_offsetshould always be zero.
Right and we really need a proper interface for this on the acpi side, so we can avoid all that dance in the usage sites.
Thanks,
tglx
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
Hi Thomas,
Should I keep the development history or start from scratch
for this ACPI resource patch set?
Thanks!
Gerry
Quoted 60 lines · Thomas Gleixner
Quoted 40 lines · Jiang Liu
Quoted 24 lines · Thomas Gleixner
Quoted 8 lines · Yinghai Lu
Review
Quoted 6 lines · Thomas Gleixner
(quoted diff) +6
-0+static inline bool is_valid_mem_resource(struct resource *res) +{ + return !(res->flags & IORESOURCE_DISABLED) && + (res->flags & IORESOURCE_MEM); +} +There is minor problem about mem pref handling, original code will ignore them.
Bah. I missed that in that well documented function...
drivers/acpi/ioapic.c struct acpi_pci_ioapic { · line 48
Quoted 11 lines
with this patch will let it follow through.
should change
is_valid_mem_resourcetois_valid_mem_nonpref_resource()...drivers/acpi/ioapic.c +3
-1+static inline bool is_valid_mem_nonpref_resource (struct resource *res) {return !(res->flags &IORESOURCE_DISABLED ) && - (res->flags &IORESOURCE_MEM ); + (res->flags &IORESOURCE_MEM ) && + !(res->flags &IORESOURCE_PREFETCH ); }Quoted 5 lines
Unfortunately that does not help, because nothing sets the
IORESOURCE_PREFETCHflag. Will fix it proper.I still have no explanation why the translation offset needs to be applied here.
Hi Thomas,
I have read related section in ACPI spec, seems the addition
oftranslation_offsetis redundant here.Quotation from ACPI spec 5a, 6.4.3.5.1
For bridges that translate addresses across the bridge, this is the offset that must be added to the address on the secondary side to obtain the address on the primary side. Non-bridge devices must list 0 for all Address Translation offset bits.Quoted 6 lines
Quotation from ACPI spec 5, 9.17 I/O APIC Device:
It must also contain a_CRSobject that reports the base address of the I/O APIC device. The_CRSobject is required to contain only one resource, a memory resource pointing to the I/O APIC register base.IO APIC is not a bridge, so
translation_offsetshould always be zero.Right and we really need a proper interface for this on the acpi side, so we can avoid all that dance in the usage sites.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
Quoted 3 lines · Jiang Liu
Hi Thomas,
Should I keep the development history or start from scratch
for this ACPI resource patch set?
We better start from scratch.
Re: [patch 4/4] acpi: ioapic: Respect the resource flags
In reply to Yinghai LuQuoted 3 lines · Yinghai Lu
On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <[email protected]> wrote:
Also please check attached following patch that will move code to acpi core.
Fun. I came up with more or less the same patch (properly split into seperate patches) yesterday morning, but decided to keep it out of the series because it looked ugly.
We really should have something like this:
struct translated_rescource {
struct resource res;
u64 offset;
};and a proper acpi function for it. Adding the offset could be done there as well. That will simplify the magic in x86/pci/acpi as you only have to deal with a single storage place.
Thanks,
tglx