Re: [PATCH v5 04/14] OvmfPkg/BaseMemcryptSevLib: Add SEV helper library

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

On 05/24/2017 08:06 AM, Laszlo Ersek wrote:
> On 05/22/17 17:23, Brijesh Singh wrote:
>> Add Secure Encrypted Virtualization (SEV) helper library.
>> The library provides the routines to:
>> -  set or clear memory encryption bit for a given memory region.
>> -  query whether SEV is enabled.
>>
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   OvmfPkg/OvmfPkgIa32.dsc                                         |   1 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                                      |   1 +
>>   OvmfPkg/OvmfPkgX64.dsc                                          |   1 +
>>   OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf   |  50 +++
>>   OvmfPkg/Include/Library/MemEncryptSevLib.h                      |  81 ++++
>>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h        | 184 +++++++++
>>   OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c    |  84 ++++
>>   OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c |  90 ++++
>>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c     |  84 ++++
>>   OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c        | 428 ++++++++++++++++++++
>>   10 files changed, 1004 insertions(+)
> 
> I have some comments for the case if you have to post a v6. Right now a
> v6 is not needed just for these comments.
> 
> (1) In
> <http://mid.mail-archive.com/1d04baaa-95c4-492a-57a0-3d91aea02c36@redhat.com>
> I mentioned
> 
> "Since this is a BASE library, please don't use EFI_STATUS,
> EFI_INVALID_PARAMETER, EFI_NO_MAPPING, EFI_SUCCESS; use RETURN_xxx instead."
> 
> You replaced most of them, but you left in "EFI_STATUS". That should be
> "RETURN_STATUS".
> 
> (2) please check the lines where you added (as I asked, thanks)
> gEfiCallerBaseName and __FUNCTION__. On most lines, the indentation is
> incorrect, relative to "DEBUG ((".
> 
> (3) Furthermore, in some spots where you added __FUNCTION__, you forgot
> to add a space after the comma.
> 
> Again, no need to resubmit just because of this, but if you do resubmit
> anyway, these should be fixed up.
> 

Okay thanks, I believe I might have similar formating issues in other patches
and if we do v6 then I will take care of them all.

-Brijesh