public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: "Eric Dong" <eric.dong@intel.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Rahul Kumar" <rahul1.kumar@intel.com>,
	"Ray Ni" <ray.ni@intel.com>, "Star Zeng" <star.zeng@intel.com>
Subject: [PATCH v2 1/4] UefiCpuPkg/CpuFeature: Don't assume CpuS3DataDxe alloc RegisterTable
Date: Tue, 19 Jan 2021 16:54:37 +0100	[thread overview]
Message-ID: <20210119155440.2262-2-lersek@redhat.com> (raw)
In-Reply-To: <20210119155440.2262-1-lersek@redhat.com>

From: Ray Ni <ray.ni@intel.com>

There are lots of fields in ACPI_CPU_DATA structure while only
followings are accessed by CpuFeature infra:
* NumberOfCpus
* PreSmmInitRegisterTable // pointer
* RegisterTable  // pointer
* CpuStatus
* ApLocation  // pointer

So it's possible that an implementation of CpuS3DataDxe doesn't
allocate memory for PreSmmInitRegisterTable/RegisterTable/ApLocation.

This patch handles the case when CpuS3DataDxe doesn't allocate
memory for PreSmmInitRegisterTable/RegisterTable.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Signed-off-by: Ray Ni <ray.ni@intel.com>
[lersek@redhat.com: update CC list, add BZ reference, add my S-o-b]
[lersek@redhat.com: deal with RegisterTable and PreSmmInitRegisterTable
 being zero independently of each other; replacing the ASSERT()]
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    
    - new patch, cherry-picked from
      <https://github.com/niruiyu/edk2/commit/7091aa50b9d8>
    
    - Update as follows: do not assume that (RegisterTable == 0) implies
      (PreSmmInitRegisterTable == 0). The ACPI_CPU_DATA documentation update
      in the next patch permits each field to be zero in separation, and
      updating Ray's patch for accommodating that is not difficult. Some
      normal RAM may be wasted if exactly one of the "RegisterTable" and
      "PreSmmInitRegisterTable" fields is zero (the memory is allocated with
      the PEI or DXE instance of MemoryAllocationLib).
    
    - this patch is easiest to review with "git show -b -W"

 UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 80 +++++++++++---------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 4063d45760ad..7bb92404027f 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -1,7 +1,7 @@
 /** @file
   CPU Register Table Library functions.
 
-  Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -937,45 +937,51 @@ GetAcpiCpuData (
   EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
 
   AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
-  if (AcpiCpuData != NULL) {
-    return AcpiCpuData;
-  }
-
-  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
-  ASSERT (AcpiCpuData != NULL);
-
-  //
-  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
-  //
-  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
-  ASSERT_EFI_ERROR (Status);
-
-  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
-  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
-
-  //
-  // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
-  //
-  TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-  RegisterTable  = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
-  ASSERT (RegisterTable != NULL);
-
-  for (Index = 0; Index < NumberOfCpus; Index++) {
-    Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+  if (AcpiCpuData == NULL) {
+    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+    ASSERT (AcpiCpuData != NULL);
+    ZeroMem (AcpiCpuData, sizeof (ACPI_CPU_DATA));
+
+    //
+    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
+    //
+    Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
     ASSERT_EFI_ERROR (Status);
 
-    RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
-    RegisterTable[Index].TableLength        = 0;
-    RegisterTable[Index].AllocatedSize      = 0;
-    RegisterTable[Index].RegisterTableEntry = 0;
-
-    RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
-    RegisterTable[NumberOfCpus + Index].TableLength        = 0;
-    RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
-    RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
+    GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+  }
+
+  if (AcpiCpuData->RegisterTable == 0 ||
+      AcpiCpuData->PreSmmInitRegisterTable == 0) {
+    //
+    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
+    //
+    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
+    RegisterTable  = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
+    ASSERT (RegisterTable != NULL);
+
+    for (Index = 0; Index < NumberOfCpus; Index++) {
+      Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+      ASSERT_EFI_ERROR (Status);
+
+      RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+      RegisterTable[Index].TableLength        = 0;
+      RegisterTable[Index].AllocatedSize      = 0;
+      RegisterTable[Index].RegisterTableEntry = 0;
+
+      RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+      RegisterTable[NumberOfCpus + Index].TableLength        = 0;
+      RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
+      RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
+    }
+    if (AcpiCpuData->RegisterTable == 0) {
+      AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
+    }
+    if (AcpiCpuData->PreSmmInitRegisterTable == 0) {
+      AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
+    }
   }
-  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
-  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
 
   return AcpiCpuData;
 }
-- 
2.19.1.3.g30247aa5d201



  reply	other threads:[~2021-01-19 15:54 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-19 15:54 [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
2021-01-19 15:54 ` Laszlo Ersek [this message]
2021-01-19 15:54 ` [PATCH v2 2/4] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
2021-01-19 15:54 ` [PATCH v2 3/4] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables Laszlo Ersek
2021-01-19 15:54 ` [PATCH v2 4/4] OvmfPkg/CpuS3DataDxe: " Laszlo Ersek
2021-01-19 16:44 ` [edk2-devel] [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
2021-01-20  9:28 ` Zeng, Star
2021-01-20 18:46   ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210119155440.2262-2-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox