From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web12.791.1584429609633227348 for ; Tue, 17 Mar 2020 00:20:09 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: ashraf.javeed@intel.com) IronPort-SDR: 02VLNjSqFVg0ugN+j0ugM8IY4UDx+vk2CvtS/9cLe+k6d6Q45H9Ec1Dp5eyATr+ZlrpNlRejHa 7t3W8cX7KmFA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Mar 2020 00:20:08 -0700 IronPort-SDR: mVktGBFXOOxYbuv4O3Rphyb1VDJJp5qFsJywQ9Rv81dSBcFIZLmehlwNIIC/MIzVFguXlDJg8Z azKzpfkZ8efA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,563,1574150400"; d="scan'208";a="236250241" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga007.fm.intel.com with ESMTP; 17 Mar 2020 00:20:08 -0700 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Mar 2020 00:20:07 -0700 Received: from bgsmsx104.gar.corp.intel.com (10.223.4.190) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Mar 2020 00:20:06 -0700 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.228]) by BGSMSX104.gar.corp.intel.com ([169.254.5.150]) with mapi id 14.03.0439.000; Tue, 17 Mar 2020 12:50:03 +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+qe92zmAgAFQkwCAAfiR8IB5l2IAgBD+mtD///5hgIABSuYQ Date: Tue, 17 Mar 2020 07:20:03 +0000 Message-ID: <95C5C2B113DE604FB208120C742E9824579C7F04@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> <95C5C2B113DE604FB208120C742E98245797C292@BGSMSX101.gar.corp.intel.com> <15E1AFB3EABD031C.30484@groups.io> <734D49CCEBEEF84792F5B80ED585239D5C47E0CF@SHSMSX104.ccr.corp.intel.com> <95C5C2B113DE604FB208120C742E9824579C6B6A@BGSMSX101.gar.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C49D05C@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C49D05C@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 My response below. Thanks Ashraf > -----Original Message----- > From: Ni, Ray > Sent: Monday, March 16, 2020 7:30 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 > Ashraf, > Thanks for taking time to review my code! Comments embedded in below. >=20 > > -----Original Message----- > > From: Javeed, Ashraf > > Sent: Monday, March 16, 2020 5:34 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 > > > > Ray, > > First of all, thank you for taking time to review and on optimizing th= e PCIe > feature support in the PciBusDxe. > > > > I shall not discuss everything in detail here, just want to bring two > > major points:- > > > > (1) commit:- MdePkg: New PCI Express Platform/Override Protocols > > > https://github.com/niruiyu/edk2/commit/52524e9654704a7f6a30ca446f2 > 15b8 > > 1fe8f0984 PCI Express Protocol not as per the ECR draft revision 0.8, > > the changes made has to be reviewed with updated ECR version... >=20 > With the code first process, we could firstly finalize the protocol inte= rfaces > in code then update the ECR. > Code first doesn't mean implementation detail has high priority than > protocol interfaces. Good quality of protocol interfaces is still desire= d. The > quality of the protocol header file is always in high priority. >=20 > > the global auto option introduces > EFI_PCI_EXPRESS_DEVICE_POLICY_AUTO; > > is used for MPS, MRRS, RO, NS, AtomicOp, LTR, results in no EFI > > encodings required for these features, as it can be used with actual v= alues > from the PciXX.h definitions. This do result in lot of code savings. >=20 > I take it as an agree of using the global AUTO. Thanks for that. Please note that there is another request of "Do not touch", and I am stil= l contemplating whether both AUTO and DO_NOT_TOUCH options could be applica= ble to all the PCIe features or not. It could be possible to take AUTO for = some of them and DO_NOT_TOUCH for others. Need to consider each PCIe featur= e separately for both these options. =20 >=20 > > > > (2) commit:- MdeModulePkg/PciBus: Add the framework to init PCIE > > features > > > https://github.com/niruiyu/edk2/commit/9fb9a3dcef06de98a76825e2fc0 > 7167 > > 446ee6fd9 The context handling of the PCIe feature MPS has fundamental > > issue as it is set to be common for all the nodes of the parent Root > > Bridge instance. The context has to be separate to each first level > > nodes of a root bridge instance. For example, a root bridge has 1 > > RCiEP and two Root ports, than there has to be 3 separate context for > > each. Since Root port can have its Endpoint device, or it can have a > > PCIe switch with 1 upstream and 2 or more downstream ports and to > each > > downstream port an Endpoint device can be connected; the context > > created for Root Port of an bridge is used for all its child hierarchy= nodes; > thus PCIe feature like Max_Payload_Size value can be specific to each RC= iEP, > and each Root ports of the Root Bridge handle. How do you propose to > maintain separate context for just first level nodes of the Root Bridge > handle? >=20 > Supposing a root bridge contains 1 RCiEP and two root ports, the > implementation ensures there will be 3 separate contexts for each. As yo= u > can see, every time the code starts enumeration from a RCiEP or a root > port, a new Context[] is setup. Context[i] is associated with the featur= e _i_. >=20 I realize now, that there are 3 nested for-loops in the EnumerateRootBridg= ePcieFeatures(), the first one enables the processing of first level nodes = of the Root Bridge instance, and in this way the context[i] used for each n= ode will be valid as it covers each node's hierarchy. >=20 > > > > The EnumeratePcieDevices() is recursively calls itself for every PCI > > node with a level advanced by 1. Thus, the usage of level N and N+1 as > > parent and its child is wrong to me, when you have a PCIe switch below > > the Root Port, and N & N+1 will not be direct relation between the > upstream port and its second downstream port...any reason why you have > not used the PCI_IO_DEVICE.Parent pointer to form relation between the > parent and the child? >=20 > The level N and N+1 is needed for AtomicOp or Ltr feature (which I need = to > go back to check the code). > I want to avoid the individual feature scan/program routine checks devic= e's > parent/children. > I understand that in PCIE spec, a switch consists of an upstream port an= d > several downstream ports. > It means a switch populates a bridge which connects to the upstream and > several bridges under the upstream bridge which connect to the > downstream. > But from software perspective, I don't think we should care about that. > Maybe I am wrong. Can you please give a real example that treating in my > way would cause some issues? >=20 Let just say we have PCIe switch connected to Root Port on a host, and it = has 2 downstream ports, each connected to one Endpoint device each. Platfor= m has selected to enable LTR for second downstream port's endpoint device, = not both. As per your code logic of LTR, where you use the N and N+1 to com= pare between the parent and child, will not be applicable if you use the sa= me condition to check downstream port (N+1), and its parent upstream port, = because N would be the Endpoint device of first downstream port of the PCIe= switch and not its upstream port. Root Port -> =09 =09 1|0|0 Switch upstream port -> =09 =092|0|0 (M-1) Switch downstream ports -> 3|0|0,(M) =095|0|0 (N (=3DM+2)) Endpoint devices -> =094|0|0 (M+1) =096|0|0 (N+1 (=3DM+3)) The level+1 logic when the EnumeratePcieDevices() is called recursively, w= ill result in one value greater than the Endpoint device for the second dow= nstream port.=20 > > > > I liked the Pre & Post order flag that is used to processing parent-to= -child > vs. child-to-parent nodes. >=20 > Actually I didn't have this flag in the first version of my code change.= But > later I found it's needed for some features like LTR which needs to be > programmed from rootbridge to endpoint according to spec. >=20 >=20 > > You are calling the first > > and last features as fakes, when you actually parse all the nodes for > > the GetDevicePolicy() and NotifyDeviceState(); to me it is actually an > > un-named phase where all the nodes are queried or informed to the > platform; that seems fine to save the NULL on the mPcieFeatures[] table. >=20 > Yes I put GetDevicePolicy() and NotifyDeviceState() in the feature array= in > my first version of code. And later I called the two separately for bett= er > code readability. But it was my fault that I didn't update the commit > message. It caused confusion to you. >=20 > > > > Thanks > > Ashraf > > > > > -----Original Message----- > > > From: Ni, Ray > > > Sent: Thursday, March 5, 2020 7:43 PM > > > To: devel@edk2.groups.io; Ni, Ray ; Javeed, Ashraf > > > > > > 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 > > > > > > Ashraf, > > > I think it might be better to describe my review comments with code > > > implementation. > > > Can you please check this branch where I did some modification based > > > on your code? > > > https://github.com/niruiyu/edk2/tree/pci/pcie2 > > > > > > Let's firstly align on the feature initialization framework > implementation. > > > To be specific, this commit: > > > MdeModulePkg/PciBus: Add the framework to init PCIE features > > > > https://github.com/niruiyu/edk2/commit/9fb9a3dcef06de98a76825e2fc0 > > > 7167446ee6fd9 > > > > > > Thanks, > > > Ray > > > > > > > -----Original Message----- > > > > From: devel@edk2.groups.io On Behalf Of > Ni, > > > Ray > > > > Sent: Thursday, December 19, 2019 1:49 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 > > > > > > > > After I reviewed the patch of enabling MaxPayloadSize, > > > > MaxReadReqSize and more 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 > > > primary > > > > // root ports > > > > // > > > > PciFeatureRootBridgeScan, > > > > // > > > > // get the PCI device-specific platform policies and align with > > > > device > > > capabilities > > > > // > > > > 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 ports under the root bridge. > > > > 2. Root ports collection is not required by all of the features, > > > > only by MPS 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 device. It also reads the PCIE configuration space to get the > > > > device's feature related capabilities, 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 ph= ase. > > > > For some of the features, this phase may do nothing. > > > > > > > > Each feature needs to provide function pointers for each phase and > > > > NULL means 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, > > > > MaxPayloadFinalize}, > > > > { TRUE, MaxReadRequestInitialize, MaxReadRequestScan, > > > > MaxReadRequestProgram, MaxReadRequestFinalize}, > > > > { TRUE, NULL, NULL, RelaxOrderProgram, NULL}, > > > > { TRUE, NULL, CompletionTimeoutScan, CompletionTimeoutProgram, > > > NULL }, > > > > ... > > > > }; > > > > > > > > PCIE_FEATURE_ENTRY.Enable can be set to FALSE according to the > > > > platform policy. > > > > > > > > The enable of PCIE features can be written as a feature agnostic f= or- > 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 > > > > > > > > > > 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 > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > +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 the 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 > > > version. > > > > > > > > > > > > > +BOOLEAN > > > > > > > > +CheckPciFeatureConfigurationRecordExist ( > > > > > > > > + IN PCI_IO_DEVICE *RootBrid= ge, > > > > > > > > + OUT PCI_FEATURE_CONFIGURATION_COMPLETION_LIST > > > > > > > > +**PciFeatureConfigRecord > > > > > > > > + ) > > > > > > > > > > > > 2. Is this function to check whether the PCIE features under a > > > > > > root bridge 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. > > > > > > > > > > > Ok, will look into this. > > > > > > > > > > > > > +EFI_STATUS > AddRootBridgeInPciFeaturesConfigCompletionList ( > > > > > > > > + IN PCI_IO_DEVICE *RootBridge, > > > > > > > > + IN BOOLEAN ReEnumerationRequired > > > > > > > > + ) > > > > > > > > > > > > 3. Same question as #2. I think by using gFullEnumeration, > > > > > > this function is > > > > > not > > > > > > needed. > > > > > > > > > > > OK > > > > > > > > > > > > > > > > > > > +BOOLEAN > > > > > > > > +IsPciRootPortEmpty ( > > > > > > > > + IN PCI_IO_DEVICE *PciDevice > > > > > > > > + ) > > > > > > > > > > > > 4. Please use IsListEmpty() directly from callers and remove > > > > > > this > > > function. > > > > > > > > > > > Will consider this. > > > > > > > > > > > > > +**/ > > > > > > > > +EFI_STATUS > > > > > > > > +EnumerateOtherPciFeatures ( > > > > > > > > > > > > 5. Can it be "EnumeratePcieFeatures"? > > > > > > > > > > > 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"")); > > > > > > > > > > > > 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 > > > > > > > > > > > > > + > > > > > > > > + 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. > > > > > > > > > > > > > + 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 PciBus > > > > > scan > > > > > > but is hidden when calling SetupPciFeatures()? > > > > > > > > > > > Yes, that is the case; device detected during the beginning of > > > > > PciBus scan 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 device existence before the publication of the PCI IO > > > > > Protocol, and removal of the PCI_IO_DEVICE instance from the > linked list. > > > > > > > > > > > 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. > > > > > > > > > > > > > + Status =3D ProgramPciFeatures (RootBridge); > > > > > > 10. ProgramPcieFeatures()? > > > > > > > > > > > OK > > > > > > > > > > > > > + > > > > > > > > + 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 > > > > > > > > > > > > > + // > > > > > > > > + // 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 required > > > > > > > > > > > > > +_PRIMARY_ROOT_PORT_NODE { > > > > > > > > > > > > > > + // > > > > > > > > + // Signature header > > > > > > > > + // > > > > > > > > + UINT32 Signature; > > > > > > > > + // > > > > > > > > + // linked list pointers to next node > > > > > > > > + // > > > > > > > > + LIST_ENTRY NeighborRootP= ort; > > > > > > > > + // > > > > > > > > + // 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 is confined to a group of PCI devices and > > > > > for the RCiEP it is not applicable hence not 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.... > > > > > > > > > > > > > +struct _PCI_FEATURE_CONFIGURATION_COMPLETION_LIST { > > > > > > 14. This structure is not needed if using gFullEnumeration. > > > > > Yes. > > > > > > > >=20