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 BB2B42195FD51 for ; Thu, 18 May 2017 11:49:35 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 May 2017 11:49:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,359,1491289200"; d="scan'208";a="1149933418" Received: from jljusten-skl.jf.intel.com (HELO localhost) ([10.54.75.25]) by fmsmga001.fm.intel.com with ESMTP; 18 May 2017 11:49:31 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <149513337025.15724.8536347902397365487@jljusten-skl.jf.intel.com> From: Jordan Justen In-Reply-To: <20170518151436.16566-3-lersek@redhat.com> References: <20170518151436.16566-1-lersek@redhat.com> <20170518151436.16566-3-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Thu, 18 May 2017 11:49:30 -0700 Subject: Re: [PATCH v2 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: Thu, 18 May 2017 18:49:35 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-05-18 08:14:33, Laszlo Ersek wrote: > 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)) In the cases where we don't exceed 80 columns, I don't see the excess newlines as helping here, style-wise. Could you add to the entry-point an assert: ASSERT(FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) % EMU_FVB_BLOCK_SIZE =3D=3D 0); We should tweak VERIFY_SIZE_OF to make a STATIC_ASSERT macro, because I guess this check should be possible at compile time. > @@ -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; Stealth bug fix? :) With the understanding that we're holding off on the final patch for now to coordinate with Xen: Series Reviewed-by: Jordan Justen