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

[Milan Đokić <milandjokic1995@xxxxxxxxx>]

On Thu, Jun 27, 2024 at 10:55 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 26.06.2024 18:16, Milan Djokic wrote:
> >> +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.
>
My assumption regarding option value when default is not specified was
wrong. You're correct, we'll omit the default line.

>
> > 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?
>
I've misinterpreted your original question here. I realize now that
this comment ("name or /12/0") is confusing (and incorrect). It was
taken over from linux kernel which on the other hand took this over
from pesign package,  so I assume that pesign had its own string table
layout and thus hardcoded 12 offset for its specific usecase. Since in
general we can have different offsets and even more than one (if e.g.
2 section names exceed 8-byte size) we'll change this comment not to
contain 12 offset hint.

>
> >> + * 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.
>
We can omit all of them since directories are not used at all in this
case. Even those 6 are set to 0 (which means not used according to
PE). One more case where we wanted to align with linux kernel / xen
arm implementation, but it is redundant in our case

> >> +/* 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.
We'll clarify this part in code directly

>
> > 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.
>
Similar as for data directories fields above, _init_end_efi is also
redundant for RISC-V case, we'll use _init_end directly instead.

> > 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?
This is perfectly clear. We'll restructure our changes in the next
version in that manner.

BR,
Milan