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. Mike > -----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
