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

["Ni, Ray" <ray.ni@intel.com>]

Leo and all,
I replied in https://bugzilla.tianocore.org/show_bug.cgi?id=2556 for a more general question about how uCode is used in AMD processors.

Because this package recently exposed a new interface ShadowMicrocodePpi, I try to involve Leo in the review from AMD uCode's needs.

Thanks,
Ray

> -----Original Message-----
> From: Duran, Leo <leo.duran@amd.com>
> Sent: Thursday, February 27, 2020 1:52 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Wu, Hao A
> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib
> 
> 
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, February 26, 2020 12:45 PM
> > To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > On 02/26/20 17:39, Duran, Leo wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > >> Sent: Wednesday, February 26, 2020 11:21 AM
> > >> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > >> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > >> <siyuan.fu@intel.com>
> > >> Cc: Dong, Eric <eric.dong@intel.com>
> > >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > >> MpInitLib
> > >>
> > >> On 02/26/20 16:46, Duran, Leo wrote:
> > >>> 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?
> > >>
> > >> I think a global flag is justified; in the above approach, "IsValidPlatformId"
> > >> would not vary across "ProcessorNumber", so it does look like useless
> > >> generality.
> > > [Duran, Leo]
> > > Great point, Laszlo.
> > > Indeed, global makes senses in the case!
> > > I can prepare a v2-set to incorporate that.
> >
> > No, sorry, that wasn't what I meant. I didn't try to suggest a global variable.
> > Instead, I meant that a "global check" (conceptually, i.e.
> > regardless of particular processor number) made sense.
> >
> > I'm also not particularly *against* a global variable. In other words, I didn't try
> > to comment on using a global variable *at all*.
> >
> > Using a global variable might as well work, I just feel that your current patches
> > are good enough.
> [Duran, Leo]
> Great... I hear you.
> Then, I'd prefer not refactoring further at this point.... I hope Ray & Eric agree.
> 
> Thanks for your feedback!
> Leo.
> 
> >
> > Thanks
> > Laszlo