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

Fri Feb 28 16:38:54 UTC 2020


> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni at intel.com]
> Sent: Friday, February 28, 2020 1:47 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,
> 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.
[Duran, Leo] 
I can't argue that point strongly.
It just seemed "related enough" to warrant the export to avoid duplication.

> 
> 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?
[Duran, Leo] 
I take this to mean duplicating the Signature function in MpLib.c
So, I will submit that patch instead.

> 
> 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%2Fbu
> > > gzill
> > >
> 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 (#55102): https://edk2.groups.io/g/devel/message/55102
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]
-=-=-=-=-=-=-=-=-=-=-=-