[edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for each core.

Eric Dong posted 3 patches 7 years, 6 months ago
[edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for each core.
Posted by Eric Dong 7 years, 6 months ago
The SDM requires only one thread per core to load the
microcode.

This change enables this solution.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 351975e2a2..122c23469d 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -61,6 +61,7 @@ MicrocodeDetect (
   VOID                                    *MicrocodeData;
   MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
   UINT32                                  ProcessorFlags;
+  UINT32                                  ThreadId;
 
   if (CpuMpData->MicrocodePatchRegionSize == 0) {
     //
@@ -77,6 +78,14 @@ MicrocodeDetect (
     return;
   }
 
+  GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL, &ThreadId);
+  if (ThreadId != 0) {
+    //
+    // Skip loading microcode if it is not the first thread in one core.
+    //
+    return;
+  }
+
   ExtendedTableLength = 0;
   //
   // Here data of CPUID leafs have not been collected into context buffer, so
-- 
2.15.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for each core.
Posted by Ni, Ruiyu 7 years, 6 months ago
On 7/16/2018 11:08 AM, Eric Dong wrote:
> GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL, &ThreadId);
> +  if (ThreadId != 0) {
> +    //
> +    // Skip loading microcode if it is not the first thread in one core.
> +    //
> +    return;
> +  }
> +

Eric,
Is it possible that Thread#0 is disabled while Thread#1 is enabled? It 
may cause the certain core with no uCode applied.

-- 
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for each core.
Posted by Laszlo Ersek 7 years, 6 months ago
On 07/17/18 12:02, Ni, Ruiyu wrote:
> On 7/16/2018 11:08 AM, Eric Dong wrote:
>> GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL,
>> &ThreadId);
>> +  if (ThreadId != 0) {
>> +    //
>> +    // Skip loading microcode if it is not the first thread in one core.
>> +    //
>> +    return;
>> +  }
>> +
> 
> Eric,
> Is it possible that Thread#0 is disabled while Thread#1 is enabled? It
> may cause the certain core with no uCode applied.
> 

I thought of this (superficially) but I figured the code would run
anyway at CpuMpPei / CpuDxe startup only, at which point no logical
processors could have been disabled yet -- programmatically, through the
MP PPI / protocol. Is there any other reason why ThreadId=0 could be
missing?

(I plan to check the rest of the series later.)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for each core.
Posted by Dong, Eric 7 years, 6 months ago
Hi Laszlo & Ray,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, July 17, 2018 10:07 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for
> each core.
> 
> On 07/17/18 12:02, Ni, Ruiyu wrote:
> > On 7/16/2018 11:08 AM, Eric Dong wrote:
> >> GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL,
> >> &ThreadId);
> >> +  if (ThreadId != 0) {
> >> +    //
> >> +    // Skip loading microcode if it is not the first thread in one core.
> >> +    //
> >> +    return;
> >> +  }
> >> +
> >
> > Eric,
> > Is it possible that Thread#0 is disabled while Thread#1 is enabled? It
> > may cause the certain core with no uCode applied.
> >
> 
> I thought of this (superficially) but I figured the code would run anyway at
> CpuMpPei / CpuDxe startup only, at which point no logical processors could
> have been disabled yet -- programmatically, through the MP PPI / protocol. Is
> there any other reason why ThreadId=0 could be missing?
> 

Yes, I think at this point, all the threads should at the ready state.  The only case I can image is that the hardware has broken and the core can't be wake up.  But I think in this case, the core should can't work, not only thread 0 can't work.  What do you think?

> (I plan to check the rest of the series later.)
> 
> Thanks
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once for each core.
Posted by Ni, Ruiyu 7 years, 6 months ago

Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, July 18, 2018 8:12 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode once
> for each core.
> 
> Hi Laszlo & Ray,
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Tuesday, July 17, 2018 10:07 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Load uCode
> > once for each core.
> >
> > On 07/17/18 12:02, Ni, Ruiyu wrote:
> > > On 7/16/2018 11:08 AM, Eric Dong wrote:
> > >> GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL,
> > >> &ThreadId);
> > >> +  if (ThreadId != 0) {
> > >> +    //
> > >> +    // Skip loading microcode if it is not the first thread in one core.
> > >> +    //
> > >> +    return;
> > >> +  }
> > >> +
> > >
> > > Eric,
> > > Is it possible that Thread#0 is disabled while Thread#1 is enabled?
> > > It may cause the certain core with no uCode applied.
> > >
> >
> > I thought of this (superficially) but I figured the code would run
> > anyway at CpuMpPei / CpuDxe startup only, at which point no logical
> > processors could have been disabled yet -- programmatically, through
> > the MP PPI / protocol. Is there any other reason why ThreadId=0 could be
> missing?
> >
> 
> Yes, I think at this point, all the threads should at the ready state.  The only
> case I can image is that the hardware has broken and the core can't be wake
> up.  But I think in this case, the core should can't work, not only thread 0 can't
> work.  What do you think?
I am not sure. We need to check the IA32 SDM carefully for any potential unexpected cases.

> 
> > (I plan to check the rest of the series later.)
> >
> > Thanks
> > Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel