Messages in this thread Patch in this message | | | Date | Mon, 22 May 2023 16:17:50 +0200 | Subject | Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR | From | Juergen Gross <> |
| |
On 11.05.23 18:32, Borislav Petkov wrote: > On Wed, May 10, 2023 at 05:53:15PM +0200, Juergen Gross wrote: >> Urgh, yes, there is something missing: >> >> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c >> index 031f7ea8e72b..9544e7d13bb3 100644 >> --- a/arch/x86/kernel/cpu/mtrr/generic.c >> +++ b/arch/x86/kernel/cpu/mtrr/generic.c >> @@ -521,8 +521,12 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform) >> for (i = 0; i < cache_map_n && start < end; i++) { >> if (start >= cache_map[i].end) >> continue; > > So the loop will go through the map until... > >> - if (start < cache_map[i].start) >> + if (start < cache_map[i].start) { > > ... it reaches the first entry where that is true. > >> type = type_merge(type, mtrr_state.def_type, uniform); > > the @type argument is MTRR_TYPE_INVALID, def_type is WRBACK so what > this'll do is simply get you the default WRBACK type: > > type_merge: > if (type == MTRR_TYPE_INVALID) > return new_type; > >> + start = cache_map[i].start; >> + if (end <= start) >> + break; > > Now you break here because end <= start. Why? > > You can just as well do: > > if (start < cache_map[i].start) { > /* region non-overlapping with the region in the map */ > if (end <= cache_map[i].start) > return type_merge(type, mtrr_state.def_type, uniform); > > ... rest of the processing ... > > In general, I get it that your code is slick but I want it to be > maintainable - not slick. I'd like for when people look at this, not > have to add a bunch of debugging output in order to swap the whole > thing back into their brains. > > So mtrr_type_lookup() definitely needs comments explaining what goes > where. > > You can send it as a diff ontop - I'll merge it.
The attached diff is for patch 13.
Juergen
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c index 031f7ea8e72b..4171788b8754 100644 --- a/arch/x86/kernel/cpu/mtrr/generic.c +++ b/arch/x86/kernel/cpu/mtrr/generic.c @@ -519,15 +519,26 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform) return MTRR_TYPE_INVALID; for (i = 0; i < cache_map_n && start < end; i++) { + /* Region after current map entry? -> continue with next one. */ if (start >= cache_map[i].end) continue; - if (start < cache_map[i].start) + + /* Start of region not covered by current map entry? */ + if (start < cache_map[i].start) { + /* At least some part of region has default type. */ type = type_merge(type, mtrr_state.def_type, uniform); + /* End of region not covered, too? -> lookup done. */ + if (end <= cache_map[i].start) + return type; + } + + /* At least part of region covered by map entry. */ type = type_merge(type, cache_map[i].type, uniform); start = cache_map[i].end; } + /* End of region past last entry in map? -> use default type. */ if (start < end) type = type_merge(type, mtrr_state.def_type, uniform); [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |