From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=RLP+TWD6; spf=pass (domain: linaro.org, ip: 209.85.166.193, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f193.google.com (mail-it1-f193.google.com [209.85.166.193]) by groups.io with SMTP; Sat, 20 Apr 2019 10:46:00 -0700 Received: by mail-it1-f193.google.com with SMTP id y204so12623474itf.3 for ; Sat, 20 Apr 2019 10:46:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=oGiKd/qpUOwQchLrtHrzuKG3RAFcA4shbVYXZJrXWqE=; b=RLP+TWD64hO88bQIynbImvc5mjFDDpbZLuNFmdwe7D13PPCKFvs/Tci0WtWOi1ELvE eX9CtKBXm//yz/letBrZuLys0eQvCg990u8CvMpankA/dROLc50f1BogyghRb85mh7mq FjkP7NgC1KSHLOrRFni+DtGQ5r6eWGyTmRAhQsIx9ty7dahT3auw+L14IIGZaCfUOKHn DcM16nyd21NkQm1fj4qGJXXO9L6JelNcM/tp6lTXVIBq0mPk4DuSRlgOQPfhd4hf8FoT 7QAnTn+Pr+aztjHEGhx8o3aEu5nsbxrH0yfvY97iB+dcgdLkZ1E1JSGzdZBie8B8v9/5 0VCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=oGiKd/qpUOwQchLrtHrzuKG3RAFcA4shbVYXZJrXWqE=; b=m4aJEH37rkof4EOowc928wecjuqWuPRuEbw/mqXyhMPp81yb/vP7fxbih3wO6bOlJ+ R2rXKZjoT9GHuc53qLIJjKGpF8MjimqXhz+YCTnD00lle5UvPW/6t6x3i2sn0Z7F4oGN 9igW78cf+u0bIvQE+z3sbrF8s/Suz2z0lX0qV8tooJ/iwaUxh6j1ORFWLqMyaDLscCQu TvxNEDK7ceUQm5O7vVpC2mw92wavv46QpCH21VvcW3jQT7efiJnRY4pWjO1S0NGoYRsI fvr5W2cxbiNWNG95MTcwe9dm7xmVG/MTosbNMttDQKuFNH97i1uiYTFTc8dYIYnxBvXa 4bgA== X-Gm-Message-State: APjAAAU3GTt84PPOcrPGfIHoYCWX/pKwQSKgcE4L9/YmVG5ZQ/Yysejb wFjcVe88P8hHjLW/UEhSF74mGmBtvJLnNy3XnrQ9aQ== X-Google-Smtp-Source: APXvYqy0FYisLn3mST4BFK9MdS+BDDGHowX9t4Xv/MOvZklUuDZYSJg6AIB2lO8bR79/kCmbaYB9GN5zWTc4OlZ40FM= X-Received: by 2002:a24:59c1:: with SMTP id p184mr8072592itb.158.1555782359581; Sat, 20 Apr 2019 10:45:59 -0700 (PDT) MIME-Version: 1.0 References: <1555524356-1740-1-git-send-email-mw@semihalf.com> <1555524356-1740-4-git-send-email-mw@semihalf.com> In-Reply-To: <1555524356-1740-4-git-send-email-mw@semihalf.com> From: "Ard Biesheuvel" Date: Sat, 20 Apr 2019 19:45:47 +0200 Message-ID: Subject: Re: [edk2-platforms: PATCH 3/3] Marvell/Drivers: Add non-mmio mode to MvFvbDxe To: Marcin Wojtas Cc: edk2-devel-groups-io , Leif Lindholm , Nadav Haklai , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Grzegorz Jaszczyk , Kostya Porotchkin , Jici Gao , Kornel Duleba Content-Type: text/plain; charset="UTF-8" On Wed, 17 Apr 2019 at 20:06, Marcin Wojtas wrote: > > From: Kornel Duleba > > This path enables support for reading variables directly from flash without > relying on it to be memory mapped. It adds PcdSpiMemoryMapped PCD that > allows to switch between the modes. When in non-memory-mapped mode the > driver will copy the variables from flash to previously allocated buffer > and set PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase64 > and PcdFlashNvStorageFtwSpareBase64 accordingly. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marcin Wojtas How does this work with VariablePei? > --- > Silicon/Marvell/Marvell.dec | 8 ++ > Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 10 +- > Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf | 17 ++- > Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h | 1 + > Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c | 132 +++++++++++++++----- > 5 files changed, 132 insertions(+), 36 deletions(-) > > diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec > index 201a62f..70929d2 100644 > --- a/Silicon/Marvell/Marvell.dec > +++ b/Silicon/Marvell/Marvell.dec > @@ -58,6 +58,12 @@ > > gMarvellFvbDxeGuid = { 0x42903750, 0x7e61, 0x4aaf, { 0x83, 0x29, 0xbf, 0x42, 0x36, 0x4e, 0x24, 0x85 } } > gMarvellSpiFlashDxeGuid = { 0x49d7fb74, 0x306d, 0x42bd, { 0x94, 0xc8, 0xc0, 0xc5, 0x4b, 0x18, 0x1d, 0xd7 } } > + # > + # Generic FaultTolerantWriteDxe driver use variables, > + # whose setting is done in MvFvbDxe driver in case > + # the SPI contents are not mapped in memory. > + # > + gFaultTolerantWriteDxeFileGuid = { 0xfe5cea76, 0x4f72, 0x49e8, { 0x98, 0x6f, 0x2c, 0xd8, 0x99, 0xdf, 0xfe, 0x5d} } > > [LibraryClasses] > ArmadaBoardDescLib|Include/Library/ArmadaBoardDescLib.h > @@ -143,6 +149,8 @@ > gMarvellTokenSpaceGuid.PcdSpiClockRegBase|0|UINT64|0x30000056 > gMarvellTokenSpaceGuid.PcdSpiRegBase|0|UINT32|0x3000051 > gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0|UINT64|0x3000059 > + gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|TRUE|BOOLEAN|0x3000060 > + gMarvellTokenSpaceGuid.PcdSpiVariableOffset|0|UINT32|0x3000061 > gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|0|UINT32|0x30000052 > gMarvellTokenSpaceGuid.PcdSpiClockFrequency|0|UINT32|0x30000053 > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc > index a1ebb81..9974d5f 100644 > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc > @@ -256,6 +256,11 @@ > # USB support > gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE > > +[PcdsDynamicDefault.common] > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0xF93C0000 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0xF93E0000 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0xF93D0000 > + > [PcdsFixedAtBuild.common] > gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"MARVELL_EFI" > gArmPlatformTokenSpaceGuid.PcdCoreCount|4 > @@ -396,11 +401,10 @@ > # Variable store - default values > # > gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0xF9000000 > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0xF93C0000 > + gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|TRUE > + gMarvellTokenSpaceGuid.PcdSpiVariableOffset|0x3C0000 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000 > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0xF93D0000 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000 > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0xF93E0000 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000 > > !if $(CAPSULE_ENABLE) > diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf > index ef10bfd..c85e8a6 100644 > --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf > +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf > @@ -76,13 +76,22 @@ > gMarvellSpiMasterProtocolGuid > > [FixedPcd] > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > gMarvellTokenSpaceGuid.PcdSpiMemoryBase > + gMarvellTokenSpaceGuid.PcdSpiMemoryMapped > + gMarvellTokenSpaceGuid.PcdSpiVariableOffset > + > +[Pcd] > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64 > > [Depex] > - gEfiCpuArchProtocolGuid > + # > + # Generic FaultTolerantWriteDxe driver use variables, > + # whose setting is done in MvFvbDxe driver in case > + # the SPI contents are not mapped in memory. > + # > + BEFORE gFaultTolerantWriteDxeFileGuid > diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h > index 31e6e44..e8df9a5 100644 > --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h > +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h > @@ -55,6 +55,7 @@ typedef struct { > > UINT32 Signature; > > + BOOLEAN IsMemoryMapped; > UINTN DeviceBaseAddress; > UINTN RegionBaseAddress; > UINTN Size; > diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c > index b57f415..b1dfd62 100644 > --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c > +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c > @@ -52,6 +52,7 @@ STATIC CONST FVB_DEVICE mMvFvbFlashInstanceTemplate = { > > FVB_FLASH_SIGNATURE, // Signature > > + FALSE, // IsMemoryMapped ... NEED TO BE FILLED > 0, // DeviceBaseAddress ... NEED TO BE FILLED > 0, // RegionBaseAddress ... NEED TO BE FILLED > SIZE_256KB, // Size > @@ -175,11 +176,14 @@ MvFvbInitFvAndVariableStoreHeaders ( > FirmwareVolumeHeader->Attributes = EFI_FVB2_READ_ENABLED_CAP | > EFI_FVB2_READ_STATUS | > EFI_FVB2_STICKY_WRITE | > - EFI_FVB2_MEMORY_MAPPED | > EFI_FVB2_ERASE_POLARITY | > EFI_FVB2_WRITE_STATUS | > EFI_FVB2_WRITE_ENABLED_CAP; > > + if (FlashInstance->IsMemoryMapped) { > + FirmwareVolumeHeader->Attributes |= EFI_FVB2_MEMORY_MAPPED; > + } > + > FirmwareVolumeHeader->HeaderLength = sizeof (EFI_FIRMWARE_VOLUME_HEADER) + > sizeof (EFI_FV_BLOCK_MAP_ENTRY); > FirmwareVolumeHeader->Revision = EFI_FVH_REVISION; > @@ -382,12 +386,15 @@ MvFvbSetAttributes ( > EFI_FVB2_WRITE_ENABLED_CAP | \ > EFI_FVB2_LOCK_CAP | \ > EFI_FVB2_STICKY_WRITE | \ > - EFI_FVB2_MEMORY_MAPPED | \ > EFI_FVB2_ERASE_POLARITY | \ > EFI_FVB2_READ_LOCK_CAP | \ > EFI_FVB2_WRITE_LOCK_CAP | \ > EFI_FVB2_ALIGNMENT; > > + if (FlashInstance->IsMemoryMapped) { > + UnchangedAttributes |= EFI_FVB2_MEMORY_MAPPED; > + } > + > // > // Some attributes of FV is read only can *not* be set > // > @@ -714,6 +721,7 @@ MvFvbWrite ( > IN UINT8 *Buffer > ) > { > + EFI_STATUS Status; > FVB_DEVICE *FlashInstance; > UINTN DataOffset; > > @@ -728,10 +736,27 @@ MvFvbWrite ( > FlashInstance->StartLba + Lba, > FlashInstance->Media.BlockSize); > > - return FlashInstance->SpiFlashProtocol->Write (&FlashInstance->SpiDevice, > - DataOffset, > - *NumBytes, > - Buffer); > + Status = FlashInstance->SpiFlashProtocol->Write (&FlashInstance->SpiDevice, > + DataOffset, > + *NumBytes, > + Buffer); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, > + "%a: Failed to write to Spi device\n", > + __FUNCTION__)); > + return Status; > + } > + > + // Update shadow buffer > + if (!FlashInstance->IsMemoryMapped) { > + DataOffset = GET_DATA_OFFSET (FlashInstance->RegionBaseAddress + Offset, > + FlashInstance->StartLba + Lba, > + FlashInstance->Media.BlockSize); > + > + CopyMem ((UINTN *)DataOffset, Buffer, *NumBytes); > + } > + > + return EFI_SUCCESS; > } > > /** > @@ -1009,6 +1034,9 @@ MvFvbConfigureFlashInstance ( > ) > { > EFI_STATUS Status; > + UINTN *NumBytes; > + UINTN DataOffset; > + UINTN VariableSize, FtwWorkingSize, FtwSpareSize, MemorySize; > > > // Locate SPI protocols > @@ -1043,25 +1071,62 @@ MvFvbConfigureFlashInstance ( > } > > // Fill remaining flash description > - FlashInstance->DeviceBaseAddress = PcdGet64 (PcdSpiMemoryBase); > - FlashInstance->RegionBaseAddress = FixedPcdGet64 (PcdFlashNvStorageVariableBase64); > - FlashInstance->FvbOffset = FlashInstance->RegionBaseAddress - > - FlashInstance->DeviceBaseAddress; > - FlashInstance->FvbSize = PcdGet32(PcdFlashNvStorageVariableSize) + > - PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > - PcdGet32(PcdFlashNvStorageFtwSpareSize); > + VariableSize = PcdGet32 (PcdFlashNvStorageVariableSize); > + FtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize); > + FtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize); > + > + FlashInstance->IsMemoryMapped = PcdGetBool (PcdSpiMemoryMapped); > + FlashInstance->FvbSize = VariableSize + FtwWorkingSize + FtwSpareSize; > + FlashInstance->FvbOffset = PcdGet32 (PcdSpiVariableOffset); > > FlashInstance->Media.MediaId = 0; > FlashInstance->Media.BlockSize = FlashInstance->SpiDevice.Info->SectorSize; > FlashInstance->Media.LastBlock = FlashInstance->Size / > FlashInstance->Media.BlockSize - 1; > > + if (FlashInstance->IsMemoryMapped) { > + FlashInstance->DeviceBaseAddress = PcdGet64 (PcdSpiMemoryBase); > + FlashInstance->RegionBaseAddress = PcdGet64 (PcdFlashNvStorageVariableBase64); > + } else { > + MemorySize = EFI_SIZE_TO_PAGES (FlashInstance->FvbSize); > + > + // FaultTolerantWriteDxe requires memory to be aligned to FtwWorkingSize > + FlashInstance->RegionBaseAddress = (UINTN) AllocateAlignedRuntimePages (MemorySize, > + SIZE_64KB); > + if (FlashInstance->RegionBaseAddress == (UINTN) NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + PcdSet64 (PcdFlashNvStorageVariableBase64, > + (UINT64) FlashInstance->RegionBaseAddress); > + PcdSet64 (PcdFlashNvStorageFtwWorkingBase64, > + (UINT64) FlashInstance->RegionBaseAddress > + + VariableSize); > + PcdSet64 (PcdFlashNvStorageFtwSpareBase64, > + (UINT64) FlashInstance->RegionBaseAddress > + + VariableSize > + + FtwWorkingSize); > + > + // Fill the buffer with data from flash > + DataOffset = GET_DATA_OFFSET (FlashInstance->FvbOffset, > + FlashInstance->StartLba, > + FlashInstance->Media.BlockSize); > + *NumBytes = FlashInstance->FvbSize; > + Status = FlashInstance->SpiFlashProtocol->Read (&FlashInstance->SpiDevice, > + DataOffset, > + *NumBytes, > + (VOID *)FlashInstance->RegionBaseAddress); > + if (EFI_ERROR (Status)) { > + goto ErrorFreeAllocatedPages; > + } > + } > + > Status = gBS->InstallMultipleProtocolInterfaces (&FlashInstance->Handle, > &gEfiDevicePathProtocolGuid, &FlashInstance->DevicePath, > &gEfiFirmwareVolumeBlockProtocolGuid, &FlashInstance->FvbProtocol, > NULL); > if (EFI_ERROR (Status)) { > - return Status; > + goto ErrorFreeAllocatedPages; > } > > Status = MvFvbPrepareFvHeader (FlashInstance); > @@ -1077,6 +1142,12 @@ ErrorPrepareFvbHeader: > &gEfiFirmwareVolumeBlockProtocolGuid, > NULL); > > +ErrorFreeAllocatedPages: > + if (!FlashInstance->IsMemoryMapped) { > + FreeAlignedPages ((VOID *)FlashInstance->RegionBaseAddress, > + MemorySize); > + } > + > return Status; > } > > @@ -1128,24 +1199,27 @@ MvFvbEntryPoint ( > // > // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME > // > - RuntimeMmioRegionSize = mFvbDevice->FvbSize; > RegionBaseAddress = mFvbDevice->RegionBaseAddress; > > - Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, > - RegionBaseAddress, > - RuntimeMmioRegionSize, > - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__)); > - goto ErrorAddSpace; > - } > + if (mFvbDevice->IsMemoryMapped) { > + RuntimeMmioRegionSize = mFvbDevice->FvbSize; > + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, > + RegionBaseAddress, > + RuntimeMmioRegionSize, > + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__)); > + goto ErrorAddSpace; > + } > > - Status = gDS->SetMemorySpaceAttributes (RegionBaseAddress, > - RuntimeMmioRegionSize, > - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", __FUNCTION__)); > - goto ErrorSetMemAttr; > + > + Status = gDS->SetMemorySpaceAttributes (RegionBaseAddress, > + RuntimeMmioRegionSize, > + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", __FUNCTION__)); > + goto ErrorSetMemAttr; > + } > } > > // > -- > 2.7.4 >