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

From: Juergen Gross
Date: Thu Apr 20 2023 - 11:10:11 EST

On 20.04.23 16:54, Borislav Petkov wrote:
On Thu, Apr 20, 2023 at 03:57:43PM +0200, Juergen Gross wrote:
So you are suggesting that prefetching can happen across one wrong speculated
branch, but not across two of them? And you are not worrying about prefetches
past the end of a copy with size > 0?

Maybe it will, maybe it won't.

I am worried about calling a function unnecessarily. I'm worried about
calling the asm version __memmove() with zero length unnecessarily. I'm
worried about executing the return thunk unnecessarily:

ffffffff8104c749: 48 83 c4 28 add $0x28,%rsp
ffffffff8104c74d: e9 72 2a 9b 00 jmp ffffffff819ff1c4 <__x86_return_thunk>
ffffffff8104c752: 31 c0 xor %eax,%eax
ffffffff8104c754: e9 6b 2a 9b 00 jmp ffffffff819ff1c4 <__x86_return_thunk>
ffffffff8104c759: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)

Just say that you don't want to do this simple check and I will do it
myself because I'm tired of debating.

I just want to make sure to understand your concerns and that the reasoning
is sane.

You seem to feel rather strong here, so I'll add the test.


"If two or more variable memory ranges match and one of the memory types is UC,
the UC memory type used."

So technically no problem, apart from lower performance.

How do you come from "Write-combining to UC memory is not allowed" to
"lower performance"?

Not allowed is not allowed. Geez.

Yes. And using UC instead of WC usually means lower performance of writes.

Would you be fine with adding that as an additional patch?

I believe if we really want that, then we should be able to disable such a
cleanup. So it should be an add-on anyway.

Sure, whatever.

Okay, thanks.

I think this will need another final loop over the MTRRs to check against the
constructed map if a MTRR is completely useless.

Another question: in case we detect such a hidden MTRR, should it be disabled
in order to have more MTRRs available for run-time adding?


I'm not against adding such additional checks. I wouldn't like to force them
into this series right now, because we need this series for Hyper-V isolated
guest support.

We will add this series when they're ready. If Hyper-V needs them
immediately they can take whatever they want and do whatever they want.

Or you can do a simpler fix for Hyper-V that goes before this, if you
want to address Hyper-V.

Just to say it explicitly: you are concerned for the case that a complete
MTRR is hidden beneath another one (e.g. a large UC MTRR hiding a smaller
MTRR with another type, or a variable MTRR being hidden by fixed MTRRs)?

I am concerned about catching any and all inconsistencies with the MTRRs
and catching them right. If we're going to spend all this time on this,
then let's do it right, once and for all and do it in a manner that can
be improved in the future.

Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature