Re: [Patch][edk2-platforms] Vlv2TbltDevicePkg: Add SMBIOS Type 19.

[Marvin H?user <Marvin.Haeuser@outlook.com>]

One comment regarding the record allocation is inline.

Regards,
Marvin.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> lushifex
> Sent: Tuesday, July 4, 2017 10:24 AM
> To: edk2-devel@lists.01.org
> Cc: david.wei@intel.com
> Subject: [edk2] [Patch][edk2-platforms] Vlv2TbltDevicePkg: Add SMBIOS
> Type 19.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: lushifex <shifeix.a.lu@intel.com>
> ---
>  .../SmBiosMiscDxe/MiscMemoryArrayMappedAddress.uni | Bin 0 -> 1318
> bytes
>  .../MiscMemoryArrayMappedAddressData.c             |  29 ++++
>  .../MiscMemoryArrayMappedAddressFunction.c         | 146
> +++++++++++++++++++++
>  .../SmBiosMiscDxe/MiscSubclassDriverDataTable.c    |   4 +-
>  Vlv2TbltDevicePkg/SmBiosMiscDxe/SmBiosMiscDxe.inf  |   5 +-
>  5 files changed, 182 insertions(+), 2 deletions(-)  create mode 100644
> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddress.uni
>  create mode 100644
> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressData.
> c
>  create mode 100644
> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFunct
> ion.c
> 
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddress.un
> i
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddress.un
> i
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..27d298a2d858de029b581553f
> 069002872d0e8ea
> GIT binary patch
> literal 1318
> zcmZvcU2oG+42FG;#D6#{H-NSlb~AwxqNN2Q*iyBb2AAo_x=8()k}}wzhdr;;G-
> *Lm
> zj(u#O*YB~N{Qc9hjs@PAJi$KLjb*m9xjnTfSmhhqtvy(QeQGJbU>EpTwy_+0ur
> 4yk
> zBmDw-$1}H?duGU-UBT1FGA5#Dk;Q1iwwzIHu-
> Enmf0eV6!9J4Zj;NgM3wUgaeGZQo
> z$TNGzxihrW{qEdO&?8$DId`r?$idK>V$IOj&ZUcAx2drU--
> +!U>pkm?$agRv+57*n
> zOJI}{9l+1QU3iWa`;;82z?KNzFNO1zh!v6YJ#cPKe83`B%9o)nL91_{V%6y-zA4(;
> znF+VT*Xh|V!#%osm)e9?=YBd1Vb@H`Tq|c?p@^CIX8Zc+P8(*SKG&mTefHl
> DxYS&K
> zc`c@_|K2u(jSy9hvR=wnu-C08@ND))RTcXi>}7|<`8P4goDS<!)>2bA*`43)Dj-JN
> zMqM>WTRQR9CtMR&)>iSkSGvvZbNbUcs(pcF0`nO&cbL5m$}Mj_#i5Zgu`f1&
> *^o(!
> z?K5kaFF}(!X6?I7utv<<l_kWg)>n3k9AFRFN`$%(5nsoEh_NHbX!;hlwvfUVdJEa}
> zy>=%JPS^N_8hgfI*F=q+*(LS}RAI!PeZ)@OWmC?CSj8x78TE-
> +W&eTvibu@#Ee>{B
> z%3BQV`yQu8kEq3v$x_aKqk{Ex>F?BW?y~89M*TIv3!i^Ys(N_JU4+{LNj~+cpf4
> >y
> P6Yw-2;WYQhdX?)D9Tmic
> 
> literal 0
> HcmV?d00001
> 
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressDat
> a.c
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressDa
> ta.c
> new file mode 100644
> index 0000000..f71b548
> --- /dev/null
> +++
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressDa
> ta.c
> @@ -0,0 +1,29 @@
> +/** @file
> +  Static data of Physical Memory Array Mapped Address. SMBIOS Type 19.
> +
> +  Copyright (c) 2012 - 2017, Intel Corporation. All rights
> + reserved.<BR>
> +
> +  This program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD License  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "CommonHeader.h"
> +#include "MiscSubclassDriver.h"
> +
> +//
> +// Static (possibly build generated) Physical Memory Array Mapped Address
> Data.
> +//
> +MISC_SMBIOS_TABLE_DATA(EFI_MEMORY_ARRAY_START_ADDRESS_DAT
> A,
> +MiscMemoryArrayMappedAddress) = {
> +    0,                                                      //< StartingAddress
> +    0,                                                      //< EndingAddress
> +    0,                                                      //< MemoryArrayHandle
> +    0x00                                                    //< Partition Width
> +};
> +
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFu
> nction.c
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFu
> nction.c
> new file mode 100644
> index 0000000..27d0ab7
> --- /dev/null
> +++
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscMemoryArrayMappedAddressFu
> ncti
> +++ on.c
> @@ -0,0 +1,146 @@
> +/** @file
> +  Dynamic data of Physical Memory Array Mapped Address. SMBIOS Type
> 19.
> +
> +  Copyright (c) 2012 - 2017, Intel Corporation. All rights
> + reserved.<BR>
> +
> +  This program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD License  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "CommonHeader.h"
> +#include "MiscSubclassDriver.h"
> +#include <Protocol/DataHub.h>
> +#include <Guid/DataHubRecords.h>
> +#include <Protocol/MemInfo.h>
> +
> +#define MAX_SOCKETS  2
> +#define SMBIOS_TYPE19_USE_EXTENDED_ADDRESSES     0xFFFFFFFF
> +
> +
> +VOID
> +GetType16Handle (
> +  IN  EFI_SMBIOS_PROTOCOL      *Smbios,
> +  OUT  EFI_SMBIOS_HANDLE       *Handle
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  EFI_SMBIOS_TYPE            RecordType;
> +  EFI_SMBIOS_TABLE_HEADER    *Buffer;
> +
> +  *Handle = 0;
> +  RecordType = EFI_SMBIOS_TYPE_PHYSICAL_MEMORY_ARRAY;
> +
> +  Status = Smbios->GetNext (
> +                     Smbios,
> +                     Handle,
> +                     &RecordType,
> +                     &Buffer,
> +                     NULL
> +                     );
> +  if (!EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  *Handle = 0xFFFF;
> +}
> +
> +
> +/**
> +  This function installs SMBIOS Type 19 Table(Physical Memory Array).
> +
> +  @param  RecordData                 Pointer to copy of RecordData from the Data
> Table.
> +
> +  @retval EFI_SUCCESS                All parameters were valid.
> +  @retval EFI_UNSUPPORTED            Unexpected RecordType value.
> +  @retval EFI_INVALID_PARAMETER      Invalid parameter was found.
> +
> +**/
> +MISC_SMBIOS_TABLE_FUNCTION (MiscMemoryArrayMappedAddress) {
> +  EFI_STATUS                               Status;
> +  UINT64                                   TotalMemorySizeInKB;
> +  UINT8                                    Dimm;
> +  EFI_SMBIOS_HANDLE                        SmbiosHandle;
> +  EFI_MEMORY_ARRAY_START_ADDRESS_DATA      *ForType19InputData;
> +  SMBIOS_TABLE_TYPE19                      *SmbiosRecord;
> +  MEM_INFO_PROTOCOL                        *MemInfoHob;
> +  UINT16                                   Type16Handle = 0;
> +
> +  TotalMemorySizeInKB = 0;
> +
> +  //
> +  // First check for invalid parameters.
> +  //
> +  if (RecordData == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  ForType19InputData        = (EFI_MEMORY_ARRAY_START_ADDRESS_DATA
> *)RecordData;
> +
> +  //
> +  // Two zeros following the last string.
> +  //
> +  SmbiosRecord = AllocatePool (sizeof (SMBIOS_TABLE_TYPE19) + 1);
> + ZeroMem (SmbiosRecord, sizeof (SMBIOS_TABLE_TYPE19) + 1);

Shouldn't 2 be added because, as the comment says, two zeros terminate the record?
Also, I would use AllocateZeroPool(), which looks cleaner in my opinion.

> +
> +  SmbiosRecord->Hdr.Type =
> EFI_SMBIOS_TYPE_MEMORY_ARRAY_MAPPED_ADDRESS;
> +  SmbiosRecord->Hdr.Length = sizeof (SMBIOS_TABLE_TYPE19);
> +
> +  //
> +  // Make handle chosen by smbios protocol.add automatically.
> +  //
> +  SmbiosRecord->Hdr.Handle = 0;
> +
> +  //
> +  // Get Memory size parameters for each rank from the chipset
> + registers  //  Status = gBS->LocateProtocol (
> +                  &gMemInfoProtocolGuid,
> +                  NULL,
> +                  (void **) &MemInfoHob
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
> +  // Calculate the TotalMemorySizeInKB by adding the size of all
> + populated sockets  //  for (Dimm = 0; Dimm < MAX_SOCKETS; Dimm++) {
> +    TotalMemorySizeInKB += LShiftU64
> + (MemInfoHob->MemInfoData.dimmSize[Dimm], 10);  }
> +
> +  if (TotalMemorySizeInKB > SMBIOS_TYPE19_USE_EXTENDED_ADDRESSES)
> {
> +    SmbiosRecord->StartingAddress =
> SMBIOS_TYPE19_USE_EXTENDED_ADDRESSES;
> +    SmbiosRecord->EndingAddress =
> SMBIOS_TYPE19_USE_EXTENDED_ADDRESSES;
> +    SmbiosRecord->ExtendedEndingAddress = TotalMemorySizeInKB - 1;  }
> + else {
> +    SmbiosRecord->EndingAddress = (UINT32) (TotalMemorySizeInKB - 1);
> + }
> +
> +  //
> +  // Memory Array Handle will be the 3rd optional string following the
> formatted structure.
> +  //
> +  GetType16Handle(Smbios, &Type16Handle);
> + SmbiosRecord->MemoryArrayHandle = Type16Handle;
> + SmbiosRecord->PartitionWidth = MAX_SOCKETS;
> +
> +  //
> +  // Now we have got the full smbios record, call smbios protocol to add this
> record.
> +  // Generate Memory Array Mapped Address info (Type 19)  //
> + SmbiosHandle = SMBIOS_HANDLE_PI_RESERVED;  Status = Smbios-> Add (
> +                      Smbios,
> +                      NULL,
> +                      &SmbiosHandle,
> +                      (EFI_SMBIOS_TABLE_HEADER *) SmbiosRecord
> +                      );
> +  FreePool (SmbiosRecord);
> +
> +  return Status;
> +}
> +
> diff --git
> a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscSubclassDriverDataTable.c
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscSubclassDriverDataTable.c
> index 3b77a75..36c31c0 100644
> --- a/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscSubclassDriverDataTable.c
> +++ b/Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscSubclassDriverDataTable.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -Copyright (c) 2004  - 2014, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2004  - 2017, Intel Corporation. All rights reserved.<BR>
> 
> 
>    This program and the accompanying materials are licensed and made
> available under
> 
>    the terms and conditions of the BSD License that accompanies this
> distribution.
> 
> @@ -39,6 +39,7 @@
> MISC_SMBIOS_TABLE_EXTERNS(EFI_MISC_CHASSIS_MANUFACTURER_DAT
> A, MiscChassisManufac
> MISC_SMBIOS_TABLE_EXTERNS(EFI_CACHE_VARIABLE_RECORD,
> MiscProcessorCache, MiscProcessorCache); //type 7
> MISC_SMBIOS_TABLE_EXTERNS(EFI_CPU_DATA_RECORD,
> MiscProcessorInformation, MiscProcessorInformation); //type 4
> MISC_SMBIOS_TABLE_EXTERNS(EFI_MEMORY_ARRAY_LOCATION_DATA,
> MiscPhysicalMemoryArray,MiscPhysicalMemoryArray);
> +MISC_SMBIOS_TABLE_EXTERNS(EFI_MEMORY_ARRAY_START_ADDRESS_D
> ATA,
> +MiscMemoryArrayMappedAddress, MiscMemoryArrayMappedAddress);
> //type 19
>  MISC_SMBIOS_TABLE_EXTERNS(EFI_MEMORY_ARRAY_LINK_DATA,
> MiscMemoryDevice, MiscMemoryDevice);
> 
> 
> MISC_SMBIOS_TABLE_EXTERNS(EFI_MISC_PORT_INTERNAL_CONNECTOR_
> DESIGNATOR_DATA, MiscPortIde1, MiscPortInternalConnectorDesignator);
> @@ -72,6 +73,7 @@ EFI_MISC_SMBIOS_DATA_TABLE
> mMiscSubclassDataTable[] = {
> 
> MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscProcessorInform
> ation, MiscProcessorInformation),  //type 4
> 
> MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscPhysicalMemory
> Array, MiscPhysicalMemoryArray),    //Type 16
> 
> MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscMemoryDevice,
> MiscMemoryDevice), //Type 17
> +
> +
> MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscMemoryArrayM
> appedAddress
> + , MiscMemoryArrayMappedAddress), //Type 19
>    MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscPortIde1,
> MiscPortInternalConnectorDesignator),
>    MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscPortIde2,
> MiscPortInternalConnectorDesignator),
>    MISC_SMBIOS_TABLE_ENTRY_DATA_AND_FUNCTION(MiscPortAtxPower,
> MiscPortInternalConnectorDesignator),
> diff --git a/Vlv2TbltDevicePkg/SmBiosMiscDxe/SmBiosMiscDxe.inf
> b/Vlv2TbltDevicePkg/SmBiosMiscDxe/SmBiosMiscDxe.inf
> index f88dcbe..b17e5b7 100644
> --- a/Vlv2TbltDevicePkg/SmBiosMiscDxe/SmBiosMiscDxe.inf
> +++ b/Vlv2TbltDevicePkg/SmBiosMiscDxe/SmBiosMiscDxe.inf
> @@ -2,7 +2,7 @@
>  # Component name for module MiscSubclass  #  # FIX ME!
> -# Copyright (c) 2006  - 2016, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006  - 2017, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # This program and the accompanying materials are licensed and made
> available under  # the terms and conditions of the BSD License that
> accompanies this distribution.
> @@ -88,6 +88,9 @@
>    MiscMemoryDevice.uni
>    MiscMemoryDeviceData.c
>    MiscMemoryDeviceFunction.c
> +  MiscMemoryArrayMappedAddress.uni
> +  MiscMemoryArrayMappedAddressData.c
> +  MiscMemoryArrayMappedAddressFunction.c
> 
>  [Packages]
>    MdeModulePkg/MdeModulePkg.dec
> --
> 2.7.0.windows.1
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel