[edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to EDK2

[edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to EDK2

[Maciej Czajkowski]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907

The purpose of those changes is to introduce the way to enumerate and assign resources in PEI
for the systems with more than one PCI root. Here is a need to have an interface that will
support such a mechanizm.
For now, the part that performs the enumeration will be implemented in the silicon code.
Sample code can be seen here: https://github.com/mczaj/edk2-platforms/commit/d443062e58f9fba228869b54f2546d9735b3b506

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Maciej Czajkowski (2):
  MdeModulePkg: Add EDKII_PCI_DEVICE_PPI definition
  MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device

 MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c    | 615 +++++++++++++++-----
 MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c |  44 --
 MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf  |   5 +-
 MdeModulePkg/Include/Ppi/PciDevice.h      |  32 +
 MdeModulePkg/MdeModulePkg.dec             |   3 +
 5 files changed, 493 insertions(+), 206 deletions(-)
 create mode 100644 MdeModulePkg/Include/Ppi/PciDevice.h

-- 
2.27.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

[edk2-devel][PATCH v1 1/2] MdeModulePkg: Add EDKII_PCI_DEVICE_PPI definition

[Maciej Czajkowski]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907

This commit introduces EDKII_PCI_DEVICE_PPI. The purpose of this PPI is
to provide a way of accessing PCI devices to drvice drivers such as
NvmExpressPei or AhciPei.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Maciej Czajkowski <maciej.czajkowski@intel.com>
---
 MdeModulePkg/Include/Ppi/PciDevice.h | 32 ++++++++++++++++++++
 MdeModulePkg/MdeModulePkg.dec        |  3 ++
 2 files changed, 35 insertions(+)

diff --git a/MdeModulePkg/Include/Ppi/PciDevice.h b/MdeModulePkg/Include/Ppi/PciDevice.h
new file mode 100644
index 000000000000..3e391c61f6d9
--- /dev/null
+++ b/MdeModulePkg/Include/Ppi/PciDevice.h
@@ -0,0 +1,32 @@
+/** @file
+
+  Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _EDKII_PCI_DEVICE_PPI_H_
+#define _EDKII_PCI_DEVICE_PPI_H_
+
+#include <Protocol/PciIo.h>
+#include <Protocol/DevicePath.h>
+
+///
+/// Global ID for the EDKII_PCI_DEVICE_PPI_GUID.
+///
+#define EDKII_PCI_DEVICE_PPI_GUID \
+  { \
+    0x1597ab4f, 0xd542, 0x4efe, { 0x9a, 0xf7, 0xb2, 0x44, 0xec, 0x54, 0x4c, 0x0b } \
+  }
+
+///
+/// PCI Device PPI structure.
+///
+typedef struct {
+  EFI_PCI_IO_PROTOCOL       PciIo;
+  EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
+} EDKII_PCI_DEVICE_PPI;
+
+extern EFI_GUID  gEdkiiPeiPciDevicePpiGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 2bcb9f9453af..7d989108324a 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -509,6 +509,9 @@ [Ppis]
   ## Include/Ppi/NvmExpressPassThru.h
   gEdkiiPeiNvmExpressPassThruPpiGuid    = { 0x6af31b2c, 0x3be, 0x46c1, { 0xb1, 0x2d, 0xea, 0x4a, 0x36, 0xdf, 0xa7, 0x4c } }
 
+  ## Include/Ppi/PciDevice.h
+  gEdkiiPeiPciDevicePpiGuid                 = { 0x1597ab4f, 0xd542, 0x4efe, { 0x9a, 0xf7, 0xb2, 0x44, 0xec, 0x54, 0x4c, 0x0b } }
+
   ## Include/Ppi/CapsuleOnDisk.h
   gEdkiiPeiCapsuleOnDiskPpiGuid             = { 0x71a9ea61, 0x5a35, 0x4a5d, { 0xac, 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }
   gEdkiiPeiBootInCapsuleOnDiskModePpiGuid   = { 0xb08a11e4, 0xe2b7, 0x4b75, { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1  } }
-- 
2.27.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [edk2-devel][PATCH v1 1/2] MdeModulePkg: Add EDKII_PCI_DEVICE_PPI definition

[Wu, Hao A]

>From the device controllers perspective, current drive implementation needs the below information:
* AHCI: HC MMIO BAR & DevicePath
* NVMe: HC MMIO BAR & DevicePath
* UFS: HC MMIO BAR
* SD/MMC: HC MMIO BAR
* USB - XHCI: HC MMIO BAR & USB controller type (I think this can be identified by consuming the HC PCI configuration space)
* USB - EHCI: Same as XHCI
* USB - UHCI: Same as XHCI

I think the interface defined in EDKII_PCI_DEVICE_PPI is sufficient for above needs.
Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: Czajkowski, Maciej <maciej.czajkowski@intel.com>
> Sent: Monday, June 6, 2022 8:45 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel][PATCH v1 1/2] MdeModulePkg: Add
> EDKII_PCI_DEVICE_PPI definition
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907
> 
> This commit introduces EDKII_PCI_DEVICE_PPI. The purpose of this PPI is to
> provide a way of accessing PCI devices to drvice drivers such as NvmExpressPei
> or AhciPei.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Maciej Czajkowski <maciej.czajkowski@intel.com>
> ---
>  MdeModulePkg/Include/Ppi/PciDevice.h | 32 ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec        |  3 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/MdeModulePkg/Include/Ppi/PciDevice.h
> b/MdeModulePkg/Include/Ppi/PciDevice.h
> new file mode 100644
> index 000000000000..3e391c61f6d9
> --- /dev/null
> +++ b/MdeModulePkg/Include/Ppi/PciDevice.h
> @@ -0,0 +1,32 @@
> +/** @file++  Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>+
> SPDX-License-Identifier: BSD-2-Clause-Patent++**/++#ifndef
> _EDKII_PCI_DEVICE_PPI_H_+#define _EDKII_PCI_DEVICE_PPI_H_++#include
> <Protocol/PciIo.h>+#include <Protocol/DevicePath.h>++///+/// Global ID for
> the EDKII_PCI_DEVICE_PPI_GUID.+///+#define EDKII_PCI_DEVICE_PPI_GUID \+
> { \+    0x1597ab4f, 0xd542, 0x4efe, { 0x9a, 0xf7, 0xb2, 0x44, 0xec, 0x54, 0x4c,
> 0x0b } \+  }++///+/// PCI Device PPI structure.+///+typedef struct {+
> EFI_PCI_IO_PROTOCOL       PciIo;+  EFI_DEVICE_PATH_PROTOCOL
> *DevicePath;+} EDKII_PCI_DEVICE_PPI;++extern EFI_GUID
> gEdkiiPeiPciDevicePpiGuid;++#endifdiff --git
> a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 2bcb9f9453af..7d989108324a 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -509,6 +509,9 @@ [Ppis]
>    ## Include/Ppi/NvmExpressPassThru.h   gEdkiiPeiNvmExpressPassThruPpiGuid
> = { 0x6af31b2c, 0x3be, 0x46c1, { 0xb1, 0x2d, 0xea, 0x4a, 0x36, 0xdf, 0xa7,
> 0x4c } } +  ## Include/Ppi/PciDevice.h+  gEdkiiPeiPciDevicePpiGuid                 =
> { 0x1597ab4f, 0xd542, 0x4efe, { 0x9a, 0xf7, 0xb2, 0x44, 0xec, 0x54, 0x4c,
> 0x0b } }+   ## Include/Ppi/CapsuleOnDisk.h   gEdkiiPeiCapsuleOnDiskPpiGuid
> = { 0x71a9ea61, 0x5a35, 0x4a5d, { 0xac, 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67,
> 0xe0 } }   gEdkiiPeiBootInCapsuleOnDiskModePpiGuid   = { 0xb08a11e4, 0xe2b7,
> 0x4b75, { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1  } }--
> 2.27.0.windows.1

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

[Maciej Czajkowski]

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@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Maciej Czajkowski <maciej.czajkowski@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
+  );
 
 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
+
+**/
+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
+
+**/
+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
+                                );
+  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;
+}
+
+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__));
+    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

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

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

[Wu, Hao A]

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@intel.com>
> Sent: Monday, June 6, 2022 8:45 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Maciej Czajkowski <maciej.czajkowski@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?


> 
> 
> 
>  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.


> 
> +
> 
> +**/
> 
> +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.


> 
> +
> 
> +**/
> 
> +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
                                     );
  }


> 
> +  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.


> +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.

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

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

[Wu, Hao A]

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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
> Sent: Thursday, June 9, 2022 10:48 AM
> To: Czajkowski, Maciej <maciej.czajkowski@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@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@intel.com>
> > Sent: Monday, June 6, 2022 8:45 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Maciej Czajkowski <maciej.czajkowski@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?
> 
> 
> >
> >
> >
> >  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.
> 
> 
> >
> > +
> >
> > +**/
> >
> > +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.
> 
> 
> >
> > +
> >
> > +**/
> >
> > +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
>                                      );
>   }
> 
> 
> >
> > +  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.
> 
> 
> > +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.
> 
> 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
> 
> 
> 
> 
> 

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

[Maciej Czajkowski]

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@intel.com> 
Sent: czwartek, 9 czerwca 2022 05:08
To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Czajkowski, Maciej <maciej.czajkowski@intel.com>
Cc: Ni, Ray <ray.ni@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao 
> A
> Sent: Thursday, June 9, 2022 10:48 AM
> To: Czajkowski, Maciej <maciej.czajkowski@intel.com>; 
> devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@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@intel.com>
> > Sent: Monday, June 6, 2022 8:45 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Maciej Czajkowski <maciej.czajkowski@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
> 
> 
> 
> 
> 

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

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

[Wu, Hao A]

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@intel.com>
> Sent: Monday, June 13, 2022 9:20 PM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@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@intel.com>
> Sent: czwartek, 9 czerwca 2022 05:08
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Czajkowski,
> Maciej <maciej.czajkowski@intel.com>
> Cc: Ni, Ray <ray.ni@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Hao
> > A
> > Sent: Thursday, June 9, 2022 10:48 AM
> > To: Czajkowski, Maciej <maciej.czajkowski@intel.com>;
> > devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@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@intel.com>
> > > Sent: Monday, June 6, 2022 8:45 PM
> > > To: devel@edk2.groups.io
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Signed-off-by: Maciej Czajkowski <maciej.czajkowski@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
> >
> >
> >
> > 
> >

Re: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to EDK2

[Wu, Hao A]

Sorry for a question, if the EDKII_PCI_DEVICE_PPI were added to edk2, would there be a plan to add support to:
* NVMe
* UFS
* SD/MMC
* USB (XHCI, EHCI and UHCI)

Best Regards,
Hao Wu

> -----Original Message-----
> From: Czajkowski, Maciej <maciej.czajkowski@intel.com>
> Sent: Monday, June 6, 2022 8:45 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to
> EDK2
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907
> 
> The purpose of those changes is to introduce the way to enumerate and assign
> resources in PEI
> for the systems with more than one PCI root. Here is a need to have an interface
> that will
> support such a mechanizm.
> For now, the part that performs the enumeration will be implemented in the
> silicon code.
> Sample code can be seen here: https://github.com/mczaj/edk2-
> platforms/commit/d443062e58f9fba228869b54f2546d9735b3b506
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> 
> Maciej Czajkowski (2):
>   MdeModulePkg: Add EDKII_PCI_DEVICE_PPI definition
>   MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device
> 
>  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c    | 615 +++++++++++++++-----
>  MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c |  44 --
>  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf  |   5 +-
>  MdeModulePkg/Include/Ppi/PciDevice.h      |  32 +
>  MdeModulePkg/MdeModulePkg.dec             |   3 +
>  5 files changed, 493 insertions(+), 206 deletions(-)
>  create mode 100644 MdeModulePkg/Include/Ppi/PciDevice.h
> 
> --
> 2.27.0.windows.1

Re: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to EDK2

[Maciej Czajkowski]

