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

[Jordan Justen <jljusten@xxxxxxxxx>]

On Tue, Nov 26, 2013 at 11:26 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> MemDetect actully does too many things, the underlying platform might
> want to have more control over memory layout.
>
> Extract the functionality of publishing PEI memory to a dedicated
> function.
>
> Also fixed wrong comment while I was there.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c |   36 +++++++++++++++++++++++++++++++++++-
>  OvmfPkg/PlatformPei/Platform.h  |    5 +++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 9f6ca19..dc44745 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -83,11 +83,45 @@ GetSystemMemorySizeAbove4gb (
>    return LShiftU64 (Size, 16);
>  }
>
> +/**
> +  Publish PEI core memory
> +
> +  @return EFI_SUCCESS     The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +PublishPeiMemory(

Space before open-paren.

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

> +  MemoryBase = PcdGet32 (PcdOvmfMemFvBase) + PcdGet32 (PcdOvmfMemFvSize);
> +  MemorySize = LowerMemorySize - MemoryBase;
> +  if (MemorySize > SIZE_64MB) {
> +    MemoryBase = LowerMemorySize - SIZE_64MB;
> +    MemorySize = SIZE_64MB;
> +  }
> +
> +  //
> +  // Publish this memory to the PEI Core
> +  //
> +  Status = PublishSystemMemory(MemoryBase, MemorySize);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return Status;
> +}
> +
>
>  /**
>    Peform Memory Detection
>
> -  @return EFI_SUCCESS     The PEIM initialized successfully.
> +  @return Top of memory
>
>  **/
>  EFI_PHYSICAL_ADDRESS
> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index d63d124..01af2a9 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -57,6 +57,11 @@ AddUntestedMemoryRangeHob (
>    EFI_PHYSICAL_ADDRESS        MemoryLimit
>    );
>
> +EFI_STATUS
> +PublishPeiMemory(

Space before open-paren.

-Jordan

> +  VOID
> +  );
> +
>  EFI_PHYSICAL_ADDRESS
>  MemDetect (
>    VOID
> --
> 1.7.10.4
>
>
> ------------------------------------------------------------------------------
> Rapidly troubleshoot problems before they affect your business. Most IT
> organizations don't have a clear picture of how application performance
> affects their revenue. With AppDynamics, you get 100% visibility into your
> Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
> http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

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