On 2015-05-18 17:40:10, Yao, Jiewen wrote: > IntelFspPkg - FspSecCore add AsmGetFspBaseAddressNoStack and > AsmGetFspInfoHeaderNoStack
Line is 81 columns. It should also be "IntelFspPkg/FspSecCore:" rather than "IntelFspPkg - FspSecCore" https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format > Fix GCC issue on FspInfoHeaderRelativeOff. > > Clean up comments for platform ID matching on Microcode and > PcdFspBootFirmwareVolumeBase Why are you doing several separate things in one patch? Why not three separate patches? 1. Cleanup comments 2. Add AsmGetFspBaseAddressNoStack 3. Add AsmGetFspInfoHeaderNoStack Where do these function get used? There is no call to them? > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: "Yao, Jiewen" <jiewen....@intel.com> > > Reviewed-by: "Rangarajan, Ravi P" <ravi.p.rangara...@intel.com> > > Reviewed-by: "Mudusuru, Giri P" <giri.p.mudus...@intel.com> > > Reviewed-by: "Ma, Maurice" <maurice...@intel.com> Lots of review before edk2-devel... :) > Thank you > > Yao Jiewen Comments on patch: > Index: FspSecCore/Ia32/FspApiEntry.asm > =================================================================== > --- FspSecCore/Ia32/FspApiEntry.asm (revision 17286) > +++ FspSecCore/Ia32/FspApiEntry.asm (working copy) > @@ -143,8 +143,8 @@ > mov ecx, MSR_IA32_PLATFORM_ID > rdmsr > mov ecx, edx > - shr ecx, 50-32 > - and ecx, 7h > + shr ecx, 50-32 ; shift (50d-32d=18d=0x12) bits > + and ecx, 7h ; platform id at bit[52..50] > mov edx, 1 > shl edx, cl > > @@ -569,7 +569,7 @@ > ; > ; Pass BFV into the PEI Core > ; It uses relative address to calucate the actual boot FV base > - ; For FSP impleantion with single FV, PcdFlashFvRecoveryBase and > + ; For FSP implementation with single FV, PcdFspBootFirmwareVolumeBase and > ; PcdFspAreaBaseAddress are the same. For FSP with mulitple FVs, > ; they are different. The code below can handle both cases. > ; > Index: FspSecCore/Ia32/FspApiEntry.s > =================================================================== > --- FspSecCore/Ia32/FspApiEntry.s (revision 17286) > +++ FspSecCore/Ia32/FspApiEntry.s (working copy) > @@ -297,8 +297,8 @@ > movl $MSR_IA32_PLATFORM_ID, %ecx > rdmsr > movl %edx, %ecx > - shrl $0x12, %ecx #($50-$32) > - andl $0x07, %ecx > + shrl $0x12, %ecx # shift (50d-32d=18d=0x12) bits Can't you use? shrl $(50-32), %ecx # shift (50d-32d=18d=0x12) bits > + andl $0x07, %ecx # platform id at bit[52..50] > movl $0x01, %edx > shll %cl,%edx > @@ -784,7 +784,7 @@ > # > # Pass BFV into the PEI Core > # It uses relative address to calucate the actual boot FV base > - # For FSP impleantion with single FV, PcdFlashFvRecoveryBase and > + # For FSP implementation with single FV, PcdFspBootFirmwareVolumeBase and > # PcdFspAreaBaseAddress are the same. For FSP with mulitple FVs, > # they are different. The code below can handle both cases. > # > Index: FspSecCore/Ia32/FspHelper.asm > =================================================================== > --- FspSecCore/Ia32/FspHelper.asm (revision 17286) > +++ FspSecCore/Ia32/FspHelper.asm (working copy) > @@ -15,6 +15,10 @@ > .model flat,C > .code > > +; > +; FspInfoHeaderRelativeOff is patched during build process and initialized > to offset of the AsmGetFspBaseAddress > +; from the FSP Info header. This line is much longer than 80 columns. > +; > FspInfoHeaderRelativeOff PROC NEAR PUBLIC > ; > ; This value will be pached by the build script > @@ -22,6 +26,11 @@ > DD 012345678h > FspInfoHeaderRelativeOff ENDP > > +; > +; Returns FSP Base Address. > +; > +; This function gets the FSP Info Header using relative addressing and > returns the FSP Base from the header structure This line is much longer than 80 columns. > +; > AsmGetFspBaseAddress PROC NEAR PUBLIC > mov eax, AsmGetFspBaseAddress > sub eax, dword ptr [FspInfoHeaderRelativeOff] > @@ -30,6 +39,22 @@ > ret > AsmGetFspBaseAddress ENDP > > +; > +; No stack counte part of AsmGetFspBaseAddress. Return address is in edi. counter What about this instead: There is no stack available, so the return address is in edi. > +; > +AsmGetFspBaseAddressNoStack PROC NEAR PUBLIC > + mov eax, AsmGetFspBaseAddress > + sub eax, dword ptr [FspInfoHeaderRelativeOff] > + add eax, 01Ch > + mov eax, dword ptr [eax] > + jmp edi > +AsmGetFspBaseAddressNoStack ENDP > + > +; > +; Returns FSP Info Header. > +; > +; This function gets the FSP Info Header using relative addressing and > returns it Line is 81 columns > +; > AsmGetFspInfoHeader PROC NEAR PUBLIC > mov eax, AsmGetFspBaseAddress > sub eax, dword ptr [FspInfoHeaderRelativeOff] > @@ -36,4 +61,13 @@ > ret > AsmGetFspInfoHeader ENDP > > +; > +; No stack counte part of AsmGetFspInfoHeader. Return address is in edi. counter. Maybe change like I recommended above? > +; > +AsmGetFspInfoHeaderNoStack PROC NEAR PUBLIC > + mov eax, AsmGetFspBaseAddress > + sub eax, dword ptr [FspInfoHeaderRelativeOff] > + jmp edi > +AsmGetFspInfoHeaderNoStack ENDP > + > END > \ No newline at end of file > Index: FspSecCore/Ia32/FspHelper.s > =================================================================== > --- FspSecCore/Ia32/FspHelper.s (revision 17286) > +++ FspSecCore/Ia32/FspHelper.s (working copy) > @@ -15,6 +15,10 @@ > # > > #------------------------------------------------------------------------------ > > +# > +# FspInfoHeaderRelativeOff is patched during build process and initialized > to offset of the AsmGetFspBaseAddress > +# from the FSP Info header. This comment is much longer than 80 columns. > +# > ASM_GLOBAL ASM_PFX(FspInfoHeaderRelativeOff) > ASM_PFX(FspInfoHeaderRelativeOff): > # > @@ -22,17 +26,46 @@ > # > .long 0x012345678 > > - > +# > +# Returns FSP Base Address. > +# > +# This function gets the FSP Info Header using relative addressing and > returns the FSP Base from the header structure > +# This comment is much longer than 80 columns. > ASM_GLOBAL ASM_PFX(AsmGetFspBaseAddress) > ASM_PFX(AsmGetFspBaseAddress): > mov $AsmGetFspBaseAddress, %eax > - sub $FspInfoHeaderRelativeOff, %eax > + sub FspInfoHeaderRelativeOff, %eax > add $0x01C, %eax > mov (%eax), %eax > ret > > +# > +# No stack counte part of AsmGetFspBaseAddress. Return address is in edi. counter. Maybe change like I recommended above? > +# > +ASM_GLOBAL ASM_PFX(AsmGetFspBaseAddressNoStack) > +ASM_PFX(AsmGetFspBaseAddressNoStack): > + mov $AsmGetFspBaseAddress, %eax > + sub FspInfoHeaderRelativeOff, %eax > + add $0x01C, %eax > + mov (%eax), %eax > + jmp *%edi > + > +# > +# Returns FSP Info Header. > +# > +# This function gets the FSP Info Header using relative addressing and > returns it Line is 81 columns > +# > ASM_GLOBAL ASM_PFX(AsmGetFspInfoHeader) > ASM_PFX(AsmGetFspInfoHeader): > mov $AsmGetFspBaseAddress, %eax > - sub $FspInfoHeaderRelativeOff, %eax > + sub FspInfoHeaderRelativeOff, %eax > ret > + > +# > +# No stack counte part of AsmGetFspInfoHeader. Return address is in edi. counter. Maybe change like I recommended above? > +# > +ASM_GLOBAL ASM_PFX(AsmGetFspInfoHeaderNoStack) > +ASM_PFX(AsmGetFspInfoHeaderNoStack): > + mov $AsmGetFspBaseAddress, %eax > + sub FspInfoHeaderRelativeOff, %eax > + jmp *%edi ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
