[edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

BTW,

I also considered adding a flag to CPU_MP_DATA to make the usage of PlatformId a bit more explicit.
E.g., something like CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId... So the init code would look like this:

  //
  // NOTE: PlatformId is not relevant on AMD platforms.
  //
  if (StandardSignatureIsAuthenticAMD ()) {
    CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
  else {
    PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
    CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)PlatformIdMsr.Bits.PlatformId;
    CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
  }

This way "IsValidPlatformId" could be checked prior to using "PlatformId".
Anyway, that seemed a bit overkill, so I opted against it... thoughts?

Leo.


> -----Original Message-----
> From: Duran, Leo
> Sent: Wednesday, February 26, 2020 10:25 AM
> To: Ni, Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> devel at edk2.groups.io; Wu, Hao A <hao.a.wu at intel.com>; Fu, Siyuan
> <siyuan.fu at intel.com>
> Cc: Dong, Eric <eric.dong at intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> 
> 
> > -----Original Message-----
> > From: Ni, Ray [mailto:ray.ni at intel.com]
> > Sent: Wednesday, February 26, 2020 2:57 AM
> > To: Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io; Duran, Leo
> > <leo.duran at amd.com>; Wu, Hao A <hao.a.wu at intel.com>; Fu, Siyuan
> > <siyuan.fu at intel.com>
> > Cc: Dong, Eric <eric.dong at intel.com>
> > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > Leo,
> >
> > > > BTW, reading the PlatformId MSR was already being done by
> > > > MicrocodeDetect(), but it never affected AMD-based platforms as
> > > > the flow never gets that far, since the Detect routine bails out
> > > > early when it
> > finds the size of the patch is zero.
> >
> > You are saying that PlatformId MSR access is not performed by CPU in
> > old code because of the zero size uCode.
> > But now with Hao or Siyuan's change, the PlatformId MSR access is
> > always performed even when there is no uCode. It sounds like a
> > regression to optimization to me.
> > Did you evaluate the path to avoid accessing PlatformID MSR when uCode
> > doesn't exist? So that the API to detect AMD processor is not needed at all.
> [Duran, Leo]
> Hi Ray,
> I think your summary is pretty accurate, except that I'd say that avoiding a
> READ from the PlatformId MSR should happen solely based on the fact that
> the MSR simply does not exist on AMD processors.
> Then as a result of that,  the usage of the PlatformId (as it relates to microcode
> or anything else) must then be dealt with separately.
> 
> To that end, I think I covered all cases where the MSR is being read, and also
> where PlatformId is being used.
> (I also added comments for each case)
> 
> Thanks,
> Leo.
> 
> >
> > Thanks,
> > Ray

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54900): https://edk2.groups.io/g/devel/message/54900
Mute This Topic: https://groups.io/mt/71541516/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-