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/9fb9a3dcef06de98a76825e2fc07167446ee6fd9 Thanks, Ray > -----Original Message----- > From: [email protected] <[email protected]> On Behalf Of Ni, Ray > Sent: Thursday, December 19, 2019 1:49 PM > To: Javeed, Ashraf <[email protected]>; [email protected] > Cc: Wang, Jian J <[email protected]>; Wu, Hao A <[email protected]> > 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 phase. > 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[] = { > { 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 for-loop. > This can make the new feature enabling code easy to add and review. > > > > -----Original Message----- > > From: Javeed, Ashraf <[email protected]> > > Sent: Wednesday, December 18, 2019 3:14 PM > > To: Ni, Ray <[email protected]>; [email protected] > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A <[email protected]> > > 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 <[email protected]> > > > Sent: Tuesday, December 17, 2019 5:26 PM > > > To: Javeed, Ashraf <[email protected]>; [email protected] > > > Cc: Wang, Jian J <[email protected]>; Wu, Hao A <[email protected]> > > > 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 = 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 *RootBridge, > > > > > + 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 = ConvertDevicePathToText ( > > > > > + DevicePathFromHandle (RootBridge->Handle), > > > > > + FALSE, > > > > > + FALSE > > > > > + ); > > > > > + DEBUG ((DEBUG_INFO, "Enumerating PCI features for Root Bridge > > > > > + %s\n", Str != 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 = PciFeatureRootBridgeScan > > > > > + ; OtherPciFeatureConfigPhase <= > > 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 = GetDevicePathSize (PciDevicePath); > > > SizeOfRootPortDevicePath = GetDevicePathSize (RootPortPath); > > > if ((SizeOfRootPortDevicePath < SizeOfPciDevicePath) && > > > CompareMem (PciDevicePath, RootPortPath, > > SizeOfRootPortDevicePath - > > > END_DEVICE_PATH_LENGTH) == 0)) { > > > // PCI device belongs to the root port. > > > } > > > > > Ok. > > > > > > > + Status = ProgramPciFeatures (RootBridge); > > > 10. ProgramPcieFeatures()? > > > > > OK > > > > > > > + > > > > > + if (Str != 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 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 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. > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55540): https://edk2.groups.io/g/devel/message/55540 Mute This Topic: https://groups.io/mt/55158724/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
