[edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device
Wu, Hao A
hao.a.wu at intel.com
Thu Jun 9 03:08:17 UTC 2022
For "3) Could you help to check if the DMA memory related codes in MdeModulePkg\Bus\Ata\AhciPei\DmaMem.c can be covered by the 'PciIo' service in EDKII_PCI_DEVICE_PPI?"
After a second thought, my take is that there will be no PciBusPei implementation added in edk2.
So there will be no enforcement for producers of EDKII_PCI_DEVICE_PPI to add IOMMU support like in PciBusDxe.
If my above understanding is correct, then I think we might still need to keep those IOMMU support codes in AhciPei PEIM.
Best Regards,
Hao Wu
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Wu, Hao A
> Sent: Thursday, June 9, 2022 10:48 AM
> To: Czajkowski, Maciej <maciej.czajkowski at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at intel.com>
> Subject: Re: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use
> PCI_DEVICE_PPI to manage AHCI device
>
> Couple of general level comments/questions:
> 1) The implementation of functions AtaAhciPciDevicePpiInstallationCallback() &
> AtaAhciInitPrivateDataFromPciDevice() has many duplications. Is it possible to
> abstract a separate function to reduce duplicated codes?
> 2) What DevicePathLib instance should be used for the PEI case? As far as I know,
> current DevicePathLib instances in edk2 do not support PEIM.
> 3) Could you help to check if the DMA memory related codes in
> MdeModulePkg\Bus\Ata\AhciPei\DmaMem.c can be covered by the 'PciIo'
> service in EDKII_PCI_DEVICE_PPI?
> 4) May I know what kind of tests are performed for this patch? Would like to
> ensure the origin gEdkiiPeiAtaAhciHostControllerPpiGuid path is not broken.
> 5) Could you help to create a GitHub Pull Request to trigger the CI tests for this
> series?
>
> More inline comments below:
>
>
> > -----Original Message-----
> > From: Czajkowski, Maciej <maciej.czajkowski at intel.com>
> > Sent: Monday, June 6, 2022 8:45 PM
> > To: devel at edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>
> > Subject: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use
> > PCI_DEVICE_PPI to manage AHCI device
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3907
> >
> > This change modifies AhciPei library to allow usage both
> > EDKII_PCI_DEVICE_PPI and EDKII_PEI_ATA_AHCI_HOST_CONTROLLER_PPI to
> > manage ATA HDD working under AHCI mode.
> >
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Signed-off-by: Maciej Czajkowski <maciej.czajkowski at intel.com>
> > ---
> > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c | 615 +++++++++++++++-----
> > MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c | 44 --
> > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf | 5 +-
> > 3 files changed, 458 insertions(+), 206 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > index 208b7e9a3606..31bb3c0760ab 100644
> > --- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > +++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c
> > @@ -9,6 +9,47 @@
> > **/
> >
> >
> >
> > #include "AhciPei.h"
> >
> > +#include <Ppi/PciDevice.h>
> >
> > +#include <Library/DevicePathLib.h>
> >
> > +#include <IndustryStandard/Pci.h>
> >
> > +
> >
> > +/**
> >
> > + Callback for EDKII_ATA_AHCI_HOST_CONTROLLER_PPI installation.
> >
> > +
> >
> > + @param[in] PeiServices Pointer to PEI Services Table.
> >
> > + @param[in] NotifyDescriptor Pointer to the descriptor for the Notification
> >
> > + event that caused this function to execute.
> >
> > + @param[in] Ppi Pointer to the PPI data associated with this
> function.
> >
> > +
> >
> > + @retval EFI_SUCCESS The function completes successfully
> >
> > +
> >
> > +**/
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> > +AtaAhciHostControllerPpiInstallationCallback (
> >
> > + IN EFI_PEI_SERVICES **PeiServices,
> >
> > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
> >
> > + IN VOID *Ppi
> >
> > + );
> >
> > +
> >
> > +/**
> >
> > + Callback for EDKII_PCI_DEVICE_PPI installation.
> >
> > +
> >
> > + @param[in] PeiServices Pointer to PEI Services Table.
> >
> > + @param[in] NotifyDescriptor Pointer to the descriptor for the Notification
> >
> > + event that caused this function to execute.
> >
> > + @param[in] Ppi Pointer to the PPI data associated with this
> function.
> >
> > +
> >
> > + @retval EFI_SUCCESS The function completes successfully
> >
> > +
> >
> > +**/
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> > +AtaAhciPciDevicePpiInstallationCallback (
> >
> > + IN EFI_PEI_SERVICES **PeiServices,
> >
> > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
> >
> > + IN VOID *Ppi
> >
> > + );
>
>
> Could you help to put the above 2 function declarations in AhciPei.h to keep
> consistency?
>
>
> >
> >
> >
> > 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list