From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 31 Jul 2019 05:43:56 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C1B523082120; Wed, 31 Jul 2019 12:43:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-110.ams2.redhat.com [10.36.116.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8D66D5D6D0; Wed, 31 Jul 2019 12:43:54 +0000 (UTC) Subject: Re: [Patch 0/2] UefiCpuPkg: Default avoid print. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni , Michael Kinney References: <20190731073502.24640-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 31 Jul 2019 14:43:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190731073502.24640-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Wed, 31 Jul 2019 12:43:55 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (adding Mike) On 07/31/19 09:35, Eric Dong wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1984 > > Current debug message brings much restriction for the platform > which use this driver. > > For PEI and DXE phase, platform mush link base DebugLib (without > using any pei/dxe services, even for its dependent libraries). > > This patch default disable this debug message, only open it when > need to debug the related code. > > Signed-off-by: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > > Eric Dong (2): > UefiCpuPkg/RegisterCpuFeaturesLib: Default avoid print. > UefiCpuPkg/PiSmmCpuDxeSmm: Default avoid print. > > .../Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 4 +++- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 4 +++- > 2 files changed, 6 insertions(+), 2 deletions(-) > The basic problem seems to be that APs should not use "thick" services that might underlie the DebugLib instance that is picked by the platform. That requirement appears sane to me. I think I disagree with the proposed mitigation though. Reasons: (a) The mitigation is duplicated to independent modules. (b) It is not possible to change the debug mask without modifying C language source code. (c) Passing a zero log mask to DEBUG() on the APs does not guarantee thread safety: - The DEBUG() macro calls DebugPrintEnabled() regardless of the log mask passed to DEBUG(). - The DEBUG() macro may or may not call DebugPrintLevelEnabled(), dependent on architecture & toolchain. - Both DebugPrintEnabled() and DebugPrintLevelEnabled() are DebugLib interfaces. The library instance may implement them unsafely for APs, and a zero log mask at the DEBUG call site could not prevent that. - Finally, DebugPrint() itself could invoke thread-unsafe logic, before consulting the log mask. I would propose the following, instead: (i) Introduce BIT6 for PcdDebugPropertyMask in "MdePkg.dec". The default value should be zero. The bit stands for "DEBUG is safe to call on APs". (ii) Add a macro called AP_DEBUG to . This macro should work the same as DEBUG, except it should do nothing if BIT6 in PcdDebugProperyMask is clear. Fetching PcdDebugPropertyMask inside AP_DEBUG() is safe, because: - the PCD can only be fixed-at-build or patchable-in-module (therefore it is safe to read on APs -- no PCD PPI or PCD Protocol is needed); - PcdDebugPropertyMask is a preexistent PCD that *all* existent DebugLib instances are expected to consume -- per the API specifications in --, therefore no new PCD dependency would be introduced to DebugLib instances. (iii) Modules that call DEBUG on APs should replace those calls with AP_DEBUG. Code that currently calls DEBUG while running on either BSP or APs should discriminate those cases from each other, and use AP_DEBUG explicitly, when it runs on APs. As a further refinement, a macro called MP_DEBUG could be introduced too, with a new initial parameter called "Bsp". If the Bsp parameter is TRUE, then MP_DEBUG is identical to DEBUG. Otherwise, MP_DEBUG is identical to AP_DEBUG. This way, DEBUG() calls such as described above wouldn't have to be split into DEBUG / AP_DEBUG calls; they could be changed into MP_DEBUG calls (with an extra parameter in the front). (iv) platforms can set BIT6 in PcdDebugPropertyMask in DSC files. This need not be a full platform-level setting: the PCD can be overridden in module scope, just like the DebugLib resolution can be module-scoped. As an end result, AP_DEBUG messages will disappear by default (safely), and platforms will have to do extra work only if they want AP_DEBUG messages to appear. Otherwise the change is transparent to platforms. And, I think that AP_DEBUG belongs in MdePkg (and not UefiCpuPkg) because both DebugLib and EFI_MP_SERVICES_PROTOCOL are declared in MdePkg. While UefiCpuPkg provides the multiprocessing implementation for IA32 and X64, the problem is architecture-independent. Furthermore, the problem is a long-standing and recurrent one -- please refer to commit 81f560498bf1, for example --, so it makes sense to solve it once and for all. Thanks Laszlo