From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web10.7466.1576653269158974757 for ; Tue, 17 Dec 2019 23:14:29 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: ashraf.javeed@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 fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Dec 2019 23:14:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,328,1571727600"; d="scan'208";a="390093405" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga005.jf.intel.com with ESMTP; 17 Dec 2019 23:14:28 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Dec 2019 23:14:28 -0800 Received: from bgsmsx103.gar.corp.intel.com (10.223.4.130) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Dec 2019 23:14:27 -0800 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.143]) by BGSMSX103.gar.corp.intel.com ([169.254.4.207]) with mapi id 14.03.0439.000; Wed, 18 Dec 2019 12:44:25 +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 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: AQHVkMaCz34rWzgvlUCjlXpiQanw+qeIgtgAgDWhUgCAAY/NQA== Date: Wed, 18 Dec 2019 07:14:24 +0000 Message-ID: <95C5C2B113DE604FB208120C742E98245797C292@BGSMSX101.gar.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> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3A05B7@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTZiZmZiYmUtNGMyMS00NDY3LWE2MDktYWUyOWZlNmFkYTc4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiXC96aGlrOXA2Rm93UnFmM3JxXC95NUdoSmVtZENRQzJRdnkyWUJTVGlaWmpGTVJUalwvZDFqcGprVU1POEVkaWZVZCJ9 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 Thanks for the review, Ray! My response in line > -----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 >=20 > Please check comments below. > I may have more comments regarding to the four phases after I finish revi= ew of > further patches. >=20 > Besides the comments below, I have a general comments to the debug > message: can you please review the existing debug message in the PciBus d= river > and make sure your newly added debug message aligns to existing style. An= d 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; >=20 > 1. Please follow existing linked list usage style. The first node in the = list is an > empty header node. >=20 > LIST_ENTRY mPrimaryRootPortList; > LIST_ENTRY mPciFeaturesConfigurationCompletionList; >=20 Ok, will make the change when I incorporate the ECR 0.75 or greater version= . > > > +BOOLEAN > > > +CheckPciFeatureConfigurationRecordExist ( > > > + IN PCI_IO_DEVICE *RootBridge, > > > + OUT PCI_FEATURE_CONFIGURATION_COMPLETION_LIST > > > +**PciFeatureConfigRecord > > > + ) >=20 > 2. Is this function to check whether the PCIE features under a root bridg= e is > already initialized? > Can you use the existing variable gFullEnumeration? > The variable is set to TRUE when the enumeration is done to a host bridge= in the > first time. > By using gFullEnumeration, the entire function is not needed. >=20 Ok, will look into this. > > > +EFI_STATUS AddRootBridgeInPciFeaturesConfigCompletionList ( > > > + IN PCI_IO_DEVICE *RootBridge, > > > + IN BOOLEAN ReEnumerationRequired > > > + ) >=20 > 3. Same question as #2. I think by using gFullEnumeration, this function = is not > needed. >=20 OK >=20 > > > +BOOLEAN > > > +IsPciRootPortEmpty ( > > > + IN PCI_IO_DEVICE *PciDevice > > > + ) >=20 > 4. Please use IsListEmpty() directly from callers and remove this functio= n. >=20 Will consider this. > > > +**/ > > > +EFI_STATUS > > > +EnumerateOtherPciFeatures ( >=20 > 5. Can it be "EnumeratePcieFeatures"? >=20 Yes, with the change to ECR 0.75, this routine name shall be changed. > > > + 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"")); >=20 > 6. Please use DEBUG_CODE macro to include ConvertDevicePathToText() and > DEBUG(). > Please remember to call FreePool(). >=20 Ok, will can under DEBUG_CODE, and free pool is called in the end > > > + > > > + for ( OtherPciFeatureConfigPhase =3D PciFeatureRootBridgeScan > > > + ; OtherPciFeatureConfigPhase <=3D PciFeatureConfigurationCompl= ete > > > + ; OtherPciFeatureConfigPhase++ > > > + ) { > > > + switch (OtherPciFeatureConfigPhase){ > > > + case PciFeatureRootBridgeScan: > > > + SetupPciFeaturesConfigurationDefaults (); > > > + // > > > + //first scan the entire root bridge heirarchy for the > > > + primary PCI root > > ports > > > + // > > > + RecordPciRootPortBridges (RootBridge); >=20 > 7. How about "RecordPciRootPorts (RootBridge)"? The "Bridges" suffix is a= bit > confusing. >=20 Fine, will change. > > > + case PciFeatureGetDevicePolicy: > > > + case PciFeatureSetupPhase: >=20 > 8. In SetupPciFeatures(), why do you need to call DeviceExist()? > Did you see any case that a device is detected in the beginning of PciBus= scan > but is hidden when calling SetupPciFeatures()? >=20 Yes, that is the case; device detected during the beginning of PciBus scan = appears to be hidden by the platform drivers, since numerous legacy callbac= ks are initiated at different phase of PCI enumeration to the PCI Host Brid= ge, and PciPlatform drivers. This can be avoided if the PciBus driver is enhanced to check for PCI devic= e existence before the publication of the PCI IO Protocol, and removal of t= he PCI_IO_DEVICE instance from the linked list. > 9. In GetPciFeaturesConfigurationTable() when checking whether a PCI devi= ce > 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. > } >=20 Ok. > > > + Status =3D ProgramPciFeatures (RootBridge); > 10. ProgramPcieFeatures()? >=20 OK > > > + > > > + if (Str !=3D NULL) { > > > + FreePool (Str); > > > + } >=20 > 11. OK the Str is freed here because Str is needed for other debug messag= es > inside the function. >=20 Yes > > > + // > > > + // mark this root bridge as PCI features configuration complete, > > > +and no new > > > + // enumeration is required > > > + // > > > + AddRootBridgeInPciFeaturesConfigCompletionList (RootBridge, > > > +FALSE); >=20 > 12. Not needed. >=20 ok, after incorporating the logic of gFullEnumeration it won't be required > > > +_PRIMARY_ROOT_PORT_NODE { >=20 > > > + // > > > + // 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; > > > +}; >=20 > 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. >=20 I think it is better to maintain separately as this configuration table is = confined to a group of PCI devices and for the RCiEP it is not applicable h= ence not required. Moreover, I am maintaining a variable for each PCIe feat= ure 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.