[Linaro-uefi,v2,10/10] Platforms/AMD/Styx: add command line flash tool

This implements a UEFI Shell application that updates the EFI NOR
partition with the contents of STYX_EFI.Fv. Note that this means that
a) EFI variables are preserved, and
b) this updater can only be used on systems that have already been flashed
   with a firmware build based on this open source branch (and not on systems
   still running AMI firmware)

In order for the flasher to have access to the various PCDs that describe
where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
platform, but after STYX_EFI.Fv has been generated, which results in a
chicken-and-egg situation. Therefore, the recommended way of generating
the flasher is

1) delete the Build/<platform> directory entirely
2) build the platform
3) build the platform again, but with -D DO_FLASHER=TRUE appended

The flasher application is called 'StyxFlashUefi'

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
 Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
 Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
 Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
 Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
 Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
 Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
 7 files changed, 286 insertions(+), 2 deletions(-)

On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
> This implements a UEFI Shell application that updates the EFI NOR

standalone UEFI Shell application

> partition with the contents of STYX_EFI.Fv. Note that this means that
> a) EFI variables are preserved, and
> b) this updater can only be used on systems that have already been flashed
>    with a firmware build based on this open source branch (and not on systems
>    still running AMI firmware)
> 
> In order for the flasher to have access to the various PCDs that describe
> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
> platform, but after STYX_EFI.Fv has been generated, which results in a
> chicken-and-egg situation. Therefore, the recommended way of generating
> the flasher is
> 
> 1) delete the Build/<platform> directory entirely
> 2) build the platform
> 3) build the platform again, but with -D DO_FLASHER=TRUE appended

Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
platform?

> The flasher application is called 'StyxFlashUefi'

Only style comments below, but one silly question before that:
Is there some particular reason why the application embeds the FV
instead of reading it from a filesystem? Or just that you figured it's
small enough that it doesn't matter?

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
>  7 files changed, 286 insertions(+), 2 deletions(-)
> 
> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds b/Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds
> new file mode 100644
> index 000000000000..7a0c87c6e32b
> --- /dev/null
> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds
> @@ -0,0 +1,86 @@
> +/** @file
> +
> +  Unified linker script for GCC based builds
> +
> +  Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2015 - 2017, Linaro Ltd. All rights reserved.<BR>
> +  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +
> +  This program and the accompanying materials are licensed and made available under
> +  the terms and conditions of the BSD License that 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.
> +
> +**/
> +
> +SECTIONS {
> +
> +  /*
> +   * The PE/COFF binary consists of DOS and PE/COFF headers, and a sequence of
> +   * section headers adding up to PECOFF_HEADER_SIZE bytes (which differs
> +   * between 32-bit and 64-bit builds). The actual start of the .text section
> +   * will be rounded up based on its actual alignment.
> +   */
> +  . = PECOFF_HEADER_SIZE;
> +
> +  .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> +    *(.text .text.* .stub .gnu.linkonce.t.*)
> +    *(.rodata .rodata.* .gnu.linkonce.r.*)
> +    *(.got .got.*)
> +
> +    /*
> +     * The contents of AutoGen.c files are mostly constant from the POV of the
> +     * program, but most of it ends up in .data or .bss by default since few of
> +     * the variable definitions that get emitted are declared as CONST.
> +     * Unfortunately, we cannot pull it into the .text section entirely, since
> +     * patchable PCDs are also emitted here, but we can at least move all of the
> +     * emitted GUIDs here.
> +     */
> +    *:AutoGen.obj(.data.g*Guid)
> +  }
> +
> +  /*
> +   * The alignment of the .data section should be less than or equal to the
> +   * alignment of the .text section. This ensures that the relative offset
> +   * between these sections is the same in the ELF and the PE/COFF versions of
> +   * this binary.
> +   */
> +  .data ALIGN(ALIGNOF(.text)) : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> +    *(.data .data.* .gnu.linkonce.d.*)
> +    *(.bss .bss.*)
> +    *(.payload)
> +  }
> +
> +  .eh_frame ALIGN(CONSTANT(COMMONPAGESIZE)) : {
> +    KEEP (*(.eh_frame))
> +  }
> +
> +  .rela (INFO) : {
> +    *(.rela .rela.*)
> +  }
> +
> +  .hii : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> +    KEEP (*(.hii))
> +  }
> +
> +  /*
> +   * Retain the GNU build id but in a non-allocatable section so GenFw
> +   * does not copy it into the PE/COFF image.
> +   */
> +  .build-id (INFO) : { *(.note.gnu.build-id) }
> +
> +  /DISCARD/ : {
> +    *(.note.GNU-stack)
> +    *(.gnu_debuglink)
> +    *(.interp)
> +    *(.dynsym)
> +    *(.dynstr)
> +    *(.dynamic)
> +    *(.hash .gnu.hash)
> +    *(.comment)
> +    *(COMMON)
> +  }
> +}
> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S
> new file mode 100644
> index 000000000000..041339ee9b47
> --- /dev/null
> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S
> @@ -0,0 +1,25 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro Ltd. 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.
> +
> +**/
> +
> +  .section  ".payload"
> +  .align    12
> +
> +ASM_GLOBAL ASM_PFX(StyxFlashImageStart)
> +ASM_PFX(StyxFlashImageStart):
> +  .incbin   "STYX_EFI.Fv"
> +
> +  .align    2
> +ASM_GLOBAL ASM_PFX(StyxFlashImageSize)
> +ASM_PFX(StyxFlashImageSize):
> +  .long    . - ASM_PFX(StyxFlashImageStart)
> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c
> new file mode 100644
> index 000000000000..a7bdbffab891
> --- /dev/null
> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c
> @@ -0,0 +1,96 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro Ltd. 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.
> +
> +**/
> +
> +#include  <Uefi.h>
> +#include <Library/BaseMemoryLib.h>
> +#include  <Library/UefiBootServicesTableLib.h>
> +#include  <Library/UefiLib.h>
> +#include  <Library/ShellCEntryLib.h>

Inconsistent whitespace. Not sorted.

