| Message ID | 1466505034-10267-5-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 06/21/16 12:30, Ard Biesheuvel wrote: > Similar to how OVMF implements this, add a FD definition for the varstore > firmware volume and the FTW areas. This can be used by host side tooling > to manipulate a pristine varstore before presenting it to the guest. Side question: what host side tooling do you have in mind? > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/ArmVirtQemu.fdf | 1 + > ArmVirtPkg/ArmVirtQemuKernel.fdf | 1 + > ArmVirtPkg/VarStore.fdf.inc | 79 ++++++++++++++++++++ > 3 files changed, 81 insertions(+) > > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf > index 422a2bf1f9a1..ac2f1f17a042 100644 > --- a/ArmVirtPkg/ArmVirtQemu.fdf > +++ b/ArmVirtPkg/ArmVirtQemu.fdf > @@ -69,6 +69,7 @@ [FD.QEMU_EFI] > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > FV = FVMAIN_COMPACT > > +!include VarStore.fdf.inc > > ################################################################################ > # > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf > index 6db0668a882d..f6dcbc1d5417 100644 > --- a/ArmVirtPkg/ArmVirtQemuKernel.fdf > +++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf > @@ -91,6 +91,7 @@ [FD.QEMU_EFI] > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > FV = FVMAIN_COMPACT > > +!include VarStore.fdf.inc > > ################################################################################ > # > diff --git a/ArmVirtPkg/VarStore.fdf.inc b/ArmVirtPkg/VarStore.fdf.inc > new file mode 100644 > index 000000000000..7d41b6be6adb > --- /dev/null > +++ b/ArmVirtPkg/VarStore.fdf.inc > @@ -0,0 +1,79 @@ > +## @file > +# FDF include file with Layout Regions that define an empty variable store. > +# > +# Copyright (c) 2016, Linaro, Ltd. All rights reserved.<BR> > +# Copyright (C) 2014, Red Hat, Inc.<BR> > +# Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> > +# > +# This program and the accompanying materials are licensed and made available > +# under the terms and conditions of the BSD License which accompanies this > +# distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > +# IMPLIED. > +# > +## > + > +[FD.QEMU_VARS] (1) Unlike under OvmfPkg, this include file doesn't just contain Layout Regions, it contains a full [FD] ("Flash Device Image") section. That's fine, but: - since you reference OVMF in the commit message, this difference should be mentioned there, - in the comment at the top of this new file, the "... with Layout Regions" is no longer precise. Please update it. > +BaseAddress = 0x04000000 (2) This has to be unified with the PcdFlashNvStorageVariableBase PCD setting that is currently seen in the QEMU DSC files, or we'll go mad down the road. In my opinion, the best way is to DEFINE a macro near the top of this file. Then reference that macro here, in the setting of the BaseAddress token, and also later, in the assignment to PcdFlashNvStorageVariableBase. For assigning PcdFlashNvStorageVariableBase, there are actually three ways. (They are all documented in the FDF spec.) * The simplest (but more limited) way is the following syntax: BaseAddress = $(VARS_BASE)|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase * The second, more complex (but more versatile) way is to employ a separate SET statement (also in this FDF include file): BaseAddress = $(VARS_BASE) [...] SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase = $(VARS_BASE) * The third way will actually be the winner, and I'll come to it a bit later. Finally, remove PcdFlashNvStorageVariableBase from the DSC files whose FDFs pull in this FDF include file. > +Size = 0x000c0000 > +ErasePolarity = 1 > + > +# This one is tricky, it must be: BlockSize * NumBlocks = Size > +BlockSize = 0x00040000 > +NumBlocks = 0x3 (3) Actually, now I suggest to DEFINE the following macros at the top of the file: DEFINE VARS_BASE = 0x04000000 DEFINE VARS_BLOCKS = 3 DEFINE BLOCK_SIZE = 0x00040000 DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) (It is possible that the multiplications above will not work in all of the contexts below -- in that case, just open-code the products please, and add some comments.) Then use these macros, for setting the above FD Tokens, like this: BaseAddress = $(VARS_BASE) Size = $(VARS_SIZE) ErasePolarity = 1 BlockSize = $(BLOCK_SIZE) NumBlocks = $(VARS_BLOCKS) Furthermore: > +0x00000000|0x00040000 (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. The most compact form for that is: 0x00000000|$(BLOCK_SIZE) gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > +#NV_VARIABLE_STORE > +DATA = { > + ## This is the EFI_FIRMWARE_VOLUME_HEADER > + # ZeroVector [] > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + # FileSystemGuid: gEfiSystemNvDataFvGuid = > + # { 0xFFF12B8D, 0x7696, 0x4C8B, > + # { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }} > + 0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C, > + 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50, > + # FvLength: 0xC0000 > + 0x00, 0x00, 0x0C, 0x00, 0x00, 0x00, 0x00, 0x00, > + # Signature "_FVH" # Attributes > + 0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00, > + # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision > + 0x48, 0x00, 0x28, 0x09, 0x00, 0x00, 0x00, 0x02, > + # Blockmap[0]: 0x3 Blocks * 0x40000 Bytes / Block > + 0x3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, > + # Blockmap[1]: End > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + ## This is the VARIABLE_STORE_HEADER > + # It is compatible with SECURE_BOOT_ENABLE == FALSE as well. > + # Signature: gEfiAuthenticatedVariableGuid = > + # { 0xaaf32c78, 0x947b, 0x439a, > + # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }} > + 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, > + 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, > + # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - > + # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8 > + # This can speed up the Variable Dispatch a bit. > + 0xB8, 0xFF, 0x03, 0x00, > + # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32 > + 0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 > +} > + > +0x00040000|0x00040000 (5) Please try the following: $(BLOCK_SIZE)|$(BLOCK_SIZE) gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > +#NV_FTW_WORKING > +DATA = { > + # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid = > + # { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }} > + 0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49, > + 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95, > + # Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved > + 0x5b, 0xe7, 0xc6, 0x86, 0xFE, 0xFF, 0xFF, 0xFF, > + # WriteQueueSize: UINT64 > + 0xE0, 0xFF, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00 > +} > + > +0x00080000|0x00040000 (6) Please try: $(SPARE_BASE)|$(BLOCK_SIZE) gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > +#NV_FTW_SPARE > (7) I notice these DATA regions differ from those in OvmfPkg, and that makes sense. However, it's pretty hard to compare them manually. - Can you please enumerate in the commit message what fields have been changed / recalculated? - Can you please confirm that the generated FD file will match the contents of an originally zero-filled flash device that has just been formatted by NorFlashDxe? .. I think you can go ahead and commit the first three patches separately (assuming Leif agrees with patch #1), and we can collaborate on this last patch separately, if you wish. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 21 June 2016 at 17:43, Laszlo Ersek <lersek@redhat.com> wrote: > On 06/21/16 12:30, Ard Biesheuvel wrote: >> Similar to how OVMF implements this, add a FD definition for the varstore >> firmware volume and the FTW areas. This can be used by host side tooling >> to manipulate a pristine varstore before presenting it to the guest. > > Side question: what host side tooling do you have in mind? > I am not sure, to be honest. Leif mentioned to me that Ubuntu cooked up some code to create an initial varstore image, and sets variables in it, but I don't know any of the details, In any case, it may not be relevent to mention this so I can remove it. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmVirtPkg/ArmVirtQemu.fdf | 1 + >> ArmVirtPkg/ArmVirtQemuKernel.fdf | 1 + >> ArmVirtPkg/VarStore.fdf.inc | 79 ++++++++++++++++++++ >> 3 files changed, 81 insertions(+) >> >> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf >> index 422a2bf1f9a1..ac2f1f17a042 100644 >> --- a/ArmVirtPkg/ArmVirtQemu.fdf >> +++ b/ArmVirtPkg/ArmVirtQemu.fdf >> @@ -69,6 +69,7 @@ [FD.QEMU_EFI] >> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize >> FV = FVMAIN_COMPACT >> >> +!include VarStore.fdf.inc >> >> ################################################################################ >> # >> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf >> index 6db0668a882d..f6dcbc1d5417 100644 >> --- a/ArmVirtPkg/ArmVirtQemuKernel.fdf >> +++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf >> @@ -91,6 +91,7 @@ [FD.QEMU_EFI] >> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize >> FV = FVMAIN_COMPACT >> >> +!include VarStore.fdf.inc >> >> ################################################################################ >> # >> diff --git a/ArmVirtPkg/VarStore.fdf.inc b/ArmVirtPkg/VarStore.fdf.inc >> new file mode 100644 >> index 000000000000..7d41b6be6adb >> --- /dev/null >> +++ b/ArmVirtPkg/VarStore.fdf.inc >> @@ -0,0 +1,79 @@ >> +## @file >> +# FDF include file with Layout Regions that define an empty variable store. >> +# >> +# Copyright (c) 2016, Linaro, Ltd. All rights reserved.<BR> >> +# Copyright (C) 2014, Red Hat, Inc.<BR> >> +# Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> >> +# >> +# This program and the accompanying materials are licensed and made available >> +# under the terms and conditions of the BSD License which accompanies this >> +# distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >> +# IMPLIED. >> +# >> +## >> + >> +[FD.QEMU_VARS] > > (1) Unlike under OvmfPkg, this include file doesn't just contain Layout Regions, it contains a full [FD] ("Flash Device Image") section. That's fine, but: > > - since you reference OVMF in the commit message, this difference should be mentioned there, > > - in the comment at the top of this new file, the "... with Layout Regions" is no longer precise. Please update it. > OK >> +BaseAddress = 0x04000000 > > (2) This has to be unified with the PcdFlashNvStorageVariableBase PCD setting that is currently seen in the QEMU DSC files, or we'll go mad down the road. > > In my opinion, the best way is to DEFINE a macro near the top of this file. Then reference that macro here, in the setting of the BaseAddress token, and also later, in the assignment to PcdFlashNvStorageVariableBase. > > For assigning PcdFlashNvStorageVariableBase, there are actually three ways. (They are all documented in the FDF spec.) > > * The simplest (but more limited) way is the following syntax: > > BaseAddress = $(VARS_BASE)|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase > > * The second, more complex (but more versatile) way is to employ a separate SET statement (also in this FDF include file): > > BaseAddress = $(VARS_BASE) > [...] > SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase = $(VARS_BASE) > > * The third way will actually be the winner, and I'll come to it a bit later. > Indeed. > Finally, remove PcdFlashNvStorageVariableBase from the DSC files whose FDFs pull in this FDF include file. > >> +Size = 0x000c0000 >> +ErasePolarity = 1 >> + >> +# This one is tricky, it must be: BlockSize * NumBlocks = Size >> +BlockSize = 0x00040000 >> +NumBlocks = 0x3 > > (3) Actually, now I suggest to DEFINE the following macros at the top of the file: > > DEFINE VARS_BASE = 0x04000000 > DEFINE VARS_BLOCKS = 3 > DEFINE BLOCK_SIZE = 0x00040000 > DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) > DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) > > (It is possible that the multiplications above will not work in all of the contexts below -- in that case, just open-code the products please, and add some comments.) > > Then use these macros, for setting the above FD Tokens, like this: > > BaseAddress = $(VARS_BASE) > Size = $(VARS_SIZE) > ErasePolarity = 1 > BlockSize = $(BLOCK_SIZE) > NumBlocks = $(VARS_BLOCKS) > > Furthermore: > >> +0x00000000|0x00040000 > > (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. > > The most compact form for that is: > > 0x00000000|$(BLOCK_SIZE) > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > >> +#NV_VARIABLE_STORE >> +DATA = { >> + ## This is the EFI_FIRMWARE_VOLUME_HEADER >> + # ZeroVector [] >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + # FileSystemGuid: gEfiSystemNvDataFvGuid = >> + # { 0xFFF12B8D, 0x7696, 0x4C8B, >> + # { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }} >> + 0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C, >> + 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50, >> + # FvLength: 0xC0000 >> + 0x00, 0x00, 0x0C, 0x00, 0x00, 0x00, 0x00, 0x00, >> + # Signature "_FVH" # Attributes >> + 0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00, >> + # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision >> + 0x48, 0x00, 0x28, 0x09, 0x00, 0x00, 0x00, 0x02, >> + # Blockmap[0]: 0x3 Blocks * 0x40000 Bytes / Block >> + 0x3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, >> + # Blockmap[1]: End >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + ## This is the VARIABLE_STORE_HEADER >> + # It is compatible with SECURE_BOOT_ENABLE == FALSE as well. >> + # Signature: gEfiAuthenticatedVariableGuid = >> + # { 0xaaf32c78, 0x947b, 0x439a, >> + # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }} >> + 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, >> + 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, >> + # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - >> + # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8 >> + # This can speed up the Variable Dispatch a bit. >> + 0xB8, 0xFF, 0x03, 0x00, >> + # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32 >> + 0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 >> +} >> + >> +0x00040000|0x00040000 > > (5) Please try the following: > > $(BLOCK_SIZE)|$(BLOCK_SIZE) > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > >> +#NV_FTW_WORKING >> +DATA = { >> + # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid = >> + # { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }} >> + 0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49, >> + 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95, >> + # Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved >> + 0x5b, 0xe7, 0xc6, 0x86, 0xFE, 0xFF, 0xFF, 0xFF, >> + # WriteQueueSize: UINT64 >> + 0xE0, 0xFF, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00 >> +} >> + >> +0x00080000|0x00040000 > > (6) Please try: > > $(SPARE_BASE)|$(BLOCK_SIZE) > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > >> +#NV_FTW_SPARE >> > > (7) I notice these DATA regions differ from those in OvmfPkg, and that makes sense. However, it's pretty hard to compare them manually. > > - Can you please enumerate in the commit message what fields have been changed / recalculated? > Will do. Only length, block size and checksum fields have been updated based on the new size and block size. > - Can you please confirm that the generated FD file will match the contents of an originally zero-filled flash device that has just been formatted by NorFlashDxe? > Yes, it will. > .. I think you can go ahead and commit the first three patches separately (assuming Leif agrees with patch #1), and we can collaborate on this last patch separately, if you wish. > I am not in a hurry, but if Leif is OK with them (#3 is against ArmPlatformPkg as well), then there is no point in sending them out again. Leif? -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 21 June 2016 at 17:43, Laszlo Ersek <lersek@redhat.com> wrote: > On 06/21/16 12:30, Ard Biesheuvel wrote: >> Similar to how OVMF implements this, add a FD definition for the varstore >> firmware volume and the FTW areas. This can be used by host side tooling >> to manipulate a pristine varstore before presenting it to the guest. > > Side question: what host side tooling do you have in mind? > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmVirtPkg/ArmVirtQemu.fdf | 1 + >> ArmVirtPkg/ArmVirtQemuKernel.fdf | 1 + >> ArmVirtPkg/VarStore.fdf.inc | 79 ++++++++++++++++++++ >> 3 files changed, 81 insertions(+) >> >> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf >> index 422a2bf1f9a1..ac2f1f17a042 100644 >> --- a/ArmVirtPkg/ArmVirtQemu.fdf >> +++ b/ArmVirtPkg/ArmVirtQemu.fdf >> @@ -69,6 +69,7 @@ [FD.QEMU_EFI] >> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize >> FV = FVMAIN_COMPACT >> >> +!include VarStore.fdf.inc >> >> ################################################################################ >> # >> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf >> index 6db0668a882d..f6dcbc1d5417 100644 >> --- a/ArmVirtPkg/ArmVirtQemuKernel.fdf >> +++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf >> @@ -91,6 +91,7 @@ [FD.QEMU_EFI] >> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize >> FV = FVMAIN_COMPACT >> >> +!include VarStore.fdf.inc >> >> ################################################################################ >> # >> diff --git a/ArmVirtPkg/VarStore.fdf.inc b/ArmVirtPkg/VarStore.fdf.inc >> new file mode 100644 >> index 000000000000..7d41b6be6adb >> --- /dev/null >> +++ b/ArmVirtPkg/VarStore.fdf.inc >> @@ -0,0 +1,79 @@ >> +## @file >> +# FDF include file with Layout Regions that define an empty variable store. >> +# >> +# Copyright (c) 2016, Linaro, Ltd. All rights reserved.<BR> >> +# Copyright (C) 2014, Red Hat, Inc.<BR> >> +# Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> >> +# >> +# This program and the accompanying materials are licensed and made available >> +# under the terms and conditions of the BSD License which accompanies this >> +# distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >> +# IMPLIED. >> +# >> +## >> + >> +[FD.QEMU_VARS] > > (1) Unlike under OvmfPkg, this include file doesn't just contain Layout Regions, it contains a full [FD] ("Flash Device Image") section. That's fine, but: > > - since you reference OVMF in the commit message, this difference should be mentioned there, > > - in the comment at the top of this new file, the "... with Layout Regions" is no longer precise. Please update it. > >> +BaseAddress = 0x04000000 > > (2) This has to be unified with the PcdFlashNvStorageVariableBase PCD setting that is currently seen in the QEMU DSC files, or we'll go mad down the road. > > In my opinion, the best way is to DEFINE a macro near the top of this file. Then reference that macro here, in the setting of the BaseAddress token, and also later, in the assignment to PcdFlashNvStorageVariableBase. > > For assigning PcdFlashNvStorageVariableBase, there are actually three ways. (They are all documented in the FDF spec.) > > * The simplest (but more limited) way is the following syntax: > > BaseAddress = $(VARS_BASE)|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase > > * The second, more complex (but more versatile) way is to employ a separate SET statement (also in this FDF include file): > > BaseAddress = $(VARS_BASE) > [...] > SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase = $(VARS_BASE) > > * The third way will actually be the winner, and I'll come to it a bit later. > > Finally, remove PcdFlashNvStorageVariableBase from the DSC files whose FDFs pull in this FDF include file. > >> +Size = 0x000c0000 >> +ErasePolarity = 1 >> + >> +# This one is tricky, it must be: BlockSize * NumBlocks = Size >> +BlockSize = 0x00040000 >> +NumBlocks = 0x3 > > (3) Actually, now I suggest to DEFINE the following macros at the top of the file: > > DEFINE VARS_BASE = 0x04000000 > DEFINE VARS_BLOCKS = 3 > DEFINE BLOCK_SIZE = 0x00040000 > DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) > DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) > > (It is possible that the multiplications above will not work in all of the contexts below -- in that case, just open-code the products please, and add some comments.) > > Then use these macros, for setting the above FD Tokens, like this: > > BaseAddress = $(VARS_BASE) It chokes at this reference already: /home/ard/build/edk2/ArmVirtPkg/VarStore.fdf.inc(26): error 3000: Invalid syntax/format expected Hex base address near line 26, column 0: BaseAddress = $(VARS_BASE) so this is not going to fly. > Size = $(VARS_SIZE) > ErasePolarity = 1 > BlockSize = $(BLOCK_SIZE) > NumBlocks = $(VARS_BLOCKS) > > Furthermore: > >> +0x00000000|0x00040000 > > (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. > > The most compact form for that is: > > 0x00000000|$(BLOCK_SIZE) > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > I have adopted this for v2, but without the BLOCK_SIZE macro, since I could not get that to work reliably either. >> +#NV_VARIABLE_STORE >> +DATA = { >> + ## This is the EFI_FIRMWARE_VOLUME_HEADER >> + # ZeroVector [] >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + # FileSystemGuid: gEfiSystemNvDataFvGuid = >> + # { 0xFFF12B8D, 0x7696, 0x4C8B, >> + # { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }} >> + 0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C, >> + 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50, >> + # FvLength: 0xC0000 >> + 0x00, 0x00, 0x0C, 0x00, 0x00, 0x00, 0x00, 0x00, >> + # Signature "_FVH" # Attributes >> + 0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00, >> + # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision >> + 0x48, 0x00, 0x28, 0x09, 0x00, 0x00, 0x00, 0x02, >> + # Blockmap[0]: 0x3 Blocks * 0x40000 Bytes / Block >> + 0x3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, >> + # Blockmap[1]: End >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + ## This is the VARIABLE_STORE_HEADER >> + # It is compatible with SECURE_BOOT_ENABLE == FALSE as well. >> + # Signature: gEfiAuthenticatedVariableGuid = >> + # { 0xaaf32c78, 0x947b, 0x439a, >> + # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }} >> + 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, >> + 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, >> + # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - >> + # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8 >> + # This can speed up the Variable Dispatch a bit. >> + 0xB8, 0xFF, 0x03, 0x00, >> + # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32 >> + 0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 >> +} >> + >> +0x00040000|0x00040000 > > (5) Please try the following: > > $(BLOCK_SIZE)|$(BLOCK_SIZE) > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > >> +#NV_FTW_WORKING >> +DATA = { >> + # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid = >> + # { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }} >> + 0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49, >> + 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95, >> + # Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved >> + 0x5b, 0xe7, 0xc6, 0x86, 0xFE, 0xFF, 0xFF, 0xFF, >> + # WriteQueueSize: UINT64 >> + 0xE0, 0xFF, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00 >> +} >> + >> +0x00080000|0x00040000 > > (6) Please try: > > $(SPARE_BASE)|$(BLOCK_SIZE) > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > >> +#NV_FTW_SPARE >> > > (7) I notice these DATA regions differ from those in OvmfPkg, and that makes sense. However, it's pretty hard to compare them manually. > > - Can you please enumerate in the commit message what fields have been changed / recalculated? > > - Can you please confirm that the generated FD file will match the contents of an originally zero-filled flash device that has just been formatted by NorFlashDxe? > > .. I think you can go ahead and commit the first three patches separately (assuming Leif agrees with patch #1), and we can collaborate on this last patch separately, if you wish. > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 06/22/16 17:30, Ard Biesheuvel wrote: > On 21 June 2016 at 17:43, Laszlo Ersek <lersek@redhat.com> wrote: >> (3) Actually, now I suggest to DEFINE the following macros at the top of the file: >> >> DEFINE VARS_BASE = 0x04000000 >> DEFINE VARS_BLOCKS = 3 >> DEFINE BLOCK_SIZE = 0x00040000 >> DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) >> DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) >> >> (It is possible that the multiplications above will not work in all of the contexts below -- in that case, just open-code the products please, and add some comments.) >> >> Then use these macros, for setting the above FD Tokens, like this: >> >> BaseAddress = $(VARS_BASE) > > It chokes at this reference already: > > /home/ard/build/edk2/ArmVirtPkg/VarStore.fdf.inc(26): error 3000: > Invalid syntax/format > expected Hex base address near line 26, column 0: BaseAddress > = $(VARS_BASE) > > so this is not going to fly. It's strange. If you check e.g. "OvmfPkgX64.fdf", we definitely use this pattern: [FD.OVMF] BaseAddress = $(FW_BASE_ADDRESS) -- [FD.OVMF_VARS] BaseAddress = $(FW_BASE_ADDRESS) -- [FD.OVMF_CODE] BaseAddress = $(CODE_BASE_ADDRESS) -- [FD.MEMFD] BaseAddress = $(MEMFD_BASE_ADDRESS) >> Size = $(VARS_SIZE) >> ErasePolarity = 1 >> BlockSize = $(BLOCK_SIZE) >> NumBlocks = $(VARS_BLOCKS) >> >> Furthermore: >> >>> +0x00000000|0x00040000 >> >> (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. >> >> The most compact form for that is: >> >> 0x00000000|$(BLOCK_SIZE) >> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize >> > > I have adopted this for v2, but without the BLOCK_SIZE macro, since I > could not get that to work reliably either. That's again strange! The OvmfPkg*.fdf files use this as well; just run: git grep '|\$' -- OvmfPkg/'*fdf' Something must be wrong with your macro definitions (perhaps due to the way I relayed them to you?) The above methods definitely work. Perhaps we should invite BaseTools people to help us debug this. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 23 June 2016 at 03:43, Gao, Liming <liming.gao@intel.com> wrote: > Ard: > Could you send the updated FDF file that causes build failure? I would like to see what issue here. > As it turns out, the defines should go into the associated .DSC file, in a [Defines] section. The .FDF parser tolerates the DEFINEs, but ignores them. So I tried putting the defines in ArmVirt.dsc.inc, which does work to some extent: these expressions DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) are rejected, and need to be open coded. Since the only macro that is actually used more than once is BLOCK_SIZE, and considering that moving these defines elsewhere and open coding some of them defeats the purpose of using macros anyway, I think it is better to go with my v2 (which Laslzo already indicated he is ok with) Thanks, Ard. >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, June 22, 2016 11:43 PM >> To: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Leif Lindholm >> <leif.lindholm@linaro.org> >> Subject: Re: [edk2] [PATCH 4/4] ArmVirtPkg: add FDF definition for empty >> varstore >> >> On 06/22/16 17:30, Ard Biesheuvel wrote: >> > On 21 June 2016 at 17:43, Laszlo Ersek <lersek@redhat.com> wrote: >> >> >> (3) Actually, now I suggest to DEFINE the following macros at the top of >> the file: >> >> >> >> DEFINE VARS_BASE = 0x04000000 >> >> DEFINE VARS_BLOCKS = 3 >> >> DEFINE BLOCK_SIZE = 0x00040000 >> >> DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) >> >> DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) >> >> >> >> (It is possible that the multiplications above will not work in all of the >> contexts below -- in that case, just open-code the products please, and add >> some comments.) >> >> >> >> Then use these macros, for setting the above FD Tokens, like this: >> >> >> >> BaseAddress = $(VARS_BASE) >> > >> > It chokes at this reference already: >> > >> > /home/ard/build/edk2/ArmVirtPkg/VarStore.fdf.inc(26): error 3000: >> > Invalid syntax/format >> > expected Hex base address near line 26, column 0: BaseAddress >> > = $(VARS_BASE) >> > >> > so this is not going to fly. >> >> It's strange. If you check e.g. "OvmfPkgX64.fdf", we definitely use this >> pattern: >> >> [FD.OVMF] >> BaseAddress = $(FW_BASE_ADDRESS) >> -- >> [FD.OVMF_VARS] >> BaseAddress = $(FW_BASE_ADDRESS) >> -- >> [FD.OVMF_CODE] >> BaseAddress = $(CODE_BASE_ADDRESS) >> -- >> [FD.MEMFD] >> BaseAddress = $(MEMFD_BASE_ADDRESS) >> >> >> Size = $(VARS_SIZE) >> >> ErasePolarity = 1 >> >> BlockSize = $(BLOCK_SIZE) >> >> NumBlocks = $(VARS_BLOCKS) >> >> >> >> Furthermore: >> >> >> >>> +0x00000000|0x00040000 >> >> >> >> (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign >> PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. >> >> >> >> The most compact form for that is: >> >> >> >> 0x00000000|$(BLOCK_SIZE) >> >> >> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiM >> deModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize >> >> >> > >> > I have adopted this for v2, but without the BLOCK_SIZE macro, since I >> > could not get that to work reliably either. >> >> That's again strange! The OvmfPkg*.fdf files use this as well; just run: >> >> git grep '|\$' -- OvmfPkg/'*fdf' >> >> Something must be wrong with your macro definitions (perhaps due to the >> way I relayed them to you?) The above methods definitely work. Perhaps >> we should invite BaseTools people to help us debug this. >> >> Thanks >> Laszlo >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 06/23/16 10:14, Ard Biesheuvel wrote: > On 23 June 2016 at 03:43, Gao, Liming <liming.gao@intel.com> wrote: >> Ard: >> Could you send the updated FDF file that causes build failure? I would like to see what issue here. >> > > As it turns out, the defines should go into the associated .DSC file, > in a [Defines] section. The .FDF parser tolerates the DEFINEs, but > ignores them. > > So I tried putting the defines in ArmVirt.dsc.inc, which does work to > some extent: these expressions > > DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) > DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) > > are rejected, and need to be open coded. > > Since the only macro that is actually used more than once is > BLOCK_SIZE, and considering that moving these defines elsewhere and > open coding some of them defeats the purpose of using macros anyway, I > think it is better to go with my v2 (which Laslzo already indicated he > is ok with) Yes, as I've indicated, I'm fine to proceed with v2. Just for curiosity's sake: can you try extracting the DEFINEs in question to a separate *FDF* include file? That's how we use them in OvmfPkg. Maybe the FDF parser in BaseTools will honor those macro defs if their definition and their place of use are separated by an !include directive in the FDF file. Thanks Laszlo > >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>> Laszlo Ersek >>> Sent: Wednesday, June 22, 2016 11:43 PM >>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Leif Lindholm >>> <leif.lindholm@linaro.org> >>> Subject: Re: [edk2] [PATCH 4/4] ArmVirtPkg: add FDF definition for empty >>> varstore >>> >>> On 06/22/16 17:30, Ard Biesheuvel wrote: >>>> On 21 June 2016 at 17:43, Laszlo Ersek <lersek@redhat.com> wrote: >>> >>>>> (3) Actually, now I suggest to DEFINE the following macros at the top of >>> the file: >>>>> >>>>> DEFINE VARS_BASE = 0x04000000 >>>>> DEFINE VARS_BLOCKS = 3 >>>>> DEFINE BLOCK_SIZE = 0x00040000 >>>>> DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) >>>>> DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) >>>>> >>>>> (It is possible that the multiplications above will not work in all of the >>> contexts below -- in that case, just open-code the products please, and add >>> some comments.) >>>>> >>>>> Then use these macros, for setting the above FD Tokens, like this: >>>>> >>>>> BaseAddress = $(VARS_BASE) >>>> >>>> It chokes at this reference already: >>>> >>>> /home/ard/build/edk2/ArmVirtPkg/VarStore.fdf.inc(26): error 3000: >>>> Invalid syntax/format >>>> expected Hex base address near line 26, column 0: BaseAddress >>>> = $(VARS_BASE) >>>> >>>> so this is not going to fly. >>> >>> It's strange. If you check e.g. "OvmfPkgX64.fdf", we definitely use this >>> pattern: >>> >>> [FD.OVMF] >>> BaseAddress = $(FW_BASE_ADDRESS) >>> -- >>> [FD.OVMF_VARS] >>> BaseAddress = $(FW_BASE_ADDRESS) >>> -- >>> [FD.OVMF_CODE] >>> BaseAddress = $(CODE_BASE_ADDRESS) >>> -- >>> [FD.MEMFD] >>> BaseAddress = $(MEMFD_BASE_ADDRESS) >>> >>>>> Size = $(VARS_SIZE) >>>>> ErasePolarity = 1 >>>>> BlockSize = $(BLOCK_SIZE) >>>>> NumBlocks = $(VARS_BLOCKS) >>>>> >>>>> Furthermore: >>>>> >>>>>> +0x00000000|0x00040000 >>>>> >>>>> (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign >>> PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. >>>>> >>>>> The most compact form for that is: >>>>> >>>>> 0x00000000|$(BLOCK_SIZE) >>>>> >>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiM >>> deModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize >>>>> >>>> >>>> I have adopted this for v2, but without the BLOCK_SIZE macro, since I >>>> could not get that to work reliably either. >>> >>> That's again strange! The OvmfPkg*.fdf files use this as well; just run: >>> >>> git grep '|\$' -- OvmfPkg/'*fdf' >>> >>> Something must be wrong with your macro definitions (perhaps due to the >>> way I relayed them to you?) The above methods definitely work. Perhaps >>> we should invite BaseTools people to help us debug this. >>> >>> Thanks >>> Laszlo >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 23 June 2016 at 12:57, Laszlo Ersek <lersek@redhat.com> wrote: > On 06/23/16 10:14, Ard Biesheuvel wrote: >> On 23 June 2016 at 03:43, Gao, Liming <liming.gao@intel.com> wrote: >>> Ard: >>> Could you send the updated FDF file that causes build failure? I would like to see what issue here. >>> >> >> As it turns out, the defines should go into the associated .DSC file, >> in a [Defines] section. The .FDF parser tolerates the DEFINEs, but >> ignores them. >> >> So I tried putting the defines in ArmVirt.dsc.inc, which does work to >> some extent: these expressions >> >> DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) >> DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) >> >> are rejected, and need to be open coded. >> >> Since the only macro that is actually used more than once is >> BLOCK_SIZE, and considering that moving these defines elsewhere and >> open coding some of them defeats the purpose of using macros anyway, I >> think it is better to go with my v2 (which Laslzo already indicated he >> is ok with) > > Yes, as I've indicated, I'm fine to proceed with v2. > OK, will do. > Just for curiosity's sake: can you try extracting the DEFINEs in > question to a separate *FDF* include file? That's how we use them in > OvmfPkg. Maybe the FDF parser in BaseTools will honor those macro defs > if their definition and their place of use are separated by an !include > directive in the FDF file. > The trick appears to be that the [Defines] .FDF section needs to precede any [FD] sections. That still means a separate .FDF include file just for these macro defs, so I am not going to bother -- Ard. >> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>>> Laszlo Ersek >>>> Sent: Wednesday, June 22, 2016 11:43 PM >>>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Leif Lindholm >>>> <leif.lindholm@linaro.org> >>>> Subject: Re: [edk2] [PATCH 4/4] ArmVirtPkg: add FDF definition for empty >>>> varstore >>>> >>>> On 06/22/16 17:30, Ard Biesheuvel wrote: >>>>> On 21 June 2016 at 17:43, Laszlo Ersek <lersek@redhat.com> wrote: >>>> >>>>>> (3) Actually, now I suggest to DEFINE the following macros at the top of >>>> the file: >>>>>> >>>>>> DEFINE VARS_BASE = 0x04000000 >>>>>> DEFINE VARS_BLOCKS = 3 >>>>>> DEFINE BLOCK_SIZE = 0x00040000 >>>>>> DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) >>>>>> DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) >>>>>> >>>>>> (It is possible that the multiplications above will not work in all of the >>>> contexts below -- in that case, just open-code the products please, and add >>>> some comments.) >>>>>> >>>>>> Then use these macros, for setting the above FD Tokens, like this: >>>>>> >>>>>> BaseAddress = $(VARS_BASE) >>>>> >>>>> It chokes at this reference already: >>>>> >>>>> /home/ard/build/edk2/ArmVirtPkg/VarStore.fdf.inc(26): error 3000: >>>>> Invalid syntax/format >>>>> expected Hex base address near line 26, column 0: BaseAddress >>>>> = $(VARS_BASE) >>>>> >>>>> so this is not going to fly. >>>> >>>> It's strange. If you check e.g. "OvmfPkgX64.fdf", we definitely use this >>>> pattern: >>>> >>>> [FD.OVMF] >>>> BaseAddress = $(FW_BASE_ADDRESS) >>>> -- >>>> [FD.OVMF_VARS] >>>> BaseAddress = $(FW_BASE_ADDRESS) >>>> -- >>>> [FD.OVMF_CODE] >>>> BaseAddress = $(CODE_BASE_ADDRESS) >>>> -- >>>> [FD.MEMFD] >>>> BaseAddress = $(MEMFD_BASE_ADDRESS) >>>> >>>>>> Size = $(VARS_SIZE) >>>>>> ErasePolarity = 1 >>>>>> BlockSize = $(BLOCK_SIZE) >>>>>> NumBlocks = $(VARS_BLOCKS) >>>>>> >>>>>> Furthermore: >>>>>> >>>>>>> +0x00000000|0x00040000 >>>>>> >>>>>> (4) This should use $(BLOCK_SIZE) after the pipe symbol, *plus* assign >>>> PcdFlashNvStorageVariableBase and PcdFlashNvStorageVariableSize at once. >>>>>> >>>>>> The most compact form for that is: >>>>>> >>>>>> 0x00000000|$(BLOCK_SIZE) >>>>>> >>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiM >>>> deModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize >>>>>> >>>>> >>>>> I have adopted this for v2, but without the BLOCK_SIZE macro, since I >>>>> could not get that to work reliably either. >>>> >>>> That's again strange! The OvmfPkg*.fdf files use this as well; just run: >>>> >>>> git grep '|\$' -- OvmfPkg/'*fdf' >>>> >>>> Something must be wrong with your macro definitions (perhaps due to the >>>> way I relayed them to you?) The above methods definitely work. Perhaps >>>> we should invite BaseTools people to help us debug this. >>>> >>>> Thanks >>>> Laszlo >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 23 June 2016 at 16:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 23 June 2016 at 12:57, Laszlo Ersek <lersek@redhat.com> wrote: >> On 06/23/16 10:14, Ard Biesheuvel wrote: >>> On 23 June 2016 at 03:43, Gao, Liming <liming.gao@intel.com> wrote: >>>> Ard: >>>> Could you send the updated FDF file that causes build failure? I would like to see what issue here. >>>> >>> >>> As it turns out, the defines should go into the associated .DSC file, >>> in a [Defines] section. The .FDF parser tolerates the DEFINEs, but >>> ignores them. >>> >>> So I tried putting the defines in ArmVirt.dsc.inc, which does work to >>> some extent: these expressions >>> >>> DEFINE VARS_SIZE = ($(VARS_BLOCKS) * $(BLOCK_SIZE)) >>> DEFINE SPARE_BASE = (2 * $(BLOCK_SIZE)) >>> >>> are rejected, and need to be open coded. >>> >>> Since the only macro that is actually used more than once is >>> BLOCK_SIZE, and considering that moving these defines elsewhere and >>> open coding some of them defeats the purpose of using macros anyway, I >>> think it is better to go with my v2 (which Laslzo already indicated he >>> is ok with) >> >> Yes, as I've indicated, I'm fine to proceed with v2. >> > > OK, will do. > Thanks all. Committed as bf57a42a0e2c -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf index 422a2bf1f9a1..ac2f1f17a042 100644 --- a/ArmVirtPkg/ArmVirtQemu.fdf +++ b/ArmVirtPkg/ArmVirtQemu.fdf @@ -69,6 +69,7 @@ [FD.QEMU_EFI] gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize FV = FVMAIN_COMPACT +!include VarStore.fdf.inc ################################################################################ # diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf index 6db0668a882d..f6dcbc1d5417 100644 --- a/ArmVirtPkg/ArmVirtQemuKernel.fdf +++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf @@ -91,6 +91,7 @@ [FD.QEMU_EFI] gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize FV = FVMAIN_COMPACT +!include VarStore.fdf.inc ################################################################################ # diff --git a/ArmVirtPkg/VarStore.fdf.inc b/ArmVirtPkg/VarStore.fdf.inc new file mode 100644 index 000000000000..7d41b6be6adb --- /dev/null +++ b/ArmVirtPkg/VarStore.fdf.inc @@ -0,0 +1,79 @@ +## @file +# FDF include file with Layout Regions that define an empty variable store. +# +# Copyright (c) 2016, Linaro, Ltd. All rights reserved.<BR> +# Copyright (C) 2014, Red Hat, Inc.<BR> +# Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> +# +# This program and the accompanying materials are licensed and made available +# under the terms and conditions of the BSD License which accompanies this +# distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR +# IMPLIED. +# +## + +[FD.QEMU_VARS] +BaseAddress = 0x04000000 +Size = 0x000c0000 +ErasePolarity = 1 + +# This one is tricky, it must be: BlockSize * NumBlocks = Size +BlockSize = 0x00040000 +NumBlocks = 0x3 + +0x00000000|0x00040000 +#NV_VARIABLE_STORE +DATA = { + ## This is the EFI_FIRMWARE_VOLUME_HEADER + # ZeroVector [] + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + # FileSystemGuid: gEfiSystemNvDataFvGuid = + # { 0xFFF12B8D, 0x7696, 0x4C8B, + # { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }} + 0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C, + 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50, + # FvLength: 0xC0000 + 0x00, 0x00, 0x0C, 0x00, 0x00, 0x00, 0x00, 0x00, + # Signature "_FVH" # Attributes + 0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00, + # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision + 0x48, 0x00, 0x28, 0x09, 0x00, 0x00, 0x00, 0x02, + # Blockmap[0]: 0x3 Blocks * 0x40000 Bytes / Block + 0x3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, + # Blockmap[1]: End + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ## This is the VARIABLE_STORE_HEADER + # It is compatible with SECURE_BOOT_ENABLE == FALSE as well. + # Signature: gEfiAuthenticatedVariableGuid = + # { 0xaaf32c78, 0x947b, 0x439a, + # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }} + 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, + 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, + # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - + # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8 + # This can speed up the Variable Dispatch a bit. + 0xB8, 0xFF, 0x03, 0x00, + # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32 + 0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 +} + +0x00040000|0x00040000 +#NV_FTW_WORKING +DATA = { + # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid = + # { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }} + 0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49, + 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95, + # Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved + 0x5b, 0xe7, 0xc6, 0x86, 0xFE, 0xFF, 0xFF, 0xFF, + # WriteQueueSize: UINT64 + 0xE0, 0xFF, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00 +} + +0x00080000|0x00040000 +#NV_FTW_SPARE
Similar to how OVMF implements this, add a FD definition for the varstore firmware volume and the FTW areas. This can be used by host side tooling to manipulate a pristine varstore before presenting it to the guest. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/ArmVirtQemu.fdf | 1 + ArmVirtPkg/ArmVirtQemuKernel.fdf | 1 + ArmVirtPkg/VarStore.fdf.inc | 79 ++++++++++++++++++++ 3 files changed, 81 insertions(+) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel