UefiCpuPkg/Include/Library/LocalApicLib.h | 15 +++++++ UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 47 +++++++++++----------- .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 47 +++++++++++----------- UefiCpuPkg/Library/MpInitLib/Microcode.c | 17 +++++++- UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 ++++- 5 files changed, 87 insertions(+), 50 deletions(-)
This patch set fixes an issue introduced recently in MpInitLib, where we read
a PlatformId MSR that is not implemented on AMD processors.
The proposed solution is to export the StandardSignatureIsAuthenticAMD function
from LocalApicLib, so that it may be used by MpInitLib or any other module that
consumes LocalApicLib.
Alternatively, we considered creating a new library, but opted against it as
that would incur quite a bit of churning across modules that consume MpInitLib.
BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(),
but it never affected AMD-based platforms as the flow never gets that far, since
the Detect routine bails out early when it finds the size of the patch is zero.
Leo Duran (2):
UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD
function
UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.
UefiCpuPkg/Include/Library/LocalApicLib.h | 15 +++++++
UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 47 +++++++++++-----------
.../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 47 +++++++++++-----------
UefiCpuPkg/Library/MpInitLib/Microcode.c | 17 +++++++-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 ++++-
5 files changed, 87 insertions(+), 50 deletions(-)
--
2.7.4
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54797): https://edk2.groups.io/g/devel/message/54797
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Leo, On 02/25/20 20:39, Leo Duran wrote: > This patch set fixes an issue introduced recently in MpInitLib, where we read > a PlatformId MSR that is not implemented on AMD processors. > > The proposed solution is to export the StandardSignatureIsAuthenticAMD function > from LocalApicLib, so that it may be used by MpInitLib or any other module that > consumes LocalApicLib. > > Alternatively, we considered creating a new library, but opted against it as > that would incur quite a bit of churning across modules that consume MpInitLib. > > BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(), > but it never affected AMD-based platforms as the flow never gets that far, since > the Detect routine bails out early when it finds the size of the patch is zero. > > > Leo Duran (2): > UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD > function > UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors. > > UefiCpuPkg/Include/Library/LocalApicLib.h | 15 +++++++ > UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 47 +++++++++++----------- > .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 47 +++++++++++----------- > UefiCpuPkg/Library/MpInitLib/Microcode.c | 17 +++++++- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 ++++- > 5 files changed, 87 insertions(+), 50 deletions(-) > from my perspective I'm OK with this approach: Acked-by: Laszlo Ersek <lersek@redhat.com> but Ray and Eric have the final word on this, of course. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54824): https://edk2.groups.io/g/devel/message/54824 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Leo, > > BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(), > > but it never affected AMD-based platforms as the flow never gets that far, since > > the Detect routine bails out early when it finds the size of the patch is zero. You are saying that PlatformId MSR access is not performed by CPU in old code because of the zero size uCode. But now with Hao or Siyuan's change, the PlatformId MSR access is always performed even when there is no uCode. It sounds like a regression to optimization to me. Did you evaluate the path to avoid accessing PlatformID MSR when uCode doesn't exist? So that the API to detect AMD processor is not needed at all. Thanks, Ray -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54841): https://edk2.groups.io/g/devel/message/54841 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Leo: Now, we enter into Hard Feature Freeze phase until edk2-stable202002 tag is created at 2020-02-28. Only critical bug fix is still allowed. Do you request to catch this fix into this stable tag? If yes, please work closely with UefiCpuPkg maintainers to catch the stable tag release schedule 2020-02-28. Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray > Sent: Wednesday, February 26, 2020 3:57 PM > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; leo.duran@amd.com; 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, > > > > BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(), > > > but it never affected AMD-based platforms as the flow never gets that far, since > > > the Detect routine bails out early when it finds the size of the patch is zero. > > You are saying that PlatformId MSR access is not performed by CPU in old code because of the zero size uCode. > But now with Hao or Siyuan's change, the PlatformId MSR access is always performed even when there is no uCode. It sounds like a > regression to optimization to me. > Did you evaluate the path to avoid accessing PlatformID MSR when uCode doesn't exist? So that the API to detect AMD processor is not > needed at all. > > Thanks, > Ray > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54842): https://edk2.groups.io/g/devel/message/54842 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: Gao, Liming [mailto:liming.gao@intel.com] > Sent: Wednesday, February 26, 2020 3:56 AM > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek > <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; Wu, Hao A > <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, Michael D > <michael.d.kinney@intel.com>; afish@apple.com; Gao, Liming > <liming.gao@intel.com> > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in > MpInitLib > > Leo: > Now, we enter into Hard Feature Freeze phase until edk2-stable202002 tag > is created at 2020-02-28. Only critical bug fix is still allowed. > > Do you request to catch this fix into this stable tag? If yes, please work closely > with UefiCpuPkg maintainers to catch the stable tag release schedule 2020- > 02-28. > > Thanks > Liming [Duran, Leo] I'm not sure I understand what my next steps are, perhaps I need to go back to the BUGZILLA? (This issue is a "must-fix", so perhaps I can tag it as such in the BUG report.). Thanks, Leo. > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray > > Sent: Wednesday, February 26, 2020 3:57 PM > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; > > leo.duran@amd.com; 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, > > > > > > BTW, reading the PlatformId MSR was already being done by > > > > MicrocodeDetect(), but it never affected AMD-based platforms as > > > > the flow never gets that far, since the Detect routine bails out early > when it finds the size of the patch is zero. > > > > You are saying that PlatformId MSR access is not performed by CPU in old > code because of the zero size uCode. > > But now with Hao or Siyuan's change, the PlatformId MSR access is > > always performed even when there is no uCode. It sounds like a regression > to optimization to me. > > Did you evaluate the path to avoid accessing PlatformID MSR when uCode > > doesn't exist? So that the API to detect AMD processor is not needed at all. > > > > Thanks, > > Ray > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54899): https://edk2.groups.io/g/devel/message/54899 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Leo, On 02/26/20 16:11, Duran, Leo wrote: > > >> -----Original Message----- >> From: Gao, Liming [mailto:liming.gao@intel.com] >> Sent: Wednesday, February 26, 2020 3:56 AM >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek >> <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; Wu, Hao A >> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com> >> Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, Michael D >> <michael.d.kinney@intel.com>; afish@apple.com; Gao, Liming >> <liming.gao@intel.com> >> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in >> MpInitLib >> >> Leo: >> Now, we enter into Hard Feature Freeze phase until edk2-stable202002 tag >> is created at 2020-02-28. Only critical bug fix is still allowed. >> >> Do you request to catch this fix into this stable tag? If yes, please work closely >> with UefiCpuPkg maintainers to catch the stable tag release schedule 2020- >> 02-28. >> >> Thanks >> Liming > [Duran, Leo] > I'm not sure I understand what my next steps are, perhaps I need to go back to the BUGZILLA? > (This issue is a "must-fix", so perhaps I can tag it as such in the BUG report.). the issue will clearly be fixed, the question is: when. Edk2 is now in hard feature freeze for the upcoming edk2-stable202002 tag. See the schedule and the definitions here: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning The question is whether you want this fix to be part of the edk2-stable202002 tag. If not, then there's no rush, the patch series (the final, reviewed version) will be merged after the tag (after 2020-02-28). If you do want to get the fix into edk2-stable202002, then: - you need to demonstrate that the patches implement an important bugfix (not a feature addition) -- it helps if you describe the problem symptoms in this case, - and you and Eric & Ray need to collaborate in a hurry to get the patches refreshed as necessary, reviewed, and merged. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54890): https://edk2.groups.io/g/devel/message/54890 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, February 26, 2020 11:25 AM > To: Duran, Leo <leo.duran@amd.com>; Gao, Liming <liming.gao@intel.com>; > devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Wu, Hao A > <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com> > Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, Michael D > <michael.d.kinney@intel.com>; afish@apple.com > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in > MpInitLib > > Hi Leo, > > On 02/26/20 16:11, Duran, Leo wrote: > > > > > >> -----Original Message----- > >> From: Gao, Liming [mailto:liming.gao@intel.com] > >> Sent: Wednesday, February 26, 2020 3:56 AM > >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek > >> <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; Wu, Hao A > >> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com> > >> Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, > >> Michael D <michael.d.kinney@intel.com>; afish@apple.com; Gao, Liming > >> <liming.gao@intel.com> > >> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in > >> MpInitLib > >> > >> Leo: > >> Now, we enter into Hard Feature Freeze phase until > >> edk2-stable202002 tag is created at 2020-02-28. Only critical bug fix is still > allowed. > >> > >> Do you request to catch this fix into this stable tag? If yes, > >> please work closely with UefiCpuPkg maintainers to catch the stable > >> tag release schedule 2020- 02-28. > >> > >> Thanks > >> Liming > > [Duran, Leo] > > I'm not sure I understand what my next steps are, perhaps I need to go back > to the BUGZILLA? > > (This issue is a "must-fix", so perhaps I can tag it as such in the BUG report.). > > the issue will clearly be fixed, the question is: when. > > Edk2 is now in hard feature freeze for the upcoming edk2-stable202002 tag. > See the schedule and the definitions here: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub > .com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release- > Planning&data=02%7C01%7Cleo.duran%40amd.com%7Cdec7fb5d02784 > 01bcf1008d7bad87003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > C0%7C637183311078914692&sdata=R%2FBNmDgmA1m7rRyK%2F2DSn > Os3Cw8W3ZgPVZoZe3QPNJg%3D&reserved=0 > > The question is whether you want this fix to be part of the > edk2-stable202002 tag. If not, then there's no rush, the patch series (the final, > reviewed version) will be merged after the tag (after 2020-02-28). > > If you do want to get the fix into edk2-stable202002, then: > > - you need to demonstrate that the patches implement an important bugfix > (not a feature addition) -- it helps if you describe the problem symptoms in > this case, > > - and you and Eric & Ray need to collaborate in a hurry to get the patches > refreshed as necessary, reviewed, and merged. > [Duran, Leo] It sounds like either way the merge would happen reasonably soon. So although it's "bug-fix" as opposed to a "feature-add", I'll defer to the maintainers for an optimal merge plan. And I suppose folks with a different opinion could possibly chime in on the opened ticket. Thanks, Leo. > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54910): https://edk2.groups.io/g/devel/message/54910 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: Ni, Ray [mailto:ray.ni@intel.com] > Sent: Wednesday, February 26, 2020 2:57 AM > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Duran, Leo > <leo.duran@amd.com>; 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, > > > > BTW, reading the PlatformId MSR was already being done by > > > MicrocodeDetect(), but it never affected AMD-based platforms as the > > > flow never gets that far, since the Detect routine bails out early when it > finds the size of the patch is zero. > > You are saying that PlatformId MSR access is not performed by CPU in old > code because of the zero size uCode. > But now with Hao or Siyuan's change, the PlatformId MSR access is always > performed even when there is no uCode. It sounds like a regression to > optimization to me. > Did you evaluate the path to avoid accessing PlatformID MSR when uCode > doesn't exist? So that the API to detect AMD processor is not needed at all. [Duran, Leo] Hi Ray, I think your summary is pretty accurate, except that I'd say that avoiding a READ from the PlatformId MSR should happen solely based on the fact that the MSR simply does not exist on AMD processors. Then as a result of that, the usage of the PlatformId (as it relates to microcode or anything else) must then be dealt with separately. To that end, I think I covered all cases where the MSR is being read, and also where PlatformId is being used. (I also added comments for each case) Thanks, Leo. > > Thanks, > Ray -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54892): https://edk2.groups.io/g/devel/message/54892 Mute This Topic: https://groups.io/mt/71541516/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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?
Leo.
> -----Original Message-----
> From: Duran, Leo
> Sent: Wednesday, February 26, 2020 10:25 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: Wednesday, February 26, 2020 2:57 AM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Duran, Leo
> > <leo.duran@amd.com>; 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,
> >
> > > > BTW, reading the PlatformId MSR was already being done by
> > > > MicrocodeDetect(), but it never affected AMD-based platforms as
> > > > the flow never gets that far, since the Detect routine bails out
> > > > early when it
> > finds the size of the patch is zero.
> >
> > You are saying that PlatformId MSR access is not performed by CPU in
> > old code because of the zero size uCode.
> > But now with Hao or Siyuan's change, the PlatformId MSR access is
> > always performed even when there is no uCode. It sounds like a
> > regression to optimization to me.
> > Did you evaluate the path to avoid accessing PlatformID MSR when uCode
> > doesn't exist? So that the API to detect AMD processor is not needed at all.
> [Duran, Leo]
> Hi Ray,
> I think your summary is pretty accurate, except that I'd say that avoiding a
> READ from the PlatformId MSR should happen solely based on the fact that
> the MSR simply does not exist on AMD processors.
> Then as a result of that, the usage of the PlatformId (as it relates to microcode
> or anything else) must then be dealt with separately.
>
> To that end, I think I covered all cases where the MSR is being read, and also
> where PlatformId is being used.
> (I also added comments for each case)
>
> Thanks,
> Leo.
>
> >
> > Thanks,
> > Ray
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54900): https://edk2.groups.io/g/devel/message/54900
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54889): https://edk2.groups.io/g/devel/message/54889
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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.
Thanks,
Leo
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54905): https://edk2.groups.io/g/devel/message/54905
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54904): https://edk2.groups.io/g/devel/message/54904
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54940): https://edk2.groups.io/g/devel/message/54940
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54977): https://edk2.groups.io/g/devel/message/54977
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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&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@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55101): https://edk2.groups.io/g/devel/message/55101
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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&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@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
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni@intel.com]
> Sent: Friday, February 28, 2020 1:47 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,
> 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@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%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@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
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Laszlo, et al,
I suppose the same can be said about the actual "PlatformId"... it should be a single/global read, correct?
But I'd prefer not tackling that in this patch-set (I'll defer to someone that may want to take that on as an optimization/clean-up.).
Leo.
> -----Original Message-----
> From: Duran, Leo
> Sent: Wednesday, February 26, 2020 11:40 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 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.
>
> Thanks,
> Leo
>
> >
> > Thanks
> > Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54914): https://edk2.groups.io/g/devel/message/54914
Mute This Topic: https://groups.io/mt/71541516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.