Re: [PATCH] xen/riscv: PE/COFF image header for RISC-V target

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2024 18:16, Milan Djokic wrote:
>> Signed-off-by: Nikola Jelic <nikola.jelic@xxxxxxxxx>
> 
> This isn't you, is it? Your S-o-b is going to be needed, too.
> 
> Nikola.jelic@xxxxxxxxx is the initial author of the patch, I'll add myself 
> also if necessary
> 
>> +config RISCV_EFI
>> +     bool "UEFI boot service support"
>> +     depends on RISCV_64
>> +     default n
> 
> Nit: This line can be omitted (and if I'm not mistaken we generally do omit
> such).
> 
> If we remove the default value, EFI header shall be included into xen image 
> by default.

Why's this? Or in other words, what are you deriving this from? Not specifying
a default implicitly means "n", from all I know.

> We want to keep it as optional for now, and generate plain elf as default 
> format (until full EFI support is implemented)

I fully second this.

>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/pe.h
>> @@ -0,0 +1,148 @@
>> +#ifndef _ASM_RISCV_PE_H
>> +#define _ASM_RISCV_PE_H
>> +
>> +#define LINUX_EFISTUB_MAJOR_VERSION     0x1
>> +#define LINUX_EFISTUB_MINOR_VERSION     0x0
>> +
>> +#define MZ_MAGIC                    0x5a4d          /* "MZ" */
>> +
>> +#define PE_MAGIC                    0x00004550      /* "PE\0\0" */
>> +#define PE_OPT_MAGIC_PE32           0x010b
>> +#define PE_OPT_MAGIC_PE32PLUS       0x020b
>> +
>> +/* machine type */
>> +#define IMAGE_FILE_MACHINE_RISCV32  0x5032
>> +#define IMAGE_FILE_MACHINE_RISCV64  0x5064
> 
> Apart from this, hardly anything in this header is RISC-V specific.
> Please consider moving to xen/include/xen/.
> 
> We shall move generic part into xen/include/xen/efi. This is something which 
> should be considered for use on arm/x86 also.

It isn't, no. That's the case for Arm only so far afaict.

> Currently PE/COFF header is directly embedded into
> head.S for arm/x86
> 
>> +    char     name[8];                /* name or "/12\0" string tbl offset */
> 
> Why 12?
> 
> Either section name is specified in this field or string table offset if 
> section name can't fit into 8 bytes, which is the case here.

Well, yes, I'm certainly aware of that. But the question wasn't about the
format, it was specifically about the hardcoded value 12. Why not 11 or 13?

> Btw this is taken over from linux kernel together with the PE/COFF header 
> structures: https://github.com/torvalds/linux/blob/master/include/linux/pe.h

Which is in no way removing the need for you to be able to explain the
changes you're making.

>> + * struct riscv_image_header - riscv xen image header
> 
> You saying "xen": Is there anything Xen-specific in this struct?
> 
> Not really related to xen, this is generic riscv PE image header, comment 
> fixed in new version
> 
>> +        .long   0                                       /* LoaderFlags */
>> +        .long   (section_table - .) / 8                 /* 
>> NumberOfRvaAndSizes */
>> +        .quad   0                                       /* ExportTable */
>> +        .quad   0                                       /* ImportTable */
>> +        .quad   0                                       /* ResourceTable */
>> +        .quad   0                                       /* ExceptionTable */
>> +        .quad   0                                       /* 
>> CertificationTable */
>> +        .quad   0                                       /* 
>> BaseRelocationTable */
> 
> Would you mind clarifying on what basis this set of 6 entries was
> chosen?
> 
> These fields and their sizes are defined in official PE format, see details 
> from specification bellow
> 
> [cid:542690de-3bb0-4708-a447-996a03277578]

Again, I'm aware of the specification. Yet like the 12 above the 6 here
looks arbitrarily chosen. There are more entries in this table which
are permitted to be present (and well-defined). There could also be
fewer of them; any absent entry is implicitly holding the value 0 afaia.

>> +/* Section table */
>> +section_table:
>> +        .ascii  ".text\0\0\0"
>> +        .long   0
>> +        .long   0
>> +        .long   0                                       /* SizeOfRawData */
>> +        .long   0                                       /* PointerToRawData 
>> */
>> +        .long   0                                       /* 
>> PointerToRelocations */
>> +        .long   0                                       /* 
>> PointerToLineNumbers */
>> +        .short  0                                       /* 
>> NumberOfRelocations */
>> +        .short  0                                       /* 
>> NumberOfLineNumbers */
>> +        .long   IMAGE_SCN_CNT_CODE | \
>> +                IMAGE_SCN_MEM_READ | \
>> +                IMAGE_SCN_MEM_EXECUTE                   /* Characteristics 
>> */
>> +
>> +        .ascii  ".data\0\0\0"
>> +        .long   _end - xen_start                        /* VirtualSize */
>> +        .long   xen_start - efi_head                    /* VirtualAddress */
>> +        .long   __init_end_efi - xen_start              /* SizeOfRawData */
>> +        .long   xen_start - efi_head                    /* PointerToRawData 
>> */
>> +        .long   0                                       /* 
>> PointerToRelocations */
>> +        .long   0                                       /* 
>> PointerToLineNumbers */
>> +        .short  0                                       /* 
>> NumberOfRelocations */
>> +        .short  0                                       /* 
>> NumberOfLineNumbers */
>> +        .long   IMAGE_SCN_CNT_INITIALIZED_DATA | \
>> +                IMAGE_SCN_MEM_READ | \
>> +                IMAGE_SCN_MEM_WRITE                    /* Characteristics */
> 
> IOW no code and the entire image expressed as data. Interesting.
> No matter whether that has a reason or is completely arbitrary, I
> think it, too, wants commenting on.
> 
> This is correct, currently we have extended image with PE/COFF (EFI) header 
> which allows xen boot from EFI loader (or U-boot) environment. And these 
> updates are pure data. We are actively working on the implementation of 
> Boot/Runtime services which shall be in the code section part and enable full 
> UEFI compatible xen application for riscv.

Such a choice, even if transient, needs explaining in the description
(or maybe even a code comment) then.

> Why does the blank line disappear? And why is ...
> 
>>      . = ALIGN(POINTER_ALIGN);
>>      __init_end = .;
> 
> ... __init_end not good enough? (I think I can guess the answer, but
> then I further think the name of the symbol is misleading. )
> 
> Init_end_efi is used only when EFI sections are included into image.

Again, my question was different: I asked why a symbol we have already
isn't good enough, i.e. why another one needs adding.

> We have aligned with arm implementation here, you can take a look also there.

And yet again, as per above, you need to be able to explain your decisions.
You can't just say "it's done this way elsewhere as well". What if that
"elsewhere" has an obvious or maybe just subtle bug?

Jan