> +
> +#include <Protocol/AmdIscpDxeProtocol.h>
> +
> +#define UEFI_IMAGE_OFFSET     FixedPcdGet64 (PcdFvBaseAddress) - FixedPcdGet64 (PcdFdBaseAddress)
> +#define BLOCK_SIZE            SIZE_64KB
> +
> +STATIC AMD_ISCP_DXE_PROTOCOL    *mIscpDxeProtocol;
> +STATIC UINT8                    Buffer[BLOCK_SIZE];
> +
> +extern CONST UINT8    StyxFlashImageStart[];
> +extern CONST UINT32   StyxFlashImageSize;
> +
> +/***
> +  Main entrypoint
> +
> +  Establishes the main structure of the application.
> +
> +  @retval  0         The application exited normally.
> +  @retval  Other     An error occurred.
> +***/
> +INTN
> +EFIAPI
> +ShellAppMain (
> +  IN UINTN Argc,
> +  IN CHAR16 **Argv
> +  )
> +{
> +  EFI_STATUS      Status;
> +  UINTN           Index;
> +  INTN            Remaining;
> +
> +  Print(L"StyxFlashUefi: firmware updater for AMD Seattle based boards.\n");

Space after "Print".
(Actually, this seems consistent on all Print statement
in this function, and nothing else.)

> +
> +  Status = gBS->LocateProtocol (&gAmdIscpDxeProtocolGuid, NULL,
> +                  (VOID **)&mIscpDxeProtocol);
> +  if (EFI_ERROR (Status)) {
> +    Print(L"Failed to locate ISCP communication protocol, terminating...\n");
> +    return (INTN)Status;
> +  }
> +
> +  Index = 0;
> +  Remaining = StyxFlashImageSize;
> +  do {
> +    Status = mIscpDxeProtocol->AmdExecuteEraseFvBlockDxe (
> +                                 mIscpDxeProtocol,
> +                                 UEFI_IMAGE_OFFSET + Index * BLOCK_SIZE,
> +                                 BLOCK_SIZE);
> +    if (EFI_ERROR (Status)) {
> +      Print(L"Erase failed!\n");
> +      return (INTN)Status;
> +    }
> +
> +    CopyMem (Buffer, StyxFlashImageStart + Index * BLOCK_SIZE,
> +      MIN (Remaining, BLOCK_SIZE));
> +
> +    Status = mIscpDxeProtocol->AmdExecuteUpdateFvBlockDxe (
> +                                 mIscpDxeProtocol,
> +                                 UEFI_IMAGE_OFFSET + Index * BLOCK_SIZE,
> +                                 Buffer,
> +                                 MIN (Remaining, BLOCK_SIZE));
> +
> +    if (EFI_ERROR (Status)) {
> +      Print(L"Update failed!\n");
> +      return (INTN)Status;
> +    }
> +
> +    Remaining -= BLOCK_SIZE;
> +    Index++;
> +
> +    Print(L"Block %d of %d updated\n", Index, StyxFlashImageSize / BLOCK_SIZE);
> +
> +  } while (Remaining > 0);
> +
> +  Print(L"\nDone!\n");
> +
> +  return 0;
> +}
> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf
> new file mode 100644
> index 000000000000..2ad7c1b98ef1
> --- /dev/null
> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf
> @@ -0,0 +1,53 @@
> +#/** @file
> +#
> +#  Copyright (c) 2017, Linaro Ltd. 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.
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010006

Not 1.25?

> +  BASE_NAME                      = StyxFlashUefi
> +  FILE_GUID                      = 07b65d9d-b1a2-416e-bd04-0b61b775f924
> +  MODULE_TYPE                    = UEFI_APPLICATION
> +  VERSION_STRING                 = 0.1
> +  ENTRY_POINT                    = ShellCEntryLib
> +
> +#
> +#  VALID_ARCHITECTURES           = AARCH64
> +#
> +
> +[Sources]
> +  StyxFlashImage.S
> +  StyxFlashUefi.c
> +
> +[Packages]
> +  AmdModulePkg/AmdModulePkg.dec
> +  ArmPkg/ArmPkg.dec
> +  MdePkg/MdePkg.dec
> +  ShellPkg/ShellPkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  UefiBootServicesTableLib
> +  UefiLib
> +  ShellCEntryLib
> +
> +[Protocols]
> +  gAmdIscpDxeProtocolGuid
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +
> +[BuildOptions]
> +  *_*_*_CC_FLAGS = -mcmodel=small
> +  *_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -Wl,-T,$(MODULE_DIR)/Scripts/GccBase.lds
> +  *_*_*_PLATFORM_FLAGS = -I$(BIN_DIR)/../FV

Ah, _that's_ how you find STYX_EFI.Fv? :)

> diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> index 091914c047a3..90cda24ae49d 100644
> --- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> +++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> @@ -17,8 +17,9 @@
>  ################################################################################
>  [Defines]
>  
> -DEFINE NUM_CORES = 4
> -DEFINE DO_KCS    = 0
> +DEFINE NUM_CORES    = 4
> +DEFINE DO_KCS       = 0
> +DEFINE DO_FLASHER   = FALSE
>  
>    PLATFORM_NAME                  = Cello
>    PLATFORM_GUID                  = 77861b3e-74b0-4ff3-8d18-c5ba5803e1bf
> @@ -690,3 +691,10 @@ DEFINE DO_KCS    = 0
>  !ifdef $(RENESAS_XHCI_FW_DIR)
>    OpenPlatformPkg/Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.inf
>  !endif
> +
> +!if $(DO_FLASHER) == TRUE
> +  OpenPlatformPkg/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf {
> +    <LibraryClasses>
> +      ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
> +  }
> +!endif
> diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> index 57d1425b2c8f..5b7d7f4a7b4a 100644
> --- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> +++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> @@ -21,6 +21,7 @@ DEFINE NUM_CORES    = 4
>  DEFINE DO_PSCI      = 1
>  DEFINE DO_ISCP      = 1
>  DEFINE DO_KCS       = 1
> +DEFINE DO_FLASHER   = FALSE
>  
>    PLATFORM_NAME                  = Overdrive1000
>    PLATFORM_GUID                  = 36774DD7-20DE-4C5B-8722-f8861DFF1F16
> @@ -701,3 +702,10 @@ DEFINE DO_KCS       = 1
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +
> +!if $(DO_FLASHER) == TRUE
> +  OpenPlatformPkg/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf {
> +    <LibraryClasses>
> +      ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
> +  }
> +!endif
> diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> index 6c284fb3b7db..662a15a9ccea 100644
> --- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> @@ -22,6 +22,7 @@ DEFINE NUM_CORES    = 8
>  DEFINE DO_PSCI      = 1
>  DEFINE DO_ISCP      = 1
>  DEFINE DO_KCS       = 1
> +DEFINE DO_FLASHER   = FALSE
>  
>    PLATFORM_NAME                  = Overdrive
>    PLATFORM_GUID                  = B2296C02-9DA1-4CD1-BD48-4D4F0F1276EB
> @@ -752,3 +753,10 @@ DEFINE DO_KCS       = 1
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
> +
> +!if $(DO_FLASHER) == TRUE
> +  OpenPlatformPkg/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf {
> +    <LibraryClasses>
> +      ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
> +  }
> +!endif
> -- 
> 2.9.3
>

On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
>> This implements a UEFI Shell application that updates the EFI NOR
>
> standalone UEFI Shell application
>
>> partition with the contents of STYX_EFI.Fv. Note that this means that
>> a) EFI variables are preserved, and
>> b) this updater can only be used on systems that have already been flashed
>>    with a firmware build based on this open source branch (and not on systems
>>    still running AMI firmware)
>>
>> In order for the flasher to have access to the various PCDs that describe
>> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
>> platform, but after STYX_EFI.Fv has been generated, which results in a
>> chicken-and-egg situation. Therefore, the recommended way of generating
>> the flasher is
>>
>> 1) delete the Build/<platform> directory entirely
>> 2) build the platform
>> 3) build the platform again, but with -D DO_FLASHER=TRUE appended
>
> Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
> platform?
>

Yes, that should be sufficient. I will add that.

>> The flasher application is called 'StyxFlashUefi'
>
> Only style comments below, but one silly question before that:
> Is there some particular reason why the application embeds the FV
> instead of reading it from a filesystem? Or just that you figured it's
> small enough that it doesn't matter?
>

