Messages in this thread | | | Date | Thu, 20 Apr 2023 14:30:09 +0200 | From | Juergen Gross <> | Subject | Re: [PATCH v5 11/15] x86/mtrr: construct a memory map with cache modes |
| |
On 20.04.23 14:15, Borislav Petkov wrote: > On Sat, Apr 01, 2023 at 08:36:48AM +0200, Juergen Gross wrote: >> +static void rm_map_entry_at(int idx) >> +{ >> + cache_map_n--; > > Let's not call memmove() when cache_map_n == idx. > > Below too. > > Especially since cache_map + idx + 1 is not valid and I wouldn't want it > getting prefetched from %rsi in the hw when there's no reason for it and > also the RET even from a function which doesn't do anything, costs.
OTOH the additional compare to 0 has costs, too, and this cost is spent for ALL calls, while the zero size call is a rather rare case.
Regarding "cache_map + idx + 1 is not valid": the standard clearly points out that a call with size 0 is valid and won't copy anything [1].
> >> + memmove(cache_map + idx, cache_map + idx + 1, >> + sizeof(*cache_map) * (cache_map_n - idx)); >> +} > > Ok, first weird issue I encountered while playing with my carved out > program to exercise this cache_map handling thing. I can share it if you > want it - it is ugly but it works. > > So while rebuilding the map, I have these current regions in the map, at > one point in time: > > Current map: > 0: start: 0x0000000000000000, end: 0x0000000000100000, type: 0x0 > 1: start: 0x0000000100000000, end: 0x0000000820000000, type: 0x6 > 2: start: 0x000002f10000c000, end: 0x000003bf0000c000, type: 0x2 > 3: start: 0x000003bf0000c000, end: 0x00000d4b0000c000, type: 0x1 > 4: start: 0x00000d4b0000c000, end: 0x00019fc000001000, type: 0x0 > 5: start: 0x00019fc000001000, end: 0x0001df2d00001000, type: 0x2 > > note entry #3. > > Now the next one it inserts is: > > add_map_entry_at: start: 0x3bf0000c000, end: 0xd4b0000c000, type: 0x0, idx: 3 > merge_prev 0: prev->fixed: 0, prev->end: 0x3bf0000c000, prev->type: 0x2 > merge_next: 1, next->fixed: 0, next->start: 0xd4b0000c000, next->type: 0x0 > add_map_entry_at: ret: 1 > > Note how it is the same as entry number #3 - just a different type. > > What it ends up doing is, it simply overwrites the previous one and > merges it with the next: > > Current map: > 0: start: 0x0000000000000000, end: 0x0000000000100000, type: 0x0 > 1: start: 0x0000000100000000, end: 0x0000000820000000, type: 0x6 > 2: start: 0x000002f10000c000, end: 0x000003bf0000c000, type: 0x2 > 3: start: 0x000003bf0000c000, end: 0x00019fc000001000, type: 0x0 > 4: start: 0x00019fc000001000, end: 0x0001df2d00001000, type: 0x2
The map would reflect hardware behavior. Type 0 wins in case of overlapping MTRRs.
> Now I think right about now we should've screamed loudly.
Now this is another requirement, right? Today's MTRR code wouldn't scream either.
> I know I know, this should never happen, right? And firmware programming > those MTRRs doesn't make mistakes...
At least we don't correct such mistakes today. Do you think we should change that?
> However, we should be loud here and scream when a MTRR range disappears > like that. > > Right?
TBH, I don't know.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |