From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.nue.novell.com (smtp.nue.novell.com [195.135.221.5]) (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 5941521A16E5F for ; Fri, 12 May 2017 02:08:30 -0700 (PDT) Received: from emea4-mta.ukb.novell.com ([10.120.13.87]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Fri, 12 May 2017 11:08:28 +0200 Received: from GaryWorkstation (nwb-a10-snat.microfocus.com [10.120.13.201]) by emea4-mta.ukb.novell.com with ESMTP (TLS encrypted); Fri, 12 May 2017 10:08:03 +0100 Date: Fri, 12 May 2017 17:07:58 +0800 From: Gary Lin To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Message-ID: <20170512090758.nijvo42z4vb7t5ei@GaryWorkstation> References: <20170506193023.4767-1-lersek@redhat.com> <20170506193023.4767-3-lersek@redhat.com> MIME-Version: 1.0 In-Reply-To: <20170506193023.4767-3-lersek@redhat.com> User-Agent: Mutt/1.6.2 (2016-07-01) Subject: Re: [PATCH 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB 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, 12 May 2017 09:08:31 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, May 06, 2017 at 09:30:20PM +0200, Laszlo Ersek wrote: > EmuVariableFvbRuntimeDxe currently produces a Firmware Volume Block > protocol that is based on a block map of two blocks, each block having > PcdFlashNvStorageFtwSpareSize for size. > > (The total size is 2 * PcdFlashNvStorageFtwSpareSize.) > > FaultTolerantWriteDxe in turn expects the block size to be a power of two. > > In the 4MB build of OVMF, PcdFlashNvStorageFtwSpareSize is 264KB, which is > not a power of two. In order to equip EmuVariableFvbRuntimeDxe for this > build, shrink the block size to 4KB (EFI_PAGE_SIZE), and grow the block > count from 2 to EFI_SIZE_TO_PAGES(2 * PcdFlashNvStorageFtwSpareSize). The > total size remains > > 2 * PcdFlashNvStorageFtwSpareSize > --------------------------------- * EFI_PAGE_SIZE > EFI_PAGE_SIZE > > Right now EmuVariableFvbRuntimeDxe open-codes the block count of 2 in > various limit checks, so introduce a few new macros: > - EMU_FVB_NUM_TOTAL_BLOCKS, for the LHS of the above product, > - EMU_FVB_NUM_SPARE_BLOCKS for the half of that. > > Also rework the FVB protocol members to support an arbitrary count of > blocks. > > Keep the invariant intact that the first half of the firmware volume hosts > the variable store and the FTW working block, and that the second half > maps the FTW spare area. > > Cc: Jordan Justen > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=527 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek Regression-tested-by: Gary Lin > --- > OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h | 10 +- > OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 147 +++++++++----------- > 2 files changed, 75 insertions(+), 82 deletions(-) > > diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h > index 4247d21d72f8..beb11e3f9a90 100644 > --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h > +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h > @@ -58,8 +58,14 @@ typedef struct { > // > // Constants > // > -#define EMU_FVB_BLOCK_SIZE (FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize)) > -#define EMU_FVB_SIZE (2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize)) > +#define EMU_FVB_BLOCK_SIZE \ > + EFI_PAGE_SIZE > +#define EMU_FVB_NUM_SPARE_BLOCKS \ > + EFI_SIZE_TO_PAGES ((UINTN)FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize)) > +#define EMU_FVB_NUM_TOTAL_BLOCKS \ > + (2 * EMU_FVB_NUM_SPARE_BLOCKS) > +#define EMU_FVB_SIZE \ > + (EMU_FVB_NUM_TOTAL_BLOCKS * EMU_FVB_BLOCK_SIZE) > #define FTW_WRITE_QUEUE_SIZE \ > (FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) - \ > sizeof (EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER)) > diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c > index 2f89632e5d75..11c8b1b75cb8 100644 > --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c > +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c > @@ -74,8 +74,8 @@ EFI_FW_VOL_BLOCK_DEVICE mEmuVarsFvb = { > } > }, > NULL, // BufferPtr > - FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize), // BlockSize > - 2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize), // Size > + EMU_FVB_BLOCK_SIZE, // BlockSize > + EMU_FVB_SIZE, // Size > { // FwVolBlockInstance > FvbProtocolGetAttributes, > FvbProtocolSetAttributes, > @@ -185,14 +185,14 @@ FvbProtocolGetBlockSize ( > { > EFI_FW_VOL_BLOCK_DEVICE *FvbDevice; > > - if (Lba > 1) { > + if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS) { > return EFI_INVALID_PARAMETER; > } > > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > *BlockSize = FvbDevice->BlockSize; > - *NumberOfBlocks = (UINTN) (2 - (UINTN) Lba); > + *NumberOfBlocks = (UINTN)(EMU_FVB_NUM_TOTAL_BLOCKS - Lba); > > return EFI_SUCCESS; > } > @@ -322,68 +322,58 @@ FvbProtocolEraseBlocks ( > ) > { > EFI_FW_VOL_BLOCK_DEVICE *FvbDevice; > - VA_LIST args; > + VA_LIST Args; > EFI_LBA StartingLba; > UINTN NumOfLba; > - UINT8 Erase; > - VOID *ErasePtr; > + UINT8 *ErasePtr; > UINTN EraseSize; > > FvbDevice = FVB_DEVICE_FROM_THIS (This); > - Erase = 0; > - > - VA_START (args, This); > > + // > + // Check input parameters > + // > + VA_START (Args, This); > do { > - StartingLba = VA_ARG (args, EFI_LBA); > + StartingLba = VA_ARG (Args, EFI_LBA); > if (StartingLba == EFI_LBA_LIST_TERMINATOR) { > break; > } > + NumOfLba = VA_ARG (Args, UINTN); > > - NumOfLba = VA_ARG (args, UINT32); > - > - // > - // Check input parameters > - // > - if ((NumOfLba == 0) || (StartingLba > 1) || ((StartingLba + NumOfLba) > 2)) { > - VA_END (args); > + if (StartingLba > EMU_FVB_NUM_TOTAL_BLOCKS || > + NumOfLba > EMU_FVB_NUM_TOTAL_BLOCKS - StartingLba) { > + VA_END (Args); > return EFI_INVALID_PARAMETER; > } > - > - if (StartingLba == 0) { > - Erase = (UINT8) (Erase | BIT0); > - } > - if ((StartingLba + NumOfLba) == 2) { > - Erase = (UINT8) (Erase | BIT1); > - } > - > } while (1); > + VA_END (Args); > > - VA_END (args); > - > - ErasePtr = (UINT8*) FvbDevice->BufferPtr; > - EraseSize = 0; > + // > + // Erase blocks > + // > + VA_START (Args, This); > + do { > + StartingLba = VA_ARG (Args, EFI_LBA); > + if (StartingLba == EFI_LBA_LIST_TERMINATOR) { > + break; > + } > + NumOfLba = VA_ARG (Args, UINTN); > > - if ((Erase & BIT0) != 0) { > - EraseSize = EraseSize + FvbDevice->BlockSize; > - } else { > - ErasePtr = (VOID*) ((UINT8*)ErasePtr + FvbDevice->BlockSize); > - } > + ErasePtr = FvbDevice->BufferPtr; > + ErasePtr += (UINTN)StartingLba * FvbDevice->BlockSize; > + EraseSize = NumOfLba * FvbDevice->BlockSize; > > - if ((Erase & BIT1) != 0) { > - EraseSize = EraseSize + FvbDevice->BlockSize; > - } > + SetMem (ErasePtr, EraseSize, ERASED_UINT8); > + } while (1); > + VA_END (Args); > > - if (EraseSize != 0) { > - SetMem ( > - (VOID*) ErasePtr, > - EraseSize, > - ERASED_UINT8 > - ); > - VA_START (args, This); > - PlatformFvbBlocksErased (This, args); > - VA_END (args); > - } > + // > + // Call platform hook > + // > + VA_START (Args, This); > + PlatformFvbBlocksErased (This, Args); > + VA_END (Args); > > return EFI_SUCCESS; > } > @@ -458,31 +448,30 @@ FvbProtocolWrite ( > IN UINT8 *Buffer > ) > { > - > EFI_FW_VOL_BLOCK_DEVICE *FvbDevice; > UINT8 *FvbDataPtr; > + EFI_STATUS Status; > > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > - if ((Lba > 1) || (Offset > FvbDevice->BlockSize)) { > + if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS || > + Offset > FvbDevice->BlockSize) { > return EFI_INVALID_PARAMETER; > } > > - if ((Offset + *NumBytes) > FvbDevice->BlockSize) { > + Status = EFI_SUCCESS; > + if (*NumBytes > FvbDevice->BlockSize - Offset) { > *NumBytes = FvbDevice->BlockSize - Offset; > + Status = EFI_BAD_BUFFER_SIZE; > } > > - FvbDataPtr = > - (UINT8*) FvbDevice->BufferPtr + > - MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) + > - Offset; > + FvbDataPtr = FvbDevice->BufferPtr; > + FvbDataPtr += (UINTN)Lba * FvbDevice->BlockSize; > + FvbDataPtr += Offset; > > - if (*NumBytes > 0) { > - CopyMem (FvbDataPtr, Buffer, *NumBytes); > - PlatformFvbDataWritten (This, Lba, Offset, *NumBytes, Buffer); > - } > - > - return EFI_SUCCESS; > + CopyMem (FvbDataPtr, Buffer, *NumBytes); > + PlatformFvbDataWritten (This, Lba, Offset, *NumBytes, Buffer); > + return Status; > } > > > @@ -545,28 +534,28 @@ FvbProtocolRead ( > { > EFI_FW_VOL_BLOCK_DEVICE *FvbDevice; > UINT8 *FvbDataPtr; > + EFI_STATUS Status; > > FvbDevice = FVB_DEVICE_FROM_THIS (This); > > - if ((Lba > 1) || (Offset > FvbDevice->BlockSize)) { > + if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS || > + Offset > FvbDevice->BlockSize) { > return EFI_INVALID_PARAMETER; > } > > - if ((Offset + *NumBytes) > FvbDevice->BlockSize) { > + Status = EFI_SUCCESS; > + if (*NumBytes > FvbDevice->BlockSize - Offset) { > *NumBytes = FvbDevice->BlockSize - Offset; > + Status = EFI_BAD_BUFFER_SIZE; > } > > - FvbDataPtr = > - (UINT8*) FvbDevice->BufferPtr + > - MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) + > - Offset; > + FvbDataPtr = FvbDevice->BufferPtr; > + FvbDataPtr += (UINTN)Lba * FvbDevice->BlockSize; > + FvbDataPtr += Offset; > > - if (*NumBytes > 0) { > - CopyMem (Buffer, FvbDataPtr, *NumBytes); > - PlatformFvbDataRead (This, Lba, Offset, *NumBytes, Buffer); > - } > - > - return EFI_SUCCESS; > + CopyMem (Buffer, FvbDataPtr, *NumBytes); > + PlatformFvbDataRead (This, Lba, Offset, *NumBytes, Buffer); > + return Status; > } > > > @@ -663,7 +652,7 @@ InitializeFvAndVariableStoreHeaders ( > // EFI_FV_BLOCK_MAP_ENTRY BlockMap[1]; > { > { > - 2, // UINT32 NumBlocks; > + EMU_FVB_NUM_TOTAL_BLOCKS, // UINT32 NumBlocks; > EMU_FVB_BLOCK_SIZE // UINT32 Length; > } > } > @@ -745,7 +734,7 @@ FvbInitialize ( > (PcdGet32 (PcdVariableStoreSize) + > PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > ) > > - EMU_FVB_BLOCK_SIZE > + EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE > ) { > DEBUG ((EFI_D_ERROR, "EMU Variable invalid PCD sizes\n")); > return EFI_INVALID_PARAMETER; > @@ -779,10 +768,7 @@ FvbInitialize ( > Initialize = FALSE; > } > } else { > - Ptr = AllocateAlignedRuntimePages ( > - EFI_SIZE_TO_PAGES (EMU_FVB_SIZE), > - SIZE_64KB > - ); > + Ptr = AllocateRuntimePages (EFI_SIZE_TO_PAGES (EMU_FVB_SIZE)); > } > > mEmuVarsFvb.BufferPtr = Ptr; > @@ -808,7 +794,8 @@ FvbInitialize ( > // > // Initialize the Fault Tolerant Write spare block > // > - SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE); > + SubPtr = (VOID*) ((UINT8*) Ptr + > + EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE); > PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase, > (UINT32)(UINTN) SubPtr); > ASSERT_RETURN_ERROR (PcdStatus); > -- > 2.9.3 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >