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

["Javeed, Ashraf" <ashraf.javeed@intel.com>]

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.