Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 10/29] MdePkg/BasePeCoff: Add RISC-V PE/Coff related code.

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Abner Chang
> Sent: Tuesday, October 15, 2019 12:03 PM
> To: devel@edk2.groups.io; leif.lindholm@linaro.org
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 10/29]
> MdePkg/BasePeCoff: Add RISC-V PE/Coff related code.
> 
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Leif Lindholm
> > Sent: Friday, September 27, 2019 7:46 AM
> > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>
> > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 10/29]
> > MdePkg/BasePeCoff: Add RISC-V PE/Coff related code.
> >
> > On Mon, Sep 23, 2019 at 08:31:36AM +0800, Abner Chang wrote:
> > > Support RISC-V image relocation.
> > >
> > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > ---
> > >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c          |   3 +-
> > >  MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf     |   5 +
> > >  MdePkg/Library/BasePeCoffLib/BasePeCoffLib.uni     |   2 +
> > >  .../Library/BasePeCoffLib/BasePeCoffLibInternals.h |   1 +
> > >  .../Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c   | 142
> > +++++++++++++++++++++
> > >  5 files changed, 152 insertions(+), 1 deletion(-)  create mode
> > > 100644 MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c
> > >
> > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > > index 07bb62f..97e0ff4 100644
> > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > > @@ -1,6 +1,6 @@
> > >  /** @file
> > >    Base PE/COFF loader supports loading any PE32/PE32+ or TE image,
> > > but
> > > -  only supports relocating IA32, x64, IPF, and EBC images.
> > > +  only supports relocating IA32, x64, IPF, ARM, RISC-V and EBC images.
> > >
> > >    Caution: This file requires additional review when modified.
> > >    This library will have external input - PE/COFF image.
> > > @@ -17,6 +17,7 @@
> > >
> > >    Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> > >    Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > > reserved.<BR>
> > > +  Portions Copyright (c) 2016, Hewlett Packard Enterprise
> > > + Development LP. All rights reserved.<BR>
> > >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  **/
> > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > > b/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > > index 395c140..b190494 100644
> > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > > @@ -3,6 +3,7 @@
> > >  #  The IPF version library supports loading IPF and EBC PE/COFF image.
> > >  #  The IA32 version library support loading IA32, X64 and EBC
> > > PE/COFF
> > images.
> > >  #  The X64 version library support loading IA32, X64 and EBC
> > > PE/COFF
> > images.
> > > +#  The RISC-V version library support loading RISC-V images.
> > >  #
> > >  #  Caution: This module requires additional review when modified.
> > >  #  This library will have external input - PE/COFF image.
> > > @@ -11,6 +12,7 @@
> > >  #
> > >  #  Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > reserved.<BR>  #  Portions copyright (c) 2008 - 2009, Apple Inc. All
> > > rights reserved.<BR>
> > > +#  Copyright (c) 2016, Hewlett Packard Enterprise Development LP.
> > > +All rights reserved.<BR>
> > >  #
> > >  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -41,6 +43,9
> > > @@ [Sources.ARM]
> > >    Arm/PeCoffLoaderEx.c
> > >
> > > +[Sources.RISCV64]
> > > +  RiscV/PeCoffLoaderEx.c
> > > +
> > >  [Packages]
> > >    MdePkg/MdePkg.dec
> > >
> > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.uni
> > > b/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.uni
> > > index b0ea702..8616ca3 100644
> > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.uni
> > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.uni
> > > @@ -4,6 +4,7 @@
> > >  // The IPF version library supports loading IPF and EBC PE/COFF image.
> > >  // The IA32 version library support loading IA32, X64 and EBC
> > > PE/COFF
> > images.
> > >  // The X64 version library support loading IA32, X64 and EBC
> > > PE/COFF
> > images.
> > > +// The RISC-V version library support loading RISC-V32 and RISC-V64
> > PE/COFF images.
> > >  //
> > >  // Caution: This module requires additional review when modified.
> > >  // This library will have external input - PE/COFF image.
> > > @@ -12,6 +13,7 @@
> > >  //
> > >  // Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > reserved.<BR>  // Portions copyright (c) 2008 - 2009, Apple Inc. All
> > > rights reserved.<BR>
> > > +// Copyright (c) 2016, Hewlett Packard Enterprise Development LP.
> > > +All rights reserved.<BR>
> > >  //
> > >  // SPDX-License-Identifier: BSD-2-Clause-Patent  // diff --git
> > > a/MdePkg/Library/BasePeCoffLib/BasePeCoffLibInternals.h
> > > b/MdePkg/Library/BasePeCoffLib/BasePeCoffLibInternals.h
> > > index b74277f..9c33703 100644
> > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoffLibInternals.h
> > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoffLibInternals.h
> > > @@ -2,6 +2,7 @@
> > >    Declaration of internal functions in PE/COFF Lib.
> > >
> > >    Copyright (c) 2006 - 2010, Intel Corporation. All rights
> > > reserved.<BR>
> > > +  Copyright (c) 2016, Hewlett Packard Enterprise Development LP.
> > > + All rights reserved.<BR>
> >
> > You only get to add copyright when you otherwise modify the file :)
> >
> > >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  **/
> > > diff --git a/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c
> > > b/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c
> > > new file mode 100644
> > > index 0000000..8eb37f9
> > > --- /dev/null
> > > +++ b/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c
> > > @@ -0,0 +1,142 @@
> > > +/** @file
> > > +  PE/Coff loader for RISC-V PE image
> > > +
> > > +  Copyright (c) 2016, Hewlett Packard Enterprise Development LP.
> > > +All rights reserved.<BR>
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/ #include
> > > +"BasePeCoffLibInternals.h"
> > > +#include <Library/BaseLib.h>
> > > +
> > > +//
> > > +// RISC-V definition.
> > > +//
> > > +#define RV_X(x, s, n) (((x) >> (s)) & ((1<<(n))-1)) #define
> > > +RISCV_IMM_BITS 12 #define RISCV_IMM_REACH
> > (1LL<<RISCV_IMM_BITS)
> > > +#define RISCV_CONST_HIGH_PART(VALUE) \
> > > +  (((VALUE) + (RISCV_IMM_REACH/2)) & ~(RISCV_IMM_REACH-1))
> >
> > This looked familiar, so I had a look.
> > This block is copied around - it exists in:
> > - BaseTools/Source/C/Common/PeCoffLoaderEx.c
> > - BaseTools/Source/C/GenFw/Elf64Convert.c
> > - MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c
> >
> > This needs to be moved somewhere central and included elsewhere.
> > BaseTools and MdePkg unfortunately duplicate a lot of stuff, but this
> > still belongs in a common header file for either.
> 
> I can consolidate that macro in two files under BaseTools, but not
> consolidating macro in files in both MdePkg and BaseTools. BaseTools and
> edk2 are two separate projects and could be built individually based on my
> understanding.
> I have no idea how to leverage one header file from both projects and I don't
> go that far to address it.

Leif, seem there is no good place and the existing header file to put this macro unless I create a new header file under BaseTools/Source/C/Include. I would like to keep this duplicate macro in both files rather than create an header file in which only define this macro. Do you have good idea?

> 
> >
> > > +
> > > +/**
> > > +  Performs an RISC-V specific relocation fixup and is a no-op on
> > > +  other instruction sets.
> > > +  RISC-V splits 32-bit fixup into 20bit and 12-bit with two
> > > +relocation
> > > +  types. We have to know the lower 12-bit fixup first then we can
> > > +deal
> > > +  carry over on high 20-bit fixup. So we log the high 20-bit in
> > > +  FixupData.
> > > +
> > > +  @param  Reloc       The pointer to the relocation record.
> > > +  @param  Fixup       The pointer to the address to fix up.
> > > +  @param  FixupData   The pointer to a buffer to log the fixups.
> > > +  @param  Adjust      The offset to adjust the fixup.
> > > +
> > > +  @return Status code.
> > > +
> > > +**/
> > > +RETURN_STATUS
> > > +PeCoffLoaderRelocateImageEx (
> > > +  IN UINT16      *Reloc,
> > > +  IN OUT CHAR8   *Fixup,
> > > +  IN OUT CHAR8   **FixupData,
> > > +  IN UINT64      Adjust
> > > +  )
> > > +{
> > > +  UINT32 Value;
> > > +  UINT32 Value2;
> > > +  UINT32 *RiscVHi20Fixup;
> > > +
> > > +  switch ((*Reloc) >> 12) {
> > > +  case EFI_IMAGE_REL_BASED_RISCV_HI20:
> > > +      *(UINT64 *)(*FixupData) = (UINT64)(UINTN)Fixup;
> > > +      break;
> > > +
> > > +  case EFI_IMAGE_REL_BASED_RISCV_LOW12I:
> > > +      RiscVHi20Fixup =  (UINT32 *)(*(UINT64 *)(*FixupData));
> > > +      if (RiscVHi20Fixup != NULL) {
> > > +
> > > +        Value = (UINT32)(RV_X(*RiscVHi20Fixup, 12, 20) << 12);
> > > +        Value2 = (UINT32)(RV_X(*(UINT32 *)Fixup, 20, 12));
> > > +        if (Value2 & (RISCV_IMM_REACH/2)) {
> > > +          Value2 |= ~(RISCV_IMM_REACH-1);
> > > +        }
> > > +        Value += Value2;
> > > +        Value += (UINT32)Adjust;
> > > +        Value2 = RISCV_CONST_HIGH_PART (Value);
> > > +        *(UINT32 *)RiscVHi20Fixup = (RV_X (Value2, 12, 20) << 12) |\
> > > +                                           (RV_X (*(UINT32 *)RiscVHi20Fixup, 0, 12));
> > > +        *(UINT32 *)Fixup = (RV_X (Value, 0, 12) << 20) |\
> > > +                           (RV_X (*(UINT32 *)Fixup, 0, 20));
> > > +      }
> > > +      break;
> > > +
> > > +  case EFI_IMAGE_REL_BASED_RISCV_LOW12S:
> > > +      RiscVHi20Fixup =  (UINT32 *)(*(UINT64 *)(*FixupData));
> > > +      if (RiscVHi20Fixup != NULL) {
> > > +        Value = (UINT32)(RV_X(*RiscVHi20Fixup, 12, 20) << 12);
> > > +        Value2 = (UINT32)(RV_X(*(UINT32 *)Fixup, 7, 5) |
> > > + (RV_X(*(UINT32
> > *)Fixup, 25, 7) << 5));
> > > +        if (Value2 & (RISCV_IMM_REACH/2)) {
> > > +          Value2 |= ~(RISCV_IMM_REACH-1);
> > > +        }
> > > +        Value += Value2;
> > > +        Value += (UINT32)Adjust;
> > > +        Value2 = RISCV_CONST_HIGH_PART (Value);
> > > +        *(UINT32 *)RiscVHi20Fixup = (RV_X (Value2, 12, 20) << 12) | \
> > > +                                           (RV_X (*(UINT32 *)RiscVHi20Fixup, 0, 12));
> > > +        Value2 = *(UINT32 *)Fixup & 0x01fff07f;
> > > +        Value &= RISCV_IMM_REACH - 1;
> > > +        *(UINT32 *)Fixup = Value2 | (UINT32)(((RV_X(Value, 0, 5) <<
> > > + 7) |
> > (RV_X(Value, 5, 7) << 25)));
> > > +      }
> > > +      break;
> > > +
> > > +  default:
> > > +      return RETURN_UNSUPPORTED;
> > > +
> > > +  }
> > > +  return RETURN_SUCCESS;
> > > +}
> > > +
> > > +/**
> > > +  Returns TRUE if the machine type of PE/COFF image is supported.
> > > +Supported
> > > +  does not mean the image can be executed it means the PE/COFF
> > > +loader supports
> > > +  loading and relocating of the image type. It's up to the caller
> > > +to support
> > > +  the entry point.
> > > +
> > > +  @param  Machine   Machine type from the PE Header.
> > > +
> > > +  @return TRUE if this PE/COFF loader can load the image
> > > +
> > > +**/
> > > +BOOLEAN
> > > +PeCoffLoaderImageFormatSupported (
> > > +  IN  UINT16  Machine
> > > +  )
> > > +{
> > > +  if ((Machine == IMAGE_FILE_MACHINE_RISCV32) || (Machine ==
> > > +IMAGE_FILE_MACHINE_RISCV64)) {
> >
> > RISCV32 is not supported by this set.
> >
> > /
> >     Leif
> >
> > > +    return TRUE;
> > > +  }
> > > +
> > > +  return FALSE;
> > > +}
> > > +
> > > +/**
> > > +  Performs an Itanium-based specific re-relocation fixup and is a
> > > +no-op on other
> > > +  instruction sets. This is used to re-relocated the image into the
> > > +EFI virtual
> > > +  space for runtime calls.
> > > +
> > > +  @param  Reloc       The pointer to the relocation record.
> > > +  @param  Fixup       The pointer to the address to fix up.
> > > +  @param  FixupData   The pointer to a buffer to log the fixups.
> > > +  @param  Adjust      The offset to adjust the fixup.
> > > +
> > > +  @return Status code.
> > > +
> > > +**/
> > > +RETURN_STATUS
> > > +PeHotRelocateImageEx (
> > > +  IN UINT16      *Reloc,
> > > +  IN OUT CHAR8   *Fixup,
> > > +  IN OUT CHAR8   **FixupData,
> > > +  IN UINT64      Adjust
> > > +  )
> > > +{
> > > +  return RETURN_UNSUPPORTED;
> > > +}
> > > --
> > > 2.7.4
> > >
> > >
> > >
> > >
> >
> >
> 
> 
>