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

["Duran, Leo" <leo.duran@amd.com>]

> -----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