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.czajkow...@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray...@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.czajkow...@intel.com> > > Sent: Monday, June 6, 2022 8:45 PM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A <hao.a...@intel.com>; Ni, Ray <ray...@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...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Signed-off-by: Maciej Czajkowski <maciej.czajkow...@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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90348): https://edk2.groups.io/g/devel/message/90348 Mute This Topic: https://groups.io/mt/91575908/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
