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

[Brijesh Singh <brijesh.ksingh@gmail.com>]

On Wed, Mar 22, 2017 at 3:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:

> 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".
>
>
WIll do.



> > +;
> > +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?
>
>
The below check should branch to NoSev  on Intel processor
(please note that 0x8000_001F is new leaf and may not exist on older AMD
processor hence we will also branch to NoSev on AMD processor which does
not support this leaf)

*; Check if we have a valid (0x8000_001F) CPUID leaf
**mov       eax, 0x80000000
**cpuid
**cmp       eax, 0x8000001f
**jl        NoSev*


 > +

> +    ; 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, 0xc001013.
> > +    rdmsr
> > +    bt        eax, 0
> > +    jnc       NoSev
>
> Do we need to check SEV-ES specifically? bit1 is not tested.
>
>
We don't need to check SEV-ES bit, I just added it here for documentation
purposes. In current patch series I am not adding any of the ES funtionality
and will be revisit later.


> +
> > +    ; 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.
>
>
Will do. Since cpuid clobbers these registers I can push/pop these inside
the CheckSevFeature.



> > @@ -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
>
>

Ah  good catch.  You are right that I should be using "sub eax, ebx" or
"sub eax, 32". I am scratching my head to figure out why SEV guest was
booting
with this bug. If the mask calculation is wrong then we should be failing
to boot the SEV guest.  But looking at "bts instruction" [1] indicates that
 some
assembler support bit offset larger than 31. Since eax still contains the
original
bit position hence the mask creation logic was still working.

I will fix it.

[1] http://x86.renejeschke.de/html/file_module_x86_id_25.html

Thank you.

-Brijesh


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


-- 
Confusion is always the most honest response.