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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, April 23, 2019 4:20 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Wang, Jian J
> Subject: RE: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode
> devices
> 
> 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

Agree. I will update the codes in V3.

> potential bug-free. And you could also create a global
> mAhciBlkIoPpi/mAhciBlkIo2Ppi variables and let the
> mAhciBlkIoPpiListTemplate/mAhciBlkIo2PpiListTemplate point to them

Please correct me if I am wrong.

The 'Ppi' field within mAhciBlkIoPpiListTemplate/mAhciBlkIo2PpiListTemplate
will be used by BlockIO(2) PPI services implementation to get the private
data for every AHCI ATA controller (or i.e. PPI instance), by using
definitions:

GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO
GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO2

If 'Ppi' field are of the same value among controllers, it seems to me
getting the controller private data will not work.

> 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;
>   }

OK. I can do this in V3.

> 
> 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;
>   }

The current logic is align with its DXE counterpart:
MdeModulePkg\Bus\Ata\AtaBusDxe\AtaBus.c - Line 1017: BlockIoReadWrite (

Could you help to provide some detailed reason for your suggestion?

> 
> 4. Can you explain more for the first patch? Why VTd PEI module cannot be
> changed to support 0xFFFF?

According to PCDs:
gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSize|0x00400000|UINT32|0x00000003 
gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3|0x00200000|UINT32|0x00000004

The default buffer size for IOMMU mapping allocated is 2M bytes for S3 and
4M bytes for non-S3 cases.

When 48-bit maximum block number is 0xFFFF and block size is 512 bytes,
the maximum buffer allowed for mapping within AhciPei driver will be close
to 32M bytes. Thus, the 1st patch limits the 48-bit maximum block number
to 0x800 (as mentioned by the comments for the definition), meaning 1M
bytes for device with 512-byte block size.

Best Regards,
Hao Wu

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