Bug 14445

Summary: Info: mapping multiple BARs. Your kernel is fine.
Product: ACPI Reporter: Len Brown (lenb)
Component: ACPICA-CoreAssignee: Robert Moore (Robert.Moore)
Status: CLOSED CODE_FIX    
Severity: normal CC: acpi-bugzilla, ming.m.lin, shane.huang, suresh.b.siddha
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.28 - 2.6.31 Subsystem:
Regression: No Bisected commit-id:
Attachments: proposed patch from Bob

Description Len Brown 2009-10-20 07:19:40 UTC
acpi_ex_system_memory_space_handler() is implemented
to round up mapping requests to ACPI_SYSMEM_REGION_WINDOW_SIZE.

However, sometimes this overlaps with other mappings.
At least in one case, this is because doing mappings of this
size crosses a page boundary.

http://lkml.org/lkml/2008/12/30/128

In Linux 2.6.28 a check was added to see if overlapping
mapping requests with different properties are made,
and various AML requests have been triggering Linux
warnings ever since:

http://kerneloops.org/searchweek.php?search=acpi_os_map_memory

The fix for this problem is likely to round up the mapping
request to a page boundary, but to not cross a page boundary.
Comment 1 Robert Moore 2009-10-20 21:42:21 UTC
This can probably be easily added.

ACPI_SYSMEM_REGION_WINDOW_SIZE is 4K by default. Most operation regions are smaller than this anyway, so it certainly makes sense to add some intelligence to the mapping operations. And 4K is the miminum page size AFAIK.

How about this: if the start and end addresses of an operation region fit in a single page, just map that page. if the region crosses a page boundary, map up to the first page boundary immediately, then successive pages later as necessary.

Will this solve the problem?
Comment 2 Suresh B Siddha 2009-10-20 21:49:32 UTC
Yes Bob. That should fix the issue.
Comment 3 Robert Moore 2009-10-20 22:16:55 UTC
Looking at the code, it looks to me that when creating a mapping, the length it uses is the smaller of:

1) The end of the operation region minus the requested address, and
2) ACPI_SYSMEM_REGION_WINDOW_SIZE 

Therefore, the code is not blindly just mapping ACPI_SYSMEM_REGION_WINDOW_SIZE; it is only mapping (at most) to the end of the operation region.

So, page boundaries should have no bearing on the problem; the code will only map to the end of the operation region and presumably sooner or later a mapping will be created that conflicts (assuming that the definition of the region actually intrudes into a page that is used for something else.)



Below,
MemInfo->Address is the base physical address of the region
MemInfo->Length is the length of the region
Address is the requested physical address


/*
 * Don't attempt to map memory beyond the end of the region, and
 * constrain the maximum mapping size to something reasonable.
 */
WindowSize = (ACPI_SIZE)
    ((MemInfo->Address + MemInfo->Length) - Address);

if (WindowSize > ACPI_SYSMEM_REGION_WINDOW_SIZE)
{
    WindowSize = ACPI_SYSMEM_REGION_WINDOW_SIZE;
}
Comment 4 Suresh B Siddha 2009-10-21 00:30:14 UTC
Instead of from start to start+ACPI_SYSTEM_REGION_WINDOW_SIZE, we should map only from start to 4k_roundedown(start+ACPI_SYSTEM_REGION_WINDOW_SIZE).

Though we are in the same operation region, this region is being chopped into sub portions used by multiple ACPI entities. And mapping up to start+ACPI_SYSTEM_REGION_WINDOW_SIZE is going beyond the 4k boundary and checks in the ioremap() complains that we are trying to map beyond one of those sub portions (these subportions are registered in the resource tree as separate entities). Sub-portion in one of the reports we saw was ACPI non-volatile storage.

Mapping upto just the 4k boundary avoids the ioremap() request potentially crossing a resource entity.
Comment 5 Suresh B Siddha 2009-10-21 00:37:28 UTC
For example, in the reported kernel log, this is the error:

resource map sanity check conflict: 0x7fffff00 0x800000ff 0x7ffff000 0x7fffffff ACPI Non-volatile Storage

