Re: [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack

[Laszlo Ersek <lersek@redhat.com>]

On 11/13/17 19:25, Jordan Justen wrote:
> On 2017-11-10 07:49:06, Laszlo Ersek wrote:
>> diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>> index 54d074e621f6..1d426fafa888 100644
>> --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm
>> +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm
>> @@ -29,6 +29,7 @@ extern ASM_PFX(SecCoreStartupWithStack)
>>  ; @param[in]  EAX   Initial value of the EAX register (BIST: Built-in Self Test)
>>  ; @param[in]  DI    'BP': boot-strap processor, or 'AP': application processor
>>  ; @param[in]  EBP   Pointer to the start of the Boot Firmware Volume
>> +; @param[in]  ES    Set to LINEAR_SEL in TransitionFromReal16To32BitFlat
> 
> Can you document all the segment registers, and also document them in
> UefiCpuPkg/ResetVector/Vtf0/Main.asm?

Do you mean the above format (i.e., @param[in]...), just repeated for
the other segment registers too?

Regarding "UefiCpuPkg/ResetVector/Vtf0/Main.asm", what format do you
suggest? The @param[in]... format wouldn't be right, because the segment
registers are set up in TransitionFromReal16To32BitFlat. Should I write
a free-form comment / list above

    OneTimeCall TransitionFromReal16To32BitFlat

?

> 
>>  ;
>>  ; @return     None  This routine does not return
>>  ;
>> @@ -44,6 +45,18 @@ ASM_PFX(_ModuleEntryPoint):
>>      mov     esp, ebx
>>      nop
>>  
>> +    ;
>> +    ; Fill the temporary RAM with the initial stack value.
>> +    ; The loop below will seed the heap as well, but that's harmless.
>> +    ;
>> +    mov     eax, FixedPcdGet32 (PcdInitValueInTempStack)  ; dword to store
>> +    mov     edi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) ; base address,
>> +                                                          ;   relative to ES
>> +    mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) ; byte count
>> +    shr     ecx, 2                                        ; dword count
> 
> I'm not sure, but I think NASM might let you do something like:
> 
>     mov     ecx, FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 4
OK, I can try that. I was worried about NASM arithmetic in general, but
I see the "info" page describes "Multiplication and Division", so the
semantics should be well-defined.

> 
>> +    cld                                                   ; store from base up
>> +    rep stosd
> 
> I think if you move this above the code in patch 1, then patch 1 is
> not needed.

I think I agree (to be seen in practice :) )

> I also think it would be reasonable to merge 2 & 3, but
> separate is fine too.

If I remove the "shr" from both, then I might feel tempted to merge
them; I'm not sure yet. For testing at least I prefer to keep them separate.

Thanks
Laszlo