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.6924.1581405309485314207 for ; Mon, 10 Feb 2020 23:15:09 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: ashraf.javeed@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; 10 Feb 2020 23:15:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,427,1574150400"; d="scan'208";a="233368415" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 10 Feb 2020 23:15:08 -0800 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 10 Feb 2020 23:15:08 -0800 Received: from bgsmsx105.gar.corp.intel.com (10.223.43.197) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 10 Feb 2020 23:15:08 -0800 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.159]) by BGSMSX105.gar.corp.intel.com ([169.254.3.119]) with mapi id 14.03.0439.000; Tue, 11 Feb 2020 12:44:24 +0530 From: "Javeed, Ashraf" To: "Ni, Ray" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase Thread-Index: AQHV3fIQbOuO9DDKrEu6zJtNcxXaxagQK1gAgAOGYwCAAGokUP//qS0AgABo+rA= Date: Tue, 11 Feb 2020 07:14:23 +0000 Message-ID: <95C5C2B113DE604FB208120C742E9824579AB919@BGSMSX101.gar.corp.intel.com> References: <20200207200447.10536-1-ashraf.javeed@intel.com> <15F137802DC889C4.7869@groups.io> <95C5C2B113DE604FB208120C742E9824579AAF7C@BGSMSX101.gar.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C42933D@SHSMSX104.ccr.corp.intel.com> <95C5C2B113DE604FB208120C742E9824579AB46F@BGSMSX101.gar.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C429588@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C429588@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2I3MmVmMjctY2U4Mi00OGRhLWJmMWQtOGFjYWZhMmFkNTgwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiKzNcL1RiV00zZ0w2WEhZZldBaDJ0RmtWME8yVVdORzQ1dUJhUHlLUlpXVXZUZTlrZlwvbUxnU01xU3Axb05HXC9zcSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.223.10.10] MIME-Version: 1.0 Return-Path: ashraf.javeed@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Ni, Ray > Sent: Monday, February 10, 2020 2:16 PM > To: Javeed, Ashraf ; devel@edk2.groups.io > Cc: Wang, Jian J ; Wu, Hao A > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12] > MdeModulePkg/PciBusDxe: Setup PCI Express init phase >=20 > My proposal defines four phases: Initialize, Scan, Program, Finalize. The= four > phases are very similar to your December's patch with just one phase opt = out. >=20 > As below: > ---BEGIN---- > Each feature needs to provide function pointers for each phase and NULL m= eans > the feature doesn't need to do anything in the specific phase. > With that, we can define a structure: > Typedef struct { > BOOLEAN Enable; > PCIE_FEATURE_INITILAIZE Initialize; > PCIE_FEATURE_SCAN Scan; > PCIE_FEATURE_PROGRAM Program; > PCIE_FEATURE_FINALIZE Finalize; > } PCIE_FEATURE_ENTRY; >=20 > With that, we can define a module level global variable: > PCIE_FEATURE_ENTRY mPcieFeatures[] =3D { > { TRUE, MaxPayloadInitialize, MaxPayloadScan, MaxPayloadProgram, > MaxPayloadFinalize}, > { TRUE, MaxReadRequestInitialize, MaxReadRequestScan, > MaxReadRequestProgram, MaxReadRequestFinalize}, > { TRUE, NULL, NULL, RelaxOrderProgram, NULL}, > { TRUE, NULL, CompletionTimeoutScan, CompletionTimeoutProgram, NULL }, > ... > }; >=20 > PCIE_FEATURE_ENTRY.Enable can be set to FALSE according to the platform > policy. >=20 > The enable of PCIE features can be written as a feature agnostic for-loop= . > This can make the new feature enabling code easy to add and review. > ---END---- >=20 I have seen this, in essence the concept is adopted, but it has been furthe= r enhanced to achieve the objectives. > While your today's patch defines a different 5-phase as below. > typedef enum { // // preprocessing applicable only to few PCI Express features to bind all = devices // under the common root bridge device (root port), that would be useful = to align // all devices with a common value. This would be optional phase based on= the // type of the PCI Express feature to be programmed based on platform pol= icy // > PciExpressFeaturePreProcessPhase, // // mandatory phase to setup the PCI Express feature to its applicable att= ribute, // based on its device-specific platform policies, matching with its devi= ce capabilities // > PciExpressFeatureSetupPhase, // // optional phase primarily to align all devices, specially required when= PCI // switch is present in the hierarchy, applicable to certain few PCI Expr= ess // features only // > PciExpressFeatureEntendedSetupPhase, // // mandatory programming phase to complete the configuration of the PCI E= xpress // features // > PciExpressFeatureProgramPhase, // // optional phase to clean up temporary buffers, like those that were pre= pared // during the preprocessing phase above // > PciExpressFeatureEndPhase > } PCI_EXPRESS_FEATURE_CONFIGURATION_PHASE; >=20 > I don't think they are very similar. And your new proposal is far differe= nt from > what you proposed in December's patch. > Did you meet any issues that triggered this new proposal? >=20 The details are as follows:- (1) Your proposal of data structure PCIE_FEATURE_ENTRY will add up 1 byte f= or Boolean and 8 * 4 bytes for 4 different types of function pointers. For = 10 PCIe features, it is going to take 330 bytes fixed size. (2) I wanted to optimized to avoid fixed size, added the phase, and feature= Id enum (4 bytes each), with just single pointer to function for the featu= re initialization phase into the data structure PCI_EXPRESS_FEATURE_INITIAL= IZATION_POINT, since I know that many don't need all the phases for initial= ization. So, the total size turn outs to be 400 bytes, which is not what I = had desired. But, I can further optimized the data structure PCI_EXPRESS_FE= ATURE_INITIALIZATION_POINT so that its size can be 10 bytes. With this, for= 10 PCIe features initialization the routines defined in the "mPciExpressFe= atureInitializationList " shall be 250 bytes, shall be less compare to usin= g the PCIE_FEATURE_ENTRY; this can be in next version. (3) The pointer-to-function name to be used as subphases appears fine for P= CIe feature specific action, but not for general action items, like free up= of temporary memory allocated resources. Explicit declarations has helped = in optimizing a code for both feature specific and for common action items,= which the subsequent patches reflect those. (4) The phase names are matching as per their scope of actions, whose comme= nts in the code section are also captured (above).=20 (5) As per the nature of the PCIe feature to align common value among all t= he PCI devices, every node in the tree has to be traversed twice to arrive = at final value for the initialization (especially required for the PCIe swi= tch multi-port device, to connect to other EndPoint devices), and the next = phase would be the actual write to the PCI device. The phases for traversin= g are the - PciExpressFeatureSetupPhase, PciExpressFeatureEntendedSetupPhas= e; and write operation phase is - PciExpressFeatureProgramPhase. (6) The preprocessing phase is - PciExpressFeaturePreProcessPhase, where th= e resources are allocated specific to PCIe feature, that needs to be bind t= o every root bridge device.=20 (7) The final phase is - PciExpressFeatureEndPhase; this phase is use for 2= purpose: (i) to invoke the NotifyDeviceState() to indicate to platform abo= ut its current device state after the PCIe feature programming is complete,= and (ii) free up the resource allocated in the preprocessing phase. > Thanks, > Ray >=20 > > -----Original Message----- > > From: Javeed, Ashraf > > Sent: Monday, February 10, 2020 4:33 PM > > To: Ni, Ray ; devel@edk2.groups.io > > Cc: Wang, Jian J ; Wu, Hao A > > > > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH > > 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase > > > > Thanks > > Ashraf > > > > > -----Original Message----- > > > From: Ni, Ray > > > Sent: Monday, February 10, 2020 1:07 PM > > > To: Javeed, Ashraf ; devel@edk2.groups.io > > > Cc: Wang, Jian J ; Wu, Hao A > > > > > > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH > > > 02/12] > > > MdeModulePkg/PciBusDxe: Setup PCI Express init phase > > > > > > > > + Status =3D EnumeratePciExpressFeatures ( > > > 1. "enumerate" means "visit". But I think this function is not just > > > visiting the features but also > > > programming them. So, How about "ProgramPciExpressFeatures"? > > > (I gave a similar review comment in last time review in Dec.) > > > > > Actually I have already used the "ProgramPciExpressFeatures" in one of > > the sub-phases of the PCI Express initialization code...do you have any= other > name for this? How about "ConfigurePciExpressFeatures"? > > > > > 2. In mail https://edk2.groups.io/g/devel/message/52399 I proposed > > > to simplify to 4 phases. > > > Did you find any issue with my proposal? > > I did simplify to 4 phases, please check. The fifth phase is to report > > out the device state to the platform through the protocol interface > "NotifyDeviceState". > > > > > > > > Thanks, > > > Ray