Wei Liu
2013-Nov-29 13:16 UTC
Re: [edk2] [PATCH v3 6/8] OvmfPkg: introduce PublishPeiMemory
On Thu, Nov 28, 2013 at 08:31:11PM -0800, Jordan Justen wrote: [...]> > + VOID > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_PHYSICAL_ADDRESS MemoryBase; > > + UINT64 MemorySize; > > + UINT64 LowerMemorySize; > > + > > + LowerMemorySize = GetSystemMemorySizeBelow4gb (); > > I''m going to say > Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > for patches 5 & 6, but I am not too happy with them. > > For 5, I think maybe it would be nice to not require the details of > ''XenLeaf'' to leak out of Xen.c. I think XenDetect should return > BOOLEAN, and store XenLeaf in a static global in Xen.c. > > For 6, I think it is inconsistent that Xen continues to use CMOS here, > but moves to the E820 tables otherwise. (GetSystemMemorySizeBelow4gb > could have a different path for Xen, or maybe PublishPeiMemory could > have an input parameter of MemoryLimit32Bit and get called by > MemDetect too.) > > But, these don''t seem critical, so to reduce thrash for you I''ll let > you move forward with these patches as-is. >Thanks for reviewing. Wei.