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.120, mailfrom: ray.ni@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Mon, 22 Apr 2019 13:20:26 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Apr 2019 13:20:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,383,1549958400"; d="scan'208";a="144923990" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga007.fm.intel.com with ESMTP; 22 Apr 2019 13:20:24 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 13:20:18 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 13:20:17 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.93]) with mapi id 14.03.0415.000; Tue, 23 Apr 2019 04:20:16 +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/ispxh59or6ZIbLrg Date: Mon, 22 Apr 2019 20:20:15 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C0FC8E8@SHSMSX104.ccr.corp.intel.com> References: <20190410011313.4268-1-hao.a.wu@intel.com> In-Reply-To: <20190410011313.4268-1-hao.a.wu@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjRjNjczZmEtNGViNy00YjgwLWJhNzItM2ZkMjI4ODZjZjBjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRW56ZDhubDJkV3A0WHVWWlpSVVwvSlF0SFwvbUpwMkx6dkRDYmpmQkF4RVI0V2lPNTBXVTN0aExcLzIyazk2MDNCXC8ifQ== 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 Comments: 1. Can we add EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST to mAhciAtaPassThruPpi= ListTemplate/mAhciBlkIoPpiListTemplate as well? I understand that requires = calling PeiServicesInstallPpi() 3 times instead of once. But I see the bene= fit of change is the code is more understandable and potential bug-free. An= d you could also create a global mAhciBlkIoPpi/mAhciBlkIo2Ppi variables and= let the mAhciBlkIoPpiListTemplate/mAhciBlkIo2PpiListTemplate point to them= respectively. The reason we cannot have a global AtaPassThruPpi is there i= s a Mode field in the PPI that cannot be read-only. 2. Below code snip is used by AhciBlockIoReadBlocks(),AhciBlockIoGetMediaIn= fo(), AhciBlockIoGetMediaInfo2() in AhciPeiBlockIo.c, maybe you can re-writ= e SearchDeviceByIndex() to include the below code. =09 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; } 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; } 4. Can you explain more for the first patch? Why VTd PEI module cannot be c= hanged to support 0xFFFF? // // 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 >=20 > The series is also available at: > https://github.com/hwu25/edk2/tree/ahci_pei_blockio_v2 >=20 >=20 > V2 changges: >=20 > Due to the file license change, rebase the whole series onto the tip of t= he > master branch. The 'Contributed-under' tag is removed from the log > messages as well. >=20 >=20 > V1 history: >=20 > The series will add the PEI BlockIO support for ATA AHCI mode devices. >=20 > Cc: Ray Ni > Cc: Eric Dong > Cc: Jian J Wang >=20 > Hao Wu (2): > MdeModulePkg/AhciPei: Limit max transfer blocknum for 48-bit address > MdeModulePkg/AhciPei: Add PEI BlockIO support >=20 > 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 >=20 > -- > 2.12.0.windows.1