Two reasons:
- simplicity; the actual code is *very* simply due to the fact that it
can refer to the payload directly, and I am not a seasoned shell
application hacker that has a clue how one would go about implementing
it using file I/O etc
- a generic flasher needs metadata that describes where the payload
needs to go, and the .Fv file does not contain that, so it would
involve a file format or a command line option with all the associated
failure modes.

>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
>>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
>>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
>>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
>>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
>>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
>>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
>>  7 files changed, 286 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds b/Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds
>> new file mode 100644
>> index 000000000000..7a0c87c6e32b
>> --- /dev/null
>> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds
>> @@ -0,0 +1,86 @@
>> +/** @file
>> +
>> +  Unified linker script for GCC based builds
>> +
>> +  Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2015 - 2017, Linaro Ltd. All rights reserved.<BR>
>> +  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>> +
>> +  This program and the accompanying materials are licensed and made available under
>> +  the terms and conditions of the BSD License that 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.
>> +
>> +**/
>> +
>> +SECTIONS {
>> +
>> +  /*
>> +   * The PE/COFF binary consists of DOS and PE/COFF headers, and a sequence of
>> +   * section headers adding up to PECOFF_HEADER_SIZE bytes (which differs
>> +   * between 32-bit and 64-bit builds). The actual start of the .text section
>> +   * will be rounded up based on its actual alignment.
>> +   */
>> +  . = PECOFF_HEADER_SIZE;
>> +
>> +  .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
>> +    *(.text .text.* .stub .gnu.linkonce.t.*)
>> +    *(.rodata .rodata.* .gnu.linkonce.r.*)
>> +    *(.got .got.*)
>> +
>> +    /*
>> +     * The contents of AutoGen.c files are mostly constant from the POV of the
>> +     * program, but most of it ends up in .data or .bss by default since few of
>> +     * the variable definitions that get emitted are declared as CONST.
>> +     * Unfortunately, we cannot pull it into the .text section entirely, since
>> +     * patchable PCDs are also emitted here, but we can at least move all of the
>> +     * emitted GUIDs here.
>> +     */
>> +    *:AutoGen.obj(.data.g*Guid)
>> +  }
>> +
>> +  /*
>> +   * The alignment of the .data section should be less than or equal to the
>> +   * alignment of the .text section. This ensures that the relative offset
>> +   * between these sections is the same in the ELF and the PE/COFF versions of
>> +   * this binary.
>> +   */
>> +  .data ALIGN(ALIGNOF(.text)) : ALIGN(CONSTANT(COMMONPAGESIZE)) {
>> +    *(.data .data.* .gnu.linkonce.d.*)
>> +    *(.bss .bss.*)
>> +    *(.payload)
>> +  }
>> +
>> +  .eh_frame ALIGN(CONSTANT(COMMONPAGESIZE)) : {
>> +    KEEP (*(.eh_frame))
>> +  }
>> +
>> +  .rela (INFO) : {
>> +    *(.rela .rela.*)
>> +  }
>> +
>> +  .hii : ALIGN(CONSTANT(COMMONPAGESIZE)) {
>> +    KEEP (*(.hii))
>> +  }
>> +
>> +  /*
>> +   * Retain the GNU build id but in a non-allocatable section so GenFw
>> +   * does not copy it into the PE/COFF image.
>> +   */
>> +  .build-id (INFO) : { *(.note.gnu.build-id) }
>> +
>> +  /DISCARD/ : {
>> +    *(.note.GNU-stack)
>> +    *(.gnu_debuglink)
>> +    *(.interp)
>> +    *(.dynsym)
>> +    *(.dynstr)
>> +    *(.dynamic)
>> +    *(.hash .gnu.hash)
>> +    *(.comment)
>> +    *(COMMON)
>> +  }
>> +}
>> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S
>> new file mode 100644
>> index 000000000000..041339ee9b47
>> --- /dev/null
>> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S
>> @@ -0,0 +1,25 @@
>> +/** @file
>> +
>> +  Copyright (c) 2017, Linaro Ltd. 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.
>> +
>> +**/
>> +
>> +  .section  ".payload"
>> +  .align    12
>> +
>> +ASM_GLOBAL ASM_PFX(StyxFlashImageStart)
>> +ASM_PFX(StyxFlashImageStart):
>> +  .incbin   "STYX_EFI.Fv"
>> +
>> +  .align    2
>> +ASM_GLOBAL ASM_PFX(StyxFlashImageSize)
>> +ASM_PFX(StyxFlashImageSize):
>> +  .long    . - ASM_PFX(StyxFlashImageStart)
>> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c
>> new file mode 100644
>> index 000000000000..a7bdbffab891
>> --- /dev/null
>> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c
>> @@ -0,0 +1,96 @@
>> +/** @file
>> +
>> +  Copyright (c) 2017, Linaro Ltd. 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.
>> +
>> +**/
>> +
>> +#include  <Uefi.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include  <Library/UefiBootServicesTableLib.h>
>> +#include  <Library/UefiLib.h>
>> +#include  <Library/ShellCEntryLib.h>
>
> Inconsistent whitespace. Not sorted.
>

Will fix

>> +
>> +#include <Protocol/AmdIscpDxeProtocol.h>
>> +
>> +#define UEFI_IMAGE_OFFSET     FixedPcdGet64 (PcdFvBaseAddress) - FixedPcdGet64 (PcdFdBaseAddress)
>> +#define BLOCK_SIZE            SIZE_64KB
>> +
>> +STATIC AMD_ISCP_DXE_PROTOCOL    *mIscpDxeProtocol;
>> +STATIC UINT8                    Buffer[BLOCK_SIZE];
>> +
>> +extern CONST UINT8    StyxFlashImageStart[];
>> +extern CONST UINT32   StyxFlashImageSize;
>> +
>> +/***
>> +  Main entrypoint
>> +
>> +  Establishes the main structure of the application.
>> +
>> +  @retval  0         The application exited normally.
>> +  @retval  Other     An error occurred.
>> +***/
>> +INTN
>> +EFIAPI
>> +ShellAppMain (
>> +  IN UINTN Argc,
>> +  IN CHAR16 **Argv
>> +  )
>> +{
>> +  EFI_STATUS      Status;
>> +  UINTN           Index;
>> +  INTN            Remaining;
>> +
>> +  Print(L"StyxFlashUefi: firmware updater for AMD Seattle based boards.\n");
>
> Space after "Print".
> (Actually, this seems consistent on all Print statement
> in this function, and nothing else.)
>

Not sure how that happened :-) Will fix.

