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

Fri Feb 28 06:47:18 UTC 2020

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 at amd.com>
> Sent: Friday, February 28, 2020 2:17 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: Thursday, February 27, 2020 12:55 AM
> > To: Duran, Leo <leo.duran at amd.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
> >
> > 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&data=02%7C01%7Cleo.
> > duran%40amd.com%7C40786a41798d4173dd8508d7bb49aaa7%7C3dd8961
> > fe4884e608e11a82d994e183d%7C0%7C0%7C637183797396444421&s
> > data=UoLRg%2ByFl%2BxyPB41xu1wOHpsf2euBrSe2HuD4CskTWg%3D&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 at amd.com>
> > > Sent: Thursday, February 27, 2020 1:52 AM
> > > To: Laszlo Ersek <lersek at redhat.com>; Ni, Ray <ray.ni at intel.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: Laszlo Ersek [mailto:lersek at redhat.com]
> > > > Sent: Wednesday, February 26, 2020 12:45 PM
> > > > To: Duran, Leo <leo.duran at amd.com>; Ni, Ray <ray.ni at intel.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
> > > >
> > > > On 02/26/20 17:39, Duran, Leo wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Laszlo Ersek [mailto:lersek at redhat.com]
> > > > >> Sent: Wednesday, February 26, 2020 11:21 AM
> > > > >> To: Duran, Leo <leo.duran at amd.com>; Ni, Ray <ray.ni at intel.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
> > > > >>
> > > > >> 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

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

View/Reply Online (#55048): https://edk2.groups.io/g/devel/message/55048
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]
-=-=-=-=-=-=-=-=-=-=-=-