Comments: 1. Can we add EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST to mAhciAtaPassThruPpiListTemplate/mAhciBlkIoPpiListTemplate as well? I understand that requires calling PeiServicesInstallPpi() 3 times instead of once. But I see the benefit of change is the code is more understandable and potential bug-free. And you could also create a global mAhciBlkIoPpi/mAhciBlkIo2Ppi variables and let the mAhciBlkIoPpiListTemplate/mAhciBlkIo2PpiListTemplate point to them respectively. The reason we cannot have a global AtaPassThruPpi is there is a Mode field in the PPI that cannot be read-only. 2. Below code snip is used by AhciBlockIoReadBlocks(),AhciBlockIoGetMediaInfo(), AhciBlockIoGetMediaInfo2() in AhciPeiBlockIo.c, maybe you can re-write SearchDeviceByIndex() to include the below code. Private = GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO (This);
// // Check parameters // if ((This == NULL) || (DeviceIndex == 0) || (DeviceIndex > Private->ActiveDevices)) { return EFI_INVALID_PARAMETER; } DeviceData = SearchDeviceByIndex (Private, DeviceIndex); if (DeviceData == NULL) { return EFI_NOT_FOUND; } 3. For the checks in AhciRead(), how about re-order as below? BlockSize = DeviceData->Media.BlockSize; if ((BufferSize % BlockSize) != 0) { return EFI_BAD_BUFFER_SIZE; } if (StartLba > DeviceData->Media.LastBlock) { return EFI_INVALID_PARAMETER; } NumberOfBlocks = BufferSize / BlockSize; if (NumberOfBlocks - 1 > DeviceData->Media.LastBlock - StartLba) { return EFI_INVALID_PARAMETER; } if (BufferSize == 0) { return EFI_SUCCESS; } if (Buffer == NULL) { return EFI_INVALID_PARAMETER; } 4. Can you explain more for the first patch? Why VTd PEI module cannot be changed to support 0xFFFF? // // Due to limited resource on VTd PEI DMA buffer size, driver limits the // maximum transfer block number for 48-bit addressing. // Setting to 0x800 here means 1M bytes for device with 512-byte block size. // #define MAX_48BIT_TRANSFER_BLOCK_NUM 0x800 > -----Original Message----- > From: Wu, Hao A > Sent: Tuesday, April 9, 2019 6:13 PM > To: devel@edk2.groups.io > Cc: Wu, Hao A <hao.a...@intel.com>; Ni, Ray <ray...@intel.com>; Dong, > Eric <eric.d...@intel.com>; Wang, Jian J <jian.j.w...@intel.com> > Subject: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode devices > > The series is also available at: > https://github.com/hwu25/edk2/tree/ahci_pei_blockio_v2 > > > V2 changges: > > Due to the file license change, rebase the whole series onto the tip of the > master branch. The 'Contributed-under' tag is removed from the log > messages as well. > > > V1 history: > > The series will add the PEI BlockIO support for ATA AHCI mode devices. > > Cc: Ray Ni <ray...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > > Hao Wu (2): > MdeModulePkg/AhciPei: Limit max transfer blocknum for 48-bit address > MdeModulePkg/AhciPei: Add PEI BlockIO support > > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf | 4 + > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h | 30 ++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiBlockIo.h | 257 ++++++++++ > MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c | 125 ++++- > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c | 35 +- > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiBlockIo.c | 526 > ++++++++++++++++++++ > 6 files changed, 975 insertions(+), 2 deletions(-) create mode 100644 > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiBlockIo.h > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPeiBlockIo.c > > -- > 2.12.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39371): https://edk2.groups.io/g/devel/message/39371 Mute This Topic: https://groups.io/mt/31014379/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-