[edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration
Ni, Ray
ray.ni at intel.com
Thu Mar 5 14:12:55 UTC 2020
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: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, December 19, 2019 1:49 PM
> To: Javeed, Ashraf <ashraf.javeed at intel.com>; devel at edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
> 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 <ashraf.javeed at intel.com>
> > Sent: Wednesday, December 18, 2019 3:14 PM
> > To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
> > 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 <ray.ni at intel.com>
> > > Sent: Tuesday, December 17, 2019 5:26 PM
> > > To: Javeed, Ashraf <ashraf.javeed at intel.com>; devel at edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
> > > 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list