From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.1394.1621127833983457422 for ; Sat, 15 May 2021 18:17:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GKTgCYsf; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621127832; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HX+8Uw7IKDuzvojWDjKYhKXRn36dDq5FkUY2Uy74/jY=; b=GKTgCYsfrvRPTeTwj8ASapxcxqF1jngtyAd+HN5b7ZJ30Wne0/SeOHUuWgT08JV5uYR5p2 gzBF+4ciNAYKozi5nvJjNV8pLpouOc2hLPWESX3WCS08O/4qv26IOIauMfeBmUk5pErGUw C6wTElktX5NGYvXEJl/Xkaeg4bKtNzs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-271-uTZ2h2tINMyGVflZManmsg-1; Sat, 15 May 2021 21:17:09 -0400 X-MC-Unique: uTZ2h2tINMyGVflZManmsg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EFF6BFCA9; Sun, 16 May 2021 01:17:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-2.ams2.redhat.com [10.36.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E90C813441; Sun, 16 May 2021 01:17:04 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: =?UTF-8?Q?Marvin_H=c3=a4user?= , devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , Eric Dong , Ray Ni , Rahul Kumar References: <1162d1f4fe09048aaafba6f6ea3046bebd34fc2b.1620766224.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <414ed071-6bed-4e41-400d-7d958b27ae3b@redhat.com> Date: Sun, 16 May 2021 03:17:03 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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