For now, the priority will be to add the support for AHCI and NVMe. However, in the future the plan is to have support in all of these drivers.

Regards,
Maciej

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: czwartek, 9 czerwca 2022 04:47
To: Czajkowski, Maciej <maciej.czajkowski@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: RE: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to EDK2

Sorry for a question, if the EDKII_PCI_DEVICE_PPI were added to edk2, would there be a plan to add support to:
* NVMe
* UFS
* SD/MMC
* USB (XHCI, EHCI and UHCI)

Best Regards,
Hao Wu

> -----Original Message-----
> From: Czajkowski, Maciej <maciej.czajkowski@intel.com>
> Sent: Monday, June 6, 2022 8:45 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao, 
> Liming <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support 
> to
> EDK2
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907
> 
> The purpose of those changes is to introduce the way to enumerate and 
> assign resources in PEI for the systems with more than one PCI root. 
> Here is a need to have an interface that will support such a 
> mechanizm.
> For now, the part that performs the enumeration will be implemented in 
> the silicon code.
> Sample code can be seen here: https://github.com/mczaj/edk2-
> platforms/commit/d443062e58f9fba228869b54f2546d9735b3b506
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> 
> Maciej Czajkowski (2):
>   MdeModulePkg: Add EDKII_PCI_DEVICE_PPI definition
>   MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device
> 
>  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c    | 615 +++++++++++++++-----
>  MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c |  44 --
>  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf  |   5 +-
>  MdeModulePkg/Include/Ppi/PciDevice.h      |  32 +
>  MdeModulePkg/MdeModulePkg.dec             |   3 +
>  5 files changed, 493 insertions(+), 206 deletions(-)  create mode 
> 100644 MdeModulePkg/Include/Ppi/PciDevice.h
> 
> --
> 2.27.0.windows.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support to EDK2

[Wu, Hao A]

Got it, thanks for the information. I am fine with the plan.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Czajkowski, Maciej <maciej.czajkowski@intel.com>
> Sent: Monday, June 13, 2022 9:20 PM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: RE: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support
> to EDK2
> 
> For now, the priority will be to add the support for AHCI and NVMe. However,
> in the future the plan is to have support in all of these drivers.
> 
> Regards,
> Maciej
> 
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: czwartek, 9 czerwca 2022 04:47
> To: Czajkowski, Maciej <maciej.czajkowski@intel.com>;
> devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: RE: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support
> to EDK2
> 
> Sorry for a question, if the EDKII_PCI_DEVICE_PPI were added to edk2,
> would there be a plan to add support to:
> * NVMe
> * UFS
> * SD/MMC
> * USB (XHCI, EHCI and UHCI)
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: Czajkowski, Maciej <maciej.czajkowski@intel.com>
> > Sent: Monday, June 6, 2022 8:45 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao,
> > Liming <gaoliming@byosoft.com.cn>
> > Subject: [edk2-devel][PATCH v1 0/2] Add EDKII_PCI_DEVICE_PPI support
> > to
> > EDK2
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907
> >
> > The purpose of those changes is to introduce the way to enumerate and
> > assign resources in PEI for the systems with more than one PCI root.
> > Here is a need to have an interface that will support such a
> > mechanizm.
> > For now, the part that performs the enumeration will be implemented in
> > the silicon code.
> > Sample code can be seen here: https://github.com/mczaj/edk2-
> > platforms/commit/d443062e58f9fba228869b54f2546d9735b3b506
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >
> > Maciej Czajkowski (2):
> >   MdeModulePkg: Add EDKII_PCI_DEVICE_PPI definition
> >   MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device
> >
> >  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c    | 615 +++++++++++++++----
> -
> >  MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c |  44 --
> >  MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf  |   5 +-
> >  MdeModulePkg/Include/Ppi/PciDevice.h      |  32 +
> >  MdeModulePkg/MdeModulePkg.dec             |   3 +
> >  5 files changed, 493 insertions(+), 206 deletions(-)  create mode
> > 100644 MdeModulePkg/Include/Ppi/PciDevice.h
> >
> > --
> > 2.27.0.windows.1