>> +
>> +  Status = gBS->LocateProtocol (&gAmdIscpDxeProtocolGuid, NULL,
>> +                  (VOID **)&mIscpDxeProtocol);
>> +  if (EFI_ERROR (Status)) {
>> +    Print(L"Failed to locate ISCP communication protocol, terminating...\n");
>> +    return (INTN)Status;
>> +  }
>> +
>> +  Index = 0;
>> +  Remaining = StyxFlashImageSize;
>> +  do {
>> +    Status = mIscpDxeProtocol->AmdExecuteEraseFvBlockDxe (
>> +                                 mIscpDxeProtocol,
>> +                                 UEFI_IMAGE_OFFSET + Index * BLOCK_SIZE,
>> +                                 BLOCK_SIZE);
>> +    if (EFI_ERROR (Status)) {
>> +      Print(L"Erase failed!\n");
>> +      return (INTN)Status;
>> +    }
>> +
>> +    CopyMem (Buffer, StyxFlashImageStart + Index * BLOCK_SIZE,
>> +      MIN (Remaining, BLOCK_SIZE));
>> +
>> +    Status = mIscpDxeProtocol->AmdExecuteUpdateFvBlockDxe (
>> +                                 mIscpDxeProtocol,
>> +                                 UEFI_IMAGE_OFFSET + Index * BLOCK_SIZE,
>> +                                 Buffer,
>> +                                 MIN (Remaining, BLOCK_SIZE));
>> +
>> +    if (EFI_ERROR (Status)) {
>> +      Print(L"Update failed!\n");
>> +      return (INTN)Status;
>> +    }
>> +
>> +    Remaining -= BLOCK_SIZE;
>> +    Index++;
>> +
>> +    Print(L"Block %d of %d updated\n", Index, StyxFlashImageSize / BLOCK_SIZE);
>> +
>> +  } while (Remaining > 0);
>> +
>> +  Print(L"\nDone!\n");
>> +
>> +  return 0;
>> +}
>> diff --git a/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf
>> new file mode 100644
>> index 000000000000..2ad7c1b98ef1
>> --- /dev/null
>> +++ b/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf
>> @@ -0,0 +1,53 @@
>> +#/** @file
>> +#
>> +#  Copyright (c) 2017, Linaro Ltd. 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.
>> +#
>> +#**/
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010006
>
> Not 1.25?
>

Copy/paste from HelloWorld or some other example app I used. Will fix.

>> +  BASE_NAME                      = StyxFlashUefi
>> +  FILE_GUID                      = 07b65d9d-b1a2-416e-bd04-0b61b775f924
>> +  MODULE_TYPE                    = UEFI_APPLICATION
>> +  VERSION_STRING                 = 0.1
>> +  ENTRY_POINT                    = ShellCEntryLib
>> +
>> +#
>> +#  VALID_ARCHITECTURES           = AARCH64
>> +#
>> +
>> +[Sources]
>> +  StyxFlashImage.S
>> +  StyxFlashUefi.c
>> +
>> +[Packages]
>> +  AmdModulePkg/AmdModulePkg.dec
>> +  ArmPkg/ArmPkg.dec
>> +  MdePkg/MdePkg.dec
>> +  ShellPkg/ShellPkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseMemoryLib
>> +  UefiBootServicesTableLib
>> +  UefiLib
>> +  ShellCEntryLib
>> +
>> +[Protocols]
>> +  gAmdIscpDxeProtocolGuid
>> +
>> +[FixedPcd]
>> +  gArmTokenSpaceGuid.PcdFdBaseAddress
>> +  gArmTokenSpaceGuid.PcdFvBaseAddress
>> +
>> +[BuildOptions]
>> +  *_*_*_CC_FLAGS = -mcmodel=small
>> +  *_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -Wl,-T,$(MODULE_DIR)/Scripts/GccBase.lds
>> +  *_*_*_PLATFORM_FLAGS = -I$(BIN_DIR)/../FV
>
> Ah, _that's_ how you find STYX_EFI.Fv? :)
>

Yes. Keepin' it simple ...

>> diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> index 091914c047a3..90cda24ae49d 100644
>> --- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> +++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> @@ -17,8 +17,9 @@
>>  ################################################################################
>>  [Defines]
>>
>> -DEFINE NUM_CORES = 4
>> -DEFINE DO_KCS    = 0
>> +DEFINE NUM_CORES    = 4
>> +DEFINE DO_KCS       = 0
>> +DEFINE DO_FLASHER   = FALSE
>>
>>    PLATFORM_NAME                  = Cello
>>    PLATFORM_GUID                  = 77861b3e-74b0-4ff3-8d18-c5ba5803e1bf
>> @@ -690,3 +691,10 @@ DEFINE DO_KCS    = 0
>>  !ifdef $(RENESAS_XHCI_FW_DIR)
>>    OpenPlatformPkg/Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.inf
>>  !endif
>> +
>> +!if $(DO_FLASHER) == TRUE
>> +  OpenPlatformPkg/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf {
>> +    <LibraryClasses>
>> +      ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
>> +  }
>> +!endif
>> diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> index 57d1425b2c8f..5b7d7f4a7b4a 100644
>> --- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> +++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> @@ -21,6 +21,7 @@ DEFINE NUM_CORES    = 4
>>  DEFINE DO_PSCI      = 1
>>  DEFINE DO_ISCP      = 1
>>  DEFINE DO_KCS       = 1
>> +DEFINE DO_FLASHER   = FALSE
>>
>>    PLATFORM_NAME                  = Overdrive1000
>>    PLATFORM_GUID                  = 36774DD7-20DE-4C5B-8722-f8861DFF1F16
>> @@ -701,3 +702,10 @@ DEFINE DO_KCS       = 1
>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>>    }
>> +
>> +!if $(DO_FLASHER) == TRUE
>> +  OpenPlatformPkg/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf {
>> +    <LibraryClasses>
>> +      ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
>> +  }
>> +!endif
>> diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> index 6c284fb3b7db..662a15a9ccea 100644
>> --- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> +++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> @@ -22,6 +22,7 @@ DEFINE NUM_CORES    = 8
>>  DEFINE DO_PSCI      = 1
>>  DEFINE DO_ISCP      = 1
>>  DEFINE DO_KCS       = 1
>> +DEFINE DO_FLASHER   = FALSE
>>
>>    PLATFORM_NAME                  = Overdrive
>>    PLATFORM_GUID                  = B2296C02-9DA1-4CD1-BD48-4D4F0F1276EB
>> @@ -752,3 +753,10 @@ DEFINE DO_KCS       = 1
>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>>    }
>> +
>> +!if $(DO_FLASHER) == TRUE
>> +  OpenPlatformPkg/Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf {
>> +    <LibraryClasses>
>> +      ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
>> +  }
>> +!endif
>> --
>> 2.9.3
>>

On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
> >> This implements a UEFI Shell application that updates the EFI NOR
> >
> > standalone UEFI Shell application
> >
> >> partition with the contents of STYX_EFI.Fv. Note that this means that
> >> a) EFI variables are preserved, and
> >> b) this updater can only be used on systems that have already been flashed
> >>    with a firmware build based on this open source branch (and not on systems
> >>    still running AMI firmware)

Request for clarification based on your explanation below:
Presumably b) is for one or both of these reasons:
1) AMI firmware will not produce all protocols needed for this
   application to run (just like this firmware will not produce all
   protocols the AMI updater needs)?
2) This application does not zero out the variable area?

> >> In order for the flasher to have access to the various PCDs that describe
> >> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
> >> platform, but after STYX_EFI.Fv has been generated, which results in a
> >> chicken-and-egg situation. Therefore, the recommended way of generating
> >> the flasher is
> >>
> >> 1) delete the Build/<platform> directory entirely
> >> 2) build the platform
> >> 3) build the platform again, but with -D DO_FLASHER=TRUE appended
> >
> > Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
> > platform?
> >
> 
> Yes, that should be sufficient. I will add that.
> 
> >> The flasher application is called 'StyxFlashUefi'
> >
> > Only style comments below, but one silly question before that:
> > Is there some particular reason why the application embeds the FV
> > instead of reading it from a filesystem? Or just that you figured it's
> > small enough that it doesn't matter?
> >
> 
> Two reasons:
> - simplicity; the actual code is *very* simply due to the fact that it
> can refer to the payload directly, and I am not a seasoned shell
> application hacker that has a clue how one would go about implementing
> it using file I/O etc
> - a generic flasher needs metadata that describes where the payload
> needs to go, and the .Fv file does not contain that, so it would
> involve a file format or a command line option with all the associated
> failure modes.

