public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
       [not found]   ` <CAKv+Gu9KvJVGgORjxnLzuGVswimavRg8h1ZXKC+=Fv48RLJGEQ@mail.gmail.com>
@ 2016-11-16 16:13     ` Marcin Wojtas
  2016-11-16 17:05       ` Carsey, Jaben
  0 siblings, 1 reply; 7+ messages in thread
From: Marcin Wojtas @ 2016-11-16 16:13 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linaro-uefi, Leif Lindholm, Neta Zur Hershkovits, Yehuda Yitschak,
	Haim Boot, Jan Dąbroś, Bartosz Szczepanek,
	edk2-devel-01, Carsey, Jaben, ruiyu.ni

+ Jaben, Ruiyu and edk2 list

Hi Ard,

Internal Shell header ("../../../ShellPkg/Application/Shell/Shell.h")
was added ONLY for being able to execute shell commands (tftp and our
custom SPI flash control) from within a new command. This is now done
with:
RunShellCommand (TftpCmd, &Status);
And it works fine.

Replacing above with:
ShellExecute (&ImageHandle, TftpCmd, FALSE, NULL, &Status);
allow to get rid of relative include and seems more appropriate, but
there is completely no effect of calling it.

I checked in edk2 sources and see no relation between above problem
and registering our command with help of ShellDynamicCommand protocol.
Unless I don't know something. Anyway, I'll apreciate any hint, how to
solve the issue in a nice way.

Best regards,
Marcin

2016-11-15 17:09 GMT+01:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 10 July 2016 at 00:21, Marcin Wojtas <mw@semihalf.com> wrote:
>> From: Jan Dąbroś <jsd@semihalf.com>
>>
>> 'fupdate' command performs updating firmware from file placed on local
>> filesystem or on TFTP server.
>> It uses tftp and sf commands.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jan Dabros <jsd@semihalf.com>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>
> Please use the Shell dynamic command protocol to add commands to the
> shell. This will remove the need for including internal Shell headers,
> and even allow your commands to work with prebuilt Shell binaries.
>
> MdePkg/Include/Protocol/ShellDynamicCommand.h
>
> Thanks,
> Ard.
>
>
>> ---
>>  Applications/FirmwareUpdate/FUpdate.c   | 448 ++++++++++++++++++++++++++++++++
>>  Applications/FirmwareUpdate/FUpdate.inf |  68 +++++
>>  Applications/FirmwareUpdate/FUpdate.uni | Bin 0 -> 5838 bytes
>>  Platforms/Marvell/Marvell.dec           |   1 +
>>  4 files changed, 517 insertions(+)
>>  create mode 100644 Applications/FirmwareUpdate/FUpdate.c
>>  create mode 100644 Applications/FirmwareUpdate/FUpdate.inf
>>  create mode 100644 Applications/FirmwareUpdate/FUpdate.uni
>>
>> diff --git a/Applications/FirmwareUpdate/FUpdate.c b/Applications/FirmwareUpdate/FUpdate.c
>> new file mode 100644
>> index 0000000..359a4ac
>> --- /dev/null
>> +++ b/Applications/FirmwareUpdate/FUpdate.c
>> @@ -0,0 +1,448 @@
>> +/*******************************************************************************
>> +Copyright (C) 2016 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.
>> +
>> +*******************************************************************************/
>> +#include <Uefi.h>
>> +#include <ShellBase.h>
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/ShellCommandLib.h>
>> +#include <Library/ShellLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/PrintLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/ShellCEntryLib.h>
>> +#include <Library/HiiLib.h>
>> +#include <Guid/ShellLibHiiGuid.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/FileHandleLib.h>
>> +// FIXME: Remove including relative path
>> +#include "../../../ShellPkg/Application/Shell/Shell.h"
>> +
>> +#define CMD_NAME_STRING       L"fupdate"
>> +
>> +#define TFTP_CMD_STRING       L"tftp "
>> +#define SPACE_STRING          L" "
>> +#define SF_PROBE_CMD_STRING   L"sf probe"
>> +#define SF_WRITE_CMD_STRING   L"sf updatefile "
>> +#define SF_LOAD_ADDR_STRING   L"0x0"
>> +
>> +#define MAIN_HDR_MAGIC        0xB105B002
>> +
>> +typedef struct {
>> +  UINT32  Magic;              //  0-3
>> +  UINT32  PrologSize;         //  4-7
>> +  UINT32  PrologChecksum;     //  8-11
>> +  UINT32  BootImageSize;      // 12-15
>> +  UINT32  BootImageChecksum;  // 16-19
>> +  UINT32  Reserved0;          // 20-23
>> +  UINT32  LoadAddr;           // 24-27
>> +  UINT32  ExecAddr;           // 28-31
>> +  UINT8   UartConfig;         //  32
>> +  UINT8   Baudrate;           //  33
>> +  UINT8   ExtCount;           //  34
>> +  UINT8   AuxFlags;           //  35
>> +  UINT32  IoArg0;             // 36-39
>> +  UINT32  IoArg1;             // 40-43
>> +  UINT32  IoArg2;             // 43-47
>> +  UINT32  IoArg3;             // 48-51
>> +  UINT32  Reserved1;          // 52-55
>> +  UINT32  Reserved2;          // 56-59
>> +  UINT32  Reserved3;          // 60-63
>> +} MV_IMAGE_HEADER;
>> +
>> +STATIC
>> +UINT32
>> +CountChecksum (
>> +  UINT32 *Start,
>> +  UINT32 Length
>> +  )
>> +{
>> +  UINT32 Sum = 0;
>> +  UINT32 *Startp = Start;
>> +
>> +  do {
>> +    Sum += *Startp;
>> +    Startp++;
>> +    Length -= 4;
>> +  } while (Length > 0);
>> +
>> +  return Sum;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +CheckImageHeader (
>> +  IN VOID *ImageHeader
>> +  )
>> +{
>> +  MV_IMAGE_HEADER *Header;
>> +  UINT32 HeaderLength, Checksum, ChecksumBackup;
>> +
>> +  Header = (MV_IMAGE_HEADER *) ImageHeader;
>> +  HeaderLength = Header->PrologSize;
>> +  ChecksumBackup = Header->PrologChecksum;
>> +
>> +  // Compare magic number
>> +  if (Header->Magic != MAIN_HDR_MAGIC) {
>> +    Print (L"%s: Bad Image magic 0x%08x != 0x%08x\n", CMD_NAME_STRING,
>> +      Header->Magic, MAIN_HDR_MAGIC);
>> +    return EFI_DEVICE_ERROR;
>> +  }
>> +
>> +  // The checksum field is discarded from calculation
>> +  Header->PrologChecksum = 0;
>> +
>> +  Checksum = CountChecksum((UINT32 *)Header, HeaderLength);
>> +  if (Checksum != ChecksumBackup) {
>> +    Print (L"%s: Bad Image checksum. 0x%x != 0x%x\n", CMD_NAME_STRING, Checksum,
>> +      ChecksumBackup);
>> +    return EFI_DEVICE_ERROR;
>> +  }
>> +
>> +  // Restore checksum backup
>> +  Header->PrologChecksum = ChecksumBackup;
>> +
>> +  return 0;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +CheckFirmwareImage (
>> +  CONST CHAR16* FirmwareImage
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +  VOID *FileBuffer;
>> +  UINT64 OpenMode;
>> +  UINTN FileSize;
>> +  SHELL_FILE_HANDLE FileHandle = NULL;
>> +
>> +  OpenMode = EFI_FILE_MODE_READ;
>> +
>> +  Status = ShellOpenFileByName (FirmwareImage, &FileHandle, OpenMode, 0);
>> +    if (EFI_ERROR (Status)) {
>> +      Print (L"%s: Cannot open Image file\n", CMD_NAME_STRING);
>> +      return EFI_DEVICE_ERROR;
>> +    }
>> +
>> +  Status = FileHandleGetSize (FileHandle, &FileSize);
>> +    if (EFI_ERROR (Status)) {
>> +      Print (L"%s: Cannot get Image file size\n", CMD_NAME_STRING);
>> +    }
>> +
>> +  FileBuffer = AllocateZeroPool (FileSize);
>> +
>> +  // Read Image header into buffer
>> +  Status = FileHandleRead (FileHandle, &FileSize, FileBuffer);
>> +    if (EFI_ERROR (Status)) {
>> +      Print (L"%s: Cannot read Image file header\n", CMD_NAME_STRING);
>> +      ShellCloseFile (&FileHandle);
>> +      FreePool (FileBuffer);
>> +      return EFI_DEVICE_ERROR;
>> +    }
>> +
>> +  Status = CheckImageHeader (FileBuffer);
>> +  if (EFI_ERROR(Status)) {
>> +    return EFI_DEVICE_ERROR;
>> +  }
>> +
>> +  FreePool (FileBuffer);
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +CONST CHAR16 *
>> +FileNameFromFilePath (
>> +  CONST CHAR16 *RemoteFilePath
>> +  )
>> +{
>> +  CONST CHAR16 *Walker;
>> +
>> +  // Gather FileName from FilePath
>> +  Walker = RemoteFilePath + StrLen (RemoteFilePath);
>> +  while ((--Walker) >= RemoteFilePath) {
>> +    if ((*Walker == L'\\') || (*Walker == L'/')) {
>> +      break;
>> +    }
>> +  }
>> +
>> +  return (Walker + 1);
>> +}
>> +
>> +STATIC
>> +CONST CHAR16*
>> +PrepareFile (
>> +  LIST_ENTRY *CheckPackage
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +  CONST CHAR16  *ValueStr;
>> +
>> +  ValueStr = ShellCommandLineGetValue (CheckPackage, L"-f");
>> +  if (ValueStr == NULL) {
>> +    Print (L"%s: No LocalFilePath parameter!\n", CMD_NAME_STRING);
>> +    return NULL;
>> +  } else {
>> +    Status = ShellIsFile (ValueStr);
>> +    if (EFI_ERROR(Status)) {
>> +      Print (L"%s: Wrong LocalFilePath parameter!\n", CMD_NAME_STRING);
>> +      return NULL;
>> +    }
>> +  }
>> +  return ValueStr;
>> +}
>> +
>> +CONST CHAR16 gShellFUpdateFileName[] = L"ShellCommand";
>> +EFI_HANDLE gShellFUpdateHiiHandle = NULL;
>> +EFI_HANDLE gShellFUpdateHiiHandle;
>> +
>> +STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
>> +  {L"help", TypeFlag},
>> +  {L"-t", TypeFlag},
>> +  {L"-f", TypeValue},
>> +  {NULL , TypeMax}
>> +  };
>> +
>> +/**
>> +  Return the file name of the help text file if not using HII.
>> +
>> +  @return The string pointer to the file name.
>> +**/
>> +CONST CHAR16*
>> +EFIAPI
>> +ShellCommandGetManFileNameFUpdate (
>> +  VOID
>> +  )
>> +{
>> +
>> +  return gShellFUpdateFileName;
>> +}
>> +
>> +VOID
>> +FUpdateUsage (
>> +  VOID
>> +  )
>> +{
>> +  Print (L"\nFirmware update command\n"
>> +         "fupdate [-f <LocalFilePath>] [-t <Host> <RemoteFilePath>]\n\n"
>> +         "LocalFilePath  - path to local firmware image file\n"
>> +         "Host           - IP number of TFTP server\n"
>> +         "RemoteFilePath - path to firmware image file on TFTP server\n"
>> +         "Examples:\n"
>> +         "Update firmware from file fs2:Uefi.img\n"
>> +         "  fupdate -f fs2:Uefi.img\n"
>> +         "Update firmware from file path/Uefi.img TFTP server with IP "
>> +         "address 10.0.0.200\n"
>> +         "  fupdate -t 10.0.0.200 path/Uefi.img\n"
>> +  );
>> +}
>> +
>> +SHELL_STATUS
>> +EFIAPI
>> +ShellCommandRunFUpdate (
>> +  IN EFI_HANDLE        ImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS    Status;
>> +  LIST_ENTRY    *CheckPackage;
>> +  CHAR16        *ProblemParam, *TftpCmd = NULL, *SfCmd = NULL;
>> +  CONST CHAR16  *RemoteFilePath, *Host, *FileToWrite;
>> +  UINT8         CmdLen;
>> +  BOOLEAN       FileFlag, TftpFlag;
>> +
>> +  Status = ShellInitialize ();
>> +  if (EFI_ERROR (Status)) {
>> +    Print (L"%s: Error while initializinf Shell\n", CMD_NAME_STRING);
>> +    ASSERT_EFI_ERROR (Status);
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  Status = ShellCommandLineParse (ParamList, &CheckPackage, &ProblemParam,
>> +    TRUE);
>> +  if (EFI_ERROR (Status)) {
>> +    Print (L"Parse error!\n");
>> +  }
>> +
>> +  if (ShellCommandLineGetFlag (CheckPackage, L"help")) {
>> +    FUpdateUsage();
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  FileFlag = ShellCommandLineGetFlag (CheckPackage, L"-f");
>> +  TftpFlag = ShellCommandLineGetFlag (CheckPackage, L"-t");
>> +
>> +  if (!FileFlag && !TftpFlag) {
>> +    Print (L"%s: Please specify -f or -t flag\n", CMD_NAME_STRING);
>> +    return SHELL_ABORTED;
>> +  } else if (FileFlag && !TftpFlag) {
>> +    // Prepare local file to be burned into flash
>> +    FileToWrite = PrepareFile (CheckPackage);
>> +    if (FileToWrite == NULL) {
>> +      Print (L"%s: Error while preparing file for burn\n", CMD_NAME_STRING);
>> +      return SHELL_ABORTED;
>> +    }
>> +  } else if (TftpFlag && !FileFlag) {
>> +    Host = ShellCommandLineGetRawValue (CheckPackage, 1);
>> +    if (Host == NULL) {
>> +      Print (L"%s: No Host parameter!\n", CMD_NAME_STRING);
>> +      return SHELL_ABORTED;
>> +    }
>> +
>> +    RemoteFilePath = ShellCommandLineGetRawValue (CheckPackage, 2);
>> +    if (RemoteFilePath == NULL) {
>> +      Print (L"%s: No remote_file_path parameter!\n", CMD_NAME_STRING);
>> +      return SHELL_ABORTED;
>> +    }
>> +
>> +    // Gather firmware image name from remote filepath
>> +    FileToWrite = FileNameFromFilePath (RemoteFilePath);
>> +
>> +    // Allocate buffer for tftp command string
>> +    CmdLen = StrSize (TFTP_CMD_STRING) + StrSize (SPACE_STRING) +
>> +      StrSize (Host) + StrSize (RemoteFilePath);
>> +    TftpCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
>> +    if (TftpCmd == NULL) {
>> +      Print (L"%s: Cannot allocate memory\n", CMD_NAME_STRING);
>> +      return SHELL_ABORTED;
>> +    }
>> +
>> +    // Concatenate parameters and form tftp command string
>> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), TFTP_CMD_STRING);
>> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)Host);
>> +    // Insert space
>> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
>> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)RemoteFilePath);
>> +
>> +    RunShellCommand (TftpCmd, &Status);
>> +    FreePool (TftpCmd);
>> +    if (EFI_ERROR(Status)) {
>> +      Print (L"%s: Error while performing tftp command\n", CMD_NAME_STRING);
>> +      return SHELL_ABORTED;
>> +    }
>> +
>> +  } else {
>> +    Print (L"%s: Both -f and -t flag specified, please choose one\n",
>> +      CMD_NAME_STRING);
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  // Check image checksum and magic
>> +  Status = CheckFirmwareImage (FileToWrite);
>> +  if (EFI_ERROR(Status)) {
>> +    Print (L"%s: Wrong firmware Image\n", CMD_NAME_STRING);
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  // Probe spi bus
>> +  RunShellCommand (SF_PROBE_CMD_STRING, &Status);
>> +  if (EFI_ERROR(Status)) {
>> +    Print (L"%s: Error while performing sf probe\n", CMD_NAME_STRING);
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  // Allocate buffer for sf command string
>> +  CmdLen = StrSize (SF_WRITE_CMD_STRING) + StrSize (FileToWrite) +
>> +    StrSize (SPACE_STRING) + StrSize (SF_LOAD_ADDR_STRING);
>> +  SfCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
>> +  if (SfCmd == NULL) {
>> +    Print (L"%s: Cannot allocate memory\n");
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  // Concatenate parameters and form command string
>> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SF_WRITE_CMD_STRING);
>> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)FileToWrite);
>> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
>> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SF_LOAD_ADDR_STRING);
>> +
>> +  // Update firmware image in flash
>> +  RunShellCommand (SfCmd, &Status);
>> +  FreePool (SfCmd);
>> +  if (EFI_ERROR(Status)) {
>> +    Print (L"%s: Error while performing sf update\n", CMD_NAME_STRING);
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +ShellFUpdateCommandConstructor (
>> +  IN EFI_HANDLE        ImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +
>> +  gShellFUpdateHiiHandle = NULL;
>> +
>> +  gShellFUpdateHiiHandle = HiiAddPackages (
>> +                          &gShellFUpdateHiiGuid, gImageHandle,
>> +                          UefiShellFUpdateCommandLibStrings, NULL
>> +                          );
>> +  if (gShellFUpdateHiiHandle == NULL) {
>> +    Print (L"%s: Cannot add Hii package\n", CMD_NAME_STRING);
>> +    return EFI_DEVICE_ERROR;
>> +  }
>> +
>> +  Status = ShellCommandRegisterCommandName (
>> +     CMD_NAME_STRING, ShellCommandRunFUpdate, ShellCommandGetManFileNameFUpdate,
>> +     0, CMD_NAME_STRING, TRUE , gShellFUpdateHiiHandle,
>> +     STRING_TOKEN (STR_GET_HELP_FUPDATE)
>> +     );
>> +  if (EFI_ERROR(Status)) {
>> +    Print (L"%s: Error while registering command\n", CMD_NAME_STRING);
>> +    return SHELL_ABORTED;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +ShellFUpdateCommandDestructor (
>> +  IN EFI_HANDLE        ImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +
>> +  if (gShellFUpdateHiiHandle != NULL) {
>> +    HiiRemovePackages (gShellFUpdateHiiHandle);
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/Applications/FirmwareUpdate/FUpdate.inf b/Applications/FirmwareUpdate/FUpdate.inf
>> new file mode 100644
>> index 0000000..53b5305
>> --- /dev/null
>> +++ b/Applications/FirmwareUpdate/FUpdate.inf
>> @@ -0,0 +1,68 @@
>> +#
>> +# 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]
>> +  INF_VERSION = 0x00010006
>> +  BASE_NAME = UefiShellFUpdateCommandLib
>> +  FILE_GUID = 470292b2-926b-4ed8-8080-be7a260db627
>> +  MODULE_TYPE = UEFI_APPLICATION
>> +  VERSION_STRING = 0.1
>> +  LIBRARY_CLASS = NULL|UEFI_APPLICATION UEFI_DRIVER
>> +  CONSTRUCTOR = ShellFUpdateCommandConstructor
>> +  DESTRUCTOR = ShellFUpdateCommandDestructor
>> +
>> +[Sources]
>> +  FUpdate.c
>> +  FUpdate.uni
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  ShellPkg/ShellPkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  OpenPlatformPkg/Platforms/Marvell/Marvell.dec
>> +
>> +[LibraryClasses]
>> +  UefiLib
>> +  UefiBootServicesTableLib
>> +  MemoryAllocationLib
>> +  BaseLib
>> +  BaseMemoryLib
>> +  DebugLib
>> +  ShellCommandLib
>> +  ShellLib
>> +  UefiLib
>> +  UefiRuntimeServicesTableLib
>> +  PcdLib
>> +  HiiLib
>> +  FileHandleLib
>> +
>> +[Guids]
>> +  gShellFUpdateHiiGuid
>> diff --git a/Applications/FirmwareUpdate/FUpdate.uni b/Applications/FirmwareUpdate/FUpdate.uni
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..6143c0580f1d1ef8ce4fe619a1df1ce380adf2c0
>> GIT binary patch
>> literal 5838
>> zcmd6r?N1v=5XSd&rTz~m@+FOuK&z@BXdB5G6OhbHeI_(&s)*P?EX6>LDdL}R`}}4Y
>> z_Fim)B2|h`_TBEy&OY<Z%kJL4|E#8GU-k8E`X+VLMY>G4X_hAGEPYJ#RHUcs{Z98E
>> z(pvgH{iLVebS#O#(@FZIt4KTPT#_g1JJ*}J#$MPH@A_(w)60Z*e$BBPsZ5(DH%TMu
>> zI8RslFVenngr*p~lTP*KR@$$1pGl4p6GJjq&s>Nn8egO>-9MS0Q^{SVsru?OKUYs^
>> znhN!;+Src8b3GNB10={X)7Ui6^*l)*^bUIqjXT$yRmsoHx~ZO!JT-lb@LbynxN^K`
>> zIn^kbyht}1doJb+wK5;k=NwOk%lRBE#O3S<wy>ix&4tFo-prWGG9T<MUWEtn%*5H1
>> zSq~;Gfgeyu8$Ga<Z3c1n|8rvmo!EG(e{`Y?JMh<~dM@O>iCW&nJZN~Bn~_GrJG$Ij
>> zd>%7RrE8wn?<20zyXy5!t%sOH*M&t|ohxPqCl(VMpv1PhX2wGq^RXfXyO3p6Go#Mc
>> z2Xdm<S*qfT=Nvu6g3sr;me~&{8O5Tg@+clDy1)!mvr2Bp%yEAVQ*Qa<_-Qib{vL}x
>> zEYC<E=8NnPMAdtpMc3xp|Nrt8nS_5%EXt~=;(06|k7OwvQyp_&MAmuLAl6(HxkV-N
>> zET8D<lf1qtOCkd^gYD|%d6DaG;EFrG-4iy??Rq_;ii11ypk6r(+2fVKanpABoH=Nv
>> z<&j#gnvCR4)Z7mbaOf4-W15N#lkkcLM+avfRa7Cb$<-)x6l5)Rn~X$TUK4VTk?W_@
>> za3fp1Vg<`y=axm`l~uNpw4drXy2AteUMrc`D;^ahSHUaU(DczTZEGzpC0A<w+e*W9
>> zlsc+aJ@t71fm-|Njar-P-BkZZdaHj%BGuK~1N96f@mhUD^=+%KZ}J1v-I3oJiR_*v
>> zkljd!dfL}Bv#)4Y?##KOyN`eYo|vzfe%Cx8D@~G-$nUDRue)P&^aC3;wBFKIlpSg5
>> zO72iNcz$iYj)AD$H{9si)o*yG4ScM|VQYOh&@Gyeoy3+#?Mll)vu@v^>B#0C!#XsJ
>> zf|V_saqr%;En@`^3fYD?u%s*OeQVKssNMs~l^TU4ynqMmO0%@1ClG9^+QQSG{;?A#
>> z-{^-Q@H*)Fy0VpvJi0GSK?vv0{BmDzNRD0Djx=xRPRnho`V*v}?g|GSgK$w_S8s?s
>> z@PZWCQnoXa8N7$+=-tHvY`itODpHvGO}x1#A6ZCqN25B52jU8RPYubJ$BO3*-oiRO
>> z;7of=l<H^yiARTeCejMczOQRj{lPA12bOhRJ(cNa!`7EnL_ZOBs8-J`#SZv^9k<$X
>> zcx*8~`ic3{Y>Z5Td2kGL9g117k*Dmy$>bxwBVzD<S9AER<a0zY(pBGjzJP<wb;~0j
>> zat46)NHg>_BmbVy#A%ML&V78q8Fhnr$<bBU=9YU^87}oEdC(K*Aj0od2E5>-sRWMZ
>> zNblhn9*8MgUOOV6qBc<l+)_Lm7Dv@^zN)=wg0E)Hg^|ck=D^1t#Rb(DByjF*hHkF`
>> z;HuY_9BtH}dYtDK`s+R&t{O-1w3>cRPwrb!Y%j@~g;kdG85b+{3yV!w6Iy3>qPfzZ
>> zX{9ojT`eo5pLsm0-dh|D(|hxLUpeqz+>;BQS42nq$~dmIf}d&5CJLW-MHLu-u?x7?
>> zdOVi2?;F<Cx1yOQLNu-H0}g9a>}aO8dtoo~QX}3;{+%qy`l;i@HPx)wdSBPauAx3(
>> z>^nf%KE)1&a40S~$+4?pPs1L58H!T5z)Evq`BJvzidIjPmp|l5{Q8S>lszr`Qdr`A
>> zePHZ;Os~?Pl0b59lG~ERx38?lX)E?;i$*TP4(h0Xk3mjUJT#X_%?n8_X0gin<(*~h
>> zvDtgF@4S(hIi(+~mG|D+OX0buJ?&^+)>Dm0=76ngbhQ19reJ%S)<S+^oo4sxJJnmY
>> zGY6%`kVg`g?m$uXqa?6%tocvmOTIZQvTJ!v1RCa!I0-gk(Id$@uiD7@iqnDTaIo8+
>> zyV>(n7Ls@DKFB<FqO7V-xpu@iN36HTd@g76i6Zb~*Ok7Rt*dfT7boHmyUtp=;i;K(
>> zo^g-W@FnOXzE@lLs*zExG5O1n$4E0i3Vj`1la=w~d*Rw6t)*%#8qJA``utI=&gXax
>> z|FhF$H#bn-d!tC;obk9gsE#aZW7`pd*NC^C^!K$wv6t-H-o^KX5&g~k(7qbL-AH!%
>> z2@qzF6)k>Vp2}uwYkVd1H)P>oTCvDQf3r9rtC-mFvyX}sR8P6g7q^*ax)F|8UHrt%
>> QHyJ;b`f6NX>FQbZ9}*T`00000
>>
>> literal 0
>> HcmV?d00001
>>
>> diff --git a/Platforms/Marvell/Marvell.dec b/Platforms/Marvell/Marvell.dec
>> index e63add4..f9d2660 100644
>> --- a/Platforms/Marvell/Marvell.dec
>> +++ b/Platforms/Marvell/Marvell.dec
>> @@ -54,6 +54,7 @@
>>
>>    gShellEepromHiiGuid = { 0xb2f4c714, 0x147f, 0x4ff7, { 0x82, 0x1b, 0xce, 0x7b, 0x91, 0x7f, 0x5f, 0x2f } }
>>    gShellSfHiiGuid = { 0x03a67756, 0x8cde, 0x4638, { 0x82, 0x34, 0x4a, 0x0f, 0x6d, 0x58, 0x81, 0x39 } }
>> +  gShellFUpdateHiiGuid = { 0x9b5d2176, 0x590a, 0x49db, { 0x89, 0x5d, 0x4a, 0x70, 0xfe, 0xad, 0xbe, 0x24 } }
>>
>>  [PcdsFixedAtBuild.common]
>>  #MPP
>> --
>> 1.8.3.1
>>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
  2016-11-16 16:13     ` [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell Marcin Wojtas
@ 2016-11-16 17:05       ` Carsey, Jaben
  2016-11-16 17:35         ` Leif Lindholm
  0 siblings, 1 reply; 7+ messages in thread
From: Carsey, Jaben @ 2016-11-16 17:05 UTC (permalink / raw)
  To: Marcin Wojtas, Ard Biesheuvel
  Cc: linaro-uefi, Leif Lindholm, Neta Zur Hershkovits, Yehuda Yitschak,
	Haim Boot, Jan Dabros, Bartosz Szczepanek, edk2-devel-01,
	Ni, Ruiyu, Carsey, Jaben

Marcin and Ard,

It sounds like there is a goal to have a UEFI Application call into an internal shell command.

There is no documented method to programmatically call an internal command. This is not a goal of the current shell implementation. ShellExecute explicitly does not allow running of internal commands (per UEFI Shell Specification 2.2).

Thus I don’t think using ShellDynamicCommandProtocol will help.  That protocol is for allowing a driver add a command to the shell's list of available commands.

-Jaben

> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Wednesday, November 16, 2016 8:13 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: linaro-uefi <linaro-uefi@lists.linaro.org>; Leif Lindholm
> <leif.lindholm@linaro.org>; Neta Zur Hershkovits <neta@marvell.com>;
> Yehuda Yitschak <yehuday@marvell.com>; Haim Boot
> <hayim@marvell.com>; Jan Dąbroś <jsd@semihalf.com>; Bartosz
> Szczepanek <bsz@semihalf.com>; edk2-devel-01 <edk2-devel@lists.01.org>;
> Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate'
> comand to shell
> Importance: High
> 
> + Jaben, Ruiyu and edk2 list
> 
> Hi Ard,
> 
> Internal Shell header ("../../../ShellPkg/Application/Shell/Shell.h")
> was added ONLY for being able to execute shell commands (tftp and our
> custom SPI flash control) from within a new command. This is now done
> with:
> RunShellCommand (TftpCmd, &Status);
> And it works fine.
> 
> Replacing above with:
> ShellExecute (&ImageHandle, TftpCmd, FALSE, NULL, &Status);
> allow to get rid of relative include and seems more appropriate, but
> there is completely no effect of calling it.
> 
> I checked in edk2 sources and see no relation between above problem
> and registering our command with help of ShellDynamicCommand protocol.
> Unless I don't know something. Anyway, I'll apreciate any hint, how to
> solve the issue in a nice way.
> 
> Best regards,
> Marcin
> 
> 2016-11-15 17:09 GMT+01:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> > On 10 July 2016 at 00:21, Marcin Wojtas <mw@semihalf.com> wrote:
> >> From: Jan Dąbroś <jsd@semihalf.com>
> >>
> >> 'fupdate' command performs updating firmware from file placed on local
> >> filesystem or on TFTP server.
> >> It uses tftp and sf commands.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >
> > Please use the Shell dynamic command protocol to add commands to the
> > shell. This will remove the need for including internal Shell headers,
> > and even allow your commands to work with prebuilt Shell binaries.
> >
> > MdePkg/Include/Protocol/ShellDynamicCommand.h
> >
> > Thanks,
> > Ard.
> >
> >
> >> ---
> >>  Applications/FirmwareUpdate/FUpdate.c   | 448
> ++++++++++++++++++++++++++++++++
> >>  Applications/FirmwareUpdate/FUpdate.inf |  68 +++++
> >>  Applications/FirmwareUpdate/FUpdate.uni | Bin 0 -> 5838 bytes
> >>  Platforms/Marvell/Marvell.dec           |   1 +
> >>  4 files changed, 517 insertions(+)
> >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.c
> >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.inf
> >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.uni
> >>
> >> diff --git a/Applications/FirmwareUpdate/FUpdate.c
> b/Applications/FirmwareUpdate/FUpdate.c
> >> new file mode 100644
> >> index 0000000..359a4ac
> >> --- /dev/null
> >> +++ b/Applications/FirmwareUpdate/FUpdate.c
> >> @@ -0,0 +1,448 @@
> >>
> +/*********************************************************
> **********************
> >> +Copyright (C) 2016 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.
> >> +
> >>
> +*********************************************************
> **********************/
> >> +#include <Uefi.h>
> >> +#include <ShellBase.h>
> >> +
> >> +#include <Library/BaseLib.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +#include <Library/ShellCommandLib.h>
> >> +#include <Library/ShellLib.h>
> >> +#include <Library/UefiLib.h>
> >> +#include <Library/PrintLib.h>
> >> +#include <Library/UefiLib.h>
> >> +#include <Library/ShellCEntryLib.h>
> >> +#include <Library/HiiLib.h>
> >> +#include <Guid/ShellLibHiiGuid.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +#include <Library/FileHandleLib.h>
> >> +// FIXME: Remove including relative path
> >> +#include "../../../ShellPkg/Application/Shell/Shell.h"
> >> +
> >> +#define CMD_NAME_STRING       L"fupdate"
> >> +
> >> +#define TFTP_CMD_STRING       L"tftp "
> >> +#define SPACE_STRING          L" "
> >> +#define SF_PROBE_CMD_STRING   L"sf probe"
> >> +#define SF_WRITE_CMD_STRING   L"sf updatefile "
> >> +#define SF_LOAD_ADDR_STRING   L"0x0"
> >> +
> >> +#define MAIN_HDR_MAGIC        0xB105B002
> >> +
> >> +typedef struct {
> >> +  UINT32  Magic;              //  0-3
> >> +  UINT32  PrologSize;         //  4-7
> >> +  UINT32  PrologChecksum;     //  8-11
> >> +  UINT32  BootImageSize;      // 12-15
> >> +  UINT32  BootImageChecksum;  // 16-19
> >> +  UINT32  Reserved0;          // 20-23
> >> +  UINT32  LoadAddr;           // 24-27
> >> +  UINT32  ExecAddr;           // 28-31
> >> +  UINT8   UartConfig;         //  32
> >> +  UINT8   Baudrate;           //  33
> >> +  UINT8   ExtCount;           //  34
> >> +  UINT8   AuxFlags;           //  35
> >> +  UINT32  IoArg0;             // 36-39
> >> +  UINT32  IoArg1;             // 40-43
> >> +  UINT32  IoArg2;             // 43-47
> >> +  UINT32  IoArg3;             // 48-51
> >> +  UINT32  Reserved1;          // 52-55
> >> +  UINT32  Reserved2;          // 56-59
> >> +  UINT32  Reserved3;          // 60-63
> >> +} MV_IMAGE_HEADER;
> >> +
> >> +STATIC
> >> +UINT32
> >> +CountChecksum (
> >> +  UINT32 *Start,
> >> +  UINT32 Length
> >> +  )
> >> +{
> >> +  UINT32 Sum = 0;
> >> +  UINT32 *Startp = Start;
> >> +
> >> +  do {
> >> +    Sum += *Startp;
> >> +    Startp++;
> >> +    Length -= 4;
> >> +  } while (Length > 0);
> >> +
> >> +  return Sum;
> >> +}
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +CheckImageHeader (
> >> +  IN VOID *ImageHeader
> >> +  )
> >> +{
> >> +  MV_IMAGE_HEADER *Header;
> >> +  UINT32 HeaderLength, Checksum, ChecksumBackup;
> >> +
> >> +  Header = (MV_IMAGE_HEADER *) ImageHeader;
> >> +  HeaderLength = Header->PrologSize;
> >> +  ChecksumBackup = Header->PrologChecksum;
> >> +
> >> +  // Compare magic number
> >> +  if (Header->Magic != MAIN_HDR_MAGIC) {
> >> +    Print (L"%s: Bad Image magic 0x%08x != 0x%08x\n",
> CMD_NAME_STRING,
> >> +      Header->Magic, MAIN_HDR_MAGIC);
> >> +    return EFI_DEVICE_ERROR;
> >> +  }
> >> +
> >> +  // The checksum field is discarded from calculation
> >> +  Header->PrologChecksum = 0;
> >> +
> >> +  Checksum = CountChecksum((UINT32 *)Header, HeaderLength);
> >> +  if (Checksum != ChecksumBackup) {
> >> +    Print (L"%s: Bad Image checksum. 0x%x != 0x%x\n",
> CMD_NAME_STRING, Checksum,
> >> +      ChecksumBackup);
> >> +    return EFI_DEVICE_ERROR;
> >> +  }
> >> +
> >> +  // Restore checksum backup
> >> +  Header->PrologChecksum = ChecksumBackup;
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +CheckFirmwareImage (
> >> +  CONST CHAR16* FirmwareImage
> >> +  )
> >> +{
> >> +  EFI_STATUS Status;
> >> +  VOID *FileBuffer;
> >> +  UINT64 OpenMode;
> >> +  UINTN FileSize;
> >> +  SHELL_FILE_HANDLE FileHandle = NULL;
> >> +
> >> +  OpenMode = EFI_FILE_MODE_READ;
> >> +
> >> +  Status = ShellOpenFileByName (FirmwareImage, &FileHandle,
> OpenMode, 0);
> >> +    if (EFI_ERROR (Status)) {
> >> +      Print (L"%s: Cannot open Image file\n", CMD_NAME_STRING);
> >> +      return EFI_DEVICE_ERROR;
> >> +    }
> >> +
> >> +  Status = FileHandleGetSize (FileHandle, &FileSize);
> >> +    if (EFI_ERROR (Status)) {
> >> +      Print (L"%s: Cannot get Image file size\n", CMD_NAME_STRING);
> >> +    }
> >> +
> >> +  FileBuffer = AllocateZeroPool (FileSize);
> >> +
> >> +  // Read Image header into buffer
> >> +  Status = FileHandleRead (FileHandle, &FileSize, FileBuffer);
> >> +    if (EFI_ERROR (Status)) {
> >> +      Print (L"%s: Cannot read Image file header\n", CMD_NAME_STRING);
> >> +      ShellCloseFile (&FileHandle);
> >> +      FreePool (FileBuffer);
> >> +      return EFI_DEVICE_ERROR;
> >> +    }
> >> +
> >> +  Status = CheckImageHeader (FileBuffer);
> >> +  if (EFI_ERROR(Status)) {
> >> +    return EFI_DEVICE_ERROR;
> >> +  }
> >> +
> >> +  FreePool (FileBuffer);
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +STATIC
> >> +CONST CHAR16 *
> >> +FileNameFromFilePath (
> >> +  CONST CHAR16 *RemoteFilePath
> >> +  )
> >> +{
> >> +  CONST CHAR16 *Walker;
> >> +
> >> +  // Gather FileName from FilePath
> >> +  Walker = RemoteFilePath + StrLen (RemoteFilePath);
> >> +  while ((--Walker) >= RemoteFilePath) {
> >> +    if ((*Walker == L'\\') || (*Walker == L'/')) {
> >> +      break;
> >> +    }
> >> +  }
> >> +
> >> +  return (Walker + 1);
> >> +}
> >> +
> >> +STATIC
> >> +CONST CHAR16*
> >> +PrepareFile (
> >> +  LIST_ENTRY *CheckPackage
> >> +  )
> >> +{
> >> +  EFI_STATUS Status;
> >> +  CONST CHAR16  *ValueStr;
> >> +
> >> +  ValueStr = ShellCommandLineGetValue (CheckPackage, L"-f");
> >> +  if (ValueStr == NULL) {
> >> +    Print (L"%s: No LocalFilePath parameter!\n", CMD_NAME_STRING);
> >> +    return NULL;
> >> +  } else {
> >> +    Status = ShellIsFile (ValueStr);
> >> +    if (EFI_ERROR(Status)) {
> >> +      Print (L"%s: Wrong LocalFilePath parameter!\n",
> CMD_NAME_STRING);
> >> +      return NULL;
> >> +    }
> >> +  }
> >> +  return ValueStr;
> >> +}
> >> +
> >> +CONST CHAR16 gShellFUpdateFileName[] = L"ShellCommand";
> >> +EFI_HANDLE gShellFUpdateHiiHandle = NULL;
> >> +EFI_HANDLE gShellFUpdateHiiHandle;
> >> +
> >> +STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> >> +  {L"help", TypeFlag},
> >> +  {L"-t", TypeFlag},
> >> +  {L"-f", TypeValue},
> >> +  {NULL , TypeMax}
> >> +  };
> >> +
> >> +/**
> >> +  Return the file name of the help text file if not using HII.
> >> +
> >> +  @return The string pointer to the file name.
> >> +**/
> >> +CONST CHAR16*
> >> +EFIAPI
> >> +ShellCommandGetManFileNameFUpdate (
> >> +  VOID
> >> +  )
> >> +{
> >> +
> >> +  return gShellFUpdateFileName;
> >> +}
> >> +
> >> +VOID
> >> +FUpdateUsage (
> >> +  VOID
> >> +  )
> >> +{
> >> +  Print (L"\nFirmware update command\n"
> >> +         "fupdate [-f <LocalFilePath>] [-t <Host> <RemoteFilePath>]\n\n"
> >> +         "LocalFilePath  - path to local firmware image file\n"
> >> +         "Host           - IP number of TFTP server\n"
> >> +         "RemoteFilePath - path to firmware image file on TFTP server\n"
> >> +         "Examples:\n"
> >> +         "Update firmware from file fs2:Uefi.img\n"
> >> +         "  fupdate -f fs2:Uefi.img\n"
> >> +         "Update firmware from file path/Uefi.img TFTP server with IP "
> >> +         "address 10.0.0.200\n"
> >> +         "  fupdate -t 10.0.0.200 path/Uefi.img\n"
> >> +  );
> >> +}
> >> +
> >> +SHELL_STATUS
> >> +EFIAPI
> >> +ShellCommandRunFUpdate (
> >> +  IN EFI_HANDLE        ImageHandle,
> >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +  LIST_ENTRY    *CheckPackage;
> >> +  CHAR16        *ProblemParam, *TftpCmd = NULL, *SfCmd = NULL;
> >> +  CONST CHAR16  *RemoteFilePath, *Host, *FileToWrite;
> >> +  UINT8         CmdLen;
> >> +  BOOLEAN       FileFlag, TftpFlag;
> >> +
> >> +  Status = ShellInitialize ();
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"%s: Error while initializinf Shell\n", CMD_NAME_STRING);
> >> +    ASSERT_EFI_ERROR (Status);
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  Status = ShellCommandLineParse (ParamList, &CheckPackage,
> &ProblemParam,
> >> +    TRUE);
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"Parse error!\n");
> >> +  }
> >> +
> >> +  if (ShellCommandLineGetFlag (CheckPackage, L"help")) {
> >> +    FUpdateUsage();
> >> +    return EFI_SUCCESS;
> >> +  }
> >> +
> >> +  FileFlag = ShellCommandLineGetFlag (CheckPackage, L"-f");
> >> +  TftpFlag = ShellCommandLineGetFlag (CheckPackage, L"-t");
> >> +
> >> +  if (!FileFlag && !TftpFlag) {
> >> +    Print (L"%s: Please specify -f or -t flag\n", CMD_NAME_STRING);
> >> +    return SHELL_ABORTED;
> >> +  } else if (FileFlag && !TftpFlag) {
> >> +    // Prepare local file to be burned into flash
> >> +    FileToWrite = PrepareFile (CheckPackage);
> >> +    if (FileToWrite == NULL) {
> >> +      Print (L"%s: Error while preparing file for burn\n",
> CMD_NAME_STRING);
> >> +      return SHELL_ABORTED;
> >> +    }
> >> +  } else if (TftpFlag && !FileFlag) {
> >> +    Host = ShellCommandLineGetRawValue (CheckPackage, 1);
> >> +    if (Host == NULL) {
> >> +      Print (L"%s: No Host parameter!\n", CMD_NAME_STRING);
> >> +      return SHELL_ABORTED;
> >> +    }
> >> +
> >> +    RemoteFilePath = ShellCommandLineGetRawValue (CheckPackage, 2);
> >> +    if (RemoteFilePath == NULL) {
> >> +      Print (L"%s: No remote_file_path parameter!\n",
> CMD_NAME_STRING);
> >> +      return SHELL_ABORTED;
> >> +    }
> >> +
> >> +    // Gather firmware image name from remote filepath
> >> +    FileToWrite = FileNameFromFilePath (RemoteFilePath);
> >> +
> >> +    // Allocate buffer for tftp command string
> >> +    CmdLen = StrSize (TFTP_CMD_STRING) + StrSize (SPACE_STRING) +
> >> +      StrSize (Host) + StrSize (RemoteFilePath);
> >> +    TftpCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
> >> +    if (TftpCmd == NULL) {
> >> +      Print (L"%s: Cannot allocate memory\n", CMD_NAME_STRING);
> >> +      return SHELL_ABORTED;
> >> +    }
> >> +
> >> +    // Concatenate parameters and form tftp command string
> >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), TFTP_CMD_STRING);
> >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)Host);
> >> +    // Insert space
> >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
> >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16
> *)RemoteFilePath);
> >> +
> >> +    RunShellCommand (TftpCmd, &Status);
> >> +    FreePool (TftpCmd);
> >> +    if (EFI_ERROR(Status)) {
> >> +      Print (L"%s: Error while performing tftp command\n",
> CMD_NAME_STRING);
> >> +      return SHELL_ABORTED;
> >> +    }
> >> +
> >> +  } else {
> >> +    Print (L"%s: Both -f and -t flag specified, please choose one\n",
> >> +      CMD_NAME_STRING);
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  // Check image checksum and magic
> >> +  Status = CheckFirmwareImage (FileToWrite);
> >> +  if (EFI_ERROR(Status)) {
> >> +    Print (L"%s: Wrong firmware Image\n", CMD_NAME_STRING);
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  // Probe spi bus
> >> +  RunShellCommand (SF_PROBE_CMD_STRING, &Status);
> >> +  if (EFI_ERROR(Status)) {
> >> +    Print (L"%s: Error while performing sf probe\n",
> CMD_NAME_STRING);
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  // Allocate buffer for sf command string
> >> +  CmdLen = StrSize (SF_WRITE_CMD_STRING) + StrSize (FileToWrite) +
> >> +    StrSize (SPACE_STRING) + StrSize (SF_LOAD_ADDR_STRING);
> >> +  SfCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
> >> +  if (SfCmd == NULL) {
> >> +    Print (L"%s: Cannot allocate memory\n");
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  // Concatenate parameters and form command string
> >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SF_WRITE_CMD_STRING);
> >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)FileToWrite);
> >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
> >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SF_LOAD_ADDR_STRING);
> >> +
> >> +  // Update firmware image in flash
> >> +  RunShellCommand (SfCmd, &Status);
> >> +  FreePool (SfCmd);
> >> +  if (EFI_ERROR(Status)) {
> >> +    Print (L"%s: Error while performing sf update\n",
> CMD_NAME_STRING);
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +ShellFUpdateCommandConstructor (
> >> +  IN EFI_HANDLE        ImageHandle,
> >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> >> +  )
> >> +{
> >> +  EFI_STATUS Status;
> >> +
> >> +  gShellFUpdateHiiHandle = NULL;
> >> +
> >> +  gShellFUpdateHiiHandle = HiiAddPackages (
> >> +                          &gShellFUpdateHiiGuid, gImageHandle,
> >> +                          UefiShellFUpdateCommandLibStrings, NULL
> >> +                          );
> >> +  if (gShellFUpdateHiiHandle == NULL) {
> >> +    Print (L"%s: Cannot add Hii package\n", CMD_NAME_STRING);
> >> +    return EFI_DEVICE_ERROR;
> >> +  }
> >> +
> >> +  Status = ShellCommandRegisterCommandName (
> >> +     CMD_NAME_STRING, ShellCommandRunFUpdate,
> ShellCommandGetManFileNameFUpdate,
> >> +     0, CMD_NAME_STRING, TRUE , gShellFUpdateHiiHandle,
> >> +     STRING_TOKEN (STR_GET_HELP_FUPDATE)
> >> +     );
> >> +  if (EFI_ERROR(Status)) {
> >> +    Print (L"%s: Error while registering command\n",
> CMD_NAME_STRING);
> >> +    return SHELL_ABORTED;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +ShellFUpdateCommandDestructor (
> >> +  IN EFI_HANDLE        ImageHandle,
> >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> >> +  )
> >> +{
> >> +
> >> +  if (gShellFUpdateHiiHandle != NULL) {
> >> +    HiiRemovePackages (gShellFUpdateHiiHandle);
> >> +  }
> >> +  return EFI_SUCCESS;
> >> +}
> >> diff --git a/Applications/FirmwareUpdate/FUpdate.inf
> b/Applications/FirmwareUpdate/FUpdate.inf
> >> new file mode 100644
> >> index 0000000..53b5305
> >> --- /dev/null
> >> +++ b/Applications/FirmwareUpdate/FUpdate.inf
> >> @@ -0,0 +1,68 @@
> >> +#
> >> +# 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]
> >> +  INF_VERSION = 0x00010006
> >> +  BASE_NAME = UefiShellFUpdateCommandLib
> >> +  FILE_GUID = 470292b2-926b-4ed8-8080-be7a260db627
> >> +  MODULE_TYPE = UEFI_APPLICATION
> >> +  VERSION_STRING = 0.1
> >> +  LIBRARY_CLASS = NULL|UEFI_APPLICATION UEFI_DRIVER
> >> +  CONSTRUCTOR = ShellFUpdateCommandConstructor
> >> +  DESTRUCTOR = ShellFUpdateCommandDestructor
> >> +
> >> +[Sources]
> >> +  FUpdate.c
> >> +  FUpdate.uni
> >> +
> >> +[Packages]
> >> +  MdePkg/MdePkg.dec
> >> +  ShellPkg/ShellPkg.dec
> >> +  MdeModulePkg/MdeModulePkg.dec
> >> +  OpenPlatformPkg/Platforms/Marvell/Marvell.dec
> >> +
> >> +[LibraryClasses]
> >> +  UefiLib
> >> +  UefiBootServicesTableLib
> >> +  MemoryAllocationLib
> >> +  BaseLib
> >> +  BaseMemoryLib
> >> +  DebugLib
> >> +  ShellCommandLib
> >> +  ShellLib
> >> +  UefiLib
> >> +  UefiRuntimeServicesTableLib
> >> +  PcdLib
> >> +  HiiLib
> >> +  FileHandleLib
> >> +
> >> +[Guids]
> >> +  gShellFUpdateHiiGuid
> >> diff --git a/Applications/FirmwareUpdate/FUpdate.uni
> b/Applications/FirmwareUpdate/FUpdate.uni
> >> new file mode 100644
> >> index
> 0000000000000000000000000000000000000000..6143c0580f1d1ef8ce4fe619a1
> df1ce380adf2c0
> >> GIT binary patch
> >> literal 5838
> >>
> zcmd6r?N1v=5XSd&rTz~m@+FOuK&z@BXdB5G6OhbHeI_(&s)*P?EX6>LDdL}
> R`}}4Y
> >> z_Fim)B2|h`_TBEy&OY<Z%kJL4|E#8GU-
> k8E`X+VLMY>G4X_hAGEPYJ#RHUcs{Z98E
> >>
> z(pvgH{iLVebS#O#(@FZIt4KTPT#_g1JJ*}J#$MPH@A_(w)60Z*e$BBPsZ5(DH%
> TMu
> >> zI8RslFVenngr*p~lTP*KR@$$1pGl4p6GJjq&s>Nn8egO>-
> 9MS0Q^{SVsru?OKUYs^
> >> znhN!;+Src8b3GNB10={X)7Ui6^*l)*^bUIqjXT$yRmsoHx~ZO!JT-
> lb@LbynxN^K`
> >> zIn^kbyht}1doJb+wK5;k=NwOk%lRBE#O3S<wy>ix&4tFo-
> prWGG9T<MUWEtn%*5H1
> >>
> zSq~;Gfgeyu8$Ga<Z3c1n|8rvmo!EG(e{`Y?JMh<~dM@O>iCW&nJZN~Bn~_Gr
> JG$Ij
> >>
> zd>%7RrE8wn?<20zyXy5!t%sOH*M&t|ohxPqCl(VMpv1PhX2wGq^RXfXyO3p
> 6Go#Mc
> >> z2Xdm<S*qfT=Nvu6g3sr;me~&{8O5Tg@+clDy1)!mvr2Bp%yEAVQ*Qa<_-
> Qib{vL}x
> >>
> zEYC<E=8NnPMAdtpMc3xp|Nrt8nS_5%EXt~=;(06|k7OwvQyp_&MAmuLAl6(
> HxkV-N
> >> zET8D<lf1qtOCkd^gYD|%d6DaG;EFrG-
> 4iy??Rq_;ii11ypk6r(+2fVKanpABoH=Nv
> >> z<&j#gnvCR4)Z7mbaOf4-W15N#lkkcLM+avfRa7Cb$<-
> )x6l5)Rn~X$TUK4VTk?W_@
> >>
> za3fp1Vg<`y=axm`l~uNpw4drXy2AteUMrc`D;^ahSHUaU(DczTZEGzpC0A<w+
> e*W9
> >> zlsc+aJ@t71fm-|Njar-P-BkZZdaHj%BGuK~1N96f@mhUD^=+%KZ}J1v-
> I3oJiR_*v
> >> zkljd!dfL}Bv#)4Y?##KOyN`eYo|vzfe%Cx8D@~G-
> $nUDRue)P&^aC3;wBFKIlpSg5
> >>
> zO72iNcz$iYj)AD$H{9si)o*yG4ScM|VQYOh&@Gyeoy3+#?Mll)vu@v^>B#0C!#
> XsJ
> >>
> zf|V_saqr%;En@`^3fYD?u%s*OeQVKssNMs~l^TU4ynqMmO0%@1ClG9^+Q
> QSG{;?A#
> >> z-{^-
> Q@H*)Fy0VpvJi0GSK?vv0{BmDzNRD0Djx=xRPRnho`V*v}?g|GSgK$w_S8s?s
> >> z@PZWCQnoXa8N7$+=-
> tHvY`itODpHvGO}x1#A6ZCqN25B52jU8RPYubJ$BO3*-oiRO
> >> z;7of=l<H^yiARTeCejMczOQRj{lPA12bOhRJ(cNa!`7EnL_ZOBs8-
> J`#SZv^9k<$X
> >>
> zcx*8~`ic3{Y>Z5Td2kGL9g117k*Dmy$>bxwBVzD<S9AER<a0zY(pBGjzJP<wb;~
> 0j
> >>
> zat46)NHg>_BmbVy#A%ML&V78q8Fhnr$<bBU=9YU^87}oEdC(K*Aj0od2E5>-
> sRWMZ
> >>
> zNblhn9*8MgUOOV6qBc<l+)_Lm7Dv@^zN)=wg0E)Hg^|ck=D^1t#Rb(DByjF*
> hHkF`
> >> z;HuY_9BtH}dYtDK`s+R&t{O-
> 1w3>cRPwrb!Y%j@~g;kdG85b+{3yV!w6Iy3>qPfzZ
> >> zX{9ojT`eo5pLsm0-dh|D(|hxLUpeqz+>;BQS42nq$~dmIf}d&5CJLW-MHLu-
> u?x7?
> >> zdOVi2?;F<Cx1yOQLNu-
> H0}g9a>}aO8dtoo~QX}3;{+%qy`l;i@HPx)wdSBPauAx3(
> >> z>^nf%KE)1&a40S~$+4?pPs1L58H!T5z)Evq`BJvzidIjPmp|l5{Q8S>lszr`Qdr`A
> >>
> zePHZ;Os~?Pl0b59lG~ERx38?lX)E?;i$*TP4(h0Xk3mjUJT#X_%?n8_X0gin<(*~h
> >>
> zvDtgF@4S(hIi(+~mG|D+OX0buJ?&^+)>Dm0=76ngbhQ19reJ%S)<S+^oo4sxJJ
> nmY
> >> zGY6%`kVg`g?m$uXqa?6%tocvmOTIZQvTJ!v1RCa!I0-
> gk(Id$@uiD7@iqnDTaIo8+
> >> zyV>(n7Ls@DKFB<FqO7V-
> xpu@iN36HTd@g76i6Zb~*Ok7Rt*dfT7boHmyUtp=;i;K(
> >> zo^g-
> W@FnOXzE@lLs*zExG5O1n$4E0i3Vj`1la=w~d*Rw6t)*%#8qJA``utI=&gXax
> >> z|FhF$H#bn-d!tC;obk9gsE#aZW7`pd*NC^C^!K$wv6t-H-o^KX5&g~k(7qbL-
> AH!%
> >> z2@qzF6)k>Vp2}uwYkVd1H)P>oTCvDQf3r9rtC-
> mFvyX}sR8P6g7q^*ax)F|8UHrt%
> >> QHyJ;b`f6NX>FQbZ9}*T`00000
> >>
> >> literal 0
> >> HcmV?d00001
> >>
> >> diff --git a/Platforms/Marvell/Marvell.dec
> b/Platforms/Marvell/Marvell.dec
> >> index e63add4..f9d2660 100644
> >> --- a/Platforms/Marvell/Marvell.dec
> >> +++ b/Platforms/Marvell/Marvell.dec
> >> @@ -54,6 +54,7 @@
> >>
> >>    gShellEepromHiiGuid = { 0xb2f4c714, 0x147f, 0x4ff7, { 0x82, 0x1b, 0xce,
> 0x7b, 0x91, 0x7f, 0x5f, 0x2f } }
> >>    gShellSfHiiGuid = { 0x03a67756, 0x8cde, 0x4638, { 0x82, 0x34, 0x4a, 0x0f,
> 0x6d, 0x58, 0x81, 0x39 } }
> >> +  gShellFUpdateHiiGuid = { 0x9b5d2176, 0x590a, 0x49db, { 0x89, 0x5d,
> 0x4a, 0x70, 0xfe, 0xad, 0xbe, 0x24 } }
> >>
> >>  [PcdsFixedAtBuild.common]
> >>  #MPP
> >> --
> >> 1.8.3.1
> >>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
  2016-11-16 17:05       ` Carsey, Jaben
@ 2016-11-16 17:35         ` Leif Lindholm
  2016-11-16 21:31           ` Carsey, Jaben
  0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2016-11-16 17:35 UTC (permalink / raw)
  To: Carsey, Jaben
  Cc: Marcin Wojtas, Ard Biesheuvel, linaro-uefi, Neta Zur Hershkovits,
	Yehuda Yitschak, Haim Boot, Jan Dabros, Bartosz Szczepanek,
	edk2-devel-01, Ni, Ruiyu

Hi Jaben,

I've seen a few different solutions to the same problem of providing
platform-specific "shorthand" commands, my least favourite one being
embedding a locally hacked EBL.

I guess we're ending up with something halfway between an application
and a script.

Is it your view that this should always be implemented as a standalone
application, to be called from the UI config menus?

Basically, is there an expected usage pattern that people keep
missing?

Regards,

Leif

On Wed, Nov 16, 2016 at 05:05:36PM +0000, Carsey, Jaben wrote:
> Marcin and Ard,
> 
> It sounds like there is a goal to have a UEFI Application call into an internal shell command.
> 
> There is no documented method to programmatically call an internal
> command. This is not a goal of the current shell
> implementation. ShellExecute explicitly does not allow running of
> internal commands (per UEFI Shell Specification 2.2).
> 
> Thus I don’t think using ShellDynamicCommandProtocol will help.
> That protocol is for allowing a driver add a command to the shell's
> list of available commands.
> 
> -Jaben
> 
> > -----Original Message-----
> > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > Sent: Wednesday, November 16, 2016 8:13 AM
> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: linaro-uefi <linaro-uefi@lists.linaro.org>; Leif Lindholm
> > <leif.lindholm@linaro.org>; Neta Zur Hershkovits <neta@marvell.com>;
> > Yehuda Yitschak <yehuday@marvell.com>; Haim Boot
> > <hayim@marvell.com>; Jan Dąbroś <jsd@semihalf.com>; Bartosz
> > Szczepanek <bsz@semihalf.com>; edk2-devel-01 <edk2-devel@lists.01.org>;
> > Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate'
> > comand to shell
> > Importance: High
> > 
> > + Jaben, Ruiyu and edk2 list
> > 
> > Hi Ard,
> > 
> > Internal Shell header ("../../../ShellPkg/Application/Shell/Shell.h")
> > was added ONLY for being able to execute shell commands (tftp and our
> > custom SPI flash control) from within a new command. This is now done
> > with:
> > RunShellCommand (TftpCmd, &Status);
> > And it works fine.
> > 
> > Replacing above with:
> > ShellExecute (&ImageHandle, TftpCmd, FALSE, NULL, &Status);
> > allow to get rid of relative include and seems more appropriate, but
> > there is completely no effect of calling it.
> > 
> > I checked in edk2 sources and see no relation between above problem
> > and registering our command with help of ShellDynamicCommand protocol.
> > Unless I don't know something. Anyway, I'll apreciate any hint, how to
> > solve the issue in a nice way.
> > 
> > Best regards,
> > Marcin
> > 
> > 2016-11-15 17:09 GMT+01:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> > > On 10 July 2016 at 00:21, Marcin Wojtas <mw@semihalf.com> wrote:
> > >> From: Jan Dąbroś <jsd@semihalf.com>
> > >>
> > >> 'fupdate' command performs updating firmware from file placed on local
> > >> filesystem or on TFTP server.
> > >> It uses tftp and sf commands.
> > >>
> > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > >> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> > >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > >
> > > Please use the Shell dynamic command protocol to add commands to the
> > > shell. This will remove the need for including internal Shell headers,
> > > and even allow your commands to work with prebuilt Shell binaries.
> > >
> > > MdePkg/Include/Protocol/ShellDynamicCommand.h
> > >
> > > Thanks,
> > > Ard.
> > >
> > >
> > >> ---
> > >>  Applications/FirmwareUpdate/FUpdate.c   | 448
> > ++++++++++++++++++++++++++++++++
> > >>  Applications/FirmwareUpdate/FUpdate.inf |  68 +++++
> > >>  Applications/FirmwareUpdate/FUpdate.uni | Bin 0 -> 5838 bytes
> > >>  Platforms/Marvell/Marvell.dec           |   1 +
> > >>  4 files changed, 517 insertions(+)
> > >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.c
> > >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.inf
> > >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.uni
> > >>
> > >> diff --git a/Applications/FirmwareUpdate/FUpdate.c
> > b/Applications/FirmwareUpdate/FUpdate.c
> > >> new file mode 100644
> > >> index 0000000..359a4ac
> > >> --- /dev/null
> > >> +++ b/Applications/FirmwareUpdate/FUpdate.c
> > >> @@ -0,0 +1,448 @@
> > >>
> > +/*********************************************************
> > **********************
> > >> +Copyright (C) 2016 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.
> > >> +
> > >>
> > +*********************************************************
> > **********************/
> > >> +#include <Uefi.h>
> > >> +#include <ShellBase.h>
> > >> +
> > >> +#include <Library/BaseLib.h>
> > >> +#include <Library/DebugLib.h>
> > >> +#include <Library/MemoryAllocationLib.h>
> > >> +#include <Library/ShellCommandLib.h>
> > >> +#include <Library/ShellLib.h>
> > >> +#include <Library/UefiLib.h>
> > >> +#include <Library/PrintLib.h>
> > >> +#include <Library/UefiLib.h>
> > >> +#include <Library/ShellCEntryLib.h>
> > >> +#include <Library/HiiLib.h>
> > >> +#include <Guid/ShellLibHiiGuid.h>
> > >> +#include <Library/UefiBootServicesTableLib.h>
> > >> +#include <Library/FileHandleLib.h>
> > >> +// FIXME: Remove including relative path
> > >> +#include "../../../ShellPkg/Application/Shell/Shell.h"
> > >> +
> > >> +#define CMD_NAME_STRING       L"fupdate"
> > >> +
> > >> +#define TFTP_CMD_STRING       L"tftp "
> > >> +#define SPACE_STRING          L" "
> > >> +#define SF_PROBE_CMD_STRING   L"sf probe"
> > >> +#define SF_WRITE_CMD_STRING   L"sf updatefile "
> > >> +#define SF_LOAD_ADDR_STRING   L"0x0"
> > >> +
> > >> +#define MAIN_HDR_MAGIC        0xB105B002
> > >> +
> > >> +typedef struct {
> > >> +  UINT32  Magic;              //  0-3
> > >> +  UINT32  PrologSize;         //  4-7
> > >> +  UINT32  PrologChecksum;     //  8-11
> > >> +  UINT32  BootImageSize;      // 12-15
> > >> +  UINT32  BootImageChecksum;  // 16-19
> > >> +  UINT32  Reserved0;          // 20-23
> > >> +  UINT32  LoadAddr;           // 24-27
> > >> +  UINT32  ExecAddr;           // 28-31
> > >> +  UINT8   UartConfig;         //  32
> > >> +  UINT8   Baudrate;           //  33
> > >> +  UINT8   ExtCount;           //  34
> > >> +  UINT8   AuxFlags;           //  35
> > >> +  UINT32  IoArg0;             // 36-39
> > >> +  UINT32  IoArg1;             // 40-43
> > >> +  UINT32  IoArg2;             // 43-47
> > >> +  UINT32  IoArg3;             // 48-51
> > >> +  UINT32  Reserved1;          // 52-55
> > >> +  UINT32  Reserved2;          // 56-59
> > >> +  UINT32  Reserved3;          // 60-63
> > >> +} MV_IMAGE_HEADER;
> > >> +
> > >> +STATIC
> > >> +UINT32
> > >> +CountChecksum (
> > >> +  UINT32 *Start,
> > >> +  UINT32 Length
> > >> +  )
> > >> +{
> > >> +  UINT32 Sum = 0;
> > >> +  UINT32 *Startp = Start;
> > >> +
> > >> +  do {
> > >> +    Sum += *Startp;
> > >> +    Startp++;
> > >> +    Length -= 4;
> > >> +  } while (Length > 0);
> > >> +
> > >> +  return Sum;
> > >> +}
> > >> +
> > >> +STATIC
> > >> +EFI_STATUS
> > >> +CheckImageHeader (
> > >> +  IN VOID *ImageHeader
> > >> +  )
> > >> +{
> > >> +  MV_IMAGE_HEADER *Header;
> > >> +  UINT32 HeaderLength, Checksum, ChecksumBackup;
> > >> +
> > >> +  Header = (MV_IMAGE_HEADER *) ImageHeader;
> > >> +  HeaderLength = Header->PrologSize;
> > >> +  ChecksumBackup = Header->PrologChecksum;
> > >> +
> > >> +  // Compare magic number
> > >> +  if (Header->Magic != MAIN_HDR_MAGIC) {
> > >> +    Print (L"%s: Bad Image magic 0x%08x != 0x%08x\n",
> > CMD_NAME_STRING,
> > >> +      Header->Magic, MAIN_HDR_MAGIC);
> > >> +    return EFI_DEVICE_ERROR;
> > >> +  }
> > >> +
> > >> +  // The checksum field is discarded from calculation
> > >> +  Header->PrologChecksum = 0;
> > >> +
> > >> +  Checksum = CountChecksum((UINT32 *)Header, HeaderLength);
> > >> +  if (Checksum != ChecksumBackup) {
> > >> +    Print (L"%s: Bad Image checksum. 0x%x != 0x%x\n",
> > CMD_NAME_STRING, Checksum,
> > >> +      ChecksumBackup);
> > >> +    return EFI_DEVICE_ERROR;
> > >> +  }
> > >> +
> > >> +  // Restore checksum backup
> > >> +  Header->PrologChecksum = ChecksumBackup;
> > >> +
> > >> +  return 0;
> > >> +}
> > >> +
> > >> +STATIC
> > >> +EFI_STATUS
> > >> +CheckFirmwareImage (
> > >> +  CONST CHAR16* FirmwareImage
> > >> +  )
> > >> +{
> > >> +  EFI_STATUS Status;
> > >> +  VOID *FileBuffer;
> > >> +  UINT64 OpenMode;
> > >> +  UINTN FileSize;
> > >> +  SHELL_FILE_HANDLE FileHandle = NULL;
> > >> +
> > >> +  OpenMode = EFI_FILE_MODE_READ;
> > >> +
> > >> +  Status = ShellOpenFileByName (FirmwareImage, &FileHandle,
> > OpenMode, 0);
> > >> +    if (EFI_ERROR (Status)) {
> > >> +      Print (L"%s: Cannot open Image file\n", CMD_NAME_STRING);
> > >> +      return EFI_DEVICE_ERROR;
> > >> +    }
> > >> +
> > >> +  Status = FileHandleGetSize (FileHandle, &FileSize);
> > >> +    if (EFI_ERROR (Status)) {
> > >> +      Print (L"%s: Cannot get Image file size\n", CMD_NAME_STRING);
> > >> +    }
> > >> +
> > >> +  FileBuffer = AllocateZeroPool (FileSize);
> > >> +
> > >> +  // Read Image header into buffer
> > >> +  Status = FileHandleRead (FileHandle, &FileSize, FileBuffer);
> > >> +    if (EFI_ERROR (Status)) {
> > >> +      Print (L"%s: Cannot read Image file header\n", CMD_NAME_STRING);
> > >> +      ShellCloseFile (&FileHandle);
> > >> +      FreePool (FileBuffer);
> > >> +      return EFI_DEVICE_ERROR;
> > >> +    }
> > >> +
> > >> +  Status = CheckImageHeader (FileBuffer);
> > >> +  if (EFI_ERROR(Status)) {
> > >> +    return EFI_DEVICE_ERROR;
> > >> +  }
> > >> +
> > >> +  FreePool (FileBuffer);
> > >> +
> > >> +  return EFI_SUCCESS;
> > >> +}
> > >> +
> > >> +STATIC
> > >> +CONST CHAR16 *
> > >> +FileNameFromFilePath (
> > >> +  CONST CHAR16 *RemoteFilePath
> > >> +  )
> > >> +{
> > >> +  CONST CHAR16 *Walker;
> > >> +
> > >> +  // Gather FileName from FilePath
> > >> +  Walker = RemoteFilePath + StrLen (RemoteFilePath);
> > >> +  while ((--Walker) >= RemoteFilePath) {
> > >> +    if ((*Walker == L'\\') || (*Walker == L'/')) {
> > >> +      break;
> > >> +    }
> > >> +  }
> > >> +
> > >> +  return (Walker + 1);
> > >> +}
> > >> +
> > >> +STATIC
> > >> +CONST CHAR16*
> > >> +PrepareFile (
> > >> +  LIST_ENTRY *CheckPackage
> > >> +  )
> > >> +{
> > >> +  EFI_STATUS Status;
> > >> +  CONST CHAR16  *ValueStr;
> > >> +
> > >> +  ValueStr = ShellCommandLineGetValue (CheckPackage, L"-f");
> > >> +  if (ValueStr == NULL) {
> > >> +    Print (L"%s: No LocalFilePath parameter!\n", CMD_NAME_STRING);
> > >> +    return NULL;
> > >> +  } else {
> > >> +    Status = ShellIsFile (ValueStr);
> > >> +    if (EFI_ERROR(Status)) {
> > >> +      Print (L"%s: Wrong LocalFilePath parameter!\n",
> > CMD_NAME_STRING);
> > >> +      return NULL;
> > >> +    }
> > >> +  }
> > >> +  return ValueStr;
> > >> +}
> > >> +
> > >> +CONST CHAR16 gShellFUpdateFileName[] = L"ShellCommand";
> > >> +EFI_HANDLE gShellFUpdateHiiHandle = NULL;
> > >> +EFI_HANDLE gShellFUpdateHiiHandle;
> > >> +
> > >> +STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> > >> +  {L"help", TypeFlag},
> > >> +  {L"-t", TypeFlag},
> > >> +  {L"-f", TypeValue},
> > >> +  {NULL , TypeMax}
> > >> +  };
> > >> +
> > >> +/**
> > >> +  Return the file name of the help text file if not using HII.
> > >> +
> > >> +  @return The string pointer to the file name.
> > >> +**/
> > >> +CONST CHAR16*
> > >> +EFIAPI
> > >> +ShellCommandGetManFileNameFUpdate (
> > >> +  VOID
> > >> +  )
> > >> +{
> > >> +
> > >> +  return gShellFUpdateFileName;
> > >> +}
> > >> +
> > >> +VOID
> > >> +FUpdateUsage (
> > >> +  VOID
> > >> +  )
> > >> +{
> > >> +  Print (L"\nFirmware update command\n"
> > >> +         "fupdate [-f <LocalFilePath>] [-t <Host> <RemoteFilePath>]\n\n"
> > >> +         "LocalFilePath  - path to local firmware image file\n"
> > >> +         "Host           - IP number of TFTP server\n"
> > >> +         "RemoteFilePath - path to firmware image file on TFTP server\n"
> > >> +         "Examples:\n"
> > >> +         "Update firmware from file fs2:Uefi.img\n"
> > >> +         "  fupdate -f fs2:Uefi.img\n"
> > >> +         "Update firmware from file path/Uefi.img TFTP server with IP "
> > >> +         "address 10.0.0.200\n"
> > >> +         "  fupdate -t 10.0.0.200 path/Uefi.img\n"
> > >> +  );
> > >> +}
> > >> +
> > >> +SHELL_STATUS
> > >> +EFIAPI
> > >> +ShellCommandRunFUpdate (
> > >> +  IN EFI_HANDLE        ImageHandle,
> > >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> > >> +  )
> > >> +{
> > >> +  EFI_STATUS    Status;
> > >> +  LIST_ENTRY    *CheckPackage;
> > >> +  CHAR16        *ProblemParam, *TftpCmd = NULL, *SfCmd = NULL;
> > >> +  CONST CHAR16  *RemoteFilePath, *Host, *FileToWrite;
> > >> +  UINT8         CmdLen;
> > >> +  BOOLEAN       FileFlag, TftpFlag;
> > >> +
> > >> +  Status = ShellInitialize ();
> > >> +  if (EFI_ERROR (Status)) {
> > >> +    Print (L"%s: Error while initializinf Shell\n", CMD_NAME_STRING);
> > >> +    ASSERT_EFI_ERROR (Status);
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  Status = ShellCommandLineParse (ParamList, &CheckPackage,
> > &ProblemParam,
> > >> +    TRUE);
> > >> +  if (EFI_ERROR (Status)) {
> > >> +    Print (L"Parse error!\n");
> > >> +  }
> > >> +
> > >> +  if (ShellCommandLineGetFlag (CheckPackage, L"help")) {
> > >> +    FUpdateUsage();
> > >> +    return EFI_SUCCESS;
> > >> +  }
> > >> +
> > >> +  FileFlag = ShellCommandLineGetFlag (CheckPackage, L"-f");
> > >> +  TftpFlag = ShellCommandLineGetFlag (CheckPackage, L"-t");
> > >> +
> > >> +  if (!FileFlag && !TftpFlag) {
> > >> +    Print (L"%s: Please specify -f or -t flag\n", CMD_NAME_STRING);
> > >> +    return SHELL_ABORTED;
> > >> +  } else if (FileFlag && !TftpFlag) {
> > >> +    // Prepare local file to be burned into flash
> > >> +    FileToWrite = PrepareFile (CheckPackage);
> > >> +    if (FileToWrite == NULL) {
> > >> +      Print (L"%s: Error while preparing file for burn\n",
> > CMD_NAME_STRING);
> > >> +      return SHELL_ABORTED;
> > >> +    }
> > >> +  } else if (TftpFlag && !FileFlag) {
> > >> +    Host = ShellCommandLineGetRawValue (CheckPackage, 1);
> > >> +    if (Host == NULL) {
> > >> +      Print (L"%s: No Host parameter!\n", CMD_NAME_STRING);
> > >> +      return SHELL_ABORTED;
> > >> +    }
> > >> +
> > >> +    RemoteFilePath = ShellCommandLineGetRawValue (CheckPackage, 2);
> > >> +    if (RemoteFilePath == NULL) {
> > >> +      Print (L"%s: No remote_file_path parameter!\n",
> > CMD_NAME_STRING);
> > >> +      return SHELL_ABORTED;
> > >> +    }
> > >> +
> > >> +    // Gather firmware image name from remote filepath
> > >> +    FileToWrite = FileNameFromFilePath (RemoteFilePath);
> > >> +
> > >> +    // Allocate buffer for tftp command string
> > >> +    CmdLen = StrSize (TFTP_CMD_STRING) + StrSize (SPACE_STRING) +
> > >> +      StrSize (Host) + StrSize (RemoteFilePath);
> > >> +    TftpCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
> > >> +    if (TftpCmd == NULL) {
> > >> +      Print (L"%s: Cannot allocate memory\n", CMD_NAME_STRING);
> > >> +      return SHELL_ABORTED;
> > >> +    }
> > >> +
> > >> +    // Concatenate parameters and form tftp command string
> > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), TFTP_CMD_STRING);
> > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)Host);
> > >> +    // Insert space
> > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
> > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16
> > *)RemoteFilePath);
> > >> +
> > >> +    RunShellCommand (TftpCmd, &Status);
> > >> +    FreePool (TftpCmd);
> > >> +    if (EFI_ERROR(Status)) {
> > >> +      Print (L"%s: Error while performing tftp command\n",
> > CMD_NAME_STRING);
> > >> +      return SHELL_ABORTED;
> > >> +    }
> > >> +
> > >> +  } else {
> > >> +    Print (L"%s: Both -f and -t flag specified, please choose one\n",
> > >> +      CMD_NAME_STRING);
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  // Check image checksum and magic
> > >> +  Status = CheckFirmwareImage (FileToWrite);
> > >> +  if (EFI_ERROR(Status)) {
> > >> +    Print (L"%s: Wrong firmware Image\n", CMD_NAME_STRING);
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  // Probe spi bus
> > >> +  RunShellCommand (SF_PROBE_CMD_STRING, &Status);
> > >> +  if (EFI_ERROR(Status)) {
> > >> +    Print (L"%s: Error while performing sf probe\n",
> > CMD_NAME_STRING);
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  // Allocate buffer for sf command string
> > >> +  CmdLen = StrSize (SF_WRITE_CMD_STRING) + StrSize (FileToWrite) +
> > >> +    StrSize (SPACE_STRING) + StrSize (SF_LOAD_ADDR_STRING);
> > >> +  SfCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
> > >> +  if (SfCmd == NULL) {
> > >> +    Print (L"%s: Cannot allocate memory\n");
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  // Concatenate parameters and form command string
> > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SF_WRITE_CMD_STRING);
> > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)FileToWrite);
> > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
> > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SF_LOAD_ADDR_STRING);
> > >> +
> > >> +  // Update firmware image in flash
> > >> +  RunShellCommand (SfCmd, &Status);
> > >> +  FreePool (SfCmd);
> > >> +  if (EFI_ERROR(Status)) {
> > >> +    Print (L"%s: Error while performing sf update\n",
> > CMD_NAME_STRING);
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  return EFI_SUCCESS;
> > >> +}
> > >> +
> > >> +EFI_STATUS
> > >> +EFIAPI
> > >> +ShellFUpdateCommandConstructor (
> > >> +  IN EFI_HANDLE        ImageHandle,
> > >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> > >> +  )
> > >> +{
> > >> +  EFI_STATUS Status;
> > >> +
> > >> +  gShellFUpdateHiiHandle = NULL;
> > >> +
> > >> +  gShellFUpdateHiiHandle = HiiAddPackages (
> > >> +                          &gShellFUpdateHiiGuid, gImageHandle,
> > >> +                          UefiShellFUpdateCommandLibStrings, NULL
> > >> +                          );
> > >> +  if (gShellFUpdateHiiHandle == NULL) {
> > >> +    Print (L"%s: Cannot add Hii package\n", CMD_NAME_STRING);
> > >> +    return EFI_DEVICE_ERROR;
> > >> +  }
> > >> +
> > >> +  Status = ShellCommandRegisterCommandName (
> > >> +     CMD_NAME_STRING, ShellCommandRunFUpdate,
> > ShellCommandGetManFileNameFUpdate,
> > >> +     0, CMD_NAME_STRING, TRUE , gShellFUpdateHiiHandle,
> > >> +     STRING_TOKEN (STR_GET_HELP_FUPDATE)
> > >> +     );
> > >> +  if (EFI_ERROR(Status)) {
> > >> +    Print (L"%s: Error while registering command\n",
> > CMD_NAME_STRING);
> > >> +    return SHELL_ABORTED;
> > >> +  }
> > >> +
> > >> +  return EFI_SUCCESS;
> > >> +}
> > >> +
> > >> +EFI_STATUS
> > >> +EFIAPI
> > >> +ShellFUpdateCommandDestructor (
> > >> +  IN EFI_HANDLE        ImageHandle,
> > >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> > >> +  )
> > >> +{
> > >> +
> > >> +  if (gShellFUpdateHiiHandle != NULL) {
> > >> +    HiiRemovePackages (gShellFUpdateHiiHandle);
> > >> +  }
> > >> +  return EFI_SUCCESS;
> > >> +}
> > >> diff --git a/Applications/FirmwareUpdate/FUpdate.inf
> > b/Applications/FirmwareUpdate/FUpdate.inf
> > >> new file mode 100644
> > >> index 0000000..53b5305
> > >> --- /dev/null
> > >> +++ b/Applications/FirmwareUpdate/FUpdate.inf
> > >> @@ -0,0 +1,68 @@
> > >> +#
> > >> +# 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]
> > >> +  INF_VERSION = 0x00010006
> > >> +  BASE_NAME = UefiShellFUpdateCommandLib
> > >> +  FILE_GUID = 470292b2-926b-4ed8-8080-be7a260db627
> > >> +  MODULE_TYPE = UEFI_APPLICATION
> > >> +  VERSION_STRING = 0.1
> > >> +  LIBRARY_CLASS = NULL|UEFI_APPLICATION UEFI_DRIVER
> > >> +  CONSTRUCTOR = ShellFUpdateCommandConstructor
> > >> +  DESTRUCTOR = ShellFUpdateCommandDestructor
> > >> +
> > >> +[Sources]
> > >> +  FUpdate.c
> > >> +  FUpdate.uni
> > >> +
> > >> +[Packages]
> > >> +  MdePkg/MdePkg.dec
> > >> +  ShellPkg/ShellPkg.dec
> > >> +  MdeModulePkg/MdeModulePkg.dec
> > >> +  OpenPlatformPkg/Platforms/Marvell/Marvell.dec
> > >> +
> > >> +[LibraryClasses]
> > >> +  UefiLib
> > >> +  UefiBootServicesTableLib
> > >> +  MemoryAllocationLib
> > >> +  BaseLib
> > >> +  BaseMemoryLib
> > >> +  DebugLib
> > >> +  ShellCommandLib
> > >> +  ShellLib
> > >> +  UefiLib
> > >> +  UefiRuntimeServicesTableLib
> > >> +  PcdLib
> > >> +  HiiLib
> > >> +  FileHandleLib
> > >> +
> > >> +[Guids]
> > >> +  gShellFUpdateHiiGuid
> > >> diff --git a/Applications/FirmwareUpdate/FUpdate.uni
> > b/Applications/FirmwareUpdate/FUpdate.uni
> > >> new file mode 100644
> > >> index
> > 0000000000000000000000000000000000000000..6143c0580f1d1ef8ce4fe619a1
> > df1ce380adf2c0
> > >> GIT binary patch
> > >> literal 5838
> > >>
> > zcmd6r?N1v=5XSd&rTz~m@+FOuK&z@BXdB5G6OhbHeI_(&s)*P?EX6>LDdL}
> > R`}}4Y
> > >> z_Fim)B2|h`_TBEy&OY<Z%kJL4|E#8GU-
> > k8E`X+VLMY>G4X_hAGEPYJ#RHUcs{Z98E
> > >>
> > z(pvgH{iLVebS#O#(@FZIt4KTPT#_g1JJ*}J#$MPH@A_(w)60Z*e$BBPsZ5(DH%
> > TMu
> > >> zI8RslFVenngr*p~lTP*KR@$$1pGl4p6GJjq&s>Nn8egO>-
> > 9MS0Q^{SVsru?OKUYs^
> > >> znhN!;+Src8b3GNB10={X)7Ui6^*l)*^bUIqjXT$yRmsoHx~ZO!JT-
> > lb@LbynxN^K`
> > >> zIn^kbyht}1doJb+wK5;k=NwOk%lRBE#O3S<wy>ix&4tFo-
> > prWGG9T<MUWEtn%*5H1
> > >>
> > zSq~;Gfgeyu8$Ga<Z3c1n|8rvmo!EG(e{`Y?JMh<~dM@O>iCW&nJZN~Bn~_Gr
> > JG$Ij
> > >>
> > zd>%7RrE8wn?<20zyXy5!t%sOH*M&t|ohxPqCl(VMpv1PhX2wGq^RXfXyO3p
> > 6Go#Mc
> > >> z2Xdm<S*qfT=Nvu6g3sr;me~&{8O5Tg@+clDy1)!mvr2Bp%yEAVQ*Qa<_-
> > Qib{vL}x
> > >>
> > zEYC<E=8NnPMAdtpMc3xp|Nrt8nS_5%EXt~=;(06|k7OwvQyp_&MAmuLAl6(
> > HxkV-N
> > >> zET8D<lf1qtOCkd^gYD|%d6DaG;EFrG-
> > 4iy??Rq_;ii11ypk6r(+2fVKanpABoH=Nv
> > >> z<&j#gnvCR4)Z7mbaOf4-W15N#lkkcLM+avfRa7Cb$<-
> > )x6l5)Rn~X$TUK4VTk?W_@
> > >>
> > za3fp1Vg<`y=axm`l~uNpw4drXy2AteUMrc`D;^ahSHUaU(DczTZEGzpC0A<w+
> > e*W9
> > >> zlsc+aJ@t71fm-|Njar-P-BkZZdaHj%BGuK~1N96f@mhUD^=+%KZ}J1v-
> > I3oJiR_*v
> > >> zkljd!dfL}Bv#)4Y?##KOyN`eYo|vzfe%Cx8D@~G-
> > $nUDRue)P&^aC3;wBFKIlpSg5
> > >>
> > zO72iNcz$iYj)AD$H{9si)o*yG4ScM|VQYOh&@Gyeoy3+#?Mll)vu@v^>B#0C!#
> > XsJ
> > >>
> > zf|V_saqr%;En@`^3fYD?u%s*OeQVKssNMs~l^TU4ynqMmO0%@1ClG9^+Q
> > QSG{;?A#
> > >> z-{^-
> > Q@H*)Fy0VpvJi0GSK?vv0{BmDzNRD0Djx=xRPRnho`V*v}?g|GSgK$w_S8s?s
> > >> z@PZWCQnoXa8N7$+=-
> > tHvY`itODpHvGO}x1#A6ZCqN25B52jU8RPYubJ$BO3*-oiRO
> > >> z;7of=l<H^yiARTeCejMczOQRj{lPA12bOhRJ(cNa!`7EnL_ZOBs8-
> > J`#SZv^9k<$X
> > >>
> > zcx*8~`ic3{Y>Z5Td2kGL9g117k*Dmy$>bxwBVzD<S9AER<a0zY(pBGjzJP<wb;~
> > 0j
> > >>
> > zat46)NHg>_BmbVy#A%ML&V78q8Fhnr$<bBU=9YU^87}oEdC(K*Aj0od2E5>-
> > sRWMZ
> > >>
> > zNblhn9*8MgUOOV6qBc<l+)_Lm7Dv@^zN)=wg0E)Hg^|ck=D^1t#Rb(DByjF*
> > hHkF`
> > >> z;HuY_9BtH}dYtDK`s+R&t{O-
> > 1w3>cRPwrb!Y%j@~g;kdG85b+{3yV!w6Iy3>qPfzZ
> > >> zX{9ojT`eo5pLsm0-dh|D(|hxLUpeqz+>;BQS42nq$~dmIf}d&5CJLW-MHLu-
> > u?x7?
> > >> zdOVi2?;F<Cx1yOQLNu-
> > H0}g9a>}aO8dtoo~QX}3;{+%qy`l;i@HPx)wdSBPauAx3(
> > >> z>^nf%KE)1&a40S~$+4?pPs1L58H!T5z)Evq`BJvzidIjPmp|l5{Q8S>lszr`Qdr`A
> > >>
> > zePHZ;Os~?Pl0b59lG~ERx38?lX)E?;i$*TP4(h0Xk3mjUJT#X_%?n8_X0gin<(*~h
> > >>
> > zvDtgF@4S(hIi(+~mG|D+OX0buJ?&^+)>Dm0=76ngbhQ19reJ%S)<S+^oo4sxJJ
> > nmY
> > >> zGY6%`kVg`g?m$uXqa?6%tocvmOTIZQvTJ!v1RCa!I0-
> > gk(Id$@uiD7@iqnDTaIo8+
> > >> zyV>(n7Ls@DKFB<FqO7V-
> > xpu@iN36HTd@g76i6Zb~*Ok7Rt*dfT7boHmyUtp=;i;K(
> > >> zo^g-
> > W@FnOXzE@lLs*zExG5O1n$4E0i3Vj`1la=w~d*Rw6t)*%#8qJA``utI=&gXax
> > >> z|FhF$H#bn-d!tC;obk9gsE#aZW7`pd*NC^C^!K$wv6t-H-o^KX5&g~k(7qbL-
> > AH!%
> > >> z2@qzF6)k>Vp2}uwYkVd1H)P>oTCvDQf3r9rtC-
> > mFvyX}sR8P6g7q^*ax)F|8UHrt%
> > >> QHyJ;b`f6NX>FQbZ9}*T`00000
> > >>
> > >> literal 0
> > >> HcmV?d00001
> > >>
> > >> diff --git a/Platforms/Marvell/Marvell.dec
> > b/Platforms/Marvell/Marvell.dec
> > >> index e63add4..f9d2660 100644
> > >> --- a/Platforms/Marvell/Marvell.dec
> > >> +++ b/Platforms/Marvell/Marvell.dec
> > >> @@ -54,6 +54,7 @@
> > >>
> > >>    gShellEepromHiiGuid = { 0xb2f4c714, 0x147f, 0x4ff7, { 0x82, 0x1b, 0xce,
> > 0x7b, 0x91, 0x7f, 0x5f, 0x2f } }
> > >>    gShellSfHiiGuid = { 0x03a67756, 0x8cde, 0x4638, { 0x82, 0x34, 0x4a, 0x0f,
> > 0x6d, 0x58, 0x81, 0x39 } }
> > >> +  gShellFUpdateHiiGuid = { 0x9b5d2176, 0x590a, 0x49db, { 0x89, 0x5d,
> > 0x4a, 0x70, 0xfe, 0xad, 0xbe, 0x24 } }
> > >>
> > >>  [PcdsFixedAtBuild.common]
> > >>  #MPP
> > >> --
> > >> 1.8.3.1
> > >>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
  2016-11-16 17:35         ` Leif Lindholm
@ 2016-11-16 21:31           ` Carsey, Jaben
  2016-11-16 21:48             ` Marcin Wojtas
  0 siblings, 1 reply; 7+ messages in thread
From: Carsey, Jaben @ 2016-11-16 21:31 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: Marcin Wojtas, Ard Biesheuvel, linaro-uefi, Neta Zur Hershkovits,
	Yehuda Yitschak, Haim Boot, Jan Dabros, Bartosz Szczepanek,
	edk2-devel-01, Ni, Ruiyu, Carsey, Jaben

Leif,

I agree on EBL, but I have very little experience with EBL so I don’t want to discuss in detail as I am not the right person without more research.  Specifically, my gut reaction is that needing a platform specific boot loader indicates that something has already gone wrong on that platform.

However, this does not seem like a boot loader or an application at all.  this is an internal shell command. The goal here seems to be to create a NULL library to add a new internal command to the UEFI Shell.  This library gets compiled/linked into the shell itself.

I feel that we have found a "new" use case that I encountered, but worked around in the past because all previous cases involved commands in the same library (there are interactions between Reconnect and Disconnect/Connect).

I would say that a new API in the ShellCommandLib that links the UEFI Shell Application to the NULL libraries that make up the internal commands would be my first choice for implementation.  I would lean to something like the function that Marcin already called.  Maybe this?

EFIAPI
RunRegisteredCommand(
  CHAR16* CommandLine, 
  EFI_STATUS *CommandReturnValue
)

-Jaben

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, November 16, 2016 9:36 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>
> Cc: Marcin Wojtas <mw@semihalf.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; linaro-uefi <linaro-uefi@lists.linaro.org>; Neta
> Zur Hershkovits <neta@marvell.com>; Yehuda Yitschak
> <yehuday@marvell.com>; Haim Boot <hayim@marvell.com>; Jan Dabros
> <jsd@semihalf.com>; Bartosz Szczepanek <bsz@semihalf.com>; edk2-devel-
> 01 <edk2-devel@lists.01.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate'
> comand to shell
> Importance: High
> 
> Hi Jaben,
> 
> I've seen a few different solutions to the same problem of providing
> platform-specific "shorthand" commands, my least favourite one being
> embedding a locally hacked EBL.
> 
> I guess we're ending up with something halfway between an application
> and a script.
> 
> Is it your view that this should always be implemented as a standalone
> application, to be called from the UI config menus?
> 
> Basically, is there an expected usage pattern that people keep
> missing?
> 
> Regards,
> 
> Leif
> 
> On Wed, Nov 16, 2016 at 05:05:36PM +0000, Carsey, Jaben wrote:
> > Marcin and Ard,
> >
> > It sounds like there is a goal to have a UEFI Application call into an internal
> shell command.
> >
> > There is no documented method to programmatically call an internal
> > command. This is not a goal of the current shell
> > implementation. ShellExecute explicitly does not allow running of
> > internal commands (per UEFI Shell Specification 2.2).
> >
> > Thus I don’t think using ShellDynamicCommandProtocol will help.
> > That protocol is for allowing a driver add a command to the shell's
> > list of available commands.
> >
> > -Jaben
> >
> > > -----Original Message-----
> > > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > > Sent: Wednesday, November 16, 2016 8:13 AM
> > > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: linaro-uefi <linaro-uefi@lists.linaro.org>; Leif Lindholm
> > > <leif.lindholm@linaro.org>; Neta Zur Hershkovits <neta@marvell.com>;
> > > Yehuda Yitschak <yehuday@marvell.com>; Haim Boot
> > > <hayim@marvell.com>; Jan Dąbroś <jsd@semihalf.com>; Bartosz
> > > Szczepanek <bsz@semihalf.com>; edk2-devel-01 <edk2-
> devel@lists.01.org>;
> > > Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > > Subject: Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add
> 'fupdate'
> > > comand to shell
> > > Importance: High
> > >
> > > + Jaben, Ruiyu and edk2 list
> > >
> > > Hi Ard,
> > >
> > > Internal Shell header ("../../../ShellPkg/Application/Shell/Shell.h")
> > > was added ONLY for being able to execute shell commands (tftp and our
> > > custom SPI flash control) from within a new command. This is now done
> > > with:
> > > RunShellCommand (TftpCmd, &Status);
> > > And it works fine.
> > >
> > > Replacing above with:
> > > ShellExecute (&ImageHandle, TftpCmd, FALSE, NULL, &Status);
> > > allow to get rid of relative include and seems more appropriate, but
> > > there is completely no effect of calling it.
> > >
> > > I checked in edk2 sources and see no relation between above problem
> > > and registering our command with help of ShellDynamicCommand
> protocol.
> > > Unless I don't know something. Anyway, I'll apreciate any hint, how to
> > > solve the issue in a nice way.
> > >
> > > Best regards,
> > > Marcin
> > >
> > > 2016-11-15 17:09 GMT+01:00 Ard Biesheuvel
> <ard.biesheuvel@linaro.org>:
> > > > On 10 July 2016 at 00:21, Marcin Wojtas <mw@semihalf.com> wrote:
> > > >> From: Jan Dąbroś <jsd@semihalf.com>
> > > >>
> > > >> 'fupdate' command performs updating firmware from file placed on
> local
> > > >> filesystem or on TFTP server.
> > > >> It uses tftp and sf commands.
> > > >>
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> > > >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > >
> > > > Please use the Shell dynamic command protocol to add commands to
> the
> > > > shell. This will remove the need for including internal Shell headers,
> > > > and even allow your commands to work with prebuilt Shell binaries.
> > > >
> > > > MdePkg/Include/Protocol/ShellDynamicCommand.h
> > > >
> > > > Thanks,
> > > > Ard.
> > > >
> > > >
> > > >> ---
> > > >>  Applications/FirmwareUpdate/FUpdate.c   | 448
> > > ++++++++++++++++++++++++++++++++
> > > >>  Applications/FirmwareUpdate/FUpdate.inf |  68 +++++
> > > >>  Applications/FirmwareUpdate/FUpdate.uni | Bin 0 -> 5838 bytes
> > > >>  Platforms/Marvell/Marvell.dec           |   1 +
> > > >>  4 files changed, 517 insertions(+)
> > > >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.c
> > > >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.inf
> > > >>  create mode 100644 Applications/FirmwareUpdate/FUpdate.uni
> > > >>
> > > >> diff --git a/Applications/FirmwareUpdate/FUpdate.c
> > > b/Applications/FirmwareUpdate/FUpdate.c
> > > >> new file mode 100644
> > > >> index 0000000..359a4ac
> > > >> --- /dev/null
> > > >> +++ b/Applications/FirmwareUpdate/FUpdate.c
> > > >> @@ -0,0 +1,448 @@
> > > >>
> > >
> +/*********************************************************
> > > **********************
> > > >> +Copyright (C) 2016 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.
> > > >> +
> > > >>
> > >
> +*********************************************************
> > > **********************/
> > > >> +#include <Uefi.h>
> > > >> +#include <ShellBase.h>
> > > >> +
> > > >> +#include <Library/BaseLib.h>
> > > >> +#include <Library/DebugLib.h>
> > > >> +#include <Library/MemoryAllocationLib.h>
> > > >> +#include <Library/ShellCommandLib.h>
> > > >> +#include <Library/ShellLib.h>
> > > >> +#include <Library/UefiLib.h>
> > > >> +#include <Library/PrintLib.h>
> > > >> +#include <Library/UefiLib.h>
> > > >> +#include <Library/ShellCEntryLib.h>
> > > >> +#include <Library/HiiLib.h>
> > > >> +#include <Guid/ShellLibHiiGuid.h>
> > > >> +#include <Library/UefiBootServicesTableLib.h>
> > > >> +#include <Library/FileHandleLib.h>
> > > >> +// FIXME: Remove including relative path
> > > >> +#include "../../../ShellPkg/Application/Shell/Shell.h"
> > > >> +
> > > >> +#define CMD_NAME_STRING       L"fupdate"
> > > >> +
> > > >> +#define TFTP_CMD_STRING       L"tftp "
> > > >> +#define SPACE_STRING          L" "
> > > >> +#define SF_PROBE_CMD_STRING   L"sf probe"
> > > >> +#define SF_WRITE_CMD_STRING   L"sf updatefile "
> > > >> +#define SF_LOAD_ADDR_STRING   L"0x0"
> > > >> +
> > > >> +#define MAIN_HDR_MAGIC        0xB105B002
> > > >> +
> > > >> +typedef struct {
> > > >> +  UINT32  Magic;              //  0-3
> > > >> +  UINT32  PrologSize;         //  4-7
> > > >> +  UINT32  PrologChecksum;     //  8-11
> > > >> +  UINT32  BootImageSize;      // 12-15
> > > >> +  UINT32  BootImageChecksum;  // 16-19
> > > >> +  UINT32  Reserved0;          // 20-23
> > > >> +  UINT32  LoadAddr;           // 24-27
> > > >> +  UINT32  ExecAddr;           // 28-31
> > > >> +  UINT8   UartConfig;         //  32
> > > >> +  UINT8   Baudrate;           //  33
> > > >> +  UINT8   ExtCount;           //  34
> > > >> +  UINT8   AuxFlags;           //  35
> > > >> +  UINT32  IoArg0;             // 36-39
> > > >> +  UINT32  IoArg1;             // 40-43
> > > >> +  UINT32  IoArg2;             // 43-47
> > > >> +  UINT32  IoArg3;             // 48-51
> > > >> +  UINT32  Reserved1;          // 52-55
> > > >> +  UINT32  Reserved2;          // 56-59
> > > >> +  UINT32  Reserved3;          // 60-63
> > > >> +} MV_IMAGE_HEADER;
> > > >> +
> > > >> +STATIC
> > > >> +UINT32
> > > >> +CountChecksum (
> > > >> +  UINT32 *Start,
> > > >> +  UINT32 Length
> > > >> +  )
> > > >> +{
> > > >> +  UINT32 Sum = 0;
> > > >> +  UINT32 *Startp = Start;
> > > >> +
> > > >> +  do {
> > > >> +    Sum += *Startp;
> > > >> +    Startp++;
> > > >> +    Length -= 4;
> > > >> +  } while (Length > 0);
> > > >> +
> > > >> +  return Sum;
> > > >> +}
> > > >> +
> > > >> +STATIC
> > > >> +EFI_STATUS
> > > >> +CheckImageHeader (
> > > >> +  IN VOID *ImageHeader
> > > >> +  )
> > > >> +{
> > > >> +  MV_IMAGE_HEADER *Header;
> > > >> +  UINT32 HeaderLength, Checksum, ChecksumBackup;
> > > >> +
> > > >> +  Header = (MV_IMAGE_HEADER *) ImageHeader;
> > > >> +  HeaderLength = Header->PrologSize;
> > > >> +  ChecksumBackup = Header->PrologChecksum;
> > > >> +
> > > >> +  // Compare magic number
> > > >> +  if (Header->Magic != MAIN_HDR_MAGIC) {
> > > >> +    Print (L"%s: Bad Image magic 0x%08x != 0x%08x\n",
> > > CMD_NAME_STRING,
> > > >> +      Header->Magic, MAIN_HDR_MAGIC);
> > > >> +    return EFI_DEVICE_ERROR;
> > > >> +  }
> > > >> +
> > > >> +  // The checksum field is discarded from calculation
> > > >> +  Header->PrologChecksum = 0;
> > > >> +
> > > >> +  Checksum = CountChecksum((UINT32 *)Header, HeaderLength);
> > > >> +  if (Checksum != ChecksumBackup) {
> > > >> +    Print (L"%s: Bad Image checksum. 0x%x != 0x%x\n",
> > > CMD_NAME_STRING, Checksum,
> > > >> +      ChecksumBackup);
> > > >> +    return EFI_DEVICE_ERROR;
> > > >> +  }
> > > >> +
> > > >> +  // Restore checksum backup
> > > >> +  Header->PrologChecksum = ChecksumBackup;
> > > >> +
> > > >> +  return 0;
> > > >> +}
> > > >> +
> > > >> +STATIC
> > > >> +EFI_STATUS
> > > >> +CheckFirmwareImage (
> > > >> +  CONST CHAR16* FirmwareImage
> > > >> +  )
> > > >> +{
> > > >> +  EFI_STATUS Status;
> > > >> +  VOID *FileBuffer;
> > > >> +  UINT64 OpenMode;
> > > >> +  UINTN FileSize;
> > > >> +  SHELL_FILE_HANDLE FileHandle = NULL;
> > > >> +
> > > >> +  OpenMode = EFI_FILE_MODE_READ;
> > > >> +
> > > >> +  Status = ShellOpenFileByName (FirmwareImage, &FileHandle,
> > > OpenMode, 0);
> > > >> +    if (EFI_ERROR (Status)) {
> > > >> +      Print (L"%s: Cannot open Image file\n", CMD_NAME_STRING);
> > > >> +      return EFI_DEVICE_ERROR;
> > > >> +    }
> > > >> +
> > > >> +  Status = FileHandleGetSize (FileHandle, &FileSize);
> > > >> +    if (EFI_ERROR (Status)) {
> > > >> +      Print (L"%s: Cannot get Image file size\n", CMD_NAME_STRING);
> > > >> +    }
> > > >> +
> > > >> +  FileBuffer = AllocateZeroPool (FileSize);
> > > >> +
> > > >> +  // Read Image header into buffer
> > > >> +  Status = FileHandleRead (FileHandle, &FileSize, FileBuffer);
> > > >> +    if (EFI_ERROR (Status)) {
> > > >> +      Print (L"%s: Cannot read Image file header\n",
> CMD_NAME_STRING);
> > > >> +      ShellCloseFile (&FileHandle);
> > > >> +      FreePool (FileBuffer);
> > > >> +      return EFI_DEVICE_ERROR;
> > > >> +    }
> > > >> +
> > > >> +  Status = CheckImageHeader (FileBuffer);
> > > >> +  if (EFI_ERROR(Status)) {
> > > >> +    return EFI_DEVICE_ERROR;
> > > >> +  }
> > > >> +
> > > >> +  FreePool (FileBuffer);
> > > >> +
> > > >> +  return EFI_SUCCESS;
> > > >> +}
> > > >> +
> > > >> +STATIC
> > > >> +CONST CHAR16 *
> > > >> +FileNameFromFilePath (
> > > >> +  CONST CHAR16 *RemoteFilePath
> > > >> +  )
> > > >> +{
> > > >> +  CONST CHAR16 *Walker;
> > > >> +
> > > >> +  // Gather FileName from FilePath
> > > >> +  Walker = RemoteFilePath + StrLen (RemoteFilePath);
> > > >> +  while ((--Walker) >= RemoteFilePath) {
> > > >> +    if ((*Walker == L'\\') || (*Walker == L'/')) {
> > > >> +      break;
> > > >> +    }
> > > >> +  }
> > > >> +
> > > >> +  return (Walker + 1);
> > > >> +}
> > > >> +
> > > >> +STATIC
> > > >> +CONST CHAR16*
> > > >> +PrepareFile (
> > > >> +  LIST_ENTRY *CheckPackage
> > > >> +  )
> > > >> +{
> > > >> +  EFI_STATUS Status;
> > > >> +  CONST CHAR16  *ValueStr;
> > > >> +
> > > >> +  ValueStr = ShellCommandLineGetValue (CheckPackage, L"-f");
> > > >> +  if (ValueStr == NULL) {
> > > >> +    Print (L"%s: No LocalFilePath parameter!\n",
> CMD_NAME_STRING);
> > > >> +    return NULL;
> > > >> +  } else {
> > > >> +    Status = ShellIsFile (ValueStr);
> > > >> +    if (EFI_ERROR(Status)) {
> > > >> +      Print (L"%s: Wrong LocalFilePath parameter!\n",
> > > CMD_NAME_STRING);
> > > >> +      return NULL;
> > > >> +    }
> > > >> +  }
> > > >> +  return ValueStr;
> > > >> +}
> > > >> +
> > > >> +CONST CHAR16 gShellFUpdateFileName[] = L"ShellCommand";
> > > >> +EFI_HANDLE gShellFUpdateHiiHandle = NULL;
> > > >> +EFI_HANDLE gShellFUpdateHiiHandle;
> > > >> +
> > > >> +STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> > > >> +  {L"help", TypeFlag},
> > > >> +  {L"-t", TypeFlag},
> > > >> +  {L"-f", TypeValue},
> > > >> +  {NULL , TypeMax}
> > > >> +  };
> > > >> +
> > > >> +/**
> > > >> +  Return the file name of the help text file if not using HII.
> > > >> +
> > > >> +  @return The string pointer to the file name.
> > > >> +**/
> > > >> +CONST CHAR16*
> > > >> +EFIAPI
> > > >> +ShellCommandGetManFileNameFUpdate (
> > > >> +  VOID
> > > >> +  )
> > > >> +{
> > > >> +
> > > >> +  return gShellFUpdateFileName;
> > > >> +}
> > > >> +
> > > >> +VOID
> > > >> +FUpdateUsage (
> > > >> +  VOID
> > > >> +  )
> > > >> +{
> > > >> +  Print (L"\nFirmware update command\n"
> > > >> +         "fupdate [-f <LocalFilePath>] [-t <Host>
> <RemoteFilePath>]\n\n"
> > > >> +         "LocalFilePath  - path to local firmware image file\n"
> > > >> +         "Host           - IP number of TFTP server\n"
> > > >> +         "RemoteFilePath - path to firmware image file on TFTP server\n"
> > > >> +         "Examples:\n"
> > > >> +         "Update firmware from file fs2:Uefi.img\n"
> > > >> +         "  fupdate -f fs2:Uefi.img\n"
> > > >> +         "Update firmware from file path/Uefi.img TFTP server with IP "
> > > >> +         "address 10.0.0.200\n"
> > > >> +         "  fupdate -t 10.0.0.200 path/Uefi.img\n"
> > > >> +  );
> > > >> +}
> > > >> +
> > > >> +SHELL_STATUS
> > > >> +EFIAPI
> > > >> +ShellCommandRunFUpdate (
> > > >> +  IN EFI_HANDLE        ImageHandle,
> > > >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> > > >> +  )
> > > >> +{
> > > >> +  EFI_STATUS    Status;
> > > >> +  LIST_ENTRY    *CheckPackage;
> > > >> +  CHAR16        *ProblemParam, *TftpCmd = NULL, *SfCmd = NULL;
> > > >> +  CONST CHAR16  *RemoteFilePath, *Host, *FileToWrite;
> > > >> +  UINT8         CmdLen;
> > > >> +  BOOLEAN       FileFlag, TftpFlag;
> > > >> +
> > > >> +  Status = ShellInitialize ();
> > > >> +  if (EFI_ERROR (Status)) {
> > > >> +    Print (L"%s: Error while initializinf Shell\n", CMD_NAME_STRING);
> > > >> +    ASSERT_EFI_ERROR (Status);
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  Status = ShellCommandLineParse (ParamList, &CheckPackage,
> > > &ProblemParam,
> > > >> +    TRUE);
> > > >> +  if (EFI_ERROR (Status)) {
> > > >> +    Print (L"Parse error!\n");
> > > >> +  }
> > > >> +
> > > >> +  if (ShellCommandLineGetFlag (CheckPackage, L"help")) {
> > > >> +    FUpdateUsage();
> > > >> +    return EFI_SUCCESS;
> > > >> +  }
> > > >> +
> > > >> +  FileFlag = ShellCommandLineGetFlag (CheckPackage, L"-f");
> > > >> +  TftpFlag = ShellCommandLineGetFlag (CheckPackage, L"-t");
> > > >> +
> > > >> +  if (!FileFlag && !TftpFlag) {
> > > >> +    Print (L"%s: Please specify -f or -t flag\n", CMD_NAME_STRING);
> > > >> +    return SHELL_ABORTED;
> > > >> +  } else if (FileFlag && !TftpFlag) {
> > > >> +    // Prepare local file to be burned into flash
> > > >> +    FileToWrite = PrepareFile (CheckPackage);
> > > >> +    if (FileToWrite == NULL) {
> > > >> +      Print (L"%s: Error while preparing file for burn\n",
> > > CMD_NAME_STRING);
> > > >> +      return SHELL_ABORTED;
> > > >> +    }
> > > >> +  } else if (TftpFlag && !FileFlag) {
> > > >> +    Host = ShellCommandLineGetRawValue (CheckPackage, 1);
> > > >> +    if (Host == NULL) {
> > > >> +      Print (L"%s: No Host parameter!\n", CMD_NAME_STRING);
> > > >> +      return SHELL_ABORTED;
> > > >> +    }
> > > >> +
> > > >> +    RemoteFilePath = ShellCommandLineGetRawValue
> (CheckPackage, 2);
> > > >> +    if (RemoteFilePath == NULL) {
> > > >> +      Print (L"%s: No remote_file_path parameter!\n",
> > > CMD_NAME_STRING);
> > > >> +      return SHELL_ABORTED;
> > > >> +    }
> > > >> +
> > > >> +    // Gather firmware image name from remote filepath
> > > >> +    FileToWrite = FileNameFromFilePath (RemoteFilePath);
> > > >> +
> > > >> +    // Allocate buffer for tftp command string
> > > >> +    CmdLen = StrSize (TFTP_CMD_STRING) + StrSize (SPACE_STRING)
> +
> > > >> +      StrSize (Host) + StrSize (RemoteFilePath);
> > > >> +    TftpCmd = (CHAR16 *) AllocateZeroPool (CmdLen +
> sizeof(CHAR16));
> > > >> +    if (TftpCmd == NULL) {
> > > >> +      Print (L"%s: Cannot allocate memory\n", CMD_NAME_STRING);
> > > >> +      return SHELL_ABORTED;
> > > >> +    }
> > > >> +
> > > >> +    // Concatenate parameters and form tftp command string
> > > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), TFTP_CMD_STRING);
> > > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)Host);
> > > >> +    // Insert space
> > > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
> > > >> +    StrCatS (TftpCmd, CmdLen / sizeof(CHAR16), (CHAR16
> > > *)RemoteFilePath);
> > > >> +
> > > >> +    RunShellCommand (TftpCmd, &Status);
> > > >> +    FreePool (TftpCmd);
> > > >> +    if (EFI_ERROR(Status)) {
> > > >> +      Print (L"%s: Error while performing tftp command\n",
> > > CMD_NAME_STRING);
> > > >> +      return SHELL_ABORTED;
> > > >> +    }
> > > >> +
> > > >> +  } else {
> > > >> +    Print (L"%s: Both -f and -t flag specified, please choose one\n",
> > > >> +      CMD_NAME_STRING);
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  // Check image checksum and magic
> > > >> +  Status = CheckFirmwareImage (FileToWrite);
> > > >> +  if (EFI_ERROR(Status)) {
> > > >> +    Print (L"%s: Wrong firmware Image\n", CMD_NAME_STRING);
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  // Probe spi bus
> > > >> +  RunShellCommand (SF_PROBE_CMD_STRING, &Status);
> > > >> +  if (EFI_ERROR(Status)) {
> > > >> +    Print (L"%s: Error while performing sf probe\n",
> > > CMD_NAME_STRING);
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  // Allocate buffer for sf command string
> > > >> +  CmdLen = StrSize (SF_WRITE_CMD_STRING) + StrSize (FileToWrite)
> +
> > > >> +    StrSize (SPACE_STRING) + StrSize (SF_LOAD_ADDR_STRING);
> > > >> +  SfCmd = (CHAR16 *) AllocateZeroPool (CmdLen + sizeof(CHAR16));
> > > >> +  if (SfCmd == NULL) {
> > > >> +    Print (L"%s: Cannot allocate memory\n");
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  // Concatenate parameters and form command string
> > > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16),
> SF_WRITE_CMD_STRING);
> > > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), (CHAR16 *)FileToWrite);
> > > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16), SPACE_STRING);
> > > >> +  StrCatS (SfCmd, CmdLen / sizeof(CHAR16),
> SF_LOAD_ADDR_STRING);
> > > >> +
> > > >> +  // Update firmware image in flash
> > > >> +  RunShellCommand (SfCmd, &Status);
> > > >> +  FreePool (SfCmd);
> > > >> +  if (EFI_ERROR(Status)) {
> > > >> +    Print (L"%s: Error while performing sf update\n",
> > > CMD_NAME_STRING);
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  return EFI_SUCCESS;
> > > >> +}
> > > >> +
> > > >> +EFI_STATUS
> > > >> +EFIAPI
> > > >> +ShellFUpdateCommandConstructor (
> > > >> +  IN EFI_HANDLE        ImageHandle,
> > > >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> > > >> +  )
> > > >> +{
> > > >> +  EFI_STATUS Status;
> > > >> +
> > > >> +  gShellFUpdateHiiHandle = NULL;
> > > >> +
> > > >> +  gShellFUpdateHiiHandle = HiiAddPackages (
> > > >> +                          &gShellFUpdateHiiGuid, gImageHandle,
> > > >> +                          UefiShellFUpdateCommandLibStrings, NULL
> > > >> +                          );
> > > >> +  if (gShellFUpdateHiiHandle == NULL) {
> > > >> +    Print (L"%s: Cannot add Hii package\n", CMD_NAME_STRING);
> > > >> +    return EFI_DEVICE_ERROR;
> > > >> +  }
> > > >> +
> > > >> +  Status = ShellCommandRegisterCommandName (
> > > >> +     CMD_NAME_STRING, ShellCommandRunFUpdate,
> > > ShellCommandGetManFileNameFUpdate,
> > > >> +     0, CMD_NAME_STRING, TRUE , gShellFUpdateHiiHandle,
> > > >> +     STRING_TOKEN (STR_GET_HELP_FUPDATE)
> > > >> +     );
> > > >> +  if (EFI_ERROR(Status)) {
> > > >> +    Print (L"%s: Error while registering command\n",
> > > CMD_NAME_STRING);
> > > >> +    return SHELL_ABORTED;
> > > >> +  }
> > > >> +
> > > >> +  return EFI_SUCCESS;
> > > >> +}
> > > >> +
> > > >> +EFI_STATUS
> > > >> +EFIAPI
> > > >> +ShellFUpdateCommandDestructor (
> > > >> +  IN EFI_HANDLE        ImageHandle,
> > > >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> > > >> +  )
> > > >> +{
> > > >> +
> > > >> +  if (gShellFUpdateHiiHandle != NULL) {
> > > >> +    HiiRemovePackages (gShellFUpdateHiiHandle);
> > > >> +  }
> > > >> +  return EFI_SUCCESS;
> > > >> +}
> > > >> diff --git a/Applications/FirmwareUpdate/FUpdate.inf
> > > b/Applications/FirmwareUpdate/FUpdate.inf
> > > >> new file mode 100644
> > > >> index 0000000..53b5305
> > > >> --- /dev/null
> > > >> +++ b/Applications/FirmwareUpdate/FUpdate.inf
> > > >> @@ -0,0 +1,68 @@
> > > >> +#
> > > >> +# 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]
> > > >> +  INF_VERSION = 0x00010006
> > > >> +  BASE_NAME = UefiShellFUpdateCommandLib
> > > >> +  FILE_GUID = 470292b2-926b-4ed8-8080-be7a260db627
> > > >> +  MODULE_TYPE = UEFI_APPLICATION
> > > >> +  VERSION_STRING = 0.1
> > > >> +  LIBRARY_CLASS = NULL|UEFI_APPLICATION UEFI_DRIVER
> > > >> +  CONSTRUCTOR = ShellFUpdateCommandConstructor
> > > >> +  DESTRUCTOR = ShellFUpdateCommandDestructor
> > > >> +
> > > >> +[Sources]
> > > >> +  FUpdate.c
> > > >> +  FUpdate.uni
> > > >> +
> > > >> +[Packages]
> > > >> +  MdePkg/MdePkg.dec
> > > >> +  ShellPkg/ShellPkg.dec
> > > >> +  MdeModulePkg/MdeModulePkg.dec
> > > >> +  OpenPlatformPkg/Platforms/Marvell/Marvell.dec
> > > >> +
> > > >> +[LibraryClasses]
> > > >> +  UefiLib
> > > >> +  UefiBootServicesTableLib
> > > >> +  MemoryAllocationLib
> > > >> +  BaseLib
> > > >> +  BaseMemoryLib
> > > >> +  DebugLib
> > > >> +  ShellCommandLib
> > > >> +  ShellLib
> > > >> +  UefiLib
> > > >> +  UefiRuntimeServicesTableLib
> > > >> +  PcdLib
> > > >> +  HiiLib
> > > >> +  FileHandleLib
> > > >> +
> > > >> +[Guids]
> > > >> +  gShellFUpdateHiiGuid
> > > >> diff --git a/Applications/FirmwareUpdate/FUpdate.uni
> > > b/Applications/FirmwareUpdate/FUpdate.uni
> > > >> new file mode 100644
> > > >> index
> > >
> 0000000000000000000000000000000000000000..6143c0580f1d1ef8ce4fe619a1
> > > df1ce380adf2c0
> > > >> GIT binary patch
> > > >> literal 5838
> > > >>
> > >
> zcmd6r?N1v=5XSd&rTz~m@+FOuK&z@BXdB5G6OhbHeI_(&s)*P?EX6>LDdL}
> > > R`}}4Y
> > > >> z_Fim)B2|h`_TBEy&OY<Z%kJL4|E#8GU-
> > > k8E`X+VLMY>G4X_hAGEPYJ#RHUcs{Z98E
> > > >>
> > >
> z(pvgH{iLVebS#O#(@FZIt4KTPT#_g1JJ*}J#$MPH@A_(w)60Z*e$BBPsZ5(DH%
> > > TMu
> > > >> zI8RslFVenngr*p~lTP*KR@$$1pGl4p6GJjq&s>Nn8egO>-
> > > 9MS0Q^{SVsru?OKUYs^
> > > >> znhN!;+Src8b3GNB10={X)7Ui6^*l)*^bUIqjXT$yRmsoHx~ZO!JT-
> > > lb@LbynxN^K`
> > > >> zIn^kbyht}1doJb+wK5;k=NwOk%lRBE#O3S<wy>ix&4tFo-
> > > prWGG9T<MUWEtn%*5H1
> > > >>
> > >
> zSq~;Gfgeyu8$Ga<Z3c1n|8rvmo!EG(e{`Y?JMh<~dM@O>iCW&nJZN~Bn~_Gr
> > > JG$Ij
> > > >>
> > >
> zd>%7RrE8wn?<20zyXy5!t%sOH*M&t|ohxPqCl(VMpv1PhX2wGq^RXfXyO3p
> > > 6Go#Mc
> > > >>
> z2Xdm<S*qfT=Nvu6g3sr;me~&{8O5Tg@+clDy1)!mvr2Bp%yEAVQ*Qa<_-
> > > Qib{vL}x
> > > >>
> > >
> zEYC<E=8NnPMAdtpMc3xp|Nrt8nS_5%EXt~=;(06|k7OwvQyp_&MAmuLAl6(
> > > HxkV-N
> > > >> zET8D<lf1qtOCkd^gYD|%d6DaG;EFrG-
> > > 4iy??Rq_;ii11ypk6r(+2fVKanpABoH=Nv
> > > >> z<&j#gnvCR4)Z7mbaOf4-W15N#lkkcLM+avfRa7Cb$<-
> > > )x6l5)Rn~X$TUK4VTk?W_@
> > > >>
> > >
> za3fp1Vg<`y=axm`l~uNpw4drXy2AteUMrc`D;^ahSHUaU(DczTZEGzpC0A<w+
> > > e*W9
> > > >> zlsc+aJ@t71fm-|Njar-P-BkZZdaHj%BGuK~1N96f@mhUD^=+%KZ}J1v-
> > > I3oJiR_*v
> > > >> zkljd!dfL}Bv#)4Y?##KOyN`eYo|vzfe%Cx8D@~G-
> > > $nUDRue)P&^aC3;wBFKIlpSg5
> > > >>
> > >
> zO72iNcz$iYj)AD$H{9si)o*yG4ScM|VQYOh&@Gyeoy3+#?Mll)vu@v^>B#0C!#
> > > XsJ
> > > >>
> > >
> zf|V_saqr%;En@`^3fYD?u%s*OeQVKssNMs~l^TU4ynqMmO0%@1ClG9^+Q
> > > QSG{;?A#
> > > >> z-{^-
> > >
> Q@H*)Fy0VpvJi0GSK?vv0{BmDzNRD0Djx=xRPRnho`V*v}?g|GSgK$w_S8s?s
> > > >> z@PZWCQnoXa8N7$+=-
> > > tHvY`itODpHvGO}x1#A6ZCqN25B52jU8RPYubJ$BO3*-oiRO
> > > >> z;7of=l<H^yiARTeCejMczOQRj{lPA12bOhRJ(cNa!`7EnL_ZOBs8-
> > > J`#SZv^9k<$X
> > > >>
> > >
> zcx*8~`ic3{Y>Z5Td2kGL9g117k*Dmy$>bxwBVzD<S9AER<a0zY(pBGjzJP<wb;~
> > > 0j
> > > >>
> > >
> zat46)NHg>_BmbVy#A%ML&V78q8Fhnr$<bBU=9YU^87}oEdC(K*Aj0od2E5>-
> > > sRWMZ
> > > >>
> > >
> zNblhn9*8MgUOOV6qBc<l+)_Lm7Dv@^zN)=wg0E)Hg^|ck=D^1t#Rb(DByjF*
> > > hHkF`
> > > >> z;HuY_9BtH}dYtDK`s+R&t{O-
> > > 1w3>cRPwrb!Y%j@~g;kdG85b+{3yV!w6Iy3>qPfzZ
> > > >> zX{9ojT`eo5pLsm0-dh|D(|hxLUpeqz+>;BQS42nq$~dmIf}d&5CJLW-
> MHLu-
> > > u?x7?
> > > >> zdOVi2?;F<Cx1yOQLNu-
> > > H0}g9a>}aO8dtoo~QX}3;{+%qy`l;i@HPx)wdSBPauAx3(
> > > >>
> z>^nf%KE)1&a40S~$+4?pPs1L58H!T5z)Evq`BJvzidIjPmp|l5{Q8S>lszr`Qdr`A
> > > >>
> > >
> zePHZ;Os~?Pl0b59lG~ERx38?lX)E?;i$*TP4(h0Xk3mjUJT#X_%?n8_X0gin<(*~h
> > > >>
> > >
> zvDtgF@4S(hIi(+~mG|D+OX0buJ?&^+)>Dm0=76ngbhQ19reJ%S)<S+^oo4sxJJ
> > > nmY
> > > >> zGY6%`kVg`g?m$uXqa?6%tocvmOTIZQvTJ!v1RCa!I0-
> > > gk(Id$@uiD7@iqnDTaIo8+
> > > >> zyV>(n7Ls@DKFB<FqO7V-
> > > xpu@iN36HTd@g76i6Zb~*Ok7Rt*dfT7boHmyUtp=;i;K(
> > > >> zo^g-
> > >
> W@FnOXzE@lLs*zExG5O1n$4E0i3Vj`1la=w~d*Rw6t)*%#8qJA``utI=&gXax
> > > >> z|FhF$H#bn-d!tC;obk9gsE#aZW7`pd*NC^C^!K$wv6t-H-
> o^KX5&g~k(7qbL-
> > > AH!%
> > > >> z2@qzF6)k>Vp2}uwYkVd1H)P>oTCvDQf3r9rtC-
> > > mFvyX}sR8P6g7q^*ax)F|8UHrt%
> > > >> QHyJ;b`f6NX>FQbZ9}*T`00000
> > > >>
> > > >> literal 0
> > > >> HcmV?d00001
> > > >>
> > > >> diff --git a/Platforms/Marvell/Marvell.dec
> > > b/Platforms/Marvell/Marvell.dec
> > > >> index e63add4..f9d2660 100644
> > > >> --- a/Platforms/Marvell/Marvell.dec
> > > >> +++ b/Platforms/Marvell/Marvell.dec
> > > >> @@ -54,6 +54,7 @@
> > > >>
> > > >>    gShellEepromHiiGuid = { 0xb2f4c714, 0x147f, 0x4ff7, { 0x82, 0x1b,
> 0xce,
> > > 0x7b, 0x91, 0x7f, 0x5f, 0x2f } }
> > > >>    gShellSfHiiGuid = { 0x03a67756, 0x8cde, 0x4638, { 0x82, 0x34, 0x4a,
> 0x0f,
> > > 0x6d, 0x58, 0x81, 0x39 } }
> > > >> +  gShellFUpdateHiiGuid = { 0x9b5d2176, 0x590a, 0x49db, { 0x89, 0x5d,
> > > 0x4a, 0x70, 0xfe, 0xad, 0xbe, 0x24 } }
> > > >>
> > > >>  [PcdsFixedAtBuild.common]
> > > >>  #MPP
> > > >> --
> > > >> 1.8.3.1
> > > >>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
  2016-11-16 21:31           ` Carsey, Jaben
@ 2016-11-16 21:48             ` Marcin Wojtas
  2016-11-16 23:58               ` Marcin Wojtas
  0 siblings, 1 reply; 7+ messages in thread
From: Marcin Wojtas @ 2016-11-16 21:48 UTC (permalink / raw)
  To: Carsey, Jaben
  Cc: Leif Lindholm, Ard Biesheuvel, linaro-uefi, Neta Zur Hershkovits,
	Yehuda Yitschak, Haim Boot, Jan Dabros, Bartosz Szczepanek,
	edk2-devel-01, Ni, Ruiyu

Hi Jaben,

Thank you for your input.

>
> I agree on EBL, but I have very little experience with EBL so I don’t want to discuss in detail as I am not the right person without more research.  Specifically, my gut reaction is that needing a platform specific boot loader indicates that something has already gone wrong on that platform.
>
> However, this does not seem like a boot loader or an application at all.  this is an internal shell command. The goal here seems to be to create a NULL library to add a new internal command to the UEFI Shell.  This library gets compiled/linked into the shell itself.

Indeed, it's nothing similar to the bootloader whatsoever. This
command simply enables updating firmware in SPI flash directly from
local path or from tftp.

>
> I feel that we have found a "new" use case that I encountered, but worked around in the past because all previous cases involved commands in the same library (there are interactions between Reconnect and Disconnect/Connect).

Right, however Reconnect is easy, as it simply calls gBS callbacks,
whose definition are also in the same location.

>
> I would say that a new API in the ShellCommandLib that links the UEFI Shell Application to the NULL libraries that make up the internal commands would be my first choice for implementation.  I would lean to something like the function that Marcin already called.  Maybe this?
>
> EFIAPI
> RunRegisteredCommand(
>   CHAR16* CommandLine,
>   EFI_STATUS *CommandReturnValue
> )

I'll try this one and let know.

Thanks,
Marcin


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
  2016-11-16 21:48             ` Marcin Wojtas
