Re: [edk2] [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.

2018-10-16 Thread Dong, Eric 
Hi Ruiyu,

> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, October 16, 2018 11:16 AM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Laszlo Ersek 
> Subject: Re: [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to
> support semaphore type.
> 
> On 10/15/2018 10:49 AM, Eric Dong wrote:
> > Because this driver needs to set MSRs saved in normal boot phase, sync
> > semaphore logic from RegisterCpuFeaturesLib code which used for normal
> boot phase.
> >
> > Detail see change SHA-1: dcdf1774212d87e2d7feb36286a408ea7475fd7b for
> > RegisterCpuFeaturesLib.
> >
> > Cc: Ruiyu Ni 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong 
> > ---
> >   UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c  | 316 -
> 
> >   UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |   3 -
> >   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   3 +-
> >   3 files changed, 180 insertions(+), 142 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 52ff9679d5..5a35f7a634 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > @@ -38,9 +38,12 @@ typedef struct {
> >   } MP_ASSEMBLY_ADDRESS_MAP;
> >
> >   //
> > -// Spin lock used to serialize MemoryMapped operation
> > +// Flags used when program the register.
> >   //
> > -SPIN_LOCK*mMemoryMappedLock = NULL;
> > +typedef struct {
> > +  volatile UINTN   MemoryMappedLock; // Spinlock used to 
> > program
> mmio
> > +  volatile UINT32  *SemaphoreCount;  // Semaphore used to
> program semaphore.
> > +} PROGRAM_CPU_REGISTER_FLAGS;
> >
> >   //
> >   // Signal that SMM BASE relocation is complete.
> > @@ -62,13 +65,11 @@ AsmGetAddressMap (
> >   #define LEGACY_REGION_SIZE(2 * 0x1000)
> >   #define LEGACY_REGION_BASE(0xA - LEGACY_REGION_SIZE)
> >
> > +PROGRAM_CPU_REGISTER_FLAGS   mCpuFlags;
> >   ACPI_CPU_DATAmAcpiCpuData;
> >   volatile UINT32  mNumberToFinish;
> >   MP_CPU_EXCHANGE_INFO *mExchangeInfo;
> >   BOOLEAN  mRestoreSmmConfigurationInS3 = FALSE;
> > -MP_MSR_LOCK  *mMsrSpinLocks = NULL;
> > -UINTNmMsrSpinLockCount;
> > -UINTNmMsrCount = 0;
> >
> >   //
> >   // S3 boot flag
> > @@ -91,89 +92,6 @@ UINT8mApHltLoopCodeTemplate[] = {
> >  0xEB, 0xFC   // jmp $-2
> >  };
> >
> > -/**
> > -  Get MSR spin lock by MSR index.
> > -
> > -  @param  MsrIndex   MSR index value.
> > -
> > -  @return Pointer to MSR spin lock.
> > -
> > -**/
> > -SPIN_LOCK *
> > -GetMsrSpinLockByIndex (
> > -  IN UINT32  MsrIndex
> > -  )
> > -{
> > -  UINTN Index;
> > -  for (Index = 0; Index < mMsrCount; Index++) {
> > -if (MsrIndex == mMsrSpinLocks[Index].MsrIndex) {
> > -  return mMsrSpinLocks[Index].SpinLock;
> > -}
> > -  }
> > -  return NULL;
> > -}
> > -
> > -/**
> > -  Initialize MSR spin lock by MSR index.
> > -
> > -  @param  MsrIndex   MSR index value.
> > -
> > -**/
> > -VOID
> > -InitMsrSpinLockByIndex (
> > -  IN UINT32  MsrIndex
> > -  )
> > -{
> > -  UINTNMsrSpinLockCount;
> > -  UINTNNewMsrSpinLockCount;
> > -  UINTNIndex;
> > -  UINTNAddedSize;
> > -
> > -  if (mMsrSpinLocks == NULL) {
> > -MsrSpinLockCount =
> mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter;
> > -mMsrSpinLocks = (MP_MSR_LOCK *) AllocatePool (sizeof
> (MP_MSR_LOCK) * MsrSpinLockCount);
> > -ASSERT (mMsrSpinLocks != NULL);
> > -for (Index = 0; Index < MsrSpinLockCount; Index++) {
> > -  mMsrSpinLocks[Index].SpinLock =
> > -   (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr +
> Index * mSemaphoreSize);
> > -  mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
> > -}
> > -mMsrSpinLockCount = MsrSpinLockCount;
> > -mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter = 0;
> > -  }
> > -  if (GetMsrSpinLockByIndex (MsrIndex) == NULL) {
> > -//
> > -// Initialize spin lock for MSR programming
> > -//
> > -mMsrSpinLocks[mMsrCount].MsrIndex = MsrIndex;
> > -InitializeSpinLock (mMsrSpinLocks[mMsrCount].SpinLock);
> > -mMsrCount ++;
> > -if (mMsrCount == mMsrSpinLockCount) {
> > -  //
> > -  // If MSR spin lock buffer is full, enlarge it
> > -  //
> > -  AddedSize = SIZE_4KB;
> > -  mSmmCpuSemaphores.SemaphoreMsr.Msr =
> > -AllocatePages (EFI_SIZE_TO_PAGES(AddedSize));
> > -  ASSERT (mSmmCpuSemaphores.SemaphoreMsr.Msr != NULL);
> > -  NewMsrSpinLockCount = mMsrSpinLockCount + AddedSize /
> mSemaphoreSize;
> > -  mMsrSpinLocks = ReallocatePool (
> > -sizeof (MP_MSR_LOCK) * mMsrSpinLockCount,
> > -sizeof (MP_MSR_LOCK) * NewMsrSpinLockCount,
> > -

Re: [edk2] [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.

2018-10-16 Thread Dong, Eric 
Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, October 16, 2018 1:13 AM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu 
> Subject: Re: [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support
> semaphore type.
> 
> On 10/15/18 04:49, Eric Dong wrote:
> > Because this driver needs to set MSRs saved in normal boot phase, sync
> > semaphore logic from RegisterCpuFeaturesLib code which used for normal
> boot phase.
> 
> (My review of this patch is going to be superficial. I'm not trying to 
> validate the
> actual algorithm. I'm mostly sanity-checking the code, and gauging whether it
> will break platforms that use CpuS3DataDxe.)
> 

Reasonable, thanks for your efforts.

> 
> > Detail see change SHA-1: dcdf1774212d87e2d7feb36286a408ea7475fd7b for
> > RegisterCpuFeaturesLib.
> 
> (1) I think it is valid to reference other patches in the same series.
> However, the commit hashes are not stable yet -- when you rebase the series,
> the commit hashes will change. Therefore, when we refer to a patch that is not
> upstream yet (i.e. it is part of the same series), it is best to spell out 
> the full
> subject, such as:
> 
> UefiCpuPkg/RegisterCpuFeaturesLib: Add logic to support semaphore type.
> 

I aware this value change when do the rebase action. I plan to update the value 
when I do the rebase action.  Your suggestion is good. I can also use the 
change header to specify the change. I will use it in my next change.

> 
> >
> > Cc: Ruiyu Ni 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong 
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c  | 316 ---
> --
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |   3 -
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   3 +-
> >  3 files changed, 180 insertions(+), 142 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 52ff9679d5..5a35f7a634 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > @@ -38,9 +38,12 @@ typedef struct {
> >  } MP_ASSEMBLY_ADDRESS_MAP;
> >
> >  //
> > -// Spin lock used to serialize MemoryMapped operation
> > +// Flags used when program the register.
> >  //
> > -SPIN_LOCK*mMemoryMappedLock = NULL;
> > +typedef struct {
> > +  volatile UINTN   MemoryMappedLock; // Spinlock used to 
> > program
> mmio
> > +  volatile UINT32  *SemaphoreCount;  // Semaphore used to 
> > program
> semaphore.
> > +} PROGRAM_CPU_REGISTER_FLAGS;
> >
> >  //
> >  // Signal that SMM BASE relocation is complete.
> > @@ -62,13 +65,11 @@ AsmGetAddressMap (
> >  #define LEGACY_REGION_SIZE(2 * 0x1000)
> >  #define LEGACY_REGION_BASE(0xA - LEGACY_REGION_SIZE)
> >
> > +PROGRAM_CPU_REGISTER_FLAGS   mCpuFlags;
> >  ACPI_CPU_DATAmAcpiCpuData;
> >  volatile UINT32  mNumberToFinish;
> >  MP_CPU_EXCHANGE_INFO *mExchangeInfo;
> >  BOOLEAN  mRestoreSmmConfigurationInS3 = FALSE;
> > -MP_MSR_LOCK  *mMsrSpinLocks = NULL;
> > -UINTNmMsrSpinLockCount;
> > -UINTNmMsrCount = 0;
> >
> >  //
> >  // S3 boot flag
> > @@ -91,89 +92,6 @@ UINT8mApHltLoopCodeTemplate[] = {
> > 0xEB, 0xFC   // jmp $-2
> > };
> >
> > -/**
> > -  Get MSR spin lock by MSR index.
> > -
> > -  @param  MsrIndex   MSR index value.
> > -
> > -  @return Pointer to MSR spin lock.
> > -
> > -**/
> > -SPIN_LOCK *
> > -GetMsrSpinLockByIndex (
> > -  IN UINT32  MsrIndex
> > -  )
> > -{
> > -  UINTN Index;
> > -  for (Index = 0; Index < mMsrCount; Index++) {
> > -if (MsrIndex == mMsrSpinLocks[Index].MsrIndex) {
> > -  return mMsrSpinLocks[Index].SpinLock;
> > -}
> > -  }
> > -  return NULL;
> > -}
> > -
> > -/**
> > -  Initialize MSR spin lock by MSR index.
> > -
> > -  @param  MsrIndex   MSR index value.
> > -
> > -**/
> > -VOID
> > -InitMsrSpinLockByIndex (
> > -  IN UINT32  MsrIndex
> > -  )
> > -{
> > -  UINTNMsrSpinLockCount;
> > -  UINTNNewMsrSpinLockCount;
> > -  UINTNIndex;
> > -  UINTNAddedSize;
> > -
> > -  if (mMsrSpinLocks == NULL) {
> > -MsrSpinLockCount =
> mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter;
> > -mMsrSpinLocks = (MP_MSR_LOCK *) AllocatePool (sizeof (MP_MSR_LOCK)
> * MsrSpinLockCount);
> > -ASSERT (mMsrSpinLocks != NULL);
> > -for (Index = 0; Index < MsrSpinLockCount; Index++) {
> > -  mMsrSpinLocks[Index].SpinLock =
> > -   (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr +
> Index * mSemaphoreSize);
> > -  mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
> > -}
> > -mMsrSpinLockCount = MsrSpinLockCount;
> > -mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter = 0;
> > -  }
> > -

Re: [edk2] [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.

2018-10-15 Thread Ni, Ruiyu 

On 10/15/2018 10:49 AM, Eric Dong wrote:

Because this driver needs to set MSRs saved in normal boot phase, sync semaphore
logic from RegisterCpuFeaturesLib code which used for normal boot phase.

Detail see change SHA-1: dcdf1774212d87e2d7feb36286a408ea7475fd7b for
RegisterCpuFeaturesLib.

Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c  | 316 -
  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |   3 -
  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   3 +-
  3 files changed, 180 insertions(+), 142 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 52ff9679d5..5a35f7a634 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -38,9 +38,12 @@ typedef struct {
  } MP_ASSEMBLY_ADDRESS_MAP;
  
  //

-// Spin lock used to serialize MemoryMapped operation
+// Flags used when program the register.
  //
-SPIN_LOCK*mMemoryMappedLock = NULL;
+typedef struct {
+  volatile UINTN   MemoryMappedLock; // Spinlock used to program 
mmio
+  volatile UINT32  *SemaphoreCount;  // Semaphore used to program 
semaphore.
+} PROGRAM_CPU_REGISTER_FLAGS;
  
  //

  // Signal that SMM BASE relocation is complete.
@@ -62,13 +65,11 @@ AsmGetAddressMap (
  #define LEGACY_REGION_SIZE(2 * 0x1000)
  #define LEGACY_REGION_BASE(0xA - LEGACY_REGION_SIZE)
  
+PROGRAM_CPU_REGISTER_FLAGS   mCpuFlags;

  ACPI_CPU_DATAmAcpiCpuData;
  volatile UINT32  mNumberToFinish;
  MP_CPU_EXCHANGE_INFO *mExchangeInfo;
  BOOLEAN  mRestoreSmmConfigurationInS3 = FALSE;
-MP_MSR_LOCK  *mMsrSpinLocks = NULL;
-UINTNmMsrSpinLockCount;
-UINTNmMsrCount = 0;
  
  //

  // S3 boot flag
@@ -91,89 +92,6 @@ UINT8mApHltLoopCodeTemplate[] = {
 0xEB, 0xFC   // jmp $-2
 };
  
-/**

-  Get MSR spin lock by MSR index.
-
-  @param  MsrIndex   MSR index value.
-
-  @return Pointer to MSR spin lock.
-
-**/
-SPIN_LOCK *
-GetMsrSpinLockByIndex (
-  IN UINT32  MsrIndex
-  )
-{
-  UINTN Index;
-  for (Index = 0; Index < mMsrCount; Index++) {
-if (MsrIndex == mMsrSpinLocks[Index].MsrIndex) {
-  return mMsrSpinLocks[Index].SpinLock;
-}
-  }
-  return NULL;
-}
-
-/**
-  Initialize MSR spin lock by MSR index.
-
-  @param  MsrIndex   MSR index value.
-
-**/
-VOID
-InitMsrSpinLockByIndex (
-  IN UINT32  MsrIndex
-  )
-{
-  UINTNMsrSpinLockCount;
-  UINTNNewMsrSpinLockCount;
-  UINTNIndex;
-  UINTNAddedSize;
-
-  if (mMsrSpinLocks == NULL) {
-MsrSpinLockCount = mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter;
-mMsrSpinLocks = (MP_MSR_LOCK *) AllocatePool (sizeof (MP_MSR_LOCK) * 
MsrSpinLockCount);
-ASSERT (mMsrSpinLocks != NULL);
-for (Index = 0; Index < MsrSpinLockCount; Index++) {
-  mMsrSpinLocks[Index].SpinLock =
-   (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr + Index * 
mSemaphoreSize);
-  mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
-}
-mMsrSpinLockCount = MsrSpinLockCount;
-mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter = 0;
-  }
-  if (GetMsrSpinLockByIndex (MsrIndex) == NULL) {
-//
-// Initialize spin lock for MSR programming
-//
-mMsrSpinLocks[mMsrCount].MsrIndex = MsrIndex;
-InitializeSpinLock (mMsrSpinLocks[mMsrCount].SpinLock);
-mMsrCount ++;
-if (mMsrCount == mMsrSpinLockCount) {
-  //
-  // If MSR spin lock buffer is full, enlarge it
-  //
-  AddedSize = SIZE_4KB;
-  mSmmCpuSemaphores.SemaphoreMsr.Msr =
-AllocatePages (EFI_SIZE_TO_PAGES(AddedSize));
-  ASSERT (mSmmCpuSemaphores.SemaphoreMsr.Msr != NULL);
-  NewMsrSpinLockCount = mMsrSpinLockCount + AddedSize / mSemaphoreSize;
-  mMsrSpinLocks = ReallocatePool (
-sizeof (MP_MSR_LOCK) * mMsrSpinLockCount,
-sizeof (MP_MSR_LOCK) * NewMsrSpinLockCount,
-mMsrSpinLocks
-);
-  ASSERT (mMsrSpinLocks != NULL);
-  mMsrSpinLockCount = NewMsrSpinLockCount;
-  for (Index = mMsrCount; Index < mMsrSpinLockCount; Index++) {
-mMsrSpinLocks[Index].SpinLock =
- (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr +
- (Index - mMsrCount)  * mSemaphoreSize);
-mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
-  }
-}
-  }
-}
-
  /**
Sync up the MTRR values for all processors.
  
@@ -204,42 +122,89 @@ Returns:

  }
  
  /**

-  Programs registers for the calling processor.
+  Increment semaphore by 1.
  
-  This function programs registers for the calling processor.

+  @param  SemIN:  32-bit unsigned

Re: [edk2] [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.

2018-10-15 Thread Laszlo Ersek 
On 10/15/18 04:49, Eric Dong wrote:
> Because this driver needs to set MSRs saved in normal boot phase, sync 
> semaphore
> logic from RegisterCpuFeaturesLib code which used for normal boot phase.

(My review of this patch is going to be superficial. I'm not trying to
validate the actual algorithm. I'm mostly sanity-checking the code, and
gauging whether it will break platforms that use CpuS3DataDxe.)


> Detail see change SHA-1: dcdf1774212d87e2d7feb36286a408ea7475fd7b for
> RegisterCpuFeaturesLib.

(1) I think it is valid to reference other patches in the same series.
However, the commit hashes are not stable yet -- when you rebase the
series, the commit hashes will change. Therefore, when we refer to a
patch that is not upstream yet (i.e. it is part of the same series), it
is best to spell out the full subject, such as:

UefiCpuPkg/RegisterCpuFeaturesLib: Add logic to support semaphore type.


> 
> Cc: Ruiyu Ni 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c  | 316 
> -
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |   3 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   3 +-
>  3 files changed, 180 insertions(+), 142 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 52ff9679d5..5a35f7a634 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -38,9 +38,12 @@ typedef struct {
>  } MP_ASSEMBLY_ADDRESS_MAP;
>  
>  //
> -// Spin lock used to serialize MemoryMapped operation
> +// Flags used when program the register.
>  //
> -SPIN_LOCK*mMemoryMappedLock = NULL;
> +typedef struct {
> +  volatile UINTN   MemoryMappedLock; // Spinlock used to program 
> mmio
> +  volatile UINT32  *SemaphoreCount;  // Semaphore used to 
> program semaphore.
> +} PROGRAM_CPU_REGISTER_FLAGS;
>  
>  //
>  // Signal that SMM BASE relocation is complete.
> @@ -62,13 +65,11 @@ AsmGetAddressMap (
>  #define LEGACY_REGION_SIZE(2 * 0x1000)
>  #define LEGACY_REGION_BASE(0xA - LEGACY_REGION_SIZE)
>  
> +PROGRAM_CPU_REGISTER_FLAGS   mCpuFlags;
>  ACPI_CPU_DATAmAcpiCpuData;
>  volatile UINT32  mNumberToFinish;
>  MP_CPU_EXCHANGE_INFO *mExchangeInfo;
>  BOOLEAN  mRestoreSmmConfigurationInS3 = FALSE;
> -MP_MSR_LOCK  *mMsrSpinLocks = NULL;
> -UINTNmMsrSpinLockCount;
> -UINTNmMsrCount = 0;
>  
>  //
>  // S3 boot flag
> @@ -91,89 +92,6 @@ UINT8mApHltLoopCodeTemplate[] = {
> 0xEB, 0xFC   // jmp $-2
> };
>  
> -/**
> -  Get MSR spin lock by MSR index.
> -
> -  @param  MsrIndex   MSR index value.
> -
> -  @return Pointer to MSR spin lock.
> -
> -**/
> -SPIN_LOCK *
> -GetMsrSpinLockByIndex (
> -  IN UINT32  MsrIndex
> -  )
> -{
> -  UINTN Index;
> -  for (Index = 0; Index < mMsrCount; Index++) {
> -if (MsrIndex == mMsrSpinLocks[Index].MsrIndex) {
> -  return mMsrSpinLocks[Index].SpinLock;
> -}
> -  }
> -  return NULL;
> -}
> -
> -/**
> -  Initialize MSR spin lock by MSR index.
> -
> -  @param  MsrIndex   MSR index value.
> -
> -**/
> -VOID
> -InitMsrSpinLockByIndex (
> -  IN UINT32  MsrIndex
> -  )
> -{
> -  UINTNMsrSpinLockCount;
> -  UINTNNewMsrSpinLockCount;
> -  UINTNIndex;
> -  UINTNAddedSize;
> -
> -  if (mMsrSpinLocks == NULL) {
> -MsrSpinLockCount = mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter;
> -mMsrSpinLocks = (MP_MSR_LOCK *) AllocatePool (sizeof (MP_MSR_LOCK) * 
> MsrSpinLockCount);
> -ASSERT (mMsrSpinLocks != NULL);
> -for (Index = 0; Index < MsrSpinLockCount; Index++) {
> -  mMsrSpinLocks[Index].SpinLock =
> -   (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr + Index * 
> mSemaphoreSize);
> -  mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
> -}
> -mMsrSpinLockCount = MsrSpinLockCount;
> -mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter = 0;
> -  }
> -  if (GetMsrSpinLockByIndex (MsrIndex) == NULL) {
> -//
> -// Initialize spin lock for MSR programming
> -//
> -mMsrSpinLocks[mMsrCount].MsrIndex = MsrIndex;
> -InitializeSpinLock (mMsrSpinLocks[mMsrCount].SpinLock);
> -mMsrCount ++;
> -if (mMsrCount == mMsrSpinLockCount) {
> -  //
> -  // If MSR spin lock buffer is full, enlarge it
> -  //
> -  AddedSize = SIZE_4KB;
> -  mSmmCpuSemaphores.SemaphoreMsr.Msr =
> -AllocatePages (EFI_SIZE_TO_PAGES(AddedSize));
> -  ASSERT (mSmmCpuSemaphores.SemaphoreMsr.Msr != NULL);
> -  NewMsrSpinLockCount = mMsrSpinLockCount + AddedSize / mSemaphoreSize;
> -  mMsrSpinLocks = ReallocatePool (
> -sizeof (MP_MSR_LOCK) * mMsrSpinLockCount,
> -

[edk2] [Patch 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type.

2018-10-14 Thread Eric Dong 
Because this driver needs to set MSRs saved in normal boot phase, sync semaphore
logic from RegisterCpuFeaturesLib code which used for normal boot phase.

Detail see change SHA-1: dcdf1774212d87e2d7feb36286a408ea7475fd7b for
RegisterCpuFeaturesLib.

Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c  | 316 -
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |   3 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   3 +-
 3 files changed, 180 insertions(+), 142 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 52ff9679d5..5a35f7a634 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -38,9 +38,12 @@ typedef struct {
 } MP_ASSEMBLY_ADDRESS_MAP;
 
 //
-// Spin lock used to serialize MemoryMapped operation
+// Flags used when program the register.
 //
-SPIN_LOCK*mMemoryMappedLock = NULL;
+typedef struct {
+  volatile UINTN   MemoryMappedLock; // Spinlock used to program 
mmio
+  volatile UINT32  *SemaphoreCount;  // Semaphore used to program 
semaphore.
+} PROGRAM_CPU_REGISTER_FLAGS;
 
 //
 // Signal that SMM BASE relocation is complete.
@@ -62,13 +65,11 @@ AsmGetAddressMap (
 #define LEGACY_REGION_SIZE(2 * 0x1000)
 #define LEGACY_REGION_BASE(0xA - LEGACY_REGION_SIZE)
 
+PROGRAM_CPU_REGISTER_FLAGS   mCpuFlags;
 ACPI_CPU_DATAmAcpiCpuData;
 volatile UINT32  mNumberToFinish;
 MP_CPU_EXCHANGE_INFO *mExchangeInfo;
 BOOLEAN  mRestoreSmmConfigurationInS3 = FALSE;
-MP_MSR_LOCK  *mMsrSpinLocks = NULL;
-UINTNmMsrSpinLockCount;
-UINTNmMsrCount = 0;
 
 //
 // S3 boot flag
@@ -91,89 +92,6 @@ UINT8mApHltLoopCodeTemplate[] = {
0xEB, 0xFC   // jmp $-2
};
 
-/**
-  Get MSR spin lock by MSR index.
-
-  @param  MsrIndex   MSR index value.
-
-  @return Pointer to MSR spin lock.
-
-**/
-SPIN_LOCK *
-GetMsrSpinLockByIndex (
-  IN UINT32  MsrIndex
-  )
-{
-  UINTN Index;
-  for (Index = 0; Index < mMsrCount; Index++) {
-if (MsrIndex == mMsrSpinLocks[Index].MsrIndex) {
-  return mMsrSpinLocks[Index].SpinLock;
-}
-  }
-  return NULL;
-}
-
-/**
-  Initialize MSR spin lock by MSR index.
-
-  @param  MsrIndex   MSR index value.
-
-**/
-VOID
-InitMsrSpinLockByIndex (
-  IN UINT32  MsrIndex
-  )
-{
-  UINTNMsrSpinLockCount;
-  UINTNNewMsrSpinLockCount;
-  UINTNIndex;
-  UINTNAddedSize;
-
-  if (mMsrSpinLocks == NULL) {
-MsrSpinLockCount = mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter;
-mMsrSpinLocks = (MP_MSR_LOCK *) AllocatePool (sizeof (MP_MSR_LOCK) * 
MsrSpinLockCount);
-ASSERT (mMsrSpinLocks != NULL);
-for (Index = 0; Index < MsrSpinLockCount; Index++) {
-  mMsrSpinLocks[Index].SpinLock =
-   (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr + Index * 
mSemaphoreSize);
-  mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
-}
-mMsrSpinLockCount = MsrSpinLockCount;
-mSmmCpuSemaphores.SemaphoreMsr.AvailableCounter = 0;
-  }
-  if (GetMsrSpinLockByIndex (MsrIndex) == NULL) {
-//
-// Initialize spin lock for MSR programming
-//
-mMsrSpinLocks[mMsrCount].MsrIndex = MsrIndex;
-InitializeSpinLock (mMsrSpinLocks[mMsrCount].SpinLock);
-mMsrCount ++;
-if (mMsrCount == mMsrSpinLockCount) {
-  //
-  // If MSR spin lock buffer is full, enlarge it
-  //
-  AddedSize = SIZE_4KB;
-  mSmmCpuSemaphores.SemaphoreMsr.Msr =
-AllocatePages (EFI_SIZE_TO_PAGES(AddedSize));
-  ASSERT (mSmmCpuSemaphores.SemaphoreMsr.Msr != NULL);
-  NewMsrSpinLockCount = mMsrSpinLockCount + AddedSize / mSemaphoreSize;
-  mMsrSpinLocks = ReallocatePool (
-sizeof (MP_MSR_LOCK) * mMsrSpinLockCount,
-sizeof (MP_MSR_LOCK) * NewMsrSpinLockCount,
-mMsrSpinLocks
-);
-  ASSERT (mMsrSpinLocks != NULL);
-  mMsrSpinLockCount = NewMsrSpinLockCount;
-  for (Index = mMsrCount; Index < mMsrSpinLockCount; Index++) {
-mMsrSpinLocks[Index].SpinLock =
- (SPIN_LOCK *)((UINTN)mSmmCpuSemaphores.SemaphoreMsr.Msr +
- (Index - mMsrCount)  * mSemaphoreSize);
-mMsrSpinLocks[Index].MsrIndex = (UINT32)-1;
-  }
-}
-  }
-}
-
 /**
   Sync up the MTRR values for all processors.
 
@@ -204,42 +122,89 @@ Returns:
 }
 
 /**
-  Programs registers for the calling processor.
+  Increment semaphore by 1.
 
-  This function programs registers for the calling processor.
+  @param  SemIN:  32-bit unsigned integer
 
-  @param  RegisterTablesPointer to register table of the running