Ashraf,
Thank you for the prompt response!
My response in below.
(I removed some earlier conversations, but the context is still kept.)
> > 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 still
> contemplating whether both AUTO and
> DO_NOT_TOUCH options could be applicable 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 feature
> separately for both these options.
I think we are basically aligned on this.
I agree that each PCIe feature should have clear statement about how to
interpret AUTO and DO_NOT_TOUCH.
DO_NOT_TOUCH in my code change is NOT_APPLICABLE.
If you check some of the features I already implemented, the comments say
clearly what AUTO and NOT_APPLICABLE mean
for that specific feature.
Looking forward to your comments on how to interpret AUTO and NOT_APPLICABLE
for each PCIe feature.
> > 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_.
> >
> I realize now, that there are 3 nested for-loops in the
> EnumerateRootBridgePcieFeatures(), 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 node will be valid as
> it covers each node's hierarchy.
I think we are aligned on this.
> >
> > 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?
> >
> 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. Platform 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 compare between the
> parent and child, will not be applicable if
> you use the same 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 -> 1|0|0
> Switch upstream port -> 2|0|0 (M-1)
> Switch downstream ports -> 3|0|0,(M) 5|0|0 (N (=M+2))
> Endpoint devices -> 4|0|0 (M+1) 6|0|0 (N+1 (=M+3))
> The level+1 logic when the EnumeratePcieDevices() is called recursively, will
> result in one value greater than the Endpoint
> device for the second downstream port.
The level value in my code will be:
Root Port -> 1|0|0 (level 0)
Switch upstream port -> 2|0|0 (level 1)
Switch downstream ports -> 3|0|0,(level 2) 5|0|0,(level 2)
Endpoint devices -> 4|0|0,(level 3) 6|0|0,(level 3)
Level of device 5|0|0 is still 2, not 4.
I also did the unit test to ensure the LTR feature logic is good.
For below device hierarchy with LTR enabled in 5/0/0 and 11/0/0, disabled in
9/0/0
The LTR setting for upstream bridges should be as following:
0/4/0[LTR:1]
2/0/0[1] | 2/1/0[1] | 2/2/0[0]
3/0/0[1] | 7/0/0[1]
4/0/0[1] | 8/0/0[1]
5/0/0[1] | 9/0/0[0] 9/1/0[1]
10/0/0[1]
11/0/0[1]
The unit test code is in commit "Test case for LTR".
>
> > >
> > > 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...@intel.com>
> > > > Sent: Thursday, March 5, 2020 7:43 PM
> > > > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Javeed, Ashraf
> > > > <ashraf.jav...@intel.com>
> > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> > > > <hao.a...@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Ni,
> > > > Ray
> > > > > Sent: Thursday, December 19, 2019 1:49 PM
> > > > > To: Javeed, Ashraf <ashraf.jav...@intel.com>; devel@edk2.groups.io
> > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> > > > > <hao.a...@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.jav...@intel.com>
> > > > > > Sent: Wednesday, December 18, 2019 3:14 PM
> > > > > > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
> > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> > > > > > <hao.a...@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...@intel.com>
> > > > > > > Sent: Tuesday, December 17, 2019 5:26 PM
> > > > > > > To: Javeed, Ashraf <ashraf.jav...@intel.com>;
> > > > > > > devel@edk2.groups.io
> > > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> > > > > > > <hao.a...@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 (#55932): https://edk2.groups.io/g/devel/message/55932
Mute This Topic: https://groups.io/mt/55158724/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
