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.
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature