From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web11.5405.1575526175958560897 for ; Wed, 04 Dec 2019 22:09:36 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Dec 2019 22:09:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,279,1571727600"; d="scan'208";a="214082840" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga003.jf.intel.com with ESMTP; 04 Dec 2019 22:09:35 -0800 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Dec 2019 22:09:34 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.195]) with mapi id 14.03.0439.000; Thu, 5 Dec 2019 14:09:32 +0800 From: "Dong, Eric" To: "Ni, Ray" , "devel@edk2.groups.io" CC: Laszlo Ersek Subject: Re: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time Thread-Topic: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time Thread-Index: AQHVqw5f6HPGAC1geEy6SqFBXpTFHKeq/iuQgAAOUDA= Date: Thu, 5 Dec 2019 06:09:32 +0000 Message-ID: References: <20191205015023.607-1-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C39021F@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C39021F@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: eric.dong@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray, > -----Original Message----- > From: Ni, Ray > Sent: Thursday, December 5, 2019 1:25 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Laszlo Ersek > Subject: RE: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate > Token every time >=20 > Some comments. > > + // > > > > + // Not free the buffer, just clear the UsedTokenNum. In order to > > > > + // avoid triggering allocate action when we need to use the token, > > > > + // do not free the buffer. > 1. Can you reword to below? > "Only free the token buffer recorded in the OldTOkenBufList upon exiting > SMI. > Current token buffer stays allocated so next SMI doesn't need to re- > allocate." [[Eric]] will change to this in next version. >=20 > > + ASSERT_EFI_ERROR (TokenCountPerChunk !=3D 0); >=20 > 2. This is not right. Should use ASSERT(). [[Eric]] Will update in next version. >=20 > > + DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size =3D 0x%x, > > PreAllocateTokenNum =3D 0x%x\n", SpinLockSize, TokenCountPerChunk)); >=20 > 3. "PreAllocateTokenNum" -> "PcdTokenCountPerChunk". Please directly > use the PCD name in debug log. [[Eric]] will update it in next version. >=20 > > > > + UINT8 *CurrentTokenBuf; > > > > + UINTN UsedTokenNum; // Only record tok= ens used in > > CurrentTokenBuf. >=20 > 4. I see a mismatch between types of UsedTokenNum and the PCD. One is > UINTN, the other is UINT32. > Better to use UINT32 here. [[Eric]] will use UINT32 in next version. >=20 > > > gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk|64|UINT32|0x3000 > 2002 >=20 > 5. The PCD name is too generic. It's hard to tell when/where the PCD is u= sed > by reading the name only. > Maybe "PcdCpuSmmMpTokenCountPerChunk"? [[Eric]] will use this name in next version. >=20 > > +#string > > STR_gUefiCpuPkgTokenSpaceGuid_PcdTokenCountPerChunk_PROMPT > > #language en-US "Specify the size of pre allocated token count per > chunk.\n" >=20 > 6. "Size of pre allocated token count per chunk" is a bit confusing. > How about "Count of pre allocated SMM MP tokens"? [[Eric]] will update to "Count of pre allocated SMM MP tokens per chunk " > Then maybe the PCD name can be "PcdCpuSmmMpPreAllocateTokenCount"? [[Eric]] I think " PcdCpuSmmMpTokenCountPerChunk " is better and will use i= t. Thanks, Eric >=20 > Thanks, > Ray