Re: [PATCH v5 11/15] x86/mtrr: construct a memory map with cache modes

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3308 bytes --]

On 20.04.23 14:30, Juergen Gross wrote:
> 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.

Bah, forgot the link:

[1]: https://lists.llvm.org/pipermail/llvm-dev/2007-October/011070.html


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]