Messages in this thread | | | Date | Thu, 29 May 2008 23:51:34 +0200 | From | Rufus & Azrael <> | Subject | Re: [2.6.25-git18 => 2.6.26-rc1-git1] Xorg crash with xf86MapVidMem error |
| |
Venki Pallipadi wrote: > On Thu, May 08, 2008 at 05:34:24PM -0700, Pallipadi, Venkatesh wrote: > >> On Thu, May 08, 2008 at 04:54:44PM -0700, H. Peter Anvin wrote: >> > Venki Pallipadi wrote: >> > > >> > >Also, note that we only look for start while looking at fixed range >> MTRRs. >> > >This is not as scary as it seems. We are finding the effective memory >> type >> > >by just looking at the start of the address range. We still go through >> > >the PAT reserve free mechanism, once we find the effective memory type >> > >and that list will catch any other users with conflicting type anywhere >> > >in the start to end range. And we will still keep effective type >> consistent >> > >across all mappings. >> > > >> > >> > So what you're saying here is "it's bogus, but it doesn't really matter >> > anyway?" Why bother having it at all, then? >> > >> > Seriously, if it's not unconditionally correct, then: >> > >> > a. it should be *clearly* labelled a heuristic. >> > b. it should be *clearly* explained why having the heuristic is much >> > better than not having anything. >> > >> > In this case, neither of those conditions appear to be addressed. >> >> Agree that is has to be called a heuristic. Yes. Not having that will work >> may be not as optimially. Having it gives us better starting point when we >> start to find a proper memory type for requests, especially when there are >> no other overlapping mappings in PAT list. >> >> One of the reason for heristic was to get proper type for /dev/mem maps >> for WB >> region (ACPI and BIOS area). /dev/mem checks to see the mtrr type of the >> start >> address and starts with that type for the request. As long as there are no >> other conflict in PAT list, we can give WB to this /dev/mem request. Not >> having this heuristic we will have to most probably default to UC. >> >> When there are overlapping PAT list entries, the mtrr_lookup does not >> matter >> as we have to inherit things from those PAT entries or conflicting with >> them. >> >> This discussion points to - code is not sufficiently commented and/or >> needs >> some refactoring. Will look at this afresh tomorrow morning and see >> whether >> I can some up with some better alternative. >> >> > > > This is an alternate solution to the problem in this thread. The change > clarifies what is expected out of mtrr_lookup in PAT code and how we handle > failure. > > Thanks, > Venki > > > > Alternate patch to clarify the usage of mtrr_lookup() in PAT code, and to make > PAT code resilient to mtrr lookup problems. > > Specifically, pat_x_mtrr_type() is restructured to highlight, under > what conditions we look for mtrr hint. pat_x_mtrr_type() uses a default > type when there are any errors in mtrr lookup (still maintaining the pat > consistency). And, reserve_memtype() highlights its usage ot mtrr_lookup for > request type of '-1' and also defaults in a sane way on any mtrr lookup failure. > > pat.c looks at mtrr type of a range to get a hint on what mapping type > to request when user/API: > (1) hasn't specified any type (/dev/mem mapping) and we do not want to take > performance hit by always mapping UC_MINUS. This will be the case > for /dev/mem mappings used to map BIOS area or ACPI region which are WB'able. > In this case, as long as MTRR is not WB, PAT will request UC_MINUS for such > mappings. > > (2) user/API requests WB mapping while in reality MTRR may have UC or WC. > In this case, PAT can map as WB (without checking MTRR) and still effective > type will be UC or WC. But, a subsequent request to map same region as > UC or WC may fail, as the region will get trackked as WB in PAT list. Looking > at MTRR hint helps us to track based on effective type rather than what user > requested. Again, here mtrr_lookup is only used as hint and we fallback to > WB mapping (as requested by user) as default. > > In both cases, after using the mtrr hint, we still go through the memtype > list to make sure there are no inconsistencies among multiple users. > > > Signed-off-by: Venkatesh Pallipadi<venkatesh.pallipadi@intel.com> > Signed-off-by: Suresh Siddha<suresh.b.siddha@intel.com> > > --- > arch/x86/mm/pat.c | 49 ++++++++++++++++++++++++------------------------- > 1 file changed, 24 insertions(+), 25 deletions(-) > > Index: linux-2.6/arch/x86/mm/pat.c > =================================================================== > --- linux-2.6.orig/arch/x86/mm/pat.c 2008-05-28 13:19:28.000000000 -0700 > +++ linux-2.6/arch/x86/mm/pat.c 2008-05-28 15:18:22.000000000 -0700 > @@ -151,32 +151,33 @@ static int pat_x_mtrr_type(u64 start, u6 > unsigned long pat_type; > u8 mtrr_type; > > - mtrr_type = mtrr_type_lookup(start, end); > - if (mtrr_type == 0xFF) { /* MTRR not enabled */ > - *ret_prot = prot; > - return 0; > - } > - if (mtrr_type == 0xFE) { /* MTRR match error */ > - *ret_prot = _PAGE_CACHE_UC; > - return -1; > - } > - if (mtrr_type != MTRR_TYPE_UNCACHABLE&& > - mtrr_type != MTRR_TYPE_WRBACK&& > - mtrr_type != MTRR_TYPE_WRCOMB) { /* MTRR type unhandled */ > - *ret_prot = _PAGE_CACHE_UC; > - return -1; > - } > - > pat_type = prot& _PAGE_CACHE_MASK; > prot&= (~_PAGE_CACHE_MASK); > > - /* Currently doing intersection by hand. Optimize it later. */ > + /* > + * We return the PAT request directly for types where PAT takes > + * precedence with respect to MTRR and for UC_MINUS. > + * Consistency checks with other PAT requests is done later > + * while going through memtype list. > + */ > if (pat_type == _PAGE_CACHE_WC) { > *ret_prot = prot | _PAGE_CACHE_WC; > + return 0; > } else if (pat_type == _PAGE_CACHE_UC_MINUS) { > *ret_prot = prot | _PAGE_CACHE_UC_MINUS; > - } else if (pat_type == _PAGE_CACHE_UC || > - mtrr_type == MTRR_TYPE_UNCACHABLE) { > + return 0; > + } else if (pat_type == _PAGE_CACHE_UC) { > + *ret_prot = prot | _PAGE_CACHE_UC; > + return 0; > + } > + > + /* > + * Look for MTRR hint to get the effective type in case where PAT > + * request is for WB. > + */ > + mtrr_type = mtrr_type_lookup(start, end); > + > + if (mtrr_type == MTRR_TYPE_UNCACHABLE) { > *ret_prot = prot | _PAGE_CACHE_UC; > } else if (mtrr_type == MTRR_TYPE_WRCOMB) { > *ret_prot = prot | _PAGE_CACHE_WC; > @@ -233,14 +234,12 @@ int reserve_memtype(u64 start, u64 end, > > if (req_type == -1) { > /* > - * Special case where caller wants to inherit from mtrr or > - * existing pat mapping, defaulting to UC_MINUS in case of > - * no match. > + * Call mtrr_lookup to get the type hint. This is an > + * optimization for /dev/mem mmap'ers into WB memory (BIOS > + * tools and ACPI tools). Use WB request for WB memory and use > + * UC_MINUS otherwise. > */ > u8 mtrr_type = mtrr_type_lookup(start, end); > - if (mtrr_type == 0xFE) { /* MTRR match error */ > - err = -1; > - } > > if (mtrr_type == MTRR_TYPE_WRBACK) { > req_type = _PAGE_CACHE_WB; > > > Hi Venki,
I try your patch above and it works fine for my config, as your previous patch.
Thanks for your help.
Regards.
| |