[Linaro-uefi,v4,15/56] Hisilicon/ACPI: Update SRAT from real memory configuration

we add updating ACPI table feature to update SRAT and SLIT
table for D05 platform.
There have many static memory nodes at SRAT table,and they
will be initalized during platform booting,how many memory nodes
be used due to the DIMM numbers,so it is dymanic,we have to remove
the uninitlizated nodes.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 .../Drivers/HisiAcpiPlatformDxe/AcpiPlatform.c     |  49 +++++---
 .../HisiAcpiPlatformDxe/AcpiPlatformDxe.inf        |   6 +
 .../Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c  | 136 +++++++++++++++++++++
 .../Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h  |  16 +++
 4 files changed, 187 insertions(+), 20 deletions(-)
 create mode 100644 Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
 create mode 100644 Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h

On Sat, Nov 19, 2016 at 04:37:30PM +0800, Heyi Guo wrote:
> we add updating ACPI table feature to update SRAT and SLIT
> table for D05 platform.
> There have many static memory nodes at SRAT table,and they
> will be initalized during platform booting,how many memory nodes
> be used due to the DIMM numbers,so it is dymanic,we have to remove
> the uninitlizated nodes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  .../Drivers/HisiAcpiPlatformDxe/AcpiPlatform.c     |  49 +++++---
>  .../HisiAcpiPlatformDxe/AcpiPlatformDxe.inf        |   6 +
>  .../Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c  | 136 +++++++++++++++++++++
>  .../Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h  |  16 +++
>  4 files changed, 187 insertions(+), 20 deletions(-)
>  create mode 100644 Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
>  create mode 100644 Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h
> 
> diff --git a/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatform.c b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatform.c
> index 5b679fa..c8b56e1 100644
> --- a/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatform.c
> +++ b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatform.c
> @@ -23,6 +23,7 @@
>  #include <Library/PcdLib.h>
>  
>  #include <IndustryStandard/Acpi.h>
> +#include "UpdateAcpiTable.h"
>  
>  /**
>    Locate the first instance of a protocol.  If the protocol requested is an
> @@ -180,6 +181,8 @@ AcpiPlatformEntryPoint (
>    UINT32                         FvStatus;
>    UINTN                          TableSize;
>    UINTN                          Size;
> +  EFI_STATUS                     TableStatus;
> +  EFI_ACPI_DESCRIPTION_HEADER    *TableHeader;
>  
>    Instance     = 0;
>    CurrentTable = NULL;
> @@ -218,26 +221,32 @@ AcpiPlatformEntryPoint (
>        //
>        // Add the table
>        //
> -      TableHandle = 0;
> -
> -      TableSize = ((EFI_ACPI_DESCRIPTION_HEADER *) CurrentTable)->Length;
> -      ASSERT (Size >= TableSize);
> -
> -      //
> -      // Checksum ACPI table
> -      //
> -      AcpiPlatformChecksum ((UINT8*)CurrentTable, TableSize);
> -
> -      //
> -      // Install ACPI table
> -      //
> -      Status = AcpiTable->InstallAcpiTable (
> -                            AcpiTable,
> -                            CurrentTable,
> -                            TableSize,
> -                            &TableHandle
> -                            );
> -
> +      TableHeader = (EFI_ACPI_DESCRIPTION_HEADER*) (CurrentTable);
> +      //Update specfic Acpi Table
> +      //If the Table is updated failed, doesn't install it,
> +      //go to find next section.
> +      TableStatus = UpdateAcpiTable(TableHeader);
> +      if (TableStatus == EFI_SUCCESS) {
> +        TableHandle = 0;
> +
> +        TableSize = ((EFI_ACPI_DESCRIPTION_HEADER *) CurrentTable)->Length;
> +        ASSERT (Size >= TableSize);
> +
> +        //
> +        // Checksum ACPI table
> +        //
> +        AcpiPlatformChecksum ((UINT8*)CurrentTable, TableSize);
> +
> +        //
> +        // Install ACPI table
> +        //
> +        Status = AcpiTable->InstallAcpiTable (
> +                              AcpiTable,
> +                              CurrentTable,
> +                              TableSize,
> +                              &TableHandle
> +                              );
> +      }
>        //
>        // Free memory allocated by ReadSection
>        //
> diff --git a/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatformDxe.inf b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatformDxe.inf
> index 80ff49b..56514e5 100644
> --- a/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatformDxe.inf
> +++ b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatformDxe.inf
> @@ -29,10 +29,12 @@
>  
>  [Sources]
>    AcpiPlatform.c
> +  UpdateAcpiTable.c
>  
>  [Packages]
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
> +  OpenPlatformPkg/Chips/Hisilicon/HisiPkg.dec
>  
>  [LibraryClasses]
>    UefiLib
> @@ -40,12 +42,16 @@
>    PcdLib
>    BaseMemoryLib
>    DebugLib
> +  HobLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>  
>  [Protocols]
>    gEfiAcpiTableProtocolGuid                     ## CONSUMES
>  
> +[Guids]
> +  gHisiEfiMemoryMapGuid
> +
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile    ## CONSUMES
>  
> diff --git a/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
> new file mode 100644
> index 0000000..37c71ad
> --- /dev/null
> +++ b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
> @@ -0,0 +1,136 @@
> +/** @file
> +  Copyright (c) 2016, Hisilicon Limited. All rights reserved.
> +  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 <PlatformArch.h>
> +#include <IndustryStandard/Acpi.h>
> +#include <Library/AcpiNextLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/HwMemInitLib.h>
> +#include <Library/OemMiscLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +#define CORE_NUM_PER_SOCKET  32
> +#define NODE_IN_SOCKET       2
> +
> +STATIC
> +VOID
> +RemoveUnusedMemoryNode (
> +  IN OUT EFI_ACPI_STATIC_RESOURCE_AFFINITY_TABLE  *Table,
> +  IN     UINTN                        MemoryNodeNum
> +)
> +{
> +  UINTN                   *CurrPtr, *NewPtr;

OK, I was not clear enough on what I wanted here:

The code in previous version was using UINT8 * in order to avoid
having to deal with the complexities of pointer arithmetic (also known
as the dumbest feature of the C programming language).

What I meant was:
<<<
  UINTN                   CurrAddr, NewAddr;

> +
> +  if (MemoryNodeNum >= EFI_ACPI_MEMORY_AFFINITY_STRUCTURE_COUNT) {
> +    return;
> +  }
> +
> +  CurrPtr = (UINTN *) &(Table->Memory[EFI_ACPI_MEMORY_AFFINITY_STRUCTURE_COUNT]);
> +  NewPtr = (UINTN *) &(Table->Memory[MemoryNodeNum]);

  CurrAddr = (UINTN) &(Table->Memory[EFI_ACPI_MEMORY_AFFINITY_STRUCTURE_COUNT]);
  NewAddr = (UINTN) &(Table->Memory[MemoryNodeNum]);

> +
> +  CopyMem (NewPtr, CurrPtr, (UINTN *)Table + Table->Header.Header.Length - CurrPtr);

  CopyMem (NewPtr, CurrPtr, (VOID *)(Table + Table->Header.Header.Length - CurrPtr));
>>>

The use of (UINTN *) here leads to param 3 expanding to
  Table + (Header.Header.Length * sizeof (UINTN))
... meaning 8 times as much data as intended is copied.

> +
> +  Table->Header.Header.Length -= CurrPtr - NewPtr;
> +
> +  return;
> +}
> +
> +STATIC
> +EFI_STATUS
> +UpdateSrat (
> +  IN OUT EFI_ACPI_STATIC_RESOURCE_AFFINITY_TABLE *Table
> +  )
> +{
> +  UINT8               Skt = 0;
> +  UINTN               Index = 0;
> +  VOID                *HobList;
> +  GBL_DATA            *Gbl_Data;
> +  UINTN               Base;
> +  UINTN               Size;
> +  UINT8               NodeId;
> +  UINT32              ScclInterleaveEn;
> +  UINTN               MemoryNode = 0;
> +
> +  DEBUG((DEBUG_INFO, "SRAT: Updating SRAT memory information.\n"));
> +
> +  HobList = GetHobList();
> +  if (HobList == NULL) {
> +    return EFI_UNSUPPORTED;
> +  }
> +  Gbl_Data = (GBL_DATA*)GetNextGuidHob(&gHisiEfiMemoryMapGuid, HobList);
> +  if (Gbl_Data == NULL) {
> +    DEBUG((DEBUG_ERROR, "Get next Guid HOb fail.\n"));
> +    return EFI_NOT_FOUND;
> +  }
> +  Gbl_Data = GET_GUID_HOB_DATA(Gbl_Data);
> +  for(Skt = 0; Skt < MAX_SOCKET; Skt++) {
> +    for(Index = 0; Index < MAX_NUM_PER_TYPE; Index++) {
> +      NodeId = Gbl_Data->NumaInfo[Skt][Index].NodeId;
> +      Base = Gbl_Data->NumaInfo[Skt][Index].Base;
> +      Size = Gbl_Data->NumaInfo[Skt][Index].Length;
> +      DEBUG((DEBUG_INFO, "Skt %d Index %d: NodeId = %d, Base = 0x%lx, Size = 0x%lx\n", Skt, Index, NodeId, Base, Size));
> +      if (Size > 0) {
> +        Table->Memory[MemoryNode].ProximityDomain = NodeId;
> +        Table->Memory[MemoryNode].AddressBaseLow = Base;
> +        Table->Memory[MemoryNode].AddressBaseHigh = Base >> 32;
> +        Table->Memory[MemoryNode].LengthLow = Size;
> +        Table->Memory[MemoryNode].LengthHigh = Size >> 32;
> +        MemoryNode = MemoryNode + 1;
> +      }
> +    }
> +    ScclInterleaveEn = Gbl_Data->NumaInfo[Skt][0].ScclInterleaveEn;
> +    DEBUG((DEBUG_INFO, "ScclInterleaveEn = %d\n", ScclInterleaveEn));
> +    //update gicc structure
> +    if (ScclInterleaveEn != 0) {
> +      DEBUG((DEBUG_INFO, "SRAT: Updating SRAT Gicc information.\n"));
> +      for (Index = Skt * CORE_NUM_PER_SOCKET; Index < (Skt + 1) * CORE_NUM_PER_SOCKET; Index++) {

My comment from last round still applies:
---
Could benefit from a local macro such as
#define CORECOUNT(X) ((X) * CORE_NUM_PER_SOCKET)
---

Which would shorten the for statement to:

      for (Index = CORECOUNT (Skt); Index < CORECOUNT (Skt + 1); Index++) {

No further comments.

Regards,

Leif

> +        Table->Gicc[Index].ProximityDomain = Skt * NODE_IN_SOCKET;
> +      }
> +    }
> +  }
> +
> +  //remove invalid memory node
> +  RemoveUnusedMemoryNode (Table, MemoryNode);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +UpdateSlit (
> +  IN OUT EFI_ACPI_DESCRIPTION_HEADER  *Table
> +  )
> +{
> +  return  EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +UpdateAcpiTable (
> +  IN OUT EFI_ACPI_DESCRIPTION_HEADER      *TableHeader
> +)
> +{
> +  EFI_STATUS Status = EFI_SUCCESS;
> +
> +  switch (TableHeader->Signature) {
> +
> +  case EFI_ACPI_6_0_SYSTEM_RESOURCE_AFFINITY_TABLE_SIGNATURE:
> +    Status = UpdateSrat ((EFI_ACPI_STATIC_RESOURCE_AFFINITY_TABLE *) TableHeader);
> +    break;
> +
> +  case EFI_ACPI_6_0_SYSTEM_LOCALITY_INFORMATION_TABLE_SIGNATURE:
> +    Status = UpdateSlit (TableHeader);
> +    break;
> +  }
> +  return Status;
> +}
> diff --git a/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h
> new file mode 100644
> index 0000000..45b3729
> --- /dev/null
> +++ b/Chips/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.h
> @@ -0,0 +1,16 @@
> +/** @file
> +  Copyright (c) 2016, Hisilicon Limited. All rights reserved.
> +  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.
> +**/
> +
> +EFI_STATUS
> +UpdateAcpiTable (
> +  IN OUT EFI_ACPI_DESCRIPTION_HEADER      *TableHeader
> +);
> +
> -- 
> 1.9.1
>