From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web10.6459.1576734531399759534 for ; Wed, 18 Dec 2019 21:48:51 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Dec 2019 21:48:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,330,1571727600"; d="scan'208";a="390414565" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga005.jf.intel.com with ESMTP; 18 Dec 2019 21:48:50 -0800 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 21:48:50 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 18 Dec 2019 21:48:49 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.214]) with mapi id 14.03.0439.000; Thu, 19 Dec 2019 13:48:47 +0800 From: "Ni, Ray" 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 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration Thread-Index: AQHVmdJK3T9Y1rxzHkawV8SqYkiDX6e92zmAgAFQkwCAAfiR8A== Date: Thu, 19 Dec 2019 05:48:47 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3A2938@SHSMSX104.ccr.corp.intel.com> References: <20191101150952.3340-1-ashraf.javeed@intel.com> <15D3127B934F51D3.12315@groups.io> <95C5C2B113DE604FB208120C742E9824579171C4@BGSMSX101.gar.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3A05B7@SHSMSX104.ccr.corp.intel.com> <95C5C2B113DE604FB208120C742E98245797C292@BGSMSX101.gar.corp.intel.com> In-Reply-To: <95C5C2B113DE604FB208120C742E98245797C292@BGSMSX101.gar.corp.intel.com> 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 After I reviewed the patch of enabling MaxPayloadSize, MaxReadReqSize and m= ore PCIE features, I can now understand the phases more than earlier. Your patch proposed five phases: // // initial phase in configuring the other PCI features to record the prim= ary // root ports // PciFeatureRootBridgeScan, // // get the PCI device-specific platform policies and align with device ca= pabilities // PciFeatureGetDevicePolicy, // // align all PCI nodes in the PCI heirarchical tree // PciFeatureSetupPhase, // // finally override to complete configuration of the PCI feature // PciFeatureConfigurationPhase, // // PCI feature configuration complete // PciFeatureConfigurationComplete I have several comments to the five phases. 1. Scan phase is not do the scanning but creates a list to hold all root po= rts under the root bridge. 2. Root ports collection is not required by all of the features, only by MP= S and MRRS. But the collection always happens even when platform doesn't require PciBus= to initialize MPS or MRRS. 3. GetDevicePolicy phase is not just call the GetDevicePolicy for each devi= ce. It also reads the PCIE configuration space to get the device's feature related capabiliti= es, for some of the features. With that, I propose to define 4 phases: 1. Initialize phase This phase is similar to your Scan phase. For some features, this phase does nothing. For MPS and MRRS, this phase creates a list holding all root ports. 2. Scan phase This phase is similar to your GetDevicePolicy phase. For some features, this phase needs nothing do to. For MPS and MRRS, this phase scan all devices and get the aligned value of = MPS or MRRS. 3. Program phase or Configuration phase This phase is similar to your Configuration phase. The Setup phase can be merged to this phase. 4. Finalize phase. This phase is similar to your ConfigurationComplete phase. This phase frees the resources occupied/allocated in Initialize phase. For some of the features, this phase may do nothing. Each feature needs to provide function pointers for each phase and NULL mea= ns 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; With that, we can define a module level global variable: PCIE_FEATURE_ENTRY mPcieFeatures[] =3D { { TRUE, MaxPayloadInitialize, MaxPayloadScan, MaxPayloadProgram, MaxPaylo= adFinalize}, { TRUE, MaxReadRequestInitialize, MaxReadRequestScan, MaxReadRequestProgr= am, MaxReadRequestFinalize}, { TRUE, NULL, NULL, RelaxOrderProgram, NULL}, { TRUE, NULL, CompletionTimeoutScan, CompletionTimeoutProgram, NULL }, ... }; PCIE_FEATURE_ENTRY.Enable can be set to FALSE according to the platform pol= icy. 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. > -----Original Message----- > From: Javeed, Ashraf > Sent: Wednesday, December 18, 2019 3:14 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 05/12] > PciBusDxe: Setup sub-phases for PCI feature enumeration >=20 > Thanks for the review, Ray! > My response in line >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Tuesday, December 17, 2019 5:26 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 > 05/12] > > PciBusDxe: Setup sub-phases for PCI feature enumeration > > > > Please check comments below. > > I may have more comments regarding to the four phases after I finish > review of > > further patches. > > > > Besides the comments below, I have a general comments to the debug > > message: can you please review the existing debug message in the PciBus > driver > > and make sure your newly added debug message aligns to existing style. > And try > > to use less lines of debug messages with still enough debug information= . > Ok, will look into that. >=20 > > > > > > +PRIMARY_ROOT_PORT_NODE *mPrimaryRootPortList; > > > > + > > > > +/** > > > > + A global pointer to > PCI_FEATURE_CONFIGURATION_COMPLETION_LIST, > > > > which > > > > +stores all > > > > + the PCI Root Bridge instances that are enumerated for the other > > > > +PCI features, > > > > + like MaxPayloadSize & MaxReadReqSize; during the the Start() > > > > +interface of the > > > > + driver binding protocol. The records pointed by this pointer > > > > +would be destroyed > > > > + when the DXE core invokes the Stop() interface. > > > > +**/ > > > > +PCI_FEATURE_CONFIGURATION_COMPLETION_LIST > > > > *mPciFeaturesConfigurationCompletionList =3D NULL; > > > > 1. Please follow existing linked list usage style. The first node in th= e list is an > > empty header node. > > > > LIST_ENTRY mPrimaryRootPortList; > > LIST_ENTRY mPciFeaturesConfigurationCompletionList; > > > Ok, will make the change when I incorporate the ECR 0.75 or greater versi= on. >=20 > > > > +BOOLEAN > > > > +CheckPciFeatureConfigurationRecordExist ( > > > > + IN PCI_IO_DEVICE *RootBridge, > > > > + OUT PCI_FEATURE_CONFIGURATION_COMPLETION_LIST > > > > +**PciFeatureConfigRecord > > > > + ) > > > > 2. Is this function to check whether the PCIE features under a root bri= dge is > > already initialized? > > Can you use the existing variable gFullEnumeration? > > The variable is set to TRUE when the enumeration is done to a host brid= ge > in the > > first time. > > By using gFullEnumeration, the entire function is not needed. > > > Ok, will look into this. >=20 > > > > +EFI_STATUS AddRootBridgeInPciFeaturesConfigCompletionList ( > > > > + IN PCI_IO_DEVICE *RootBridge, > > > > + IN BOOLEAN ReEnumerationRequired > > > > + ) > > > > 3. Same question as #2. I think by using gFullEnumeration, this functio= n is > not > > needed. > > > OK >=20 > > > > > > +BOOLEAN > > > > +IsPciRootPortEmpty ( > > > > + IN PCI_IO_DEVICE *PciDevice > > > > + ) > > > > 4. Please use IsListEmpty() directly from callers and remove this funct= ion. > > > Will consider this. >=20 > > > > +**/ > > > > +EFI_STATUS > > > > +EnumerateOtherPciFeatures ( > > > > 5. Can it be "EnumeratePcieFeatures"? > > > Yes, with the change to ECR 0.75, this routine name shall be changed. >=20 > > > > + IN PCI_IO_DEVICE *RootBridge > > > > + ) > > > > +{ > > > > + EFI_STATUS Status; > > > > + UINTN OtherPciFeatureConfigPhase; > > > > + > > > > + // > > > > + // check on PCI features configuration is complete and > > > > + re-enumeration is required // if > > > > + (!CheckPciFeaturesConfigurationRequired > > > > + (RootBridge)) { > > > > + return EFI_ALREADY_STARTED; > > > > + } > > > > + > > > > + CHAR16 *Str; > > > > + Str =3D ConvertDevicePathToText ( > > > > + DevicePathFromHandle (RootBridge->Handle), > > > > + FALSE, > > > > + FALSE > > > > + ); > > > > + DEBUG ((DEBUG_INFO, "Enumerating PCI features for Root Bridge > > > > + %s\n", Str !=3D NULL ? Str : L"")); > > > > 6. Please use DEBUG_CODE macro to include ConvertDevicePathToText() > and > > DEBUG(). > > Please remember to call FreePool(). > > > Ok, will can under DEBUG_CODE, and free pool is called in the end >=20 > > > > + > > > > + for ( OtherPciFeatureConfigPhase =3D PciFeatureRootBridgeScan > > > > + ; OtherPciFeatureConfigPhase <=3D > PciFeatureConfigurationComplete > > > > + ; OtherPciFeatureConfigPhase++ > > > > + ) { > > > > + switch (OtherPciFeatureConfigPhase){ > > > > + case PciFeatureRootBridgeScan: > > > > + SetupPciFeaturesConfigurationDefaults (); > > > > + // > > > > + //first scan the entire root bridge heirarchy for the > > > > + primary PCI root > > > ports > > > > + // > > > > + RecordPciRootPortBridges (RootBridge); > > > > 7. How about "RecordPciRootPorts (RootBridge)"? The "Bridges" suffix is= a > bit > > confusing. > > > Fine, will change. >=20 > > > > + case PciFeatureGetDevicePolicy: > > > > + case PciFeatureSetupPhase: > > > > 8. In SetupPciFeatures(), why do you need to call DeviceExist()? > > Did you see any case that a device is detected in the beginning of PciB= us > scan > > but is hidden when calling SetupPciFeatures()? > > > Yes, that is the case; device detected during the beginning of PciBus sca= n > appears to be hidden by the platform drivers, since numerous legacy > callbacks are initiated at different phase of PCI enumeration to the PCI = Host > Bridge, and PciPlatform drivers. > This can be avoided if the PciBus driver is enhanced to check for PCI dev= ice > existence before the publication of the PCI IO Protocol, and removal of t= he > PCI_IO_DEVICE instance from the linked list. >=20 > > 9. In GetPciFeaturesConfigurationTable() when checking whether a PCI > device > > belongs to a root port, we can use below simpler logic: > > SizeOfPciDevicePath =3D GetDevicePathSize (PciDevicePath); > > SizeOfRootPortDevicePath =3D GetDevicePathSize (RootPortPath); > > if ((SizeOfRootPortDevicePath < SizeOfPciDevicePath) && > > CompareMem (PciDevicePath, RootPortPath, > SizeOfRootPortDevicePath - > > END_DEVICE_PATH_LENGTH) =3D=3D 0)) { > > // PCI device belongs to the root port. > > } > > > Ok. >=20 > > > > + Status =3D ProgramPciFeatures (RootBridge); > > 10. ProgramPcieFeatures()? > > > OK >=20 > > > > + > > > > + if (Str !=3D NULL) { > > > > + FreePool (Str); > > > > + } > > > > 11. OK the Str is freed here because Str is needed for other debug > messages > > inside the function. > > > Yes >=20 > > > > + // > > > > + // mark this root bridge as PCI features configuration complete, > > > > +and no new > > > > + // enumeration is required > > > > + // > > > > + AddRootBridgeInPciFeaturesConfigCompletionList (RootBridge, > > > > +FALSE); > > > > 12. Not needed. > > > ok, after incorporating the logic of gFullEnumeration it won't be require= d >=20 > > > > +_PRIMARY_ROOT_PORT_NODE { > > > > > > + // > > > > + // Signature header > > > > + // > > > > + UINT32 Signature; > > > > + // > > > > + // linked list pointers to next node > > > > + // > > > > + LIST_ENTRY NeighborRootPort; > > > > + // > > > > + // pointer to PCI_IO_DEVICE of the primary PCI Controller device > > > > + // > > > > + EFI_DEVICE_PATH_PROTOCOL *RootPortDevicePath; > > > > + // > > > > + // pointer to the corresponding PCI feature configuration Table > > > > +node > > > > + // all the child PCI devices of the controller are aligned based > > > > +on this table > > > > + // > > > > + OTHER_PCI_FEATURES_CONFIGURATION_TABLE > > > > *OtherPciFeaturesConfigurationTable; > > > > +}; > > > > 13. Can you add the OTHER_PCI_FEATURES_CONFIGURATION_TABLE field > to > > PCI_IO_DEVICE structure? > > So this structure PRIMARY_ROOT_PORT_NODE is not needed. > > > I think it is better to maintain separately as this configuration table i= s confined > to a group of PCI devices and for the RCiEP it is not applicable hence no= t > required. Moreover, I am maintaining a variable for each PCIe feature in = the > PCI_IO_DEVICE; perhaps I can consider having just pointer of it.... >=20 > > > > +struct _PCI_FEATURE_CONFIGURATION_COMPLETION_LIST { > > 14. This structure is not needed if using gFullEnumeration. > Yes.