Right, so basically because PcdFvBaseAddress and PcdFdBaseAddress
could change? But would such a change not mean that variables would
not be preserved after all?

> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
> >>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
> >>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
> >>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
> >>  7 files changed, 286 insertions(+), 2 deletions(-)

On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
>> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
>> >> This implements a UEFI Shell application that updates the EFI NOR
>> >
>> > standalone UEFI Shell application
>> >
>> >> partition with the contents of STYX_EFI.Fv. Note that this means that
>> >> a) EFI variables are preserved, and
>> >> b) this updater can only be used on systems that have already been flashed
>> >>    with a firmware build based on this open source branch (and not on systems
>> >>    still running AMI firmware)
>
> Request for clarification based on your explanation below:
> Presumably b) is for one or both of these reasons:
> 1) AMI firmware will not produce all protocols needed for this
>    application to run (just like this firmware will not produce all
>    protocols the AMI updater needs)?
> 2) This application does not zero out the variable area?
>

The AMI firmware is also based on SeattleFDK, so it does produce the
ISCP protocol that exposes the NOR read/write/erase methods. This is
what makes it dangerous, given that the flasher will most likely run
happily, but will not leave the NOR in a state where a varstore FV
header appears in the expected place. This is what will make the
firmware crash.

Note that none of these issues are particularly difficult to deal
with, but it increases the complexity of this tools, which is not
intended as a production quality firmware update distribution method
but simply to allow me to distribute firmware updates to people like
Lorenzo and Marc, and now some Cello users as well which shouldn't
have been shipped boards to begin with.


>> >> In order for the flasher to have access to the various PCDs that describe
>> >> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
>> >> platform, but after STYX_EFI.Fv has been generated, which results in a
>> >> chicken-and-egg situation. Therefore, the recommended way of generating
>> >> the flasher is
>> >>
>> >> 1) delete the Build/<platform> directory entirely
>> >> 2) build the platform
>> >> 3) build the platform again, but with -D DO_FLASHER=TRUE appended
>> >
>> > Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
>> > platform?
>> >
>>
>> Yes, that should be sufficient. I will add that.
>>
>> >> The flasher application is called 'StyxFlashUefi'
>> >
>> > Only style comments below, but one silly question before that:
>> > Is there some particular reason why the application embeds the FV
>> > instead of reading it from a filesystem? Or just that you figured it's
>> > small enough that it doesn't matter?
>> >
>>
>> Two reasons:
>> - simplicity; the actual code is *very* simply due to the fact that it
>> can refer to the payload directly, and I am not a seasoned shell
>> application hacker that has a clue how one would go about implementing
>> it using file I/O etc
>> - a generic flasher needs metadata that describes where the payload
>> needs to go, and the .Fv file does not contain that, so it would
>> involve a file format or a command line option with all the associated
>> failure modes.
>
> Right, so basically because PcdFvBaseAddress and PcdFdBaseAddress
> could change? But would such a change not mean that variables would
> not be preserved after all?
>

Not just because they could change, but because their value is set in
the .FDF, and so any attempt to obtain the values elsewhere would
involve adding a redundant definition, which I would like to avoid.


>> >> Contributed-under: TianoCore Contribution Agreement 1.0
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
>> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
>> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
>> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
>> >>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
>> >>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
>> >>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
>> >>  7 files changed, 286 insertions(+), 2 deletions(-)

On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
> On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
> >> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
> >> >> This implements a UEFI Shell application that updates the EFI NOR
> >> >
> >> > standalone UEFI Shell application
> >> >
> >> >> partition with the contents of STYX_EFI.Fv. Note that this means that
> >> >> a) EFI variables are preserved, and
> >> >> b) this updater can only be used on systems that have already been flashed
> >> >>    with a firmware build based on this open source branch (and not on systems
> >> >>    still running AMI firmware)
> >
> > Request for clarification based on your explanation below:
> > Presumably b) is for one or both of these reasons:
> > 1) AMI firmware will not produce all protocols needed for this
> >    application to run (just like this firmware will not produce all
> >    protocols the AMI updater needs)?
> > 2) This application does not zero out the variable area?
> >
> 
> The AMI firmware is also based on SeattleFDK, so it does produce the
> ISCP protocol that exposes the NOR read/write/erase methods. This is
> what makes it dangerous, given that the flasher will most likely run
> happily, but will not leave the NOR in a state where a varstore FV
> header appears in the expected place. This is what will make the
> firmware crash.
> 
> Note that none of these issues are particularly difficult to deal
> with, but it increases the complexity of this tools, which is not
> intended as a production quality firmware update distribution method
> but simply to allow me to distribute firmware updates to people like
> Lorenzo and Marc, and now some Cello users as well which shouldn't
> have been shipped boards to begin with.

All I'm really getting at with these questions around a very much
welcome new tool is that I have a feeling it isn't necessarily
end-user proof at this stage, so I think I would like to see either a
few warnings in this commit message (which currently reads more like
it's saying more along the lines of "it will only work if" than "you
will become a customer of Dediprog if you try") or some attempt to
determine whether we are on a platform where it is safe to attempt an
upgrade.
 
> >> >> In order for the flasher to have access to the various PCDs that describe
> >> >> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
> >> >> platform, but after STYX_EFI.Fv has been generated, which results in a
> >> >> chicken-and-egg situation. Therefore, the recommended way of generating
> >> >> the flasher is
> >> >>
> >> >> 1) delete the Build/<platform> directory entirely
> >> >> 2) build the platform
> >> >> 3) build the platform again, but with -D DO_FLASHER=TRUE appended
> >> >
> >> > Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
> >> > platform?
> >> >
> >>
> >> Yes, that should be sufficient. I will add that.
> >>
> >> >> The flasher application is called 'StyxFlashUefi'
> >> >
> >> > Only style comments below, but one silly question before that:
> >> > Is there some particular reason why the application embeds the FV
> >> > instead of reading it from a filesystem? Or just that you figured it's
> >> > small enough that it doesn't matter?
> >> >
> >>
> >> Two reasons:
> >> - simplicity; the actual code is *very* simply due to the fact that it
> >> can refer to the payload directly, and I am not a seasoned shell
> >> application hacker that has a clue how one would go about implementing
> >> it using file I/O etc
> >> - a generic flasher needs metadata that describes where the payload
> >> needs to go, and the .Fv file does not contain that, so it would
> >> involve a file format or a command line option with all the associated
> >> failure modes.
> >
> > Right, so basically because PcdFvBaseAddress and PcdFdBaseAddress
> > could change? But would such a change not mean that variables would
> > not be preserved after all?
> 
> Not just because they could change, but because their value is set in
> the .FDF, and so any attempt to obtain the values elsewhere would
> involve adding a redundant definition, which I would like to avoid.

Oh, indeed.

> >> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> ---
> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
> >> >>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
> >> >>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
> >> >>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
> >> >>  7 files changed, 286 insertions(+), 2 deletions(-)

On 27 June 2017 at 17:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
>> On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
>> >> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> >> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
>> >> >> This implements a UEFI Shell application that updates the EFI NOR
>> >> >
>> >> > standalone UEFI Shell application
>> >> >
>> >> >> partition with the contents of STYX_EFI.Fv. Note that this means that
>> >> >> a) EFI variables are preserved, and
>> >> >> b) this updater can only be used on systems that have already been flashed
>> >> >>    with a firmware build based on this open source branch (and not on systems
>> >> >>    still running AMI firmware)
>> >
>> > Request for clarification based on your explanation below:
>> > Presumably b) is for one or both of these reasons:
>> > 1) AMI firmware will not produce all protocols needed for this
>> >    application to run (just like this firmware will not produce all
>> >    protocols the AMI updater needs)?
>> > 2) This application does not zero out the variable area?
>> >
>>
>> The AMI firmware is also based on SeattleFDK, so it does produce the
>> ISCP protocol that exposes the NOR read/write/erase methods. This is
>> what makes it dangerous, given that the flasher will most likely run
>> happily, but will not leave the NOR in a state where a varstore FV
>> header appears in the expected place. This is what will make the
>> firmware crash.
>>
>> Note that none of these issues are particularly difficult to deal
>> with, but it increases the complexity of this tools, which is not
>> intended as a production quality firmware update distribution method
>> but simply to allow me to distribute firmware updates to people like
>> Lorenzo and Marc, and now some Cello users as well which shouldn't
>> have been shipped boards to begin with.
>
> All I'm really getting at with these questions around a very much
> welcome new tool is that I have a feeling it isn't necessarily
> end-user proof at this stage, so I think I would like to see either a
> few warnings in this commit message (which currently reads more like
> it's saying more along the lines of "it will only work if" than "you
> will become a customer of Dediprog if you try") or some attempt to
> determine whether we are on a platform where it is safe to attempt an
> upgrade.
>

Oh, this is absolutely unsuitable for general use. For instance, it
does not distinguish between Cello, Overdrive and Overdrive1000, nor
does it perform any kind of verification after the flash, all of which
are not that difficult to implement, but simply not things that I can
be bothered to care about at the moment, given the sorry state of
Cello, and the fact that the only three users of this firmware for
Overdrive are one floor down from where your desk is.

So for now, it is either merging this code, with perhaps some
discouraging capitalized scary words added to the commit message, or
just disregarding the patch for now, in the hope that someone else
(Alan perhaps?) can be bothered to pick it up later and polish it up a
bit. I honestly don't care either way.



>> >> >> In order for the flasher to have access to the various PCDs that describe
>> >> >> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
>> >> >> platform, but after STYX_EFI.Fv has been generated, which results in a
>> >> >> chicken-and-egg situation. Therefore, the recommended way of generating
>> >> >> the flasher is
>> >> >>
>> >> >> 1) delete the Build/<platform> directory entirely
>> >> >> 2) build the platform
>> >> >> 3) build the platform again, but with -D DO_FLASHER=TRUE appended
>> >> >
>> >> > Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
>> >> > platform?
>> >> >
>> >>
>> >> Yes, that should be sufficient. I will add that.
>> >>
>> >> >> The flasher application is called 'StyxFlashUefi'
>> >> >
>> >> > Only style comments below, but one silly question before that:
>> >> > Is there some particular reason why the application embeds the FV
>> >> > instead of reading it from a filesystem? Or just that you figured it's
>> >> > small enough that it doesn't matter?
>> >> >
>> >>
>> >> Two reasons:
>> >> - simplicity; the actual code is *very* simply due to the fact that it
>> >> can refer to the payload directly, and I am not a seasoned shell
>> >> application hacker that has a clue how one would go about implementing
>> >> it using file I/O etc
>> >> - a generic flasher needs metadata that describes where the payload
>> >> needs to go, and the .Fv file does not contain that, so it would
>> >> involve a file format or a command line option with all the associated
>> >> failure modes.
>> >
>> > Right, so basically because PcdFvBaseAddress and PcdFdBaseAddress
>> > could change? But would such a change not mean that variables would
>> > not be preserved after all?
>>
>> Not just because they could change, but because their value is set in
>> the .FDF, and so any attempt to obtain the values elsewhere would
>> involve adding a redundant definition, which I would like to avoid.
>
> Oh, indeed.
>
>> >> >> Contributed-under: TianoCore Contribution Agreement 1.0
>> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> >> ---
>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
>> >> >>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
>> >> >>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
>> >> >>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
>> >> >>  7 files changed, 286 insertions(+), 2 deletions(-)

