[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
Mon Mar 16 14:00:25 UTC 2020
Ashraf,
Thanks for taking time to review my code! Comments embedded in below.
> -----Original Message-----
> From: Javeed, Ashraf <ashraf.javeed at intel.com>
> Sent: Monday, March 16, 2020 5:34 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
>
> Ray,
> First of all, thank you for taking time to review and on optimizing the 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/52524e9654704a7f6a30ca446f215b81fe8f0984
> PCI Express Protocol not as per the ECR draft revision 0.8, the changes made has to be reviewed with updated ECR
> version...
With the code first process, we could firstly finalize the protocol interfaces 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 desired. The quality of the protocol header file is always in high priority.
> 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 values from the PciXX.h definitions. This do result in lot of code savings.
I take it as an agree of using the global AUTO. Thanks for that.
>
> (2) commit:- MdeModulePkg/PciBus: Add the framework to init PCIE features
> https://github.com/niruiyu/edk2/commit/9fb9a3dcef06de98a76825e2fc07167446ee6fd9
> 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 RCiEP, 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?
Supposing a root bridge contains 1 RCiEP and two root ports, the implementation ensures there will be 3 separate contexts for each. As you 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 feature _i_.
>
> 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?
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 device's parent/children.
I understand that in PCIE spec, a switch consists of an upstream port and 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?
>
> I liked the Pre & Post order flag that is used to processing parent-to-child vs. child-to-parent nodes.
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.
> 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.
Yes I put GetDevicePolicy() and NotifyDeviceState() in the feature array in my first version of code. And later I called the two separately for better code readability. But it was my fault that I didn't update the commit message. It caused confusion to you.
>
> Thanks
> Ashraf
>
> > -----Original Message-----
> > From: Ni, Ray <ray.ni at intel.com>
> > Sent: Thursday, March 5, 2020 7:43 PM
> > To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Javeed, Ashraf
> > <ashraf.javeed at intel.com>
> > 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
> >
> > 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 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 (#55867): https://edk2.groups.io/g/devel/message/55867
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