Re: [PATCH v6 02/17] OvmfPkg/ResetVector: Set C-bit when building initial page table

[Brijesh Singh <brijesh.singh@amd.com>]

On 06/01/2017 03:09 AM, Jordan Justen wrote:
> On 2017-05-26 07:43:50, Brijesh Singh wrote:
>> SEV guest VMs have the concept of private and shared memory. Private
>> memory is encrypted with the guest-specific key, while shared memory
>> may be encrypted with hypervisor key. Certain types of memory (namely
>> instruction pages and guest page tables) are always treated as private
>> memory by the hardware. The C-bit in PTE indicate whether the page is
>> private or shared. The C-bit position for the PTE can be obtained from
>> CPUID Fn8000_001F[EBX].
>>
>> When SEV is active, the BIOS is encrypted by the Qemu launch sequence,
>> we must set the C-bit when building the page table.
>>
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm | 70 +++++++++++++++++++-
>>   1 file changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index 6201cad1f5dc..3d4b04844cdf 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -37,9 +37,60 @@ BITS    32
>>                          PAGE_READ_WRITE + \
>>                          PAGE_PRESENT)
>>   
>> +; Check if Secure Encrypted Virtualization (SEV) feature is enabled
>> +;
>> +; If SEV is enabled then EAX will be at least 32
>> +; If SEV is disabled then EAX will be zero.
>> +;
>> +CheckSevFeature:
>> +    ; CPUID will clobber EBX, ECX, EDX, save these registers
>> +    push  ebx
>> +    push  ecx
>> +    push  edx
> 
> I don't think we have a stack set up in this code, which is why
> OneTimeCall/OneTimeCallRet is used. I'm wondering how this is working
> at all.
> 
> I don't think we have a stack until OvmfPkg/Sec/*/SecEntry.nasm.
> 
> More below...
> 

Thanks for catching this Jordan. I am also wondering why the code has been
working.

>> +
>> +    ; Check if we have a valid (0x8000_001F) CPUID leaf
>> +    mov       eax, 0x80000000
>> +    cpuid
>> +
>> +    ; This check should fail on Intel or Non SEV AMD CPUs and in future if
>> +    ; Intel CPUs supports this CPUID leaf then we are guranteed to have exact
>> +    ; same bit definition.
>> +    cmp       eax, 0x8000001f
>> +    jl        NoSev
>> +
>> +    ; Check for memory encryption feature:
>> +    ;  CPUID  Fn8000_001F[EAX] - Bit 1
>> +    ;
>> +    mov       eax,  0x8000001f
>> +    cpuid
>> +    bt        eax, 1
>> +    jnc       NoSev
>> +
>> +    ; Check if memory encryption is enabled
>> +    ;  MSR_0xC0010131 - Bit 0 (SEV enabled)
>> +    mov       ecx, 0xc0010131
>> +    rdmsr
>> +    bt        eax, 0
>> +    jnc       NoSev
>> +
>> +    ; Get pte bit position to enable memory encryption
>> +    ; CPUID Fn8000_001F[EBX] - Bits 5:0
>> +    ;
>> +    mov       eax, ebx
>> +    and       eax, 0x3f
>> +    jmp       SevExit
>> +
>> +NoSev:
>> +    xor       eax, eax
>> +
>> +SevExit:
>> +    pop       edx
>> +    pop       ecx
>> +    pop       ebx
>> +    OneTimeCallRet CheckSevFeature
>>   
>>   ;
>> -; Modified:  EAX, ECX
>> +; Modified:  EAX, ECX, EDX
> 
> Maybe you can add EBX here as well and call CheckSevFeature earlier?
> You'd need to make sure we are not trying to preserve anything in
> EBX/EDX in the other VTF-0 code that calls this.
> 
> If that gets unworkable, then we could setup a tiny temp stack in RAM
> near where we are putting the page tables.
> 

I looked at the call sequence from VTF-0 and it seems nothing is getting
preserved in EBX/EDX. I should be able remove those push/pop instructions
and move the call CheckSevFeature in start of SetCr3ForPageTables64.

> -Jordan
> 
>>   ;
>>   SetCr3ForPageTables64:
>>   
>> @@ -60,18 +111,34 @@ clearPageTablesMemoryLoop:
>>       mov     dword[ecx * 4 + PT_ADDR (0) - 4], eax
>>       loop    clearPageTablesMemoryLoop
>>   
>> +    OneTimeCall   CheckSevFeature
>> +    xor     edx, edx
>> +    test    eax, eax
>> +    jz      SevNotActive
>> +
>> +    ; If SEV is enabled, Memory encryption bit is always above 31
>> +    sub     eax, 32
>> +    bts     edx, eax
>> +
>> +SevNotActive:
>> +
>>       ;
>>       ; Top level Page Directory Pointers (1 * 512GB entry)
>>       ;
>>       mov     dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDP_ATTR
>> +    mov     dword[PT_ADDR (4)], edx
>>   
>>       ;
>>       ; Next level Page Directory Pointers (4 * 1GB entries => 4GB)
>>       ;
>>       mov     dword[PT_ADDR (0x1000)], PT_ADDR (0x2000) + PAGE_PDP_ATTR
>> +    mov     dword[PT_ADDR (0x1004)], edx
>>       mov     dword[PT_ADDR (0x1008)], PT_ADDR (0x3000) + PAGE_PDP_ATTR
>> +    mov     dword[PT_ADDR (0x100C)], edx
>>       mov     dword[PT_ADDR (0x1010)], PT_ADDR (0x4000) + PAGE_PDP_ATTR
>> +    mov     dword[PT_ADDR (0x1014)], edx
>>       mov     dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR
>> +    mov     dword[PT_ADDR (0x101C)], edx
>>   
>>       ;
>>       ; Page Table Entries (2048 * 2MB entries => 4GB)
>> @@ -83,6 +150,7 @@ pageTableEntriesLoop:
>>       shl     eax, 21
>>       add     eax, PAGE_2M_PDE_ATTR
>>       mov     [ecx * 8 + PT_ADDR (0x2000 - 8)], eax
>> +    mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
>>       loop    pageTableEntriesLoop
>>   
>>       ;
>> -- 
>> 2.7.4
>>