From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: hao.a.wu@intel.com) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by groups.io with SMTP; Mon, 22 Apr 2019 22:23:00 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Apr 2019 22:22:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,384,1549958400"; d="scan'208";a="137973422" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga006.jf.intel.com with ESMTP; 22 Apr 2019 22:17:59 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 22:17:59 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 22:17:58 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.25]) with mapi id 14.03.0415.000; Tue, 23 Apr 2019 13:17:56 +0800 From: "Wu, Hao A" To: "Ni, Ray" , "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 Thread-Topic: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode devices Thread-Index: AQHU+UjM5VrsYkbWF0G4mKN0rAhKEaZI7q+w Date: Tue, 23 Apr 2019 05:17:56 +0000 Message-ID: References: <20190410011313.4268-1-hao.a.wu@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C0FC8E8@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C0FC8E8@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 >=20 > 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. >=20 > Private =3D GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO (This); >=20 > // > // Check parameters > // > if ((This =3D=3D NULL) || > (DeviceIndex =3D=3D 0) || > (DeviceIndex > Private->ActiveDevices)) { > return EFI_INVALID_PARAMETER; > } >=20 > DeviceData =3D SearchDeviceByIndex (Private, DeviceIndex); > if (DeviceData =3D=3D NULL) { > return EFI_NOT_FOUND; > } OK. I can do this in V3. >=20 > 3. For the checks in AhciRead(), how about re-order as below? >=20 > BlockSize =3D DeviceData->Media.BlockSize; > if ((BufferSize % BlockSize) !=3D 0) { > return EFI_BAD_BUFFER_SIZE; > } >=20 > if (StartLba > DeviceData->Media.LastBlock) { > return EFI_INVALID_PARAMETER; > } > NumberOfBlocks =3D BufferSize / BlockSize; > if (NumberOfBlocks - 1 > DeviceData->Media.LastBlock - StartLba) { > return EFI_INVALID_PARAMETER; > } >=20 > if (BufferSize =3D=3D 0) { > return EFI_SUCCESS; > } >=20 > if (Buffer =3D=3D 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? >=20 > 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|0x0= 0000003=20 gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3|0x00200000|UINT32|0= x00000004 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 >=20 > // > // 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 si= ze. > // > #define MAX_48BIT_TRANSFER_BLOCK_NUM 0x800 >=20 >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Tuesday, April 9, 2019 6:13 PM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A ; Ni, Ray ; Dong, > > Eric ; Wang, Jian J > > Subject: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode devic= es > > > > 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 > > Cc: Eric Dong > > Cc: Jian J Wang > > > > 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