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

[Borislav Petkov]

On Thu, Apr 20, 2023 at 02:30:09PM +0200, Juergen Gross wrote:
> OTOH the additional compare to 0 has costs, too, and this cost is spent for
> ALL calls

I'll take the cost of a single

	cmpl    %edi, %edx

for a handful of entries any day of the week.

> while the zero size call is a rather rare case.

$ grep "memmove size" /tmp/mtrr.txt
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0
memmove size 0

for - I admit a largely contrived map - but 5 entries only:

Current map:
0: start: 0x0000000000000000, end: 0x0000000000100000, type: 0x0
1: start: 0x0000000100000000, end: 0x0000000820000000, type: 0x6
2: start: 0x000050d70000c000, end: 0x000062a60000c000, type: 0x4
3: start: 0x000062a60000c000, end: 0x0001d6d200001000, type: 0x0
4: start: 0x0001d6d200001000, end: 0x0001dd8100001000, type: 0x6

> 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

That's not what I meant. Please read again what I said:

"I wouldn't want it getting prefetched from %rsi in the hw when there's
no reason for it".

IOW, I don't want to put invalid values in hw registers if I can. Think
hw mitigations and *very* hungry prefetchers.

> > 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.

Type 0 is MTRR_TYPE_UNCACHABLE, 1 is MTRR_TYPE_WRCOMB.

"Uncacheable (UC)—Reads from, and writes to, UC memory are not cacheable. Reads from UC
memory cannot be speculative. Write-combining to UC memory is not
allowed."

That last sentence.

So if you have conflicting regions and one is WC but then something is
expecting it to be UC and that something doesn't want for it to
*especially* to be WC because it should not combine writes to it, then
that clearly is a misconfiguration, I'd say.

> Now this is another requirement, right? Today's MTRR code wouldn't scream
> either.

So are we fixing this right or only a little?

> At least we don't correct such mistakes today. Do you think we should change
> that?

I'm thinking considering how often we've seen all those error messages
from mtrr_state_warn(), we should at least warn when we encounter
inconsistencies.

This won't help with released BIOSes but it will help when they do new
BIOS verification and see those messages. People do use Linux for that
a lot and then they'll definitely look and address them.

And this is a pretty good goal to have, IMO.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette