Re: [PATCH] OvmfPkg/Sec: Clear the Cache Disable flag in the CR0 register

[Jordan Justen <jordan.l.justen@intel.com>]

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.justen@intel.com>
> > Tested-by: Peter Fang <peter.fang@intel.com>
> > Cc: Peter Fang <peter.fang@intel.com>
> > Cc: Maurice Ma <maurice.ma@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Julien Grall <julien.grall@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