On 07/07/16 19:33, Kinney, Michael D wrote: > Laszlo, > > If all memory below 640K is allocated for use by other PEIMs, the "borrow" > technique can still work > for the CpuMpPeim. > > If we add the scanning code for 2B, we will just ignore the result anyways if > all memory below > 640K is already allocated.
I think I disagree. In that case CpuMpPei should ASSERT() and then call CpuDeadLoop() explicitly. Minimally, some kind of arbitration is necessary between CpuMpPei and another PEIM that performs the same kind of borrowing. If both of these PEIMs go ahead and just ignore the results anyways, that could be very bad. Either way, the fact that CpuMpPei produces a memalloc HOB at the end, for the buffer that it borrows, is just inconsistent with the fact that CpuMpPei does not consider memalloc HOBs that other PEIMs have produced earlier. > So we will add logic with no value. I think there are two different > cases here. One for permanent allocation of memory by a PEIM, and another is > "borrow" for a > short period of time with no chance for confict. If it's only for a short period of time, with no chance for conflict, then what justifies the BuildMemoryAllocationHob() call when GetWakeupBuffer() is about to return with success? Actually, I think there's a pretty large chance for conflict here. The borrowed area is retained by CpuMpPei until End-of-PEI. At End-of-PEI, the area is either simply released (normal boot path) or restored (S3 resume path). The CpuMpEndOfPeiCallback() function is called by the PEI Core way after CpuMpPeimInit() returns -- an arbitrary number of other PEIMs can be dispatched in the meantime, at least one of which could perform a similar scan. So, I actually agree with the BuildMemoryAllocationHob() call in GetWakeupBuffer(). I disagree with "no chance for conflict", and for that reason I think GetWakeupBuffer() should consider preexistent memalloc HOBs too. > You raise good points to make sure we provide > great examples for other PEIMs to follow. In this case, I would recommend a > detailed comment > block that describes the "borrow" technique and why the memory allocation > HOBs are not scanned > in this specific case. I'd be really pleased with that, but only if it proves my above argument wrong :) Thanks! Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Thursday, July 7, 2016 2:15 AM >> To: Fan, Jeff <jeff....@intel.com>; Kinney, Michael D >> <michael.d.kin...@intel.com>; >> Justen, Jordan L <jordan.l.jus...@intel.com> >> Cc: Paolo Bonzini <pbonz...@redhat.com>; edk2-devel-01 >> <edk2-de...@ml01.01.org> >> Subject: Re: [edk2] multiprocessing in PEI? >> >> On 07/07/16 10:56, Fan, Jeff wrote: >>> Laszlo, >>> >>> Any PEI module cannot allocate the memory < 1MB range if using PEI memory >>> allocation >> services only. >> >> Okay. >> >>> If other PEI module wants to allocate the memory by scanning memory >>> resource HOB, it >> has to follow the same rule with the CpuMpPei driver. >> >> Okay. >> >>> If platform build allocation HOB for the whole range < 1MB, how to avoid >>> memory >> conflicts between CpuMpPei and other PEI modules who is scanning memory >> resource HOB? >> >> You are right about this, in theory. >> >> Your suggestion is that any PEIM that needs memory under 1MB look for >> memory HOBs, and stay away from memory allocation HOBs. In other words, >> to build a map from memory HOBs, subtract the memory allocation HOBs, >> grab a chunk from what remains, and then cover that chunk with another >> allocation HOB. This will ensure complete understanding between the >> platfom PEIM, CpuMpPei, and any other PEIMs. >> >> I agree completely, except for the fact that in practice, CpuMpPei >> currently *ignores* any memory allocation HOBs! >> >> CpuMpPei creates a memalloc HOB for the chunk that it ultimately >> borrows, yes -- but how can CpuMpPei expect other PEIMs to honor its >> memalloc HOB when CpuMpPei ignores memalloc HOBs from other PEIMs? >> >> In other words, I can agree that OVMF's Platform PEIM should not create >> a memalloc HOB (only the memory HOB) for 0..640KB, but *only if* >> CpuMpPei is patched to honor preexistent memalloc HOBs in GetWakeupBuffer(). >> >> Then we can say we have a pattern that all PEIMs are expected to follow: >> >> (1) memory can be allocated for any use with the PEI services >> >> (2) memory can be allocated under 1MB by >> (2a) looking for memory HOBs >> (2b) *and* honoring existing memalloc HOBs. >> >> (2c) The memory taken this way should be covered by the consumer >> with another memalloc HOB. >> (2d) Also, if allocation occurs on the S3 resume path, then the >> original contents must be restored at End-of-PEI. >> >> This method is safe for all PEIMs to follow, but CpuMpPei itself does >> not comply with it (step (2b)). >> >> Thanks >> Laszlo >> >> >>> >>> Jeff >>> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>> Sent: Thursday, July 07, 2016 4:48 PM >>> To: Fan, Jeff; Kinney, Michael D; Justen, Jordan L >>> Cc: Paolo Bonzini; edk2-devel-01 >>> Subject: Re: [edk2] multiprocessing in PEI? >>> >>> On 07/07/16 10:29, Fan, Jeff wrote: >>>> Laszlo, >>>> >>>> Actually, CpuMpPei has no knowledge to know which memory range < 1MB is >>>> available or >> not. That's why we cannot assume one hardcode address < 1MB for borrowing. >>> >>> I agree that this makes sense. >>> >>>> >>>> Platform has such knowledge and need one mechanism to tell CpuMpPei. >>> >>> I agree 100%. >>> >>>> We choose Memory Resource HOB here. >>> >>> I believe this is not a perfect choice. >>> >>> The information that the memory resource HOB conveys can be unwittingly >>> mis-used by >> other PEIMs. The information from the platform is good, but it is not >> targeted / >> restricted well enough. >>> >>> As I wrote in my other email, the restriction should be >>> >>> available for temporary use, but only if the original contents are >>> restored at End-of-PEI. >>> >>> CpuMpPei certainly complies with this restriction, but other PEIMs won't >>> realize it >> just from looking at a memory resource HOB. >>> >>> So, as a workaround, I have to install a memory allocation HOB as well, in >>> OVMF's >> Platform PEIM. CpuMpPei will ignore the memalloc HOB (and take care to >> restore the >> borrowed area at End-of-PEI). And any other PEIMs should steer clear of the >> area >> because of the memalloc HOB. >>> >>> Thanks >>> Laszlo >>> >>>> >>>> Thanks! >>>> Jeff >>>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>>> Sent: Thursday, July 07, 2016 3:04 PM >>>> To: Fan, Jeff; Kinney, Michael D; Justen, Jordan L >>>> Cc: Paolo Bonzini; edk2-devel-01 >>>> Subject: Re: [edk2] multiprocessing in PEI? >>>> >>>> On 07/07/16 04:52, Fan, Jeff wrote: >>>>> Laszlo, >>>>> >>>>> On PEI phase, whatever it is normal boot or S3 boot, PEI memory >>>>> allocation >> services should not allocate memory < 1 MB address. >>>>> >>>>> If any other PEIM module wants to allocate memory < 1 MB address, it has >>>>> to scan >> Memory resource HOB to find the available memory range and build memory >> allocation HOB >> like what CpuMpPei did. >>>>> >>>>> If this method is OK, CpuMpPei or other module are required to check >>>>> existing both >> memory resource HOB and memory allocation HOB. >>>>> >>>>> If we just borrow one piece of range < 1 MB address around each of >>>>> INIT-SIPI-SIPI >> to AP, we still need to get the available memory address. >>>>> >>>>> So, platform still needs to build memory resource HOB < 1MB on S3 boot >>>>> like normal >> boot. I think this update on platform should be simpler than new pair of >> PCDs >> introducing. >>>>> >>>>> What do you think of it? >>>> >>>> * First, at this point I'm now thinking that "borrowing" and "allocating" >>>> are >> actually separate things: >>>> >>>> (a) For allocating memory under 1MB, scanning both the memory resource >>>> HOBs and the memory allocation HOBs would be necessary, yes. >>>> >>>> (b) However, in case the memory under 1MB is only borrowed (and the >>>> backup buffer is allocated with the PEI serivces), the HOB scanning >>>> should not be necessary at all (for any kinds of HOBs). CpuMpPei >>>> is intimately tied to Intel real mode anyway (hence the < 1MB >>>> requirement in the first place), so why can't CpuMpPei just borrow >>>> a few pages at a fixed low address (under 640KB)? That memory is >>>> required to exist on platforms where CpuMpPei is supposed to run, >>>> isn't it? >>>> >>>> * Second, the reason I'm reluctant to produce the same set of memory >>>> resource and >> alloc HOBs on the S3 resume path as on the normal boot path is the >> following: >>>> >>>> In my opinion, that would be misleading. Other PEIMs that execute (a) -- >>>> that is, >> allocation and not borrowing -- might think that any memory that is covered >> by a >> resource descriptor HOB, but not covered by any allocation HOBs, is "free >> memory". >> While in fact that memory could well be used by the OS. >>>> >>>> In other words, the reason why we don't produce any of these HOBs is >>>> exactly to >> communicate that there is zero free memory *for allocation*, outside of the >> PEI memory >> services. Any PEIM that wants to allocate (as opposed to borrow) *should* >> fail, unless >> it uses the PEI memory services. And if a PEIM wants to *borrow* (as opposed >> to >> allocate), then it shouldn't need to scan HOBs. >>>> >>>> * If there is a requirement in the PI spec that the HOBs be produced >>>> on the S3 path as well, then I guess we'll have to comply with that. >>>> With my current understanding though, I feel that producing the same >>>> set of HOBs would give other PEIMs an incorrect view of memory >>>> ownership. The result of >>>> >>>> all system memory HOBs minus all allocation HOBs >>>> >>>> would appear as >>>> >>>> memory that is free for further allocation >>>> >>>> which would be false. >>>> >>>> * So my proposal would be to allow the platform to advertize a range for >>>> CpuMpPei to >> borrow from, with two new PCDs. >>>> >>>> If you are strongly opposed to that, then I guess I can write a patch for >>>> OvmfPkg >> that, on the S3 resume path, produces one (1) system memory HOB as an >> *exception*, >> specifically to allow CpuMpPei's scanning to succeed (with an appropriate >> code comment >> of course). I could also cover the entire area in question with a memory >> allocation HOB >> as well, hoping that it would keep away other PEIMs that look for "free >> memory", like >> in (a). Something like: >>>> >>>> // >>>> // Allow CpuMpPei to borrow memory from this range for the AP startup >>>> // code, but prevent other PEIMs from thinking the range is "free >>>> // memory". >>>> // >>>> AddMemoryRangeHob (0, BASE_512KB + BASE_128KB); >>>> BuildMemoryAllocationHob (0, BASE_512KB + BASE_128KB, >>>> EfiBootServicesData); >>>> >>>> Actually, let me try that, and see where I land. >>>> >>>> Thanks, >>>> Laszlo >>>> >>>> >>>>> -----Original Message----- >>>>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>>>> Sent: Thursday, July 07, 2016 2:52 AM >>>>> To: Fan, Jeff; Kinney, Michael D; Justen, Jordan L >>>>> Cc: Paolo Bonzini; edk2-devel-01 >>>>> Subject: Re: [edk2] multiprocessing in PEI? >>>>> >>>>> On 07/06/16 19:08, Laszlo Ersek wrote: >>>>>> Hi Jeff, >>>>>> >>>>>> On 07/05/16 15:50, Fan, Jeff wrote: >>>>>>> When CPU MP PPI installed, CPU MP PPI Services will be fully usable. >>>>>> >>>>>> I included CpuMpPei in OVMF, and it works fine on the normal boot path. >>>>>> (I haven't done anything special with it thus far, just built it >>>>>> into the firmware, and it starts up fine.) >>>>>> >>>>>> However, on the S3 resume path, it runs into an assertion failure: >>>>>> the >>>>>> GetWakeupBuffer() function fails. That is actually expected, because >>>>>> at that point the runtime OS being resumed owns all memory under 1MB. >>>>>> >>>>>> On the S3 resume path, OVMF does install a tiny bit of permanent PEI >>>>>> RAM: 32 KB in size (PcdS3AcpiReservedMemorySize), starting at 8288 >>>>>> KB (PcdS3AcpiReservedMemoryBase). On the normal boot path, we >>>>>> reserve this area as AcpiNVS type memory, so that the runtime OS stays >>>>>> out of it. >>>>>> >>>>>> Is CpuMpPei meant to be used on the S3 resume path in general? >>>>> >>>>> It looks like CpuMpPei expects memory resource descriptor HOBs to exist >>>>> on the S3 >> resume path as well. Then it seems to lay the wakeup buffer over OS-owned >> memory under >> 1MB. But, it saves the original memory contents into a backup buffer >> allocated from >> permanent PEI RAM. And, at end-of-PEI, the memory contents are restored. >>>>> >>>>> OVMF does not produce memory / IO / MMIO resource descriptors on the S3 >>>>> resume >> path. The assumption is that no PEIM will directly look for any such HOBs on >> the S3 >> path, only use the permament PEI RAM for memory allocation. Does this >> conflict with the >> PI spec? >>>>> >>>>> I wonder if GetWakeupBuffer()'s scanning of EFI_RESOURCE_SYSTEM_MEMORY >>>>> descriptor >> HOBs is a good idea. When it finds a location that is suitable as a wakeup >> buffer, it >> "claims" that area by building a memory allocation HOB that covers the area. >> I think >> that's fine; however, the scanning itself does not consider any other memory >> allocation >> HOBs that may have been carved out of system memory by other PEIMs, earlier. >>>>> >>>>> I wonder if platforms would be better served by a (base, size) pair of >>>>> PCDs. The >> platform could set these PCDs, and make sure that both the OS and other >> firmware >> modules (PEIMs and DXE drivers) stay out of this area. (For example, if S3 >> is not >> supported or enabled on the platform, then the platform could allocate this >> area as >> Boot Services Data. >>>>> Otherwise, it would be AcpiNVS.) CpuMpPei could then remove the >>>>> resource HOB scanning, and it would only need to validate if (a) the >>>>> size PCD was big enough to contain the wakeup buffer, and (b) the >>>>> base PCD was low enough to keep the wakeup buffer under 1MB, (c) any >>>>> necessary alignments. Basically, ask the platform to preallocate the >>>>> wakeup buffer. (The minimal suitable size can be found statically, by >>>>> trial and >>>>> error.) The backup buffer (in permanent PEI RAM) could also be dropped, >>>>> decreasing >> footprint. >>>>> >>>>> What do you think? >>>>> >>>>> Thanks >>>>> Laszlo >>>>> >>>> >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
