Re: [platforms PATCH v2 5/5] Marvell/Armada7k8k: Wire up capsule support

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

2018-06-04 12:08 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>
> On Mon, Jun 04, 2018 at 07:29:35AM +0200, Marcin Wojtas wrote:
> > All required components are in place, so we can now
> > add all necessary dependencies to build and use capsule support
> > for Armada7k8k platforms. It is conditionally enabled
> > with '-D CAPSULE_ENABLE' flag added during build time.
> >
> > Because the capsule generation must be sequential,
> > due to boot requirements and glueing all binaries
> > (BLE, ARM-TF, BL33) externally, introduce additional
> > .dsc and .fdf file solely for creating the capsule.
> > Prior to this step 'flash-image.bin' binary must be placed
> > under Platform/Marvell path.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > Signed-off-by: David Sniatkiwicz <davidsn@marvell.com>
> > ---
> >  Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf        | 66 ++++++++++++++++++
> >  Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.dsc | 46 +++++++++++++
> >  Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.fdf | 70 ++++++++++++++++++++
> >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc         | 41 ++++++++++++
> >  4 files changed, 223 insertions(+)
> >
> > diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf b/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf
> > index bf04f4d..e5e5443 100644
> > --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf
> > +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf
> > @@ -201,6 +201,15 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
> >    INF MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> >    INF MdeModulePkg/Application/UiApp/UiApp.inf
> >
> > +!if $(CAPSULE_ENABLE)
> > +  # Firmware update
> > +  INF MdeModulePkg/Universal/EsrtDxe/EsrtDxe.inf
> > +  INF SignedCapsulePkg/Universal/SystemFirmwareUpdate/SystemFirmwareReportDxe.inf
> > +  FILE FREEFORM = PCD(gEfiSignedCapsulePkgTokenSpaceGuid.PcdEdkiiPkcs7TestPublicKeyFileGuid) {
> > +     SECTION RAW = BaseTools/Source/Python/Pkcs7Sign/TestRoot.cer
> > +     SECTION UI = "Pkcs7TestRoot"
> > +  }
> > +!endif
> >
> >  # PEI phase firmware volume
> >  [FV.FVMAIN_COMPACT]
> > @@ -228,6 +237,11 @@ READ_LOCK_STATUS   = TRUE
> >    INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> >    INF ArmPkg/Drivers/CpuPei/CpuPei.inf
> >    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> > +!if $(CAPSULE_ENABLE)
> > +  INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
> > +  INF MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> > +  INF RuleOverride = FMP_IMAGE_DESC Silicon/Marvell/Armada7k8k/Feature/Capsule/SystemFirmwareDescriptor/SystemFirmwareDescriptor.inf
> > +!endif
> >    INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >
> >    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
> > @@ -236,6 +250,49 @@ READ_LOCK_STATUS   = TRUE
> >      }
> >    }
> >
> > +!if $(CAPSULE_ENABLE)
> > +[FV.SystemFirmwareDescriptor]
> > +FvAlignment        = 8
> > +ERASE_POLARITY     = 1
> > +MEMORY_MAPPED      = TRUE
> > +STICKY_WRITE       = TRUE
> > +LOCK_CAP           = TRUE
> > +LOCK_STATUS        = TRUE
> > +WRITE_DISABLED_CAP = TRUE
> > +WRITE_ENABLED_CAP  = TRUE
> > +WRITE_STATUS       = TRUE
> > +WRITE_LOCK_CAP     = TRUE
> > +WRITE_LOCK_STATUS  = TRUE
> > +READ_DISABLED_CAP  = TRUE
> > +READ_ENABLED_CAP   = TRUE
> > +READ_STATUS        = TRUE
> > +READ_LOCK_CAP      = TRUE
> > +READ_LOCK_STATUS   = TRUE
> > +
> > +  INF RuleOverride = FMP_IMAGE_DESC Silicon/Marvell/Armada7k8k/Feature/Capsule/SystemFirmwareDescriptor/SystemFirmwareDescriptor.inf
> > +
> > +[FV.CapsuleDispatchFv]
> > +FvAlignment        = 8
> > +ERASE_POLARITY     = 1
> > +MEMORY_MAPPED      = TRUE
> > +STICKY_WRITE       = TRUE
> > +LOCK_CAP           = TRUE
> > +LOCK_STATUS        = TRUE
> > +WRITE_DISABLED_CAP = TRUE
> > +WRITE_ENABLED_CAP  = TRUE
> > +WRITE_STATUS       = TRUE
> > +WRITE_LOCK_CAP     = TRUE
> > +WRITE_LOCK_STATUS  = TRUE
> > +READ_DISABLED_CAP  = TRUE
> > +READ_ENABLED_CAP   = TRUE
> > +READ_STATUS        = TRUE
> > +READ_LOCK_CAP      = TRUE
> > +READ_LOCK_STATUS   = TRUE
> > +
> > +  INF  SignedCapsulePkg/Universal/SystemFirmwareUpdate/SystemFirmwareUpdateDxe.inf
> > +
> > +!endif
> > +
> >  ################################################################################
> >  #
> >  # Rules are use with the [FV] section's module INF type to define
> > @@ -331,3 +388,12 @@ READ_LOCK_STATUS   = TRUE
> >      UI     STRING ="$(MODULE_NAME)" Optional
> >      PE32   PE32                         $(INF_OUTPUT)/$(MODULE_NAME).efi
> >    }
> > +
> > +[Rule.Common.PEIM.FMP_IMAGE_DESC]
> > +  FILE PEIM = $(NAMED_GUID) {
> > +     RAW BIN                  |.acpi
> > +     PEI_DEPEX PEI_DEPEX Optional        $(INF_OUTPUT)/$(MODULE_NAME).depex
> > +     PE32      PE32    Align=4K          $(INF_OUTPUT)/$(MODULE_NAME).efi
> > +     UI       STRING="$(MODULE_NAME)" Optional
> > +     VERSION  STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER)
> > +  }
> > diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.dsc b/Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.dsc
> > new file mode 100644
> > index 0000000..a7d1382
> > --- /dev/null
> > +++ b/Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.dsc
> > @@ -0,0 +1,46 @@
> > +#Copyright (C) 2018 Marvell International Ltd.
> > +#
> > +#Marvell BSD License Option
> > +#
> > +#If you received this File from Marvell, you may opt to use, redistribute and/or
> > +#modify this File under the following licensing terms.
> > +#Redistribution and use in source and binary forms, with or without modification,
> > +#are permitted provided that the following conditions are met:
> > +#
> > +# * Redistributions of source code must retain the above copyright notice,
> > +# this list of conditions and the following disclaimer.
> > +#
> > +# * Redistributions in binary form must reproduce the above copyright
> > +# notice, this list of conditions and the following disclaimer in the
> > +# documentation and/or other materials provided with the distribution.
> > +#
> > +# * Neither the name of Marvell nor the names of its contributors may be
> > +# used to endorse or promote products derived from this software without
> > +# specific prior written permission.
> > +#
> > +#THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> > +#ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> > +#WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> > +#DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> > +#ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> > +#(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> > +#LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> > +#ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > +#(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> > +#SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > +#
> > +################################################################################
> > +#
> > +# Defines Section - statements that will be processed to create a Makefile.
> > +#
> > +################################################################################
> > +[Defines]
> > +  PLATFORM_NAME                  = Armada70x0Db
> > +  PLATFORM_GUID                  = 982e2ab1-26ca-4617-92e6-061bd6ba9ae3
> > +  PLATFORM_VERSION               = 0.1
> > +  DSC_SPECIFICATION              = 0x00010019
> > +  OUTPUT_DIRECTORY               = Build/$(PLATFORM_NAME)-$(ARCH)
> > +  SUPPORTED_ARCHITECTURES        = AARCH64|ARM
> > +  BUILD_TARGETS                  = DEBUG|RELEASE
> > +  SKUID_IDENTIFIER               = DEFAULT
> > +  FLASH_DEFINITION               = Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.fdf
> > diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.fdf b/Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.fdf
> > new file mode 100644
> > index 0000000..e5a9c68
> > --- /dev/null
> > +++ b/Platform/Marvell/Armada70x0Db/Armada70x0DbCapsule.fdf
> > @@ -0,0 +1,70 @@
> > +#
> > +#  Copyright (C) Marvell International Ltd. and its affiliates
> > +#
> > +#  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.Armada_Capsule]
> > +BaseAddress   = 0x00000000 #|gArmTokenSpaceGuid.PcdFdBaseAddress  # The base address of the Firmware in NOR Flash.
> > +Size          = 0x00400000 #|gArmTokenSpaceGuid.PcdFdSize         # The size in bytes of the FLASH Device
> > +ErasePolarity = 1
> > +
> > +0x00000000|0x00010000
> > +FILE = $(WORKSPACE)/$(OUTPUT_DIRECTORY)/$(TARGET)_$(TOOL_CHAIN_TAG)/FV/SYSTEMFIRMWAREDESCRIPTOR.Fv
> > +
> > +0x00100000|0x00300000
> > +FILE = Platform/Marvell/flash-image.bin
>
> So, I'm happy with everything in this set other than the line above.
> (You can take this as Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> for 1-4/5.)
>
> Mandating magic copying into the source tree of special binaries (that
> can then not be used to build separate DEBUG/RELEASE images in one
> session without deleting them in the meantime) is out.
>
> When I integrated arm-tf build support into uefi-tools, I did the
> following, to allow the build artefacts to all reside within the Build
> directory:
> https://git.linaro.org/uefi/uefi-tools.git/tree/atf-build.sh#n201
>
> Ard hates this, because it makes assumptions about the internals of
> the EDK2 build environment not overriding things in Conf/target.txt
> (which is admittedly valid when then turning edk2 back into a consumer
> of the output - the above was only ever used for publishing purposes).
>
> His preference is replacing the "Platform/Marvell/" bit with a -D
> option to tell the build system in which directory to look for the
> file. I could go along with that.
>
> So the question remaining is what do we call the option?
> Since this situation is likely to reappear for other platforms, let's
> keep it generic. ARM_TRUSTED_FIRMWARE_IMAGE_PREFIX or suchlike.
>

Sure, I think -D
ARM_TRUSTED_FIRMWARE_IMAGE_PREFIX=<path/to/flash-image.bin> would
work.

Also after internal review I was asked for a change - how about making
a common Armada7k8kCapsule.dsc / .fdf and simply pick the platform
with -D PLATFORM_NAME option?

Thanks,
Marcin