On Tue, Jun 27, 2017 at 10:56 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 27 June 2017 at 17:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
>>> On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> > On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
>>> >> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> >> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
>>> >> >> This implements a UEFI Shell application that updates the EFI NOR
>>> >> >
>>> >> > standalone UEFI Shell application
>>> >> >
>>> >> >> partition with the contents of STYX_EFI.Fv. Note that this means that
>>> >> >> a) EFI variables are preserved, and
>>> >> >> b) this updater can only be used on systems that have already been flashed
>>> >> >>    with a firmware build based on this open source branch (and not on systems
>>> >> >>    still running AMI firmware)
>>> >
>>> > Request for clarification based on your explanation below:
>>> > Presumably b) is for one or both of these reasons:
>>> > 1) AMI firmware will not produce all protocols needed for this
>>> >    application to run (just like this firmware will not produce all
>>> >    protocols the AMI updater needs)?
>>> > 2) This application does not zero out the variable area?
>>> >
>>>
>>> The AMI firmware is also based on SeattleFDK, so it does produce the
>>> ISCP protocol that exposes the NOR read/write/erase methods. This is
>>> what makes it dangerous, given that the flasher will most likely run
>>> happily, but will not leave the NOR in a state where a varstore FV
>>> header appears in the expected place. This is what will make the
>>> firmware crash.
>>>
>>> Note that none of these issues are particularly difficult to deal
>>> with, but it increases the complexity of this tools, which is not
>>> intended as a production quality firmware update distribution method
>>> but simply to allow me to distribute firmware updates to people like
>>> Lorenzo and Marc, and now some Cello users as well which shouldn't
>>> have been shipped boards to begin with.
>>
>> All I'm really getting at with these questions around a very much
>> welcome new tool is that I have a feeling it isn't necessarily
>> end-user proof at this stage, so I think I would like to see either a
>> few warnings in this commit message (which currently reads more like
>> it's saying more along the lines of "it will only work if" than "you
>> will become a customer of Dediprog if you try") or some attempt to
>> determine whether we are on a platform where it is safe to attempt an
>> upgrade.
>>
>
> Oh, this is absolutely unsuitable for general use. For instance, it
> does not distinguish between Cello, Overdrive and Overdrive1000, nor
> does it perform any kind of verification after the flash, all of which
> are not that difficult to implement, but simply not things that I can
> be bothered to care about at the moment, given the sorry state of
> Cello, and the fact that the only three users of this firmware for
> Overdrive are one floor down from where your desk is.
>
> So for now, it is either merging this code, with perhaps some
> discouraging capitalized scary words added to the commit message, or
> just disregarding the patch for now, in the hope that someone else
> (Alan perhaps?) can be bothered to pick it up later and polish it up a
> bit. I honestly don't care either way.
>
>

I'll give this a try.  I can use a Bus Pirate to program the flash,
but it's slow
and I don't have a proper cable, so I have to connect the wires individually.
I'd think merging with warnings would be useful.  It won't be built by
default, so
users would have to deliberately use this.

Roy

>
>>> >> >> In order for the flasher to have access to the various PCDs that describe
>>> >> >> where STYX_EFI.Fv lives in the NOR, it needs to be built as part of the
>>> >> >> platform, but after STYX_EFI.Fv has been generated, which results in a
>>> >> >> chicken-and-egg situation. Therefore, the recommended way of generating
>>> >> >> the flasher is
>>> >> >>
>>> >> >> 1) delete the Build/<platform> directory entirely
>>> >> >> 2) build the platform
>>> >> >> 3) build the platform again, but with -D DO_FLASHER=TRUE appended
>>> >> >
>>> >> > Build with -m .../StyxFlashUefi.inf instead of rebuilding the whole
>>> >> > platform?
>>> >> >
>>> >>
>>> >> Yes, that should be sufficient. I will add that.
>>> >>
>>> >> >> The flasher application is called 'StyxFlashUefi'
>>> >> >
>>> >> > Only style comments below, but one silly question before that:
>>> >> > Is there some particular reason why the application embeds the FV
>>> >> > instead of reading it from a filesystem? Or just that you figured it's
>>> >> > small enough that it doesn't matter?
>>> >> >
>>> >>
>>> >> Two reasons:
>>> >> - simplicity; the actual code is *very* simply due to the fact that it
>>> >> can refer to the payload directly, and I am not a seasoned shell
>>> >> application hacker that has a clue how one would go about implementing
>>> >> it using file I/O etc
>>> >> - a generic flasher needs metadata that describes where the payload
>>> >> needs to go, and the .Fv file does not contain that, so it would
>>> >> involve a file format or a command line option with all the associated
>>> >> failure modes.
>>> >
>>> > Right, so basically because PcdFvBaseAddress and PcdFdBaseAddress
>>> > could change? But would such a change not mean that variables would
>>> > not be preserved after all?
>>>
>>> Not just because they could change, but because their value is set in
>>> the .FDF, and so any attempt to obtain the values elsewhere would
>>> involve adding a redundant definition, which I would like to avoid.
>>
>> Oh, indeed.
>>
>>> >> >> Contributed-under: TianoCore Contribution Agreement 1.0
>>> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> >> >> ---
>>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/Scripts/GccBase.lds | 86 ++++++++++++++++++
>>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashImage.S    | 25 +++++
>>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.c     | 96 ++++++++++++++++++++
>>> >> >>  Platforms/AMD/Styx/Applications/StyxFlashUefi/StyxFlashUefi.inf   | 53 +++++++++++
>>> >> >>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                      | 12 ++-
>>> >> >>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc      |  8 ++
>>> >> >>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc              |  8 ++
>>> >> >>  7 files changed, 286 insertions(+), 2 deletions(-)

