Re: [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled

[Laszlo Ersek <lersek@redhat.com>]

On 03/21/17 22:12, 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 pre-encrypted by the Qemu launch sequence,
> we must set the C-bit when building the page table for 64-bit or 32-bit
> PAE mode.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm |   62 +++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 6201cad..7083f6b 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -37,6 +37,47 @@ BITS    32
>                         PAGE_READ_WRITE + \
>                         PAGE_PRESENT)
>  
> +; Check if Secure Encrypted Virtualization (SEV) feature
> +;  
> +;  If SEV is enabled, then EAX will contain Memory encryption bit position

I suggest to say, in the comment here, that a valid eax will be at least
32, and 0 on "SEV unavailable or disabled".

> +;
> +CheckSevFeature:
> +    xor       eax, eax

I think this xor may not be necessary (there's no path out of this
function that wouldn't set eax intentionally), but it doesn't hurt.

(Sorry if I should have noticed this in v1 -- there's no need to
resubmit just because of this.)

In the below logic, which branch exactly (to NoSev) will be taken on
Intel processors?

> +
> +    ; Check if we have a valid (0x8000_001F) CPUID leaf
> +    mov       eax, 0x80000000
> +    cpuid
> +    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)
> +    ;  MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
> +    mov       ecx, 0xc0010131
> +    rdmsr
> +    bt        eax, 0
> +    jnc       NoSev

Do we need to check SEV-ES specifically? bit1 is not tested.

> +
> +    ; 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:
> +    OneTimeCallRet CheckSevFeature
>  
>  ;
>  ; Modified:  EAX, ECX

Can you please update (or delete) this comment? I think EBX and EDX may
be modified too.

> @@ -60,18 +101,38 @@ clearPageTablesMemoryLoop:
>      mov     dword[ecx * 4 + PT_ADDR (0) - 4], eax
>      loop    clearPageTablesMemoryLoop
>  
> +    ; Check if its SEV-enabled Guest
> +    ;
> +    OneTimeCall   CheckSevFeature
> +    xor     edx, edx
> +    test    eax, eax
> +    jz      SevNotActive
> +
> +    ; If SEV is enabled, Memory encryption bit is always above 31
> +    mov     ebx, 32
> +    sub     ebx, eax

I apologize if I'm mistaken, but I think we want to decrease eax with 32
(i.e., with ebx), so that we get the C bit's position within the high
address (= more significant) DWORD of the PTE.

But, isn't the argument order for SUB reversed? To me the above seems to
imply EBX := EBX - EAX, which appears wrong.

... I think we could even say

  sub eax, 32

The rest looks good to me.

Thanks
Laszlo

> +    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 +144,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
>  
>      ;
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>