Re: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode devices

["Ni, Ray" <ray.ni@intel.com>]

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.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@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.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@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