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

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

[Jordan Justen]

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)
-
-- 
2.20.1

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

[Laszlo Ersek]

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.

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

Thanks
Laszlo

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

[Jordan Justen]

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

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

[Laszlo Ersek]

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 <lersek@redhat.com>; edk2-devel@lists.01.org
>> Cc: Fang, Peter <peter.fang@intel.com>; Ma, Maurice
>> <maurice.ma@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>> Anthony Perard <anthony.perard@citrix.com>; Julien Grall
>> <julien.grall@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.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

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

[Laszlo Ersek]

generic comment (applies to all NASM usage I guess):

On 02/18/19 11:10, Jordan Justen wrote:

> +    mov     eax, cr0
> +    and     eax, ~(1 << 30)
> +    mov     cr0, eax

> +    mov     rax, cr0
> +    and     eax, ~(1 << 30)
> +    mov     cr0, rax

I've read up on the << and ~ operators in the NASM documentation, and I
think the above build-time calculations of the masks are well-defined
and correct.

- bit shifts are always unsigned
- given bit position 30, ~(1 << 30) will be a value with 32 bits
- bit-neg simply flips bits (one's complement)

On the other hand, I find these NASM specifics counter-intuitive. The
expression ~(1 << 30) looks like valid C, but in C, it means a quite
different thing.

I think calculating the mask with "strict dword" somehow (not exactly
sure how) would make this more readable; or else the BTR instruction would.

Opinions? (Again, pertaining to all NASM usage in edk2.)

Thanks
Laszlo

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

[Andrew Fish]

> On Feb 18, 2019, at 5:23 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> generic comment (applies to all NASM usage I guess):
> 
> On 02/18/19 11:10, Jordan Justen wrote:
> 
>> +    mov     eax, cr0
>> +    and     eax, ~(1 << 30)
>> +    mov     cr0, eax
> 
>> +    mov     rax, cr0
>> +    and     eax, ~(1 << 30)
>> +    mov     cr0, rax
> 
> I've read up on the << and ~ operators in the NASM documentation, and I
> think the above build-time calculations of the masks are well-defined
> and correct.
> 
> - bit shifts are always unsigned
> - given bit position 30, ~(1 << 30) will be a value with 32 bits
> - bit-neg simply flips bits (one's complement)
> 
> On the other hand, I find these NASM specifics counter-intuitive. The
> expression ~(1 << 30) looks like valid C, but in C, it means a quite
> different thing.
> 
> I think calculating the mask with "strict dword" somehow (not exactly
> sure how) would make this more readable; or else the BTR instruction would.
> 
> Opinions? (Again, pertaining to all NASM usage in edk2.)
> 

Lazlo,

I guess comments, or #defines, are other options?

Thanks,

Andrew Fish

> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 02/19/19 20:51, Andrew Fish wrote:
> 
> 
>> On Feb 18, 2019, at 5:23 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> generic comment (applies to all NASM usage I guess):
>>
>> On 02/18/19 11:10, Jordan Justen wrote:
>>
>>> +    mov     eax, cr0
>>> +    and     eax, ~(1 << 30)
>>> +    mov     cr0, eax
>>
>>> +    mov     rax, cr0
>>> +    and     eax, ~(1 << 30)
>>> +    mov     cr0, rax
>>
>> I've read up on the << and ~ operators in the NASM documentation, and I
>> think the above build-time calculations of the masks are well-defined
>> and correct.
>>
>> - bit shifts are always unsigned
>> - given bit position 30, ~(1 << 30) will be a value with 32 bits
>> - bit-neg simply flips bits (one's complement)
>>
>> On the other hand, I find these NASM specifics counter-intuitive. The
>> expression ~(1 << 30) looks like valid C, but in C, it means a quite
>> different thing.
>>
>> I think calculating the mask with "strict dword" somehow (not exactly
>> sure how) would make this more readable; or else the BTR instruction would.
>>
>> Opinions? (Again, pertaining to all NASM usage in edk2.)
>>
> 
> Lazlo,
> 
> I guess comments, or #defines, are other options?

Good point! They are.

Thanks,
Laszlo

> Thanks,
> 
> Andrew Fish
> 
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

[Jordan Justen]

On 2019-02-18 05:23:28, Laszlo Ersek wrote:
> generic comment (applies to all NASM usage I guess):
> 
> On 02/18/19 11:10, Jordan Justen wrote:
> 
> > +    mov     eax, cr0
> > +    and     eax, ~(1 << 30)
> > +    mov     cr0, eax
> 
> > +    mov     rax, cr0
> > +    and     eax, ~(1 << 30)
> > +    mov     cr0, rax
> 
> I've read up on the << and ~ operators in the NASM documentation, and I
> think the above build-time calculations of the masks are well-defined
> and correct.
> 
> - bit shifts are always unsigned
> - given bit position 30, ~(1 << 30) will be a value with 32 bits
> - bit-neg simply flips bits (one's complement)
> 
> On the other hand, I find these NASM specifics counter-intuitive. The
> expression ~(1 << 30) looks like valid C, but in C, it means a quite
> different thing.

Can you elaborate? I guess there might be something subtly different,
but for the most part it means the same thing, right?

> I think calculating the mask with "strict dword" somehow (not exactly
> sure how) would make this more readable;

Oh, are you saying that (1 << 30) doesn't necessarily mean we are
operating on a 32-bit value?

> or else the BTR instruction would.

Yeah, I guess this works.

> Opinions? (Again, pertaining to all NASM usage in edk2.)

As always, my opinion is to avoid writing assembly code. :)

We actually had a version that set this just before the decompress in
SecMain.c. Then I noted that we were initializing temp-ram here, so I
moved it, even though the memory init doesn't take a significant
amount of time compared to the decompress. Maybe we should just do
that instead?

-Jordan

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

[Laszlo Ersek]

On 02/19/19 20:59, Jordan Justen wrote:
> On 2019-02-18 05:23:28, Laszlo Ersek wrote:
>> generic comment (applies to all NASM usage I guess):
>>
>> On 02/18/19 11:10, Jordan Justen wrote:
>>
>>> +    mov     eax, cr0
>>> +    and     eax, ~(1 << 30)
>>> +    mov     cr0, eax
>>
>>> +    mov     rax, cr0
>>> +    and     eax, ~(1 << 30)
>>> +    mov     cr0, rax
>>
>> I've read up on the << and ~ operators in the NASM documentation, and I
>> think the above build-time calculations of the masks are well-defined
>> and correct.
>>
>> - bit shifts are always unsigned
>> - given bit position 30, ~(1 << 30) will be a value with 32 bits
>> - bit-neg simply flips bits (one's complement)
>>
>> On the other hand, I find these NASM specifics counter-intuitive. The
>> expression ~(1 << 30) looks like valid C, but in C, it means a quite
>> different thing.
> 
> Can you elaborate? I guess there might be something subtly different,
> but for the most part it means the same thing, right?
> 
>> I think calculating the mask with "strict dword" somehow (not exactly
>> sure how) would make this more readable;
> 
> Oh, are you saying that (1 << 30) doesn't necessarily mean we are
> operating on a 32-bit value?

I think the code is fine as-is, in the technical sense; it is just
confusing/concerning to read for someone with a background in C. The
expression *looks* like C, but doesn't *behave* like in C. Ultimately,
the actual behavior is *right* for the assembly, but the code is still
difficult to read. (To me anyway.)

>> or else the BTR instruction would.
> 
> Yeah, I guess this works.
> 
>> Opinions? (Again, pertaining to all NASM usage in edk2.)
> 
> As always, my opinion is to avoid writing assembly code. :)
> 
> We actually had a version that set this just before the decompress in
> SecMain.c. Then I noted that we were initializing temp-ram here, so I
> moved it, even though the memory init doesn't take a significant
> amount of time compared to the decompress. Maybe we should just do
> that instead?

I think I would prefer that, yes, if the performance on ACRN remains
tolerable like that.

In fact... can you identify ACRN through AsmCpuidEx() or similar, and
restrict the CR0.CD manipulation to ACRN?

Thanks,
Laszlo