[edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device

Wu, Hao A hao.a.wu at intel.com
Tue Jun 14 01:26:24 UTC 2022


Thanks.

For 2 (DevicePathLib PEIM instance), please ensure it is done before merging this series.

For 3 (IOMMU codes in storage device PEIMs):
Yes, you are right that we still need to consider the EDKII_ATA_AHCI_HOST_CONTROLLER_PPI case.
As far as I can recall, this PPI was added for the support of OPAL/HDD Password S3 unlock feature. So my take is that the PPI producers are mainly Intel Client platforms.
I think in order to add a library (or directly use services in EDKII_PCI_DEVICE_PPI) to abstract the IOMMU related logics in AhciPei, both 2 conditions below should be met:
a) Retire the EDKII_ATA_AHCI_HOST_CONTROLLER_PPI
This should be no hard, as there are not many producers of this PPI.
b) A public reference PciBusPei module is needed in edk2 like PciBusDxe for DXE case
As I mentioned earlier, an enforcement for producers of EDKII_PCI_DEVICE_PPI to add IOMMU support is required.
If there is no reference module (like PciBusDxe for DXE), such enforcement cannot be guaranteed.
Meanwhile, adding a reference PciBusPei implementation might not be easy.
As I went through the RFC discussion (https://edk2.groups.io/g/rfc/topic/86658203), it seems to me that the PEI phase enumeration requirement varies dramatically between platforms.
Not sure what is the long-term goal for the current silicon code that produces of EDKII_PCI_DEVICE_PPI, if the target is to eventually put it in edk2 (not edk2-platform), then I see the feasibility of handling PEI phase IOMMU in one common place.

For 5 (GitHub Pull Request to trigger the CI tests), since 2 is not done yet, my take is that the build tests are likely to fail. Need to wait for DevicePathLib PEIM instance being merged for this.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Czajkowski, Maciej <maciej.czajkowski at intel.com>
> Sent: Monday, June 13, 2022 9:20 PM
> To: Wu, Hao A <hao.a.wu at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at intel.com>
> Subject: RE: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use
> PCI_DEVICE_PPI to manage AHCI device
> 
> Hello,
> 
> 1. Yes, I will try to fix that in the v2 patch.
> 2. We have a review opened to add such instance -
> https://edk2.groups.io/g/devel/message/89970
> 3. For now it will be implemented in the silicon code, so you are right - we
> should keep them. Also, it would require a larger library refactor to consume
> such code from PCI_DEVICE_PPI if we are going to still support both
> PCI_DEVICE_PPI and AHCI_HOST_CONTROLLER_PPI. However, what are you
> thoughts about future of the library? If we can get rid of
> AHCI_HOST_CONTROLLER_PPI, I think that it is possible to remove the
> IOMMU code.
> 4. It has been run in the simulation environment, and a BlockIo read has been
> performed in PEI phase - and it was performed successfully.
> 5. Sure, will do that for v2 patch.
> 
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu at intel.com>
> Sent: czwartek, 9 czerwca 2022 05:08
> To: devel at edk2.groups.io; Wu, Hao A <hao.a.wu at intel.com>; Czajkowski,
> Maciej <maciej.czajkowski at intel.com>
> Cc: Ni, Ray <ray.ni at intel.com>
> Subject: RE: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use
> PCI_DEVICE_PPI to manage AHCI device
> 
> For "3) Could you help to check if the DMA memory related codes in
> MdeModulePkg\Bus\Ata\AhciPei\DmaMem.c can be covered by the 'PciIo'
> service in EDKII_PCI_DEVICE_PPI?"
> After a second thought, my take is that there will be no PciBusPei
> implementation added in edk2.
> So there will be no enforcement for producers of EDKII_PCI_DEVICE_PPI to
> add IOMMU support like in PciBusDxe.
> 
> If my above understanding is correct, then I think we might still need to keep
> those IOMMU support codes in AhciPei PEIM.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Wu,
> Hao
> > A
> > Sent: Thursday, June 9, 2022 10:48 AM
> > To: Czajkowski, Maciej <maciej.czajkowski at intel.com>;
> > devel at edk2.groups.io
> > Cc: Ni, Ray <ray.ni at intel.com>
> > Subject: Re: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use
> > PCI_DEVICE_PPI to manage AHCI device
> >
> > Couple of general level comments/questions:
> > 1) The implementation of functions
> > AtaAhciPciDevicePpiInstallationCallback() &
> > AtaAhciInitPrivateDataFromPciDevice() has many duplications. Is it
> > possible to abstract a separate function to reduce duplicated codes?
> > 2) What DevicePathLib instance should be used for the PEI case? As far
> > as I know, current DevicePathLib instances in edk2 do not support PEIM.
> > 3) Could you help to check if the DMA memory related codes in
> > MdeModulePkg\Bus\Ata\AhciPei\DmaMem.c can be covered by the 'PciIo'
> > service in EDKII_PCI_DEVICE_PPI?
> > 4) May I know what kind of tests are performed for this patch? Would
> > like to ensure the origin gEdkiiPeiAtaAhciHostControllerPpiGuid path is not
> broken.
> > 5) Could you help to create a GitHub Pull Request to trigger the CI
> > tests for this series?
> >
> > More inline comments below:
> >
> >
> > > -----Original Message-----
> > > From: Czajkowski, Maciej <maciej.czajkowski at intel.com>
> > > Sent: Monday, June 6, 2022 8:45 PM
> > > To: devel at edk2.groups.io
> > > Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>
> > > Subject: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use
> > > PCI_DEVICE_PPI to manage AHCI device
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907
> > >
> > > This change modifies AhciPei library to allow usage both
> > > EDKII_PCI_DEVICE_PPI and
> EDKII_PEI_ATA_AHCI_HOST_CONTROLLER_PPI to
> > > manage ATA HDD working under AHCI mode.
> > >
> > > Cc: Hao A Wu <hao.a.wu at intel.com>
> > > Cc: Ray Ni <ray.ni at intel.com>
> > > Signed-off-by: Maciej Czajkowski <maciej.czajkowski at intel.com>
> > > ---
> > >  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c    | 615 +++++++++++++++--
> ---
> > >  MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c |  44 --
> > >  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf  |   5 +-
> > >  3 files changed, 458 insertions(+), 206 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > > b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > > index 208b7e9a3606..31bb3c0760ab 100644
> > > --- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > > +++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > > @@ -9,6 +9,47 @@
> > >  **/
> > >
> > >
> > >
> > >  #include "AhciPei.h"
> > >
> > > +#include <Ppi/PciDevice.h>
> > >
> > > +#include <Library/DevicePathLib.h>
> > >
> > > +#include <IndustryStandard/Pci.h>
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Callback for EDKII_ATA_AHCI_HOST_CONTROLLER_PPI installation.
> > >
> > > +
> > >
> > > +  @param[in] PeiServices         Pointer to PEI Services Table.
> > >
> > > +  @param[in] NotifyDescriptor    Pointer to the descriptor for the
> Notification
> > >
> > > +                                 event that caused this function to execute.
> > >
> > > +  @param[in] Ppi                 Pointer to the PPI data associated with this
> > function.
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS    The function completes successfully
> > >
> > > +
> > >
> > > +**/
> > >
> > > +EFI_STATUS
> > >
> > > +EFIAPI
> > >
> > > +AtaAhciHostControllerPpiInstallationCallback (
> > >
> > > +  IN EFI_PEI_SERVICES           **PeiServices,
> > >
> > > +  IN EFI_PEI_NOTIFY_DESCRIPTOR  *NotifyDescriptor,
> > >
> > > +  IN VOID                       *Ppi
> > >
> > > +  );
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Callback for EDKII_PCI_DEVICE_PPI installation.
> > >
> > > +
> > >
> > > +  @param[in] PeiServices         Pointer to PEI Services Table.
> > >
> > > +  @param[in] NotifyDescriptor    Pointer to the descriptor for the
> Notification
> > >
> > > +                                 event that caused this function to execute.
> > >
> > > +  @param[in] Ppi                 Pointer to the PPI data associated with this
> > function.
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS    The function completes successfully
> > >
> > > +
> > >
> > > +**/
> > >
> > > +EFI_STATUS
> > >
> > > +EFIAPI
> > >
> > > +AtaAhciPciDevicePpiInstallationCallback (
> > >
> > > +  IN EFI_PEI_SERVICES           **PeiServices,
> > >
> > > +  IN EFI_PEI_NOTIFY_DESCRIPTOR  *NotifyDescriptor,
> > >
> > > +  IN VOID                       *Ppi
> > >
> > > +  );
> >
> >
> > Could you help to put the above 2 function declarations in AhciPei.h
> > to keep consistency?
> 
> Sure, will fix that in v2 patch.
> 
> >
> >
> > >
> > >
> > >
> > >  EFI_PEI_PPI_DESCRIPTOR  mAhciAtaPassThruPpiListTemplate = {
> > >
> > >    (EFI_PEI_PPI_DESCRIPTOR_PPI |
> > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> > >
> > > @@ -40,6 +81,18 @@ EFI_PEI_NOTIFY_DESCRIPTOR
> > > mAhciEndOfPeiNotifyListTemplate = {
> > >    AhciPeimEndOfPei
> > >
> > >  };
> > >
> > >
> > >
> > > +EFI_PEI_NOTIFY_DESCRIPTOR  mAtaAhciHostControllerNotify = {
> > >
> > > +  (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK |
> > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> > >
> > > +  &gEdkiiPeiAtaAhciHostControllerPpiGuid,
> > >
> > > +  AtaAhciHostControllerPpiInstallationCallback
> > >
> > > +};
> > >
> > > +
> > >
> > > +EFI_PEI_NOTIFY_DESCRIPTOR  mPciDevicePpiNotify = {
> > >
> > > +  (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK |
> > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> > >
> > > +  &gEdkiiPeiPciDevicePpiGuid,
> > >
> > > +  AtaAhciPciDevicePpiInstallationCallback
> > >
> > > +};
> > >
> > > +
> > >
> > >  /**
> > >
> > >    Free the DMA resources allocated by an ATA AHCI controller.
> > >
> > >
> > >
> > > @@ -111,33 +164,28 @@ AhciPeimEndOfPei (  }
> > >
> > >
> > >
> > >  /**
> > >
> > > -  Entry point of the PEIM.
> > >
> > > +  Initialize and install PrivateData PPIs.
> > >
> > >
> > >
> > > -  @param[in] FileHandle     Handle of the file being invoked.
> > >
> > > -  @param[in] PeiServices    Describes the list of possible PEI Services.
> > >
> > > -
> > >
> > > -  @retval EFI_SUCCESS    PPI successfully installed.
> > >
> > > +  @param[in] Private    A pointer to the
> > > PEI_AHCI_CONTROLLER_PRIVATE_DATA data
> > >
> > > +                        structure.
> > >
> > >
> > >
> > > +  @retval EFI_SUCCESS  AHCI controller initialized and PPIs
> > > + installed
> > >
> > > +  @retval others       Failed to initialize AHCI controller
> > >
> > >  **/
> > >
> > >  EFI_STATUS
> > >
> > > -EFIAPI
> > >
> > > -AtaAhciPeimEntry (
> > >
> > > -  IN EFI_PEI_FILE_HANDLE     FileHandle,
> > >
> > > -  IN CONST EFI_PEI_SERVICES  **PeiServices
> > >
> > > +AtaAhciInitPrivateData (
> > >
> > > +  IN UINTN                             MmioBase,
> > >
> > > +  IN EFI_DEVICE_PATH_PROTOCOL          *DevicePath,
> > >
> > > +  IN UINTN                             DevicePathLength
> > >
> > >    )
> > >
> > >  {
> > >
> > > -  EFI_STATUS                          Status;
> > >
> > > -  EFI_BOOT_MODE                       BootMode;
> > >
> > > -  EDKII_ATA_AHCI_HOST_CONTROLLER_PPI  *AhciHcPpi;
> > >
> > > -  UINT8                               Controller;
> > >
> > > -  UINTN                               MmioBase;
> > >
> > > -  UINTN                               DevicePathLength;
> > >
> > > -  EFI_DEVICE_PATH_PROTOCOL            *DevicePath;
> > >
> > > -  UINT32                              PortBitMap;
> > >
> > > -  PEI_AHCI_CONTROLLER_PRIVATE_DATA    *Private;
> > >
> > > -  UINT8                               NumberOfPorts;
> > >
> > > +  EFI_STATUS                        Status;
> > >
> > > +  UINT32                            PortBitMap;
> > >
> > > +  UINT8                             NumberOfPorts;
> > >
> > > +  PEI_AHCI_CONTROLLER_PRIVATE_DATA  *Private;
> > >
> > > +  EFI_BOOT_MODE                     BootMode;
> > >
> > >
> > >
> > > -  DEBUG ((DEBUG_INFO, "%a: Enters.\n", __FUNCTION__));
> > >
> > > +  DEBUG ((DEBUG_INFO, "Initializing private data for ATA\n"));
> > >
> > >
> > >
> > >    //
> > >
> > >    // Get the current boot mode.
> > >
> > > @@ -149,19 +197,149 @@ AtaAhciPeimEntry (
> > >    }
> > >
> > >
> > >
> > >    //
> > >
> > > -  // Locate the ATA AHCI host controller PPI.
> > >
> > > -  //
> > >
> > > -  Status = PeiServicesLocatePpi (
> > >
> > > -             &gEdkiiPeiAtaAhciHostControllerPpiGuid,
> > >
> > > -             0,
> > >
> > > -             NULL,
> > >
> > > -             (VOID **)&AhciHcPpi
> > >
> > > -             );
> > >
> > > +  // Check validity of the device path of the ATA AHCI controller.
> > >
> > > +  //
> > >
> > > +  Status = AhciIsHcDevicePathValid (DevicePath, DevicePathLength);
> > >
> > > +  if (EFI_ERROR (Status)) {
> > >
> > > +    DEBUG ((
> > >
> > > +      DEBUG_ERROR,
> > >
> > > +      "%a: The device path is invalid.\n",
> > >
> > > +      __FUNCTION__
> > >
> > > +      ));
> > >
> > > +    return Status;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // For S3 resume performance consideration, not all ports on an
> > > + ATA AHCI
> > >
> > > +  // controller will be enumerated/initialized. The driver consumes
> > > + the
> > >
> > > +  // content within S3StorageDeviceInitList LockBox to get the
> > > + ports that
> > >
> > > +  // will be enumerated/initialized during S3 resume.
> > >
> > > +  //
> > >
> > > +  if (BootMode == BOOT_ON_S3_RESUME) {
> > >
> > > +    NumberOfPorts = AhciS3GetEumeratePorts (DevicePath,
> > > + DevicePathLength,
> > > &PortBitMap);
> > >
> > > +    if (NumberOfPorts == 0) {
> > >
> > > +      return EFI_SUCCESS;
> > >
> > > +    }
> > >
> > > +  } else {
> > >
> > > +    PortBitMap = MAX_UINT32;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // Memory allocation for controller private data.
> > >
> > > +  //
> > >
> > > +  Private = AllocateZeroPool (sizeof
> > > + (PEI_AHCI_CONTROLLER_PRIVATE_DATA));
> > >
> > > +  if (Private == NULL) {
> > >
> > > +    DEBUG ((
> > >
> > > +      DEBUG_ERROR,
> > >
> > > +      "%a: Fail to allocate private data.\n",
> > >
> > > +      __FUNCTION__
> > >
> > > +      ));
> > >
> > > +    return EFI_OUT_OF_RESOURCES;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // Initialize controller private data.
> > >
> > > +  //
> > >
> > > +  Private->Signature        =
> > > AHCI_PEI_CONTROLLER_PRIVATE_DATA_SIGNATURE;
> > >
> > > +  Private->MmioBase         = MmioBase;
> > >
> > > +  Private->DevicePathLength = DevicePathLength;
> > >
> > > +  Private->DevicePath       = DevicePath;
> > >
> > > +  Private->PortBitMap       = PortBitMap;
> > >
> > > +  InitializeListHead (&Private->DeviceList);
> > >
> > > +
> > >
> > > +  Status = AhciModeInitialization (Private);
> > >
> > >    if (EFI_ERROR (Status)) {
> > >
> > > -    DEBUG ((DEBUG_ERROR, "%a: Failed to locate
> > AtaAhciHostControllerPpi.\n",
> > > __FUNCTION__));
> > >
> > > -    return EFI_UNSUPPORTED;
> > >
> > > +    return Status;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  Private->AtaPassThruMode.Attributes =
> > > EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL |
> > >
> > > +
> > > + EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL;
> > >
> > > +  Private->AtaPassThruMode.IoAlign      = sizeof (UINTN);
> > >
> > > +  Private->AtaPassThruPpi.Revision      =
> > > EDKII_PEI_ATA_PASS_THRU_PPI_REVISION;
> > >
> > > +  Private->AtaPassThruPpi.Mode          = &Private->AtaPassThruMode;
> > >
> > > +  Private->AtaPassThruPpi.PassThru      = AhciAtaPassThruPassThru;
> > >
> > > +  Private->AtaPassThruPpi.GetNextPort   =
> AhciAtaPassThruGetNextPort;
> > >
> > > +  Private->AtaPassThruPpi.GetNextDevice =
> > > + AhciAtaPassThruGetNextDevice;
> > >
> > > +  Private->AtaPassThruPpi.GetDevicePath =
> > > + AhciAtaPassThruGetDevicePath;
> > >
> > > +  CopyMem (
> > >
> > > +    &Private->AtaPassThruPpiList,
> > >
> > > +    &mAhciAtaPassThruPpiListTemplate,
> > >
> > > +    sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > +    );
> > >
> > > +  Private->AtaPassThruPpiList.Ppi = &Private->AtaPassThruPpi;
> > >
> > > +  PeiServicesInstallPpi (&Private->AtaPassThruPpiList);
> > >
> > > +
> > >
> > > +  Private->BlkIoPpi.GetNumberOfBlockDevices =
> > > + AhciBlockIoGetDeviceNo;
> > >
> > > +  Private->BlkIoPpi.GetBlockDeviceMediaInfo =
> > > + AhciBlockIoGetMediaInfo;
> > >
> > > +  Private->BlkIoPpi.ReadBlocks              = AhciBlockIoReadBlocks;
> > >
> > > +  CopyMem (
> > >
> > > +    &Private->BlkIoPpiList,
> > >
> > > +    &mAhciBlkIoPpiListTemplate,
> > >
> > > +    sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > +    );
> > >
> > > +  Private->BlkIoPpiList.Ppi = &Private->BlkIoPpi;
> > >
> > > +  PeiServicesInstallPpi (&Private->BlkIoPpiList);
> > >
> > > +
> > >
> > > +  Private->BlkIo2Ppi.Revision                =
> > > EFI_PEI_RECOVERY_BLOCK_IO2_PPI_REVISION;
> > >
> > > +  Private->BlkIo2Ppi.GetNumberOfBlockDevices =
> > > + AhciBlockIoGetDeviceNo2;
> > >
> > > +  Private->BlkIo2Ppi.GetBlockDeviceMediaInfo =
> > > + AhciBlockIoGetMediaInfo2;
> > >
> > > +  Private->BlkIo2Ppi.ReadBlocks              = AhciBlockIoReadBlocks2;
> > >
> > > +  CopyMem (
> > >
> > > +    &Private->BlkIo2PpiList,
> > >
> > > +    &mAhciBlkIo2PpiListTemplate,
> > >
> > > +    sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > +    );
> > >
> > > +  Private->BlkIo2PpiList.Ppi = &Private->BlkIo2Ppi;
> > >
> > > +  PeiServicesInstallPpi (&Private->BlkIo2PpiList);
> > >
> > > +
> > >
> > > +  if (Private->TrustComputingDevices != 0) {
> > >
> > > +    DEBUG ((
> > >
> > > +      DEBUG_INFO,
> > >
> > > +      "%a: Security Security Command PPI will be produced.\n",
> > >
> > > +      __FUNCTION__
> > >
> > > +      ));
> > >
> > > +    Private->StorageSecurityPpi.Revision           =
> > > EDKII_STORAGE_SECURITY_PPI_REVISION;
> > >
> > > +    Private->StorageSecurityPpi.GetNumberofDevices =
> > > AhciStorageSecurityGetDeviceNo;
> > >
> > > +    Private->StorageSecurityPpi.GetDevicePath      =
> > > AhciStorageSecurityGetDevicePath;
> > >
> > > +    Private->StorageSecurityPpi.ReceiveData        =
> > > AhciStorageSecurityReceiveData;
> > >
> > > +    Private->StorageSecurityPpi.SendData           =
> > AhciStorageSecuritySendData;
> > >
> > > +    CopyMem (
> > >
> > > +      &Private->StorageSecurityPpiList,
> > >
> > > +      &mAhciStorageSecurityPpiListTemplate,
> > >
> > > +      sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > +      );
> > >
> > > +    Private->StorageSecurityPpiList.Ppi =
> > > + &Private->StorageSecurityPpi;
> > >
> > > +    PeiServicesInstallPpi (&Private->StorageSecurityPpiList);
> > >
> > >    }
> > >
> > >
> > >
> > > +  CopyMem (
> > >
> > > +    &Private->EndOfPeiNotifyList,
> > >
> > > +    &mAhciEndOfPeiNotifyListTemplate,
> > >
> > > +    sizeof (EFI_PEI_NOTIFY_DESCRIPTOR)
> > >
> > > +    );
> > >
> > > +  PeiServicesNotifyPpi (&Private->EndOfPeiNotifyList);
> > >
> > > +
> > >
> > > +  return EFI_SUCCESS;
> > >
> > > +}
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Initialize AHCI controller from
> > > + EDKII_ATA_AHCI_HOST_CONTROLLER_PPI
> > > instance.
> > >
> > > +
> > >
> > > +  @param[in] AhciHcPpi  Pointer to the AHCI Host Controller PPI instance.
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS   PPI successfully installed.
> > >
> > > +**/
> > >
> > > +EFI_STATUS
> > >
> > > +AtaAhciInitPrivateDataFromHostControllerPpi (
> > >
> > > +  IN EDKII_ATA_AHCI_HOST_CONTROLLER_PPI  *AhciHcPpi
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  UINT8  Controller;
> > >
> > > +  UINTN                               MmioBase;
> > >
> > > +  UINTN                               DevicePathLength;
> > >
> > > +  EFI_DEVICE_PATH_PROTOCOL            *DevicePath;
> > >
> > > +  EFI_STATUS                          Status;
> > >
> > > +
> > >
> > >    Controller = 0;
> > >
> > >    MmioBase   = 0;
> > >
> > >    while (TRUE) {
> > >
> > > @@ -193,65 +371,7 @@ AtaAhciPeimEntry (
> > >        return Status;
> > >
> > >      }
> > >
> > >
> > >
> > > -    //
> > >
> > > -    // Check validity of the device path of the ATA AHCI controller.
> > >
> > > -    //
> > >
> > > -    Status = AhciIsHcDevicePathValid (DevicePath, DevicePathLength);
> > >
> > > -    if (EFI_ERROR (Status)) {
> > >
> > > -      DEBUG ((
> > >
> > > -        DEBUG_ERROR,
> > >
> > > -        "%a: The device path is invalid for Controller %d.\n",
> > >
> > > -        __FUNCTION__,
> > >
> > > -        Controller
> > >
> > > -        ));
> > >
> > > -      Controller++;
> > >
> > > -      continue;
> > >
> > > -    }
> > >
> > > -
> > >
> > > -    //
> > >
> > > -    // For S3 resume performance consideration, not all ports on an ATA
> AHCI
> > >
> > > -    // controller will be enumerated/initialized. The driver consumes the
> > >
> > > -    // content within S3StorageDeviceInitList LockBox to get the ports that
> > >
> > > -    // will be enumerated/initialized during S3 resume.
> > >
> > > -    //
> > >
> > > -    if (BootMode == BOOT_ON_S3_RESUME) {
> > >
> > > -      NumberOfPorts = AhciS3GetEumeratePorts (DevicePath,
> > DevicePathLength,
> > > &PortBitMap);
> > >
> > > -      if (NumberOfPorts == 0) {
> > >
> > > -        //
> > >
> > > -        // No ports need to be enumerated for this controller.
> > >
> > > -        //
> > >
> > > -        Controller++;
> > >
> > > -        continue;
> > >
> > > -      }
> > >
> > > -    } else {
> > >
> > > -      PortBitMap = MAX_UINT32;
> > >
> > > -    }
> > >
> > > -
> > >
> > > -    //
> > >
> > > -    // Memory allocation for controller private data.
> > >
> > > -    //
> > >
> > > -    Private = AllocateZeroPool (sizeof
> > (PEI_AHCI_CONTROLLER_PRIVATE_DATA));
> > >
> > > -    if (Private == NULL) {
> > >
> > > -      DEBUG ((
> > >
> > > -        DEBUG_ERROR,
> > >
> > > -        "%a: Fail to allocate private data for Controller %d.\n",
> > >
> > > -        __FUNCTION__,
> > >
> > > -        Controller
> > >
> > > -        ));
> > >
> > > -      return EFI_OUT_OF_RESOURCES;
> > >
> > > -    }
> > >
> > > -
> > >
> > > -    //
> > >
> > > -    // Initialize controller private data.
> > >
> > > -    //
> > >
> > > -    Private->Signature        =
> > > AHCI_PEI_CONTROLLER_PRIVATE_DATA_SIGNATURE;
> > >
> > > -    Private->MmioBase         = MmioBase;
> > >
> > > -    Private->DevicePathLength = DevicePathLength;
> > >
> > > -    Private->DevicePath       = DevicePath;
> > >
> > > -    Private->PortBitMap       = PortBitMap;
> > >
> > > -    InitializeListHead (&Private->DeviceList);
> > >
> > > -
> > >
> > > -    Status = AhciModeInitialization (Private);
> > >
> > > +    Status = AtaAhciInitPrivateData (MmioBase, DevicePath,
> > > + DevicePathLength);
> > >
> > >      if (EFI_ERROR (Status)) {
> > >
> > >        DEBUG ((
> > >
> > >          DEBUG_ERROR,
> > >
> > > @@ -260,86 +380,261 @@ AtaAhciPeimEntry (
> > >          Controller,
> > >
> > >          Status
> > >
> > >          ));
> > >
> > > -      Controller++;
> > >
> > > -      continue;
> > >
> > > -    }
> > >
> > > -
> > >
> > > -    Private->AtaPassThruMode.Attributes =
> > > EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL |
> > >
> > > -                                          EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL;
> > >
> > > -    Private->AtaPassThruMode.IoAlign      = sizeof (UINTN);
> > >
> > > -    Private->AtaPassThruPpi.Revision      =
> > > EDKII_PEI_ATA_PASS_THRU_PPI_REVISION;
> > >
> > > -    Private->AtaPassThruPpi.Mode          = &Private->AtaPassThruMode;
> > >
> > > -    Private->AtaPassThruPpi.PassThru      = AhciAtaPassThruPassThru;
> > >
> > > -    Private->AtaPassThruPpi.GetNextPort   =
> AhciAtaPassThruGetNextPort;
> > >
> > > -    Private->AtaPassThruPpi.GetNextDevice =
> AhciAtaPassThruGetNextDevice;
> > >
> > > -    Private->AtaPassThruPpi.GetDevicePath =
> AhciAtaPassThruGetDevicePath;
> > >
> > > -    CopyMem (
> > >
> > > -      &Private->AtaPassThruPpiList,
> > >
> > > -      &mAhciAtaPassThruPpiListTemplate,
> > >
> > > -      sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > -      );
> > >
> > > -    Private->AtaPassThruPpiList.Ppi = &Private->AtaPassThruPpi;
> > >
> > > -    PeiServicesInstallPpi (&Private->AtaPassThruPpiList);
> > >
> > > -
> > >
> > > -    Private->BlkIoPpi.GetNumberOfBlockDevices =
> AhciBlockIoGetDeviceNo;
> > >
> > > -    Private->BlkIoPpi.GetBlockDeviceMediaInfo =
> AhciBlockIoGetMediaInfo;
> > >
> > > -    Private->BlkIoPpi.ReadBlocks              = AhciBlockIoReadBlocks;
> > >
> > > -    CopyMem (
> > >
> > > -      &Private->BlkIoPpiList,
> > >
> > > -      &mAhciBlkIoPpiListTemplate,
> > >
> > > -      sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > -      );
> > >
> > > -    Private->BlkIoPpiList.Ppi = &Private->BlkIoPpi;
> > >
> > > -    PeiServicesInstallPpi (&Private->BlkIoPpiList);
> > >
> > > -
> > >
> > > -    Private->BlkIo2Ppi.Revision                =
> > > EFI_PEI_RECOVERY_BLOCK_IO2_PPI_REVISION;
> > >
> > > -    Private->BlkIo2Ppi.GetNumberOfBlockDevices =
> AhciBlockIoGetDeviceNo2;
> > >
> > > -    Private->BlkIo2Ppi.GetBlockDeviceMediaInfo =
> AhciBlockIoGetMediaInfo2;
> > >
> > > -    Private->BlkIo2Ppi.ReadBlocks              = AhciBlockIoReadBlocks2;
> > >
> > > -    CopyMem (
> > >
> > > -      &Private->BlkIo2PpiList,
> > >
> > > -      &mAhciBlkIo2PpiListTemplate,
> > >
> > > -      sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > -      );
> > >
> > > -    Private->BlkIo2PpiList.Ppi = &Private->BlkIo2Ppi;
> > >
> > > -    PeiServicesInstallPpi (&Private->BlkIo2PpiList);
> > >
> > > -
> > >
> > > -    if (Private->TrustComputingDevices != 0) {
> > >
> > > +    } else {
> > >
> > >        DEBUG ((
> > >
> > >          DEBUG_INFO,
> > >
> > > -        "%a: Security Security Command PPI will be produced for
> > Controller %d.\n",
> > >
> > > +        "%a: Controller %d has been successfully initialized.\n",
> > >
> > >          __FUNCTION__,
> > >
> > >          Controller
> > >
> > >          ));
> > >
> > > -      Private->StorageSecurityPpi.Revision           =
> > > EDKII_STORAGE_SECURITY_PPI_REVISION;
> > >
> > > -      Private->StorageSecurityPpi.GetNumberofDevices =
> > > AhciStorageSecurityGetDeviceNo;
> > >
> > > -      Private->StorageSecurityPpi.GetDevicePath      =
> > > AhciStorageSecurityGetDevicePath;
> > >
> > > -      Private->StorageSecurityPpi.ReceiveData        =
> > > AhciStorageSecurityReceiveData;
> > >
> > > -      Private->StorageSecurityPpi.SendData           =
> > AhciStorageSecuritySendData;
> > >
> > > -      CopyMem (
> > >
> > > -        &Private->StorageSecurityPpiList,
> > >
> > > -        &mAhciStorageSecurityPpiListTemplate,
> > >
> > > -        sizeof (EFI_PEI_PPI_DESCRIPTOR)
> > >
> > > -        );
> > >
> > > -      Private->StorageSecurityPpiList.Ppi = &Private->StorageSecurityPpi;
> > >
> > > -      PeiServicesInstallPpi (&Private->StorageSecurityPpiList);
> > >
> > >      }
> > >
> > >
> > >
> > > -    CopyMem (
> > >
> > > -      &Private->EndOfPeiNotifyList,
> > >
> > > -      &mAhciEndOfPeiNotifyListTemplate,
> > >
> > > -      sizeof (EFI_PEI_NOTIFY_DESCRIPTOR)
> > >
> > > -      );
> > >
> > > -    PeiServicesNotifyPpi (&Private->EndOfPeiNotifyList);
> > >
> > > -
> > >
> > > -    DEBUG ((
> > >
> > > -      DEBUG_INFO,
> > >
> > > -      "%a: Controller %d has been successfully initialized.\n",
> > >
> > > -      __FUNCTION__,
> > >
> > > -      Controller
> > >
> > > -      ));
> > >
> > >      Controller++;
> > >
> > >    }
> > >
> > >
> > >
> > >    return EFI_SUCCESS;
> > >
> > >  }
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Callback for EDKII_ATA_AHCI_HOST_CONTROLLER_PPI installation.
> > >
> > > +
> > >
> > > +  @param[in] PeiServices         Pointer to PEI Services Table.
> > >
> > > +  @param[in] NotifyDescriptor    Pointer to the descriptor for the
> Notification
> > >
> > > +                                 event that caused this function to execute.
> > >
> > > +  @param[in] Ppi                 Pointer to the PPI data associated with this
> > function.
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS            The function completes successfully
> >
> >
> > Please help to add the descriptions for function returning error status.
> 
> Sure, will fix that in v2 patch.
> 
> >
> >
> > >
> > > +
> > >
> > > +**/
> > >
> > > +EFI_STATUS
> > >
> > > +EFIAPI
> > >
> > > +AtaAhciHostControllerPpiInstallationCallback (
> > >
> > > +  IN EFI_PEI_SERVICES           **PeiServices,
> > >
> > > +  IN EFI_PEI_NOTIFY_DESCRIPTOR  *NotifyDescriptor,
> > >
> > > +  IN VOID                       *Ppi
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  EDKII_ATA_AHCI_HOST_CONTROLLER_PPI  *AhciHcPpi;
> > >
> > > +
> > >
> > > +  if (Ppi == NULL) {
> > >
> > > +    return EFI_INVALID_PARAMETER;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  AhciHcPpi = (EDKII_ATA_AHCI_HOST_CONTROLLER_PPI*) Ppi;
> > >
> > > +
> > >
> > > +  return AtaAhciInitPrivateDataFromHostControllerPpi (AhciHcPpi);
> > >
> > > +}
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Callback for EDKII_PCI_DEVICE_PPI installation.
> > >
> > > +
> > >
> > > +  @param[in] PeiServices         Pointer to PEI Services Table.
> > >
> > > +  @param[in] NotifyDescriptor    Pointer to the descriptor for the
> Notification
> > >
> > > +                                 event that caused this function to execute.
> > >
> > > +  @param[in] Ppi                 Pointer to the PPI data associated with this
> > function.
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS            The function completes successfully
> >
> >
> > Please help to add the descriptions for function returning error status.
> 
> Sure, will fix that in v2 patch.
> 
> >
> >
> > >
> > > +
> > >
> > > +**/
> > >
> > > +EFI_STATUS
> > >
> > > +EFIAPI
> > >
> > > +AtaAhciPciDevicePpiInstallationCallback (
> > >
> > > +  IN EFI_PEI_SERVICES           **PeiServices,
> > >
> > > +  IN EFI_PEI_NOTIFY_DESCRIPTOR  *NotifyDescriptor,
> > >
> > > +  IN VOID                       *Ppi
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  EDKII_PCI_DEVICE_PPI      *PciDevice;
> > >
> > > +  PCI_TYPE00                PciData;
> > >
> > > +  UINTN                     MmioBase;
> > >
> > > +  EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> > >
> > > +  UINTN                     DevicePathLength;
> > >
> > > +  EFI_STATUS                Status;
> > >
> > > +
> > >
> > > +  PciDevice = (EDKII_PCI_DEVICE_PPI*) Ppi;
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // Now further check the PCI header: Base Class (offset 0x0B) and
> > >
> > > +  // Sub Class (offset 0x0A). This controller should be an SATA
> > > + controller
> > >
> > > +  //
> > >
> > > +  Status = PciDevice->PciIo.Pci.Read (
> > >
> > > +                                  &PciDevice->PciIo,
> > >
> > > +                                  EfiPciIoWidthUint8,
> > >
> > > +                                  PCI_CLASSCODE_OFFSET,
> > >
> > > +                                  sizeof (PciData.Hdr.ClassCode),
> > >
> > > +                                  PciData.Hdr.ClassCode
> > >
> > > +                                  );
> > >
> > > +  if (EFI_ERROR (Status)) {
> > >
> > > +    return EFI_UNSUPPORTED;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  if (!IS_PCI_IDE (&PciData) && !IS_PCI_SATADPA (&PciData)) {
> > >
> > > +    return EFI_UNSUPPORTED;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  Status = PciDevice->PciIo.Attributes (
> > >
> > > +                                &PciDevice->PciIo,
> > >
> > > +                                EfiPciIoAttributeOperationSet,
> > >
> > > +                                EFI_PCI_DEVICE_ENABLE,
> > >
> > > +                                NULL
> > >
> > > +                                );
> >
> >
> > Do you think we need to align with AtaAtapiPassThru DXE counterpart
> > when enabling the controller?
> > MdeModulePkg\Bus\Ata\AtaAtapiPassThru\AtaAtapiPassThru.c -
> > AtaAtapiPassThruStart():
> >   Status = PciIo->Attributes (
> >                     PciIo,
> >                     EfiPciIoAttributeOperationSupported,
> >                     0,
> >                     &EnabledPciAttributes
> >                     );
> >   if (!EFI_ERROR (Status)) {
> >     EnabledPciAttributes &= (UINT64)EFI_PCI_DEVICE_ENABLE;
> >     Status                = PciIo->Attributes (
> >                                      PciIo,
> >                                      EfiPciIoAttributeOperationEnable,
> >                                      EnabledPciAttributes,
> >                                      NULL
> >                                      );
> >   }
> 
> Yes, I think it is a good idea. Will be changed in v2 patch.
> >
> >
> > >
> > > +  if (EFI_ERROR (Status)) {
> > >
> > > +    return EFI_UNSUPPORTED;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  Status = PciDevice->PciIo.Pci.Read (
> > >
> > > +                                  &PciDevice->PciIo,
> > >
> > > +                                  EfiPciIoWidthUint32,
> > >
> > > +                                  0x24,
> > >
> > > +                                  sizeof (UINTN),
> > >
> > > +                                  &MmioBase
> > >
> > > +                                  );
> > >
> > > +  if (EFI_ERROR (Status)) {
> > >
> > > +    return EFI_UNSUPPORTED;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  DevicePathLength = GetDevicePathSize (PciDevice->DevicePath);
> > >
> > > +  DevicePath = PciDevice->DevicePath;
> > >
> > > +
> > >
> > > +  Status = AtaAhciInitPrivateData (MmioBase, DevicePath,
> > > + DevicePathLength);
> > >
> > > +  if (EFI_ERROR (Status)) {
> > >
> > > +    DEBUG ((
> > >
> > > +      DEBUG_INFO,
> > >
> > > +      "%a: Failed to init controller, with Status - %r\n",
> > >
> > > +      __FUNCTION__,
> > >
> > > +      Status
> > >
> > > +      ));
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  return EFI_SUCCESS;
> > >
> > > +}
> > >
> > > +
> > >
> >
> >
> > Missing function description comments for
> > AtaAhciInitPrivateDataFromPciDevice.
> 
> I will fix that in v2 patch.
> 
> >
> >
> > > +EFI_STATUS
> > >
> > > +AtaAhciInitPrivateDataFromPciDevice (
> > >
> > > +  VOID
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  EFI_STATUS                Status;
> > >
> > > +  UINTN                     ControllerIndex;
> > >
> > > +  EDKII_PCI_DEVICE_PPI      *PciDevice;
> > >
> > > +  PCI_TYPE00                PciData;
> > >
> > > +  UINTN                     MmioBase;
> > >
> > > +  EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> > >
> > > +  UINTN                     DevicePathLength;
> > >
> > > +
> > >
> > > +  Status = EFI_SUCCESS;
> > >
> > > +  for (ControllerIndex = 0; Status != EFI_NOT_FOUND;
> > > + ControllerIndex++ ) {
> > >
> > > +    Status = PeiServicesLocatePpi (
> > >
> > > +               &gEdkiiPeiPciDevicePpiGuid,
> > >
> > > +               ControllerIndex,
> > >
> > > +               NULL,
> > >
> > > +               (VOID**) &PciDevice
> > >
> > > +                );
> > >
> > > +    if (EFI_ERROR (Status)) {
> > >
> > > +      continue;
> > >
> > > +    }
> > >
> > > +
> > >
> > > +    //
> > >
> > > +    // Now further check the PCI header: Base Class (offset 0x0B)
> > > + and
> > >
> > > +    // Sub Class (offset 0x0A). This controller should be an SATA
> > > + controller
> > >
> > > +    //
> > >
> > > +    Status = PciDevice->PciIo.Pci.Read (
> > >
> > > +                                    &PciDevice->PciIo,
> > >
> > > +                                    EfiPciIoWidthUint8,
> > >
> > > +                                    PCI_CLASSCODE_OFFSET,
> > >
> > > +                                    sizeof (PciData.Hdr.ClassCode),
> > >
> > > +                                    PciData.Hdr.ClassCode
> > >
> > > +                                    );
> > >
> > > +    if (EFI_ERROR (Status)) {
> > >
> > > +      continue;
> > >
> > > +    }
> > >
> > > +
> > >
> > > +    if (!IS_PCI_IDE (&PciData) && !IS_PCI_SATADPA (&PciData)) {
> > >
> > > +      continue;
> > >
> > > +    }
> > >
> > > +
> > >
> > > +    Status = PciDevice->PciIo.Attributes (
> > >
> > > +                                &PciDevice->PciIo,
> > >
> > > +                                EfiPciIoAttributeOperationSet,
> > >
> > > +                                EFI_PCI_DEVICE_ENABLE,
> > >
> > > +                                NULL
> > >
> > > +                                );
> > >
> > > +    if (EFI_ERROR (Status)) {
> > >
> > > +      continue;
> > >
> > > +    }
> > >
> > > +
> > >
> > > +    Status = PciDevice->PciIo.Pci.Read (
> > >
> > > +                                    &PciDevice->PciIo,
> > >
> > > +                                    EfiPciIoWidthUint32,
> > >
> > > +                                    0x24,
> > >
> > > +                                    sizeof (UINTN),
> > >
> > > +                                    &MmioBase
> > >
> > > +                                    );
> > >
> > > +    if (EFI_ERROR (Status)) {
> > >
> > > +      continue;
> > >
> > > +    }
> > >
> > > +
> > >
> > > +    DevicePathLength = GetDevicePathSize (PciDevice->DevicePath);
> > >
> > > +    DevicePath = PciDevice->DevicePath;
> > >
> > > +
> > >
> > > +    Status = AtaAhciInitPrivateData (MmioBase, DevicePath,
> > > + DevicePathLength);
> > >
> > > +    if (EFI_ERROR (Status)) {
> > >
> > > +      DEBUG ((
> > >
> > > +      DEBUG_INFO,
> > >
> > > +      "%a: Failed to init controller, with Status - %r\n",
> > >
> > > +      __FUNCTION__,
> > >
> > > +      Status
> > >
> > > +      ));
> > >
> > > +    }
> > >
> > > +    //
> > >
> > > +    // Override the status to continue the for loop
> > >
> > > +    //
> > >
> > > +    Status = EFI_SUCCESS;
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  return EFI_SUCCESS;
> > >
> > > +}
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Entry point of the PEIM.
> > >
> > > +
> > >
> > > +  @param[in] FileHandle     Handle of the file being invoked.
> > >
> > > +  @param[in] PeiServices    Describes the list of possible PEI Services.
> > >
> > > +
> > >
> > > +  @retval EFI_SUCCESS    PPI successfully installed.
> > >
> > > +
> > >
> > > +**/
> > >
> > > +EFI_STATUS
> > >
> > > +EFIAPI
> > >
> > > +AtaAhciPeimEntry (
> > >
> > > +  IN EFI_PEI_FILE_HANDLE     FileHandle,
> > >
> > > +  IN CONST EFI_PEI_SERVICES  **PeiServices
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  EFI_STATUS                          Status;
> > >
> > > +  EDKII_ATA_AHCI_HOST_CONTROLLER_PPI  *AhciHcPpi;
> > >
> > > +
> > >
> > > +  DEBUG ((DEBUG_INFO, "%a: Enters.\n", __FUNCTION__));
> > >
> > > +
> > >
> > > +  PeiServicesNotifyPpi (&mAtaAhciHostControllerNotify);
> > >
> > > +
> > >
> > > +  PeiServicesNotifyPpi (&mPciDevicePpiNotify);
> > >
> > > +
> > >
> > > +  Status = AtaAhciInitPrivateDataFromPciDevice ();
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // Locate the ATA AHCI host controller PPI.
> > >
> > > +  //
> > >
> > > +  Status = PeiServicesLocatePpi (
> > >
> > > +             &gEdkiiPeiAtaAhciHostControllerPpiGuid,
> > >
> > > +             0,
> > >
> > > +             NULL,
> > >
> > > +             (VOID **)&AhciHcPpi
> > >
> > > +             );
> > >
> > > +  if (!EFI_ERROR (Status)) {
> > >
> > > +    DEBUG ((DEBUG_ERROR, "%a: Using AtaAhciHostControllerPpi to
> > > + initialize
> > > private data.\n", __FUNCTION__));
> >
> >
> > Suggest to use DEBUG_INFO here.
> 
> Ok, will fix that in v2 patch.
> 
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > +    Status = AtaAhciInitPrivateDataFromHostControllerPpi
> > > + (AhciHcPpi);
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  return Status;
> > >
> > > +}
> > >
> > > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c
> > > b/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c
> > > index 81f8743d40d8..cf0955929dde 100644
> > > --- a/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c
> > > +++ b/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c
> > > @@ -38,50 +38,6 @@ EFI_DEVICE_PATH_PROTOCOL
> > > mAhciEndDevicePathNodeTemplate = {
> > >    }
> > >
> > >  };
> > >
> > >
> > >
> > > -/**
> > >
> > > -  Returns the 16-bit Length field of a device path node.
> > >
> > > -
> > >
> > > -  Returns the 16-bit Length field of the device path node specified by
> Node.
> > >
> > > -  Node is not required to be aligned on a 16-bit boundary, so it is
> > > recommended
> > >
> > > -  that a function such as ReadUnaligned16() be used to extract the
> > > contents of
> > >
> > > -  the Length field.
> > >
> > > -
> > >
> > > -  If Node is NULL, then ASSERT().
> > >
> > > -
> > >
> > > -  @param  Node      A pointer to a device path node data structure.
> > >
> > > -
> > >
> > > -  @return The 16-bit Length field of the device path node specified by
> Node.
> > >
> > > -
> > >
> > > -**/
> > >
> > > -UINTN
> > >
> > > -DevicePathNodeLength (
> > >
> > > -  IN CONST VOID  *Node
> > >
> > > -  )
> > >
> > > -{
> > >
> > > -  ASSERT (Node != NULL);
> > >
> > > -  return ReadUnaligned16 ((UINT16 *)&((EFI_DEVICE_PATH_PROTOCOL
> > > *)(Node))->Length[0]);
> > >
> > > -}
> > >
> > > -
> > >
> > > -/**
> > >
> > > -  Returns a pointer to the next node in a device path.
> > >
> > > -
> > >
> > > -  If Node is NULL, then ASSERT().
> > >
> > > -
> > >
> > > -  @param  Node    A pointer to a device path node data structure.
> > >
> > > -
> > >
> > > -  @return a pointer to the device path node that follows the device
> > > path node
> > >
> > > -  specified by Node.
> > >
> > > -
> > >
> > > -**/
> > >
> > > -EFI_DEVICE_PATH_PROTOCOL *
> > >
> > > -NextDevicePathNode (
> > >
> > > -  IN CONST VOID  *Node
> > >
> > > -  )
> > >
> > > -{
> > >
> > > -  ASSERT (Node != NULL);
> > >
> > > -  return (EFI_DEVICE_PATH_PROTOCOL *)((UINT8 *)(Node) +
> > > DevicePathNodeLength (Node));
> > >
> > > -}
> > >
> > > -
> > >
> > >  /**
> > >
> > >    Get the size of the current device path instance.
> > >
> > >
> > >
> > > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
> > > b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
> > > index 912ff7a8ba4f..788660b33299 100644
> > > --- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
> > > +++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf
> > > @@ -50,11 +50,13 @@ [LibraryClasses]
> > >    TimerLib
> > >
> > >    LockBoxLib
> > >
> > >    PeimEntryPoint
> > >
> > > +  DevicePathLib
> > >
> > >
> > >
> > >  [Ppis]
> > >
> > >    gEdkiiPeiAtaAhciHostControllerPpiGuid          ## CONSUMES
> > >
> > >    gEdkiiIoMmuPpiGuid                             ## CONSUMES
> > >
> > >    gEfiEndOfPeiSignalPpiGuid                      ## CONSUMES
> > >
> > > +  gEdkiiPeiPciDevicePpiGuid                      ## CONSUMES
> > >
> > >    gEdkiiPeiAtaPassThruPpiGuid                    ## SOMETIMES_PRODUCES
> > >
> > >    gEfiPeiVirtualBlockIoPpiGuid                   ## SOMETIMES_PRODUCES
> > >
> > >    gEfiPeiVirtualBlockIo2PpiGuid                  ## SOMETIMES_PRODUCES
> > >
> > > @@ -65,8 +67,7 @@ [Guids]
> > >
> > >
> > >  [Depex]
> > >
> > >    gEfiPeiMemoryDiscoveredPpiGuid AND
> > >
> > > -  gEfiPeiMasterBootModePpiGuid AND
> > >
> > > -  gEdkiiPeiAtaAhciHostControllerPpiGuid
> > >
> > > +  gEfiPeiMasterBootModePpiGuid
> > >
> > >
> > >
> > >  [UserExtensions.TianoCore."ExtraFiles"]
> > >
> > >    AhciPeiExtra.uni
> > >
> > > --
> > > 2.27.0.windows.1
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90509): https://edk2.groups.io/g/devel/message/90509
Mute This Topic: https://groups.io/mt/91575908/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