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

[Abner Chang]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm at linaro.org]
> Sent: Tuesday, October 15, 2019 6:42 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com>
> Cc: devel at edk2.groups.io
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 10/29]
> MdePkg/BasePeCoff: Add RISC-V PE/Coff related code.
> 
> On Tue, Oct 15, 2019 at 04:26:12AM +0000, Chang, Abner (HPS SW/FW
> Technologist) wrote:
> >
> >
> > > -----Original Message-----
> > > From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf
> > > Of Abner Chang
> > > Sent: Tuesday, October 15, 2019 12:03 PM
> > > To: devel at edk2.groups.io; leif.lindholm at 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf
> > > > Of Leif Lindholm
> > > > Sent: Friday, September 27, 2019 7:46 AM
> > > > To: devel at edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > > > <abner.chang at 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 at 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/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?
> 
> There is never a good reason to duplicate code. You will always end up
> changing one and forgetting the other.
Sure.
> 
> I see no problem with creating a header file which contains nothing else. But I
> also think it would be valid to put this into C/Common/PeCoffLib.h - and this
> file is already included by the affected .c files.
Hmm. Then put it into PeCoffLib. I am good with this if no one opposite.
> 
> /
>     Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49000): https://edk2.groups.io/g/devel/message/49000
Mute This Topic: https://groups.io/mt/34258204/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-