From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 95C3721A16E47 for ; Mon, 15 May 2017 16:50:11 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 May 2017 16:50:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,346,1491289200"; d="scan'208";a="968753906" Received: from casander-mobl.amr.corp.intel.com (HELO localhost) ([10.254.34.31]) by orsmga003.jf.intel.com with ESMTP; 15 May 2017 16:50:10 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <149489220988.32379.16373155581502002671@jljusten-skl> From: Jordan Justen In-Reply-To: <20170506193023.4767-3-lersek@redhat.com> References: <20170506193023.4767-1-lersek@redhat.com> <20170506193023.4767-3-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Mon, 15 May 2017 16:50:10 -0700 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: Mon, 15 May 2017 23:50:11 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-05-06 12:30:20, 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=3D527 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > 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/EmuVariable= FvbRuntimeDxe/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/EmuVariable= FvbRuntimeDxe/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 =3D { > } > }, > 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 >=3D EMU_FVB_NUM_TOTAL_BLOCKS) { > return EFI_INVALID_PARAMETER; > } > = > FvbDevice =3D FVB_DEVICE_FROM_THIS (This); > = > *BlockSize =3D FvbDevice->BlockSize; > - *NumberOfBlocks =3D (UINTN) (2 - (UINTN) Lba); > + *NumberOfBlocks =3D (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 =3D FVB_DEVICE_FROM_THIS (This); > - Erase =3D 0; > - > - VA_START (args, This); > = > + // > + // Check input parameters > + // > + VA_START (Args, This); > do { > - StartingLba =3D VA_ARG (args, EFI_LBA); > + StartingLba =3D VA_ARG (Args, EFI_LBA); > if (StartingLba =3D=3D EFI_LBA_LIST_TERMINATOR) { > break; > } > + NumOfLba =3D VA_ARG (Args, UINTN); > = > - NumOfLba =3D VA_ARG (args, UINT32); This bug seems mildly concerning. I guess real users are not going to specify an lba count > 4G. Still I think you should break this fix out, and perhaps notify other package owners: $ git grep -e NumOfLba --and -e VA_ARG --and -e UINT32 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c: NumOfLba =3D VA_ARG= (Args, UINT32); ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c: NumOfLba =3D VA_ARG= (Args, UINT32); DuetPkg/FvbRuntimeService/FWBlockService.c: NumOfLba =3D VA_ARG (args, U= INT32); DuetPkg/FvbRuntimeService/FWBlockService.c: NumOfLba =3D VA_ARG (args, U= INT32); EmulatorPkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba =3D VA_ARG = (args, UINT32); EmulatorPkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba =3D VA_ARG = (args, UINT32); Nt32Pkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba =3D VA_ARG (arg= s, UINT32); Nt32Pkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba =3D VA_ARG (arg= s, UINT32); OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c: NumOfLba =3D VA_ARG (args, UINT3= 2); OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c: NumOfLba =3D VA= _ARG (args, UINT32); OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c: NumOfLba =3D VA= _ARG (args, UINT32); QuarkPlatformPkg/Platform/SpiFvbServices/FwBlockService.c: NumOfLba =3D = VA_ARG (args, UINT32); QuarkPlatformPkg/Platform/SpiFvbServices/FwBlockService.c: NumOfLba =3D = VA_ARG (args, UINT32); Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c: NumOfLba =3D VA_ARG (args,= UINT32); Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c: NumOfLba =3D VA_ARG (args,= UINT32); Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.c: NumOfLba =3D VA_ARG (Marker= , UINT32); Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.c: NumOfLba =3D VA_ARG (Marker= , UINT32); -Jordan > - > - // > - // Check input parameters > - // > - if ((NumOfLba =3D=3D 0) || (StartingLba > 1) || ((StartingLba + NumO= fLba) > 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 =3D=3D 0) { > - Erase =3D (UINT8) (Erase | BIT0); > - } > - if ((StartingLba + NumOfLba) =3D=3D 2) { > - Erase =3D (UINT8) (Erase | BIT1); > - } > - > } while (1); > + VA_END (Args); > = > - VA_END (args); > - > - ErasePtr =3D (UINT8*) FvbDevice->BufferPtr; > - EraseSize =3D 0; > + // > + // Erase blocks > + // > + VA_START (Args, This); > + do { > + StartingLba =3D VA_ARG (Args, EFI_LBA); > + if (StartingLba =3D=3D EFI_LBA_LIST_TERMINATOR) { > + break; > + } > + NumOfLba =3D VA_ARG (Args, UINTN); > = > - if ((Erase & BIT0) !=3D 0) { > - EraseSize =3D EraseSize + FvbDevice->BlockSize; > - } else { > - ErasePtr =3D (VOID*) ((UINT8*)ErasePtr + FvbDevice->BlockSize); > - } > + ErasePtr =3D FvbDevice->BufferPtr; > + ErasePtr +=3D (UINTN)StartingLba * FvbDevice->BlockSize; > + EraseSize =3D NumOfLba * FvbDevice->BlockSize; > = > - if ((Erase & BIT1) !=3D 0) { > - EraseSize =3D EraseSize + FvbDevice->BlockSize; > - } > + SetMem (ErasePtr, EraseSize, ERASED_UINT8); > + } while (1); > + VA_END (Args); > = > - if (EraseSize !=3D 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 =3D FVB_DEVICE_FROM_THIS (This); > = > - if ((Lba > 1) || (Offset > FvbDevice->BlockSize)) { > + if (Lba >=3D EMU_FVB_NUM_TOTAL_BLOCKS || > + Offset > FvbDevice->BlockSize) { > return EFI_INVALID_PARAMETER; > } > = > - if ((Offset + *NumBytes) > FvbDevice->BlockSize) { > + Status =3D EFI_SUCCESS; > + if (*NumBytes > FvbDevice->BlockSize - Offset) { > *NumBytes =3D FvbDevice->BlockSize - Offset; > + Status =3D EFI_BAD_BUFFER_SIZE; > } > = > - FvbDataPtr =3D > - (UINT8*) FvbDevice->BufferPtr + > - MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) + > - Offset; > + FvbDataPtr =3D FvbDevice->BufferPtr; > + FvbDataPtr +=3D (UINTN)Lba * FvbDevice->BlockSize; > + FvbDataPtr +=3D 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 =3D FVB_DEVICE_FROM_THIS (This); > = > - if ((Lba > 1) || (Offset > FvbDevice->BlockSize)) { > + if (Lba >=3D EMU_FVB_NUM_TOTAL_BLOCKS || > + Offset > FvbDevice->BlockSize) { > return EFI_INVALID_PARAMETER; > } > = > - if ((Offset + *NumBytes) > FvbDevice->BlockSize) { > + Status =3D EFI_SUCCESS; > + if (*NumBytes > FvbDevice->BlockSize - Offset) { > *NumBytes =3D FvbDevice->BlockSize - Offset; > + Status =3D EFI_BAD_BUFFER_SIZE; > } > = > - FvbDataPtr =3D > - (UINT8*) FvbDevice->BufferPtr + > - MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) + > - Offset; > + FvbDataPtr =3D FvbDevice->BufferPtr; > + FvbDataPtr +=3D (UINTN)Lba * FvbDevice->BlockSize; > + FvbDataPtr +=3D 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 =3D FALSE; > } > } else { > - Ptr =3D AllocateAlignedRuntimePages ( > - EFI_SIZE_TO_PAGES (EMU_FVB_SIZE), > - SIZE_64KB > - ); > + Ptr =3D AllocateRuntimePages (EFI_SIZE_TO_PAGES (EMU_FVB_SIZE)); > } > = > mEmuVarsFvb.BufferPtr =3D Ptr; > @@ -808,7 +794,8 @@ FvbInitialize ( > // > // Initialize the Fault Tolerant Write spare block > // > - SubPtr =3D (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE); > + SubPtr =3D (VOID*) ((UINT8*) Ptr + > + EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE); > PcdStatus =3D PcdSet32S (PcdFlashNvStorageFtwSpareBase, > (UINT32)(UINTN) SubPtr); > ASSERT_RETURN_ERROR (PcdStatus); > -- = > 2.9.3 > = >=20