On 02/20/19 03:43, Fang, Peter wrote:
> When CR0.CD is set, KVM *might* emulate it by setting MTRR_TYPE_UNCACHABLE in
> its EPTE if non-coherent DMA is detected, because VT-d does not provide
> snooping in this case.
I think this was precisely the use case that I had to tweak with Linux
commit 879ae1880449. (Which complemented earlier tweak fb279950ba02.)
The original commit fb279950ba02 -- i.e., the kernel regression --
mentions "noncoherent_dma guests".
> AFAIK, leaving the bit on can have a performance impact for Xen as well.
I can't comment on that; haven't seen it reported.
> This issue was found when we tried to boot OVMF on ACRN.
Ah, yet another "small footprint" VMM. :)
> ACRN emulates CR0.CD through guest IPAT and memory decompression took a very
> long time to finish (tens of seconds).
OK. Please include this description of the use case in the commit
message. If we have a specific use case that triggers a symptom, the
commit message shouldn't be general only. It can be general too, but the
specifics should be mentioned.
I consider this patch easy to revert (if necessary) for a long time to
come. So I'm not too worried about regressing other setups. If we do, we
can revert the patch.
I'm OK with the patch once the commit message is cleaned up, and we
figure out what we want to do about "~(1 << 30)" vs. BTR in the assembly.
Thanks,
Laszlo
>> -----Original Message-----
>> From: Justen, Jordan L
>> Sent: Tuesday, February 19, 2019 11:45 AM
>> To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org
>> Cc: Fang, Peter <peter.f...@intel.com>; Ma, Maurice
>> <maurice...@intel.com>; Ard Biesheuvel <ard.biesheu...@linaro.org>;
>> Anthony Perard <anthony.per...@citrix.com>; Julien Grall
>> <julien.gr...@linaro.org>
>> Subject: Re: [PATCH] OvmfPkg/Sec: Clear the Cache Disable flag in the CR0
>> register
>>
>> On 2019-02-18 04:17:26, Laszlo Ersek wrote:
>>> On 02/18/19 11:10, Jordan Justen wrote:
>>>> Clear the CD (Cache Disable) flag in the CR0 register. When the VM
>>>> implements the CD flag, this can substantially decrease the time it
>>>> takes to decompress the firmware volumes.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
>>>> Tested-by: Peter Fang <peter.f...@intel.com>
>>>> Cc: Peter Fang <peter.f...@intel.com>
>>>> Cc: Maurice Ma <maurice...@intel.com>
>>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>>> Cc: Anthony Perard <anthony.per...@citrix.com>
>>>> Cc: Julien Grall <julien.gr...@linaro.org>
>>>> ---
>>>> OvmfPkg/Sec/Ia32/SecEntry.nasm | 8 +++++++-
>>>> OvmfPkg/Sec/X64/SecEntry.nasm | 8 +++++++-
>>>> 2 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm
>>>> b/OvmfPkg/Sec/Ia32/SecEntry.nasm index 03501969eb..fc7f47385a
>> 100644
>>>> --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
>>>> +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>>>> @@ -40,6 +40,13 @@ extern ASM_PFX(SecCoreStartupWithStack) global
>>>> ASM_PFX(_ModuleEntryPoint)
>>>> ASM_PFX(_ModuleEntryPoint):
>>>>
>>>> + ;
>>>> + ; Clear the CD (Cache Disable) flag in the CR0 register.
>>>> + ;
>>>> + mov eax, cr0
>>>> + and eax, ~(1 << 30)
>>>> + mov cr0, eax
>>>> +
>>>> ;
>>>> ; Fill the temporary RAM with the initial stack value.
>>>> ; The loop below will seed the heap as well, but that's harmless.
>>>> @@ -71,4 +78,3 @@ ASM_PFX(_ModuleEntryPoint):
>>>> push eax
>>>> push ebp
>>>> call ASM_PFX(SecCoreStartupWithStack)
>>>> -
>>>> diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm
>>>> b/OvmfPkg/Sec/X64/SecEntry.nasm index d76adcffd8..7471b3a3e3
>> 100644
>>>> --- a/OvmfPkg/Sec/X64/SecEntry.nasm
>>>> +++ b/OvmfPkg/Sec/X64/SecEntry.nasm
>>>> @@ -41,6 +41,13 @@ extern ASM_PFX(SecCoreStartupWithStack) global
>>>> ASM_PFX(_ModuleEntryPoint)
>>>> ASM_PFX(_ModuleEntryPoint):
>>>>
>>>> + ;
>>>> + ; Clear the CD (Cache Disable) flag in the CR0 register.
>>>> + ;
>>>> + mov rax, cr0
>>>> + and eax, ~(1 << 30)
>>>> + mov cr0, rax
>>>> +
>>>> ;
>>>> ; Fill the temporary RAM with the initial stack value.
>>>> ; The loop below will seed the heap as well, but that's harmless.
>>>> @@ -72,4 +79,3 @@ ASM_PFX(_ModuleEntryPoint):
>>>> mov rdx, rsp
>>>> sub rsp, 0x20
>>>> call ASM_PFX(SecCoreStartupWithStack)
>>>> -
>>>>
>>>
>>> In commit 98f378a7be12 ("OvmfPkg/ResetVector: enable caching in
>>> initial page tables", 2013-09-24), we said
>>>
>>> In UEFI X64 we use other mechanisms to disable caching.
>>> (CD/NW in CR0 and MTRRs.)
>>>
>>> That suggests that having caching disabled in SEC is a good thing.
>>
>> I think there can be good reasons to disable caching on a platform, such as
>> during memory initialization, but I'm not sure any apply to OVMF.
>>
>> As far as I know, kvm still doesn't attempt to disable caching via the CR0.CD
>> bit or the MTRRs. This kind of backs up the previous statement about there
>> not being a reason to disable caching in OVMF.
>>
>>> What has changed? I assume Peter reported a problem.
>>>
>>> ... Is this by any chance related to Linux commit 879ae1880449 ("KVM:
>>> x86: obey KVM_X86_QUIRK_CD_NW_CLEARED in kvm_set_cr0()", 2015-
>> 11-04)
>>> -- or CR0.CD virtualization in KVM, in general?
>>>
>>> (The commit message says "When the VM implements the CD flag", and
>> not
>>> "When the *VMM* implements the CD flag", so I'm unsure.)
>>
>> Yeah, I guess we probably want VMM, and I don't think the observed issue
>> applies to kvm.
>>
>> -Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
