From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=ZshNpv8q; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by groups.io with SMTP; Thu, 01 Aug 2019 14:14:19 -0700 Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id x71LBq5Z048768; Thu, 1 Aug 2019 14:14:17 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=FoL+d1a6LWGPS0BngrbL6y+ooKdFfTNvzIXsrPCZ1O0=; b=ZshNpv8qTRH1yna/xC2/BrLFYmmGPHL/xnZIOCBansWDPFXId3wlQtQaSW1SP4YanFlW WuXyQkAqa5sVPbXmKftNnzU2zzN5nXeCu/yAtEzjJd9I3KBD/DugvqEMNoTuvE6tF8HL KR5+7923ZPHRMzicQckrrl3VJ0PV/jSwKZhyfeNH8olqZAFNGseWPcm5GEpIKdflngXi NX7b5o9WrYMfUpN+5rhdvgVeeTxHLma+mKt4o7Ohk3nR29uNuT5P6k39xe9U45D93ha3 KMQod5WkGK424QyVLIvNhZm0iosn5FMXf2s+fhQ+0dgSUtB3bmKkhq/QNX3pEY1Nsft2 kg== Received: from ma1-mtap-s01.corp.apple.com (ma1-mtap-s01.corp.apple.com [17.40.76.5]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 2u2tged5k1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 01 Aug 2019 14:14:17 -0700 Received: from nwk-mmpp-sz10.apple.com (nwk-mmpp-sz10.apple.com [17.128.115.122]) by ma1-mtap-s01.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PVK0065HTNSWE20@ma1-mtap-s01.corp.apple.com>; Thu, 01 Aug 2019 14:14:17 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz10.apple.com by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PVK00500TIYZD00@nwk-mmpp-sz10.apple.com>; Thu, 01 Aug 2019 14:14:16 -0700 (PDT) X-Va-A: X-Va-T-CD: f96143a6ea401bdc25dedb56e9fa8c43 X-Va-E-CD: 9e7cb8c6aa8d6eddfb837a735a3f2f20 X-Va-R-CD: 94a6eae90dc3c7c516495aa7ca0df601 X-Va-CD: 0 X-Va-ID: da588490-704a-4c60-97f2-f6d98b5bc535 X-V-A: X-V-T-CD: 9a944300b48f07f06aa7e0165fb7dbb6 X-V-E-CD: 9e7cb8c6aa8d6eddfb837a735a3f2f20 X-V-R-CD: 94a6eae90dc3c7c516495aa7ca0df601 X-V-CD: 0 X-V-ID: aeb76443-db6e-47ea-b4ae-187171e93ffd X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-08-01_09:,, signatures=0 Received: from [17.115.223.75] (unknown [17.115.223.75]) by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PVK00NCPTNP3700@nwk-mmpp-sz10.apple.com>; Thu, 01 Aug 2019 14:14:16 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [edk2-devel] [Patch 0/2] UefiCpuPkg: Default avoid print. Date: Thu, 01 Aug 2019 14:14:00 -0700 In-reply-to: Cc: Laszlo Ersek , Eric Dong , Ray Ni , Mike Kinney To: devel@edk2.groups.io, brian.johnson@hpe.com References: <20190731073502.24640-1-eric.dong@intel.com> <3a28f2c6-6ef1-c830-b3c6-3cf69c5ca60f@hpe.com> <93d4a7e7-9b86-b3c9-a476-ac1a40dc4723@redhat.com> X-Mailer: Apple Mail (2.3445.104.11) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-08-01_09:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_B83BDE7E-BA6A-4634-9A07-67906EC7268B" --Apple-Mail=_B83BDE7E-BA6A-4634-9A07-67906EC7268B Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii > On Aug 1, 2019, at 1:20 PM, Brian J. Johnson wro= te: >=20 > On 7/31/19 1:58 PM, Laszlo Ersek wrote: >> On 07/31/19 18:34, Brian J. Johnson wrote: >>> I do wonder if there would be a clean way to let a DebugLib instance >>> itself declare that AP_DEBUG() is safe. That way a platform would >>> only need to override the DebugLib instance in the DSC file, rather >>> than both the instance and the PCD. (I know, I'm nitpicking.) A >>> library can't override PCDs in its calling modules, of course. I >>> suppose the AP_DEBUG() macro could call a new DebugLib entry point to >>> test for AP safety before doing anything else, say >>> DebugPrintOnApIsSafe(). Or it could even be a global CONST BOOLEAN >>> defined by the library. But that would require all DebugLib instances >>> to change, which is something you were trying to avoid. >> That's right -- I tried to imagine some other approaches, but they'd >> need new DebugLib functions, and likely force platforms to write new >> code. >>> However, it's not always practical to track down all uses of DEBUG(). >>> An AP can easily call a library routine which uses DEBUG() rather than >>> AP_DEBUG(), buried under several layers of transitive library >>> dependencies. In other words, it's not always practical to determine >>> ahead of time if a given DEBUG() call may be done on an AP. >> This problem is valid IMO, but I think its scope is a lot wider than >> just DebugLib. Assume the programmer is looking at a function that may >> be invoked on an AP, and they are about to call a function for taking >> care of a specific sub-task. If the programmer cannot prove the >> thread-safety of the *entire* call tree underneath the function call >> they are about to add, they simply must not add the call. The >> thread-safety of the DebugLib instance in use is just a part of the >> thread-safety of said call tree. >> Put differently, code that runs APs must be extremely self-contained; >> I'd rule out any and all lib classes from direct use unless a specific >> library instance, advertizing thread safety, would be chosen in the >> platform DSC file. But, if we adopted this approach, we could even >> introduce a new AP-oriented library *class* for debug messages, offer a >> Null implementation in edk2, and ask platforms to bring their own. >>> I know that AP code runs in a very restricted environment and that >>> people who use MpServices are supposed to understand the >>> repercussions, but it gets very difficult when libraries are >>> involved. :( >> Exactly -- the first restriction people should understand is, "stay awa= y >> from libraries as much as you can". >>> So would a better solution be to modify the common unsafe DebugLib >>> instances to have DebugPrintEnabled() return FALSE on APs? That would >>> probably require a new BaseLib interface to determine if the caller is >>> running on the BSP or an AP. >> I agree that "AmIAnAP()" would be a pre-requisite. >>> (For IA32/X64 this isn't too hard -- it just needs to check a bit in >>> the local APIC. >> Still not trivial, as some DebugLib instances might want to target >> runtime drivers (or even SMM drivers). For runtime drivers the >> complication is that a runtime (virtual address) mapping for the LAPIC >> MMIO range would be needed (if I understand correctly anyway). And for >> both runtime and SMM drivers, it could be a problem that on physical >> hardware, the MMIO range of the LAPIC can be moved (reprogrammed) to a >> different base address, possibly by the OS too. >> I could be quite confused about this, of course; I don't eat LAPICs for >> breakfast :) I just recall an SMM firmware vulnerability that was in >> part based on moving the LAPIC base address elsewhere. Hm... googling >> suggests the attack was called "The Memory Sinkhole". >=20 > I finally looked at the documentation (imagine!), and the BSP flag is bi= t 8 in the IA32_APIC_BASE (0x1B) MSR. So no local APIC access is necessary= , only reading an MSR. This MSR has been present since the Pentium Pro (fa= mily/model 06_01H), and is considered "architectural." >=20 Brian, But this is still only valid for x86 CPUs, so it is not generic to all = supported CPUs. I guess we could add a library for "MP" debugging, or a cou= ple of libraries as you may want a simple library that gives you info like = are you running on the BSP, and maybe a more complex lib that has AP safe f= unctions. For the check the BSP function you could have a MpServices versio= n, x86 MSR version, and a NULL version.=20 Given I did this once to debug some diagnostics code I'd not be opposed to= having the simple lib and actually adding an ASSERT to every EFI Service i= n the DXE Core if it is called from the AP. People can turn it off via the = NULL instance of the lib.=20 I don't like the idea of using the MSR vs. the MpServices protocol, but I = have to admit I've used that trick in JTAG debugger scripts to auto find th= e BSP.=20 The MpServices API and even the BSP don't apply at runtime, but then again= you can't run code on the APs at runtime. I'm not sure that SMM/MM really = follows the same rules.=20 Thanks, Andrew Fish >>> I have no idea about other architectures.) That wouldn't solve the >>> problem everywhere -- anyone using a custom DebugLib would have to >>> update it themselves. But it would solve it solidly in the majority >>> of cases. >>>=20 >>> Thoughts? >> My fear might not be reasonable, but I feel quite uncomfortable about >> LAPIC accesses in DebugLib APIs. The information ("BSP or AP") is safer >> to determine at the call site, I think, even if it takes more human >> work. >=20 > I understand the concern. DebugLib is exposed in a lot of awkward end c= ases and odd environments. Caution is warranted. But checking a bit in a = MSR is about as simple a test as you can do. It sounds reasonable to me. A= ctually reading the local APIC would, as you said, be a lot more problemati= c. >=20 > The observation remains that the problem is a lot bigger than DebugLib. = We can't add AmIAnAP() calls on every single library interface. Are there = any other spots people know of which tend to cause a lot of grief for APs? = Maybe the memory allocation boot services? At least if there's a simple t= est for AP-ness which doesn't have additional dependencies, it's easy to ad= d checks as needed. >=20 > I don't know if this discussion will go anywhere... but it has been an i= nteresting one to have. >=20 >> I could very well be biased. In OvmfPkg we have a minuscule amount of >> code that runs on APs, and even that code is written with total >> minimalism in mind. >> Leaping to a different topic... Years ago I was tracking down an MTRR >> setup bug in the Xen hypervisor (as it was shipped as a part of RHEL5). >> It is necessary to setup MTRRs identically on all CPUs, plus it has to >> be done while all CPUs are in a "pen" doing nothing but setting up >> MTRRs. The bug was exactly in that part of the code (running >> simultaneously on more than a hundred CPUs). It was impossible to print >> anything to the serial console -- first because it would be unreadable >> for humans, and second because the delays would perturb the buggy >> behavior. In the end I had to introduce an array of per-CPU debug >> structures where each CPU would record its own view (snapshot) of a >> shared resource -- a shared resource that should have been protected by >> mutual exclusion between the CPUs. After the CPUs left the "pen" (with >> the invalid MTRR configuration established), I'd use the BSP to dump th= e >> array. That showed me that some CPUs had overlapping / inconsistent >> views of the shared resource between each other. This proved that the >> mutual exclusion primitive didn't work as expected -- it turns out that >> the semaphore (or spinlock, not sure) in question used an INT8 counter, >> which overflowed when more than 127 CPUs contended for the resource. >=20 > I too have done battle with MTRR initialization, with over a thousand pr= ocessors. I feel your pain. >=20 >> I'm sure I'm misremembering parts of this story (from several years >> distance), the moral is that debugging in a multiprocessing environment >> may easily require its own dedicated infrastructure. In edk2, we don't >> have anything like that, I think. Could we build it, sufficiently >> generally? Like, prepare a log buffer for each CPU before calling >> StartupAllAps(), log only to that buffer during the concurrent >> execution, and finally dump the buffers? I guess if we don't *reach* th= e >> "finally" part, we could still dump the RAM and investigate the log >> buffers that way... Dumping the RAM is certainly an option for virtual >> machines, but it might be viable for physical setups too (JTAG, debug >> agent... dunno). >> Sorry about the wild speculation :) >> Thanks >> Laszlo >=20 >=20 > --=20 >=20 > Brian >=20 > -------------------------------------------------------------------- >=20 > "Are we going to push the edge of the envelope, Brain?" > "No, Pinky, but we may get to the sticky part." > -- Quoted on the Net >=20 >=20 --Apple-Mail=_B83BDE7E-BA6A-4634-9A07-67906EC7268B Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
On Aug 1,= 2019, at 1:20 PM, Brian J. Johnson <brian.johnson@hpe.com> wrote:

On 7/31/19 1:58 PM, Laszlo E= rsek wrote:
On 07/31/19 18:34, Brian J. Johnson wrote:
I do wonder if there would be a clean way to let = a DebugLib instance
itself declare that AP_DEBUG() is safe. &= nbsp;That way a platform would
only need to override the Debu= gLib instance in the DSC file, rather
than both the instance = and the PCD.  (I know, I'm nitpicking.)  A
library = can't override PCDs in its calling modules, of course.  I
suppose the AP_DEBUG() macro could call a new DebugLib entry point to
test for AP safety before doing anything else, say
DebugPrintOnApIsSafe().  Or it could even be a global CONST BOOLEANdefined by the library.  But that would require all Debug= Lib instances
to change, which is something you were trying t= o avoid.
That's right -- I tried to imagine some= other approaches, but they'd
need new DebugLib functions, an= d likely force platforms to write new
code.
However, it's not always practical to tra= ck down all uses of DEBUG().
An AP can easily call a library = routine which uses DEBUG() rather than
AP_DEBUG(), buried und= er several layers of transitive library
dependencies.  I= n other words, it's not always practical to determine
ahead o= f time if a given DEBUG() call may be done on an AP.
This problem is valid IMO, but I think its scope is a lot wider thanjust DebugLib. Assume the programmer is looking at a function = that may
be invoked on an AP, and they are about to call a fu= nction for taking
care of a specific sub-task. If the program= mer cannot prove the
thread-safety of the *entire* call tree = underneath the function call
they are about to add, they simp= ly must not add the call. The
thread-safety of the DebugLib i= nstance in use is just a part of the
thread-safety of said ca= ll tree.
Put differently, code that runs APs must be extremel= y self-contained;
I'd rule out any and all lib classes from d= irect use unless a specific
library instance, advertizing thr= ead safety, would be chosen in the
platform DSC file. But, if= we adopted this approach, we could even
introduce a new AP-o= riented library *class* for debug messages, offer a
Null impl= ementation in edk2, and ask platforms to bring their own.
I know that AP code runs in a very restri= cted environment and that
people who use MpServices are suppo= sed to understand the
repercussions, but it gets very difficu= lt when libraries are
involved.  :(
Exactly -- the first restriction people should understand is, "stay = away
from libraries as much as you can".
So would a better solution be to modify the = common unsafe DebugLib
instances to have DebugPrintEnabled() = return FALSE on APs?  That would
probably require a new = BaseLib interface to determine if the caller is
running on th= e BSP or an AP.
I agree that "AmIAnAP()" would b= e a pre-requisite.
(For = IA32/X64 this isn't too hard -- it just needs to check a bit in
the local APIC.
Still not trivial, as some De= bugLib instances might want to target
runtime drivers (or eve= n SMM drivers). For runtime drivers the
complication is that = a runtime (virtual address) mapping for the LAPIC
MMIO range = would be needed (if I understand correctly anyway). And for
b= oth runtime and SMM drivers, it could be a problem that on physical
hardware, the MMIO range of the LAPIC can be moved (reprogrammed) to= a
different base address, possibly by the OS too.
I could be quite confused about this, of course; I don't eat LAPICs = for
breakfast :) I just recall an SMM firmware vulnerability = that was in
part based on moving the LAPIC base address elsew= here. Hm... googling
suggests the attack was called "The Memo= ry Sinkhole".

I finally looked at the documentation (imagine!), and the BSP= flag is bit 8 in the IA32_APIC_BASE (0x1B) MSR.  So no local APIC acc= ess is necessary, only reading an MSR.  This MSR has been present sinc= e the Pentium Pro (family/model 06_01H), and is considered "architectural."=


Brian,

But this  is  still  only valid = for x86 CPUs, so it is not generic to all supported CPUs. I guess we could = add a library for "MP" debugging, or a couple of libraries as you may want = a simple library that gives you info like are you running on the BSP, and m= aybe a more complex lib that has AP safe functions. For the check the BSP f= unction you could have a MpServices version, x86 MSR version, and a NULL ve= rsion. 

Given I did this once to d= ebug some diagnostics code I'd not be opposed to having the simple lib and = actually adding an ASSERT to every EFI Service in the DXE Core if it is cal= led from the AP. People can turn it off via the NULL instance of the lib.&n= bsp;

I don't like the idea of using the= MSR vs. the MpServices protocol, but I have to admit I've used that trick = in JTAG debugger scripts to auto find the BSP. 

The MpServices API and even the BSP don't apply at runtime, b= ut then again you can't run code on the APs at runtime. I'm not sure that S= MM/MM really follows the same rules. 

<= div>Thanks,

Andrew Fish

I have no idea about other arch= itectures.)  That wouldn't solve the
problem everywhere = -- anyone using a custom DebugLib would have to
update it the= mselves.  But it would solve it solidly in the majority
= of cases.

Thoughts?
My fear might not be reasonable, but I feel quite uncomfortable about
LAPIC accesses in DebugLib APIs. The information ("BSP or AP") is= safer
to determine at the call site, I think, even if it tak= es more human
work.

I understand the concern.  DebugLib is= exposed in a lot of awkward end cases and odd environments.  Caution = is warranted.  But checking a bit in a MSR is about as simple a test a= s you can do. It sounds reasonable to me.  Actually reading the local = APIC would, as you said, be a lot more problematic.

The obs= ervation remains that the problem is a lot bigger than DebugLib. We can't a= dd AmIAnAP() calls on every single library interface.  Are there any o= ther spots people know of which tend to cause a lot of grief for APs?  = ;Maybe the memory allocation boot services?  At least if there's a sim= ple test for AP-ness which doesn't have additional dependencies, it's easy = to add checks as needed.

I don't know if this discussion wi= ll go anywhere... but it has been an interesting one to have.

I could very well be biased. In OvmfPkg we have a minuscule= amount of
code that runs on APs, and even that code is writt= en with total
minimalism in mind.
Leaping to a = different topic... Years ago I was tracking down an MTRR
setu= p bug in the Xen hypervisor (as it was shipped as a part of RHEL5).
It is necessary to setup MTRRs identically on all CPUs, plus it has = to
be done while all CPUs are in a "pen" doing nothing but se= tting up
MTRRs. The bug was exactly in that part of the code = (running
simultaneously on more than a hundred CPUs). It was = impossible to print
anything to the serial console -- first b= ecause it would be unreadable
for humans, and second because = the delays would perturb the buggy
behavior. In the end I had= to introduce an array of per-CPU debug
structures where each= CPU would record its own view (snapshot) of a
shared resourc= e -- a shared resource that should have been protected by
mut= ual exclusion between the CPUs. After the CPUs left the "pen" (with
the invalid MTRR configuration established), I'd use the BSP to dump= the
array. That showed me that some CPUs had overlapping / i= nconsistent
views of the shared resource between each other. = This proved that the
mutual exclusion primitive didn't work a= s expected -- it turns out that
the semaphore (or spinlock, n= ot sure) in question used an INT8 counter,
which overflowed w= hen more than 127 CPUs contended for the resource.

I too have done battle wit= h MTRR initialization, with over a thousand processors.  I feel your p= ain.

I'm sure I'm misremembering parts of this= story (from several years
distance), the moral is that debug= ging in a multiprocessing environment
may easily require its = own dedicated infrastructure. In edk2, we don't
have anything= like that, I think. Could we build it, sufficiently
generall= y? Like, prepare a log buffer for each CPU before calling
Sta= rtupAllAps(), log only to that buffer during the concurrent
e= xecution, and finally dump the buffers? I guess if we don't *reach* the
"finally" part,  we could still dump the RAM and investigat= e the log
buffers that way... Dumping the RAM is certainly an= option for virtual
machines, but it might be viable for phys= ical setups too (JTAG, debug
agent... dunno).
S= orry about the wild speculation :)
Thanks
Laszl= o


-- 

       = ;            &n= bsp;            = ;            &n= bsp;  Brian

-------------------------------------= -------------------------------

 "Are we going to push= the edge of the envelope, Brain?"
 "No, Pinky, but we may get to the sticky part."<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">     =             &nb= sp;            =             -- = Quoted on the Net


--Apple-Mail=_B83BDE7E-BA6A-4634-9A07-67906EC7268B--