@ 2016-11-16 23:58               ` Marcin Wojtas
  2016-11-17  0:57                 ` Carsey, Jaben
  0 siblings, 1 reply; 7+ messages in thread
From: Marcin Wojtas @ 2016-11-16 23:58 UTC (permalink / raw)
  To: Carsey, Jaben
  Cc: Leif Lindholm, Ard Biesheuvel, linaro-uefi, Neta Zur Hershkovits,
	Yehuda Yitschak, Haim Boot, Jan Dabros, Bartosz Szczepanek,
	edk2-devel-01, Ni, Ruiyu

Hi Jaben,

I haven't found RunRegisteredCommand in newest edk2. Can you please
point exactly what you meant?

As an alternative I checked ShellCommandRunCommandHandler - this is in
fact a library helper function and it's nested deep down in
RunShellCommand, which is for now the only working option.
ShellCommandRunCommandHandler requires a lot of additional processing
of the command line. All is done in Application/Shell code.

Is there any chance to expose RunShellCommand (or equivalent), so that
it can be used in a nice way, not with including multi "../../"
relative path header?

I expected, that the commands, which are in fact some wrappers or they
mix multiple others is pretty much of a standard, I'm pretty surprised
there are so huge difficulties in EDK2. I'm wondering of if there are
any other options to be used here (unless we accept, what we have for
now:) ).

