[PATCH v3 0/5] Simplify SMM Relocation Process

[PATCH v3 0/5] Simplify SMM Relocation Process

[Wu, Jiaxin]

The default SMBASE for the x86 processor is 0x30000. When
SMI happens, CPU runs the SMI handler at SMBASE+0x8000.
Also, the SMM save state area is within SMBASE+0x10000.

One of the SMM initialization from CPU perspective is to relocate
and program the new SMBASE (in TSEG range) for each CPU thread. When
the SMBASE relocation happens in a PEI module, the PEI module shall
produce the SMM_BASE_HOB in HOB database which tells the
PiSmmCpuDxeSmm driver (runs at a later phase) about the new SMBASE
for each CPU thread. PiSmmCpuDxeSmm driver installs the SMI handler
at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Index. When
the HOB doesn't exist, PiSmmCpuDxeSmm driver shall relocate and
program the new SMBASE itself.

Those patches add the SMM Base HOB, which can be produced
by any PEI module to do the SmBase relocation ahead of
PiSmmCpuDxeSmm driver and store the relocated SmBase
address in array for reach Processors. PiSmmCpuDxeSmm
and SmmCpuFeaturesLib will consume the HOB to simplify
SMM relocation process.

With SMM Base Hob, PiSmmCpuDxeSmm does not need the RSM
instruction to reload the SMBASE register with the new allocated
SMBASE each time when it exits SMM. SMBASE Register for each
processors have already been programmed and all SMBASE address
have recorded in SMM Base Hob. So the same default SMBASE Address
(0x30000) will not be used, thus the CPUs over-writing each
other's SMM Save State Area will not happen in PiSmmCpuDxeSmm
driver. This way makes the first SMI init can be executed in
parallel and save boot time on multi-core system.

Jiaxin Wu (5):
  UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
  UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call
  UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
  UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration
  OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  37 ++++-
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 UefiCpuPkg/Include/Guid/SmmBaseHob.h               |  46 ++++++
 .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |   2 +
 .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     |  23 ++-
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   4 +
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |   1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |   1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |   4 +
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  29 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |  23 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         | 184 ++++++++++++++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  24 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                          |   3 +
 15 files changed, 329 insertions(+), 57 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h

-- 
2.16.2.windows.1

[PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

The default SMBASE for the x86 processor is 0x30000. When
SMI happens, CPU runs the SMI handler at SMBASE+0x8000.
Also, the SMM save state area is within SMBASE+0x10000.

One of the SMM initialization from CPU perspective is to relocate
and program the new SMBASE (in TSEG range) for each CPU thread. When
the SMBASE relocation happens in a PEI module, the PEI module shall
produce the SMM_BASE_HOB in HOB database which tells the
PiSmmCpuDxeSmm driver (runs at a later phase) about the new SMBASE
for each CPU thread. PiSmmCpuDxeSmm driver installs the SMI handler
at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Index. When
the HOB doesn't exist, PiSmmCpuDxeSmm driver shall relocate and
program the new SMBASE itself.

This patch adds the SMM Base HOB for any PEI module to do
the SmBase relocation ahead of PiSmmCpuDxeSmm driver and
store the relocated SmBase address in array for reach
Processors.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/Include/Guid/SmmBaseHob.h | 46 ++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.dec            |  3 +++
 2 files changed, 49 insertions(+)
 create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h

diff --git a/UefiCpuPkg/Include/Guid/SmmBaseHob.h b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
new file mode 100644
index 0000000000..488c0b76e9
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
@@ -0,0 +1,46 @@
+/** @file
+  The Smm Base HOB is used to store the information of:
+  * The relocated SmBase address in array for each Processors.
+
+  The default SMBASE for the x86 processor is 0x30000. When SMI happens, CPU
+  runs the SMI handler at SMBASE+0x8000. Also, the SMM save state area is within
+  SMBASE+0x10000.
+
+  One of the SMM initialization from CPU perspective is to relocate and program
+  the new SMBASE (in TSEG range) for each CPU thread. When the SMBASE relocation
+  happens in a PEI module, the PEI module shall produce the SMM_BASE_HOB in HOB
+  database which tells the PiSmmCpuDxeSmm driver (which runs at a later phase)
+  about the new SMBASE for each CPU thread. PiSmmCpuDxeSmm driver installs the
+  SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Index.
+  When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall relocate and program
+  the new SMBASE itself.
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_BASE_HOB_H_
+#define SMM_BASE_HOB_H_
+
+#define SMM_BASE_HOB_DATA_GUID \
+  { \
+    0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73}  \
+  }
+
+#pragma pack(1)
+typedef struct {
+  ///
+  /// Describes the Number of all max supported processors.
+  ///
+  UINT64    NumberOfProcessors;
+  ///
+  /// Pointer to SmBase address for each Processors.
+  ///
+  UINT64    SmBase[];
+} SMM_BASE_HOB_DATA;
+#pragma pack()
+
+extern EFI_GUID  gSmmBaseHobGuid;
+
+#endif
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index cff239d528..2afd08cdd2 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -76,10 +76,13 @@
   gEdkiiCpuFeaturesInitDoneGuid  = { 0xc77c3a41, 0x61ab, 0x4143, { 0x98, 0x3e, 0x33, 0x39, 0x28, 0x6, 0x28, 0xe5 }}
 
   ## Include/Guid/MicrocodePatchHob.h
   gEdkiiMicrocodePatchHobGuid    = { 0xd178f11d, 0x8716, 0x418e, { 0xa1, 0x31, 0x96, 0x7d, 0x2a, 0xc4, 0x28, 0x43 }}
 
+  ## Include/Guid/SmmBaseHob.h
+  gSmmBaseHobGuid      = { 0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73 }}
+
 [Protocols]
   ## Include/Protocol/SmmCpuService.h
   gEfiSmmCpuServiceProtocolGuid   = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94, 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
   gEdkiiSmmCpuRendezvousProtocolGuid = { 0xaa00d50b, 0x4911, 0x428f, { 0xb9, 0x1a, 0xa5, 0x9d, 0xdb, 0x13, 0xe2, 0x4c }}
 
-- 
2.16.2.windows.1

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

  Hi,

> +#pragma pack(1)
> +typedef struct {
> +  ///
> +  /// Describes the Number of all max supported processors.
> +  ///
> +  UINT64    NumberOfProcessors;
> +  ///
> +  /// Pointer to SmBase address for each Processors.
> +  ///
> +  UINT64    SmBase[];
> +} SMM_BASE_HOB_DATA;
> +#pragma pack()

HOBs are limited to 64k in size.  So this can by design support at most
8191 processors.  Adding such a interface to edk2 doesn't look like a
good idea to me.  It probably is not that far off that we'll hit that
limit given that high-end systems with 1024+ processors exist today.

take care,
  Gerd

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

Gerd,
In another mail with title "MdePkg: Remove Itanium leftover data structure",
we are discussing the HOB EFI_SEC_PLATFORM_INFORMATION_RECORD2 that helps
share the BIST information with CpuDxe driver.
That HOB is of format: <UINT32> ( <UINT32> <UINT32> ) +.
Very similar to the format of SMM_BASE_HOB_DATA: <UINT64> <UINT64>+

So, if we use a pattern here to remove the 8K CPU limitation, the same pattern will
be used by EFI_SEC_PLATFORM_INFORMATION_RECORD2 as well.

How about a new format as below? + Mike and Vincent for comments since the same
pattern may be used for EFI_SEC_PLATFORM_INFORMATION_RECORD3.

#pragma pack(1)
typedef struct {
  UINT32    CpuIndex;
  UINT32    NumberOfCpus;  // align to EFI_SEC_PLATFORM_INFORMATION_RECORD2.NumberOfCpus
  UINT64    SmBase[];
} SMM_BASE_HOB_DATA;
#pragma pack()

For system with less than 8K CPUs, one HOB is produced. CpuIndex is set to 0 indicating
the HOB describes the CPU from 0 to NumberOfCpus-1.

The HOB list may contains multiple such HOB instances each describing the information for
CPU from CpuIndex to CpuIndex + NumberOfCpus - 1.
The instance order in the HOB list is random so consumer cannot assume the CpuIndex
of first instance is 0.

Thanks,
Ray

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, January 18, 2023 7:19 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB
> Data
> 
>   Hi,
> 
> > +#pragma pack(1)
> > +typedef struct {
> > +  ///
> > +  /// Describes the Number of all max supported processors.
> > +  ///
> > +  UINT64    NumberOfProcessors;
> > +  ///
> > +  /// Pointer to SmBase address for each Processors.
> > +  ///
> > +  UINT64    SmBase[];
> > +} SMM_BASE_HOB_DATA;
> > +#pragma pack()
> 
> HOBs are limited to 64k in size.  So this can by design support at most
> 8191 processors.  Adding such a interface to edk2 doesn't look like a
> good idea to me.  It probably is not that far off that we'll hit that
> limit given that high-end systems with 1024+ processors exist today.
> 
> take care,
>   Gerd

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

On Wed, Jan 18, 2023 at 03:06:11PM +0000, Ni, Ray wrote:
> Gerd,
> In another mail with title "MdePkg: Remove Itanium leftover data structure",
> we are discussing the HOB EFI_SEC_PLATFORM_INFORMATION_RECORD2 that helps
> share the BIST information with CpuDxe driver.
> That HOB is of format: <UINT32> ( <UINT32> <UINT32> ) +.
> Very similar to the format of SMM_BASE_HOB_DATA: <UINT64> <UINT64>+

Same problem indeed.

> So, if we use a pattern here to remove the 8K CPU limitation, the same pattern will
> be used by EFI_SEC_PLATFORM_INFORMATION_RECORD2 as well.
> 
> How about a new format as below? + Mike and Vincent for comments since the same
> pattern may be used for EFI_SEC_PLATFORM_INFORMATION_RECORD3.
> 
> #pragma pack(1)
> typedef struct {
>   UINT32    CpuIndex;
>   UINT32    NumberOfCpus;  // align to EFI_SEC_PLATFORM_INFORMATION_RECORD2.NumberOfCpus
>   UINT64    SmBase[];
> } SMM_BASE_HOB_DATA;
> #pragma pack()
> 
> For system with less than 8K CPUs, one HOB is produced. CpuIndex is set to 0 indicating
> the HOB describes the CPU from 0 to NumberOfCpus-1.
> 
> The HOB list may contains multiple such HOB instances each describing the information for
> CPU from CpuIndex to CpuIndex + NumberOfCpus - 1.
> The instance order in the HOB list is random so consumer cannot assume the CpuIndex
> of first instance is 0.

The idea looks good to me.

For the smbase case it might also be possible to store the base
address and the per-cpu entry size, then use

	smbase = base + size * cpuindex 

to calculate the per-cpu base address.  Puts some restrictions on the
memory allocation (must be one big block), not sure this is possible,
that part of the code is not (yet?) posted for review.

take care,
  Gerd

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

> 
> For the smbase case it might also be possible to store the base
> address and the per-cpu entry size, then use
> 
> 	smbase = base + size * cpuindex
> 
> to calculate the per-cpu base address.  Puts some restrictions on the
> memory allocation (must be one big block), not sure this is possible,
> that part of the code is not (yet?) posted for review.


It's not a good idea that only store the start address of smbase and the per-cpu entry size. There are multiple algorithms to calculate each cpu's smbase, if hob producer (any peim) & consumer (SMM CPU driver) use different algorithm for each cpu's smbase, then there will be a big problem. So, we need straightforward provide the each processors smbase in the hob to keep them consistency.

> 
> take care,
>   Gerd

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

On Sun, Jan 29, 2023 at 05:08:49AM +0000, Wu, Jiaxin wrote:
> > 
> > For the smbase case it might also be possible to store the base
> > address and the per-cpu entry size, then use
> > 
> > 	smbase = base + size * cpuindex
> > 
> > to calculate the per-cpu base address.  Puts some restrictions on the
> > memory allocation (must be one big block), not sure this is possible,
> > that part of the code is not (yet?) posted for review.
> 
> 
> It's not a good idea that only store the start address of smbase and
> the per-cpu entry size. There are multiple algorithms to calculate
> each cpu's smbase, if hob producer (any peim) & consumer (SMM CPU
> driver) use different algorithm for each cpu's smbase, then there will
> be a big problem.

The algorithm needs to be part of the HOB specification of course.

take care,
  Gerd

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Laszlo Ersek]

On 1/18/23 16:06, Ni, Ray wrote:

> #pragma pack(1)
> typedef struct {
>   UINT32    CpuIndex;
>   UINT32    NumberOfCpus;  // align to EFI_SEC_PLATFORM_INFORMATION_RECORD2.NumberOfCpus
>   UINT64    SmBase[];
> } SMM_BASE_HOB_DATA;
> #pragma pack()
> 
> For system with less than 8K CPUs, one HOB is produced. CpuIndex is set to 0 indicating
> the HOB describes the CPU from 0 to NumberOfCpus-1.
> 
> The HOB list may contains multiple such HOB instances each describing the information for
> CPU from CpuIndex to CpuIndex + NumberOfCpus - 1.
> The instance order in the HOB list is random so consumer cannot assume the CpuIndex
> of first instance is 0.

When using discontiguous blocks that are limited to ~64KB each:

- The consumer won't be able to access elements of the "conceptual" big
array in a truly random (= random-access) fashion. There won't be a
single contiguous domain of valid subscripts. It's "bank switching", and
switching banks should be avoided IMO. It effectively replaces the
vector data structure with a linked list. The consequence is that the
consumer will have to either (a) build a new (temporary, or permanent)
index table of sorts, for implementing the "conceptual" big array as a
factual contiguous array, or (b) traverse the HOB list multiple times.

- If the element size of the array increases (which is otherwise
possible to do compatibly, e.g. by placing a GUID and/or revision# in
the HOB), the number of elements that fit in a single HOB decreases. I
think that's an artifact that needlessly complicates debugging, and
maybe performance too (it might increase the number of full-list
traversals).

- With relatively many elements fitting into a single HOB, on most
platforms, just one HOB is going to be used. While that may be good for
performance, it is not good for code coverage (testing). The quirky
indexing method will not be exercised by most platforms.

What's wrong with:

- restricting the creation of all such HOBs after
"gEfiPeiMemoryDiscoveredPpiGuid"

- using a page allocation, for representing the array contiguously

- in the HOB, only storing the address of the page allocation.

Page allocations performed after gEfiPeiMemoryDiscoveredPpiGuid will not
be moved, so the address in the HOB would be stable.

Laszlo

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

Thanks Gerd raise this open -- how to support more processors due to hob size limitation. 

Looks multiple hobs is the only way since we have to store each cpu's info? Sorry, allow me ask a stupid question: why DataLength in hob defined as UINT16 causing the hob size limitation? Any design background here?    

For smbase case: 
I doubt CpuIndex is really required, because we can't avoid define another hob, and we can't avoid add statement for each hob cpu ranges (0 - 8191, 8192 - 16382,...), then what's meaning for the CpuIndex, we don't expect hob producer create smaller granularity CPU ranges that one hob CpuIndex associate with previous NumberOfCpus. With above consideration, I prefer keep existing patch as is, but only add statement gSmmBaseHobGuid only support max 8191 processes (which means to fix the CPU range for each hob)?

Thanks,
Jiaxin



> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 20, 2023 4:21 PM
> To: Ni, Ray <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>
> Subject: Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base
> HOB Data
> 
> On 1/18/23 16:06, Ni, Ray wrote:
> 
> > #pragma pack(1)
> > typedef struct {
> >   UINT32    CpuIndex;
> >   UINT32    NumberOfCpus;  // align to
> EFI_SEC_PLATFORM_INFORMATION_RECORD2.NumberOfCpus
> >   UINT64    SmBase[];
> > } SMM_BASE_HOB_DATA;
> > #pragma pack()
> >
> > For system with less than 8K CPUs, one HOB is produced. CpuIndex is set to
> 0 indicating
> > the HOB describes the CPU from 0 to NumberOfCpus-1.
> >
> > The HOB list may contains multiple such HOB instances each describing the
> information for
> > CPU from CpuIndex to CpuIndex + NumberOfCpus - 1.
> > The instance order in the HOB list is random so consumer cannot assume
> the CpuIndex
> > of first instance is 0.
> 
> When using discontiguous blocks that are limited to ~64KB each:
> 
> - The consumer won't be able to access elements of the "conceptual" big
> array in a truly random (= random-access) fashion. There won't be a
> single contiguous domain of valid subscripts. It's "bank switching", and
> switching banks should be avoided IMO. It effectively replaces the
> vector data structure with a linked list. The consequence is that the
> consumer will have to either (a) build a new (temporary, or permanent)
> index table of sorts, for implementing the "conceptual" big array as a
> factual contiguous array, or (b) traverse the HOB list multiple times.
> 
> - If the element size of the array increases (which is otherwise
> possible to do compatibly, e.g. by placing a GUID and/or revision# in
> the HOB), the number of elements that fit in a single HOB decreases. I
> think that's an artifact that needlessly complicates debugging, and
> maybe performance too (it might increase the number of full-list
> traversals).
> 
> - With relatively many elements fitting into a single HOB, on most
> platforms, just one HOB is going to be used. While that may be good for
> performance, it is not good for code coverage (testing). The quirky
> indexing method will not be exercised by most platforms.
> 
> What's wrong with:
> 
> - restricting the creation of all such HOBs after
> "gEfiPeiMemoryDiscoveredPpiGuid"
> 
> - using a page allocation, for representing the array contiguously
> 
> - in the HOB, only storing the address of the page allocation.
> 
> Page allocations performed after gEfiPeiMemoryDiscoveredPpiGuid will not
> be moved, so the address in the HOB would be stable.
> 
> Laszlo

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

On Sun, Jan 29, 2023 at 05:24:31AM +0000, Wu, Jiaxin wrote:
> Thanks Gerd raise this open -- how to support more processors due to hob size limitation. 
> 
> Looks multiple hobs is the only way since we have to store each cpu's
> info? Sorry, allow me ask a stupid question: why DataLength in hob
> defined as UINT16 causing the hob size limitation? Any design
> background here?

Probably just nobody expected that big hobs being ever needed when this
was designed looooong ago.

But as laszlo outlined:  There is the option to use a page allocation
for the array and store a pointer to the array in the HOB.  Which is
probably the simplest approach given you have a single, linear array
then.

> For smbase case: I doubt CpuIndex is really required, because we can't
> avoid define another hob, and we can't avoid add statement for each
> hob cpu ranges (0 - 8191, 8192 - 16382,...), then what's meaning for
> the CpuIndex, we don't expect hob producer create smaller granularity
> CPU ranges that one hob CpuIndex associate with previous NumberOfCpus.

With multiple HOBs the consumer needs to know which HOB covers which CPU
range.  So in addition to the number of cpus covered by the HOB you also
need to know what the first CPU is.

take care,
  Gerd

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

> 
> Probably just nobody expected that big hobs being ever needed when this
> was designed looooong ago.
> 
> But as laszlo outlined:  There is the option to use a page allocation
> for the array and store a pointer to the array in the HOB.  Which is
> probably the simplest approach given you have a single, linear array
> then.
> 

Pointer is not the right direction usage in the HOB, it will still bring the complexity in standalone MM part. Because we need duplicate the same hobs for Standalone MM usage, if the hobs contains the point, it will bring another steps to allocate the buffer again for SMM usage.

> > For smbase case: I doubt CpuIndex is really required, because we can't
> > avoid define another hob, and we can't avoid add statement for each
> > hob cpu ranges (0 - 8191, 8192 - 16382,...), then what's meaning for
> > the CpuIndex, we don't expect hob producer create smaller granularity
> > CPU ranges that one hob CpuIndex associate with previous NumberOfCpus.
> 
> With multiple HOBs the consumer needs to know which HOB covers which CPU
> range.  So in addition to the number of cpus covered by the HOB you also
> need to know what the first CPU is.
> 

I original thought that we can fix the CPU ranges for each CPU hob, but it does need define the new hob guid again and again. Well, one hob guid with cpuindex is fine.  

> take care,
>   Gerd

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Wednesday, February 1, 2023 9:15 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add
> SMM Base HOB Data
> 
> On Sun, Jan 29, 2023 at 05:24:31AM +0000, Wu, Jiaxin wrote:
> > Thanks Gerd raise this open -- how to support more processors due to hob
> size limitation.
> >
> > Looks multiple hobs is the only way since we have to store each cpu's
> > info? Sorry, allow me ask a stupid question: why DataLength in hob
> > defined as UINT16 causing the hob size limitation? Any design
> > background here?
> 
> Probably just nobody expected that big hobs being ever needed when this
> was designed looooong ago.
> 
> But as laszlo outlined:  There is the option to use a page allocation
> for the array and store a pointer to the array in the HOB.  Which is
> probably the simplest approach given you have a single, linear array
> then.

Gerd,
I replied in another thread to Laszlo's page allocation proposal to explain
why page allocation is not preferred (considering the standalone MM).

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 20, 2023 4:21 PM
> To: Ni, Ray <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>
> Subject: Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base
> HOB Data
> 
> On 1/18/23 16:06, Ni, Ray wrote:
> 
> > #pragma pack(1)
> > typedef struct {
> >   UINT32    CpuIndex;
> >   UINT32    NumberOfCpus;  // align to
> EFI_SEC_PLATFORM_INFORMATION_RECORD2.NumberOfCpus
> >   UINT64    SmBase[];
> > } SMM_BASE_HOB_DATA;
> > #pragma pack()
> >
> > For system with less than 8K CPUs, one HOB is produced. CpuIndex is set to
> 0 indicating
> > the HOB describes the CPU from 0 to NumberOfCpus-1.
> >
> > The HOB list may contains multiple such HOB instances each describing the
> information for
> > CPU from CpuIndex to CpuIndex + NumberOfCpus - 1.
> > The instance order in the HOB list is random so consumer cannot assume
> the CpuIndex
> > of first instance is 0.
> 
> When using discontiguous blocks that are limited to ~64KB each:
> 
> - The consumer won't be able to access elements of the "conceptual" big
> array in a truly random (= random-access) fashion. There won't be a
> single contiguous domain of valid subscripts. It's "bank switching", and
> switching banks should be avoided IMO. It effectively replaces the
> vector data structure with a linked list. The consequence is that the
> consumer will have to either (a) build a new (temporary, or permanent)
> index table of sorts, for implementing the "conceptual" big array as a
> factual contiguous array, or (b) traverse the HOB list multiple times.

I agree. My recommendation is the consumer queries the total cpu count and
allocate a big-enough temp buffer, then it traverse the HOB and copies
each instance of the SMM information HOB to the proper location in the buffer.
After the SMM rebase is done, the temp buffer can be freed to reduce
SMM memory consumption.

Multi-traverse of the HOB can avoid the temp buffer allocation. But the code
logic will be a bit more complicated. I don't prefer.


> 
> - If the element size of the array increases (which is otherwise
> possible to do compatibly, e.g. by placing a GUID and/or revision# in
> the HOB), the number of elements that fit in a single HOB decreases. I
> think that's an artifact that needlessly complicates debugging, and
> maybe performance too (it might increase the number of full-list
> traversals).

TRUE that multi-instance of the HOBs make the debugging harder.
That's why I propose to have "CpuIndex" field to tell which range CPU the
specific HOB describes.

> 
> - With relatively many elements fitting into a single HOB, on most
> platforms, just one HOB is going to be used. While that may be good for
> performance, it is not good for code coverage (testing). The quirky
> indexing method will not be exercised by most platforms.

TRUE so I propose that the first version of the code change only expects
the HOB.NumberOfCpus equals to the NumberOfCpus returned from MP
service, meaning the code logic only supports single instance of the HOB.
When a platform that contains >8000 cpu threads resulting in multiple HOBs
produced, the expectation will break and remind us that the CpuSmm driver
needs to update to handle multiple HOBs.

> 
> What's wrong with:
> 
> - restricting the creation of all such HOBs after
> "gEfiPeiMemoryDiscoveredPpiGuid"
> 
> - using a page allocation, for representing the array contiguously
> 
> - in the HOB, only storing the address of the page allocation.
> 
> Page allocations performed after gEfiPeiMemoryDiscoveredPpiGuid will not
> be moved, so the address in the HOB would be stable.

It can work. I agree.
However, please think about future standalone MM case. The StandaloneMmIpl
builds HOB list and pass to StandaloneMmCore. The HOB list shall contain the
SMM base information HOB. If we only store a pointer in the HOB, the pointer will
points to a SMRAM carved out from SMRAM by StandaloneMmIpl. Memory service
in StandaloneMmCore cannot be used because StandaloneMmCore hasn't been loaded.
I don't prefer to introduce extra memory management logic in StandaloneMmIpl though
the logic is very simple to carve out some pages from SMRAM.

Thanks,
Ray

Re: [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

  Hi,

> > - With relatively many elements fitting into a single HOB, on most
> > platforms, just one HOB is going to be used. While that may be good for
> > performance, it is not good for code coverage (testing). The quirky
> > indexing method will not be exercised by most platforms.
> 
> TRUE so I propose that the first version of the code change only expects
> the HOB.NumberOfCpus equals to the NumberOfCpus returned from MP
> service, meaning the code logic only supports single instance of the HOB.
> When a platform that contains >8000 cpu threads resulting in multiple HOBs
> produced, the expectation will break and remind us that the CpuSmm driver
> needs to update to handle multiple HOBs.

Given that this is already the second case where we hit the 64k size
limit and I expect it will not be the last one:  I think it makes sense
to introduce a generic and reusable concept of chunked HOBs, so you can
add helper functions to HobLib for splitting and reassembling, with a
struct along the lines of:

typedef struct {
	// offset and size of this particular chunk
	UINT32	ChunkOffset;
	UINT32	ChunkSize;

	// number of chunks and size of all chunks combined.
	UINT32	ChunkCount;
	UINT32	TotalSize;

	// chunk data
	UINT8   Data[0];
} EFI_HOB_CHUNK;

take care,
  Gerd

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Brian J. Johnson]

On 2/2/23 06:51, Gerd Hoffmann wrote:
>    Hi,
> 
>>> - With relatively many elements fitting into a single HOB, on most
>>> platforms, just one HOB is going to be used. While that may be good for
>>> performance, it is not good for code coverage (testing). The quirky
>>> indexing method will not be exercised by most platforms.
>>
>> TRUE so I propose that the first version of the code change only expects
>> the HOB.NumberOfCpus equals to the NumberOfCpus returned from MP
>> service, meaning the code logic only supports single instance of the HOB.
>> When a platform that contains >8000 cpu threads resulting in multiple HOBs
>> produced, the expectation will break and remind us that the CpuSmm driver
>> needs to update to handle multiple HOBs.
> 
> Given that this is already the second case where we hit the 64k size
> limit and I expect it will not be the last one:  I think it makes sense
> to introduce a generic and reusable concept of chunked HOBs, so you can
> add helper functions to HobLib for splitting and reassembling, with a
> struct along the lines of:
> 
> typedef struct {
> 	// offset and size of this particular chunk
> 	UINT32	ChunkOffset;
> 	UINT32	ChunkSize;
> 
> 	// number of chunks and size of all chunks combined.
> 	UINT32	ChunkCount;
> 	UINT32	TotalSize;
> 
> 	// chunk data
> 	UINT8   Data[0];
> } EFI_HOB_CHUNK;
> 
> take care,
>    Gerd
> 

Gerd's suggestion could be handy.  Here's another approach when data is 
too large to fit in a HOB, which doesn't require splitting up the data:

PEI tracks page allocations by generating memory allocation HOBs 
(EFI_HOB_TYPE_MEMORY_ALLOCATION.)  The EFI_HOB_MEMORY_ALLOCATION_HEADER 
structure in the HOB contains a "Name" field of type EFI_GUID which can 
be used to track the purpose of that particular page allocation.  It's 
zeroed by BuildMemoryAllocationHob(), and not usually used.  But if you 
put your own GUID in there, you can use it to track which memory 
allocation HOB corresponds to your data, without having to manage a 
separate HOB with a pointer.  The allocation will be automatically 
tracked through pre-RAM PEI, post-RAM PEI, and DXE, and the pages 
(although not the HOB) will even persist into Runtime (if you use an 
EfiRuntimeServices memory type.)

That wouldn't help the OP with SMM, though.  They would still have to 
copy the pages into SMRAM somehow.

Unfortunately, neither HobLib nor AllocatePages() has an interface for 
setting the "Name" field.  But you can call AllocatePages(), then search 
the HOB list for the resulting HOB, and update it's AllocDescriptor.Name 
field.

-- 
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

Gerd,
Can you please explain a bit more on the chunk idea?

Brian,
Page allocation is not preferred in this SMM case because the pointer
in HOB entry points to another memory. StandaloneMmIpl has to
migrate the "another memory" manually to SMRAM. I want to avoid
that.

Thanks,
Ray

> -----Original Message-----
> From: Brian J. Johnson <brian.johnson@hpe.com>
> Sent: Friday, February 3, 2023 6:29 AM
> To: devel@edk2.groups.io; kraxel@redhat.com; Ni, Ray <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Zimmer, Vincent <vincent.zimmer@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM
> Base HOB Data
> 
> On 2/2/23 06:51, Gerd Hoffmann wrote:
> >    Hi,
> >
> >>> - With relatively many elements fitting into a single HOB, on most
> >>> platforms, just one HOB is going to be used. While that may be good for
> >>> performance, it is not good for code coverage (testing). The quirky
> >>> indexing method will not be exercised by most platforms.
> >>
> >> TRUE so I propose that the first version of the code change only expects
> >> the HOB.NumberOfCpus equals to the NumberOfCpus returned from MP
> >> service, meaning the code logic only supports single instance of the HOB.
> >> When a platform that contains >8000 cpu threads resulting in multiple HOBs
> >> produced, the expectation will break and remind us that the CpuSmm driver
> >> needs to update to handle multiple HOBs.
> >
> > Given that this is already the second case where we hit the 64k size
> > limit and I expect it will not be the last one:  I think it makes sense
> > to introduce a generic and reusable concept of chunked HOBs, so you can
> > add helper functions to HobLib for splitting and reassembling, with a
> > struct along the lines of:
> >
> > typedef struct {
> > 	// offset and size of this particular chunk
> > 	UINT32	ChunkOffset;
> > 	UINT32	ChunkSize;
> >
> > 	// number of chunks and size of all chunks combined.
> > 	UINT32	ChunkCount;
> > 	UINT32	TotalSize;
> >
> > 	// chunk data
> > 	UINT8   Data[0];
> > } EFI_HOB_CHUNK;
> >
> > take care,
> >    Gerd
> >
> 
> Gerd's suggestion could be handy.  Here's another approach when data is
> too large to fit in a HOB, which doesn't require splitting up the data:
> 
> PEI tracks page allocations by generating memory allocation HOBs
> (EFI_HOB_TYPE_MEMORY_ALLOCATION.)  The
> EFI_HOB_MEMORY_ALLOCATION_HEADER
> structure in the HOB contains a "Name" field of type EFI_GUID which can
> be used to track the purpose of that particular page allocation.  It's
> zeroed by BuildMemoryAllocationHob(), and not usually used.  But if you
> put your own GUID in there, you can use it to track which memory
> allocation HOB corresponds to your data, without having to manage a
> separate HOB with a pointer.  The allocation will be automatically
> tracked through pre-RAM PEI, post-RAM PEI, and DXE, and the pages
> (although not the HOB) will even persist into Runtime (if you use an
> EfiRuntimeServices memory type.)
> 
> That wouldn't help the OP with SMM, though.  They would still have to
> copy the pages into SMRAM somehow.
> 
> Unfortunately, neither HobLib nor AllocatePages() has an interface for
> setting the "Name" field.  But you can call AllocatePages(), then search
> the HOB list for the resulting HOB, and update it's AllocDescriptor.Name
> field.
> 
> --
> Brian J. Johnson
> Enterprise X86 Lab
> 
> Hewlett Packard Enterprise

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

On Fri, Feb 03, 2023 at 03:14:42AM +0000, Ni, Ray wrote:
> Gerd,
> Can you please explain a bit more on the chunk idea?

> > > to introduce a generic and reusable concept of chunked HOBs, so you can
> > > add helper functions to HobLib for splitting and reassembling, with a
> > > struct along the lines of:
> > >
> > > typedef struct {
> > > 	// offset and size of this particular chunk
> > > 	UINT32	ChunkOffset;
> > > 	UINT32	ChunkSize;
> > >
> > > 	// number of chunks and size of all chunks combined.
> > > 	UINT32	ChunkCount;
> > > 	UINT32	TotalSize;
> > >
> > > 	// chunk data
> > > 	UINT8   Data[0];
> > > } EFI_HOB_CHUNK;

Reassembling would work like this:

  // once
  AssembledHob = malloc(HobChunk->TotalSize);

  // for each chunk
  memcpy(AssembledHob + HobChunk->ChunkOffset, HobChunk->Data, HobChunk->ChunkSize);

Possible shortcut:

  if (HobChunk->ChunkSize == HobChunk->TotalSize)
    // data is not splitted into multiple chunks
    AssembledHob = HobChunk->Data

Advantage: you avoid the allocation in case the data fits into a single
HOB.  Disadvantage: you need to track whenever AssembledHob is allocated
(and must eventually be freed) or not.

HobChunk->ChunkCount is not really needed but might be useful for sanity
checking.

take care,
  Gerd

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Wu, Jiaxin]

Hi Gerd,

Is it still the solution with multiple hobs created for big data but you want the hob splitting and reassembling can be encapsulated in the Hoblib?

Does it need define the new hob type for chunk?

Thanks,
Jiaxin

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Friday, February 3, 2023 3:55 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Johnson, Brian <brian.johnson@hpe.com>; devel@edk2.groups.io; Laszlo
> Ersek <lersek@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Zimmer, Vincent <vincent.zimmer@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add
> SMM Base HOB Data
> 
> On Fri, Feb 03, 2023 at 03:14:42AM +0000, Ni, Ray wrote:
> > Gerd,
> > Can you please explain a bit more on the chunk idea?
> 
> > > > to introduce a generic and reusable concept of chunked HOBs, so you can
> > > > add helper functions to HobLib for splitting and reassembling, with a
> > > > struct along the lines of:
> > > >
> > > > typedef struct {
> > > > 	// offset and size of this particular chunk
> > > > 	UINT32	ChunkOffset;
> > > > 	UINT32	ChunkSize;
> > > >
> > > > 	// number of chunks and size of all chunks combined.
> > > > 	UINT32	ChunkCount;
> > > > 	UINT32	TotalSize;
> > > >
> > > > 	// chunk data
> > > > 	UINT8   Data[0];
> > > > } EFI_HOB_CHUNK;
> 
> Reassembling would work like this:
> 
>   // once
>   AssembledHob = malloc(HobChunk->TotalSize);
> 
>   // for each chunk
>   memcpy(AssembledHob + HobChunk->ChunkOffset, HobChunk->Data,
> HobChunk->ChunkSize);
> 
> Possible shortcut:
> 
>   if (HobChunk->ChunkSize == HobChunk->TotalSize)
>     // data is not splitted into multiple chunks
>     AssembledHob = HobChunk->Data
> 
> Advantage: you avoid the allocation in case the data fits into a single
> HOB.  Disadvantage: you need to track whenever AssembledHob is allocated
> (and must eventually be freed) or not.
> 
> HobChunk->ChunkCount is not really needed but might be useful for sanity
> checking.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Ni, Ray]

I think we need to define a new chunk hob type for solution proposed by Gerd.
I will leave that to PIWG (PI spec working group) for discussion.

In this topic, I prefer to solve the problem in the CPU domain. 

> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Friday, February 3, 2023 9:23 PM
> To: kraxel@redhat.com; Ni, Ray <ray.ni@intel.com>
> Cc: Johnson, Brian <brian.johnson@hpe.com>; devel@edk2.groups.io; Laszlo
> Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM
> Base HOB Data
> 
> Hi Gerd,
> 
> Is it still the solution with multiple hobs created for big data but you want the
> hob splitting and reassembling can be encapsulated in the Hoblib?
> 
> Does it need define the new hob type for chunk?
> 
> Thanks,
> Jiaxin
> 
> > -----Original Message-----
> > From: kraxel@redhat.com <kraxel@redhat.com>
> > Sent: Friday, February 3, 2023 3:55 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: Johnson, Brian <brian.johnson@hpe.com>; devel@edk2.groups.io; Laszlo
> > Ersek <lersek@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Kumar, Rahul R
> > <rahul.r.kumar@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Zimmer, Vincent <vincent.zimmer@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add
> > SMM Base HOB Data
> >
> > On Fri, Feb 03, 2023 at 03:14:42AM +0000, Ni, Ray wrote:
> > > Gerd,
> > > Can you please explain a bit more on the chunk idea?
> >
> > > > > to introduce a generic and reusable concept of chunked HOBs, so you
> can
> > > > > add helper functions to HobLib for splitting and reassembling, with a
> > > > > struct along the lines of:
> > > > >
> > > > > typedef struct {
> > > > > 	// offset and size of this particular chunk
> > > > > 	UINT32	ChunkOffset;
> > > > > 	UINT32	ChunkSize;
> > > > >
> > > > > 	// number of chunks and size of all chunks combined.
> > > > > 	UINT32	ChunkCount;
> > > > > 	UINT32	TotalSize;
> > > > >
> > > > > 	// chunk data
> > > > > 	UINT8   Data[0];
> > > > > } EFI_HOB_CHUNK;
> >
> > Reassembling would work like this:
> >
> >   // once
> >   AssembledHob = malloc(HobChunk->TotalSize);
> >
> >   // for each chunk
> >   memcpy(AssembledHob + HobChunk->ChunkOffset, HobChunk->Data,
> > HobChunk->ChunkSize);
> >
> > Possible shortcut:
> >
> >   if (HobChunk->ChunkSize == HobChunk->TotalSize)
> >     // data is not splitted into multiple chunks
> >     AssembledHob = HobChunk->Data
> >
> > Advantage: you avoid the allocation in case the data fits into a single
> > HOB.  Disadvantage: you need to track whenever AssembledHob is allocated
> > (and must eventually be freed) or not.
> >
> > HobChunk->ChunkCount is not really needed but might be useful for sanity
> > checking.
> >
> > take care,
> >   Gerd

Re: [edk2-devel] [PATCH v3 1/5] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data

[Gerd Hoffmann]

On Fri, Feb 03, 2023 at 01:22:48PM +0000, Wu, Jiaxin wrote:
> Hi Gerd,
> 
> Is it still the solution with multiple hobs created for big data but you want the hob splitting and reassembling can be encapsulated in the Hoblib?

Exactly.

> Does it need define the new hob type for chunk?

Adding EFI_HOB_TYPE_GUID_EXTENSION_CHUNKED is probably a good idea.

take care,
  Gerd

[PATCH v3 2/5] UefiCpuPkg/PiSmmCpuDxeSmm: Fix invalid InitializeMpSyncData call

[Wu, Jiaxin]

No need call InitializeMpSyncData during normal boot SMI init,
because mSmmMpSyncData is NULL at that time. mSmmMpSyncData is
allocated in InitializeMpServiceData, which is invoked after
normal boot SMI init (SmmRelocateBases).

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 655175a2c6..f723b1d253 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -369,13 +369,11 @@ SmmInitHandler (
       if (!mSmmS3Flag) {
         //
         // Check XD and BTS features on each processor on normal boot
         //
         CheckFeatureSupported ();
-      }
-
-      if (mIsBsp) {
+      } else if (mIsBsp) {
         //
         // BSP rebase is already done above.
         // Initialize private data during S3 resume
         //
         InitializeMpSyncData ();
-- 
2.16.2.windows.1

[PATCH v3 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

PiSmmCpuDxeSmm shall retrieve the SMBASE addresses from SMM Base Hob
and installs the SMI handler at [SMBASE+8000h] for each processor
instead of relocating SMM Base addresses from SMRAM again if SMM Base
Hob existed.

With SMM Base Hob, PiSmmCpuDxeSmm does not need the RSM
instruction to reload the SMBASE register with the new allocated
SMBASE each time when it exits SMM. SMBASE Register for each
processors have already been programmed and all SMBASE address
have recorded in SMM Base Hob. So the same default SMBASE Address
(0x30000) will not be used, thus the CPUs over-writing each
other's SMM Save State Area will not happen in PiSmmCpuDxeSmm
driver. This way makes the first SMI init can be executed in
parallel and save boot time on multi-core system.

Mainly changes as below:
*Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one
(gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths:
one to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core
Entry Point.
*Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for first
SMI init before normal SMI sources happen.
*Call SmmCpuFeaturesInitializeProcessor() in parallel.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c            |  29 ++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        |  23 ++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 182 +++++++++++++++++++++------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  24 ++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   1 +
 5 files changed, 217 insertions(+), 42 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index fb4a44eab6..39c0b002f0 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -822,13 +822,38 @@ SmmRestoreCpu (
     //
     InitializeCpuBeforeRebase ();
   }
 
   //
-  // Restore SMBASE for BSP and all APs
+  // Make sure the gSmmBaseHobGuid existence status is the same between normal and S3 boot.
   //
-  SmmRelocateBases ();
+  ASSERT (mSmmRelocated == (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL));
+  if (mSmmRelocated != (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "gSmmBaseHobGuid %a produced in normal boot but %a in S3 boot!",
+      mSmmRelocated ? "is" : "is not",
+      mSmmRelocated ? "is not" : "is"
+      ));
+    CpuDeadLoop ();
+  }
+
+  //
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
+  //
+  if (!mSmmRelocated) {
+    //
+    // Restore SMBASE for BSP and all APs
+    //
+    SmmRelocateBases ();
+  } else {
+    //
+    // Issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+    //
+    ExecuteFirstSmiInit ();
+  }
 
   //
   // Skip initialization if mAcpiCpuData is not valid
   //
   if (mAcpiCpuData.NumberOfCpus > 0) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index a0967eb69c..3744f35214 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1721,17 +1721,40 @@ SmiRendezvous (
   UINTN       Index;
   UINTN       Cr2;
 
   ASSERT (CpuIndex < mMaxNumberOfCpus);
 
+  if (mSmmRelocated) {
+    ASSERT (mSmmInitialized != NULL);
+  }
+
   //
   // Save Cr2 because Page Fault exception in SMM may override its value,
   // when using on-demand paging for above 4G memory.
   //
   Cr2 = 0;
   SaveCr2 (&Cr2);
 
+  if (mSmmRelocated && !mSmmInitialized[CpuIndex]) {
+    //
+    // Perform SmmInitHandler for CpuIndex
+    //
+    SmmInitHandler ();
+
+    //
+    // Restore Cr2
+    //
+    RestoreCr2 (Cr2);
+
+    //
+    // Mark the first SMI init for CpuIndex has been done so as to avoid the reentry.
+    //
+    mSmmInitialized[CpuIndex] = TRUE;
+
+    return;
+  }
+
   //
   // Call the user register Startup function first.
   //
   if (mSmmMpSyncData->StartupProcedure != NULL) {
     mSmmMpSyncData->StartupProcedure (mSmmMpSyncData->StartupProcArgs);
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index f723b1d253..a39d8528db 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -57,11 +57,10 @@ SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate = &mSmmCpuPrivateData;
 
 //
 // SMM Relocation variables
 //
 volatile BOOLEAN  *mRebased;
-volatile BOOLEAN  mIsBsp;
 
 ///
 /// Handle for the SMM CPU Protocol
 ///
 EFI_HANDLE  mSmmCpuHandle = NULL;
@@ -83,10 +82,14 @@ EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL  mSmmMemoryAttribute = {
   EdkiiSmmClearMemoryAttributes
 };
 
 EFI_CPU_INTERRUPT_HANDLER  mExternalVectorTable[EXCEPTION_VECTOR_NUMBER];
 
+BOOLEAN  mSmmRelocated    = FALSE;
+BOOLEAN  *mSmmInitialized = NULL;
+UINT32   mBspApicId       = 0;
+
 //
 // SMM stack information
 //
 UINTN  mSmmStackArrayBase;
 UINTN  mSmmStackArrayEnd;
@@ -341,58 +344,108 @@ VOID
 EFIAPI
 SmmInitHandler (
   VOID
   )
 {
-  UINT32  ApicId;
-  UINTN   Index;
+  UINT32   ApicId;
+  UINTN    Index;
+  BOOLEAN  IsBsp;
 
   //
   // Update SMM IDT entries' code segment and load IDT
   //
   AsmWriteIdtr (&gcSmiIdtr);
   ApicId = GetApicId ();
 
+  IsBsp = (BOOLEAN)(mBspApicId == ApicId);
+
   ASSERT (mNumberOfCpus <= mMaxNumberOfCpus);
 
   for (Index = 0; Index < mNumberOfCpus; Index++) {
     if (ApicId == (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
       //
       // Initialize SMM specific features on the currently executing CPU
       //
       SmmCpuFeaturesInitializeProcessor (
         Index,
-        mIsBsp,
+        IsBsp,
         gSmmCpuPrivate->ProcessorInfo,
         &mCpuHotPlugData
         );
 
       if (!mSmmS3Flag) {
         //
         // Check XD and BTS features on each processor on normal boot
         //
         CheckFeatureSupported ();
-      } else if (mIsBsp) {
+      } else if (IsBsp) {
         //
         // BSP rebase is already done above.
         // Initialize private data during S3 resume
         //
         InitializeMpSyncData ();
       }
 
-      //
-      // Hook return after RSM to set SMM re-based flag
-      //
-      SemaphoreHook (Index, &mRebased[Index]);
+      if (!mSmmRelocated) {
+        //
+        // Hook return after RSM to set SMM re-based flag
+        //
+        SemaphoreHook (Index, &mRebased[Index]);
+      }
 
       return;
     }
   }
 
   ASSERT (FALSE);
 }
 
+/**
+  Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+
+**/
+VOID
+ExecuteFirstSmiInit (
+  VOID
+  )
+{
+  UINTN  Index;
+
+  if (mSmmInitialized == NULL) {
+    mSmmInitialized = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+  }
+
+  ASSERT (mSmmInitialized != NULL);
+  if (mSmmInitialized == NULL) {
+    return;
+  }
+
+  //
+  // Reset the mSmmInitialized to false.
+  //
+  ZeroMem (mSmmInitialized, sizeof (BOOLEAN) * mMaxNumberOfCpus);
+
+  //
+  // Get the BSP ApicId.
+  //
+  mBspApicId = GetApicId ();
+
+  //
+  // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
+  //
+  SendSmiIpi (mBspApicId);
+  SendSmiIpiAllExcludingSelf ();
+
+  //
+  // Wait for all processors to finish its 1st SMI
+  //
+  for (Index = 0; Index < mNumberOfCpus; Index++) {
+    while (mSmmInitialized[Index] == FALSE) {
+    }
+  }
+}
+
 /**
   Relocate SmmBases for each processor.
 
   Execute on first boot and all S3 resumes
 
@@ -405,11 +458,10 @@ SmmRelocateBases (
 {
   UINT8                 BakBuf[BACK_BUF_SIZE];
   SMRAM_SAVE_STATE_MAP  BakBuf2;
   SMRAM_SAVE_STATE_MAP  *CpuStatePtr;
   UINT8                 *U8Ptr;
-  UINT32                ApicId;
   UINTN                 Index;
   UINTN                 BspIndex;
 
   //
   // Make sure the reserved size is large enough for procedure SmmInitTemplate.
@@ -446,21 +498,20 @@ SmmRelocateBases (
   CopyMem (U8Ptr, gcSmmInitTemplate, gcSmmInitSize);
 
   //
   // Retrieve the local APIC ID of current processor
   //
-  ApicId = GetApicId ();
+  mBspApicId = GetApicId ();
 
   //
   // Relocate SM bases for all APs
   // This is APs' 1st SMI - rebase will be done here, and APs' default SMI handler will be overridden by gcSmmInitTemplate
   //
-  mIsBsp   = FALSE;
   BspIndex = (UINTN)-1;
   for (Index = 0; Index < mNumberOfCpus; Index++) {
     mRebased[Index] = FALSE;
-    if (ApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
+    if (mBspApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
       SendSmiIpi ((UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId);
       //
       // Wait for this AP to finish its 1st SMI
       //
       while (!mRebased[Index]) {
@@ -475,12 +526,11 @@ SmmRelocateBases (
 
   //
   // Relocate BSP's SMM base
   //
   ASSERT (BspIndex != (UINTN)-1);
-  mIsBsp = TRUE;
-  SendSmiIpi (ApicId);
+  SendSmiIpi (mBspApicId);
   //
   // Wait for the BSP to finish its 1st SMI
   //
   while (!mRebased[BspIndex]) {
   }
@@ -559,10 +609,15 @@ PiCpuSmmEntry (
   UINT32                    RegEcx;
   UINT32                    RegEdx;
   UINTN                     FamilyId;
   UINTN                     ModelId;
   UINT32                    Cr3;
+  EFI_HOB_GUID_TYPE         *GuidHob;
+  SMM_BASE_HOB_DATA         *SmmBaseHobData;
+
+  GuidHob        = NULL;
+  SmmBaseHobData = NULL;
 
   //
   // Initialize address fixup
   //
   PiSmmCpuSmmInitFixupAddress ();
@@ -787,30 +842,55 @@ PiCpuSmmEntry (
   // context must be reduced.
   //
   ASSERT (TileSize <= (SMRAM_SAVE_STATE_MAP_OFFSET + sizeof (SMRAM_SAVE_STATE_MAP) - SMM_HANDLER_OFFSET));
 
   //
-  // Allocate buffer for all of the tiles.
+  // Check whether the Required TileSize is enough.
   //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 34.11 SMBASE Relocation
-  //   For Pentium and Intel486 processors, the SMBASE values must be
-  //   aligned on a 32-KByte boundary or the processor will enter shutdown
-  //   state during the execution of a RSM instruction.
+  if (TileSize > SIZE_8KB) {
+    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough -- Required TileSize = 0x%08x, Actual TileSize = 0x%08x\n", TileSize, SIZE_8KB));
+    CpuDeadLoop ();
+    return RETURN_BUFFER_TOO_SMALL;
+  }
+
   //
-  // Intel486 processors: FamilyId is 4
-  // Pentium processors : FamilyId is 5
+  // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
+  // means the SmBase relocation has been done.
   //
-  BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1));
-  if ((FamilyId == 4) || (FamilyId == 5)) {
-    Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
+  GuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
+  if (GuidHob != NULL) {
+    SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
+
+    ASSERT (SmmBaseHobData->NumberOfProcessors == mMaxNumberOfCpus);
+    mSmmRelocated = TRUE;
   } else {
-    Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
-  }
+    //
+    // When the HOB doesn't exist, allocate new SMBASE itself.
+    //
+    DEBUG ((DEBUG_INFO, "PiCpuSmmEntry: gSmmBaseHobGuid not found!\n"));
+    //
+    // Allocate buffer for all of the tiles.
+    //
+    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+    // Volume 3C, Section 34.11 SMBASE Relocation
+    //   For Pentium and Intel486 processors, the SMBASE values must be
+    //   aligned on a 32-KByte boundary or the processor will enter shutdown
+    //   state during the execution of a RSM instruction.
+    //
+    // Intel486 processors: FamilyId is 4
+    // Pentium processors : FamilyId is 5
+    //
+    BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1));
+    if ((FamilyId == 4) || (FamilyId == 5)) {
+      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
+    } else {
+      Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
+    }
 
-  ASSERT (Buffer != NULL);
-  DEBUG ((DEBUG_INFO, "SMRAM SaveState Buffer (0x%08x, 0x%08x)\n", Buffer, EFI_PAGES_TO_SIZE (BufferPages)));
+    ASSERT (Buffer != NULL);
+    DEBUG ((DEBUG_INFO, "New Allcoated SMRAM SaveState Buffer (0x%08x, 0x%08x)\n", Buffer, EFI_PAGES_TO_SIZE (BufferPages)));
+  }
 
   //
   // Allocate buffer for pointers to array in  SMM_CPU_PRIVATE_DATA.
   //
   gSmmCpuPrivate->ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EFI_PROCESSOR_INFORMATION) * mMaxNumberOfCpus);
@@ -841,11 +921,12 @@ PiCpuSmmEntry (
   // Retrieve APIC ID of each enabled processor from the MP Services protocol.
   // Also compute the SMBASE address, CPU Save State address, and CPU Save state
   // size for each CPU in the platform
   //
   for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
-    mCpuHotPlugData.SmBase[Index]           = (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+    mCpuHotPlugData.SmBase[Index] = mSmmRelocated ? (UINTN)SmmBaseHobData->SmBase[Index] : (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+
     gSmmCpuPrivate->CpuSaveStateSize[Index] = sizeof (SMRAM_SAVE_STATE_MAP);
     gSmmCpuPrivate->CpuSaveState[Index]     = (VOID *)(mCpuHotPlugData.SmBase[Index] + SMRAM_SAVE_STATE_MAP_OFFSET);
     gSmmCpuPrivate->Operation[Index]        = SmmCpuNone;
 
     if (Index < mNumberOfCpus) {
@@ -954,21 +1035,27 @@ PiCpuSmmEntry (
   // Initialize IDT
   //
   InitializeSmmIdt ();
 
   //
-  // Relocate SMM Base addresses to the ones allocated from SMRAM
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
   //
-  mRebased = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
-  ASSERT (mRebased != NULL);
-  SmmRelocateBases ();
+  if (!mSmmRelocated) {
+    //
+    // Relocate SMM Base addresses to the ones allocated from SMRAM
+    //
+    mRebased = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+    ASSERT (mRebased != NULL);
+    SmmRelocateBases ();
 
-  //
-  // Call hook for BSP to perform extra actions in normal mode after all
-  // SMM base addresses have been relocated on all CPUs
-  //
-  SmmCpuFeaturesSmmRelocationComplete ();
+    //
+    // Call hook for BSP to perform extra actions in normal mode after all
+    // SMM base addresses have been relocated on all CPUs
+    //
+    SmmCpuFeaturesSmmRelocationComplete ();
+  }
 
   DEBUG ((DEBUG_INFO, "mXdSupported - 0x%x\n", mXdSupported));
 
   //
   // SMM Time initialization
@@ -995,10 +1082,25 @@ PiCpuSmmEntry (
           );
       }
     }
   }
 
+  //
+  // For relocated SMBASE, some MSRs & CSRs are still required to be configured in SMM Mode for SMM Initialization.
+  // Those MSRs & CSRs must be configured before normal SMI sources happen.
+  // So, here is to issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+  //
+  if (mSmmRelocated) {
+    ExecuteFirstSmiInit ();
+
+    //
+    // Call hook for BSP to perform extra actions in normal mode after all
+    // SMM base addresses have been relocated on all CPUs
+    //
+    SmmCpuFeaturesSmmRelocationComplete ();
+  }
+
   //
   // Fill in SMM Reserved Regions
   //
   gSmmCpuPrivate->SmmReservedSmramRegion[0].SmramReservedStart = 0;
   gSmmCpuPrivate->SmmReservedSmramRegion[0].SmramReservedSize  = 0;
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 5f0a38e400..50c9695c4f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -23,10 +23,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/MmMp.h>
 
 #include <Guid/AcpiS3Context.h>
 #include <Guid/MemoryAttributesTable.h>
 #include <Guid/PiSmmMemoryAttributesTable.h>
+#include <Guid/SmmBaseHob.h>
 
 #include <Library/BaseLib.h>
 #include <Library/IoLib.h>
 #include <Library/TimerLib.h>
 #include <Library/SynchronizationLib.h>
@@ -346,10 +347,29 @@ SmmWriteSaveState (
   IN EFI_SMM_SAVE_STATE_REGISTER  Register,
   IN UINTN                        CpuIndex,
   IN CONST VOID                   *Buffer
   );
 
+/**
+  C function for SMI handler. To change all processor's SMMBase Register.
+
+**/
+VOID
+EFIAPI
+SmmInitHandler (
+  VOID
+  );
+
+/**
+  Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+
+**/
+VOID
+ExecuteFirstSmiInit (
+  VOID
+  );
+
 /**
 Read a CPU Save State register on the target processor.
 
 This function abstracts the differences that whether the CPU Save State register is in the
 IA32 CPU Save State Map or X64 CPU Save State Map.
@@ -400,10 +420,14 @@ WriteSaveStateRegister (
   IN EFI_SMM_SAVE_STATE_REGISTER  Register,
   IN UINTN                        Width,
   IN CONST VOID                   *Buffer
   );
 
+extern BOOLEAN  mSmmRelocated;
+extern BOOLEAN  *mSmmInitialized;
+extern UINT32   mBspApicId;
+
 extern CONST UINT8        gcSmmInitTemplate[];
 extern CONST UINT16       gcSmmInitSize;
 X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr0;
 extern UINT32             mSmmCr0;
 X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr3;
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index b4b327f60c..6dbed17b96 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -112,10 +112,11 @@
 
 [Guids]
   gEfiAcpiVariableGuid                     ## SOMETIMES_CONSUMES ## HOB # it is used for S3 boot.
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
   gEfiMemoryAttributesTableGuid            ## CONSUMES ## SystemTable
+  gSmmBaseHobGuid                          ## CONSUMES
 
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
-- 
2.16.2.windows.1

Re: [PATCH v3 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Gerd Hoffmann]

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index f723b1d253..a39d8528db 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -57,11 +57,10 @@ SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate = &mSmmCpuPrivateData;
>  
>  //
>  // SMM Relocation variables
>  //
>  volatile BOOLEAN  *mRebased;
> -volatile BOOLEAN  mIsBsp;

> +UINT32   mBspApicId       = 0;

This should be moved to a separate patch with commit message explaining
the reasons for the change.  My guess would be this is required to allow
processors running SmmInitHandler in parallel.

Why mIsBsp is removed but mRebased is not?

> -  // Allocate buffer for all of the tiles.
> +  // Check whether the Required TileSize is enough.
>    //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 34.11 SMBASE Relocation
> -  //   For Pentium and Intel486 processors, the SMBASE values must be
> -  //   aligned on a 32-KByte boundary or the processor will enter shutdown
> -  //   state during the execution of a RSM instruction.
> +  if (TileSize > SIZE_8KB) {
> +    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough -- Required TileSize = 0x%08x, Actual TileSize = 0x%08x\n", TileSize, SIZE_8KB));
> +    CpuDeadLoop ();
> +    return RETURN_BUFFER_TOO_SMALL;
> +  }

Where does the 8K come from?

This change is not mentioned in the commit message and most likely
should be a separate patch.

take care,
  Gerd

Re: [edk2-devel] [PATCH v3 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

[Wu, Jiaxin]

> 
> > +UINT32   mBspApicId       = 0;
> 
> This should be moved to a separate patch with commit message explaining
> the reasons for the change.  My guess would be this is required to allow
> processors running SmmInitHandler in parallel.
> 

Yes, it's part of work to combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one (gcSmiHandlerTemplate). And gcSmiHandlerTemplate will call the same SmmInitHandler for the first smi init. For smm CPU driver, we need keep it in the same patch, separate patch will make another patch not work, because we need replace the mIsBsp to IsBsp: 
IsBsp = (BOOLEAN)(mBspApicId == ApicId);

While the mBspApicId is need added one.


> Why mIsBsp is removed but mRebased is not?

mRebased is critical semaphore for each cpu to finish its 1st SMI. Remove it will make the function not work.


> 
> > -  // Allocate buffer for all of the tiles.
> > +  // Check whether the Required TileSize is enough.
> >    //
> > -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > -  // Volume 3C, Section 34.11 SMBASE Relocation
> > -  //   For Pentium and Intel486 processors, the SMBASE values must be
> > -  //   aligned on a 32-KByte boundary or the processor will enter shutdown
> > -  //   state during the execution of a RSM instruction.
> > +  if (TileSize > SIZE_8KB) {
> > +    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not
> enough -- Required TileSize = 0x%08x, Actual TileSize = 0x%08x\n", TileSize,
> SIZE_8KB));
> > +    CpuDeadLoop ();
> > +    return RETURN_BUFFER_TOO_SMALL;
> > +  }
> 
> Where does the 8K come from?
> 
> This change is not mentioned in the commit message and most likely
> should be a separate patch.
> 

This is about the tile size assumption: 
One processor SMM Base tile size of buffer required to hold in SMRAM:
1. CPU SMRAM Save State Map starts at SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET(0xfc00),
2. extra CPU specific context start starts at SMBASE + SMM_PSD_OFFSET (PROCESSOR SMM DESCRIPTO, 0xfb00),
3. SMI entry point starts at SMBASE + SMM_HANDLER_OFFSET (0x8000).
This size is rounded up to nearest power of 2.
The pre-assigned smbase in hob is based on the biggest possibility of  tile size. We think it's impossible that tile size bigger than 8k, that's the reason we add the check here. I agree to make this as separate patch.


> take care,
>   Gerd
> 
> 
> 
> 
> 

[PATCH v3 4/5] UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

This patch is to avoid configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |  2 ++
 .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     | 23 +++++++++++++++++++---
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 ++++
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |  1 +
 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |  1 -
 .../StandaloneMmCpuFeaturesLib.inf                 |  4 ++++
 6 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
index fd3e902547..c2e4fbe96b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -7,15 +7,17 @@
 **/
 
 #ifndef CPU_FEATURES_LIB_H_
 #define CPU_FEATURES_LIB_H_
 
+#include <Guid/SmmBaseHob.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/BaseLib.h>
 #include <Library/PcdLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
 
 /**
   Performs library initialization.
 
   This initialization function contains common functionality shared betwen all
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
index d5eaaa7a99..7c3836286b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
@@ -36,10 +36,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 // Set default value to assume IA-32 Architectural MSRs are used
 //
 UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
 UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
 
+//
+// Indicate SmBase for each Processors has been relocated or not. If TRUE,
+// means no need to do the relocation in SmmCpuFeaturesInitializeProcessor().
+//
+BOOLEAN  mSmmCpuFeaturesSmmRelocated;
+
 //
 // Set default value to assume MTRRs need to be configured on each SMI
 //
 BOOLEAN  mNeedConfigureMtrrs = TRUE;
 
@@ -142,10 +148,16 @@ CpuFeaturesLibInitialization (
   //
   // Allocate array for state of SMRR enable on all CPUs
   //
   mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * GetCpuMaxLogicalProcessorNumber ());
   ASSERT (mSmrrEnabled != NULL);
+
+  //
+  // If gSmmBaseHobGuid found, means SmBase info has been relocated and recorded
+  // in the SmBase array.
+  //
+  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
 }
 
 /**
   Called during the very first SMI into System Management Mode to initialize
   CPU features, including SMBASE, for the currently executing CPU.  Since this
@@ -185,14 +197,19 @@ SmmCpuFeaturesInitializeProcessor (
   UINT32                RegEdx;
   UINTN                 FamilyId;
   UINTN                 ModelId;
 
   //
-  // Configure SMBASE.
+  // No need to configure SMBASE if SmBase relocation has been done.
   //
-  CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
-  CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  if (!mSmmCpuFeaturesSmmRelocated) {
+    //
+    // Configure SMBASE.
+    //
+    CpuState             = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
+    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  }
 
   //
   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
   // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
   //
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 9ac7dde78f..280a4b8b39 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -31,10 +31,14 @@
 [LibraryClasses]
   BaseLib
   PcdLib
   MemoryAllocationLib
   DebugLib
+  HobLib
+
+[Guids]
+  gSmmBaseHobGuid                ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 86d367e0a0..4bb045244b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -62,10 +62,11 @@
 
 [Guids]
   gMsegSmramGuid                           ## SOMETIMES_CONSUMES ## HOB
   gEfiAcpi20TableGuid                      ## SOMETIMES_CONSUMES ## SystemTable
   gEfiAcpi10TableGuid                      ## SOMETIMES_CONSUMES ## SystemTable
+  gSmmBaseHobGuid                          ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize                         ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize         ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
index 3cf162ada0..455fe83991 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
@@ -6,11 +6,10 @@
 
 **/
 
 #include <PiMm.h>
 #include <Library/BaseMemoryLib.h>
-#include <Library/HobLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/TpmMeasurementLib.h>
 #include <Register/Intel/Cpuid.h>
 #include <Register/Intel/ArchitecturalMsr.h>
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index b1f60a5505..63259e44e7 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -32,10 +32,14 @@
 [LibraryClasses]
   BaseLib
   DebugLib
   MemoryAllocationLib
   PcdLib
+  HobLib
+
+[Guids]
+  gSmmBaseHobGuid                ## CONSUMES
 
 [FixedPcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
-- 
2.16.2.windows.1

[PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

This patch is to avoid configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 37 ++++++++++++++++------
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 +++
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 6693666d04..d2841af6a4 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,20 +15,28 @@
 #include <Library/PcdLib.h>
 #include <Library/SafeIntLib.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/HobLib.h>
 #include <Pcd/CpuHotEjectData.h>
 #include <PiSmm.h>
 #include <Register/Intel/SmramSaveStateMap.h>
 #include <Register/QemuSmramSaveStateMap.h>
+#include <Guid/SmmBaseHob.h>
 
 //
 // EFER register LMA bit
 //
 #define LMA  BIT10
 
+//
+// Indicate SmBase for each Processors has been relocated or not. If TRUE,
+// means no need to do the relocation in SmmCpuFeaturesInitializeProcessor().
+//
+BOOLEAN  mSmmCpuFeaturesSmmRelocated;
+
 /**
   The constructor function
 
   @param[in]  ImageHandle  The firmware allocated handle for the EFI image.
   @param[in]  SystemTable  A pointer to the EFI System Table.
@@ -41,10 +49,16 @@ EFIAPI
 SmmCpuFeaturesLibConstructor (
   IN EFI_HANDLE        ImageHandle,
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  //
+  // If gSmmBaseHobGuid found, means SmBase info has been relocated and recorded
+  // in the SmBase array.
+  //
+  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
+
   //
   // No need to program SMRRs on our virtual platform.
   //
   return EFI_SUCCESS;
 }
@@ -83,20 +97,25 @@ SmmCpuFeaturesInitializeProcessor (
   )
 {
   QEMU_SMRAM_SAVE_STATE_MAP  *CpuState;
 
   //
-  // Configure SMBASE.
+  // No need to configure SMBASE if SmBase relocation has been done.
   //
-  CpuState = (QEMU_SMRAM_SAVE_STATE_MAP *)(UINTN)(
-                                                  SMM_DEFAULT_SMBASE +
-                                                  SMRAM_SAVE_STATE_MAP_OFFSET
-                                                  );
-  if ((CpuState->x86.SMMRevId & 0xFFFF) == 0) {
-    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
-  } else {
-    CpuState->x64.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+  if (!mSmmCpuFeaturesSmmRelocated) {
+    //
+    // Configure SMBASE.
+    //
+    CpuState = (QEMU_SMRAM_SAVE_STATE_MAP *)(UINTN)(
+                                                    SMM_DEFAULT_SMBASE +
+                                                    SMRAM_SAVE_STATE_MAP_OFFSET
+                                                    );
+    if ((CpuState->x86.SMMRevId & 0xFFFF) == 0) {
+      CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+    } else {
+      CpuState->x64.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+    }
   }
 
   //
   // No need to program SMRRs on our virtual platform.
   //
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 8a426a4c10..6a281518f5 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -33,10 +33,14 @@
   MemoryAllocationLib
   PcdLib
   SafeIntLib
   SmmServicesTableLib
   UefiBootServicesTableLib
+  HobLib
+
+[Guids]
+  gSmmBaseHobGuid                ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
   gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
-- 
2.16.2.windows.1

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

On Wed, Jan 18, 2023 at 05:56:20PM +0800, Jiaxin Wu wrote:
> This patch is to avoid configure SMBASE if SmBase relocation has been
> done. If gSmmBaseHobGuid found, means SmBase info has been relocated
> and recorded in the SmBase array. No need to do the relocation in
> SmmCpuFeaturesInitializeProcessor().

Still not answered:  Who produces the gSmmBaseHobGuid?

If you intend to submit the producer code to edk2 please do so as part
of this patch series, so it is possible to see the whole picture instead
of only some pieces of the puzzle when reviewing the code.

If you don't submit the producer code it is pointless to touch OVMF.
You are only adding dead code.  That would be a rather questionable
decision though and add a big question mark to intels open source
commitment.

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Ni, Ray]

Gerd,
I think comments from patch #1 explains:
# The default SMBASE for the x86 processor is 0x30000. When SMI happens, CPU
+  runs the SMI handler at SMBASE+0x8000. Also, the SMM save state area is within
+  SMBASE+0x10000.
+
+  One of the SMM initialization from CPU perspective is to relocate and program
+  the new SMBASE (in TSEG range) for each CPU thread. When the SMBASE relocation
+  happens in a PEI module, the PEI module shall produce the SMM_BASE_HOB in HOB
+  database which tells the PiSmmCpuDxeSmm driver (which runs at a later phase)
+  about the new SMBASE for each CPU thread. PiSmmCpuDxeSmm driver installs the
+  SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Index.
+  When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall relocate and program
+  the new SMBASE itself.

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, January 18, 2023 8:20 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE
> configuration
> 
> On Wed, Jan 18, 2023 at 05:56:20PM +0800, Jiaxin Wu wrote:
> > This patch is to avoid configure SMBASE if SmBase relocation has been
> > done. If gSmmBaseHobGuid found, means SmBase info has been relocated
> > and recorded in the SmBase array. No need to do the relocation in
> > SmmCpuFeaturesInitializeProcessor().
> 
> Still not answered:  Who produces the gSmmBaseHobGuid?
> 
> If you intend to submit the producer code to edk2 please do so as part
> of this patch series, so it is possible to see the whole picture instead
> of only some pieces of the puzzle when reviewing the code.
> 
> If you don't submit the producer code it is pointless to touch OVMF.
> You are only adding dead code.  That would be a rather questionable
> decision though and add a big question mark to intels open source
> commitment.
> 
> take care,
>   Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

On Wed, Jan 18, 2023 at 02:37:11PM +0000, Ni, Ray wrote:
> Gerd,
> I think comments from patch #1 explains:

Not, it doesn't.  We are running in circles.  I keep asking about the
producer, you are answering that saying what the consumer should do.

So, you apparently want do SMM initialization earlier, in some PEI
module.  OK.  Open questions:

 * Why do you want do it in PEI instead?  The cover letter claims this
   improves boot performance.  I don't buy that.  You are not skipping
   the relocation process, you are just doing it somewhere else.
 * Where is the code?
 * It is totally unclear whenever it is possible and/or useful to
   initialize SMM that way on OVMF.

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

> 
>  * Why do you want do it in PEI instead?  The cover letter claims this
>    improves boot performance.  I don't buy that.  You are not skipping
>    the relocation process, you are just doing it somewhere else.

I clarified why it can improves boot performance, it's not just do somewhere else, but also each cpus smbase can be programmed in parallel. See [PATCH v3 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info:

"> With SMM Base Hob, PiSmmCpuDxeSmm does not need the RSM
> instruction to reload the SMBASE register with the new allocated
> SMBASE each time when it exits SMM. SMBASE Register for each

See existing code smm cpu driver implementation:  
        //
        // Hook return after RSM to set SMM re-based flag
        //
        SemaphoreHook (Index, &mRebased[Index]);

With above Semaphore hook, SMM CPU init for each processor must do one by one to avoid the CPUs over-writing each other's SMM Save State Area. As you mentioned, if system has thousands of cores, early smm init in parallel has the advantage over existing serial lines.   

> processors have already been programmed and all SMBASE address
> have recorded in SMM Base Hob. So the same default SMBASE Address
> (0x30000) will not be used, thus the CPUs over-writing each
> other's SMM Save State Area will not happen in PiSmmCpuDxeSmm
> driver. This way makes the first SMI init can be executed in
> parallel and save boot time on multi-core system."


>  * Where is the code?
See the design of existing function: SemaphoreHook (Index, &mRebased[Index]); 

>  * It is totally unclear whenever it is possible and/or useful to
>    initialize SMM that way on OVMF.
> 

Add the same handing logic code in OVMF is necessary to make sure it work. If someone produced such hob in OVMF platform, and OVMF also use the same Pi smm cpu driver, then it will be a problem. Changes in OVMF is make sure it runs into right way.

> take care,
>   Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

On Sun, Jan 29, 2023 at 05:47:00AM +0000, Wu, Jiaxin wrote:
> > 
> >  * Why do you want do it in PEI instead?  The cover letter claims this
> >    improves boot performance.  I don't buy that.  You are not skipping
> >    the relocation process, you are just doing it somewhere else.
> 
> I clarified why it can improves boot performance, it's not just do
> somewhere else, but also each cpus smbase can be programmed in
> parallel. See [PATCH v3 3/5] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM
> Base Hob for SmBase info:

Current state:

 * PiSmmCpuDxeSmm does SMM initialization, including programming SMBASE,
   serialized.

With this series applied:

 * Some PEI module programs SMBASE, serialized.
 * PiSmmCpuDxeSmm does SMM initialization, but NOT programming SMBASE,
   in parallel.

Sure PiSmmCpuDxeSmm alone will run faster because it can run parallel
now.

But the serialized SMBASE programming still happens, now in the PEI
module, and I don't see a good reason why the runtime the new PEI module
and the runtime of PiSmmCpuDxeSmm combined is faster than before.

> >  * Where is the code?
> See the design of existing function: SemaphoreHook (Index, &mRebased[Index]); 

I mean the PEI module code.

> >  * It is totally unclear whenever it is possible and/or useful to
> >    initialize SMM that way on OVMF.

> Add the same handing logic code in OVMF is necessary to make sure it
> work. If someone produced such hob in OVMF platform,

Do you intent submitting code for OVMF producing such a HOB?
There isn't any in this series.

> Changes in OVMF is make sure it runs into right way.

As it stands you are only adding dead code for no reason.

How is the SMM initialization of hotplugged CPUs
supposed to work with the new mode of operation?

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

> 
> Current state:
> 
>  * PiSmmCpuDxeSmm does SMM initialization, including programming
> SMBASE,
>    serialized.
> 

Yes. 

> With this series applied:
> 
>  * Some PEI module programs SMBASE, serialized.

PEI module can also programs SMBASE in parallel. It depends on the implementation. both is fine. 

>  * PiSmmCpuDxeSmm does SMM initialization, but NOT programming SMBASE,
>    in parallel.
> 

Yes.


> Sure PiSmmCpuDxeSmm alone will run faster because it can run parallel
> now.
> 
> But the serialized SMBASE programming still happens, now in the PEI
> module, and I don't see a good reason why the runtime the new PEI module
> and the runtime of PiSmmCpuDxeSmm combined is faster than before.

As I said, PEI module can also programs SMBASE in parallel, for example program the some register directly instead of depending the existing RSM instruction to reload the SMBASE register with the new allocated SMBASE each time when it exits SMM. Different vender might has different implementation.  Another benefit with this series will make the smbase relocation more independent & more simple compared with existing process in smm cpu driver. 

As you can see, this patch doesn't break existing code functionality& work flow, which means it's the compatible changes, which will bring the new approach for the pre-allocated smbase solution.

> 
> > >  * Where is the code?
> > See the design of existing function: SemaphoreHook (Index,
> &mRebased[Index]);
> 
> I mean the PEI module code.

Gerd, different user (hob producer) might have different implementation for the pre-allocated smbase in the hob. It's flexible for the producer to implement the pre-allocated smbase and make sure it has take effect in the system. PEI module code is not in this series.

> 
> > >  * It is totally unclear whenever it is possible and/or useful to
> > >    initialize SMM that way on OVMF.
> 
> > Add the same handing logic code in OVMF is necessary to make sure it
> > work. If someone produced such hob in OVMF platform,
> 
> Do you intent submitting code for OVMF producing such a HOB?
> There isn't any in this series.

No, we won't do that. 

> 
> > Changes in OVMF is make sure it runs into right way.
> 
> As it stands you are only adding dead code for no reason.
> 

I'm not looking for trouble for the OVMF part, as I also explained the OVMF patch: 

This patch is to avoid configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().

Change the SmmCpuFeaturesLib in OVMF is also follow the Laszlo suggestion, and I also think it's the right way instead of adding dead code. Laszlo is the OVMF 'D' for ovmf part, I'm fine with the decision if we want to remove it (but I still hold the opinion keep it even no producer for OVMF).  

> How is the SMM initialization of hotplugged CPUs
> supposed to work with the new mode of operation?
> 

Yes, that's already considered. For hot plugged CPU supported, the max number of CPUs should be defined in the PcdCpuMaxLogicalProcessorNumber, and all CPU Hot Plug Data is recorded in the PcdCpuHotPlugDataAddress, which contains the smbase info pre-allocated in the hob (filling the value in pi smm cpu driver), then CPU Hotplug MMI handler will relocate the SMBASE for new CPUs. 

> take care,
>   Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

  Hi,

> > But the serialized SMBASE programming still happens, now in the PEI
> > module, and I don't see a good reason why the runtime the new PEI module
> > and the runtime of PiSmmCpuDxeSmm combined is faster than before.
> 
> As I said, PEI module can also programs SMBASE in parallel, for
> example program the some register directly instead of depending the
> existing RSM instruction to reload the SMBASE register with the new
> allocated SMBASE each time when it exits SMM.

Ok.  So new Intel processors apparently got new MSR(s) to set SMBASE
directly.  Any specific reason why you don't add support for that to
PiSmmCpuDxeSmm?  That would avoid needing the new HOB (and the related
problems with the 8190 cpu limit) in the first place.

> Different vender might
> has different implementation.

Yes.

We can have different implementations in PiSmmCpuDxeSmm and/or
SmmCpuFeaturesLib to handle that.

> Another benefit with this series will make the smbase relocation more
> independent & more simple compared with existing process in smm cpu
> driver. 

Maybe it is.  Hard to justify from outside if you are not willing to
show the code of the PEI module.

> > Do you intent submitting code for OVMF producing such a HOB?
> > There isn't any in this series.
> 
> No, we won't do that. 

Then there is no point in changing the OVMF code,
other than maybe adding an ASSERT that there is no such HOB.

> > How is the SMM initialization of hotplugged CPUs
> > supposed to work with the new mode of operation?
> 
> Yes, that's already considered. For hot plugged CPU supported, the max
> number of CPUs should be defined in the
> PcdCpuMaxLogicalProcessorNumber, and all CPU Hot Plug Data is recorded
> in the PcdCpuHotPlugDataAddress, which contains the smbase info
> pre-allocated in the hob (filling the value in pi smm cpu driver),
> then CPU Hotplug MMI handler will relocate the SMBASE for new CPUs.

So that keeps the old workflow.

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Laszlo Ersek]

I'm going to comment on this one email up-stream, because it showcases
the community problem, as far as I'm concerned, and because Jiaxin made
a reference to my initial request.

On 2/2/23 10:00, Gerd Hoffmann wrote:
>   Hi,
> 
>>> But the serialized SMBASE programming still happens, now in the PEI
>>> module, and I don't see a good reason why the runtime the new PEI module
>>> and the runtime of PiSmmCpuDxeSmm combined is faster than before.
>>
>> As I said, PEI module can also programs SMBASE in parallel, for
>> example program the some register directly instead of depending the
>> existing RSM instruction to reload the SMBASE register with the new
>> allocated SMBASE each time when it exits SMM.
> 
> Ok.  So new Intel processors apparently got new MSR(s) to set SMBASE
> directly.  Any specific reason why you don't add support for that to
> PiSmmCpuDxeSmm?  That would avoid needing the new HOB (and the related
> problems with the 8190 cpu limit) in the first place.

See this is *exactly* my problem. The *whole work* on this should have
started like this, with a new Feature Request Bugzilla:

"Intel are introducing a new processor register (MSR or other method)
with their XY product line where firmware can program the SMBASE
independently of the RSM instruction. The PEI code performing this logic
will not be open sourced, similarly to other things that are kept
binary-only in the FSP (firmware support packages), and perhaps
similarly to how memory chips are initialized in the PEI phase too, by
"MRC" (memory reference code). Because there is no intent to open source
the initialization logic, possibly due to the new MSR not even being
slated for documentation in the Intel SDM, we need a new *binary-only*
interface."

*That* would have been honest, straight talk. Not this smoke-screen with
"another vendor might have a different method". That's entirely
speculative generality. Speculative generality has been an anti-pattern
in software development for decades, even merely for technical means,
but here the justification for it is not even technical: the language
around the generality is just to hide the one actual purpose of the
feature. Don't do that please. Describe your *specific* use case, list
your arguments, and then explain your approach for making it
regression-free for the existent cases.

PI and UEFI are all about binary interfaces between proprietary vendors.
As much as I disagree with the entire concept [*], I accept it as a fact
of life. I just ask that, whenever that pattern (= introducing ABIs,
rather than APIs) is exercised, at least the *actual goal* be documented.

([*] I disagree with the concept for two reasons. One, ideological (no
further explanation needed). Two, practical. If you "standardize" an
interface when you have *one* application of it, that's always trouble.
The second application, if there's ever going to be a second one, will
almost surely not fit within the framework of the "standard". So it's
best to either open source all the implementations, or at least openly
document their *actual* interface needs. Clearly state that the initial
interface implementation is "work in progress". If there are multiple
(divergent) applications, *then* try to extract something common, either
from the open source code bases, or the clearly documented data
dependencies, and then codify that. UEFI is *hugely* harmed by the
proliferation of protocols, where every new feature needs to be
standardized, as soon as one implementation exists. These protocols then
get ossified and linger around for absolutely forever. I feel that it's
totally self-inflicted damage; it is the consequence of the proprietary
software development model -- effectively the unwillingness to share.)

>>> Do you intent submitting code for OVMF producing such a HOB?
>>> There isn't any in this series.
>>
>> No, we won't do that. 
> 
> Then there is no point in changing the OVMF code,
> other than maybe adding an ASSERT that there is no such HOB.

I agree.

My initial request was for *considering* OVMF's library instance.
"Considering" means evaluating, and modifying *if* necessary, and *as*
necessary. I could not perform this evaluation myself: initially, the
purpose of the new interface was obscure, so I couldn't tell if OVMF was
affected or not.

Now that we *know* that OVMF is unaffected, we should just use library
instances for what library instances exist for: customization (platform
or otherwise). Because OVMF will not contain a PEI module initializing
the SMBASE regs as described, there is no need for adding (dead) code to
OVMF's lib instance. Adding the ASSERT() *definitely* makes sense
however: it's a statement that we have *considered* the applicability of
the feature. That is *precisely* what I missed from the initial
announcement.

Look, when you see a summary diffstat for a patch set that modifies N
instances of a library class, but not the one in OVMF -- or in any one
particular subsystem --, you ask yourself: "is this an oversight, or is
this intentional? did they just miss or ignore that platform or
subsystem, or did they evaluate it as unaffected"?

*That* is exactly what should have been in the original BZ or cover
letter. "I've grepped the edk2 and edk2-platforms tree for all instances
of this library class; I need to modify instances X, Y, Z; I *know* I
*need not* modify P, Q; and I'm *unsure* about U, V and W. Please advise".

The ASSERT() will give us a good opportunity to write a comment and/or
commit message around this.

This whole ordeal is just another piece of evidence for why I cannot
remain a member of this community. Hiding information, as a *basic modus
operandi*, is incompatible with open source development.

It's *fine* if a project or a contributor adopts a "my way or the high
way" attitude. Throwing open source code over the wall is still open
source. It's not open development any more, but still open source -- it
is still valuable (though less so). There may be many *valid* reasons
for doing open source, but not open development. What pains me is the
dishonest or at least mixed / sloppy messaging about *what edk2 is*. Is
it open source, or is it open development?

https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are

Laszlo

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

  Hi,

> Hiding information, as a *basic modus operandi*,
> is incompatible with open source development.

On point.
I want that printed on a t-shirt.

> What pains me is the dishonest or at least mixed / sloppy messaging
> about *what edk2 is*. Is it open source, or is it open development?

I have the same question.

Having to mail back and forth for weeks for (still incomplete!)
information on why you want change the smbase workflow is clearly
not "open development".

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

Restatement here: we don't want to hide something, it's not my intention to do that (apologize if give you such impress). I appeal to everyone in community can have more *inclusion* to every patch contributor. 

Please don't bring any aggressive or suggestive words to comment someone or patch. Not everyone in the same mind, but we can become more inclusion, let's trust everyone in the community can be contribute to their fullest potential and deliver their best work. We welcome the differences, knowing it makes us better. 

Again, thanks all great comments!

Thanks,
Jiaxin 


> 
> > Hiding information, as a *basic modus operandi*,
> > is incompatible with open source development.
> 
> On point.
> I want that printed on a t-shirt.
> 
> > What pains me is the dishonest or at least mixed / sloppy messaging
> > about *what edk2 is*. Is it open source, or is it open development?
> 
> I have the same question.
> 
> Having to mail back and forth for weeks for (still incomplete!)
> information on why you want change the smbase workflow is clearly
> not "open development".
> 
> take care,
>   Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

Hi Laszlo,

See below my feedback.

> 
> See this is *exactly* my problem. The *whole work* on this should have
> started like this, with a new Feature Request Bugzilla:
> 
> "Intel are introducing a new processor register (MSR or other method)
> with their XY product line where firmware can program the SMBASE
> independently of the RSM instruction. The PEI code performing this logic
> will not be open sourced, similarly to other things that are kept
> binary-only in the FSP (firmware support packages), and perhaps
> similarly to how memory chips are initialized in the PEI phase too, by
> "MRC" (memory reference code). Because there is no intent to open source
> the initialization logic, possibly due to the new MSR not even being
> slated for documentation in the Intel SDM, we need a new *binary-only*
> interface."
> 

This is about the edk2 open source working process, I agree Bugzilla is one of good approach for new feature info. I though Bugzilla REF is optional item in the patch if we can document the details in the Commit message.

If Bugzilla for each patch series is the mandatory item, let me know, I apologize for that missing.

> *That* would have been honest, straight talk. Not this smoke-screen with
> "another vendor might have a different method". That's entirely
> speculative generality. Speculative generality has been an anti-pattern
> in software development for decades, even merely for technical means,
> but here the justification for it is not even technical: the language
> around the generality is just to hide the one actual purpose of the
> feature. Don't do that please. Describe your *specific* use case, list
> your arguments, and then explain your approach for making it
> regression-free for the existent cases.
> 

I don't agree it's the "smoke-screen" or " speculative generality" with "...another vendor might have a different method...". below is my *original words*, it's not only that one in short words:

"As I said, PEI module can also programs SMBASE in parallel, for example program the some register directly instead of depending the existing RSM instruction to reload the SMBASE register with the new allocated SMBASE each time when it exits SMM. Different vender might has different implementation.  Another benefit with this series will make the smbase relocation more independent & more simple compared with existing process in smm cpu driver. 
As you can see, this patch doesn't break existing code functionality& work flow, which means it's the compatible changes, which will bring the new approach for the pre-allocated smbase solution."

For pre-allocated smbase solution ---> Different vender might has different implementation --> It's flexible for the producer to implement the pre-allocated smbase and make sure it has taken effect in the system. PEI module (to do the pre-allocated smbase) code is not in this series.

I agree Laszlo show me the more precise words than me in above (appreciate that), it's more easier for anyone in community to understand why I don't want to open source the hob producer. I'm willing adopt the Laszlo's comments to continue refine the patch series. But my attitude to the open source --  it's not the words like "smoke-screen" or "speculative generality".  You can say I'm unprofessional to handle such case, I agree I'm not aware so much design detail need exposed. But it's not intended to do that.


I apologize for the first version patch not clearly document the hob usage. But from the patch series v2 (https://edk2.groups.io/g/devel/message/98484), most of info are included for the hob, no matter in the commit message or in the hob .h file (https://edk2.groups.io/g/devel/message/98485) and I even explained the benefit of pismmcpu driver about the smm init (https://edk2.groups.io/g/devel/message/98486). I agree the explaining might be not enough for all of us understanding the design, but I flatter myself I have a positive attitude towards any opinion for anyone. With all your comments, I can make it more better. Thanks all comments from Laszlo & Gerd & Ray.  


> PI and UEFI are all about binary interfaces between proprietary vendors.
> As much as I disagree with the entire concept [*], I accept it as a fact
> of life. I just ask that, whenever that pattern (= introducing ABIs,
> rather than APIs) is exercised, at least the *actual goal* be documented.
> 
> ([*] I disagree with the concept for two reasons. One, ideological (no
> further explanation needed). Two, practical. If you "standardize" an
> interface when you have *one* application of it, that's always trouble.
> The second application, if there's ever going to be a second one, will
> almost surely not fit within the framework of the "standard". So it's
> best to either open source all the implementations, or at least openly
> document their *actual* interface needs. Clearly state that the initial
> interface implementation is "work in progress". If there are multiple
> (divergent) applications, *then* try to extract something common, either
> from the open source code bases, or the clearly documented data
> dependencies, and then codify that. UEFI is *hugely* harmed by the
> proliferation of protocols, where every new feature needs to be
> standardized, as soon as one implementation exists. These protocols then
> get ossified and linger around for absolutely forever. I feel that it's
> totally self-inflicted damage; it is the consequence of the proprietary
> software development model -- effectively the unwillingness to share.)
> 

This is really good comments impress me.


> >>> Do you intent submitting code for OVMF producing such a HOB?
> >>> There isn't any in this series.
> >>
> >> No, we won't do that.
> >
> > Then there is no point in changing the OVMF code,
> > other than maybe adding an ASSERT that there is no such HOB.
> 
> I agree.
> 
> My initial request was for *considering* OVMF's library instance.
> "Considering" means evaluating, and modifying *if* necessary, and *as*
> necessary. I could not perform this evaluation myself: initially, the
> purpose of the new interface was obscure, so I couldn't tell if OVMF was
> affected or not.
> 
> Now that we *know* that OVMF is unaffected, we should just use library
> instances for what library instances exist for: customization (platform
> or otherwise). Because OVMF will not contain a PEI module initializing
> the SMBASE regs as described, there is no need for adding (dead) code to
> OVMF's lib instance. Adding the ASSERT() *definitely* makes sense
> however: it's a statement that we have *considered* the applicability of
> the feature. That is *precisely* what I missed from the initial
> announcement.
> 
> Look, when you see a summary diffstat for a patch set that modifies N
> instances of a library class, but not the one in OVMF -- or in any one
> particular subsystem --, you ask yourself: "is this an oversight, or is
> this intentional? did they just miss or ignore that platform or
> subsystem, or did they evaluate it as unaffected"?
> 
> *That* is exactly what should have been in the original BZ or cover
> letter. "I've grepped the edk2 and edk2-platforms tree for all instances
> of this library class; I need to modify instances X, Y, Z; I *know* I
> *need not* modify P, Q; and I'm *unsure* about U, V and W. Please advise".
> 
> The ASSERT() will give us a good opportunity to write a comment and/or
> commit message around this.
> 

ASSERT is fine to me by replacing the "if condition" to handle that. we can discuss the patch for correct changes in OVMF. 



> This whole ordeal is just another piece of evidence for why I cannot
> remain a member of this community. Hiding information, as a *basic modus
> operandi*, is incompatible with open source development.
> 
> It's *fine* if a project or a contributor adopts a "my way or the high
> way" attitude. Throwing open source code over the wall is still open
> source. It's not open development any more, but still open source -- it
> is still valuable (though less so). There may be many *valid* reasons
> for doing open source, but not open development. What pains me is the
> dishonest or at least mixed / sloppy messaging about *what edk2 is*. Is
> it open source, or is it open development?
> 
> https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are
> 

Thanks,
Jiaxin

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Ni, Ray]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, February 2, 2023 7:47 PM
> To: Gerd Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE
> configuration
> 
> I'm going to comment on this one email up-stream, because it showcases
> the community problem, as far as I'm concerned, and because Jiaxin made
> a reference to my initial request.
> 
> On 2/2/23 10:00, Gerd Hoffmann wrote:
> >   Hi,
> >
> >>> But the serialized SMBASE programming still happens, now in the PEI
> >>> module, and I don't see a good reason why the runtime the new PEI module
> >>> and the runtime of PiSmmCpuDxeSmm combined is faster than before.
> >>
> >> As I said, PEI module can also programs SMBASE in parallel, for
> >> example program the some register directly instead of depending the
> >> existing RSM instruction to reload the SMBASE register with the new
> >> allocated SMBASE each time when it exits SMM.
> >
> > Ok.  So new Intel processors apparently got new MSR(s) to set SMBASE
> > directly.  Any specific reason why you don't add support for that to
> > PiSmmCpuDxeSmm?  That would avoid needing the new HOB (and the related
> > problems with the 8190 cpu limit) in the first place.

The reason is the hardware interface to set SMBASE is in thread scope meaning
such firmware-hardware communication needs to be done for each CPU thread.

It's doable to program the hardware interface using DXE MP service protocol in
CpuSmm driver's entry point.
But, considering the standalone MM environment where the CpuMm driver runs
in a isolated environment and it cannot invoke any DXE or PEI MP service, you could
understand that why we choose to put the hardware interface programming in a separate
PEI module. This is the major reason.
I admit that a minor benefit of this design is we can isolate the private hardware interface
programming in a close-source module. Otherwise, the SmmCpuFeaturesLib might need to
expose a new API for the hardware interface programming.


> 
> See this is *exactly* my problem. The *whole work* on this should have
> started like this, with a new Feature Request Bugzilla:
> 
> "Intel are introducing a new processor register (MSR or other method)
> with their XY product line where firmware can program the SMBASE
> independently of the RSM instruction. The PEI code performing this logic
> will not be open sourced, similarly to other things that are kept
> binary-only in the FSP (firmware support packages), and perhaps
> similarly to how memory chips are initialized in the PEI phase too, by
> "MRC" (memory reference code). Because there is no intent to open source
> the initialization logic, possibly due to the new MSR not even being
> slated for documentation in the Intel SDM, we need a new *binary-only*
> interface."

Fair point. We will submit a Bugzilla for this.

> 
> *That* would have been honest, straight talk. Not this smoke-screen with
> "another vendor might have a different method". That's entirely
> speculative generality. Speculative generality has been an anti-pattern
> in software development for decades, even merely for technical means,
> but here the justification for it is not even technical: the language
> around the generality is just to hide the one actual purpose of the
> feature. Don't do that please. Describe your *specific* use case, list
> your arguments, and then explain your approach for making it
> regression-free for the existent cases.

Agree. Be specific and honest. Tell what we can tell and explain what we
cannot tell.

> 
> PI and UEFI are all about binary interfaces between proprietary vendors.
> As much as I disagree with the entire concept [*], I accept it as a fact
> of life. I just ask that, whenever that pattern (= introducing ABIs,
> rather than APIs) is exercised, at least the *actual goal* be documented.
> 
> ([*] I disagree with the concept for two reasons. One, ideological (no
> further explanation needed). Two, practical. If you "standardize" an
> interface when you have *one* application of it, that's always trouble.
> The second application, if there's ever going to be a second one, will
> almost surely not fit within the framework of the "standard". So it's
> best to either open source all the implementations, or at least openly
> document their *actual* interface needs. Clearly state that the initial
> interface implementation is "work in progress". If there are multiple
> (divergent) applications, *then* try to extract something common, either
> from the open source code bases, or the clearly documented data
> dependencies, and then codify that. UEFI is *hugely* harmed by the
> proliferation of protocols, where every new feature needs to be
> standardized, as soon as one implementation exists. These protocols then
> get ossified and linger around for absolutely forever. I feel that it's
> totally self-inflicted damage; it is the consequence of the proprietary
> software development model -- effectively the unwillingness to share.)

When we designed the flow for the new feature, we examined the design by
thinking about if current SMBASE relocation done in PiSmmCpu driver can fit
in the new design. Our conclusion is yes though we don’t have a plan to do it
meaning we don't want to separate today's SMBASE relocation logic in a separate
driver then produce a HOB.
I hate introducing new interfaces as well. A better design should not only cover
today's use case but also future's use case (5-10 years maybe, no guarantee of forever).
Sometimes the designer might invent some interfaces not flexible enough for satisfying
the future different instances of the similar use cases, that's a bad design in my opinion
but we cannot avoid that. I think that triggered the code-first process which requires
implementation first and standardization second. 
Though this new HOB is not in PI spec, you remind me that we might need to add more fields
to the HOB so a way to distinguish between different versions of the HOB should be considered.
The way could be to introduce a new GUID for new version of HOB, or add a field (version?) in the HOB.
I prefer the second.

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

  Hi,

> > > Ok.  So new Intel processors apparently got new MSR(s) to set SMBASE
> > > directly.  Any specific reason why you don't add support for that to
> > > PiSmmCpuDxeSmm?  That would avoid needing the new HOB (and the related
> > > problems with the 8190 cpu limit) in the first place.
> 
> The reason is the hardware interface to set SMBASE is in thread scope meaning
> such firmware-hardware communication needs to be done for each CPU thread.
> 
> It's doable to program the hardware interface using DXE MP service protocol in
> CpuSmm driver's entry point.
> But, considering the standalone MM environment where the CpuMm driver runs
> in a isolated environment and it cannot invoke any DXE or PEI MP service, you could
> understand that why we choose to put the hardware interface programming in a separate
> PEI module. This is the major reason.

Ok, *that* finally makes sense to me.  Can you please add a source code
comment explaining this to the patch series?  Patch #1 (which adds the
interface) is the best place I think.

> I admit that a minor benefit of this design is we can isolate the
> private hardware interface programming in a close-source module.
> Otherwise, the SmmCpuFeaturesLib might need to expose a new API for
> the hardware interface programming.

"benefit" and "closed-source" in one sentence while discussion patches
for an open source project.

And you are wondering (see parallel mail by Jiaxin) why outsiders get
the impression you are trying to hide information.

No further questions.

> Though this new HOB is not in PI spec, you remind me that we might
> need to add more fields to the HOB so a way to distinguish between
> different versions of the HOB should be considered.  The way could be
> to introduce a new GUID for new version of HOB, or add a field
> (version?) in the HOB.  I prefer the second.

Established practice is to use a new GUID.  We should stick to that.

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Ni, Ray]

> >
> > It's doable to program the hardware interface using DXE MP service
> protocol in
> > CpuSmm driver's entry point.
> > But, considering the standalone MM environment where the CpuMm
> driver runs
> > in a isolated environment and it cannot invoke any DXE or PEI MP service,
> you could
> > understand that why we choose to put the hardware interface
> programming in a separate
> > PEI module. This is the major reason.
> 
> Ok, *that* finally makes sense to me.  Can you please add a source code
> comment explaining this to the patch series?  Patch #1 (which adds the
> interface) is the best place I think.

Sure. Jiaxin, please.


> 
> > I admit that a minor benefit of this design is we can isolate the
> > private hardware interface programming in a close-source module.
> > Otherwise, the SmmCpuFeaturesLib might need to expose a new API for
> > the hardware interface programming.
> 
> "benefit" and "closed-source" in one sentence while discussion patches
> for an open source project.
> 
> And you are wondering (see parallel mail by Jiaxin) why outsiders get
> the impression you are trying to hide information.
> 
> No further questions.

Gerd, the benefit is to have a better modular design (separate PEIM instead of
extending existing SmmCpuFeaturesLib), NOT "close-source" module.
I don't have the power to argue with you why not open source the PEIM. Sorry:(
I like open source world and the open-discussions here. It's the open-discussions
that help to produce better design/code.
Please don't imagine that "I" want to hide something. If I cannot tell you something,
that's because the information cannot be public for now required by
the company policy. Maybe people like you working on all open-source code
cannot understand the difficulty of mixing open source and close source code.

> 
> > Though this new HOB is not in PI spec, you remind me that we might
> > need to add more fields to the HOB so a way to distinguish between
> > different versions of the HOB should be considered.  The way could be
> > to introduce a new GUID for new version of HOB, or add a field
> > (version?) in the HOB.  I prefer the second.
> 
> Established practice is to use a new GUID.  We should stick to that.
No concern from my side to have a new GUID once the HOB format changes.

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

  Hi,

> Please don't imagine that "I" want to hide something. If I cannot tell you something,
> that's because the information cannot be public for now required by
> the company policy.

I fully understand that it not your personal choice but company policy.

Just explicitly say so -- ideally right from the start -- when this is
the case instead of leaving others in the void would make discussions
alot easier.

So, to summarize:  Intel processors have some new way to set SMBASE
which does not require entering SMM mode.  It requires running code on
each CPU thread though, which is incompatible with StandaloneMM.  So it
needs to be done before setting up StandaloneMM and you choose to do it
in a PEI module.  More details can not be shared per intel company
policy.  The PEIM is closed source.

Would that be known right from start the discussion on the OVMF changes
would have been much shorter and much less frustrating.  That
information makes pretty clear that it is not relevant at all for OVMF.
Even in case the PEIM gets released as open source in the future it is
not relevant.  What matters for OVMF is what qemu and kvm support (which
is why it has its own SmmCpuFeaturesLib variant in the first place).

take care,
  Gerd

Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

> > >
> > > It's doable to program the hardware interface using DXE MP service
> > protocol in
> > > CpuSmm driver's entry point.
> > > But, considering the standalone MM environment where the CpuMm
> > driver runs
> > > in a isolated environment and it cannot invoke any DXE or PEI MP service,
> > you could
> > > understand that why we choose to put the hardware interface
> > programming in a separate
> > > PEI module. This is the major reason.
> >
> > Ok, *that* finally makes sense to me.  Can you please add a source code
> > comment explaining this to the patch series?  Patch #1 (which adds the
> > interface) is the best place I think.
> 
> Sure. Jiaxin, please.
> 

Ok, I will follow this suggestion.

> 
> >
> > > I admit that a minor benefit of this design is we can isolate the
> > > private hardware interface programming in a close-source module.
> > > Otherwise, the SmmCpuFeaturesLib might need to expose a new API for
> > > the hardware interface programming.
> >
> > "benefit" and "closed-source" in one sentence while discussion patches
> > for an open source project.
> >
> > And you are wondering (see parallel mail by Jiaxin) why outsiders get
> > the impression you are trying to hide information.
> >
> > No further questions.
> 
> Gerd, the benefit is to have a better modular design (separate PEIM instead of
> extending existing SmmCpuFeaturesLib), NOT "close-source" module.
> I don't have the power to argue with you why not open source the PEIM. Sorry:(
> I like open source world and the open-discussions here. It's the open-discussions
> that help to produce better design/code.
> Please don't imagine that "I" want to hide something. If I cannot tell you
> something,
> that's because the information cannot be public for now required by
> the company policy. Maybe people like you working on all open-source code
> cannot understand the difficulty of mixing open source and close source code.
> 
> >
> > > Though this new HOB is not in PI spec, you remind me that we might
> > > need to add more fields to the HOB so a way to distinguish between
> > > different versions of the HOB should be considered.  The way could be
> > > to introduce a new GUID for new version of HOB, or add a field
> > > (version?) in the HOB.  I prefer the second.
> >
> > Established practice is to use a new GUID.  We should stick to that.
> No concern from my side to have a new GUID once the HOB format changes.

Agree, it's fine to have a new GUID.

Re: [edk2-devel] [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

Hi Laszlo & Gerd,

>  SmmCpuFeaturesLibConstructor (
>    IN EFI_HANDLE        ImageHandle,
>    IN EFI_SYSTEM_TABLE  *SystemTable
>    )
>  {
> +  //
> +  // If gSmmBaseHobGuid found, means SmBase info has been relocated and
> recorded
> +  // in the SmBase array.
> +  //
> +  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL);
> +

Do you agree just assert mSmmCpuFeaturesSmmRelocated is the false for OVMF?

Thanks,
Jiaxin
 

Re: [edk2-devel] [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Gerd Hoffmann]

On Fri, Feb 03, 2023 at 08:59:06AM +0000, Wu, Jiaxin wrote:
> Hi Laszlo & Gerd,
> 
> >  SmmCpuFeaturesLibConstructor (
> >    IN EFI_HANDLE        ImageHandle,
> >    IN EFI_SYSTEM_TABLE  *SystemTable
> >    )
> >  {
> > +  //
> > +  // If gSmmBaseHobGuid found, means SmBase info has been relocated and
> > recorded
> > +  // in the SmBase array.
> > +  //
> > +  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> > (&gSmmBaseHobGuid) != NULL);
> > +
> 
> Do you agree just assert mSmmCpuFeaturesSmmRelocated is the false for OVMF?

Skip the indirection, just "ASSERT(GetFirstGuidHob(...) == NULL)".

take care,
  Gerd

Re: [edk2-devel] [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

[Wu, Jiaxin]

> >
> > Do you agree just assert mSmmCpuFeaturesSmmRelocated is the false for
> OVMF?
> 
> Skip the indirection, just "ASSERT(GetFirstGuidHob(...) == NULL)".
> 

Yes, sure, thanks.

> take care,
>   Gerd
> 
> 
> 
> 
>