From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: ray.ni@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Mon, 22 Apr 2019 23:40:22 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Apr 2019 23:40:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,384,1549958400"; d="scan'208";a="163974171" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga002.fm.intel.com with ESMTP; 22 Apr 2019 23:40:22 -0700 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 23:40:21 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 23:40:21 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.39]) with mapi id 14.03.0415.000; Tue, 23 Apr 2019 14:40:20 +0800 From: "Ni, Ray" 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 Thread-Topic: [PATCH v2 0/2] Add PEI BlockIO support for ATA AHCI mode devices Thread-Index: AQHU7zqYJPHGfqAn5ky/ispxh59or6ZIbLrggABWhQCAAJugYA== Date: Tue, 23 Apr 2019 06:40:19 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C0FD8E3@SHSMSX104.ccr.corp.intel.com> References: <20190410011313.4268-1-hao.a.wu@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C0FC8E8@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2Y3M2JlNGYtMDI2MC00ZGIyLTliYzYtOTBhMDdjMTM0MWU3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiNFR0K1p4b3VMNWg5UzRhWVJsN2puYTBVdEliMXBGMXY4NDZSWGxqV0RjQngyRDF1aUkrM2Ztc09KdlF3bnN6VSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Wu, Hao A > Sent: Monday, April 22, 2019 10:18 PM > 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 >=20 > > -----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 instea= d of > once. > > But I see the benefit of change is the code is more understandable and >=20 > Agree. I will update the codes in V3. Thanks. >=20 > > potential bug-free. And you could also create a global > > mAhciBlkIoPpi/mAhciBlkIo2Ppi variables and let the > > mAhciBlkIoPpiListTemplate/mAhciBlkIo2PpiListTemplate point to them >=20 > Please correct me if I am wrong. >=20 > 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: >=20 > GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO > GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO2 >=20 > If 'Ppi' field are of the same value among controllers, it seems to me ge= tting > the controller private data will not work. I agree. Your current code is good. >=20 > > 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 =3D GET_AHCI_PEIM_HC_PRIVATE_DATA_FROM_THIS_BLKIO (This); > > > > // > > // Check parameters > > // > > if ((This =3D=3D NULL) || > > (DeviceIndex =3D=3D 0) || > > (DeviceIndex > Private->ActiveDevices)) { > > return EFI_INVALID_PARAMETER; > > } > > > > DeviceData =3D SearchDeviceByIndex (Private, DeviceIndex); > > if (DeviceData =3D=3D NULL) { > > return EFI_NOT_FOUND; > > } >=20 > OK. I can do this in V3. Thanks. >=20 > > > > 3. For the checks in AhciRead(), how about re-order as below? > > > > BlockSize =3D DeviceData->Media.BlockSize; > > if ((BufferSize % BlockSize) !=3D 0) { > > return EFI_BAD_BUFFER_SIZE; > > } > > > > if (StartLba > DeviceData->Media.LastBlock) { > > return EFI_INVALID_PARAMETER; > > } > > NumberOfBlocks =3D BufferSize / BlockSize; > > if (NumberOfBlocks - 1 > DeviceData->Media.LastBlock - StartLba) { > > return EFI_INVALID_PARAMETER; > > } > > > > if (BufferSize =3D=3D 0) { > > return EFI_SUCCESS; > > } > > > > if (Buffer =3D=3D NULL) { > > return EFI_INVALID_PARAMETER; > > } >=20 > The current logic is align with its DXE counterpart: > MdeModulePkg\Bus\Ata\AtaBusDxe\AtaBus.c - Line 1017: BlockIoReadWrite > ( >=20 > Could you help to provide some detailed reason for your suggestion? Then I'm ok with that. >=20 > > > > 4. Can you explain more for the first patch? Why VTd PEI module cannot > > be changed to support 0xFFFF? >=20 > According to PCDs: > gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSize|0x00400000|UIN > T32|0x00000003 > gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3|0x00200000|UI > NT32|0x00000004 >=20 > The default buffer size for IOMMU mapping allocated is 2M bytes for S3 an= d > 4M bytes for non-S3 cases. >=20 > 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 clos= e > 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? =20 >=20 > 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 > 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 ; Ni, Ray ; > > > Dong, Eric ; Wang, Jian J > > > > > > 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 > > > Cc: Eric Dong > > > Cc: Jian J Wang > > > > > > Hao Wu (2): > > > MdeModulePkg/AhciPei: Limit max transfer blocknum for 48-bit addres= s > > > 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