Re: [PATCH 2/2] OvmfPkg/PlatformPei: DENY_EXECUTE_ON_SECURITY_VIOLATION when SEV is active

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

Hi Laszlo,

On 10/05/2017 02:46 PM, Laszlo Ersek wrote:

....>>   ################################################################################
>>   #
>>   # Components Section - list of all EDK II Modules needed by this Platform.
> 
> (1) It's hard to see what section of the DSC file the lines are moved
> from, and to what section.
> 
> Git can be configured so that it display the section names in the diff
> hunk header (following the "@@" marks). Please refer to the following:
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09
> 
> In particular, see the "diff.ini.xfuncname" config option, and the
> matching "diff=ini" attributes.
> 
> (This remark does not affect the patch, only how it is formatted.
> Showing the DSC section names in the diff hunk headers helps quite a bit
> with review.)
> 

I recently changed by build box and I did carried over my previous gitconfig
setting so that it generate the correct formating per your week but I might
have missed something in the process. I will fix it. sorry about that.


> 
> 
> (2) Please apply the same code movement to "OvmfPkg/OvmfPkgIa32.dsc".
> 
> As written, the PcdSet32S() macro invocation in PlatformPei below will
> not even compile for OvmfPkgIa32. Namely, the PCD will remain
> fixed-at-build, and because of that, the replacement text of the
> PcdSet32S() macro, i.e.
> 
>    _PCD_SET_MODE_32_S_##TokenName    ((Value))
> 
> from
> 
>    MdePkg/Include/Library/PcdLib.h
> 
> will not have any *further* replacement text, from "AutoGen.h".
> 

Ah good catch. I will fix in v2.

>>
> 
> (3) I find it sub-optimal that the
> 
>    DENY_EXECUTE_ON_SECURITY_VIOLATION
> 
> macro, from
> 
>    SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
> 
> is internal to that library instance, and isn't exposed in some library
> class header.
> 

Yep, I had similar concern but as you said SecurityPkg.dec spells out
very well hence I did not bother to move the header file from internal
to somewhere its accessible by other modules.


> Anyway, I guess we can't do much about it here, and at least
> SecurityPkg.dec spells out the meanings of the values. So this hunk
> looks fine to me.
>