On 10/13/15 14:38, Leif Lindholm wrote: > On Tue, Oct 13, 2015 at 01:55:47PM +0200, Laszlo Ersek wrote: >> The ValidateFvHeader() function checks several conditions against the >> firmware volume header. Failure of the first of these checks, reported as >> "No Firmware Volume header present", is a common situation for unformatted >> flash images, especially when a new virtual machine is created. >> >> Similarly, "Variable Store Guid non-compatible" is common when the >> firmware binary is switched from Secure Boot-incapable to Secure >> Boot-capable, or vice versa. >> >> The only caller of ValidateFvHeader(), NorFlashFvbInitialize(), handles >> all these mismatches by installing a new FVB header. It also emits >> another, loud ERROR message (which is even less justified when it is >> triggered by (BootMode == BOOT_WITH_DEFAULT_SETTINGS)). >> >> Downgrade these messages from EFI_D_ERROR to EFI_D_INFO, so that they >> don't clutter the debug output when the PcdDebugPrintErrorLevel mask only >> enables EFI_D_ERROR (i.e., in a "silent" build). >> >> These messages have annoyed / confused users; see for example: >> - https://bugzilla.redhat.com/show_bug.cgi?id=1270279 >> - http://thread.gmane.org/gmane.comp.bios.edk2.devel/2772/focus=2869 > > This all looks very sensible. One comment inline. > >> Cc: Leif Lindholm <[email protected]> >> Cc: Ard Biesheuvel <[email protected]> >> Cc: Drew Jones <[email protected]> >> Cc: Yehuda Yitschak <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> index 3ed3bb9..13df62a 100644 >> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> @@ -151,48 +151,53 @@ ValidateFvHeader ( >> // >> // Verify the header revision, header signature, length >> // Length of FvBlock cannot be 2**64-1 >> // HeaderLength cannot be an odd number >> // >> if ( (FwVolHeader->Revision != EFI_FVH_REVISION) >> || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) >> || (FwVolHeader->FvLength != FvLength) >> ) >> { >> - DEBUG ((EFI_D_ERROR, "ValidateFvHeader: No Firmware Volume header >> present\n")); >> + DEBUG ((EFI_D_INFO, "%a: No Firmware Volume header present\n", >> + __FUNCTION__)); >> return EFI_NOT_FOUND; >> } >> >> // Check the Firmware Volume Guid >> if( CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid) >> == FALSE ) { >> - DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Firmware Volume Guid >> non-compatible\n")); >> + DEBUG ((EFI_D_INFO, "%a: Firmware Volume Guid non-compatible\n", >> + __FUNCTION__)); >> return EFI_NOT_FOUND; >> } >> >> // Verify the header checksum >> Checksum = CalculateSum16((UINT16*)FwVolHeader, >> FwVolHeader->HeaderLength); >> if (Checksum != 0) { >> - DEBUG ((EFI_D_ERROR, "ValidateFvHeader: FV checksum is invalid >> (Checksum:0x%X)\n",Checksum)); >> + DEBUG ((EFI_D_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n", >> + __FUNCTION__, Checksum)); >> return EFI_NOT_FOUND; >> } >> >> VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + >> FwVolHeader->HeaderLength); >> >> // Check the Variable Store Guid >> if (!CompareGuid (&VariableStoreHeader->Signature, >> mNorFlashVariableGuid)) { >> - DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid >> non-compatible\n")); >> + DEBUG ((EFI_D_INFO, "%a: Variable Store Guid non-compatible\n", >> + __FUNCTION__)); >> return EFI_NOT_FOUND; >> } >> >> VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) - >> FwVolHeader->HeaderLength; >> if (VariableStoreHeader->Size != VariableStoreLength) { >> - DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Length does not >> match\n")); >> + DEBUG ((EFI_D_INFO, "%a: Variable Store Length does not match\n", >> + __FUNCTION__)); >> return EFI_NOT_FOUND; >> } >> >> return EFI_SUCCESS; >> } >> >> /** >> The GetAttributes() function retrieves the attributes and >> current settings of the block. >> >> @@ -724,21 +729,22 @@ NorFlashFvbInitialize ( >> if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) { >> Status = EFI_INVALID_PARAMETER; >> } else { >> // Determine if there is a valid header at the beginning of the NorFlash >> Status = ValidateFvHeader (Instance); >> } >> >> // Install the Default FVB header if required >> if (EFI_ERROR(Status)) { >> // There is no valid header, so time to install one. >> - DEBUG((EFI_D_ERROR,"NorFlashFvbInitialize: ERROR - The FVB Header is >> not valid. Installing a correct one for this volume.\n")); >> + DEBUG ((EFI_D_INFO, "%a: The FVB Header is not valid. Installing a " >> + "correct one for this volume.\n", __FUNCTION__)); > > While I approve of breaking output lines up to reviewable lengths, > this one actually breaks up the message string itself - making > tracking it down in the source more tedious. Could you either leave > the message string whole, or break the two sentences into separate > DEBUG statements?
I'll break them up. I don't like overlong lines. :) I assume you'd like to see the two sentences on separate lines in the log output as well (so that noone is tempted to search for the full line in the source). Is that right? > Do that and: > Reviewed-by: Leif Lindholm <[email protected]> Thanks! Laszlo > >> >> // Erase all the NorFlash that is reserved for variable storage >> FvbNumLba = (PcdGet32(PcdFlashNvStorageVariableSize) + >> PcdGet32(PcdFlashNvStorageFtwWorkingSize) + >> PcdGet32(PcdFlashNvStorageFtwSpareSize)) / Instance->Media.BlockSize; >> >> Status = FvbEraseBlocks (&Instance->FvbProtocol, (EFI_LBA)0, FvbNumLba, >> EFI_LBA_LIST_TERMINATOR); >> if (EFI_ERROR(Status)) { >> return Status; >> } >> >> // Install all appropriate headers >> -- >> 1.8.3.1 >> >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
