On 07/07/16 20:02, Kinney, Michael D wrote: > Laszlo, > > Good feedback. As I mentioned earlier, we are cleaning up these modules. We > can make sure everything is consistent for the "borrow" case. > > I agree it would be better to "borrow" a page that is not being used by > any other PEIM. We will consider ASSERT() if there is no free memory.
Thank you for considering it! Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:[email protected]] >> Sent: Thursday, July 7, 2016 10:49 AM >> To: Kinney, Michael D <[email protected]>; Fan, Jeff >> <[email protected]>; >> Justen, Jordan L <[email protected]> >> Cc: Paolo Bonzini <[email protected]>; edk2-devel-01 >> <[email protected]> >> Subject: Re: [edk2] multiprocessing in PEI? >> >> 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:[email protected]] >>>> Sent: Thursday, July 7, 2016 2:15 AM >>>> To: Fan, Jeff <[email protected]>; Kinney, Michael D >>>> <[email protected]>; >>>> Justen, Jordan L <[email protected]> >>>> Cc: Paolo Bonzini <[email protected]>; edk2-devel-01 >>>> <[email protected]> >>>> 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:[email protected]] >>>>> 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:[email protected]] >>>>>> 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:[email protected]] >>>>>>> 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