Best regards,
Marcin

2016-11-16 22:48 GMT+01:00 Marcin Wojtas <mw@semihalf.com>:
> Hi Jaben,
>
> Thank you for your input.
>
>>
>> I agree on EBL, but I have very little experience with EBL so I don’t want to discuss in detail as I am not the right person without more research.  Specifically, my gut reaction is that needing a platform specific boot loader indicates that something has already gone wrong on that platform.
>>
>> However, this does not seem like a boot loader or an application at all.  this is an internal shell command. The goal here seems to be to create a NULL library to add a new internal command to the UEFI Shell.  This library gets compiled/linked into the shell itself.
>
> Indeed, it's nothing similar to the bootloader whatsoever. This
> command simply enables updating firmware in SPI flash directly from
> local path or from tftp.
>
>>
>> I feel that we have found a "new" use case that I encountered, but worked around in the past because all previous cases involved commands in the same library (there are interactions between Reconnect and Disconnect/Connect).
>
> Right, however Reconnect is easy, as it simply calls gBS callbacks,
> whose definition are also in the same location.
>
>>
>> I would say that a new API in the ShellCommandLib that links the UEFI Shell Application to the NULL libraries that make up the internal commands would be my first choice for implementation.  I would lean to something like the function that Marcin already called.  Maybe this?
>>
>> EFIAPI
>> RunRegisteredCommand(
>>   CHAR16* CommandLine,
>>   EFI_STATUS *CommandReturnValue
>> )
>
> I'll try this one and let know.
>
> Thanks,
> Marcin


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell
  2016-11-16 23:58               ` Marcin Wojtas
@ 2016-11-17  0:57                 ` Carsey, Jaben
  0 siblings, 0 replies; 7+ messages in thread
