* [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active
2017-03-06 23:27 [RFC PATCH v1 0/5] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
@ 2017-03-06 23:27 ` Brijesh Singh
[not found] ` <3ec1cf2d-952d-97fa-108d-a6c70e613277@amd.com>
2017-03-08 18:38 ` Jordan Justen
2017-03-06 23:27 ` [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library Brijesh Singh
` (3 subsequent siblings)
4 siblings, 2 replies; 33+ messages in thread
From: Brijesh Singh @ 2017-03-06 23:27 UTC (permalink / raw)
To: jordan.l.justen, edk2-devel, lersek
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
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. The C-bit (encryption attribute)
in PTE indicates whether the page is private or shared.
If SEV is active, set the memory encryption attribute while building
the page table.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 52 +++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 6201cad..eaf9732 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -26,6 +26,7 @@ BITS 32
%define PAGE_GLOBAL 0x0100
%define PAGE_2M_MBO 0x080
%define PAGE_2M_PAT 0x01000
+%define KVM_FEATURE_SEV 0x08
%define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
PAGE_ACCESSED + \
@@ -37,6 +38,33 @@ BITS 32
PAGE_READ_WRITE + \
PAGE_PRESENT)
+; Check if Secure Encrypted Virtualization (SEV) feature
+; is enabled in KVM
+;
+; If SEV is enabled, then EAX will contain Memory encryption bit position
+;
+CheckKVMSEVFeature:
+ ; Check for SEV feature
+ ; CPUID KVM_FEATURE - Bit 8
+ mov eax, 0x40000001
+ cpuid
+ bt eax, KVM_FEATURE_SEV
+ jnc NoSev
+
+ ; Get memory encryption information
+ ; CPUID Fn8000_001F[EBX] - Bits 5:0
+ ;
+ mov eax, 0x8000001f
+ cpuid
+ mov eax, ebx
+ and eax, 0x3f
+ jmp SevExit
+
+NoSev:
+ xor eax, eax
+
+SevExit:
+ OneTimeCallRet CheckKVMSEVFeature
;
; Modified: EAX, ECX
@@ -60,18 +88,41 @@ clearPageTablesMemoryLoop:
mov dword[ecx * 4 + PT_ADDR (0) - 4], eax
loop clearPageTablesMemoryLoop
+ ; Check if it SEV-enabled Guest
+ ;
+ OneTimeCall CheckKVMSEVFeature
+ 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)
;
+ ; edx contain the memory encryption bit mask, must be applied
+ ; to upper 31 bit on 64-bit address
+ ;
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 (0x1004)], edx
mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR
+ mov dword[PT_ADDR (0x100C)], edx
;
; Page Table Entries (2048 * 2MB entries => 4GB)
@@ -83,6 +134,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
;
^ permalink raw reply related [flat|nested] 33+ messages in thread[parent not found: <3ec1cf2d-952d-97fa-108d-a6c70e613277@amd.com>]
* Re: [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active
[not found] ` <3ec1cf2d-952d-97fa-108d-a6c70e613277@amd.com>
@ 2017-03-07 16:34 ` Brijesh Singh
2017-03-07 16:35 ` Laszlo Ersek
1 sibling, 0 replies; 33+ messages in thread
From: Brijesh Singh @ 2017-03-07 16:34 UTC (permalink / raw)
To: Tom Lendacky
Cc: edk2-devel, lersek, jordan.l.justen, leo.duran, brijesh.singh
On Mar 7, 2017 10:25 AM, "Tom Lendacky" <thomas.lendacky@amd.com> wrote:
On 3/6/2017 5:27 PM, 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. The C-bit (encryption attribute)
> in PTE indicates whether the page is private or shared.
>
> If SEV is active, set the memory encryption attribute while building
> the page table.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 52
> +++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 6201cad..eaf9732 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -26,6 +26,7 @@ BITS 32
> %define PAGE_GLOBAL 0x0100
> %define PAGE_2M_MBO 0x080
> %define PAGE_2M_PAT 0x01000
> +%define KVM_FEATURE_SEV 0x08
>
> %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
> PAGE_ACCESSED + \
> @@ -37,6 +38,33 @@ BITS 32
> PAGE_READ_WRITE + \
> PAGE_PRESENT)
>
> +; Check if Secure Encrypted Virtualization (SEV) feature
> +; is enabled in KVM
> +;
> +; If SEV is enabled, then EAX will contain Memory encryption bit position
> +;
> +CheckKVMSEVFeature:
> + ; Check for SEV feature
> + ; CPUID KVM_FEATURE - Bit 8
> + mov eax, 0x40000001
> + cpuid
> + bt eax, KVM_FEATURE_SEV
> + jnc NoSev
> +
> + ; Get memory encryption information
> + ; CPUID Fn8000_001F[EBX] - Bits 5:0
> + ;
> + mov eax, 0x8000001f
> + cpuid
> + mov eax, ebx
> + and eax, 0x3f
> + jmp SevExit
> +
> +NoSev:
> + xor eax, eax
> +
> +SevExit:
> + OneTimeCallRet CheckKVMSEVFeature
>
> ;
> ; Modified: EAX, ECX
> @@ -60,18 +88,41 @@ clearPageTablesMemoryLoop:
> mov dword[ecx * 4 + PT_ADDR (0) - 4], eax
> loop clearPageTablesMemoryLoop
>
> + ; Check if it SEV-enabled Guest
> + ;
> + OneTimeCall CheckKVMSEVFeature
> + 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)
> ;
> + ; edx contain the memory encryption bit mask, must be applied
> + ; to upper 31 bit on 64-bit address
> + ;
> 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 (0x1004)], edx
>
Shouldn't this be 0x1014?
Agreed will fix it. Seems like my copy/paste error.
mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR
> + mov dword[PT_ADDR (0x100C)], edx
>
Same here, shouldn't this be 0x101C?
Agreed will fix it.
Thanks,
Tom
> ;
> ; Page Table Entries (2048 * 2MB entries => 4GB)
> @@ -83,6 +134,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
>
> ;
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active
[not found] ` <3ec1cf2d-952d-97fa-108d-a6c70e613277@amd.com>
2017-03-07 16:34 ` Brijesh Singh
@ 2017-03-07 16:35 ` Laszlo Ersek
1 sibling, 0 replies; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-07 16:35 UTC (permalink / raw)
To: Tom Lendacky, Brijesh Singh, jordan.l.justen, edk2-devel
Cc: leo.duran, brijesh.sing
On 03/07/17 17:25, Tom Lendacky wrote:
> On 3/6/2017 5:27 PM, 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. The C-bit (encryption attribute)
>> in PTE indicates whether the page is private or shared.
>>
>> If SEV is active, set the memory encryption attribute while building
>> the page table.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 52
>> +++++++++++++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>>
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index 6201cad..eaf9732 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -26,6 +26,7 @@ BITS 32
>> %define PAGE_GLOBAL 0x0100
>> %define PAGE_2M_MBO 0x080
>> %define PAGE_2M_PAT 0x01000
>> +%define KVM_FEATURE_SEV 0x08
>>
>> %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
>> PAGE_ACCESSED + \
>> @@ -37,6 +38,33 @@ BITS 32
>> PAGE_READ_WRITE + \
>> PAGE_PRESENT)
>>
>> +; Check if Secure Encrypted Virtualization (SEV) feature
>> +; is enabled in KVM
>> +;
>> +; If SEV is enabled, then EAX will contain Memory encryption bit
>> position
>> +;
>> +CheckKVMSEVFeature:
>> + ; Check for SEV feature
>> + ; CPUID KVM_FEATURE - Bit 8
>> + mov eax, 0x40000001
>> + cpuid
>> + bt eax, KVM_FEATURE_SEV
>> + jnc NoSev
>> +
>> + ; Get memory encryption information
>> + ; CPUID Fn8000_001F[EBX] - Bits 5:0
>> + ;
>> + mov eax, 0x8000001f
>> + cpuid
>> + mov eax, ebx
>> + and eax, 0x3f
>> + jmp SevExit
>> +
>> +NoSev:
>> + xor eax, eax
>> +
>> +SevExit:
>> + OneTimeCallRet CheckKVMSEVFeature
>>
>> ;
>> ; Modified: EAX, ECX
>> @@ -60,18 +88,41 @@ clearPageTablesMemoryLoop:
>> mov dword[ecx * 4 + PT_ADDR (0) - 4], eax
>> loop clearPageTablesMemoryLoop
>>
>> + ; Check if it SEV-enabled Guest
>> + ;
>> + OneTimeCall CheckKVMSEVFeature
>> + 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)
>> ;
>> + ; edx contain the memory encryption bit mask, must be applied
>> + ; to upper 31 bit on 64-bit address
>> + ;
>> 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 (0x1004)], edx
>
> Shouldn't this be 0x1014?
>
>> mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR
>> + mov dword[PT_ADDR (0x100C)], edx
>
> Same here, shouldn't this be 0x101C?
Right. Other than that, it looks sane to me.
Thanks
Laszlo
>
> Thanks,
> Tom
>
>>
>> ;
>> ; Page Table Entries (2048 * 2MB entries => 4GB)
>> @@ -83,6 +134,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
>>
>> ;
>>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active
2017-03-06 23:27 ` [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active Brijesh Singh
[not found] ` <3ec1cf2d-952d-97fa-108d-a6c70e613277@amd.com>
@ 2017-03-08 18:38 ` Jordan Justen
2017-03-08 18:42 ` Brijesh Singh
1 sibling, 1 reply; 33+ messages in thread
From: Jordan Justen @ 2017-03-08 18:38 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel, lersek
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
On 2017-03-06 15:27:35, 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. The C-bit (encryption attribute)
> in PTE indicates whether the page is private or shared.
>
> If SEV is active, set the memory encryption attribute while building
> the page table.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 52 +++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 6201cad..eaf9732 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -26,6 +26,7 @@ BITS 32
> %define PAGE_GLOBAL 0x0100
> %define PAGE_2M_MBO 0x080
> %define PAGE_2M_PAT 0x01000
> +%define KVM_FEATURE_SEV 0x08
>
> %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
> PAGE_ACCESSED + \
> @@ -37,6 +38,33 @@ BITS 32
> PAGE_READ_WRITE + \
> PAGE_PRESENT)
>
> +; Check if Secure Encrypted Virtualization (SEV) feature
> +; is enabled in KVM
> +;
> +; If SEV is enabled, then EAX will contain Memory encryption bit position
> +;
> +CheckKVMSEVFeature:
Code style would be CheckKvmSevFeature.
-Jordan
> + ; Check for SEV feature
> + ; CPUID KVM_FEATURE - Bit 8
> + mov eax, 0x40000001
> + cpuid
> + bt eax, KVM_FEATURE_SEV
> + jnc NoSev
> +
> + ; Get memory encryption information
> + ; CPUID Fn8000_001F[EBX] - Bits 5:0
> + ;
> + mov eax, 0x8000001f
> + cpuid
> + mov eax, ebx
> + and eax, 0x3f
> + jmp SevExit
> +
> +NoSev:
> + xor eax, eax
> +
> +SevExit:
> + OneTimeCallRet CheckKVMSEVFeature
>
> ;
> ; Modified: EAX, ECX
> @@ -60,18 +88,41 @@ clearPageTablesMemoryLoop:
> mov dword[ecx * 4 + PT_ADDR (0) - 4], eax
> loop clearPageTablesMemoryLoop
>
> + ; Check if it SEV-enabled Guest
> + ;
> + OneTimeCall CheckKVMSEVFeature
> + 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)
> ;
> + ; edx contain the memory encryption bit mask, must be applied
> + ; to upper 31 bit on 64-bit address
> + ;
> 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 (0x1004)], edx
> mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR
> + mov dword[PT_ADDR (0x100C)], edx
>
> ;
> ; Page Table Entries (2048 * 2MB entries => 4GB)
> @@ -83,6 +134,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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active
2017-03-08 18:38 ` Jordan Justen
@ 2017-03-08 18:42 ` Brijesh Singh
0 siblings, 0 replies; 33+ messages in thread
From: Brijesh Singh @ 2017-03-08 18:42 UTC (permalink / raw)
To: Jordan Justen
Cc: edk2-devel, Laszlo Ersek, Tom Lendacky, Leo Duran, brijesh.singh
On Wed, Mar 8, 2017 at 12:38 PM, Jordan Justen <jordan.l.justen@intel.com>
wrote:
> On 2017-03-06 15:27:35, 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. The C-bit (encryption attribute)
> > in PTE indicates whether the page is private or shared.
> >
> > If SEV is active, set the memory encryption attribute while building
> > the page table.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 52
> +++++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > index 6201cad..eaf9732 100644
> > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > @@ -26,6 +26,7 @@ BITS 32
> > %define PAGE_GLOBAL 0x0100
> > %define PAGE_2M_MBO 0x080
> > %define PAGE_2M_PAT 0x01000
> > +%define KVM_FEATURE_SEV 0x08
> >
> > %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \
> > PAGE_ACCESSED + \
> > @@ -37,6 +38,33 @@ BITS 32
> > PAGE_READ_WRITE + \
> > PAGE_PRESENT)
> >
> > +; Check if Secure Encrypted Virtualization (SEV) feature
> > +; is enabled in KVM
> > +;
> > +; If SEV is enabled, then EAX will contain Memory encryption bit
> position
> > +;
> > +CheckKVMSEVFeature:
>
> Code style would be CheckKvmSevFeature.
>
>
Thanks Jordan, I will fix the coding style in next rev
> > + ; Check for SEV feature
> > + ; CPUID KVM_FEATURE - Bit 8
> > + mov eax, 0x40000001
> > + cpuid
> > + bt eax, KVM_FEATURE_SEV
> > + jnc NoSev
> > +
> > + ; Get memory encryption information
> > + ; CPUID Fn8000_001F[EBX] - Bits 5:0
> > + ;
> > + mov eax, 0x8000001f
> > + cpuid
> > + mov eax, ebx
> > + and eax, 0x3f
> > + jmp SevExit
> > +
> > +NoSev:
> > + xor eax, eax
> > +
> > +SevExit:
> > + OneTimeCallRet CheckKVMSEVFeature
> >
> > ;
> > ; Modified: EAX, ECX
> > @@ -60,18 +88,41 @@ clearPageTablesMemoryLoop:
> > mov dword[ecx * 4 + PT_ADDR (0) - 4], eax
> > loop clearPageTablesMemoryLoop
> >
> > + ; Check if it SEV-enabled Guest
> > + ;
> > + OneTimeCall CheckKVMSEVFeature
> > + 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)
> > ;
> > + ; edx contain the memory encryption bit mask, must be applied
> > + ; to upper 31 bit on 64-bit address
> > + ;
> > 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 (0x1004)], edx
> > mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR
> > + mov dword[PT_ADDR (0x100C)], edx
> >
> > ;
> > ; Page Table Entries (2048 * 2MB entries => 4GB)
> > @@ -83,6 +134,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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-06 23:27 [RFC PATCH v1 0/5] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
2017-03-06 23:27 ` [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active Brijesh Singh
@ 2017-03-06 23:27 ` Brijesh Singh
2017-03-07 17:06 ` Laszlo Ersek
2017-03-06 23:27 ` [RFC PATCH v1 3/5] OvmfPkg/PlatformPei: Initialize SEV support Brijesh Singh
` (2 subsequent siblings)
4 siblings, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-06 23:27 UTC (permalink / raw)
To: jordan.l.justen, edk2-devel, lersek
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
The library contain common helper routines for SEV feature.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Include/Library/MemcryptSevLib.h | 42 +++++++++++++
OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66 +++++++++++++++++++++
OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44 ++++++++++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
OvmfPkg/OvmfPkgX64.dsc | 4 +
5 files changed, 160 insertions(+)
create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h b/OvmfPkg/Include/Library/MemcryptSevLib.h
new file mode 100644
index 0000000..89f9c86
--- /dev/null
+++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
@@ -0,0 +1,42 @@
+/** @file
+ Copyright (C) 2017 Advanced Micro Devices.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#ifndef __MEMCRYPT_SEV_LIB_H__
+#define __MEMCRYPT_SEV_LIB_H__
+
+#include <Uefi/UefiBaseType.h>
+#include <Base.h>
+
+/**
+
+ Initialize SEV memory encryption
+
+**/
+
+RETURN_STATUS
+EFIAPI
+MemcryptSevInitialize (
+ VOID
+ );
+
+/**
+
+ Return TRUE when SEV is active otherwise FALSE
+
+ **/
+BOOLEAN
+EFIAPI
+SevActive (
+ VOID
+ );
+
+#endif
diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
new file mode 100644
index 0000000..2d60b75
--- /dev/null
+++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
@@ -0,0 +1,66 @@
+/** @file
+
+ Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "Uefi.h"
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/MemcryptSevLib.h>
+
+#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
+
+RETURN_STATUS
+EFIAPI
+MemcryptSevInitialize (
+ VOID
+ )
+{
+ UINT32 EBX;
+ UINT64 MeMask = 0;
+
+ if (SevActive ()) {
+ // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit position)
+ AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
+ MeMask = (1ULL << (EBX & 0x3f));
+ DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV) is enabled\n"));
+ DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
+ }
+
+ PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
+
+ return RETURN_SUCCESS;
+}
+
+BOOLEAN
+EFIAPI
+SevActive (
+ VOID
+ )
+{
+ UINT32 KVMFeatures, EAX;
+
+ // Check if KVM memory encyption feature is set
+ AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL);
+ if (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
+
+ // Check whether SEV is enabled
+ // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled)
+ AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
+
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
new file mode 100644
index 0000000..8e8d7e0
--- /dev/null
+++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
@@ -0,0 +1,44 @@
+## @file
+#
+# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.<BR>
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = MemcryptSevLib
+ FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = MemcryptSevLib
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64
+#
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
+ UefiCpuPkg/UefiCpuPkg.dec
+
+[Sources]
+ MemcryptSevLib.c
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ PcdLib
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 56f7ff9..a35e1d2 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -129,6 +129,7 @@
QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
+ MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
!if $(SMM_REQUIRE) == FALSE
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
!endif
@@ -509,6 +510,9 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+ # Set memory encryption mask
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
+
!if $(SMM_REQUIRE) == TRUE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index d0b0b0e..5d853d6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -129,6 +129,7 @@
QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
+ MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
!if $(SMM_REQUIRE) == FALSE
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
!endif
@@ -508,6 +509,9 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+ # Set memory encryption mask
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
+
!if $(SMM_REQUIRE) == TRUE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-06 23:27 ` [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library Brijesh Singh
@ 2017-03-07 17:06 ` Laszlo Ersek
2017-03-07 19:14 ` Brijesh Singh
0 siblings, 1 reply; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-07 17:06 UTC (permalink / raw)
To: Brijesh Singh, jordan.l.justen, edk2-devel
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
On 03/07/17 00:27, Brijesh Singh wrote:
> The library contain common helper routines for SEV feature.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/Include/Library/MemcryptSevLib.h | 42 +++++++++++++
> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66 +++++++++++++++++++++
> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44 ++++++++++++++
> OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
> OvmfPkg/OvmfPkgX64.dsc | 4 +
> 5 files changed, 160 insertions(+)
> create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
> create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
New files -- can you please double-check they are CRLF terminated?
>
> diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h b/OvmfPkg/Include/Library/MemcryptSevLib.h
> new file mode 100644
> index 0000000..89f9c86
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
> @@ -0,0 +1,42 @@
> +/** @file
Please add one or two sentences about the purpose of this library class.
> + Copyright (C) 2017 Advanced Micro Devices.
> +
> + This program and the accompanying materials are licensed and made available
> + under the terms and conditions of the BSD License which accompanies this
> + distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +**/
> +
> +#ifndef __MEMCRYPT_SEV_LIB_H__
> +#define __MEMCRYPT_SEV_LIB_H__
> +
> +#include <Uefi/UefiBaseType.h>
I think it shouldn't be necessary to include this, especially not for a
BASE library (class). What type exactly did you need this include for?
> +#include <Base.h>
> +
> +/**
> +
> + Initialize SEV memory encryption
> +
> +**/
> +
> +RETURN_STATUS
> +EFIAPI
> +MemcryptSevInitialize (
> + VOID
> + );
> +
> +/**
> +
> + Return TRUE when SEV is active otherwise FALSE
> +
> + **/
Is this function restricted to code that runs after a successful
invocation of MemcryptSevInitialize()? If so, please document that.
Please also document the return values (with @retval and/or @return),
even though the explanation is likely trivial.
> +BOOLEAN
> +EFIAPI
> +SevActive (
> + VOID
> + );
> +
I'd prefer if we could use a common prefix for the two functions,
something that is unique to / characteristic of the new library class.
> +#endif
> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> new file mode 100644
> index 0000000..2d60b75
> --- /dev/null
> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> @@ -0,0 +1,66 @@
> +/** @file
> +
> + Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
> + This program and the accompanying materials
> + are licensed and made available under the terms and conditions of the BSD License
> + which accompanies this distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "Uefi.h"
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/MemcryptSevLib.h>
> +
> +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
Can you move all these magic constants (CPUID leaves as well) to
OvmfPkg/Include/IndustryStandard/? I guess the filename could be
"AmdSev.h", or something similar.
Such header files are also prime locations to capture the great
standards references that you added to the blurb.
> +
> +RETURN_STATUS
> +EFIAPI
> +MemcryptSevInitialize (
> + VOID
> + )
> +{
> + UINT32 EBX;
Should be Ebx, IMO.
> + UINT64 MeMask = 0;
Initialization of local variables is forbidden by the edk2 coding style.
> +
> + if (SevActive ()) {
Aha! So it's the other way around than I expected.
> + // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit position)
Comment style is
//
// CPUID ...
//
> + AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
> + MeMask = (1ULL << (EBX & 0x3f));
You can't bit shift 64-bit values in edk2 with the C-language operator;
it won't build for Ia32.
Please either use LShiftU64() here, or -- if you are sure the code will
never be reached in Ia32 guests -- use (UINTN)1, and shift that with <<.
BTW, in what PI phases do you intend to use this library? For example,
in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit, and the DXE
phase runs in 64-bit.
Hm, in the next patch, it seems that the library is put to use in
PlatformPei (and only there). Since these functions are really small, I
think I would prefer having a new file under OvmfPkg/PlatformPei only.
More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in
32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear
active when the 32-bit PlatformPei module queries it?
* If it doesn't, that's a problem, because then the PCD will be set
incorrectly. In this case, it might make sense to limit SEV only to the
X64 build of OVMF.
* If SEV does appear active when the 32-bit PlatformPei module queries
it, then we definitely need to use LShiftU64 here.
Have you tested this series in all three builds of OVMF? (Ia32, Ia32X64,
and X64?) I'm not asking about SMM, I can see it's on the TODO list.
> + DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV) is enabled\n"));
> + DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
> + }
> +
> + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
Whiile we do exped PcdSet64S to succeed, please add a separate Status
variable, and add an ASSERT_RETURN_ERROR on that.
> +
> + return RETURN_SUCCESS;
> +}
> +
> +BOOLEAN
> +EFIAPI
> +SevActive (
> + VOID
> + )
> +{
> + UINT32 KVMFeatures, EAX;
Should be KvmFeatures and Eax.
> +
> + // Check if KVM memory encyption feature is set
comment style
> + AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL);
> + if (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
> +
> + // Check whether SEV is enabled
> + // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled)
> + AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
Tom commented on this -- the result is not checked.
> +
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
> +
> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> new file mode 100644
> index 0000000..8e8d7e0
> --- /dev/null
> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> @@ -0,0 +1,44 @@
> +## @file
> +#
> +# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.<BR>
> +#
> +# This program and the accompanying materials
> +# are licensed and made available under the terms and conditions of the BSD License
> +# which accompanies this distribution. The full text of the license may be found at
> +# http://opensource.org/licenses/bsd-license.php
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = MemcryptSevLib
> + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = MemcryptSevLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64
> +#
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OvmfPkg/OvmfPkg.dec
> + UefiCpuPkg/UefiCpuPkg.dec
> +
> +[Sources]
> + MemcryptSevLib.c
> +
> +[LibraryClasses]
> + BaseLib
> + DebugLib
> + PcdLib
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 56f7ff9..a35e1d2 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -129,6 +129,7 @@
> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> !if $(SMM_REQUIRE) == FALSE
> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> !endif
Please configure your git instance so that it includes the section names
of INI-style files in hunk headers:
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05
see xfuncname there, and:
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09
> @@ -509,6 +510,9 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>
> + # Set memory encryption mask
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> +
> !if $(SMM_REQUIRE) == TRUE
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index d0b0b0e..5d853d6 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -129,6 +129,7 @@
> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> !if $(SMM_REQUIRE) == FALSE
> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> !endif
> @@ -508,6 +509,9 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>
> + # Set memory encryption mask
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> +
> !if $(SMM_REQUIRE) == TRUE
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>
The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is
not resolved for the Ia32 build, hence PlatformPei will fail to build in
the next patch.
Please build and regression-test all three builds of OVMF (SMM disabled,
for now) in the development of this feature.
Regression-testing should in particular include S3 suspend/resume
(again, with SMM disabled, for now). If it breaks (and it is expected to
break), please extend the S3Verification() function so that it prevents
the firmware from booting (with a reasonable error message) if it sees
that SEV is active and S3 was *not* disabled on the QEMU command line.
Losing a guest at S3 resume is very annoying, it's better not to boot in
that case (and to ask the user to disable S3 on the QEMU command line).
Anyhow, I think these functions should go directly under
OvmfPkg/PlatformPei, into a new file.
I may have missed a few things, but I'm (theoretically) at a conference,
and right now it seems more correct for me to give (perhaps spotty)
feedback *quickly*, than to let my backlog pile up.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-07 17:06 ` Laszlo Ersek
@ 2017-03-07 19:14 ` Brijesh Singh
2017-03-07 22:08 ` Laszlo Ersek
0 siblings, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-07 19:14 UTC (permalink / raw)
To: Laszlo Ersek
Cc: jordan.l.justen, edk2-devel, Tom Lendacky, Leo Duran,
brijesh.singh
On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/07/17 00:27, Brijesh Singh wrote:
> > The library contain common helper routines for SEV feature.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> > OvmfPkg/Include/Library/MemcryptSevLib.h | 42 +++++++++++++
> > OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66
> +++++++++++++++++++++
> > OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44 ++++++++++++++
> > OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
> > OvmfPkg/OvmfPkgX64.dsc | 4 +
> > 5 files changed, 160 insertions(+)
> > create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
> > create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> > create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>
> New files -- can you please double-check they are CRLF terminated?
>
>
This version does not have CRLF, I will ensure that nex rev contains CRLF.
> >
> > diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h
> b/OvmfPkg/Include/Library/MemcryptSevLib.h
> > new file mode 100644
> > index 0000000..89f9c86
> > --- /dev/null
> > +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
> > @@ -0,0 +1,42 @@
> > +/** @file
>
> Please add one or two sentences about the purpose of this library class.
Will do.
>
>
> + Copyright (C) 2017 Advanced Micro Devices.
> > +
> > + This program and the accompanying materials are licensed and made
> available
> > + under the terms and conditions of the BSD License which accompanies
> this
> > + distribution. The full text of the license may be found at
> > + http://opensource.org/licenses/bsd-license.php
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> WITHOUT
> > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +**/
> > +
> > +#ifndef __MEMCRYPT_SEV_LIB_H__
> > +#define __MEMCRYPT_SEV_LIB_H__
> > +
> > +#include <Uefi/UefiBaseType.h>
>
> I think it shouldn't be necessary to include this, especially not for a
> BASE library (class). What type exactly did you need this include for?
>
>
I should be able to remove the inclusion of this header.
> > +#include <Base.h>
> > +
> > +/**
> > +
> > + Initialize SEV memory encryption
> > +
> > +**/
> > +
> > +RETURN_STATUS
> > +EFIAPI
> > +MemcryptSevInitialize (
> > + VOID
> > + );
> > +
> > +/**
> > +
> > + Return TRUE when SEV is active otherwise FALSE
> > +
> > + **/
>
> Is this function restricted to code that runs after a successful
> invocation of MemcryptSevInitialize()? If so, please document that.
>
>
Both are independent functions, SevActive() function returns whether SEV is
active or not. Whereas SevInitialize(), calls SevActive() and if SEV is
active then it sets the dynamic PtePcdMemoryEncryptionMask.
Please also document the return values (with @retval and/or @return),
> even though the explanation is likely trivial.
>
> I will document it.
> > +BOOLEAN
> > +EFIAPI
> > +SevActive (
> > + VOID
> > + );
> > +
>
> I'd prefer if we could use a common prefix for the two functions,
> something that is unique to / characteristic of the new library class.
>
>
Will fix it. In general, are we are okay with MemcryptSevLib naming
convensio ? If we are okay with that then I was going to prefix all the
function with Sev e.g
SevInitialize() : this sets the dynamic PCD
SevActive() : returns TRUE when SEV is active
> +#endif
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/MemcryptSevLib.h>
> > +
> > +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
>
> Can you move all these magic constants (CPUID leaves as well) to
> OvmfPkg/Include/IndustryStandard/? I guess the filename could be
> "AmdSev.h", or something similar.
>
> Such header files are also prime locations to capture the great
> standards references that you added to the blurb.
>
>
Will do it.
> > +
> > +RETURN_STATUS
> > +EFIAPI
> > +MemcryptSevInitialize (
> > + VOID
> > + )
> > +{
> > + UINT32 EBX;
>
> Should be Ebx, IMO.
>
>
Will fix it
> + UINT64 MeMask = 0;
>
> Initialization of local variables is forbidden by the edk2 coding style.
>
>
Will fix it.
> > +
> > + if (SevActive ()) {
>
> Aha! So it's the other way around than I expected.
>
> > + // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit
> position)
>
> Comment style is
>
> //
> // CPUID ...
> //
>
>
Will fix it.
> > + AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
> > + MeMask = (1ULL << (EBX & 0x3f));
>
> You can't bit shift 64-bit values in edk2 with the C-language operator;
> it won't build for Ia32.
>
> Please either use LShiftU64() here, or -- if you are sure the code will
> never be reached in Ia32 guests -- use (UINTN)1, and shift that with <<.
>
>
I will fix it to use LShiftU64() version.
> BTW, in what PI phases do you intend to use this library? For example,
> in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit, and the DXE
> phase runs in 64-bit.
>
>
So my rational behind creating a new library was to have a flexibilty of
adding more SEV specifc functions. The functions which I have mind is:
SevChangeMemoryAttributeEncrypted(Address, Length) : set the encryption
attribute
SevChangeMemoryAttributeDecrypted(Address, Length) : clear the encryption
attribute
SevChangeMemoryAttribute*() are not implemented in this RFC and I am
working to add in next rev. This will be mainly execute in Dxe phase. These
functions will be update the pagetable to clear/set encryption masks. It
will be mainly used for Dma libraries and possibly QemuVideoDxe (since
framebuffer is shared between HV and Guest hence we will need to clear the
encryption attribute of framebuffer memory).
I would potentially add two libraries:
* SevBaseLib: it provides SevActive() and SevInitialize() routines which
can be called anytime
* SevDxeLib: it will provide routines which can be called used by Dxe
drivers.
Does it make sense to you. I am open to suggestions.
Hm, in the next patch, it seems that the library is put to use in
> PlatformPei (and only there). Since these functions are really small, I
> think I would prefer having a new file under OvmfPkg/PlatformPei only.
>
> More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in
> 32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear
> active when the 32-bit PlatformPei module queries it?
>
> Yes SEV will appear active on both Ia32X64 and X64. I have tried running
the OVMF build with both version.
> * If it doesn't, that's a problem, because then the PCD will be set
> incorrectly. In this case, it might make sense to limit SEV only to the
> X64 build of OVMF.
>
> * If SEV does appear active when the 32-bit PlatformPei module queries
> it, then we definitely need to use LShiftU64 here.
>
> Have you tested this series in all three builds of OVMF? (Ia32, Ia32X64,
> and X64?) I'm not asking about SMM, I can see it's on the TODO list.
>
> I have tried Ia3264 and X64 but have not tried Ia32. I will try to and let
you know If I find any issues.
> > + DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV) is
> enabled\n"));
> > + DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
> > + }
> > +
> > + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
>
> Whiile we do exped PcdSet64S to succeed, please add a separate Status
> variable, and add an ASSERT_RETURN_ERROR on that.
>
>
Will do
> +
> > + return RETURN_SUCCESS;
> > +}
> > +
> > +BOOLEAN
> > +EFIAPI
> > +SevActive (
> > + VOID
> > + )
> > +{
> > + UINT32 KVMFeatures, EAX;
>
> Should be KvmFeatures and Eax.
>
> > +
> > + // Check if KVM memory encyption feature is set
>
> comment style
>
> > + AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL);
> > + if (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
> > +
> > + // Check whether SEV is enabled
> > + // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled)
> > + AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
>
> Tom commented on this -- the result is not checked.
>
>
Will fix it.
> > +
> > + return TRUE;
> > + }
> > +
> > + return FALSE;
> > +}
> > +
> > diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> > new file mode 100644
> > index 0000000..8e8d7e0
> > --- /dev/null
> > +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> > @@ -0,0 +1,44 @@
> > +## @file
> > +#
> > +# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.<BR>
> > +#
> > +# This program and the accompanying materials
> > +# are licensed and made available under the terms and conditions of
> the BSD License
> > +# which accompanies this distribution. The full text of the license
> may be found at
> > +# http://opensource.org/licenses/bsd-license.php
> > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> > +#
> > +#
> > +##
> > +
> > +[Defines]
> > + INF_VERSION = 0x00010005
> > + BASE_NAME = MemcryptSevLib
> > + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b
> > + MODULE_TYPE = BASE
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = MemcryptSevLib
> > +
> > +#
> > +# The following information is for reference only and not required by
> the build tools.
> > +#
> > +# VALID_ARCHITECTURES = IA32 X64
> > +#
> > +
> > +[Packages]
> > + MdePkg/MdePkg.dec
> > + MdeModulePkg/MdeModulePkg.dec
> > + OvmfPkg/OvmfPkg.dec
> > + UefiCpuPkg/UefiCpuPkg.dec
> > +
> > +[Sources]
> > + MemcryptSevLib.c
> > +
> > +[LibraryClasses]
> > + BaseLib
> > + DebugLib
> > + PcdLib
> > +
> > +[Pcd]
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> > index 56f7ff9..a35e1d2 100644
> > --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> > @@ -129,6 +129,7 @@
> > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> > + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> > !if $(SMM_REQUIRE) == FALSE
> > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> > !endif
>
> Please configure your git instance so that it includes the section names
> of INI-style files in hunk headers:
>
> https://github.com/tianocore/tianocore.github.io/wiki/
> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
> maintainers#contrib-05
>
> see xfuncname there, and:
>
> https://github.com/tianocore/tianocore.github.io/wiki/
> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
> maintainers#contrib-09
>
>
I will go through INI-Style file and git setting etc.
>
> > @@ -509,6 +510,9 @@
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> >
> > + # Set memory encryption mask
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
> rMask|0x0
> > +
> > !if $(SMM_REQUIRE) == TRUE
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> > index d0b0b0e..5d853d6 100644
> > --- a/OvmfPkg/OvmfPkgX64.dsc
> > +++ b/OvmfPkg/OvmfPkgX64.dsc
> > @@ -129,6 +129,7 @@
> > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> > + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> > !if $(SMM_REQUIRE) == FALSE
> > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> > !endif
> > @@ -508,6 +509,9 @@
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> >
> > + # Set memory encryption mask
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
> rMask|0x0
> > +
> > !if $(SMM_REQUIRE) == TRUE
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
> >
>
> The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is
> not resolved for the Ia32 build, hence PlatformPei will fail to build in
> the next patch.
>
> Please build and regression-test all three builds of OVMF (SMM disabled,
> for now) in the development of this feature.
>
> Regression-testing should in particular include S3 suspend/resume
> (again, with SMM disabled, for now). If it breaks (and it is expected to
> break), please extend the S3Verification() function so that it prevents
> the firmware from booting (with a reasonable error message) if it sees
> that SEV is active and S3 was *not* disabled on the QEMU command line.
> Losing a guest at S3 resume is very annoying, it's better not to boot in
> that case (and to ask the user to disable S3 on the QEMU command line).
>
> Anyhow, I think these functions should go directly under
> OvmfPkg/PlatformPei, into a new file.
>
> I may have missed a few things, but I'm (theoretically) at a conference,
> and right now it seems more correct for me to give (perhaps spotty)
> feedback *quickly*, than to let my backlog pile up.
>
>
Appriciate your quick feedbacks, I will go through each of them and will
try to address in v2.
> Thanks!
> Laszlo
>
--
Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-07 19:14 ` Brijesh Singh
@ 2017-03-07 22:08 ` Laszlo Ersek
2017-03-07 22:36 ` Brijesh Singh
2017-03-08 14:56 ` Duran, Leo
0 siblings, 2 replies; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-07 22:08 UTC (permalink / raw)
To: Brijesh Singh
Cc: jordan.l.justen, edk2-devel, Leo Duran, brijesh.singh,
Tom Lendacky
On 03/07/17 20:14, Brijesh Singh wrote:
> On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 03/07/17 00:27, Brijesh Singh wrote:
>>> The library contain common helper routines for SEV feature.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> ---
>>> OvmfPkg/Include/Library/MemcryptSevLib.h | 42 +++++++++++++
>>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66
>> +++++++++++++++++++++
>>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44 ++++++++++++++
>>> OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
>>> OvmfPkg/OvmfPkgX64.dsc | 4 +
>>> 5 files changed, 160 insertions(+)
>>> create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
>>> create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
>>> create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>
>> New files -- can you please double-check they are CRLF terminated?
>>
>>
> This version does not have CRLF, I will ensure that nex rev contains CRLF.
>
>
>>>
>>> diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h
>> b/OvmfPkg/Include/Library/MemcryptSevLib.h
>>> new file mode 100644
>>> index 0000000..89f9c86
>>> --- /dev/null
>>> +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
>>> @@ -0,0 +1,42 @@
>>> +/** @file
>>
>> Please add one or two sentences about the purpose of this library class.
>
>
> Will do.
>
>
>>
>>
>> + Copyright (C) 2017 Advanced Micro Devices.
>>> +
>>> + This program and the accompanying materials are licensed and made
>> available
>>> + under the terms and conditions of the BSD License which accompanies
>> this
>>> + distribution. The full text of the license may be found at
>>> + http://opensource.org/licenses/bsd-license.php
>>> +
>>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> WITHOUT
>>> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>> +**/
>>> +
>>> +#ifndef __MEMCRYPT_SEV_LIB_H__
>>> +#define __MEMCRYPT_SEV_LIB_H__
>>> +
>>> +#include <Uefi/UefiBaseType.h>
>>
>> I think it shouldn't be necessary to include this, especially not for a
>> BASE library (class). What type exactly did you need this include for?
>>
>>
> I should be able to remove the inclusion of this header.
>
>
>>> +#include <Base.h>
>>> +
>>> +/**
>>> +
>>> + Initialize SEV memory encryption
>>> +
>>> +**/
>>> +
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +MemcryptSevInitialize (
>>> + VOID
>>> + );
>>> +
>>> +/**
>>> +
>>> + Return TRUE when SEV is active otherwise FALSE
>>> +
>>> + **/
>>
>> Is this function restricted to code that runs after a successful
>> invocation of MemcryptSevInitialize()? If so, please document that.
>>
>>
> Both are independent functions, SevActive() function returns whether SEV is
> active or not. Whereas SevInitialize(), calls SevActive() and if SEV is
> active then it sets the dynamic PtePcdMemoryEncryptionMask.
>
>
> Please also document the return values (with @retval and/or @return),
>> even though the explanation is likely trivial.
>>
>> I will document it.
>
>
>
>>> +BOOLEAN
>>> +EFIAPI
>>> +SevActive (
>>> + VOID
>>> + );
>>> +
>>
>> I'd prefer if we could use a common prefix for the two functions,
>> something that is unique to / characteristic of the new library class.
>>
>>
> Will fix it. In general, are we are okay with MemcryptSevLib naming
> convensio ?
Regarding library naming conventions, we can consider functions, and
library instance names.
For functions, I tend to prefer a common prefix, although I don't
believe this is a hard requirement in edk2.
For library instance names, the naming convention seems to be:
<PHASE>LibClassName<VARIANT>
Where <PHASE> describes the phases the library instance can be used in
(so, Base, Dxe, Pei, PeiDxe, and so on), and <VARIANT> describes, very
tersely, the underlying implementation (for example, Null means the
library does nothing). I think <VARIANT> can be omitted if there's only
one library instance.
> If we are okay with that then I was going to prefix all the
> function with Sev e.g
>
> SevInitialize() : this sets the dynamic PCD
> SevActive() : returns TRUE when SEV is active
>
>> +#endif
>>> +#include <Library/BaseLib.h>
>>> +#include <Library/DebugLib.h>
>>> +#include <Library/PcdLib.h>
>>> +#include <Library/MemcryptSevLib.h>
>>> +
>>> +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
>>
>> Can you move all these magic constants (CPUID leaves as well) to
>> OvmfPkg/Include/IndustryStandard/? I guess the filename could be
>> "AmdSev.h", or something similar.
>>
>> Such header files are also prime locations to capture the great
>> standards references that you added to the blurb.
>>
>>
> Will do it.
>
>
>>> +
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +MemcryptSevInitialize (
>>> + VOID
>>> + )
>>> +{
>>> + UINT32 EBX;
>>
>> Should be Ebx, IMO.
>>
>>
> Will fix it
>
>> + UINT64 MeMask = 0;
>>
>> Initialization of local variables is forbidden by the edk2 coding style.
>>
>>
> Will fix it.
>
>
>>> +
>>> + if (SevActive ()) {
>>
>> Aha! So it's the other way around than I expected.
>>
>>> + // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit
>> position)
>>
>> Comment style is
>>
>> //
>> // CPUID ...
>> //
>>
>>
> Will fix it.
>
>
>>> + AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
>>> + MeMask = (1ULL << (EBX & 0x3f));
>>
>> You can't bit shift 64-bit values in edk2 with the C-language operator;
>> it won't build for Ia32.
>>
>> Please either use LShiftU64() here, or -- if you are sure the code will
>> never be reached in Ia32 guests -- use (UINTN)1, and shift that with <<.
>>
>>
> I will fix it to use LShiftU64() version.
>
>
>> BTW, in what PI phases do you intend to use this library? For example,
>> in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit, and the DXE
>> phase runs in 64-bit.
>>
>>
> So my rational behind creating a new library was to have a flexibilty of
> adding more SEV specifc functions. The functions which I have mind is:
>
> SevChangeMemoryAttributeEncrypted(Address, Length) : set the encryption
> attribute
> SevChangeMemoryAttributeDecrypted(Address, Length) : clear the encryption
> attribute
>
> SevChangeMemoryAttribute*() are not implemented in this RFC and I am
> working to add in next rev. This will be mainly execute in Dxe phase. These
> functions will be update the pagetable to clear/set encryption masks. It
> will be mainly used for Dma libraries and possibly QemuVideoDxe (since
> framebuffer is shared between HV and Guest hence we will need to clear the
> encryption attribute of framebuffer memory).
>
> I would potentially add two libraries:
>
> * SevBaseLib: it provides SevActive() and SevInitialize() routines which
> can be called anytime
> * SevDxeLib: it will provide routines which can be called used by Dxe
> drivers.
>
> Does it make sense to you. I am open to suggestions.
I think SevActive() and SevInitialize() should become part of
PlatformPei only (if that's possible).
The upcoming PTE massaging functions could become part of the DMA lib
stuff that you mention (as functions with external linkage), and then
you could pull the DMA lib into QemuVideoDxe just to make these
functions available.
Presently the suggested functions don't seem to justify two (or even
one) new libclass.
Thanks
Laszlo
>
> Hm, in the next patch, it seems that the library is put to use in
>> PlatformPei (and only there). Since these functions are really small, I
>> think I would prefer having a new file under OvmfPkg/PlatformPei only.
>>
>> More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in
>> 32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear
>> active when the 32-bit PlatformPei module queries it?
>>
>> Yes SEV will appear active on both Ia32X64 and X64. I have tried running
> the OVMF build with both version.
>
>
>> * If it doesn't, that's a problem, because then the PCD will be set
>> incorrectly. In this case, it might make sense to limit SEV only to the
>> X64 build of OVMF.
>>
>> * If SEV does appear active when the 32-bit PlatformPei module queries
>> it, then we definitely need to use LShiftU64 here.
>>
>> Have you tested this series in all three builds of OVMF? (Ia32, Ia32X64,
>> and X64?) I'm not asking about SMM, I can see it's on the TODO list.
>>
>> I have tried Ia3264 and X64 but have not tried Ia32. I will try to and let
> you know If I find any issues.
>
>
>>> + DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV) is
>> enabled\n"));
>>> + DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
>>> + }
>>> +
>>> + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
>>
>> Whiile we do exped PcdSet64S to succeed, please add a separate Status
>> variable, and add an ASSERT_RETURN_ERROR on that.
>>
>>
> Will do
>
>> +
>>> + return RETURN_SUCCESS;
>>> +}
>>> +
>>> +BOOLEAN
>>> +EFIAPI
>>> +SevActive (
>>> + VOID
>>> + )
>>> +{
>>> + UINT32 KVMFeatures, EAX;
>>
>> Should be KvmFeatures and Eax.
>>
>>> +
>>> + // Check if KVM memory encyption feature is set
>>
>> comment style
>>
>>> + AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL);
>>> + if (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
>>> +
>>> + // Check whether SEV is enabled
>>> + // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled)
>>> + AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
>>
>> Tom commented on this -- the result is not checked.
>>
>>
> Will fix it.
>
>
>>> +
>>> + return TRUE;
>>> + }
>>> +
>>> + return FALSE;
>>> +}
>>> +
>>> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>> b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>> new file mode 100644
>>> index 0000000..8e8d7e0
>>> --- /dev/null
>>> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>> @@ -0,0 +1,44 @@
>>> +## @file
>>> +#
>>> +# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.<BR>
>>> +#
>>> +# This program and the accompanying materials
>>> +# are licensed and made available under the terms and conditions of
>> the BSD License
>>> +# which accompanies this distribution. The full text of the license
>> may be found at
>>> +# http://opensource.org/licenses/bsd-license.php
>>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>>> +#
>>> +#
>>> +##
>>> +
>>> +[Defines]
>>> + INF_VERSION = 0x00010005
>>> + BASE_NAME = MemcryptSevLib
>>> + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b
>>> + MODULE_TYPE = BASE
>>> + VERSION_STRING = 1.0
>>> + LIBRARY_CLASS = MemcryptSevLib
>>> +
>>> +#
>>> +# The following information is for reference only and not required by
>> the build tools.
>>> +#
>>> +# VALID_ARCHITECTURES = IA32 X64
>>> +#
>>> +
>>> +[Packages]
>>> + MdePkg/MdePkg.dec
>>> + MdeModulePkg/MdeModulePkg.dec
>>> + OvmfPkg/OvmfPkg.dec
>>> + UefiCpuPkg/UefiCpuPkg.dec
>>> +
>>> +[Sources]
>>> + MemcryptSevLib.c
>>> +
>>> +[LibraryClasses]
>>> + BaseLib
>>> + DebugLib
>>> + PcdLib
>>> +
>>> +[Pcd]
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index 56f7ff9..a35e1d2 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -129,6 +129,7 @@
>>> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>>> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>>> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>>> + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>> !if $(SMM_REQUIRE) == FALSE
>>> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>>> !endif
>>
>> Please configure your git instance so that it includes the section names
>> of INI-style files in hunk headers:
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/
>> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
>> maintainers#contrib-05
>>
>> see xfuncname there, and:
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/
>> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
>> maintainers#contrib-09
>>
>>
>
> I will go through INI-Style file and git setting etc.
>
>
>>
>>> @@ -509,6 +510,9 @@
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>>
>>> + # Set memory encryption mask
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
>> rMask|0x0
>>> +
>>> !if $(SMM_REQUIRE) == TRUE
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index d0b0b0e..5d853d6 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -129,6 +129,7 @@
>>> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>>> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>>> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>>> + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>> !if $(SMM_REQUIRE) == FALSE
>>> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>>> !endif
>>> @@ -508,6 +509,9 @@
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>>
>>> + # Set memory encryption mask
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
>> rMask|0x0
>>> +
>>> !if $(SMM_REQUIRE) == TRUE
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>>>
>>
>> The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is
>> not resolved for the Ia32 build, hence PlatformPei will fail to build in
>> the next patch.
>>
>> Please build and regression-test all three builds of OVMF (SMM disabled,
>> for now) in the development of this feature.
>>
>> Regression-testing should in particular include S3 suspend/resume
>> (again, with SMM disabled, for now). If it breaks (and it is expected to
>> break), please extend the S3Verification() function so that it prevents
>> the firmware from booting (with a reasonable error message) if it sees
>> that SEV is active and S3 was *not* disabled on the QEMU command line.
>> Losing a guest at S3 resume is very annoying, it's better not to boot in
>> that case (and to ask the user to disable S3 on the QEMU command line).
>>
>> Anyhow, I think these functions should go directly under
>> OvmfPkg/PlatformPei, into a new file.
>>
>> I may have missed a few things, but I'm (theoretically) at a conference,
>> and right now it seems more correct for me to give (perhaps spotty)
>> feedback *quickly*, than to let my backlog pile up.
>>
>>
> Appriciate your quick feedbacks, I will go through each of them and will
> try to address in v2.
>
>
>> Thanks!
>> Laszlo
>>
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-07 22:08 ` Laszlo Ersek
@ 2017-03-07 22:36 ` Brijesh Singh
2017-03-08 8:40 ` Laszlo Ersek
2017-03-08 14:56 ` Duran, Leo
1 sibling, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-07 22:36 UTC (permalink / raw)
To: Laszlo Ersek
Cc: jordan.l.justen, edk2-devel, Leo Duran, brijesh.singh,
Tom Lendacky
On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/07/17 20:14, Brijesh Singh wrote:
> > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> I think SevActive() and SevInitialize() should become part of
> PlatformPei only (if that's possible).
>
> The upcoming PTE massaging functions could become part of the DMA lib
> stuff that you mention (as functions with external linkage), and then
> you could pull the DMA lib into QemuVideoDxe just to make these
> functions available.
>
> Presently the suggested functions don't seem to justify two (or even
> one) new libclass.
>
I think I should be able to accomate SevInitialize() and SevActive()
function inside the PlatformPei. I will drop MemcryptSevLib library in next
rev.
I will go with your idea for adding PTE massaging function directly inside
the DMA library and will link that into QemuVideoDxe.
Only part which I have not yet figured out, how to deal with Qemu FW_CFG
DMA support, I believe some of FW_CFG DMA read and write
happens fairly early (PEI stage). The PTE massaging code may need to
allocate memory and not sure how to allocate dynamic memory in early stages.
Any pointers ?
Thanks
Brijesh
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-07 22:36 ` Brijesh Singh
@ 2017-03-08 8:40 ` Laszlo Ersek
2017-03-17 2:02 ` Brijesh Singh
0 siblings, 1 reply; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-08 8:40 UTC (permalink / raw)
To: Brijesh Singh
Cc: jordan.l.justen, edk2-devel, Leo Duran, brijesh.singh,
Tom Lendacky
On 03/07/17 23:36, Brijesh Singh wrote:
>
>
> On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
>
> On 03/07/17 20:14, Brijesh Singh wrote:
> > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> I think SevActive() and SevInitialize() should become part of
> PlatformPei only (if that's possible).
>
> The upcoming PTE massaging functions could become part of the DMA lib
> stuff that you mention (as functions with external linkage), and then
> you could pull the DMA lib into QemuVideoDxe just to make these
> functions available.
>
> Presently the suggested functions don't seem to justify two (or even
> one) new libclass.
>
>
>
> I think I should be able to accomate SevInitialize() and SevActive()
> function inside the PlatformPei. I will drop MemcryptSevLib library in
> next rev.
>
> I will go with your idea for adding PTE massaging function directly
> inside the DMA library and will link that into QemuVideoDxe.
>
> Only part which I have not yet figured out, how to deal with Qemu
> FW_CFG DMA support, I believe some of FW_CFG DMA read and write
> happens fairly early (PEI stage).
That's right, off the top of my head, minimally PlatformPei uses fw_cfg
heavily during PEI.
> The PTE massaging code may need to
> allocate memory and not sure how to allocate dynamic memory in early stages.
> Any pointers ?
You can use MemoryAllocationLib functions for that (such as
AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib
class for the PEI phase like this:
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
and the PeiMemoryAllocationLib instance maps those functions to the PEI
services.
A few important things about this:
- AllocatePool() works up to only ~64KB in size, and the allocation is
backed by a new HOB. Generally speaking, the HOB may be moved to a
different spot in memory before entering the DXE phase, so pointers
returned by such AllocatePool() calls (in PEI) are not safe to
dereference in the DXE phase.
- FreePool() does nothing, the allocated memory (the HOB, see above) is
only released when the guest OS starts (and it drops all boot services
data type memory).
- AllocatePages() works as it says on the tin, and the pointer returned
by it is safe to dereference in DXE.
- FreePages() however is again a no-op, it practically leaks the memory,
and only the guest OS will be able to release it (see FreePool() above).
One workaround for this could be to stash the address of the PEI-phase
allocation in a GUID HOB or a PCD, and then let some DXE driver in the
DXE phase release the memory with gBS->FreePages(). I'm not sure though
if this complexity is worth it.
- Note that it is PlatformPei itself that installs the permanent PEI
RAM. Before that happens, PEIMs (including PlatformPei itself) can only
allocate memory from the temporary SEC/PEI heap, which is very very
small, and only AllocatePool() would work at that point (AllocatePages()
wouldn't). However, if you place the AllocatePages() function call after
PublishPeiMemory(), then things should work.
As far as I can see, you added MemcryptSevInitialize() to the end of
InitializePlatform(); allocating pages at that point should be fine.
- During S3 resume, a different (pre-reserved) memory area is used as
permanent PEI RAM, which is quite smaller than the one used during
normal boot. It means that, if you need a lot of memory for setting up
SEV during S3 resume, AllocatePages() might run out of memory, and we
might have to resize the pre-reservation mentioned above.
- If you could reasonably bound the allocation size with a constant, it
might be simplest to use static arrays / variables. Those would be
dropped as soon as the PEI phase was exited. As one quirk however, you
should not rely on such variables being zero-initialized during S3 resume.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-08 8:40 ` Laszlo Ersek
@ 2017-03-17 2:02 ` Brijesh Singh
2017-03-17 10:29 ` Laszlo Ersek
0 siblings, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-17 2:02 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Justen, Jordan L, edk2-devel, Leo Duran, brijesh.singh,
Tom Lendacky
On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/07/17 23:36, Brijesh Singh wrote:
> >
> >
> > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> >
> > On 03/07/17 20:14, Brijesh Singh wrote:
> > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> > I think SevActive() and SevInitialize() should become part of
> > PlatformPei only (if that's possible).
> >
> > The upcoming PTE massaging functions could become part of the DMA lib
> > stuff that you mention (as functions with external linkage), and then
> > you could pull the DMA lib into QemuVideoDxe just to make these
> > functions available.
> >
> > Presently the suggested functions don't seem to justify two (or even
> > one) new libclass.
> >
> >
> >
> > I think I should be able to accomate SevInitialize() and SevActive()
> > function inside the PlatformPei. I will drop MemcryptSevLib library in
> > next rev.
> >
> > I will go with your idea for adding PTE massaging function directly
> > inside the DMA library and will link that into QemuVideoDxe.
> >
> > Only part which I have not yet figured out, how to deal with Qemu
> > FW_CFG DMA support, I believe some of FW_CFG DMA read and write
> > happens fairly early (PEI stage).
>
> That's right, off the top of my head, minimally PlatformPei uses fw_cfg
> heavily during PEI.
>
> > The PTE massaging code may need to
> > allocate memory and not sure how to allocate dynamic memory in early
> stages.
> > Any pointers ?
>
> You can use MemoryAllocationLib functions for that (such as
> AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib
> class for the PEI phase like this:
>
>
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/
> PeiMemoryAllocationLib.inf
>
> and the PeiMemoryAllocationLib instance maps those functions to the PEI
> services.
>
> A few important things about this:
>
> - AllocatePool() works up to only ~64KB in size, and the allocation is
> backed by a new HOB. Generally speaking, the HOB may be moved to a
> different spot in memory before entering the DXE phase, so pointers
> returned by such AllocatePool() calls (in PEI) are not safe to
> dereference in the DXE phase.
>
> - FreePool() does nothing, the allocated memory (the HOB, see above) is
> only released when the guest OS starts (and it drops all boot services
> data type memory).
>
> - AllocatePages() works as it says on the tin, and the pointer returned
> by it is safe to dereference in DXE.
>
>
I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib.
But I found that QemuFWCfgLib is used in both PEI and Dxe phases.
It makes things interesting, in SEV guest we can perform DMA operation only
when processor is either in 32-bit PAE or long mode (mainly because C-bit
is not accessiable in 32 or 16-bit mode). It limits us to using OvmfX64.dsc.
Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and
OvmfPkgX64.dsc.
In my code browsing so far I have found that only QemuFwCfgLib does DMA
operations in PEI phase and other packages perform the DMA operation in DXE
phase.
If we can somehow manage to not require the DMA support in PEI phase then we
should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc.
How about defaulting to I/O operations in PEI stage for SEV guest ? I do
understand
that QEMU prefers us to use DMA interface for FwCfg write.
Additionally, I found that some FwCfg DMA access happens before
PublishPeiMemory()
hence AllocatePages() was failing to allocate the bounce buffer for SEV DMA.
I was thinking that for SEV guest if we get a request to perform smaller
reads or writes
(maybe < 64 bytes) then silently fallback to IO else perform DMA operations.
Thoughts ?
-Brijesh
- FreePages() however is again a no-op, it practically leaks the memory,
> and only the guest OS will be able to release it (see FreePool() above).
> One workaround for this could be to stash the address of the PEI-phase
> allocation in a GUID HOB or a PCD, and then let some DXE driver in the
> DXE phase release the memory with gBS->FreePages(). I'm not sure though
> if this complexity is worth it.
>
> - Note that it is PlatformPei itself that installs the permanent PEI
> RAM. Before that happens, PEIMs (including PlatformPei itself) can only
> allocate memory from the temporary SEC/PEI heap, which is very very
> small, and only AllocatePool() would work at that point (AllocatePages()
> wouldn't). However, if you place the AllocatePages() function call after
> PublishPeiMemory(), then things should work.
>
> As far as I can see, you added MemcryptSevInitialize() to the end of
> InitializePlatform(); allocating pages at that point should be fine.
>
> - During S3 resume, a different (pre-reserved) memory area is used as
> permanent PEI RAM, which is quite smaller than the one used during
> normal boot. It means that, if you need a lot of memory for setting up
> SEV during S3 resume, AllocatePages() might run out of memory, and we
> might have to resize the pre-reservation mentioned above.
>
> - If you could reasonably bound the allocation size with a constant, it
> might be simplest to use static arrays / variables. Those would be
> dropped as soon as the PEI phase was exited. As one quirk however, you
> should not rely on such variables being zero-initialized during S3 resume.
>
> Thanks
> Laszlo
>
--
Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-17 2:02 ` Brijesh Singh
@ 2017-03-17 10:29 ` Laszlo Ersek
2017-03-17 14:08 ` Brijesh Singh
0 siblings, 1 reply; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-17 10:29 UTC (permalink / raw)
To: Brijesh Singh
Cc: Justen, Jordan L, edk2-devel, Leo Duran, brijesh.singh,
Tom Lendacky
On 03/17/17 03:02, Brijesh Singh wrote:
>
>
> On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
>
> On 03/07/17 23:36, Brijesh Singh wrote:
> >
> >
> > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>
> > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
> >
> > On 03/07/17 20:14, Brijesh Singh wrote:
> > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>
> > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
> > I think SevActive() and SevInitialize() should become part of
> > PlatformPei only (if that's possible).
> >
> > The upcoming PTE massaging functions could become part of the DMA lib
> > stuff that you mention (as functions with external linkage), and then
> > you could pull the DMA lib into QemuVideoDxe just to make these
> > functions available.
> >
> > Presently the suggested functions don't seem to justify two (or even
> > one) new libclass.
> >
> >
> >
> > I think I should be able to accomate SevInitialize() and SevActive()
> > function inside the PlatformPei. I will drop MemcryptSevLib library in
> > next rev.
> >
> > I will go with your idea for adding PTE massaging function directly
> > inside the DMA library and will link that into QemuVideoDxe.
> >
> > Only part which I have not yet figured out, how to deal with Qemu
> > FW_CFG DMA support, I believe some of FW_CFG DMA read and write
> > happens fairly early (PEI stage).
>
> That's right, off the top of my head, minimally PlatformPei uses fw_cfg
> heavily during PEI.
>
> > The PTE massaging code may need to
> > allocate memory and not sure how to allocate dynamic memory in early stages.
> > Any pointers ?
>
> You can use MemoryAllocationLib functions for that (such as
> AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib
> class for the PEI phase like this:
>
>
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>
> and the PeiMemoryAllocationLib instance maps those functions to the PEI
> services.
>
> A few important things about this:
>
> - AllocatePool() works up to only ~64KB in size, and the allocation is
> backed by a new HOB. Generally speaking, the HOB may be moved to a
> different spot in memory before entering the DXE phase, so pointers
> returned by such AllocatePool() calls (in PEI) are not safe to
> dereference in the DXE phase.
>
> - FreePool() does nothing, the allocated memory (the HOB, see above) is
> only released when the guest OS starts (and it drops all boot services
> data type memory).
>
> - AllocatePages() works as it says on the tin, and the pointer returned
> by it is safe to dereference in DXE.
>
>
>
> I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib.
> But I found that QemuFWCfgLib is used in both PEI and Dxe phases.
> It makes things interesting, in SEV guest we can perform DMA operation only
> when processor is either in 32-bit PAE or long mode (mainly because C-bit
> is not accessiable in 32 or 16-bit mode). It limits us to using OvmfX64.dsc.
>
> Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and
> OvmfPkgX64.dsc.
> In my code browsing so far I have found that only QemuFwCfgLib does DMA
> operations in PEI phase and other packages perform the DMA operation in
> DXE phase.
> If we can somehow manage to not require the DMA support in PEI phase then we
> should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc.
> How about defaulting to I/O operations in PEI stage for SEV guest ?
I suggest the following:
(1) Introduce an internal API to "QemuFwCfgLibInternal.h", called
InternalQemuFwCfgSevIsEnabled().
(2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c"
without using global variables (i.e., with a CPUID on each call, on AMD
processors, and return constant FALSE on Intel processors).
(3) Create two copies of "QemuFwCfgPeiDxe.c", using the following names:
QemuFwCfgPei.c, QemuFwCfgDxe.c, and then delete the original.
(4) Accordingly, create two copies of "QemuFwCfgLib.inf",
QemuFwCfgPeiLib.inf and QemuFwCfgDxeLib.inf, then delete the original.
Don't forget to update the FILE_GUID defines.
Additionally, update the library client module type restrictions in each
(near LIBRARY_CLASS).
(5) In QemuFwCfgDxeLib.inf / QemuFwCfgDxe.c, the constructor function
should detect SEV, and store the result in a global variable. (As far as
I remember, this detection should only happen on AMD processors, on
Intel processors, the CPUID should not even be attempted -- is that right?)
The InternalQemuFwCfgSevIsEnabled() function should be implemented to
return the global variable.
(6) In QemuFwCfgPeiLib.inf / QemuFwCfgPei.c,
InternalQemuFwCfgSevIsEnabled() should be implemented the same way, but
the constructor function should also avoid setting
mQemuFwCfgDmaSupported to TRUE if SEV is detected.
(7) In "QemuFwCfgLib.c", the InternalQemuFwCfgDmaBytes() function should
use a bounce buffer if InternalQemuFwCfgSevIsEnabled() returns TRUE.
(Unfortunately, InternalQemuFwCfgDmaBytes() is not capable of returning
an error, so if the bounce buffer allocation fails, you'll have to call
ASSERT_EFI_ERROR (Status), and then an explicit CpuDeadLoop() too.)
Note that skip operations never need a bounce buffer.
The end result is that PEIMs will fall back to "IO only" if SEV is
enabled (*), while DXE clients will use bounce buffers with DMA if SEV
is enabled.
(*) It's OK to use less performant solutions in PEI.
> I do
> understand
> that QEMU prefers us to use DMA interface for FwCfg write.
... It's also OK to use less functional solutions in PEI. Normally, no
fw_cfg write occurs in the PEI phase.
Unfortunately, during S3 resume, fw_cfg writes (with DMA) do occur in
PEI, performed by the stashed BootScriptExecutorDxe driver. And, in the
S3 boot script, you cannot allocate memory (for bounce buffering)
*dynamically*.
However, in the S3 boot script, you cannot allocate memory dynamically
for any other purpose either!
Which is why QemuFwCfgS3Lib already pre-allocates reserved scratch space
anyway, for the boot script opcodes (and QEMU) to operate upon.
Thus, it should be possible to customize that too -- find the
AllocateReservedPool() call in "QemuFwCfgS3Dxe.c", and make it allocate
non-encrypted full pages instead, if SEV is enabled (you can call CPUID
right there, on AMD processors).
This way, whenever a DXE driver saves fw_cfg DMA writes into the ACPI S3
boot script, to be run on S3 resume, the scratch space that it allocates
in advance (for both the DMA control struct and the data to transfer),
via QemuFwCfgS3CallWhenBootScriptReady(), will be plain-text.
>
> Additionally, I found that some FwCfg DMA access happens before
> PublishPeiMemory()
> hence AllocatePages() was failing to allocate the bounce buffer for SEV DMA.
> I was thinking that for SEV guest if we get a request to perform smaller
> reads or writes
> (maybe < 64 bytes) then silently fallback to IO else perform DMA operations.
This should not be necessary with the above approach.
(In general, AllocatePages() should not be used for temporary purposes
in PEI (for bounce buffering or anything else), because those pages will
be leaked for the rest of the firmware (unless something in DXE
explicitly frees them up).)
Thanks!
Laszlo
>
> Thoughts ?
>
> -Brijesh
>
> - FreePages() however is again a no-op, it practically leaks the memory,
>
> and only the guest OS will be able to release it (see FreePool() above).
> One workaround for this could be to stash the address of the PEI-phase
> allocation in a GUID HOB or a PCD, and then let some DXE driver in the
> DXE phase release the memory with gBS->FreePages(). I'm not sure though
> if this complexity is worth it.
>
> - Note that it is PlatformPei itself that installs the permanent PEI
> RAM. Before that happens, PEIMs (including PlatformPei itself) can only
> allocate memory from the temporary SEC/PEI heap, which is very very
> small, and only AllocatePool() would work at that point (AllocatePages()
> wouldn't). However, if you place the AllocatePages() function call after
> PublishPeiMemory(), then things should work.
>
> As far as I can see, you added MemcryptSevInitialize() to the end of
> InitializePlatform(); allocating pages at that point should be fine.
>
> - During S3 resume, a different (pre-reserved) memory area is used as
> permanent PEI RAM, which is quite smaller than the one used during
> normal boot. It means that, if you need a lot of memory for setting up
> SEV during S3 resume, AllocatePages() might run out of memory, and we
> might have to resize the pre-reservation mentioned above.
>
> - If you could reasonably bound the allocation size with a constant, it
> might be simplest to use static arrays / variables. Those would be
> dropped as soon as the PEI phase was exited. As one quirk however, you
> should not rely on such variables being zero-initialized during S3
> resume.
>
> Thanks
> Laszlo
>
>
>
>
> --
> Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-17 10:29 ` Laszlo Ersek
@ 2017-03-17 14:08 ` Brijesh Singh
0 siblings, 0 replies; 33+ messages in thread
From: Brijesh Singh @ 2017-03-17 14:08 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Justen, Jordan L, edk2-devel, Leo Duran, brijesh.singh,
Tom Lendacky
On Fri, Mar 17, 2017 at 5:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/17/17 03:02, Brijesh Singh wrote:
> >
> >
> > On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> >
> > On 03/07/17 23:36, Brijesh Singh wrote:
> > >
> > >
> > > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>
> > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
> > >
> > > On 03/07/17 20:14, Brijesh Singh wrote:
> > > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <
> lersek@redhat.com <mailto:lersek@redhat.com>
> > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
> > > I think SevActive() and SevInitialize() should become part of
> > > PlatformPei only (if that's possible).
> > >
> > > The upcoming PTE massaging functions could become part of the
> DMA lib
> > > stuff that you mention (as functions with external linkage),
> and then
> > > you could pull the DMA lib into QemuVideoDxe just to make these
> > > functions available.
> > >
> > > Presently the suggested functions don't seem to justify two
> (or even
> > > one) new libclass.
> > >
> > >
> > >
> > > I think I should be able to accomate SevInitialize() and
> SevActive()
> > > function inside the PlatformPei. I will drop MemcryptSevLib
> library in
> > > next rev.
> > >
> > > I will go with your idea for adding PTE massaging function directly
> > > inside the DMA library and will link that into QemuVideoDxe.
> > >
> > > Only part which I have not yet figured out, how to deal with Qemu
> > > FW_CFG DMA support, I believe some of FW_CFG DMA read and write
> > > happens fairly early (PEI stage).
> >
> > That's right, off the top of my head, minimally PlatformPei uses
> fw_cfg
> > heavily during PEI.
> >
> > > The PTE massaging code may need to
> > > allocate memory and not sure how to allocate dynamic memory in
> early stages.
> > > Any pointers ?
> >
> > You can use MemoryAllocationLib functions for that (such as
> > AllocatePool() and AllocatePages()). The OVMF DSC files resolve the
> lib
> > class for the PEI phase like this:
> >
> >
> > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/
> PeiMemoryAllocationLib.inf
> >
> > and the PeiMemoryAllocationLib instance maps those functions to the
> PEI
> > services.
> >
> > A few important things about this:
> >
> > - AllocatePool() works up to only ~64KB in size, and the allocation
> is
> > backed by a new HOB. Generally speaking, the HOB may be moved to a
> > different spot in memory before entering the DXE phase, so pointers
> > returned by such AllocatePool() calls (in PEI) are not safe to
> > dereference in the DXE phase.
> >
> > - FreePool() does nothing, the allocated memory (the HOB, see above)
> is
> > only released when the guest OS starts (and it drops all boot
> services
> > data type memory).
> >
> > - AllocatePages() works as it says on the tin, and the pointer
> returned
> > by it is safe to dereference in DXE.
> >
> >
> >
> > I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib.
> > But I found that QemuFWCfgLib is used in both PEI and Dxe phases.
> > It makes things interesting, in SEV guest we can perform DMA operation
> only
> > when processor is either in 32-bit PAE or long mode (mainly because C-bit
> > is not accessiable in 32 or 16-bit mode). It limits us to using
> OvmfX64.dsc.
> >
> > Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and
> > OvmfPkgX64.dsc.
> > In my code browsing so far I have found that only QemuFwCfgLib does DMA
> > operations in PEI phase and other packages perform the DMA operation in
> > DXE phase.
> > If we can somehow manage to not require the DMA support in PEI phase
> then we
> > should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc.
> > How about defaulting to I/O operations in PEI stage for SEV guest ?
>
> I suggest the following:
>
> (1) Introduce an internal API to "QemuFwCfgLibInternal.h", called
> InternalQemuFwCfgSevIsEnabled().
>
> (2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c"
> without using global variables (i.e., with a CPUID on each call, on AMD
> processors, and return constant FALSE on Intel processors).
>
> (3) Create two copies of "QemuFwCfgPeiDxe.c", using the following names:
> QemuFwCfgPei.c, QemuFwCfgDxe.c, and then delete the original.
>
> (4) Accordingly, create two copies of "QemuFwCfgLib.inf",
> QemuFwCfgPeiLib.inf and QemuFwCfgDxeLib.inf, then delete the original.
>
> Don't forget to update the FILE_GUID defines.
>
> Additionally, update the library client module type restrictions in each
> (near LIBRARY_CLASS).
>
> (5) In QemuFwCfgDxeLib.inf / QemuFwCfgDxe.c, the constructor function
> should detect SEV, and store the result in a global variable. (As far as
> I remember, this detection should only happen on AMD processors, on
> Intel processors, the CPUID should not even be attempted -- is that right?)
>
>
Yes that is right. The CPUID detection function will return TRUE only when
SEV is enabled on AMD processor. On Intel it will return FALSE.
> The InternalQemuFwCfgSevIsEnabled() function should be implemented to
> return the global variable.
>
> (6) In QemuFwCfgPeiLib.inf / QemuFwCfgPei.c,
> InternalQemuFwCfgSevIsEnabled() should be implemented the same way, but
> the constructor function should also avoid setting
> mQemuFwCfgDmaSupported to TRUE if SEV is detected.
>
> (7) In "QemuFwCfgLib.c", the InternalQemuFwCfgDmaBytes() function should
> use a bounce buffer if InternalQemuFwCfgSevIsEnabled() returns TRUE.
>
> (Unfortunately, InternalQemuFwCfgDmaBytes() is not capable of returning
> an error, so if the bounce buffer allocation fails, you'll have to call
> ASSERT_EFI_ERROR (Status), and then an explicit CpuDeadLoop() too.)
>
> Note that skip operations never need a bounce buffer.
>
>
> The end result is that PEIMs will fall back to "IO only" if SEV is
> enabled (*), while DXE clients will use bounce buffers with DMA if SEV
> is enabled.
>
> (*) It's OK to use less performant solutions in PEI.
>
> > I do
> > understand
> > that QEMU prefers us to use DMA interface for FwCfg write.
>
> ... It's also OK to use less functional solutions in PEI. Normally, no
> fw_cfg write occurs in the PEI phase.
>
> Unfortunately, during S3 resume, fw_cfg writes (with DMA) do occur in
> PEI, performed by the stashed BootScriptExecutorDxe driver. And, in the
> S3 boot script, you cannot allocate memory (for bounce buffering)
> *dynamically*.
>
> However, in the S3 boot script, you cannot allocate memory dynamically
> for any other purpose either!
>
> Which is why QemuFwCfgS3Lib already pre-allocates reserved scratch space
> anyway, for the boot script opcodes (and QEMU) to operate upon.
>
> Thus, it should be possible to customize that too -- find the
> AllocateReservedPool() call in "QemuFwCfgS3Dxe.c", and make it allocate
> non-encrypted full pages instead, if SEV is enabled (you can call CPUID
> right there, on AMD processors).
>
> This way, whenever a DXE driver saves fw_cfg DMA writes into the ACPI S3
> boot script, to be run on S3 resume, the scratch space that it allocates
> in advance (for both the DMA control struct and the data to transfer),
> via QemuFwCfgS3CallWhenBootScriptReady(), will be plain-text.
>
> >
> > Additionally, I found that some FwCfg DMA access happens before
> > PublishPeiMemory()
> > hence AllocatePages() was failing to allocate the bounce buffer for SEV
> DMA.
> > I was thinking that for SEV guest if we get a request to perform smaller
> > reads or writes
> > (maybe < 64 bytes) then silently fallback to IO else perform DMA
> operations.
>
> This should not be necessary with the above approach.
>
> (In general, AllocatePages() should not be used for temporary purposes
> in PEI (for bounce buffering or anything else), because those pages will
> be leaked for the rest of the firmware (unless something in DXE
> explicitly frees them up).)
>
> Thanks!
> Laszlo
>
>
> >
> > Thoughts ?
>
>
> >
> > -Brijesh
> >
> > - FreePages() however is again a no-op, it practically leaks the memory,
> >
> > and only the guest OS will be able to release it (see FreePool()
> above).
> > One workaround for this could be to stash the address of the
> PEI-phase
> > allocation in a GUID HOB or a PCD, and then let some DXE driver in
> the
> > DXE phase release the memory with gBS->FreePages(). I'm not sure
> though
> > if this complexity is worth it.
> >
> > - Note that it is PlatformPei itself that installs the permanent PEI
> > RAM. Before that happens, PEIMs (including PlatformPei itself) can
> only
> > allocate memory from the temporary SEC/PEI heap, which is very very
> > small, and only AllocatePool() would work at that point
> (AllocatePages()
> > wouldn't). However, if you place the AllocatePages() function call
> after
> > PublishPeiMemory(), then things should work.
> >
> > As far as I can see, you added MemcryptSevInitialize() to the end of
> > InitializePlatform(); allocating pages at that point should be fine.
> >
> > - During S3 resume, a different (pre-reserved) memory area is used as
> > permanent PEI RAM, which is quite smaller than the one used during
> > normal boot. It means that, if you need a lot of memory for setting
> up
> > SEV during S3 resume, AllocatePages() might run out of memory, and we
> > might have to resize the pre-reservation mentioned above.
> >
> > - If you could reasonably bound the allocation size with a constant,
> it
> > might be simplest to use static arrays / variables. Those would be
> > dropped as soon as the PEI phase was exited. As one quirk however,
> you
> > should not rely on such variables being zero-initialized during S3
> > resume.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >
> > --
> > Confusion is always the most honest response.
>
>
--
Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-07 22:08 ` Laszlo Ersek
2017-03-07 22:36 ` Brijesh Singh
@ 2017-03-08 14:56 ` Duran, Leo
2017-03-08 15:19 ` Laszlo Ersek
1 sibling, 1 reply; 33+ messages in thread
From: Duran, Leo @ 2017-03-08 14:56 UTC (permalink / raw)
To: 'Laszlo Ersek', Brijesh Singh
Cc: jordan.l.justen@intel.com, edk2-devel@ml01.01.org, Singh, Brijesh,
Lendacky, Thomas
For libraries, I've noticed this usage pattern:
INF File:
- Name: <BASE_NAME>.inf
- Path: xxxPkg/Library/<BASE_NAME>/<BASE_NAME>.inf
INCLUDE File:
- Name: <LIBRARY_CLASS>.h
- Path: xxxPkg/Include/Library/<LIBRARY_CLASS>.h
Leo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, March 07, 2017 4:08 PM
> To: Brijesh Singh <brijesh.ksingh@gmail.com>
> Cc: jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Duran, Leo
> <leo.duran@amd.com>; Singh, Brijesh <brijesh.singh@amd.com>; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>
> Subject: Re: [edk2] [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV
> helper library
>
> On 03/07/17 20:14, Brijesh Singh wrote:
> > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> >> On 03/07/17 00:27, Brijesh Singh wrote:
> >>> The library contain common helper routines for SEV feature.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> >>> ---
> >>> OvmfPkg/Include/Library/MemcryptSevLib.h | 42
> +++++++++++++
> >>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66
> >> +++++++++++++++++++++
> >>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44
> ++++++++++++++
> >>> OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
> >>> OvmfPkg/OvmfPkgX64.dsc | 4 +
> >>> 5 files changed, 160 insertions(+)
> >>> create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
> >>> create mode 100644
> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> >>> create mode 100644
> >>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> >>
> >> New files -- can you please double-check they are CRLF terminated?
> >>
> >>
> > This version does not have CRLF, I will ensure that nex rev contains CRLF.
> >
> >
> >>>
> >>> diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h
> >> b/OvmfPkg/Include/Library/MemcryptSevLib.h
> >>> new file mode 100644
> >>> index 0000000..89f9c86
> >>> --- /dev/null
> >>> +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
> >>> @@ -0,0 +1,42 @@
> >>> +/** @file
> >>
> >> Please add one or two sentences about the purpose of this library class.
> >
> >
> > Will do.
> >
> >
> >>
> >>
> >> + Copyright (C) 2017 Advanced Micro Devices.
> >>> +
> >>> + This program and the accompanying materials are licensed and made
> >> available
> >>> + under the terms and conditions of the BSD License which
> >>> + accompanies
> >> this
> >>> + distribution. The full text of the license may be found at
> >>> + http://opensource.org/licenses/bsd-license.php
> >>> +
> >>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
> IS"
> >>> + BASIS,
> >> WITHOUT
> >>> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> >>> +**/
> >>> +
> >>> +#ifndef __MEMCRYPT_SEV_LIB_H__
> >>> +#define __MEMCRYPT_SEV_LIB_H__
> >>> +
> >>> +#include <Uefi/UefiBaseType.h>
> >>
> >> I think it shouldn't be necessary to include this, especially not for
> >> a BASE library (class). What type exactly did you need this include for?
> >>
> >>
> > I should be able to remove the inclusion of this header.
> >
> >
> >>> +#include <Base.h>
> >>> +
> >>> +/**
> >>> +
> >>> + Initialize SEV memory encryption
> >>> +
> >>> +**/
> >>> +
> >>> +RETURN_STATUS
> >>> +EFIAPI
> >>> +MemcryptSevInitialize (
> >>> + VOID
> >>> + );
> >>> +
> >>> +/**
> >>> +
> >>> + Return TRUE when SEV is active otherwise FALSE
> >>> +
> >>> + **/
> >>
> >> Is this function restricted to code that runs after a successful
> >> invocation of MemcryptSevInitialize()? If so, please document that.
> >>
> >>
> > Both are independent functions, SevActive() function returns whether
> > SEV is active or not. Whereas SevInitialize(), calls SevActive() and
> > if SEV is active then it sets the dynamic PtePcdMemoryEncryptionMask.
> >
> >
> > Please also document the return values (with @retval and/or @return),
> >> even though the explanation is likely trivial.
> >>
> >> I will document it.
> >
> >
> >
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +SevActive (
> >>> + VOID
> >>> + );
> >>> +
> >>
> >> I'd prefer if we could use a common prefix for the two functions,
> >> something that is unique to / characteristic of the new library class.
> >>
> >>
> > Will fix it. In general, are we are okay with MemcryptSevLib naming
> > convensio ?
>
> Regarding library naming conventions, we can consider functions, and library
> instance names.
>
> For functions, I tend to prefer a common prefix, although I don't believe this
> is a hard requirement in edk2.
>
> For library instance names, the naming convention seems to be:
>
> <PHASE>LibClassName<VARIANT>
>
> Where <PHASE> describes the phases the library instance can be used in (so,
> Base, Dxe, Pei, PeiDxe, and so on), and <VARIANT> describes, very tersely,
> the underlying implementation (for example, Null means the library does
> nothing). I think <VARIANT> can be omitted if there's only one library
> instance.
>
>
> > If we are okay with that then I was going to prefix all the function
> > with Sev e.g
> >
> > SevInitialize() : this sets the dynamic PCD
> > SevActive() : returns TRUE when SEV is active
> >
> >> +#endif
> >>> +#include <Library/BaseLib.h>
> >>> +#include <Library/DebugLib.h>
> >>> +#include <Library/PcdLib.h>
> >>> +#include <Library/MemcryptSevLib.h>
> >>> +
> >>> +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
> >>
> >> Can you move all these magic constants (CPUID leaves as well) to
> >> OvmfPkg/Include/IndustryStandard/? I guess the filename could be
> >> "AmdSev.h", or something similar.
> >>
> >> Such header files are also prime locations to capture the great
> >> standards references that you added to the blurb.
> >>
> >>
> > Will do it.
> >
> >
> >>> +
> >>> +RETURN_STATUS
> >>> +EFIAPI
> >>> +MemcryptSevInitialize (
> >>> + VOID
> >>> + )
> >>> +{
> >>> + UINT32 EBX;
> >>
> >> Should be Ebx, IMO.
> >>
> >>
> > Will fix it
> >
> >> + UINT64 MeMask = 0;
> >>
> >> Initialization of local variables is forbidden by the edk2 coding style.
> >>
> >>
> > Will fix it.
> >
> >
> >>> +
> >>> + if (SevActive ()) {
> >>
> >> Aha! So it's the other way around than I expected.
> >>
> >>> + // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit
> >> position)
> >>
> >> Comment style is
> >>
> >> //
> >> // CPUID ...
> >> //
> >>
> >>
> > Will fix it.
> >
> >
> >>> + AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
> >>> + MeMask = (1ULL << (EBX & 0x3f));
> >>
> >> You can't bit shift 64-bit values in edk2 with the C-language
> >> operator; it won't build for Ia32.
> >>
> >> Please either use LShiftU64() here, or -- if you are sure the code
> >> will never be reached in Ia32 guests -- use (UINTN)1, and shift that with
> <<.
> >>
> >>
> > I will fix it to use LShiftU64() version.
> >
> >
> >> BTW, in what PI phases do you intend to use this library? For
> >> example, in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit,
> >> and the DXE phase runs in 64-bit.
> >>
> >>
> > So my rational behind creating a new library was to have a flexibilty
> > of adding more SEV specifc functions. The functions which I have mind is:
> >
> > SevChangeMemoryAttributeEncrypted(Address, Length) : set the
> > encryption attribute SevChangeMemoryAttributeDecrypted(Address,
> > Length) : clear the encryption attribute
> >
> > SevChangeMemoryAttribute*() are not implemented in this RFC and I am
> > working to add in next rev. This will be mainly execute in Dxe phase.
> > These functions will be update the pagetable to clear/set encryption
> > masks. It will be mainly used for Dma libraries and possibly
> > QemuVideoDxe (since framebuffer is shared between HV and Guest
> hence
> > we will need to clear the encryption attribute of framebuffer memory).
> >
> > I would potentially add two libraries:
> >
> > * SevBaseLib: it provides SevActive() and SevInitialize() routines
> > which can be called anytime
> > * SevDxeLib: it will provide routines which can be called used by Dxe
> > drivers.
> >
> > Does it make sense to you. I am open to suggestions.
>
> I think SevActive() and SevInitialize() should become part of PlatformPei only
> (if that's possible).
>
> The upcoming PTE massaging functions could become part of the DMA lib
> stuff that you mention (as functions with external linkage), and then you
> could pull the DMA lib into QemuVideoDxe just to make these functions
> available.
>
> Presently the suggested functions don't seem to justify two (or even
> one) new libclass.
>
> Thanks
> Laszlo
>
> >
> > Hm, in the next patch, it seems that the library is put to use in
> >> PlatformPei (and only there). Since these functions are really small,
> >> I think I would prefer having a new file under OvmfPkg/PlatformPei only.
> >>
> >> More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in
> >> 32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear
> >> active when the 32-bit PlatformPei module queries it?
> >>
> >> Yes SEV will appear active on both Ia32X64 and X64. I have tried
> >> running
> > the OVMF build with both version.
> >
> >
> >> * If it doesn't, that's a problem, because then the PCD will be set
> >> incorrectly. In this case, it might make sense to limit SEV only to
> >> the
> >> X64 build of OVMF.
> >>
> >> * If SEV does appear active when the 32-bit PlatformPei module
> >> queries it, then we definitely need to use LShiftU64 here.
> >>
> >> Have you tested this series in all three builds of OVMF? (Ia32,
> >> Ia32X64, and X64?) I'm not asking about SMM, I can see it's on the TODO
> list.
> >>
> >> I have tried Ia3264 and X64 but have not tried Ia32. I will try to
> >> and let
> > you know If I find any issues.
> >
> >
> >>> + DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV)
> >>> + is
> >> enabled\n"));
> >>> + DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
> }
> >>> +
> >>> + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
> >>
> >> Whiile we do exped PcdSet64S to succeed, please add a separate Status
> >> variable, and add an ASSERT_RETURN_ERROR on that.
> >>
> >>
> > Will do
> >
> >> +
> >>> + return RETURN_SUCCESS;
> >>> +}
> >>> +
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +SevActive (
> >>> + VOID
> >>> + )
> >>> +{
> >>> + UINT32 KVMFeatures, EAX;
> >>
> >> Should be KvmFeatures and Eax.
> >>
> >>> +
> >>> + // Check if KVM memory encyption feature is set
> >>
> >> comment style
> >>
> >>> + AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL); if
> >>> + (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
> >>> +
> >>> + // Check whether SEV is enabled
> >>> + // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled)
> >>> + AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
> >>
> >> Tom commented on this -- the result is not checked.
> >>
> >>
> > Will fix it.
> >
> >
> >>> +
> >>> + return TRUE;
> >>> + }
> >>> +
> >>> + return FALSE;
> >>> +}
> >>> +
> >>> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> >> b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> >>> new file mode 100644
> >>> index 0000000..8e8d7e0
> >>> --- /dev/null
> >>> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> >>> @@ -0,0 +1,44 @@
> >>> +## @file
> >>> +#
> >>> +# Copyright (c) 2017 Advanced Micro Devices. All rights
> >>> +reserved.<BR> # # This program and the accompanying materials #
> >>> +are licensed and made available under the terms and conditions of
> >> the BSD License
> >>> +# which accompanies this distribution. The full text of the
> >>> +license
> >> may be found at
> >>> +# http://opensource.org/licenses/bsd-license.php
> >>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
> IS"
> >>> +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER
> >>> +EXPRESS OR
> >> IMPLIED.
> >>> +#
> >>> +#
> >>> +##
> >>> +
> >>> +[Defines]
> >>> + INF_VERSION = 0x00010005
> >>> + BASE_NAME = MemcryptSevLib
> >>> + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b
> >>> + MODULE_TYPE = BASE
> >>> + VERSION_STRING = 1.0
> >>> + LIBRARY_CLASS = MemcryptSevLib
> >>> +
> >>> +#
> >>> +# The following information is for reference only and not required
> >>> +by
> >> the build tools.
> >>> +#
> >>> +# VALID_ARCHITECTURES = IA32 X64
> >>> +#
> >>> +
> >>> +[Packages]
> >>> + MdePkg/MdePkg.dec
> >>> + MdeModulePkg/MdeModulePkg.dec
> >>> + OvmfPkg/OvmfPkg.dec
> >>> + UefiCpuPkg/UefiCpuPkg.dec
> >>> +
> >>> +[Sources]
> >>> + MemcryptSevLib.c
> >>> +
> >>> +[LibraryClasses]
> >>> + BaseLib
> >>> + DebugLib
> >>> + PcdLib
> >>> +
> >>> +[Pcd]
> >>> +
> >>>
> +gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOr
> Mask
> >>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc
> b/OvmfPkg/OvmfPkgIa32X64.dsc
> >>> index 56f7ff9..a35e1d2 100644
> >>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> >>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> >>> @@ -129,6 +129,7 @@
> >>> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> >>> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> >>> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> >>> +
> MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> >>> !if $(SMM_REQUIRE) == FALSE
> >>> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> >>> !endif
> >>
> >> Please configure your git instance so that it includes the section
> >> names of INI-style files in hunk headers:
> >>
> >> https://github.com/tianocore/tianocore.github.io/wiki/
> >> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
> >> maintainers#contrib-05
> >>
> >> see xfuncname there, and:
> >>
> >> https://github.com/tianocore/tianocore.github.io/wiki/
> >> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
> >> maintainers#contrib-09
> >>
> >>
> >
> > I will go through INI-Style file and git setting etc.
> >
> >
> >>
> >>> @@ -509,6 +510,9 @@
> >>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> >>>
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> >>>
> >>> + # Set memory encryption mask
> >>> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
> >> rMask|0x0
> >>> +
> >>> !if $(SMM_REQUIRE) == TRUE
> >>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> >>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
> >>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index
> >>> d0b0b0e..5d853d6 100644
> >>> --- a/OvmfPkg/OvmfPkgX64.dsc
> >>> +++ b/OvmfPkg/OvmfPkgX64.dsc
> >>> @@ -129,6 +129,7 @@
> >>> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> >>> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> >>> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> >>> +
> MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
> >>> !if $(SMM_REQUIRE) == FALSE
> >>> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> >>> !endif
> >>> @@ -508,6 +509,9 @@
> >>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> >>>
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> >>>
> >>> + # Set memory encryption mask
> >>> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
> >> rMask|0x0
> >>> +
> >>> !if $(SMM_REQUIRE) == TRUE
> >>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> >>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
> >>>
> >>
> >> The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is
> >> not resolved for the Ia32 build, hence PlatformPei will fail to build
> >> in the next patch.
> >>
> >> Please build and regression-test all three builds of OVMF (SMM
> >> disabled, for now) in the development of this feature.
> >>
> >> Regression-testing should in particular include S3 suspend/resume
> >> (again, with SMM disabled, for now). If it breaks (and it is expected
> >> to break), please extend the S3Verification() function so that it
> >> prevents the firmware from booting (with a reasonable error message)
> >> if it sees that SEV is active and S3 was *not* disabled on the QEMU
> command line.
> >> Losing a guest at S3 resume is very annoying, it's better not to boot
> >> in that case (and to ask the user to disable S3 on the QEMU command
> line).
> >>
> >> Anyhow, I think these functions should go directly under
> >> OvmfPkg/PlatformPei, into a new file.
> >>
> >> I may have missed a few things, but I'm (theoretically) at a
> >> conference, and right now it seems more correct for me to give
> >> (perhaps spotty) feedback *quickly*, than to let my backlog pile up.
> >>
> >>
> > Appriciate your quick feedbacks, I will go through each of them and
> > will try to address in v2.
> >
> >
> >> Thanks!
> >> Laszlo
> >>
> >
> >
> >
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library
2017-03-08 14:56 ` Duran, Leo
@ 2017-03-08 15:19 ` Laszlo Ersek
0 siblings, 0 replies; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-08 15:19 UTC (permalink / raw)
To: Duran, Leo, Brijesh Singh
Cc: jordan.l.justen@intel.com, edk2-devel@ml01.01.org, Singh, Brijesh,
Lendacky, Thomas
On 03/08/17 15:56, Duran, Leo wrote:
> For libraries, I've noticed this usage pattern:
>
> INF File:
> - Name: <BASE_NAME>.inf
> - Path: xxxPkg/Library/<BASE_NAME>/<BASE_NAME>.inf
>
> INCLUDE File:
> - Name: <LIBRARY_CLASS>.h
> - Path: xxxPkg/Include/Library/<LIBRARY_CLASS>.h
Correct.
What I described earlier was the relationship between <LIBRARY_CLASS>
and <BASE_NAME>. Namely, <BASE_NAME> is specific to the library
instance, and if you have multiple instances for the same library class,
then there is a convention for composing <BASE_NAME> from <LIBRARY_CLASS>:
<BASE_NAME> := <PHASE><LIBRARY_CLASS><VARIANT>
Laszlo
>
> Leo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, March 07, 2017 4:08 PM
>> To: Brijesh Singh <brijesh.ksingh@gmail.com>
>> Cc: jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Duran, Leo
>> <leo.duran@amd.com>; Singh, Brijesh <brijesh.singh@amd.com>; Lendacky,
>> Thomas <Thomas.Lendacky@amd.com>
>> Subject: Re: [edk2] [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV
>> helper library
>>
>> On 03/07/17 20:14, Brijesh Singh wrote:
>>> On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 03/07/17 00:27, Brijesh Singh wrote:
>>>>> The library contain common helper routines for SEV feature.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>>> ---
>>>>> OvmfPkg/Include/Library/MemcryptSevLib.h | 42
>> +++++++++++++
>>>>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66
>>>> +++++++++++++++++++++
>>>>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44
>> ++++++++++++++
>>>>> OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
>>>>> OvmfPkg/OvmfPkgX64.dsc | 4 +
>>>>> 5 files changed, 160 insertions(+)
>>>>> create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
>>>>> create mode 100644
>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
>>>>> create mode 100644
>>>>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>
>>>> New files -- can you please double-check they are CRLF terminated?
>>>>
>>>>
>>> This version does not have CRLF, I will ensure that nex rev contains CRLF.
>>>
>>>
>>>>>
>>>>> diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h
>>>> b/OvmfPkg/Include/Library/MemcryptSevLib.h
>>>>> new file mode 100644
>>>>> index 0000000..89f9c86
>>>>> --- /dev/null
>>>>> +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
>>>>> @@ -0,0 +1,42 @@
>>>>> +/** @file
>>>>
>>>> Please add one or two sentences about the purpose of this library class.
>>>
>>>
>>> Will do.
>>>
>>>
>>>>
>>>>
>>>> + Copyright (C) 2017 Advanced Micro Devices.
>>>>> +
>>>>> + This program and the accompanying materials are licensed and made
>>>> available
>>>>> + under the terms and conditions of the BSD License which
>>>>> + accompanies
>>>> this
>>>>> + distribution. The full text of the license may be found at
>>>>> + http://opensource.org/licenses/bsd-license.php
>>>>> +
>>>>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
>> IS"
>>>>> + BASIS,
>>>> WITHOUT
>>>>> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
>> OR IMPLIED.
>>>>> +**/
>>>>> +
>>>>> +#ifndef __MEMCRYPT_SEV_LIB_H__
>>>>> +#define __MEMCRYPT_SEV_LIB_H__
>>>>> +
>>>>> +#include <Uefi/UefiBaseType.h>
>>>>
>>>> I think it shouldn't be necessary to include this, especially not for
>>>> a BASE library (class). What type exactly did you need this include for?
>>>>
>>>>
>>> I should be able to remove the inclusion of this header.
>>>
>>>
>>>>> +#include <Base.h>
>>>>> +
>>>>> +/**
>>>>> +
>>>>> + Initialize SEV memory encryption
>>>>> +
>>>>> +**/
>>>>> +
>>>>> +RETURN_STATUS
>>>>> +EFIAPI
>>>>> +MemcryptSevInitialize (
>>>>> + VOID
>>>>> + );
>>>>> +
>>>>> +/**
>>>>> +
>>>>> + Return TRUE when SEV is active otherwise FALSE
>>>>> +
>>>>> + **/
>>>>
>>>> Is this function restricted to code that runs after a successful
>>>> invocation of MemcryptSevInitialize()? If so, please document that.
>>>>
>>>>
>>> Both are independent functions, SevActive() function returns whether
>>> SEV is active or not. Whereas SevInitialize(), calls SevActive() and
>>> if SEV is active then it sets the dynamic PtePcdMemoryEncryptionMask.
>>>
>>>
>>> Please also document the return values (with @retval and/or @return),
>>>> even though the explanation is likely trivial.
>>>>
>>>> I will document it.
>>>
>>>
>>>
>>>>> +BOOLEAN
>>>>> +EFIAPI
>>>>> +SevActive (
>>>>> + VOID
>>>>> + );
>>>>> +
>>>>
>>>> I'd prefer if we could use a common prefix for the two functions,
>>>> something that is unique to / characteristic of the new library class.
>>>>
>>>>
>>> Will fix it. In general, are we are okay with MemcryptSevLib naming
>>> convensio ?
>>
>> Regarding library naming conventions, we can consider functions, and library
>> instance names.
>>
>> For functions, I tend to prefer a common prefix, although I don't believe this
>> is a hard requirement in edk2.
>>
>> For library instance names, the naming convention seems to be:
>>
>> <PHASE>LibClassName<VARIANT>
>>
>> Where <PHASE> describes the phases the library instance can be used in (so,
>> Base, Dxe, Pei, PeiDxe, and so on), and <VARIANT> describes, very tersely,
>> the underlying implementation (for example, Null means the library does
>> nothing). I think <VARIANT> can be omitted if there's only one library
>> instance.
>>
>>
>>> If we are okay with that then I was going to prefix all the function
>>> with Sev e.g
>>>
>>> SevInitialize() : this sets the dynamic PCD
>>> SevActive() : returns TRUE when SEV is active
>>>
>>>> +#endif
>>>>> +#include <Library/BaseLib.h>
>>>>> +#include <Library/DebugLib.h>
>>>>> +#include <Library/PcdLib.h>
>>>>> +#include <Library/MemcryptSevLib.h>
>>>>> +
>>>>> +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
>>>>
>>>> Can you move all these magic constants (CPUID leaves as well) to
>>>> OvmfPkg/Include/IndustryStandard/? I guess the filename could be
>>>> "AmdSev.h", or something similar.
>>>>
>>>> Such header files are also prime locations to capture the great
>>>> standards references that you added to the blurb.
>>>>
>>>>
>>> Will do it.
>>>
>>>
>>>>> +
>>>>> +RETURN_STATUS
>>>>> +EFIAPI
>>>>> +MemcryptSevInitialize (
>>>>> + VOID
>>>>> + )
>>>>> +{
>>>>> + UINT32 EBX;
>>>>
>>>> Should be Ebx, IMO.
>>>>
>>>>
>>> Will fix it
>>>
>>>> + UINT64 MeMask = 0;
>>>>
>>>> Initialization of local variables is forbidden by the edk2 coding style.
>>>>
>>>>
>>> Will fix it.
>>>
>>>
>>>>> +
>>>>> + if (SevActive ()) {
>>>>
>>>> Aha! So it's the other way around than I expected.
>>>>
>>>>> + // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit
>>>> position)
>>>>
>>>> Comment style is
>>>>
>>>> //
>>>> // CPUID ...
>>>> //
>>>>
>>>>
>>> Will fix it.
>>>
>>>
>>>>> + AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
>>>>> + MeMask = (1ULL << (EBX & 0x3f));
>>>>
>>>> You can't bit shift 64-bit values in edk2 with the C-language
>>>> operator; it won't build for Ia32.
>>>>
>>>> Please either use LShiftU64() here, or -- if you are sure the code
>>>> will never be reached in Ia32 guests -- use (UINTN)1, and shift that with
>> <<.
>>>>
>>>>
>>> I will fix it to use LShiftU64() version.
>>>
>>>
>>>> BTW, in what PI phases do you intend to use this library? For
>>>> example, in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit,
>>>> and the DXE phase runs in 64-bit.
>>>>
>>>>
>>> So my rational behind creating a new library was to have a flexibilty
>>> of adding more SEV specifc functions. The functions which I have mind is:
>>>
>>> SevChangeMemoryAttributeEncrypted(Address, Length) : set the
>>> encryption attribute SevChangeMemoryAttributeDecrypted(Address,
>>> Length) : clear the encryption attribute
>>>
>>> SevChangeMemoryAttribute*() are not implemented in this RFC and I am
>>> working to add in next rev. This will be mainly execute in Dxe phase.
>>> These functions will be update the pagetable to clear/set encryption
>>> masks. It will be mainly used for Dma libraries and possibly
>>> QemuVideoDxe (since framebuffer is shared between HV and Guest
>> hence
>>> we will need to clear the encryption attribute of framebuffer memory).
>>>
>>> I would potentially add two libraries:
>>>
>>> * SevBaseLib: it provides SevActive() and SevInitialize() routines
>>> which can be called anytime
>>> * SevDxeLib: it will provide routines which can be called used by Dxe
>>> drivers.
>>>
>>> Does it make sense to you. I am open to suggestions.
>>
>> I think SevActive() and SevInitialize() should become part of PlatformPei only
>> (if that's possible).
>>
>> The upcoming PTE massaging functions could become part of the DMA lib
>> stuff that you mention (as functions with external linkage), and then you
>> could pull the DMA lib into QemuVideoDxe just to make these functions
>> available.
>>
>> Presently the suggested functions don't seem to justify two (or even
>> one) new libclass.
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Hm, in the next patch, it seems that the library is put to use in
>>>> PlatformPei (and only there). Since these functions are really small,
>>>> I think I would prefer having a new file under OvmfPkg/PlatformPei only.
>>>>
>>>> More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in
>>>> 32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear
>>>> active when the 32-bit PlatformPei module queries it?
>>>>
>>>> Yes SEV will appear active on both Ia32X64 and X64. I have tried
>>>> running
>>> the OVMF build with both version.
>>>
>>>
>>>> * If it doesn't, that's a problem, because then the PCD will be set
>>>> incorrectly. In this case, it might make sense to limit SEV only to
>>>> the
>>>> X64 build of OVMF.
>>>>
>>>> * If SEV does appear active when the 32-bit PlatformPei module
>>>> queries it, then we definitely need to use LShiftU64 here.
>>>>
>>>> Have you tested this series in all three builds of OVMF? (Ia32,
>>>> Ia32X64, and X64?) I'm not asking about SMM, I can see it's on the TODO
>> list.
>>>>
>>>> I have tried Ia3264 and X64 but have not tried Ia32. I will try to
>>>> and let
>>> you know If I find any issues.
>>>
>>>
>>>>> + DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV)
>>>>> + is
>>>> enabled\n"));
>>>>> + DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
>> }
>>>>> +
>>>>> + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
>>>>
>>>> Whiile we do exped PcdSet64S to succeed, please add a separate Status
>>>> variable, and add an ASSERT_RETURN_ERROR on that.
>>>>
>>>>
>>> Will do
>>>
>>>> +
>>>>> + return RETURN_SUCCESS;
>>>>> +}
>>>>> +
>>>>> +BOOLEAN
>>>>> +EFIAPI
>>>>> +SevActive (
>>>>> + VOID
>>>>> + )
>>>>> +{
>>>>> + UINT32 KVMFeatures, EAX;
>>>>
>>>> Should be KvmFeatures and Eax.
>>>>
>>>>> +
>>>>> + // Check if KVM memory encyption feature is set
>>>>
>>>> comment style
>>>>
>>>>> + AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL); if
>>>>> + (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
>>>>> +
>>>>> + // Check whether SEV is enabled
>>>>> + // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled)
>>>>> + AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
>>>>
>>>> Tom commented on this -- the result is not checked.
>>>>
>>>>
>>> Will fix it.
>>>
>>>
>>>>> +
>>>>> + return TRUE;
>>>>> + }
>>>>> +
>>>>> + return FALSE;
>>>>> +}
>>>>> +
>>>>> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>> b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>> new file mode 100644
>>>>> index 0000000..8e8d7e0
>>>>> --- /dev/null
>>>>> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>> @@ -0,0 +1,44 @@
>>>>> +## @file
>>>>> +#
>>>>> +# Copyright (c) 2017 Advanced Micro Devices. All rights
>>>>> +reserved.<BR> # # This program and the accompanying materials #
>>>>> +are licensed and made available under the terms and conditions of
>>>> the BSD License
>>>>> +# which accompanies this distribution. The full text of the
>>>>> +license
>>>> may be found at
>>>>> +# http://opensource.org/licenses/bsd-license.php
>>>>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
>> IS"
>>>>> +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER
>>>>> +EXPRESS OR
>>>> IMPLIED.
>>>>> +#
>>>>> +#
>>>>> +##
>>>>> +
>>>>> +[Defines]
>>>>> + INF_VERSION = 0x00010005
>>>>> + BASE_NAME = MemcryptSevLib
>>>>> + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b
>>>>> + MODULE_TYPE = BASE
>>>>> + VERSION_STRING = 1.0
>>>>> + LIBRARY_CLASS = MemcryptSevLib
>>>>> +
>>>>> +#
>>>>> +# The following information is for reference only and not required
>>>>> +by
>>>> the build tools.
>>>>> +#
>>>>> +# VALID_ARCHITECTURES = IA32 X64
>>>>> +#
>>>>> +
>>>>> +[Packages]
>>>>> + MdePkg/MdePkg.dec
>>>>> + MdeModulePkg/MdeModulePkg.dec
>>>>> + OvmfPkg/OvmfPkg.dec
>>>>> + UefiCpuPkg/UefiCpuPkg.dec
>>>>> +
>>>>> +[Sources]
>>>>> + MemcryptSevLib.c
>>>>> +
>>>>> +[LibraryClasses]
>>>>> + BaseLib
>>>>> + DebugLib
>>>>> + PcdLib
>>>>> +
>>>>> +[Pcd]
>>>>> +
>>>>>
>> +gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOr
>> Mask
>>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc
>> b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> index 56f7ff9..a35e1d2 100644
>>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> @@ -129,6 +129,7 @@
>>>>> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>>>>> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>>>>> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>>>>> +
>> MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>> !if $(SMM_REQUIRE) == FALSE
>>>>> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>>>>> !endif
>>>>
>>>> Please configure your git instance so that it includes the section
>>>> names of INI-style files in hunk headers:
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/
>>>> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
>>>> maintainers#contrib-05
>>>>
>>>> see xfuncname there, and:
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/
>>>> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
>>>> maintainers#contrib-09
>>>>
>>>>
>>>
>>> I will go through INI-Style file and git setting etc.
>>>
>>>
>>>>
>>>>> @@ -509,6 +510,9 @@
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>>>>
>>>>> + # Set memory encryption mask
>>>>> +
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
>>>> rMask|0x0
>>>>> +
>>>>> !if $(SMM_REQUIRE) == TRUE
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index
>>>>> d0b0b0e..5d853d6 100644
>>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>>> @@ -129,6 +129,7 @@
>>>>> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>>>>> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>>>>> LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>>>>> +
>> MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>> !if $(SMM_REQUIRE) == FALSE
>>>>> LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>>>>> !endif
>>>>> @@ -508,6 +509,9 @@
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>>>>
>>>>> + # Set memory encryption mask
>>>>> +
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
>>>> rMask|0x0
>>>>> +
>>>>> !if $(SMM_REQUIRE) == TRUE
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>>>>>
>>>>
>>>> The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is
>>>> not resolved for the Ia32 build, hence PlatformPei will fail to build
>>>> in the next patch.
>>>>
>>>> Please build and regression-test all three builds of OVMF (SMM
>>>> disabled, for now) in the development of this feature.
>>>>
>>>> Regression-testing should in particular include S3 suspend/resume
>>>> (again, with SMM disabled, for now). If it breaks (and it is expected
>>>> to break), please extend the S3Verification() function so that it
>>>> prevents the firmware from booting (with a reasonable error message)
>>>> if it sees that SEV is active and S3 was *not* disabled on the QEMU
>> command line.
>>>> Losing a guest at S3 resume is very annoying, it's better not to boot
>>>> in that case (and to ask the user to disable S3 on the QEMU command
>> line).
>>>>
>>>> Anyhow, I think these functions should go directly under
>>>> OvmfPkg/PlatformPei, into a new file.
>>>>
>>>> I may have missed a few things, but I'm (theoretically) at a
>>>> conference, and right now it seems more correct for me to give
>>>> (perhaps spotty) feedback *quickly*, than to let my backlog pile up.
>>>>
>>>>
>>> Appriciate your quick feedbacks, I will go through each of them and
>>> will try to address in v2.
>>>
>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC PATCH v1 3/5] OvmfPkg/PlatformPei: Initialize SEV support
2017-03-06 23:27 [RFC PATCH v1 0/5] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
2017-03-06 23:27 ` [RFC PATCH v1 1/5] OvmfPkg/ResetVector: Set memory encryption when SEV is active Brijesh Singh
2017-03-06 23:27 ` [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library Brijesh Singh
@ 2017-03-06 23:27 ` Brijesh Singh
2017-03-07 17:08 ` Laszlo Ersek
2017-03-06 23:27 ` [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package Brijesh Singh
2017-03-06 23:28 ` [RFC PATCH v1 5/5] OvmfPkg/BaseIoLibIntrinsic: Unroll String I/O when SEV is active Brijesh Singh
4 siblings, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-06 23:27 UTC (permalink / raw)
To: jordan.l.justen, edk2-devel, lersek
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
Initialize Secure Encrypted Virtualization (SEV) support.
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/PlatformPei/Platform.c | 6 ++++++
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
2 files changed, 7 insertions(+)
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0be8672..a948037 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -33,6 +33,7 @@
#include <Library/PeiServicesLib.h>
#include <Library/QemuFwCfgLib.h>
#include <Library/ResourcePublicationLib.h>
+#include <Library/MemcryptSevLib.h>
#include <Guid/MemoryTypeInformation.h>
#include <Ppi/MasterBootMode.h>
#include <IndustryStandard/Pci22.h>
@@ -669,5 +670,10 @@ InitializePlatform (
MiscInitialization ();
InstallFeatureControlCallback ();
+ //
+ // Initialize SEV support
+ //
+ MemcryptSevInitialize ();
+
return EFI_SUCCESS;
}
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index fbaed31..f85b208 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -60,6 +60,7 @@
QemuFwCfgLib
MtrrLib
PcdLib
+ MemcryptSevLib
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 3/5] OvmfPkg/PlatformPei: Initialize SEV support
2017-03-06 23:27 ` [RFC PATCH v1 3/5] OvmfPkg/PlatformPei: Initialize SEV support Brijesh Singh
@ 2017-03-07 17:08 ` Laszlo Ersek
2017-03-07 19:17 ` Brijesh Singh
0 siblings, 1 reply; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-07 17:08 UTC (permalink / raw)
To: Brijesh Singh, jordan.l.justen, edk2-devel
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
On 03/07/17 00:27, Brijesh Singh wrote:
> Initialize Secure Encrypted Virtualization (SEV) support.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/PlatformPei/Platform.c | 6 ++++++
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 0be8672..a948037 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -33,6 +33,7 @@
> #include <Library/PeiServicesLib.h>
> #include <Library/QemuFwCfgLib.h>
> #include <Library/ResourcePublicationLib.h>
> +#include <Library/MemcryptSevLib.h>
> #include <Guid/MemoryTypeInformation.h>
> #include <Ppi/MasterBootMode.h>
> #include <IndustryStandard/Pci22.h>
> @@ -669,5 +670,10 @@ InitializePlatform (
> MiscInitialization ();
> InstallFeatureControlCallback ();
>
> + //
> + // Initialize SEV support
> + //
> + MemcryptSevInitialize ();
> +
This will be called on both the normal boot path and the S3 resume boot
path. (Similarly to InstallFeatureControlCallback() above.) IIRC, you
did modify S3Resume2Pei to consider the PCD (and at that time we
discussed that setting the PCD here, in PlatformPei, would be
sufficiently early even for S3 resume). Thus, can you please confirm (in
the commit message) whether this works with S3 resume?
> return EFI_SUCCESS;
> }
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index fbaed31..f85b208 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -60,6 +60,7 @@
> QemuFwCfgLib
> MtrrLib
> PcdLib
> + MemcryptSevLib
As noted before, I suggest to replace this new libclass (introduction
and dependency) simply with a new file here.
Thanks!
Laszlo
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 3/5] OvmfPkg/PlatformPei: Initialize SEV support
2017-03-07 17:08 ` Laszlo Ersek
@ 2017-03-07 19:17 ` Brijesh Singh
0 siblings, 0 replies; 33+ messages in thread
From: Brijesh Singh @ 2017-03-07 19:17 UTC (permalink / raw)
To: Laszlo Ersek
Cc: jordan.l.justen, edk2-devel, Tom Lendacky, Leo Duran,
brijesh.singh
On Tue, Mar 7, 2017 at 11:08 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/07/17 00:27, Brijesh Singh wrote:
> > Initialize Secure Encrypted Virtualization (SEV) support.
> >
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> > OvmfPkg/PlatformPei/Platform.c | 6 ++++++
> > OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/
> Platform.c
> > index 0be8672..a948037 100644
> > --- a/OvmfPkg/PlatformPei/Platform.c
> > +++ b/OvmfPkg/PlatformPei/Platform.c
> > @@ -33,6 +33,7 @@
> > #include <Library/PeiServicesLib.h>
> > #include <Library/QemuFwCfgLib.h>
> > #include <Library/ResourcePublicationLib.h>
> > +#include <Library/MemcryptSevLib.h>
> > #include <Guid/MemoryTypeInformation.h>
> > #include <Ppi/MasterBootMode.h>
> > #include <IndustryStandard/Pci22.h>
> > @@ -669,5 +670,10 @@ InitializePlatform (
> > MiscInitialization ();
> > InstallFeatureControlCallback ();
> >
> > + //
> > + // Initialize SEV support
> > + //
> > + MemcryptSevInitialize ();
> > +
>
> This will be called on both the normal boot path and the S3 resume boot
> path. (Similarly to InstallFeatureControlCallback() above.) IIRC, you
> did modify S3Resume2Pei to consider the PCD (and at that time we
> discussed that setting the PCD here, in PlatformPei, would be
> sufficiently early even for S3 resume). Thus, can you please confirm (in
> the commit message) whether this works with S3 resume?
>
>
I have not tested te S3 resume funcationlity, should have mentioned in my
TODO list. I will test it before next rfc and update the commit messages
accordingly.
> > return EFI_SUCCESS;
> > }
> > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/
> PlatformPei.inf
> > index fbaed31..f85b208 100644
> > --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> > @@ -60,6 +60,7 @@
> > QemuFwCfgLib
> > MtrrLib
> > PcdLib
> > + MemcryptSevLib
>
> As noted before, I suggest to replace this new libclass (introduction
> and dependency) simply with a new file here.
>
>
Sure I will remove the dependency and create a new file in PlatformPei.
> Thanks!
> Laszlo
>
> >
> > [Pcd]
> > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
> >
>
>
--
Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-06 23:27 [RFC PATCH v1 0/5] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
` (2 preceding siblings ...)
2017-03-06 23:27 ` [RFC PATCH v1 3/5] OvmfPkg/PlatformPei: Initialize SEV support Brijesh Singh
@ 2017-03-06 23:27 ` Brijesh Singh
2017-03-07 17:20 ` Laszlo Ersek
2017-03-06 23:28 ` [RFC PATCH v1 5/5] OvmfPkg/BaseIoLibIntrinsic: Unroll String I/O when SEV is active Brijesh Singh
4 siblings, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-06 23:27 UTC (permalink / raw)
To: jordan.l.justen, edk2-devel, lersek
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
Imports IoLib into OvmfPkg to make the changes to support SEV guest.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
.../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 0
.../BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni | 0
.../BaseIoLibIntrinsicInternal.h | 0
OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm | 0
.../Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c | 0
.../Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c | 0
OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm | 0
OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm | 0
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
18 files changed, 2 insertions(+), 2 deletions(-)
copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLib.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm (100%)
copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm (100%)
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
similarity index 100%
copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a35e1d2..fd89518 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -106,7 +106,7 @@
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
- IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+ IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 5d853d6..ce77170 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -106,7 +106,7 @@
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
- IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+ IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-06 23:27 ` [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package Brijesh Singh
@ 2017-03-07 17:20 ` Laszlo Ersek
2017-03-07 20:06 ` Jordan Justen
0 siblings, 1 reply; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-07 17:20 UTC (permalink / raw)
To: Brijesh Singh, jordan.l.justen, edk2-devel,
Jordan Justen (Intel address), Gao, Liming
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
On 03/07/17 00:27, Brijesh Singh wrote:
> Imports IoLib into OvmfPkg to make the changes to support SEV guest.
Ugh, this looks terrible.
$ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
6062 total
Jordan, Liming, if I recall correctly, you guys were leading the
IoFifoLib discussion a few weeks back. At that time, I would have
preferred to add those functions to a separate IoFifoLib class (like
Brijesh originally suggested), but seeing the consensus on adding the
Fifo primitives to IoLib instead, I didn't speak up.
So now that the Fifo primitives have to be customized (unrolled), and
the selection should be made dynamically (at runtime), what do you guys
suggest for the implementation, without importing six thousand lines
into OvmfPkg?
I think this patch should be dropped, and the next patch (#5) should be
applied straight to MdePkg. SEV detection happens via the CPUID
instruction, and it is specified by a public industry standard, so
adding the code to MdePkg looks appropriate to me.
If even the CPUID check should be omitted in the default case, then we
should use a new FeaturePCD.
Thanks,
Laszlo
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 0
> .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni | 0
> .../BaseIoLibIntrinsicInternal.h | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm | 0
> .../Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c | 0
> .../Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm | 0
> OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm | 0
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
> OvmfPkg/OvmfPkgX64.dsc | 2 +-
> 18 files changed, 2 insertions(+), 2 deletions(-)
> copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h => OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLib.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c => OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm (100%)
> copy MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm => OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm (100%)
>
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLib.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c b/OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> similarity index 100%
> copy from MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> copy to OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index a35e1d2..fd89518 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -106,7 +106,7 @@
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> - IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> + IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
> SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5d853d6..ce77170 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -106,7 +106,7 @@
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> - IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> + IoLib|OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
> SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-07 17:20 ` Laszlo Ersek
@ 2017-03-07 20:06 ` Jordan Justen
2017-03-07 22:18 ` Laszlo Ersek
2017-03-08 15:41 ` Gao, Liming
0 siblings, 2 replies; 33+ messages in thread
From: Jordan Justen @ 2017-03-07 20:06 UTC (permalink / raw)
To: Gao, Liming, Brijesh Singh, Laszlo Ersek, edk2-devel,
Kinney, Michael D
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
On 2017-03-07 09:20:14, Laszlo Ersek wrote:
> On 03/07/17 00:27, Brijesh Singh wrote:
> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>
> Ugh, this looks terrible.
>
> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
> 82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> 24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> 26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> 141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> 137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> 2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> 317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> 599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> 342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> 196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> 214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> 736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> 411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> 228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> 127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> 126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> 6062 total
>
> Jordan, Liming, if I recall correctly, you guys were leading the
> IoFifoLib discussion a few weeks back. At that time, I would have
> preferred to add those functions to a separate IoFifoLib class (like
> Brijesh originally suggested), but seeing the consensus on adding the
> Fifo primitives to IoLib instead, I didn't speak up.
>
> So now that the Fifo primitives have to be customized (unrolled), and
> the selection should be made dynamically (at runtime), what do you guys
> suggest for the implementation, without importing six thousand lines
> into OvmfPkg?
>
> I think this patch should be dropped, and the next patch (#5) should be
> applied straight to MdePkg. SEV detection happens via the CPUID
> instruction, and it is specified by a public industry standard, so
> adding the code to MdePkg looks appropriate to me.
Yeah, I agree. (Not sure if Liming and Mike agree though. :)
Additionally, it would be nice to have a spec citation for the "Public
Industry Standard" in the commit message.
> If even the CPUID check should be omitted in the default case, then we
> should use a new FeaturePCD.
Apparently we don't mind terribly about adding a cpuid call straight
into the normal flow of commonly used functions
(881813d7a93d9009c873515b043c41c4554779e4). :)
I would say that I don't quite agree with that. And, further, it could
be that once per I/O operation has more of a perf impact than once per
flush. Do we know that cpuid time is so far down in the noise compared
to I/O that it won't matter?
One other thought is, should we consider a DxeSmm alternative .inf for
BaseIoLibIntrinsic.inf? In that case we could use a global variable to
help out. Maybe this could prevent the concern that might drive a new
PCD to be added?
-Jordan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-07 20:06 ` Jordan Justen
@ 2017-03-07 22:18 ` Laszlo Ersek
2017-03-08 15:41 ` Gao, Liming
1 sibling, 0 replies; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-07 22:18 UTC (permalink / raw)
To: Jordan Justen, Gao, Liming, Brijesh Singh, edk2-devel,
Kinney, Michael D
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
On 03/07/17 21:06, Jordan Justen wrote:
> On 2017-03-07 09:20:14, Laszlo Ersek wrote:
>> On 03/07/17 00:27, Brijesh Singh wrote:
>>> Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>>
>> Ugh, this looks terrible.
>>
>> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>> 82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>> 24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>> 26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>> 141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>> 137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>> 2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>> 317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>> 599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>> 342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>> 196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>> 214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>> 736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>> 411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>> 228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>> 127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>> 126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>> 6062 total
>>
>> Jordan, Liming, if I recall correctly, you guys were leading the
>> IoFifoLib discussion a few weeks back. At that time, I would have
>> preferred to add those functions to a separate IoFifoLib class (like
>> Brijesh originally suggested), but seeing the consensus on adding the
>> Fifo primitives to IoLib instead, I didn't speak up.
>>
>> So now that the Fifo primitives have to be customized (unrolled), and
>> the selection should be made dynamically (at runtime), what do you guys
>> suggest for the implementation, without importing six thousand lines
>> into OvmfPkg?
>>
>> I think this patch should be dropped, and the next patch (#5) should be
>> applied straight to MdePkg. SEV detection happens via the CPUID
>> instruction, and it is specified by a public industry standard, so
>> adding the code to MdePkg looks appropriate to me.
>
> Yeah, I agree. (Not sure if Liming and Mike agree though. :)
>
> Additionally, it would be nice to have a spec citation for the "Public
> Industry Standard" in the commit message.
>
>> If even the CPUID check should be omitted in the default case, then we
>> should use a new FeaturePCD.
>
> Apparently we don't mind terribly about adding a cpuid call straight
> into the normal flow of commonly used functions
> (881813d7a93d9009c873515b043c41c4554779e4). :)
>
> I would say that I don't quite agree with that.
Well, I'm neutral on it. If keeping the CPUID for SEV feature detection
is okay as far as the coding style is concerned, I"m all for it.
> And, further, it could
> be that once per I/O operation has more of a perf impact than once per
> flush. Do we know that cpuid time is so far down in the noise compared
> to I/O that it won't matter?
Well, for fifo ops at least, the overhead of CPUID shouldn't be large.
>
> One other thought is, should we consider a DxeSmm alternative .inf for
> BaseIoLibIntrinsic.inf?
I sort of dislike that, unless Brijesh can solve it with another INF
file within the same directory, and minimal code duplication. (I.e.,
with most source files reused.)
> In that case we could use a global variable to
> help out. Maybe this could prevent the concern that might drive a new
> PCD to be added?
This crossed my mind (and then the CPUID would be executed only once per
library constructor invocation), but I was (somewhat inexplicably?)
worried that such a library instance would not be suitable for SEC code,
and for PEI code running on different platforms (i.e., XIP from flash).
Those restrictions don't seem to apply to OVMF though, so I guess the
idea is worth exploring!
Thanks
Laszlo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-07 20:06 ` Jordan Justen
2017-03-07 22:18 ` Laszlo Ersek
@ 2017-03-08 15:41 ` Gao, Liming
2017-03-08 16:26 ` Brijesh Singh
2017-03-08 18:58 ` Jordan Justen
1 sibling, 2 replies; 33+ messages in thread
From: Gao, Liming @ 2017-03-08 15:41 UTC (permalink / raw)
To: Justen, Jordan L, Brijesh Singh, Laszlo Ersek,
edk2-devel@lists.01.org, Kinney, Michael D
Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, brijesh.sing@amd.com
>-----Original Message-----
>From: Justen, Jordan L
>Sent: Wednesday, March 08, 2017 4:06 AM
>To: Gao, Liming <liming.gao@intel.com>; Brijesh Singh
><brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>; edk2-
>devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
>brijesh.sing@amd.com
>Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>BaseIoLibIntrinsic package
>
>On 2017-03-07 09:20:14, Laszlo Ersek wrote:
>> On 03/07/17 00:27, Brijesh Singh wrote:
>> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>>
>> Ugh, this looks terrible.
>>
>> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>> 82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>> 24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>> 26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>> 141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>> 137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>> 2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>> 317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>> 599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>> 342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>> 196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>> 214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>> 736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>> 411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>> 228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>> 127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>> 126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>> 6062 total
>>
>> Jordan, Liming, if I recall correctly, you guys were leading the
>> IoFifoLib discussion a few weeks back. At that time, I would have
>> preferred to add those functions to a separate IoFifoLib class (like
>> Brijesh originally suggested), but seeing the consensus on adding the
>> Fifo primitives to IoLib instead, I didn't speak up.
>>
>> So now that the Fifo primitives have to be customized (unrolled), and
>> the selection should be made dynamically (at runtime), what do you guys
>> suggest for the implementation, without importing six thousand lines
>> into OvmfPkg?
>>
>> I think this patch should be dropped, and the next patch (#5) should be
>> applied straight to MdePkg. SEV detection happens via the CPUID
>> instruction, and it is specified by a public industry standard, so
>> adding the code to MdePkg looks appropriate to me.
>
>Yeah, I agree. (Not sure if Liming and Mike agree though. :)
I agree. If SEV is the public industry standard, it can be added into MdePkg Library implementation. I suggest to add spec refer in file header.
>
>Additionally, it would be nice to have a spec citation for the "Public
>Industry Standard" in the commit message.
>
>> If even the CPUID check should be omitted in the default case, then we
>> should use a new FeaturePCD.
>
>Apparently we don't mind terribly about adding a cpuid call straight
>into the normal flow of commonly used functions
>(881813d7a93d9009c873515b043c41c4554779e4). :)
>
>I would say that I don't quite agree with that. And, further, it could
>be that once per I/O operation has more of a perf impact than once per
>flush. Do we know that cpuid time is so far down in the noise compared
>to I/O that it won't matter?
>
I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU? If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If Intel X86 CPU doesn't work, we need to add new PCD to control its logic.
>One other thought is, should we consider a DxeSmm alternative .inf for
>BaseIoLibIntrinsic.inf? In that case we could use a global variable to
>help out. Maybe this could prevent the concern that might drive a new
>PCD to be added?
>
>-Jordan
Current patch has stored the check state into data section. In PEI phase, the data section can't be written. So, every call will check CpuId. In DXE and SMM phase, the data section can be written. The first call will cache the check state. So, no DxeSmm INF is required.
Thanks
Liming
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-08 15:41 ` Gao, Liming
@ 2017-03-08 16:26 ` Brijesh Singh
2017-03-09 1:43 ` Gao, Liming
2017-03-08 18:58 ` Jordan Justen
1 sibling, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-08 16:26 UTC (permalink / raw)
To: Gao, Liming
Cc: Justen, Jordan L, Laszlo Ersek, edk2-devel@lists.01.org,
Kinney, Michael D, Thomas.Lendacky@amd.com, leo.duran@amd.com,
brijesh.singh
On Wed, Mar 8, 2017 at 9:41 AM, Gao, Liming <liming.gao@intel.com> wrote:
>
>
> >-----Original Message-----
> >From: Justen, Jordan L
> >Sent: Wednesday, March 08, 2017 4:06 AM
> >To: Gao, Liming <liming.gao@intel.com>; Brijesh Singh
> ><brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>; edk2-
> >devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> >Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
> >brijesh.sing@amd.com
> >Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
> >BaseIoLibIntrinsic package
> >
> >On 2017-03-07 09:20:14, Laszlo Ersek wrote:
> >> On 03/07/17 00:27, Brijesh Singh wrote:
> >> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
> >>
> >> Ugh, this looks terrible.
> >>
> >> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
> >> 82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> >> 24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
> >> 26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
> >> 141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
> >> 137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
> >> 2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
> >> 317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> >> 599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
> >> 342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
> >> 196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
> >> 214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
> >> 736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
> >> 411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
> >> 228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
> >> 127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
> >> 126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
> >> 6062 total
> >>
> >> Jordan, Liming, if I recall correctly, you guys were leading the
> >> IoFifoLib discussion a few weeks back. At that time, I would have
> >> preferred to add those functions to a separate IoFifoLib class (like
> >> Brijesh originally suggested), but seeing the consensus on adding the
> >> Fifo primitives to IoLib instead, I didn't speak up.
> >>
> >> So now that the Fifo primitives have to be customized (unrolled), and
> >> the selection should be made dynamically (at runtime), what do you guys
> >> suggest for the implementation, without importing six thousand lines
> >> into OvmfPkg?
> >>
> >> I think this patch should be dropped, and the next patch (#5) should be
> >> applied straight to MdePkg. SEV detection happens via the CPUID
> >> instruction, and it is specified by a public industry standard, so
> >> adding the code to MdePkg looks appropriate to me.
> >
> >Yeah, I agree. (Not sure if Liming and Mike agree though. :)
> I agree. If SEV is the public industry standard, it can be added into
> MdePkg Library implementation. I suggest to add spec refer in file header.
>
> >
> >Additionally, it would be nice to have a spec citation for the "Public
> >Industry Standard" in the commit message.
> >
> >> If even the CPUID check should be omitted in the default case, then we
> >> should use a new FeaturePCD.
> >
> >Apparently we don't mind terribly about adding a cpuid call straight
> >into the normal flow of commonly used functions
> >(881813d7a93d9009c873515b043c41c4554779e4). :)
> >
> >I would say that I don't quite agree with that. And, further, it could
> >be that once per I/O operation has more of a perf impact than once per
> >flush. Do we know that cpuid time is so far down in the noise compared
> >to I/O that it won't matter?
> >
> I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU?
> If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If
> Intel X86 CPU doesn't work, we need to add new PCD to control its logic.
>
>
The CheckSevFeature() should return 0 on non SEV platform. I have tried
booting a virtual guest on Intel X86 CPU and it seems to be work fine (
defaults to rep string I/O). Both Intel and AMD have reserved CPUID
4000_0000 - 4000_00FF for software usage. So if we are booting in non
virtualized environment then CPUID 4000_0001 should return 0 and in
virtualized environment the BIT 8 should be 1 if hypervisor support SEV.
So far I have using KVM hypervisor to test my code but I will investigate
more to ensure that the check works accross multiple hypervisors.
>One other thought is, should we consider a DxeSmm alternative .inf for
> >BaseIoLibIntrinsic.inf? In that case we could use a global variable to
> >help out. Maybe this could prevent the concern that might drive a new
> >PCD to be added?
> >
> >-Jordan
> Current patch has stored the check state into data section. In PEI phase,
> the data section can't be written. So, every call will check CpuId. In DXE
> and SMM phase, the data section can be written. The first call will cache
> the check state. So, no DxeSmm INF is required.
>
>
> Thanks
> Liming
>
--
Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-08 16:26 ` Brijesh Singh
@ 2017-03-09 1:43 ` Gao, Liming
0 siblings, 0 replies; 33+ messages in thread
From: Gao, Liming @ 2017-03-09 1:43 UTC (permalink / raw)
To: Brijesh Singh
Cc: Justen, Jordan L, Laszlo Ersek, edk2-devel@lists.01.org,
Kinney, Michael D, Thomas.Lendacky@amd.com, leo.duran@amd.com,
brijesh.singh@amd.com
From: Brijesh Singh [mailto:brijesh.ksingh@gmail.com]
Sent: Thursday, March 9, 2017 12:27 AM
To: Gao, Liming <liming.gao@intel.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Thomas.Lendacky@amd.com; leo.duran@amd.com; brijesh.singh@amd.com
Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
On Wed, Mar 8, 2017 at 9:41 AM, Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> wrote:
>-----Original Message-----
>From: Justen, Jordan L
>Sent: Wednesday, March 08, 2017 4:06 AM
>To: Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Brijesh Singh
><brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-
>devel@lists.01.org<mailto:devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>Cc: Thomas.Lendacky@amd.com<mailto:Thomas.Lendacky@amd.com>; leo.duran@amd.com<mailto:leo.duran@amd.com>;
>brijesh.sing@amd.com<mailto:brijesh.sing@amd.com>
>Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>BaseIoLibIntrinsic package
>
>On 2017-03-07 09:20:14, Laszlo Ersek wrote:
>> On 03/07/17 00:27, Brijesh Singh wrote:
>> > Imports IoLib into OvmfPkg to make the changes to support SEV guest.
>>
>> Ugh, this looks terrible.
>>
>> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/)
>> 82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>> 24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni
>> 26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h
>> 141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm
>> 137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
>> 2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c
>> 317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
>> 599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c
>> 342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c
>> 196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c
>> 214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c
>> 736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c
>> 411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c
>> 228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c
>> 127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm
>> 126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
>> 6062 total
>>
>> Jordan, Liming, if I recall correctly, you guys were leading the
>> IoFifoLib discussion a few weeks back. At that time, I would have
>> preferred to add those functions to a separate IoFifoLib class (like
>> Brijesh originally suggested), but seeing the consensus on adding the
>> Fifo primitives to IoLib instead, I didn't speak up.
>>
>> So now that the Fifo primitives have to be customized (unrolled), and
>> the selection should be made dynamically (at runtime), what do you guys
>> suggest for the implementation, without importing six thousand lines
>> into OvmfPkg?
>>
>> I think this patch should be dropped, and the next patch (#5) should be
>> applied straight to MdePkg. SEV detection happens via the CPUID
>> instruction, and it is specified by a public industry standard, so
>> adding the code to MdePkg looks appropriate to me.
>
>Yeah, I agree. (Not sure if Liming and Mike agree though. :)
I agree. If SEV is the public industry standard, it can be added into MdePkg Library implementation. I suggest to add spec refer in file header.
>
>Additionally, it would be nice to have a spec citation for the "Public
>Industry Standard" in the commit message.
>
>> If even the CPUID check should be omitted in the default case, then we
>> should use a new FeaturePCD.
>
>Apparently we don't mind terribly about adding a cpuid call straight
>into the normal flow of commonly used functions
>(881813d7a93d9009c873515b043c41c4554779e4). :)
>
>I would say that I don't quite agree with that. And, further, it could
>be that once per I/O operation has more of a perf impact than once per
>flush. Do we know that cpuid time is so far down in the noise compared
>to I/O that it won't matter?
>
I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU? If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If Intel X86 CPU doesn't work, we need to add new PCD to control its logic.
The CheckSevFeature() should return 0 on non SEV platform. I have tried booting a virtual guest on Intel X86 CPU and it seems to be work fine ( defaults to rep string I/O). Both Intel and AMD have reserved CPUID 4000_0000 - 4000_00FF for software usage. So if we are booting in non virtualized environment then CPUID 4000_0001 should return 0 and in virtualized environment the BIT 8 should be 1 if hypervisor support SEV. So far I have using KVM hypervisor to test my code but I will investigate more to ensure that the check works accross multiple hypervisors.
[Liming] As you say CPUID 4000_0001 is reserved, AMD uses it for SEV. Intel may use it for other purpose. If so, we can’t make sure CheckSevFeature() always work. Then, I suggest to introduce Pcd to enable/disable SEV checker.
>One other thought is, should we consider a DxeSmm alternative .inf for
>BaseIoLibIntrinsic.inf? In that case we could use a global variable to
>help out. Maybe this could prevent the concern that might drive a new
>PCD to be added?
>
>-Jordan
Current patch has stored the check state into data section. In PEI phase, the data section can't be written. So, every call will check CpuId. In DXE and SMM phase, the data section can be written. The first call will cache the check state. So, no DxeSmm INF is required.
Thanks
Liming
--
Confusion is always the most honest response.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-08 15:41 ` Gao, Liming
2017-03-08 16:26 ` Brijesh Singh
@ 2017-03-08 18:58 ` Jordan Justen
2017-03-09 1:48 ` Gao, Liming
1 sibling, 1 reply; 33+ messages in thread
From: Jordan Justen @ 2017-03-08 18:58 UTC (permalink / raw)
To: Gao, Liming, Kinney, Michael D, edk2-devel@lists.01.org,
Brijesh Singh, Laszlo Ersek
Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, brijesh.sing@amd.com
On 2017-03-08 07:41:58, Gao, Liming wrote:
>
> >-----Original Message-----
> >From: Justen, Jordan L
>
> >One other thought is, should we consider a DxeSmm alternative .inf for
> >BaseIoLibIntrinsic.inf? In that case we could use a global variable to
> >help out. Maybe this could prevent the concern that might drive a new
> >PCD to be added?
> >
> >-Jordan
> Current patch has stored the check state into data section. In PEI
> phase, the data section can't be written. So, every call will check
> CpuId. In DXE and SMM phase, the data section can be written. The
> first call will cache the check state. So, no DxeSmm INF is
> required.
>
I don't think we can attempt to write a variable to memory in generic
SEC/PEI code. Some flash memory treat memory writes as an I/O for
programming purposes. I think we added
PcdGuidedExtractHandlerTableAddress for this reason. This is why I
suggested that maybe we could add a DXE/SMM .inf where we could assume
writeable global variables exit.
-Jordan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-08 18:58 ` Jordan Justen
@ 2017-03-09 1:48 ` Gao, Liming
2017-03-09 15:36 ` Duran, Leo
0 siblings, 1 reply; 33+ messages in thread
From: Gao, Liming @ 2017-03-09 1:48 UTC (permalink / raw)
To: Justen, Jordan L, Kinney, Michael D, edk2-devel@lists.01.org,
Brijesh Singh, Laszlo Ersek
Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, brijesh.sing@amd.com
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Thursday, March 9, 2017 2:59 AM
> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com; brijesh.sing@amd.com
> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
>
> On 2017-03-08 07:41:58, Gao, Liming wrote:
> >
> > >-----Original Message-----
> > >From: Justen, Jordan L
> >
> > >One other thought is, should we consider a DxeSmm alternative .inf for
> > >BaseIoLibIntrinsic.inf? In that case we could use a global variable to
> > >help out. Maybe this could prevent the concern that might drive a new
> > >PCD to be added?
> > >
> > >-Jordan
> > Current patch has stored the check state into data section. In PEI
> > phase, the data section can't be written. So, every call will check
> > CpuId. In DXE and SMM phase, the data section can be written. The
> > first call will cache the check state. So, no DxeSmm INF is
> > required.
> >
>
> I don't think we can attempt to write a variable to memory in generic
> SEC/PEI code. Some flash memory treat memory writes as an I/O for
> programming purposes. I think we added
> PcdGuidedExtractHandlerTableAddress for this reason. This is why I
> suggested that maybe we could add a DXE/SMM .inf where we could assume
> writeable global variables exit.
>
> -Jordan
I get your point. So, I suggest to always check SEV in BaseIoLib. If people meet with the performance issue, DXE/SMM version IoLib can be added later.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-09 1:48 ` Gao, Liming
@ 2017-03-09 15:36 ` Duran, Leo
2017-03-09 16:36 ` Laszlo Ersek
0 siblings, 1 reply; 33+ messages in thread
From: Duran, Leo @ 2017-03-09 15:36 UTC (permalink / raw)
To: 'Gao, Liming', Justen, Jordan L, Kinney, Michael D,
edk2-devel@lists.01.org, Brijesh Singh, Laszlo Ersek
Cc: Lendacky, Thomas, brijesh.sing@amd.com
OK, what I'm hearing is:
- Let's leave the Fifo routines in BaseIoLib (as we currently have)
- And let's add the SEV checks to BaseIoLib, so that OvmPkg (et al) can consume it as-is
If so, I could put a patch-set together for that... Please confirm.
BTW, I'd try to minimize the need for CPUID (e.g., check it once and cache the result)
Leo
> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Wednesday, March 08, 2017 7:48 PM
> To: Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Duran, Leo
> <leo.duran@amd.com>; brijesh.sing@amd.com
> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
> BaseIoLibIntrinsic package
>
>
>
> > -----Original Message-----
> > From: Justen, Jordan L
> > Sent: Thursday, March 9, 2017 2:59 AM
> > To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
> > <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
> > Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
> brijesh.sing@amd.com
> > Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
> > BaseIoLibIntrinsic package
> >
> > On 2017-03-08 07:41:58, Gao, Liming wrote:
> > >
> > > >-----Original Message-----
> > > >From: Justen, Jordan L
> > >
> > > >One other thought is, should we consider a DxeSmm alternative .inf
> > > >for BaseIoLibIntrinsic.inf? In that case we could use a global
> > > >variable to help out. Maybe this could prevent the concern that
> > > >might drive a new PCD to be added?
> > > >
> > > >-Jordan
> > > Current patch has stored the check state into data section. In PEI
> > > phase, the data section can't be written. So, every call will check
> > > CpuId. In DXE and SMM phase, the data section can be written. The
> > > first call will cache the check state. So, no DxeSmm INF is
> > > required.
> > >
> >
> > I don't think we can attempt to write a variable to memory in generic
> > SEC/PEI code. Some flash memory treat memory writes as an I/O for
> > programming purposes. I think we added
> > PcdGuidedExtractHandlerTableAddress for this reason. This is why I
> > suggested that maybe we could add a DXE/SMM .inf where we could
> assume
> > writeable global variables exit.
> >
> > -Jordan
>
> I get your point. So, I suggest to always check SEV in BaseIoLib. If people
> meet with the performance issue, DXE/SMM version IoLib can be added
> later.
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package
2017-03-09 15:36 ` Duran, Leo
@ 2017-03-09 16:36 ` Laszlo Ersek
0 siblings, 0 replies; 33+ messages in thread
From: Laszlo Ersek @ 2017-03-09 16:36 UTC (permalink / raw)
To: Duran, Leo, 'Gao, Liming', Justen, Jordan L,
Kinney, Michael D, edk2-devel@lists.01.org, Brijesh Singh
Cc: Lendacky, Thomas, brijesh.sing@amd.com
On 03/09/17 16:36, Duran, Leo wrote:
> OK, what I'm hearing is:
> - Let's leave the Fifo routines in BaseIoLib (as we currently have)
> - And let's add the SEV checks to BaseIoLib, so that OvmPkg (et al) can consume it as-is
>
> If so, I could put a patch-set together for that... Please confirm.
Confirmed from my side.
> BTW, I'd try to minimize the need for CPUID (e.g., check it once and cache the result)
In a general purpose BASE library instance, you can't rely on memory
(writeable memory) being available. The library instance could be linked
into SEC or PEI phase modules that (generally speaking) have no
writeable global variables.
It's only an OvmfPkg specialty that PEI has writeable global variables.
(And it comes with a non-intuitive downside: in the SMM-less build of
OVMF, on S3 resume, the PEI global variables are not re-initialized to
the values that the C language would require. This is something we
always have to keep in mind.)
I don't think the CPUID checks will have a huge performance impact,
especially because (IIUC) you only have to customize the Fifo routines.
The cost of the CPUID will be amortized over all the individual IO port
accesses (--> traps) that you will perform individually anyway.
IMO the FeaturePCD check (which will be evaluated at compile time) is a
valid requirement, the CPUID result caching is not -- not until you can
measure the CPUID's impact to be "grave" under "general" workloads. Even
then, the separate DxeSmm instance can be added incrementally (as
suggested by others).
Thanks!
Laszlo
>
> Leo
>
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Wednesday, March 08, 2017 7:48 PM
>> To: Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
>> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
>> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Duran, Leo
>> <leo.duran@amd.com>; brijesh.sing@amd.com
>> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>> BaseIoLibIntrinsic package
>>
>>
>>
>>> -----Original Message-----
>>> From: Justen, Jordan L
>>> Sent: Thursday, March 9, 2017 2:59 AM
>>> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
>>> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
>>> Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
>> brijesh.sing@amd.com
>>> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>>> BaseIoLibIntrinsic package
>>>
>>> On 2017-03-08 07:41:58, Gao, Liming wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Justen, Jordan L
>>>>
>>>>> One other thought is, should we consider a DxeSmm alternative .inf
>>>>> for BaseIoLibIntrinsic.inf? In that case we could use a global
>>>>> variable to help out. Maybe this could prevent the concern that
>>>>> might drive a new PCD to be added?
>>>>>
>>>>> -Jordan
>>>> Current patch has stored the check state into data section. In PEI
>>>> phase, the data section can't be written. So, every call will check
>>>> CpuId. In DXE and SMM phase, the data section can be written. The
>>>> first call will cache the check state. So, no DxeSmm INF is
>>>> required.
>>>>
>>>
>>> I don't think we can attempt to write a variable to memory in generic
>>> SEC/PEI code. Some flash memory treat memory writes as an I/O for
>>> programming purposes. I think we added
>>> PcdGuidedExtractHandlerTableAddress for this reason. This is why I
>>> suggested that maybe we could add a DXE/SMM .inf where we could
>> assume
>>> writeable global variables exit.
>>>
>>> -Jordan
>>
>> I get your point. So, I suggest to always check SEV in BaseIoLib. If people
>> meet with the performance issue, DXE/SMM version IoLib can be added
>> later.
>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC PATCH v1 5/5] OvmfPkg/BaseIoLibIntrinsic: Unroll String I/O when SEV is active
2017-03-06 23:27 [RFC PATCH v1 0/5] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
` (3 preceding siblings ...)
2017-03-06 23:27 ` [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package Brijesh Singh
@ 2017-03-06 23:28 ` Brijesh Singh
[not found] ` <5a66f334-27e1-3b49-150e-c01209ecb2f6@amd.com>
4 siblings, 1 reply; 33+ messages in thread
From: Brijesh Singh @ 2017-03-06 23:28 UTC (permalink / raw)
To: jordan.l.justen, edk2-devel, lersek
Cc: Thomas.Lendacky, leo.duran, brijesh.sing
Secure Encrypted Virtualization (SEV) does not support string I/O, so
unroll the string I/O operation into a loop operating on one element at
a time.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
.../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 3
.../Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm | 19 +++
.../Library/BaseIoLibIntrinsic/Ia32/SevIoFifo.nasm | 141 ++++++++++++++++++++
OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm | 19 +++
.../Library/BaseIoLibIntrinsic/X64/SevIoFifo.nasm | 143 ++++++++++++++++++++
5 files changed, 324 insertions(+), 1 deletion(-)
create mode 100644 OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/SevIoFifo.nasm
create mode 100644 OvmfPkg/Library/BaseIoLibIntrinsic/X64/SevIoFifo.nasm
diff --git a/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
index 8844b1c..e7eeb59 100644
--- a/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+++ b/OvmfPkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
@@ -28,7 +28,6 @@
VERSION_STRING = 1.0
LIBRARY_CLASS = IoLib
-
#
# VALID_ARCHITECTURES = IA32 X64 EBC IPF ARM AARCH64
#
@@ -45,6 +44,7 @@
IoLib.c
Ia32/IoFifo.nasm
Ia32/IoFifo.asm
+ Ia32/SevIoFifo.nasm
[Sources.X64]
IoLibGcc.c | GCC
@@ -53,6 +53,7 @@
IoLib.c
X64/IoFifo.nasm
X64/IoFifo.asm
+ X64/SevIoFifo.nasm
[Sources.EBC]
IoLibEbc.c
diff --git a/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
index bcaa743..fb585e6 100644
--- a/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
+++ b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
@@ -13,6 +13,10 @@
;
;------------------------------------------------------------------------------
+ EXTERN ASM_PFX(SevIoReadFifo8)
+ EXTERN ASM_PFX(SevIoReadFifo16)
+ EXTERN ASM_PFX(SevIoReadFifo32)
+
SECTION .text
;------------------------------------------------------------------------------
@@ -31,7 +35,12 @@ ASM_PFX(IoReadFifo8):
mov dx, [esp + 8]
mov ecx, [esp + 12]
mov edi, [esp + 16]
+ call SevIoReadFifo8
+ cmp ecx, 0
+ jz IoReadFifo8Exit
rep insb
+
+IoReadFifo8Exit:
pop edi
ret
@@ -51,7 +60,12 @@ ASM_PFX(IoReadFifo16):
mov dx, [esp + 8]
mov ecx, [esp + 12]
mov edi, [esp + 16]
+ call SevIoReadFifo16
+ cmp ecx, 0
+ jz IoReadFifo16Exit
rep insw
+
+IoReadFifo16Exit:
pop edi
ret
@@ -71,7 +85,12 @@ ASM_PFX(IoReadFifo32):
mov dx, [esp + 8]
mov ecx, [esp + 12]
mov edi, [esp + 16]
+ call SevIoReadFifo32
+ cmp ecx, 0
+ jz IoReadFifo32Exit
rep insd
+
+IoReadFifo32Exit:
pop edi
ret
diff --git a/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/SevIoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/SevIoFifo.nasm
new file mode 100644
index 0000000..ac6bee3
--- /dev/null
+++ b/OvmfPkg/Library/BaseIoLibIntrinsic/Ia32/SevIoFifo.nasm
@@ -0,0 +1,141 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
+;
+; This program and the accompanying materials are licensed and made available
+; under the terms and conditions of the BSD License which accompanies this
+; distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+%define KVM_FEATURE_SEV 8
+
+ SECTION .data
+SevCheckedOnce db 0
+SevStatus db 0
+
+ SECTION .text
+
+;------------------------------------------------------------------------------
+; Check if Secure Encrypted Virtualization (SEV) feature
+; is enabled in KVM
+;
+; Return // eax (1 - active, 0 - not active)
+;------------------------------------------------------------------------------
+global ASM_PFX(CheckSevFeature)
+ASM_PFX(CheckSevFeature):
+ ; Check CPUID once, if its already checked then return SevStatus
+ mov eax, 1
+ cmp [SevCheckedOnce], eax
+ jz SevFeatureCheckExit
+
+ ; Start the SEV feature check
+ mov [SevCheckedOnce], eax
+
+ ; CPUID clobbers ebx, ecx and edx
+ push ebx
+ push ecx
+ push edx
+
+ mov eax, 0x40000001
+ cpuid
+
+ bt eax, KVM_FEATURE_SEV
+ jnc SevCheckExit
+
+ ; Check for memory encryption feature:
+ ; CPUID Fn8000_001F[EAX] - Bit 0
+ ;
+ mov eax, 0x8000001f
+ cpuid
+ bt eax, 0
+ jnc SevCheckExit
+ mov eax, 1
+ mov [SevStatus], eax
+
+SevCheckExit:
+ pop edx
+ pop ecx
+ pop ebx
+
+SevFeatureCheckExit:
+ mov eax, [SevStatus]
+ ret
+
+;------------------------------------------------------------------------------
+; unroll 'rep ins' String I/O instructions when SEV is active
+; nothing
+;
+; Port // dx
+; Size // ecx
+; Buffer // rdi
+;
+;------------------------------------------------------------------------------
+global ASM_PFX(SevIoReadFifo8)
+ASM_PFX(SevIoReadFifo8):
+ call CheckSevFeature
+ cmp eax, 1
+ jnz ReadFifo8Exit
+ReadFifo8Loop:
+ cmp ecx, 0
+ jz ReadFifo8Exit
+ in al, dx
+ mov [edi], al
+ dec ecx
+ inc edi
+ jmp ReadFifo8Loop
+ReadFifo8Exit:
+ ret
+
+;------------------------------------------------------------------------------
+; unroll 'rep insw' String I/O instructions when SEV is active
+;
+; Port // dx
+; Size // ecx
+; Buffer // rdi
+;
+;------------------------------------------------------------------------------
+global ASM_PFX(SevIoReadFifo16)
+ASM_PFX(SevIoReadFifo16):
+ call CheckSevFeature
+ cmp eax, 1
+ jnz ReadFifo16Exit
+ReadFifo16Loop:
+ cmp ecx, 0
+ jz ReadFifo16Exit
+ in ax, dx
+ mov [edi], ax
+ dec ecx
+ add edi, 2
+ jmp ReadFifo16Loop
+ReadFifo16Exit:
+ ret
+
+;------------------------------------------------------------------------------
+; unroll 'rep insl' String I/O instructions when SEV is active
+;
+; Port // dx
+; Size // ecx
+; Buffer // rdi
+;
+;------------------------------------------------------------------------------
+global ASM_PFX(SevIoReadFifo32)
+ASM_PFX(SevIoReadFifo32):
+ call CheckSevFeature
+ cmp eax, 1
+ jnz ReadFifo32Exit
+ReadFifo32Loop:
+ cmp ecx, 0
+ jz ReadFifo32Exit
+ in eax, dx
+ mov [edi], eax
+ dec ecx
+ add edi, 4
+ jmp ReadFifo32Loop
+ReadFifo32Exit:
+ ret
+
diff --git a/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
index 7bd72d0..71fbe62 100644
--- a/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
+++ b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
@@ -13,6 +13,10 @@
;
;------------------------------------------------------------------------------
+ EXTERN ASM_PFX(SevIoReadFifo8)
+ EXTERN ASM_PFX(SevIoReadFifo16)
+ EXTERN ASM_PFX(SevIoReadFifo32)
+
DEFAULT REL
SECTION .text
@@ -30,7 +34,12 @@ ASM_PFX(IoReadFifo8):
cld
xchg rcx, rdx
xchg rdi, r8 ; rdi: buffer address; r8: save rdi
+ call SevIoReadFifo8
+ cmp ecx, 0
+ jz IoReadFifo8Exit
rep insb
+
+IoReadFifo8Exit:
mov rdi, r8 ; restore rdi
ret
@@ -48,7 +57,12 @@ ASM_PFX(IoReadFifo16):
cld
xchg rcx, rdx
xchg rdi, r8 ; rdi: buffer address; r8: save rdi
+ call SevIoReadFifo16
+ cmp ecx, 0
+ jz IoReadFifo16Exit
rep insw
+
+IoReadFifo16Exit:
mov rdi, r8 ; restore rdi
ret
@@ -66,7 +80,12 @@ ASM_PFX(IoReadFifo32):
cld
xchg rcx, rdx
xchg rdi, r8 ; rdi: buffer address; r8: save rdi
+ call SevIoReadFifo32
+ cmp ecx, 0
+ jz IoReadFifo32Exit
rep insd
+
+IoReadFifo32Exit:
mov rdi, r8 ; restore rdi
ret
diff --git a/OvmfPkg/Library/BaseIoLibIntrinsic/X64/SevIoFifo.nasm b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/SevIoFifo.nasm
new file mode 100644
index 0000000..5e70cb6
--- /dev/null
+++ b/OvmfPkg/Library/BaseIoLibIntrinsic/X64/SevIoFifo.nasm
@@ -0,0 +1,143 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
+;
+; This program and the accompanying materials are licensed and made available
+; under the terms and conditions of the BSD License which accompanies this
+; distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+%define KVM_FEATURE_SEV 8
+
+ EXTERN ASM_PFX(SevEnabled)
+
+ SECTION .data
+SevCheckedOnce db 0
+SevStatus db 0
+
+ SECTION .text
+
+;------------------------------------------------------------------------------
+; Check if Secure Encrypted Virtualization (SEV) feature
+; is enabled in KVM
+;
+; Return // eax (1 - active, 0 - not active)
+;------------------------------------------------------------------------------
+global ASM_PFX(CheckSevFeature)
+ASM_PFX(CheckSevFeature):
+ ; Check CPUID once, if its already checked then return SevStatus
+ mov eax, 1
+ cmp [SevCheckedOnce], eax
+ jz SevFeatureCheckExit
+
+ ; Start the SEV feature check
+ mov [SevCheckedOnce], eax
+
+ ; CPUID clobbers ebx, ecx and edx
+ push rbx
+ push rcx
+ push rdx
+
+ mov eax, 0x40000001
+ cpuid
+
+ bt eax, KVM_FEATURE_SEV
+ jnc SevCheckExit
+
+ ; Check for memory encryption feature:
+ ; CPUID Fn8000_001F[EAX] - Bit 0
+ ;
+ mov eax, 0x8000001f
+ cpuid
+ bt eax, 0
+ jnc SevCheckExit
+ mov eax, 1
+ mov [SevStatus], eax
+
+SevCheckExit:
+ pop rdx
+ pop rcx
+ pop rbx
+
+SevFeatureCheckExit:
+ mov eax, [SevStatus]
+ ret
+
+;------------------------------------------------------------------------------
+; unroll 'rep ins' String I/O instructions when SEV is active
+; nothing
+;
+; Port // dx
+; Size // ecx
+; Buffer // rdi
+;
+;------------------------------------------------------------------------------
+global ASM_PFX(SevIoReadFifo8)
+ASM_PFX(SevIoReadFifo8):
+ call CheckSevFeature
+ cmp eax, 1
+ jnz ReadFifo8Exit
+ReadFifo8Loop:
+ cmp ecx, 0
+ jz ReadFifo8Exit
+ in al, dx
+ mov [edi], al
+ dec ecx
+ inc edi
+ jmp ReadFifo8Loop
+ReadFifo8Exit:
+ ret
+
+;------------------------------------------------------------------------------
+; unroll 'rep insw' String I/O instructions when SEV is active
+;
+; Port // dx
+; Size // ecx
+; Buffer // rdi
+;
+;------------------------------------------------------------------------------
+global ASM_PFX(SevIoReadFifo16)
+ASM_PFX(SevIoReadFifo16):
+ call CheckSevFeature
+ cmp eax, 1
+ jnz ReadFifo16Exit
+ReadFifo16Loop:
+ cmp ecx, 0
+ jz ReadFifo16Exit
+ in ax, dx
+ mov [edi], ax
+ dec ecx
+ add edi, 2
+ jmp ReadFifo16Loop
+ReadFifo16Exit:
+ ret
+
+;------------------------------------------------------------------------------
+; unroll 'rep insl' String I/O instructions when SEV is active
+;
+; Port // dx
+; Size // ecx
+; Buffer // rdi
+;
+;------------------------------------------------------------------------------
+global ASM_PFX(SevIoReadFifo32)
+ASM_PFX(SevIoReadFifo32):
+ call CheckSevFeature
+ cmp eax, 1
+ jnz ReadFifo32Exit
+ReadFifo32Loop:
+ cmp ecx, 0
+ jz ReadFifo32Exit
+ in eax, dx
+ mov [edi], eax
+ dec ecx
+ add edi, 4
+ jmp ReadFifo32Loop
+ReadFifo32Exit:
+ ret
+
^ permalink raw reply related [flat|nested] 33+ messages in thread