[edk2] [PATCH v4 03/19] UefiCpuPkg: CpuDxe: broadcast MTRR changes to APs

2015-10-19 Thread Michael Kinney 
From: edk2-devel 

The Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe
driver applies any MTRR changes to APs, if the
EFI_MP_SERVICES_PROTOCOL is available. We should do the same.

Additionally, the broadcast should occur at MP startup as well,
not only when MTRR settings are changed. The inspiration is
taken from

  Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuMpDxe/

(see the EarlyMpInit() function and its call sites in
"ProcessorConfig.c").

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
Reviewed-by: Jeff Fan 
Reviewed-by: Michael Kinney 
---
 UefiCpuPkg/CpuDxe/CpuDxe.c | 26 ++
 UefiCpuPkg/CpuDxe/CpuMp.c  | 34 +-
 UefiCpuPkg/CpuDxe/CpuMp.h  | 13 +
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index c9df4e1..daf97bd 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -350,6 +350,9 @@ CpuSetMemoryAttributes (
 {
   RETURN_STATUS Status;
   MTRR_MEMORY_CACHE_TYPECacheType;
+  EFI_STATUSMpStatus;
+  EFI_MP_SERVICES_PROTOCOL  *MpService;
+  MTRR_SETTINGS MtrrSettings;
 
   if (!IsMtrrSupported ()) {
 return EFI_UNSUPPORTED;
@@ -405,6 +408,29 @@ CpuSetMemoryAttributes (
  CacheType
  );
 
+  if (!RETURN_ERROR (Status)) {
+MpStatus = gBS->LocateProtocol (
+  ,
+  NULL,
+  (VOID **)
+  );
+//
+// Synchronize the update with all APs
+//
+if (!EFI_ERROR (MpStatus)) {
+  MtrrGetAllMtrrs ();
+  MpStatus = MpService->StartupAllAPs (
+  MpService,  // This
+  SetMtrrsFromBuffer, // Procedure
+  TRUE,   // SingleThread
+  NULL,   // WaitEvent
+  0,  // TimeoutInMicrosecsond
+  ,  // ProcedureArgument
+  NULL// FailedCpuList
+  );
+  ASSERT (MpStatus == EFI_SUCCESS || MpStatus == EFI_NOT_STARTED);
+}
+  }
   return (EFI_STATUS) Status;
 }
 
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index da3686e..fbe43f5 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -1626,6 +1626,22 @@ ExitBootServicesCallback (
 }
 
 /**
+  A minimal wrapper function that allows MtrrSetAllMtrrs() to be passed to
+  EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() as Procedure.
+
+  @param[in] Buffer  Pointer to an MTRR_SETTINGS object, to be passed to
+ MtrrSetAllMtrrs().
+**/
+VOID
+EFIAPI
+SetMtrrsFromBuffer (
+  IN VOID *Buffer
+  )
+{
+  MtrrSetAllMtrrs (Buffer);
+}
+
+/**
   Initialize Multi-processor support.
 
 **/
@@ -1634,7 +1650,8 @@ InitializeMpSupport (
   VOID
   )
 {
-  EFI_STATUS Status;
+  EFI_STATUSStatus;
+  MTRR_SETTINGS MtrrSettings;
 
   gMaxLogicalProcessorNumber = (UINTN) PcdGet32 
(PcdCpuMaxLogicalProcessorNumber);
   if (gMaxLogicalProcessorNumber < 1) {
@@ -1690,6 +1707,21 @@ InitializeMpSupport (
   //
   CollectBistDataFromHob ();
 
+  //
+  // Synchronize MTRR settings to APs.
+  //
+  MtrrGetAllMtrrs ();
+  Status = mMpServicesTemplate.StartupAllAPs (
+ , // This
+ SetMtrrsFromBuffer,   // Procedure
+ TRUE, // SingleThread
+ NULL, // WaitEvent
+ 0,// TimeoutInMicrosecsond
+ ,// ProcedureArgument
+ NULL  // FailedCpuList
+ );
+  ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_STARTED);
+
   Status = gBS->InstallMultipleProtocolInterfaces (
   ,
   ,  ,
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
index d2866e4..503f3ae 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.h
+++ b/UefiCpuPkg/CpuDxe/CpuMp.h
@@ -643,5 +643,18 @@ ResetApStackless (
   IN UINT32 ProcessorId
   );
 
+/**
+  A minimal wrapper function that allows MtrrSetAllMtrrs() to be passed to
+  EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() as Procedure.
+
+  @param[in] Buffer  Pointer to an MTRR_SETTINGS object, to be passed to
+ MtrrSetAllMtrrs().
+**/
+VOID
+EFIAPI
+SetMtrrsFromBuffer (
+  IN VOID *Buffer
+  );
+
 #endif // _CPU_MP_H_
 
-- 
1.9.5.msysgit.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 03/19] UefiCpuPkg: CpuDxe: broadcast MTRR changes to APs

2015-10-19 Thread Kinney, Michael D 
Laszlo,

Thanks for the feedback.  

1) I have updated the author using --author='Laszlo Ersek <ler...@redhat.com>'
2) I changed the order of the commits so the MP race condition is fixed before 
your patch to sync MTRRs to all APs.

Best regards,

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Monday, October 19, 2015 6:43 AM
>To: Kinney, Michael D; edk2-de...@ml01.01.org
>Cc: edk2-devel
>Subject: Re: [edk2] [PATCH v4 03/19] UefiCpuPkg: CpuDxe: broadcast MTRR
>changes to APs
>
>On 10/19/15 09:44, Michael Kinney wrote:
>> From: edk2-devel <edk2-devel-boun...@lists.01.org>
>
>With SVN it won't matter much ultimately, but considering git per se, I
>think in your git tree my authorship was lost somehow. (The
>Signed-off-by below is correct though.) This can be fixed by:
>- rebasing the series
>- selecting "edit" for this patch
>- committing it without any changes, but with different authorship:
>  git commit --amend --author='Laszlo Ersek <ler...@redhat.com>'
>
>Not overly important, but I think it's useful to know about git commit
>--author.
>
>Another (not super-important) comment: in cases like this, a patch
>series usually applies the fix for the dormant bug first (i.e., patch v4
>04/19), and then introduces the change that would expose the bug (i.e.,
>this patch). The idea being, since the bug is known, there should be no
>point in history where the bug is exposed -- it could be triggered by an
>unrelated bisection, for example.
>
>Again, not too important, but it's so great that you've been using git
>for this work that I can't hold back some unsolicited git tips. :)
>
>Thanks!
>Laszlo
>
>>
>> The Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe
>> driver applies any MTRR changes to APs, if the
>> EFI_MP_SERVICES_PROTOCOL is available. We should do the same.
>>
>> Additionally, the broadcast should occur at MP startup as well,
>> not only when MTRR settings are changed. The inspiration is
>> taken from
>>
>>   Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuMpDxe/
>>
>> (see the EarlyMpInit() function and its call sites in
>> "ProcessorConfig.c").
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> Reviewed-by: Jeff Fan <jeff@intel.com>
>> Reviewed-by: Michael Kinney <michael.d.kin...@intel.com>
>> ---
>>  UefiCpuPkg/CpuDxe/CpuDxe.c | 26 ++
>>  UefiCpuPkg/CpuDxe/CpuMp.c  | 34 +-
>>  UefiCpuPkg/CpuDxe/CpuMp.h  | 13 +
>>  3 files changed, 72 insertions(+), 1 deletion(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
>> index c9df4e1..daf97bd 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
>> @@ -350,6 +350,9 @@ CpuSetMemoryAttributes (
>>  {
>>RETURN_STATUS Status;
>>MTRR_MEMORY_CACHE_TYPECacheType;
>> +  EFI_STATUSMpStatus;
>> +  EFI_MP_SERVICES_PROTOCOL  *MpService;
>> +  MTRR_SETTINGS MtrrSettings;
>>
>>if (!IsMtrrSupported ()) {
>>  return EFI_UNSUPPORTED;
>> @@ -405,6 +408,29 @@ CpuSetMemoryAttributes (
>>   CacheType
>>   );
>>
>> +  if (!RETURN_ERROR (Status)) {
>> +MpStatus = gBS->LocateProtocol (
>> +  ,
>> +  NULL,
>> +  (VOID **)
>> +  );
>> +//
>> +// Synchronize the update with all APs
>> +//
>> +if (!EFI_ERROR (MpStatus)) {
>> +  MtrrGetAllMtrrs ();
>> +  MpStatus = MpService->StartupAllAPs (
>> +  MpService,  // This
>> +  SetMtrrsFromBuffer, // Procedure
>> +  TRUE,   // SingleThread
>> +  NULL,   // WaitEvent
>> +  0,  // TimeoutInMicrosecsond
>> +  ,  // ProcedureArgument
>> +  NULL// FailedCpuList
>> +  );
>> +  ASSERT (MpStatus == EFI_SUCCESS || MpStatus == EFI_NOT_STARTED);
>> +}
>> +  }
>>return (EFI_STATUS) Status;
>>  }
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
>> index da3686e..fbe43f5 100644
>> --- a/Uefi