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