Here, acpi is trying to map from 0x7fffff00 0x800000ff, and in the resource tree we have a ACPI Non-volatile storage which is from 0x7ffff000 0x7fffffff.

So acpi map request is spanning more than one resource entity.

We should chnage the acpi map request to map only from 0x7fffff00 to 0x7fffffff.
Comment 6 Robert Moore 2009-10-21 00:56:23 UTC
So, in your example, it looks like we have an operation region that is defined from 0x7fffff00 to 0x800000ff.

Are you saying that it would be OK if we created two separate mappings (not at the same time) of:

0x7fffff00 to 0x7fffffff, and
0x80000000 to 0x800000ff
Comment 7 Suresh B Siddha 2009-10-21 01:06:25 UTC
Yes. First ioremap() from 0x7fffff00 to 0x7fffffff followed by another ioremap() from 0x80000000 to 0x800000ff (while the first mapping is active/inactive) is ok.

As the intelligence lies with the caller, we have to do this.

ioremap() checks happen at the resource level and it just happens that we span two different resource entities when we try to map the whole of 0x7fffff00 to 0x800000ff in one ioremap() request.

Hope this helps.
Comment 8 Robert Moore 2009-10-21 01:44:55 UTC
I think I understand the issue.

However, in ACPICA, we don't know what the page size is.
Comment 9 Suresh B Siddha 2009-10-21 12:59:10 UTC
hmm, for now, can't we round down to ACPI_SYSTEM_REGION_WINDOW_SIZE? In general that also makes sense.
Comment 10 Robert Moore 2009-10-21 19:55:30 UTC
Yes, we can do that. Probably rename the #define since we are now dealing with actual pages, not some arbitrary (and configurable) window size.
Comment 11 Robert Moore 2009-10-21 21:15:03 UTC
Another question: Why would there exist an operation region which would cross the boundary into ACPI Non-volatile storage?
Comment 12 Lin Ming 2009-10-22 03:17:34 UTC
Created attachment 23491 [details]
proposed patch from Bob

Patch to ensure that memory mappings created for operation regions do not cross page boundaries.
Crossing a page boundary while mapping regions can cause warnings if the pages have different attributes.
Such regions are probably BIOS bugs, and this is the workaround.
Comment 13 Suresh B Siddha 2009-10-22 06:59:54 UTC
regarding Comment #12, Lin Ming, Please add my Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

regarding Comment #11,

in the lkml report mentioned by the bug description,

BIOS-e820: 000000007fff9000 - 000000007ffff000 (ACPI data)
BIOS-e820: 000000007ffff000 - 0000000080000000 (ACPI NVS)

And we are trying to map from 0x7fffff00 to 0x800000ff and we are crossing the resource boundary of "0x7ffff000 0x7fffffff ACPI Non-volatile Storage"

some bug some where (not sure where! perhaps bios) that was mentioning that resource map is spanning upto 0x800000ff.
Comment 14 Robert Moore 2009-10-22 16:09:02 UTC
>>some bug some where (not sure where! perhaps bios)

Please post the acpidump for the machine, we would like to look at the operation region definitions.
Comment 15 Suresh B Siddha 2009-10-22 21:52:21 UTC
Bob, Have you read the  bug description?

> http://lkml.org/lkml/2008/12/30/128

this url in bug description seems to have the acpi dump. Nevertheless  Idon't have a system where I see this.
Comment 16 Suresh B Siddha 2009-10-26 11:38:01 UTC
Lin Ming, Forgot to add one more request. Can you please add a line in the changelog of the patch that says

[Kernel summit hacking hour]

We root caused it during kernel summit hacking hour. Arjan and others wanted to track the patches using a tag like this, for evaluating the usefulness of the hacking hour during kernel summit. Thanks.
Comment 17 Lin Ming 2009-10-27 00:57:35 UTC
OK.

The patch will first go into ACPICA tree and then be merged into kernel.
Comment 18 Len Brown 2009-10-27 01:30:19 UTC
marking RESOLVED, per patch in comment #12
Comment 19 Len Brown 2009-11-05 22:02:42 UTC
patch in comment #12 applied to acpi tree
Comment 20 Len Brown 2009-11-23 16:32:11 UTC
shipped in v2.6.32-rc7
closed