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

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

On Mon, Feb 28, 2022 at 03:20:51PM +0800, Min Xu wrote:
> Below functions are introduced in PlatformInitLib:
>  - PlatformGetFirstNonAddress
>  - PlatformAddressWidthInitialization
>  - PlatformGetSystemMemorySizeBelow4gb
>  - PlatformQemuUc32BaseInitialization
>  - PlatformInitializeRamRegions
> 
> They correspond to the below functions in OvmfPkg/PlatformPei:
>  - GetFirstNonAddress
>  - AddressWidthInitialization
>  - GetSystemMemorySizeBelow4gb
>  - QemuUc32BaseInitialization
>  - InitializeRamRegions
> 
> After that OvmfPkg/PlatformPei is refactored with this library.
> 
> Note: PlatformInitLib will not determine whether SMM or S3 is supported
> or not. Instead the caller of these functions should input SMM / S3
> support as the IN parameter by themselves. This is to reduce the
> complexity of PlatformInitLib. Another reason is that some PCDs cannot
> be declared as FixedAtBuild while PlatformInitLib is designed to be used
> in both SEC and PEI phase.

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.

> +PlatformGetFirstNonAddress (
> +  OUT UINT64  *Pci64Base,
> +  OUT UINT64  *Pci64Size,
> +  IN  UINT64  DefaultPciMmio64Size
> +  )

> +VOID
> +QemuInitializeRam (
> +  UINT32         Uc32Base,
> +  UINT16         HostBridgeDevId,
> +  EFI_BOOT_MODE  BootMode,
> +  BOOLEAN        SmmSmramRequire,
> +  UINT32         LowerMemorySize,
> +  UINT16         Q35TsegMbytes
> +  )

> +VOID
> +EFIAPI
> +PlatformInitializeRamRegions (
> +  IN UINT32         Uc32Base,
> +  IN UINT16         HostBridgeDevId,
> +  IN BOOLEAN        SmmSmramRequire,
> +  IN EFI_BOOT_MODE  BootMode,
> +  IN BOOLEAN        S3Supported,
> +  IN UINT32         LowerMemorySize,
> +  IN UINT16         Q35TsegMbytes
> +  )

I think we should add all those parameters to the PLATFORM_INFO struct,
then simply pass around a pointer to that struct instead of having tons
of parameters for each function.

Due to this patch needing an update anyway it might be easier to do it
right away instead of an incremental cleanup after merging this series.

> @@ -85,7 +87,7 @@ MemMapInitialization (
>      return;
>    }
>  
> -  TopOfLowRam  = GetSystemMemorySizeBelow4gb ();
> +  TopOfLowRam  = PlatformGetSystemMemorySizeBelow4gb ();
>    PciExBarBase = 0;
>    if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>      //
> @@ -736,6 +738,11 @@ InitializePlatform (
>      Q35SmramAtDefaultSmbaseInitialization ();
>    }
>  
> +  //
> +  // Fetch the lower memory size (Below 4G)
> +  //
> +  mLowerMemorySize = PlatformGetSystemMemorySizeBelow4gb ();

Can't you just use TopOfLowRam here?

take care,
  Gerd