On Tue, Jun 27, 2017 at 05:56:35PM +0000, Ard Biesheuvel wrote:
> On 27 June 2017 at 17:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
> >> On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
> >> >> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> >> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
> >> >> >> This implements a UEFI Shell application that updates the EFI NOR
> >> >> >
> >> >> > standalone UEFI Shell application
> >> >> >
> >> >> >> partition with the contents of STYX_EFI.Fv. Note that this means that
> >> >> >> a) EFI variables are preserved, and
> >> >> >> b) this updater can only be used on systems that have already been flashed
> >> >> >>    with a firmware build based on this open source branch (and not on systems
> >> >> >>    still running AMI firmware)
> >> >
> >> > Request for clarification based on your explanation below:
> >> > Presumably b) is for one or both of these reasons:
> >> > 1) AMI firmware will not produce all protocols needed for this
> >> >    application to run (just like this firmware will not produce all
> >> >    protocols the AMI updater needs)?
> >> > 2) This application does not zero out the variable area?
> >> >
> >>
> >> The AMI firmware is also based on SeattleFDK, so it does produce the
> >> ISCP protocol that exposes the NOR read/write/erase methods. This is
> >> what makes it dangerous, given that the flasher will most likely run
> >> happily, but will not leave the NOR in a state where a varstore FV
> >> header appears in the expected place. This is what will make the
> >> firmware crash.
> >>
> >> Note that none of these issues are particularly difficult to deal
> >> with, but it increases the complexity of this tools, which is not
> >> intended as a production quality firmware update distribution method
> >> but simply to allow me to distribute firmware updates to people like
> >> Lorenzo and Marc, and now some Cello users as well which shouldn't
> >> have been shipped boards to begin with.
> >
> > All I'm really getting at with these questions around a very much
> > welcome new tool is that I have a feeling it isn't necessarily
> > end-user proof at this stage, so I think I would like to see either a
> > few warnings in this commit message (which currently reads more like
> > it's saying more along the lines of "it will only work if" than "you
> > will become a customer of Dediprog if you try") or some attempt to
> > determine whether we are on a platform where it is safe to attempt an
> > upgrade.
> 
> Oh, this is absolutely unsuitable for general use. For instance, it
> does not distinguish between Cello, Overdrive and Overdrive1000, nor
> does it perform any kind of verification after the flash, all of which
> are not that difficult to implement, but simply not things that I can
> be bothered to care about at the moment, given the sorry state of
> Cello, and the fact that the only three users of this firmware for
> Overdrive are one floor down from where your desk is.

Yeah, I get it.

> So for now, it is either merging this code, with perhaps some
> discouraging capitalized scary words added to the commit message, or
> just disregarding the patch for now, in the hope that someone else
> (Alan perhaps?) can be bothered to pick it up later and polish it up a
> bit. I honestly don't care either way.

I'm good with some capitalized scary words.

/
    Leif

On Tue, Jun 27, 2017 at 11:53 AM, Leif Lindholm
<leif.lindholm@linaro.org> wrote:
> On Tue, Jun 27, 2017 at 05:56:35PM +0000, Ard Biesheuvel wrote:
>> On 27 June 2017 at 17:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
>> >> On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> >> > On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
>> >> >> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> >> >> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
>> >> >> >> This implements a UEFI Shell application that updates the EFI NOR
>> >> >> >
>> >> >> > standalone UEFI Shell application
>> >> >> >
>> >> >> >> partition with the contents of STYX_EFI.Fv. Note that this means that
>> >> >> >> a) EFI variables are preserved, and
>> >> >> >> b) this updater can only be used on systems that have already been flashed
>> >> >> >>    with a firmware build based on this open source branch (and not on systems
>> >> >> >>    still running AMI firmware)
>> >> >
>> >> > Request for clarification based on your explanation below:
>> >> > Presumably b) is for one or both of these reasons:
>> >> > 1) AMI firmware will not produce all protocols needed for this
>> >> >    application to run (just like this firmware will not produce all
>> >> >    protocols the AMI updater needs)?
>> >> > 2) This application does not zero out the variable area?
>> >> >
>> >>
>> >> The AMI firmware is also based on SeattleFDK, so it does produce the
>> >> ISCP protocol that exposes the NOR read/write/erase methods. This is
>> >> what makes it dangerous, given that the flasher will most likely run
>> >> happily, but will not leave the NOR in a state where a varstore FV
>> >> header appears in the expected place. This is what will make the
>> >> firmware crash.
>> >>
>> >> Note that none of these issues are particularly difficult to deal
>> >> with, but it increases the complexity of this tools, which is not
>> >> intended as a production quality firmware update distribution method
>> >> but simply to allow me to distribute firmware updates to people like
>> >> Lorenzo and Marc, and now some Cello users as well which shouldn't
>> >> have been shipped boards to begin with.
>> >
>> > All I'm really getting at with these questions around a very much
>> > welcome new tool is that I have a feeling it isn't necessarily
>> > end-user proof at this stage, so I think I would like to see either a
>> > few warnings in this commit message (which currently reads more like
>> > it's saying more along the lines of "it will only work if" than "you
>> > will become a customer of Dediprog if you try") or some attempt to
>> > determine whether we are on a platform where it is safe to attempt an
>> > upgrade.
>>
>> Oh, this is absolutely unsuitable for general use. For instance, it
>> does not distinguish between Cello, Overdrive and Overdrive1000, nor
>> does it perform any kind of verification after the flash, all of which
>> are not that difficult to implement, but simply not things that I can
>> be bothered to care about at the moment, given the sorry state of
>> Cello, and the fact that the only three users of this firmware for
>> Overdrive are one floor down from where your desk is.
>
> Yeah, I get it.
>
>> So for now, it is either merging this code, with perhaps some
>> discouraging capitalized scary words added to the commit message, or
>> just disregarding the patch for now, in the hope that someone else
>> (Alan perhaps?) can be bothered to pick it up later and polish it up a
>> bit. I honestly don't care either way.
>
> I'm good with some capitalized scary words.
>
> /
>     Leif

You can add:
Tested-by: Roy Franz <roy.franz@cavium.com>

I did see some unexpected output in the early Linux boot, but that is
unrelated to this patch, as
I didn't apply other patches from this series, as some of them were
already on the master branch.
I built against edk2 51bf49ee53a289 (Current UDK2017 branch)  and
OpenPlatformPkg 8a854db1f5,
and I'm seeing the following:

EFI stub: Booting Linux Kernel...
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
[    0.403578] ssp-pl022 e1020000.ssp: probe: no chip select defined
[    0.409716] ssp-pl022 e1030000.ssp: Failed to work in dma mode, work without!

The boot continues, and seems to work fine, however the two ssp-pl022
messages are not
there on the firmware build that I got from Ard with the SATA
detection timeout fix.

Roy

On 27 June 2017 at 21:29, Roy Franz <rfranz@cavium.com> wrote:
> On Tue, Jun 27, 2017 at 11:53 AM, Leif Lindholm
> <leif.lindholm@linaro.org> wrote:
>> On Tue, Jun 27, 2017 at 05:56:35PM +0000, Ard Biesheuvel wrote:
>>> On 27 June 2017 at 17:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> > On Tue, Jun 27, 2017 at 05:24:30PM +0000, Ard Biesheuvel wrote:
>>> >> On 27 June 2017 at 17:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> >> > On Tue, Jun 27, 2017 at 04:39:47PM +0000, Ard Biesheuvel wrote:
>>> >> >> On 27 June 2017 at 16:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> >> >> > On Tue, Jun 27, 2017 at 01:21:45PM +0000, Ard Biesheuvel wrote:
>>> >> >> >> This implements a UEFI Shell application that updates the EFI NOR
>>> >> >> >
>>> >> >> > standalone UEFI Shell application
>>> >> >> >
>>> >> >> >> partition with the contents of STYX_EFI.Fv. Note that this means that
>>> >> >> >> a) EFI variables are preserved, and
>>> >> >> >> b) this updater can only be used on systems that have already been flashed
>>> >> >> >>    with a firmware build based on this open source branch (and not on systems
>>> >> >> >>    still running AMI firmware)
>>> >> >
>>> >> > Request for clarification based on your explanation below:
>>> >> > Presumably b) is for one or both of these reasons:
>>> >> > 1) AMI firmware will not produce all protocols needed for this
>>> >> >    application to run (just like this firmware will not produce all
>>> >> >    protocols the AMI updater needs)?
>>> >> > 2) This application does not zero out the variable area?
>>> >> >
>>> >>
>>> >> The AMI firmware is also based on SeattleFDK, so it does produce the
>>> >> ISCP protocol that exposes the NOR read/write/erase methods. This is
>>> >> what makes it dangerous, given that the flasher will most likely run
>>> >> happily, but will not leave the NOR in a state where a varstore FV
>>> >> header appears in the expected place. This is what will make the
>>> >> firmware crash.
>>> >>
>>> >> Note that none of these issues are particularly difficult to deal
>>> >> with, but it increases the complexity of this tools, which is not
>>> >> intended as a production quality firmware update distribution method
>>> >> but simply to allow me to distribute firmware updates to people like
>>> >> Lorenzo and Marc, and now some Cello users as well which shouldn't
>>> >> have been shipped boards to begin with.
>>> >
>>> > All I'm really getting at with these questions around a very much
>>> > welcome new tool is that I have a feeling it isn't necessarily
>>> > end-user proof at this stage, so I think I would like to see either a
>>> > few warnings in this commit message (which currently reads more like
>>> > it's saying more along the lines of "it will only work if" than "you
>>> > will become a customer of Dediprog if you try") or some attempt to
>>> > determine whether we are on a platform where it is safe to attempt an
>>> > upgrade.
>>>
>>> Oh, this is absolutely unsuitable for general use. For instance, it
>>> does not distinguish between Cello, Overdrive and Overdrive1000, nor
>>> does it perform any kind of verification after the flash, all of which
>>> are not that difficult to implement, but simply not things that I can
>>> be bothered to care about at the moment, given the sorry state of
>>> Cello, and the fact that the only three users of this firmware for
>>> Overdrive are one floor down from where your desk is.
>>
>> Yeah, I get it.
>>
>>> So for now, it is either merging this code, with perhaps some
>>> discouraging capitalized scary words added to the commit message, or
>>> just disregarding the patch for now, in the hope that someone else
>>> (Alan perhaps?) can be bothered to pick it up later and polish it up a
>>> bit. I honestly don't care either way.
>>
>> I'm good with some capitalized scary words.
>>
>> /
>>     Leif
>
> You can add:
> Tested-by: Roy Franz <roy.franz@cavium.com>
>

Thanks!

> I did see some unexpected output in the early Linux boot, but that is
> unrelated to this patch, as
> I didn't apply other patches from this series, as some of them were
> already on the master branch.
> I built against edk2 51bf49ee53a289 (Current UDK2017 branch)  and
> OpenPlatformPkg 8a854db1f5,
> and I'm seeing the following:
>
> EFI stub: Booting Linux Kernel...
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
> [    0.403578] ssp-pl022 e1020000.ssp: probe: no chip select defined
> [    0.409716] ssp-pl022 e1030000.ssp: Failed to work in dma mode, work without!
>
> The boot continues, and seems to work fine, however the two ssp-pl022
> messages are not
> there on the firmware build that I got from Ard with the SATA
> detection timeout fix.
>

This is because you are now booting in DT mode. The messages
themselves are harmless afaict.

You can switch back to ACPI in the UEFI setup menu (under Device
Manager) and it will stick