From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5F60F21D490EB for ; Thu, 10 Aug 2017 21:18:37 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Aug 2017 21:20:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,356,1498546800"; d="scan'208";a="138355497" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga005.fm.intel.com with ESMTP; 10 Aug 2017 21:20:56 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 10 Aug 2017 21:20:56 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.114]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.236]) with mapi id 14.03.0319.002; Fri, 11 Aug 2017 12:20:54 +0800 From: "Wu, Hao A" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" Thread-Topic: [edk2] [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra data is erased by EraseBlocks Thread-Index: AQHTEXcVZ3+vhBMfH0qFPpOFycqtwKJ97FMAgACjBcA= Date: Fri, 11 Aug 2017 04:20:54 +0000 Message-ID: References: <20170810012125.18044-1-hao.a.wu@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B918177@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B918177@shsmsx102.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 Subject: Re: [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra data is erased by EraseBlocks X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Aug 2017 04:18:37 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Zeng, Star > Sent: Friday, August 11, 2017 10:37 AM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Wu, Hao A; Ni, Ruiyu; Zeng, Star > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra > data is erased by EraseBlocks >=20 > Two minor comments, others are good to me. >=20 > 1. Could the code use (LastLba + 1 - EndGroupLba) instead of (LastLba - > EndGroupLba + 1)? >=20 > 2. Could the code have the debug message "DEBUG ((EFI_D_ERROR, > "EmmcEraseBlocks(): Lba 0x%x BlkNo 0x%x Event %p with %r\n", Lba, > BlockNum, Token->Event, Status))" for the new return paths the patch adde= d? > And the debug level should be DEBUG_INFO instead of EFI_D_ERROR? >=20 Thanks for the feedbacks. I will submit a new version of the patch. Best Regards, Hao Wu >=20 > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ha= o > Wu > Sent: Thursday, August 10, 2017 9:21 AM > To: edk2-devel@lists.01.org > Cc: Wu, Hao A ; Ni, Ruiyu ; Zeng, > Star > Subject: [edk2] [PATCH v2] MdeModulePkg/EmmcDxe: Make sure no extra > data is erased by EraseBlocks >=20 > V2 changes: >=20 > The Trim command is not supported on all eMMC devices. For those devices > that do not support such command, add codes to handle the scenario. >=20 > Commit message: >=20 > The current implementation of the Erase Block Protocol service > EraseBlocks() uses the erase command. According to spec eMMC Electrical > Standard 5.1, Section 6.6.9: >=20 > The erasable unit of the eMMC is the "Erase Group"; Erase group is measur= ed > in write blocks that are the basic writable units of the Device. > ... > When the Erase is executed it will apply to all write blocks within an er= ase > group. >=20 > However, code logic in function EmmcEraseBlocks() does not check whether > the blocks to be erased form complete erase groups. Missing such checks w= ill > lead to erasing extra data on the device. >=20 > This commit will: > a. If the device support the Trim command, use the Trim command to perfor= m > the erase operations for eMMC devices. >=20 > According to the spec: > Unlike the Erase command, the Trim function applies the erase operation t= o > write blocks instead of erase groups. >=20 > b. If the device does not support the Trim command, use the Erase command= to > erase the data in the erase groups. And write zeros to those blocks that = cannot > form a complete erase group. >=20 > Cc: Star Zeng > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Hao Wu > --- > MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c | 127 > +++++++++++++++++++++++++++++- > 1 file changed, 125 insertions(+), 2 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c > b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c > index c432d26801..916f15d429 100644 > --- a/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c > +++ b/MdeModulePkg/Bus/Sd/EmmcDxe/EmmcBlockIo.c > @@ -1851,6 +1851,14 @@ EmmcEraseBlock ( > EraseBlock->SdMmcCmdBlk.CommandIndex =3D EMMC_ERASE; > EraseBlock->SdMmcCmdBlk.CommandType =3D SdMmcCommandTypeAc; > EraseBlock->SdMmcCmdBlk.ResponseType =3D SdMmcResponseTypeR1b; > + if ((Device->ExtCsd.SecFeatureSupport & BIT4) !=3D 0) { > + // > + // Perform a Trim operation which applies the erase operation to wri= te > blocks > + // instead of erase groups. (Spec JESD84-B51, eMMC Electrical Standa= rd 5.1, > + // Section 6.6.10 and 6.10.4) > + // > + EraseBlock->SdMmcCmdBlk.CommandArgument =3D 1; } >=20 > EraseBlock->IsEnd =3D IsEnd; > EraseBlock->Token =3D Token; > @@ -1903,6 +1911,43 @@ Error: > } >=20 > /** > + Write zeros to specified blocks. > + > + @param[in] Partition A pointer to the EMMC_PARTITION instance= . > + @param[in] StartLba The starting logical block address to wr= ite zeros. > + @param[in] Size The size in bytes to fill with zeros. Th= is must be a > multiple of > + the physical block size of the device. > + > + @retval EFI_SUCCESS The request is executed successfully. > + @retval EFI_OUT_OF_RESOURCES The request could not be executed due to > a lack of resources. > + @retval Others The request could not be executed succes= sfully. > + > +**/ > +EFI_STATUS > +EmmcWriteZeros ( > + IN EMMC_PARTITION *Partition, > + IN EFI_LBA StartLba, > + IN UINTN Size > + ) > +{ > + EFI_STATUS Status; > + UINT8 *Buffer; > + UINT32 MediaId; > + > + Buffer =3D AllocateZeroPool (Size); > + if (Buffer =3D=3D NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + MediaId =3D Partition->BlockMedia.MediaId; > + > + Status =3D EmmcReadWrite (Partition, MediaId, StartLba, Buffer, Size, > + FALSE, NULL); FreePool (Buffer); > + > + return Status; > +} > + > +/** > Erase a specified number of device blocks. >=20 > @param[in] This Indicates a pointer to the calling con= text. > @@ -1943,7 +1988,13 @@ EmmcEraseBlocks ( > EFI_BLOCK_IO_MEDIA *Media; > UINTN BlockSize; > UINTN BlockNum; > + EFI_LBA FirstLba; > EFI_LBA LastLba; > + EFI_LBA StartGroupLba; > + EFI_LBA EndGroupLba; > + UINT32 EraseGroupSize; > + UINT32 Remainder; > + UINTN WriteZeroSize; > UINT8 PartitionConfig; > EMMC_PARTITION *Partition; > EMMC_DEVICE *Device; > @@ -1978,7 +2029,8 @@ EmmcEraseBlocks ( > Token->TransactionStatus =3D EFI_SUCCESS; > } >=20 > - LastLba =3D Lba + BlockNum - 1; > + FirstLba =3D Lba; > + LastLba =3D Lba + BlockNum - 1; >=20 > // > // Check if needs to switch partition access. > @@ -1994,7 +2046,78 @@ EmmcEraseBlocks ( > Device->ExtCsd.PartitionConfig =3D PartitionConfig; > } >=20 > - Status =3D EmmcEraseBlockStart (Partition, Lba, > (EFI_BLOCK_IO2_TOKEN*)Token, FALSE); > + if ((Device->ExtCsd.SecFeatureSupport & BIT4) =3D=3D 0) { > + // > + // If the Trim operation is not supported by the device, handle the = erase > + // of blocks that do not form a complete erase group separately. > + // > + EraseGroupSize =3D This->EraseLengthGranularity; > + > + DivU64x32Remainder (FirstLba, EraseGroupSize, &Remainder); > + StartGroupLba =3D (Remainder =3D=3D 0) ? FirstLba : (FirstLba + > + EraseGroupSize - Remainder); > + > + DivU64x32Remainder (LastLba + 1, EraseGroupSize, &Remainder); > + EndGroupLba =3D LastLba + 1 - Remainder; > + > + // > + // If the size to erase is smaller than the erase group size, the wh= ole > + // erase operation is performed by writting zeros. > + // > + if (BlockNum < EraseGroupSize) { > + Status =3D EmmcWriteZeros (Partition, FirstLba, Size); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if ((Token !=3D NULL) && (Token->Event !=3D NULL)) { > + Token->TransactionStatus =3D EFI_SUCCESS; > + gBS->SignalEvent (Token->Event); > + } > + return EFI_SUCCESS; > + } > + > + // > + // If the starting LBA to erase is not aligned with the start of an = erase > + // group, write zeros to erase the data from starting LBA to the end= of the > + // current erase group. > + // > + if (StartGroupLba > FirstLba) { > + WriteZeroSize =3D (UINTN)(StartGroupLba - FirstLba) * BlockSize; > + Status =3D EmmcWriteZeros (Partition, FirstLba, WriteZeroSize); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + } > + > + // > + // If the ending LBA to erase is not aligned with the end of an eras= e > + // group, write zeros to erase the data from the start of the erase = group > + // to the ending LBA. > + // > + if (EndGroupLba <=3D LastLba) { > + WriteZeroSize =3D (UINTN)(LastLba - EndGroupLba + 1) * BlockSize; > + Status =3D EmmcWriteZeros (Partition, EndGroupLba, WriteZeroSize); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + } > + > + // > + // Check whether there is erase group to erase. > + // > + if (EndGroupLba <=3D StartGroupLba) { > + if ((Token !=3D NULL) && (Token->Event !=3D NULL)) { > + Token->TransactionStatus =3D EFI_SUCCESS; > + gBS->SignalEvent (Token->Event); > + } > + return EFI_SUCCESS; > + } > + > + FirstLba =3D StartGroupLba; > + LastLba =3D EndGroupLba - 1; > + } > + > + Status =3D EmmcEraseBlockStart (Partition, FirstLba, > + (EFI_BLOCK_IO2_TOKEN*)Token, FALSE); > if (EFI_ERROR (Status)) { > return Status; > } > -- > 2.12.0.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel