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=YCTjX6Np; spf=pass (domain: linaro.org, ip: 209.85.166.195, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f195.google.com (mail-it1-f195.google.com [209.85.166.195]) by groups.io with SMTP; Wed, 24 Apr 2019 00:11:28 -0700 Received: by mail-it1-f195.google.com with SMTP id e13so4501560itk.4 for ; Wed, 24 Apr 2019 00:11:28 -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=PoZ66orQ00N7JBnerp8N2aopMF3LSXJDVj4mQvFJAjU=; b=YCTjX6NpIZyoMDJ750xoHOmf2VOpySs+Xu5ccM8Jg7Jznv9olkJD0vSR78cK1zmm+H hxHfOkytNmcp0L6BT379Bw2nECkSnCQdCdw3LgQ9mxwKhQV4Jb3APucET0BI8tS7ZQ73 1bZk4ad4SzbkUEjOqEsXnAb5zrVnCo4v47y/JWUdwkWGaQrKVDdOuQdnh/R2gzNlglIp w45WP0wuAt/dEbhShSaNYbFEsmYDjpBkfyldj4QgqHcEkoHHUEgrO81yDnYNxGuc4rJy VP+k6Eur3n1c66R4q1298nEonpu9921HLnkI1UmSmaFyhpCGDMKK1ZaJMCMbSSpSZ5YR DHeg== 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=PoZ66orQ00N7JBnerp8N2aopMF3LSXJDVj4mQvFJAjU=; b=TZJ73tBDGJ36+VeL3/Nl5rWeKEEhqpIEuMWN3A0IMoh96vavlD/BvEBD8vQuaE0JqO 9Y/V1eveN/7fVDx9OVA4tIuz1m/L70tamZbsypM3l+Cr7JgmzbgWQAn2d8FTrL9Hv6Wb ONKhx4bSyb4ecakctAtB8Z7zY0RSeTwbL0BDaK9jeO4zCv4fWLsDB+cuiJZeJzvjvTUC C4Q7v3EpIWiXOdJHE0xcm/wtLpxN4Djo2ZLuKwxrxKV1Tn06Xv4MbTyChP2ZGNnf6fYb EeSc2PdT5kaFYpo30Mx4umkKMa09YdQJdTWNz1PPJz8wUKnsi1Y6ypyxPkv2EAyWmdie RIjg== X-Gm-Message-State: APjAAAURTxcshRZeVAWzUiFQa7lcMX1Ci7vR5bl+OzPx1EomuW7MEmBx M1uX2ahJIgQfTQIg0f8x0Z+1b1lmwsl4JnmeDEHcxw== X-Google-Smtp-Source: APXvYqy/qhRV4ceVajXIUO79t2O4rJYijczuL6AzvvIk/p4Z+J3AFdWmaxgQ2CVKbc1wYabUhT26G07ANu95+Myem7I= X-Received: by 2002:a24:59c1:: with SMTP id p184mr5974513itb.158.1556089887658; Wed, 24 Apr 2019 00:11:27 -0700 (PDT) MIME-Version: 1.0 References: <1556088711-14442-1-git-send-email-mw@semihalf.com> <1556088711-14442-4-git-send-email-mw@semihalf.com> In-Reply-To: <1556088711-14442-4-git-send-email-mw@semihalf.com> From: "Ard Biesheuvel" Date: Wed, 24 Apr 2019 09:11:15 +0200 Message-ID: Subject: Re: [edk2-platforms: PATCH v2 3/3] Marvell/Drivers: Add non-mmio mode to MvFvbDxe To: Marcin Wojtas Cc: edk2-devel-groups-io , Leif Lindholm , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Grzegorz Jaszczyk , Kostya Porotchkin , Jici Gao , Kornel Duleba Content-Type: text/plain; charset="UTF-8" On Wed, 24 Apr 2019 at 08:52, 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 > --- > 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 | 135 +++++++++++++++----- > 5 files changed, 135 insertions(+), 36 deletions(-) > > diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec > index 7210ba2..a23c329 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 > @@ -140,6 +146,8 @@ > #SPI > 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 ca3de2e..d53d128 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 Apologies for not spotting this before, but there is a problem here: FaultTolerantWriteDxe.inf does not depend on gEfiCpuArchProtocolGuid, but we do, and so we could now be dispatched before gEfiCpuArchProtocolGuid becomes available. This means you need to update the code to deal with that (or explain to me how if it already does) > 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 cb006cd..b4fd29c 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; > @@ -349,10 +353,13 @@ MvFvbSetAttributes ( > EFI_FVB_ATTRIBUTES_2 OldAttributes; > EFI_FVB_ATTRIBUTES_2 FlashFvbAttributes; > EFI_FVB_ATTRIBUTES_2 UnchangedAttributes; > + FVB_DEVICE *FlashInstance; > UINT32 Capabilities; > UINT32 OldStatus; > UINT32 NewStatus; > > + FlashInstance = INSTANCE_FROM_FVB_THIS (This); > + > // > // Obtain attributes from FVB header > // > @@ -369,12 +376,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 > // > @@ -692,6 +702,7 @@ MvFvbWrite ( > IN UINT8 *Buffer > ) > { > + EFI_STATUS Status; > FVB_DEVICE *FlashInstance; > UINTN DataOffset; > > @@ -701,10 +712,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; > } > > /** > @@ -975,6 +1003,9 @@ MvFvbConfigureFlashInstance ( > ) > { > EFI_STATUS Status; > + UINTN *NumBytes; > + UINTN DataOffset; > + UINTN VariableSize, FtwWorkingSize, FtwSpareSize, MemorySize; > > > // Locate SPI protocols > @@ -1009,25 +1040,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); > @@ -1043,6 +1111,12 @@ ErrorPrepareFvbHeader: > &gEfiFirmwareVolumeBlockProtocolGuid, > NULL); > > +ErrorFreeAllocatedPages: > + if (!FlashInstance->IsMemoryMapped) { > + FreeAlignedPages ((VOID *)FlashInstance->RegionBaseAddress, > + MemorySize); > + } > + > return Status; > } > > @@ -1094,24 +1168,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 >