From: Carsey, Jaben @ 2016-11-17  0:57 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: Leif Lindholm, Ard Biesheuvel, linaro-uefi, Neta Zur Hershkovits,
	Yehuda Yitschak, Haim Boot, Jan Dabros, Bartosz Szczepanek,
	edk2-devel-01, Ni, Ruiyu, Carsey, Jaben



> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Wednesday, November 16, 2016 3:58 PM
> To: Carsey, Jaben <jaben.carsey@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; linaro-uefi <linaro-uefi@lists.linaro.org>; Neta Zur
> Hershkovits <neta@marvell.com>; Yehuda Yitschak <yehuday@marvell.com>;
> Haim Boot <hayim@marvell.com>; Jan Dabros <jsd@semihalf.com>; Bartosz
> Szczepanek <bsz@semihalf.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Ni,
> Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate'
> comand to shell
> Importance: High
> 
> Hi Jaben,
> 
> I haven't found RunRegisteredCommand in newest edk2. Can you please point
> exactly what you meant?
> 
I meant add a new function like that.  It does not exist at all now.

> As an alternative I checked ShellCommandRunCommandHandler - this is in fact a
> library helper function and it's nested deep down in RunShellCommand, which is
> for now the only working option.
> ShellCommandRunCommandHandler requires a lot of additional processing of
> the command line. All is done in Application/Shell code.

What's the difference that you need?  I see this:  looks like you would need a command line (which I assume you have), the return value (which I assume you want), and control over whether you want LastError changed based on this (I would suggest not, but...)

RETURN_STATUS
EFIAPI
ShellCommandRunCommandHandler (
  IN CONST CHAR16               *CommandString,
  IN OUT SHELL_STATUS           *RetVal,
  IN OUT BOOLEAN                *CanAffectLE OPTIONAL
  )

The main difference between that and the RunShellCommand is things like file redirection, trimming space, alias replacement, and things that feel very "command-line" processing.  I would think that your TFTP command line would be less needy of fixing up.

> 
> Is there any chance to expose RunShellCommand (or equivalent), so that it can
> be used in a nice way, not with including multi "../../"
> relative path header?
> 
> I expected, that the commands, which are in fact some wrappers or they mix
> multiple others is pretty much of a standard, I'm pretty surprised there are so
> huge difficulties in EDK2. I'm wondering of if there are any other options to be
> used here (unless we accept, what we have for
> now:) ).

I think we have less wrapping and less mixing that it appears at first.  Those that are related are (I believe) in the same lib and as such, just call each other.

> 
> Best regards,
> Marcin
> 
> 2016-11-16 22:48 GMT+01:00 Marcin Wojtas <mw@semihalf.com>:
> > Hi Jaben,
> >
> > Thank you for your input.
> >
> >>
> >> I agree on EBL, but I have very little experience with EBL so I don’t want to
> discuss in detail as I am not the right person without more research.  Specifically,
> my gut reaction is that needing a platform specific boot loader indicates that
> something has already gone wrong on that platform.
> >>
> >> However, this does not seem like a boot loader or an application at all.  this is
> an internal shell command. The goal here seems to be to create a NULL library
> to add a new internal command to the UEFI Shell.  This library gets
> compiled/linked into the shell itself.
> >
> > Indeed, it's nothing similar to the bootloader whatsoever. This
> > command simply enables updating firmware in SPI flash directly from
> > local path or from tftp.
> >
> >>
> >> I feel that we have found a "new" use case that I encountered, but worked
> around in the past because all previous cases involved commands in the same
> library (there are interactions between Reconnect and Disconnect/Connect).
> >
> > Right, however Reconnect is easy, as it simply calls gBS callbacks,
> > whose definition are also in the same location.
> >
> >>
> >> I would say that a new API in the ShellCommandLib that links the UEFI Shell
> Application to the NULL libraries that make up the internal commands would be
> my first choice for implementation.  I would lean to something like the function
> that Marcin already called.  Maybe this?
> >>
> >> EFIAPI
> >> RunRegisteredCommand(
> >>   CHAR16* CommandLine,
> >>   EFI_STATUS *CommandReturnValue
> >> )
> >
> > I'll try this one and let know.
> >
> > Thanks,
> > Marcin

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-11-17  0:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1468110107-18979-1-git-send-email-mw@semihalf.com>
     [not found] ` <1468110107-18979-22-git-send-email-mw@semihalf.com>
     [not found]   ` <CAKv+Gu9KvJVGgORjxnLzuGVswimavRg8h1ZXKC+=Fv48RLJGEQ@mail.gmail.com>
2016-11-16 16:13     ` [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell Marcin Wojtas
2016-11-16 17:05       ` Carsey, Jaben
2016-11-16 17:35         ` Leif Lindholm
2016-11-16 21:31           ` Carsey, Jaben
2016-11-16 21:48             ` Marcin Wojtas
2016-11-16 23:58               ` Marcin Wojtas
2016-11-17  0:57                 ` Carsey, Jaben

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox