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

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

> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, April 22, 2019 10:18 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode
> devices
> 
> > -----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.
Thanks.

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

I agree. Your current code is good.

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

Thanks.

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

Then I'm ok with that.

> 
> >
> > 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|UIN
> T32|0x00000003
> gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3|0x00200000|UI
> NT32|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.

Thanks for the explanation. Can you put the explanation to code comments?
 
> 
> 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