From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web09.10827.1582872443733394627 for ; Thu, 27 Feb 2020 22:47:24 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Feb 2020 22:47:22 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,493,1574150400"; d="scan'208";a="261771640" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga004.fm.intel.com with ESMTP; 27 Feb 2020 22:47:22 -0800 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 27 Feb 2020 22:47:22 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 27 Feb 2020 22:47:21 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.206]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.235]) with mapi id 14.03.0439.000; Fri, 28 Feb 2020 14:47:19 +0800 From: "Ni, Ray" To: "Duran, Leo" , Laszlo Ersek , "devel@edk2.groups.io" , "Wu, Hao A" , "Fu, Siyuan" CC: "Dong, Eric" Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib Thread-Topic: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib Thread-Index: AQHV7B0pGPIUTxhG5EqTSXQjAjWqRagsIFiAgAD60nD///hQAIAABf+AgAAJk4CAAAVYgIAAEk2AgAABuACAAU/CYIAASbcAgAFTp2A= Date: Fri, 28 Feb 2020 06:47:18 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C46BE23@SHSMSX104.ccr.corp.intel.com> References: <1582659566-9893-1-git-send-email-leo.duran@amd.com> <734D49CCEBEEF84792F5B80ED585239D5C4542DA@SHSMSX104.ccr.corp.intel.com> <444c59ea-70dc-0edd-d680-add054dad2c5@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C463F32@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Leo, Thanks for your quick reply. My most concern part to your patch is to expose a new API in LocalApicLib c= hecking whether the processor is AMD type. LocalApicLib is a library that o= perates on Local APIC registers while checking CPU type deals with CPUID si= gnature. It's not proper to mix the two things together just because LocalA= picLib needs to know CPU type in some of the operation. Because BaseLib already provides AsmCpuid() API and the check of CPU ID sig= nature 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 micro= code 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 > Sent: Friday, February 28, 2020 2:17 AM > To: Ni, Ray ; Laszlo Ersek ; > devel@edk2.groups.io; Wu, Hao A ; Fu, Siyuan > > Cc: Dong, Eric > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpIn= itLib >=20 >=20 >=20 > > -----Original Message----- > > From: Ni, Ray [mailto:ray.ni@intel.com] > > Sent: Thursday, February 27, 2020 12:55 AM > > To: Duran, Leo ; Laszlo Ersek ; > > devel@edk2.groups.io; Wu, Hao A ; Fu, Siyuan > > > > Cc: Dong, Eric > > 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=3Dhttps%3A%2F%2Fbug= zill > > a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&data=3D02%7C01%7Cleo. > > duran%40amd.com%7C40786a41798d4173dd8508d7bb49aaa7%7C3dd8961 > > fe4884e608e11a82d994e183d%7C0%7C0%7C637183797396444421&s > > data=3DUoLRg%2ByFl%2BxyPB41xu1wOHpsf2euBrSe2HuD4CskTWg%3D&r > > eserved=3D0 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 hoo= ks > (ShadowMicrocode PPI, et al) are not required. > (Hence my comments in the MpInitLib patch I just submitted) >=20 > > > > > -----Original Message----- > > > From: Duran, Leo > > > Sent: Thursday, February 27, 2020 1:52 AM > > > To: Laszlo Ersek ; Ni, Ray ; > > > devel@edk2.groups.io; Wu, Hao A ; Fu, Siyuan > > > > > > Cc: Dong, Eric > > > 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 ; Ni, Ray ; > > > > devel@edk2.groups.io; Wu, Hao A ; Fu, Siyuan > > > > > > > > Cc: Dong, Eric > > > > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug i= n > > > > 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 ; Ni, Ray ; > > > > >> devel@edk2.groups.io; Wu, Hao A ; Fu, Siyuan > > > > >> > > > > >> Cc: Dong, Eric > > > > >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bu= g > > > > >> 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 usag= e > > > > >>> 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 =3D > > FALSE; > > > > >>> else { > > > > >>> PlatformIdMsr.Uint64 =3D AsmReadMsr64 > > (MSR_IA32_PLATFORM_ID); > > > > >>> CpuMpData->CpuData[ProcessorNumber].PlatformId =3D > > > > >> (UINT8)PlatformIdMsr.Bits.PlatformId; > > > > >>> CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId =3D T= RUE; > > > > >>> } > > > > >>> > > > > >>> This way "IsValidPlatformId" could be checked prior to using > > "PlatformId". > > > > >>> Anyway, that seemed a bit overkill, so I opted against it... th= oughts? > > > > >> > > > > >> 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 glob= al 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