Re: [edk2-devel] [PATCH V7 19/37] OvmfPkg/PlatformInitLib: Add memory functions

["Gerd Hoffmann" <kraxel@redhat.com>]

  Hi,

> > Hmm.  Unlike patches 17+18 which are pure code motion (except the
> > function renaming but that doesn't change the workflow) this patch mixes
> > code changes and code moving which makes it hard to review.
> > 
> > It should be splitted into one (or more) patches changing the functions as
> > needed (and keeping the code in PlatformPei), and one patch moving things
> > over to PlatformInitLib without functional changes.
> Ok.  Looks like #21 & #22 in tdvf_wave2.v6?
> https://github.com/mxu9/edk2/commit/ef0615ca5665b2058e4352a322dfa74d258f9f31
> https://github.com/mxu9/edk2/commit/25f356a0bf7ee347be30e270aeffe6cbd8e0b464

No.  The idea is to changes to the code in PlatformPei with small &
one-patch-per-update patches, which allow easy review.  Also helps
debugging in case something go wrong, when bisecting found the broken
patch it's *much* easier to find the actual bug when the patch is small.

Rough plan:

  (1) a patch allocating PLATFORM_INFO  struct in PlatformPei.
  (2) one or more patches moving global variables into PLATFORM_INFO
      struct.
  (3) one or more patches restructing functions.  Stuff like like
      splitting functions which set PCDs into two, one for
      PlatformInitLib and one for PlatformPei.

Final step is a pure move from PlatformPei to PlatformInitLib without
changing code.

> > > +  // Fetch the lower memory size (Below 4G)  //  mLowerMemorySize =
> > > + PlatformGetSystemMemorySizeBelow4gb ();
> This is in function InitializePlatform().
> > 
> > Can't you just use TopOfLowRam here?
> TopOfLowRam is a local variable in function MemMapInitialization(). It cannot be used in function InitialziePlatform().

Ah, didn't notice it is another function.  High time to introduce
PLATFORM_INFO->TopOfLowRam ;)

take care,
  Gerd