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

Brijesh Singh posted 10 patches 7 years, 8 months ago
There is a newer version of this series
[edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Brijesh Singh 7 years, 8 months ago
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
+;
+CheckSevFeature:
+    xor       eax, eax
+
+    ; 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
+
+    ; 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
@@ -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
+    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
Re: [edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Laszlo Ersek 7 years, 8 months ago
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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Brijesh Singh 7 years, 8 months ago
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Laszlo Ersek 7 years, 8 months ago
On 03/23/17 16:05, Brijesh Singh wrote:
> On Wed, Mar 22, 2017 at 3:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:

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

Yes, I figured that maybe this "highest supported cpuid leaf" check
would catch Intel processors.

But, what prevents a future Intel processor from exposing such a high
CPUID leaf, for a completely different purpose? Should we not perform
some kind of vendor-id check? (I'm quite unfamiliar with CPUID, as you
can tell, I just seem to recall a comment (from Liming perhaps?) for one
of your MdeModulePkg patches, where he said that directly using a
software-defined CPUID leaf (?) would not be portable across all CPU
vendors, or some such.)

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Brijesh Singh 7 years, 8 months ago
On Thu, Mar 23, 2017 at 11:16 AM, Laszlo Ersek <lersek@redhat.com> wrote:

> On 03/23/17 16:05, Brijesh Singh wrote:
> > On Wed, Mar 22, 2017 at 3:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> >> 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*
>
> Yes, I figured that maybe this "highest supported cpuid leaf" check
> would catch Intel processors.
>
> But, what prevents a future Intel processor from exposing such a high
> CPUID leaf, for a completely different purpose? Should we not perform
> some kind of vendor-id check? (I'm quite unfamiliar with CPUID, as you
> can tell, I just seem to recall a comment (from Liming perhaps?) for one
> of your MdeModulePkg patches, where he said that directly using a
> software-defined CPUID leaf (?) would not be portable across all CPU
> vendors, or some such.)
>
>

I had similar question in my mind.  Before creating the patch I checked
with
AMD architecture team. The response was, CPUID 0x8000_001F definition
is fixed for x86 architecture. So, if Intel ever decides to report this
CPUID leaf
then it will comply to exact same definition.

I think vendor check is weak, e.g user could change the vendor string
through the qemu property which will break this loop. Because of this, I
wanted
to avoid any kind of vendor string or hypervisor defined CPUID checks
inside the code.

BTW, in my previous patches I was using KVM software defined CPUID leaf and
I
got rid of it because hypervisor's are flexiable to define that CPUID in
whatever
way they want.

-Brijesh
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Laszlo Ersek 7 years, 8 months ago
On 03/23/17 17:48, Brijesh Singh wrote:
> On Thu, Mar 23, 2017 at 11:16 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 03/23/17 16:05, Brijesh Singh wrote:
>>> On Wed, Mar 22, 2017 at 3:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>> 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*
>>
>> Yes, I figured that maybe this "highest supported cpuid leaf" check
>> would catch Intel processors.
>>
>> But, what prevents a future Intel processor from exposing such a high
>> CPUID leaf, for a completely different purpose? Should we not perform
>> some kind of vendor-id check? (I'm quite unfamiliar with CPUID, as you
>> can tell, I just seem to recall a comment (from Liming perhaps?) for one
>> of your MdeModulePkg patches, where he said that directly using a
>> software-defined CPUID leaf (?) would not be portable across all CPU
>> vendors, or some such.)
>>
>>
> 
> I had similar question in my mind.  Before creating the patch I checked
> with
> AMD architecture team. The response was, CPUID 0x8000_001F definition
> is fixed for x86 architecture. So, if Intel ever decides to report this
> CPUID leaf
> then it will comply to exact same definition.

OK, this looks safe then. Can you please add it as a comment to the code?

Thanks!
Laszlo

> 
> I think vendor check is weak, e.g user could change the vendor string
> through the qemu property which will break this loop. Because of this, I
> wanted
> to avoid any kind of vendor string or hypervisor defined CPUID checks
> inside the code.
> 
> BTW, in my previous patches I was using KVM software defined CPUID leaf and
> I
> got rid of it because hypervisor's are flexiable to define that CPUID in
> whatever
> way they want.
> 
> -Brijesh
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled
Posted by Brijesh Singh 7 years, 8 months ago
On Thu, Mar 23, 2017 at 11:54 AM, Laszlo Ersek <lersek@redhat.com> >
>
> > I had similar question in my mind.  Before creating the patch I checked
> > with
> > AMD architecture team. The response was, CPUID 0x8000_001F definition
> > is fixed for x86 architecture. So, if Intel ever decides to report this
> > CPUID leaf
> > then it will comply to exact same definition.
>
> OK, this looks safe then. Can you please add it as a comment to the code?
>
>

Sure will do.

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