Re: [PATCH] SimicsOpenBoardPkg: Replace CMOS Hardcoded Addresses

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

Please include [edk2-platforms] in the subject in the future.

Reviewed-by: Michael Kubacki <michael.a.kubacki@intel.com>

> -----Original Message-----
> From: Agyeman, Prince <prince.agyeman@intel.com>
> Sent: Friday, December 6, 2019 9:32 AM
> To: devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> Michael A <michael.a.kubacki@intel.com>
> Subject: [PATCH] SimicsOpenBoardPkg: Replace CMOS Hardcoded Addresses
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2330
> 
> Changes:
> * Added CmosMap.h that defines CMOS addresses used in
>   SimicsOpenBoardPkg as macros
> 
> * Replaced hardcoded CMOS addresses with the macros
>   defined in CmosMap.h
> 
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> 
> Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
> ---
>  .../SimicsOpenBoardPkg/Include/CmosMap.h      | 35
> +++++++++++++++++++
>  .../SimicsOpenBoardPkg/SimicsPei/MemDetect.c  | 23 ++++++++----
>  .../SmbiosPlatformDxe/SmbiosPlatformDxe.c     | 24 ++++++++-----
>  .../SmbiosPlatformDxe/SmbiosPlatformDxe.h     |  1 +
>  4 files changed, 68 insertions(+), 15 deletions(-)  create mode 100644
> Platform/Intel/SimicsOpenBoardPkg/Include/CmosMap.h
> 
> diff --git a/Platform/Intel/SimicsOpenBoardPkg/Include/CmosMap.h
> b/Platform/Intel/SimicsOpenBoardPkg/Include/CmosMap.h
> new file mode 100644
> index 0000000000..3221ce9a5b
> --- /dev/null
> +++ b/Platform/Intel/SimicsOpenBoardPkg/Include/CmosMap.h
> @@ -0,0 +1,35 @@
> +/** @file
> +Cmos address definition macros header file.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef _CMOS_MAP_H_
> +#define _CMOS_MAP_H_
> +
> +//
> +// CMOS 0x34/0x35 specifies the system memory above 16 MB.
> +// * CMOS(0x35) is the high byte
> +// * CMOS(0x34) is the low byte
> +// * The size is specified in 64kb chunks // * Since this is memory
> +above 16MB, the 16MB must be added
> +//   into the calculation to get the total memory size.
> +//
> +#define CMOS_SYSTEM_MEM_ABOVE_16MB_LOW_BYTE         0x34
> +#define CMOS_SYSTEM_MEM_ABOVE_16MB_HIGH_BYTE        0x35
> +
> +//
> +// CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
> +// * CMOS(0x5d) is the most significant size byte // * CMOS(0x5c) is
> +the middle size byte // * CMOS(0x5b) is the least significant size byte
> +// * The size is specified in 64kb chunks //
> +#define CMOS_SYSTEM_MEM_ABOVE_4GB_LOW_BYTE          0x5b
> +#define CMOS_SYSTEM_MEM_ABOVE_4GB_MIDDLE_BYTE       0x5c
> +#define CMOS_SYSTEM_MEM_ABOVE_4GB_HIGH_BYTE         0x5d
> +
> +
> +#endif // _CMOS_MAP_H_
> diff --git a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> index e547de0045..60aa54be9e 100644
> --- a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> +++ b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> @@ -26,6 +26,8 @@
>  #include <SimicsPlatforms.h>
>  #include <Guid/SmramMemoryReserve.h>
> 
> +#include <CmosMap.h>
> +
>  #include "Platform.h"
> 
>  UINT8 mPhysMemAddressWidth;
> @@ -74,24 +76,33 @@ X58TsegMbytesInitialization(
>    return;
>  }
> 
> +/**
> +  Get the system memory size below 4GB
> 
> +  @return The size of system memory below 4GB **/
>  UINT32
>  GetSystemMemorySizeBelow4gb (
>    VOID
>    )
>  {
> +  UINT32 Size;
>    //
>    // CMOS 0x34/0x35 specifies the system memory above 16 MB.
> -  // * CMOS(0x35) is the high byte
> -  // * CMOS(0x34) is the low byte
>    // * The size is specified in 64kb chunks
>    // * Since this is memory above 16MB, the 16MB must be added
>    //   into the calculation to get the total memory size.
>    //
> -  return (UINT32) (((UINTN)CmosRead16 (0x34) << 16) + SIZE_16MB);
> +  Size = (UINT32) ((CmosRead16
> (CMOS_SYSTEM_MEM_ABOVE_16MB_LOW_BYTE) << 16)
> +           + SIZE_16MB);
> +  return Size;
>  }
> 
> +/**
> +  Get the system memory size  above 4GB
> 
> +  @return The size of system memory above 4GB **/
>  STATIC
>  UINT64
>  GetSystemMemorySizeAbove4gb (
> @@ -100,12 +111,10 @@ GetSystemMemorySizeAbove4gb (
>    UINT32 Size;
>    //
>    // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
> -  // * CMOS(0x5d) is the most significant size byte
> -  // * CMOS(0x5c) is the middle size byte
> -  // * CMOS(0x5b) is the least significant size byte
>    // * The size is specified in 64kb chunks
>    //
> -  Size = (CmosRead16 (0x5c) << 8) + CmosRead8 (0x5b);
> +  Size = (CmosRead16 (CMOS_SYSTEM_MEM_ABOVE_4GB_MIDDLE_BYTE)
> << 8)
> +           + CmosRead8 (CMOS_SYSTEM_MEM_ABOVE_4GB_LOW_BYTE);
> 
>    return LShiftU64 (Size, 16);
>  }
> diff --git
> a/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> Dxe.c
> b/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> Dxe.c
> index 37c659e275..23b284d2fa 100644
> ---
> a/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> Dxe.c
> +++
> b/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> +++ Dxe.c
> @@ -9,23 +9,33 @@
> 
>  #include "SmbiosPlatformDxe.h"
> 
> +/**
> +  Get the system memory size below 4GB
> 
> +  @return The size of system memory below 4GB **/
>  UINT32
>  GetSystemMemorySizeBelow4gb(
>    VOID
>    )
>  {
> +  UINT32 Size;
>    //
>    // CMOS 0x34/0x35 specifies the system memory above 16 MB.
> -  // * CMOS(0x35) is the high byte
> -  // * CMOS(0x34) is the low byte
>    // * The size is specified in 64kb chunks
>    // * Since this is memory above 16MB, the 16MB must be added
>    //   into the calculation to get the total memory size.
>    //
> -  return (UINT32) (((UINTN) CmosRead16 (0x34) << 16) + SIZE_16MB);
> +  Size = (UINT32) ((CmosRead16
> (CMOS_SYSTEM_MEM_ABOVE_16MB_LOW_BYTE) << 16)
> +           + SIZE_16MB);
> +  return Size;
>  }
> 
> +/**
> +  Get the system memory size  above 4GB
> +
> +  @return The size of system memory above 4GB **/
>  STATIC
>  UINT64
>  GetSystemMemorySizeAbove4gb(
> @@ -35,14 +45,12 @@ GetSystemMemorySizeAbove4gb(
>    UINT32 Size;
>    //
>    // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
> -  // * CMOS(0x5d) is the most significant size byte
> -  // * CMOS(0x5c) is the middle size byte
> -  // * CMOS(0x5b) is the least significant size byte
>    // * The size is specified in 64kb chunks
>    //
> -  Size = (CmosRead16 (0x5c) << 8) + CmosRead8 (0x5b);
> +  Size = (CmosRead16 (CMOS_SYSTEM_MEM_ABOVE_4GB_MIDDLE_BYTE)
> << 8)
> +           + CmosRead8 (CMOS_SYSTEM_MEM_ABOVE_4GB_LOW_BYTE);
> 
> -  return LShiftU64(Size, 16);
> +  return LShiftU64 (Size, 16);
>  }
> 
>  /**
> diff --git
> a/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> Dxe.h
> b/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> Dxe.h
> index 0dc174421c..ccd35e2924 100644
> ---
> a/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> Dxe.h
> +++
> b/Platform/Intel/SimicsOpenBoardPkg/SmbiosPlatformDxe/SmbiosPlatform
> +++ Dxe.h
> @@ -21,6 +21,7 @@
>  #include <Library/MemoryAllocationLib.h>  #include <Library/IoLib.h>
> #include <Library/CmosAccessLib.h>
> +#include <CmosMap.h>
> 
>  /**
>    Validates the SMBIOS entry point structure
> --
> 2.19.1.windows.1