> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, April 22, 2019 10:18 PM
> To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@intel.com>; Wang, Jian J <jian.j.w...@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...@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 (#39394): https://edk2.groups.io/g/devel/message/39394
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]
-=-=-=-=-=-=-=-=-=-=-=-
