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

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

Leo,
Thanks for your quick reply.

My most concern part to your patch is to expose a new API in LocalApicLib checking whether the processor is AMD type. LocalApicLib is a library that operates on Local APIC registers while checking CPU type deals with CPUID signature. It's not proper to mix the two things together just because LocalApicLib needs to know CPU type in some of the operation.

Because BaseLib already provides AsmCpuid() API and the check of CPU ID signature is just one line of comparison, I prefer to call AsmCpuid() directly in MpInitLib.

And in the patch to MpInitLib, since the AMD platform can set the two microcode PCDs to 0 to skip the microcode detection, I think the only place that needs to take care is in InitializeApData().

What do you think?

Thanks,
Ray



> -----Original Message-----
> From: Duran, Leo <leo.duran@amd.com>
> Sent: Friday, February 28, 2020 2:17 AM
> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.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: Ni, Ray [mailto:ray.ni@intel.com]
> > Sent: Thursday, February 27, 2020 12:55 AM
> > To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.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
> >
> > Leo and all,
> > I replied in
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzill
> > a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> > duran%40amd.com%7C40786a41798d4173dd8508d7bb49aaa7%7C3dd8961
> > fe4884e608e11a82d994e183d%7C0%7C0%7C637183797396444421&amp;s
> > data=UoLRg%2ByFl%2BxyPB41xu1wOHpsf2euBrSe2HuD4CskTWg%3D&amp;r
> > eserved=0 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
> [Duran, Leo] Hi Ray, I just updated the ticket to say:
> AMD handles microcode patches outside of the context of UEFI. So EDK2 hooks
> (ShadowMicrocode PPI, et al) are not required.
> (Hence my comments in the MpInitLib patch I just submitted)
> 
> >
> > > -----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