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


Duran, Leo <leo.duran@...>
 

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

Join devel@edk2.groups.io to automatically receive all group messages.