MTRR setup in OVMF [was: PATCH v3 01/10 KVM: MMU: fix decoding cache type from MTRR]

From: Laszlo Ersek <lersek@redhat.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>
Cc: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	edk2-devel list <edk2-devel@lists.sourceforge.net>,
	"Jordan Justen (Intel address)" <jordan.l.justen@intel.com>
Subject: MTRR setup in OVMF [was: PATCH v3 01/10 KVM: MMU: fix decoding cache type from MTRR]
Date: Tue, 14 Jul 2015 23:12:52 +0200	[thread overview]
Message-ID: <55A57B54.2040305@redhat.com> (raw)
In-Reply-To: <55A3D616.8010209@linux.intel.com>

Cross-posting to edk2-devel.

Original sub-thread starts here:
http://thread.gmane.org/gmane.linux.kernel/1952205/focus=1994315

On 07/13/15 17:15, Xiao Guangrong wrote:
> 
> 
> On 07/13/2015 11:13 PM, Paolo Bonzini wrote:
>> On 13/07/2015 16:45, Xiao Guangrong wrote:
>>>> +    /* MTRR is completely disabled, use UC for all of physical
>>>> memory. */
>>>> +    if (!(mtrr_state->enabled & 0x2))
>>>> +        return MTRR_TYPE_UNCACHABLE;
>>>>
>>>> actually disappears in commit fa61213746a7 (KVM: MTRR: simplify
>>>> kvm_mtrr_get_guest_memory_type, 2015-06-15).
>>>
>>> :(
>>>
>>> Based on the SDM, UC is applied to all memory rather than default-type
>>> if MTRR is disabled.
>>
>> There are two issues I think.  One is that I cannot find in the current
>> code that "UC is applied to all memory rather than default-type if MTRR
>> is disabled".  mtrr_default_type unconditionally looks at
>> mtrr_state->deftype.
> 
> Yes... Will fix.
> 
>>
>>> However, fast boot came back if "return 0xFF" here. So fast boot expects
>>> that the memory type is WB.
>>
>> Yes.
>>
>>>>
>>>> static u8 mtrr_default_type(struct kvm_mtrr *mtrr_state)
>>>> {
>>>>           if (mtrr_is_enabled(mtrr_state))
>>>>                   return mtrr_state->deftype &
>>>> IA32_MTRR_DEF_TYPE_TYPE_MASK;
>>>>           else
>>>>                   return MTRR_TYPE_UNCACHABLE;
>>>> }
>>>>
>>>> ?  Then it's easy to add a quirk that makes the default WRITEBACK until
>>>> MTRRs are enabled.
>>>
>>> It is the wrong configure in OVMF... shall we need to adjust KVM to
>>> satisfy
>>> OVMF?
>>
>> That's what quirks are for...  The firmware should still be fixed of
>> course.
> 
> I see, will do it.

The long delay that Alex reported (for the case when all guest memory
was set to UC up-front) is due to the fact that the SEC phase of OVMF
decompresses an approximately 1712 KB sized, LZMA-compressed blob, to
approx. 896 KB worth of PEI drivers and 8192 KB worth of DXE and UEFI
drivers -- and this decompression is extremely memory-intensive.

(Refer to "Firmware image structure" in the OVMF whitepaper,
<http://www.linux-kvm.org/page/OVMF>.)

Perhaps also significant, with this initial MTRR change: the x86_64
reset vector builds some page tables too. (Refer to "Select features |
X64-specific reset vector for OVMF" in the OVMF whitepaper.)

(When Jordan implemented that reset vector first, we saw similar
performance degradation on AMD hosts (albeit not due to MTRR but due to
page attributes). See
<https://github.com/tianocore/edk2/commit/98f378a7>. I'm only mentioning
it here because it makes me appreciate the current problem report.)

Anyway, the reset vector's page table building is implemented in
"OvmfPkg/ResetVector/Ia32/PageTables64.asm". The decompression in SEC
can be found in "OvmfPkg/Sec/SecMain.c", function DecompressMemFvs().
(It is recommended to refer heavily to the OVMF whitepaper, or at least
to drink heavily.)

In the PEI phase, we do set up MTRRs sensibly, see
"OvmfPkg/PlatformPei/MemDetect.c", function QemuInitializeRam().
However, that's too late with regard this problem report, because PEI
modules run *from* one of the firmware volumes that SEC expands with LZMA.

Anyway, the logic in QemuInitializeRam() relies on the MtrrLib library
class, which OVMF resolves to the
"UefiCpuPkg/Library/MtrrLib/MtrrLib.inf" instance. The latter has no
client module type restriction (it's BASE), so it could be used in SEC
too, if someone were to write patches.

I'm sorry but I really can't take this on now. So, respectfully:
- please quirk it in KVM for now (SeaBIOS is also affected),
- "patches welcome" for OVMF, as always.

Thanks
Laszlo