Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area

["Laszlo Ersek" <lersek@redhat.com>]

On 05/14/21 17:44, Marvin Häuser wrote:
> On 14.05.21 17:23, Lendacky, Thomas wrote:
>> On 5/14/21 10:04 AM, Marvin Häuser wrote:

>>>> +      // Check to be sure that the "allocate below" behavior hasn't
>>>> changed.
>>>> +      // This will also catch a failed allocation, as "-1" is
>>>> returned on
>>>> +      // failure.
>>>> +      //
>>>> +      if (CpuMpData->SevEsAPResetStackStart >=
>>>> CpuMpData->WakeupBuffer) {
>>>> +        DEBUG ((DEBUG_ERROR,
>>>> +          "SEV-ES AP reset stack is not below wakeup buffer\n"));
>>>> +
>>>> +        ASSERT (FALSE);
>>> Should the ASSERT not only catch the broken "allocate below" behaviour,
>>> i.e. not trigger on failed allocation?
>> I think it's best to trigger on a failed allocation as well rather than
>> continuing and allowing a page fault or some other problem to occur.
> 
> Well, it should handle the error in a safe way, i.e. the deadloop below.
> To not ASSERT on plausible conditions is a common design guideline in
> most low-level projects including Linux kernel.
> 
> Best regards,
> Marvin
> 
>> Thanks,
>> Tom
>>
>>>> +        CpuDeadLoop ();

"DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.

In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
-- don't continue execution if the condition controlling the whole block
fired.

In DEBUG and NOOPT builds, the pattern will lead to a debug message
(usually at the "error" level), followed by an assertion failure. The
error message of the assertion failure is irrelevant ("FALSE"). The
point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
hangs execution is customizable, via "PcdDebugPropertyMask", unlike
CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
effect is the same -- the explicit CpuDeadLoop is not reached. In other
configs, ASSERT() can raise a debug exception (CpuBreakpoint()).

The required part of the pattern is CpuDeadLoop(); the DEBUG message
makes it more debugging-friendly, and the ASSERT(), with the tweakable
"hang method", makes it even more debugging-friendly.

Thanks
Laszlo