[U-Boot] [PATCH v2 6/6] riscv: efi: Generate Microsoft PE format compliant images

[Bin Meng]

Hi Heinrich,

On Wed, Oct 3, 2018 at 2:19 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10/02/2018 04:39 PM, Bin Meng wrote:
> > Per Microsoft PE Format documentation [1], PointerToSymbolTable and
> > NumberOfSymbols should be zero for an image in the COFF file header.
> > Currently the COFF file header is hardcoded on RISC-V and these two
> > members are not zero.
> >
> > This updates the hardcoded structure to clear these two members, as
> > well as setting the flag IMAGE_FILE_LOCAL_SYMS_STRIPPED so that we
> > can generate compliant *.efi images.
> >
> > [1] https://docs.microsoft.com/zh-cn/windows/desktop/Debug/pe-format
> >
> > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> >
> > ---
> >
> > Changes in v2:
> > - use macros in pe.h for the characteristics field
> >
> >  arch/riscv/lib/crt0_riscv_efi.S | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/riscv/lib/crt0_riscv_efi.S b/arch/riscv/lib/crt0_riscv_efi.S
> > index 18f61f5..b7b5329 100644
> > --- a/arch/riscv/lib/crt0_riscv_efi.S
> > +++ b/arch/riscv/lib/crt0_riscv_efi.S
> > @@ -41,13 +41,13 @@ coff_header:
> >       .short  2                               /* nr_sections */
> >       .long   0                               /* TimeDateStamp */
> >       .long   0                               /* PointerToSymbolTable */
> > -     .long   1                               /* NumberOfSymbols */
> > +     .long   0                               /* NumberOfSymbols */
>
> Looking at the top of the file it is meant both for 32bit and for 64bit
> systems:
>
> #if __riscv_xlen == 64
> #define SIZE_LONG       8
> #define SAVE_LONG(reg, idx)     sd      reg, (idx*SIZE_LONG)(sp)
> #define LOAD_LONG(reg, idx)     ld      reg, (idx*SIZE_LONG)(sp)
> #define PE_MACHINE      0x5064
> #else
> #define SIZE_LONG       4
> #define SAVE_LONG(reg, idx)     sw      reg, (idx*SIZE_LONG)(sp)
> #define LOAD_LONG(reg, idx)     lw      reg, (idx*SIZE_LONG)(sp)
> #define PE_MACHINE      0x5032
> #endif
>
> But for 32bit and 64bit we cannot use the same values of characteristics
> and we need different optional header formats.
>
> For the ARM and the x86 architecture this has resulted in separate files
> crt0*.S - one for 32bit, the other for 64bit.
>
> Shouldn't we go the same way for RISC-V?
>

You observation is correct. We should use separate files to support
32-bit and 64-bit.

> >       .short  section_table - optional_header /* SizeOfOptionalHeader */
> > -     /*
> > -      * Characteristics: IMAGE_FILE_DEBUG_STRIPPED |
> > -      * IMAGE_FILE_EXECUTABLE_IMAGE | IMAGE_FILE_LINE_NUMS_STRIPPED
> > -      */
> > -     .short  0x206
> > +     /* Characteristics */
> > +     .short  (IMAGE_FILE_EXECUTABLE_IMAGE | \
> > +              IMAGE_FILE_LINE_NUMS_STRIPPED | \
> > +              IMAGE_FILE_LOCAL_SYMS_STRIPPED | \
> > +              IMAGE_FILE_DEBUG_STRIPPED)
>
> These values suit 64bit only.
>
> As currently we only have a defconfig for a 64bit system -
> ax25-ae350_defconfig - let's merge this patch.
>
> @Bin:
> Your "riscv: Add QEMU virt board support" patch series adds a 32bit
> board. Please, consider adding a separate 32bit crt0*.S there.
>

Sure, will do it in coming patches.

> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>
> >  optional_header:
> >       .short  0x20b                           /* PE32+ format */
> >       .byte   0x02                            /* MajorLinkerVersion */
> >

Regards,
Bin