Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration

["Ni, Ray" <ray.ni@intel.com>]

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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, December 19, 2019 1:49 PM
> To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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@intel.com>
> > Sent: Wednesday, December 18, 2019 3:14 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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@intel.com>
> > > Sent: Tuesday, December 17, 2019 5:26 PM
> > > To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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.
> 
>