rufus-azrael at numericable
May 29, 2008, 2:51 PM
Post #31 of 32
(869 views)
Permalink
|
|
Re: [2.6.25-git18 => 2.6.26-rc1-git1] Xorg crash with xf86MapVidMem error
[In reply to]
|
|
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 [at] intel>
> Signed-off-by: Suresh Siddha<suresh.b.siddha [at] intel>
>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
|
