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

[Laszlo Ersek <lersek@redhat.com>]

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