From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 60B8621A16EFC for ; Thu, 18 May 2017 13:56:26 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP; 18 May 2017 13:56:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,360,1491289200"; d="scan'208";a="88956607" Received: from jljusten-skl.jf.intel.com (HELO localhost) ([10.54.75.25]) by orsmga002.jf.intel.com with ESMTP; 18 May 2017 13:56:26 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <149514098528.16116.10665105642348221621@jljusten-skl.jf.intel.com> From: Jordan Justen In-Reply-To: <8b42c90a-31fb-d1c5-307c-5bc78760660a@redhat.com> References: <20170518151436.16566-1-lersek@redhat.com> <20170518151436.16566-3-lersek@redhat.com> <149513337025.15724.8536347902397365487@jljusten-skl.jf.intel.com> <8b42c90a-31fb-d1c5-307c-5bc78760660a@redhat.com> User-Agent: alot/0.5.1 Date: Thu, 18 May 2017 13:56:25 -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 20:56:26 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-05-18 12:40:30, Laszlo Ersek wrote: > On 05/18/17 20:49, Jordan Justen wrote: > > On 2017-05-18 08:14:33, Laszlo Ersek wrote: > >> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h b/OvmfPkg/EmuVaria= bleFvbRuntimeDxe/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 (PcdFlashNvStorageFtwSpareS= ize)) > >> -#define EMU_FVB_SIZE (2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSiz= e)) > >> +#define EMU_FVB_BLOCK_SIZE \ > >> + EFI_PAGE_SIZE > >> +#define EMU_FVB_NUM_SPARE_BLOCKS \ > >> + EFI_SIZE_TO_PAGES ((UINTN)FixedPcdGet32 (PcdFlashNvStorageFtwSpareS= ize)) > >> +#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. > = > My first preference would have been > = > #define SHORT_MACRO_NAME replacement text 1 > #define ANNOYINGLY_LONG_MACRO_NAME replacement text 2 > = > That is, to keep both the macro names and the replacement texts aligned. > However, that way I wouldn't fit into 80 chars on some lines, and then > breaking only *some* macro definitions to multiple lines looked > horrible. Which is why I opted for the current layout: it is uniform, > and it does preserve the alignment for both macro names and replacement > texts separately. I don't think you would make a block of function calls all multiline if one call required it. I see your point and I agree that aligning things can be nice if it works out. It seems like it doesn't in this case. Could FTW_SPARE_SIZE and FTW_WORKING_SIZE macros help? If you feel strongly about this current format, then keep it, as I don't feel too strongly about it. > > = > > Could you add to the entry-point an assert: > > = > > ASSERT(FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) % > > EMU_FVB_BLOCK_SIZE =3D=3D 0); > = > Should I squash that into this patch? Yeah. No need for resend. -Jordan