Re: [Xen-devel] [edk2] [PATCH v3 6/8] OvmfPkg: introduce PublishPeiMemory

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxx>
> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel