Re: [PATCH v2] xen: PE/COFF image header

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2024 20:22, Milan Djokic wrote:
> From: Nikola Jelic <nikola.jelic@xxxxxxxxx>
> 
> Added PE/COFF generic image header which shall be used for EFI
> application format for x86/risc-v. x86 and risc-v source shall be adjusted
> to use this header in following commits. pe.h header is taken over from
> linux kernel with minor changes in terms of formatting and structure member 
> comments.
> Also, COFF relocation and win cert structures are ommited, since these are 
> not relevant for Xen.
> 
> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> 36e4fc57fc16
> 
> Signed-off-by: Nikola Jelic <nikola.jelic@xxxxxxxxx>
> Signed-off-by: Milan Djokic <milan.djokic@xxxxxxxxx>

This looks okay to me now, but I can't ack it (or more precisely my ack
wouldn't mean anything). There are a few style issues in comments, but
leaving them as they are in Linux may be intentional. Just one question,
more to other maintainers than to you:

> +#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE         0x0040
> +#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY      0x0080
> +#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT            0x0100
> +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION          0x0200
> +#define IMAGE_DLLCHARACTERISTICS_NO_SEH                0x0400
> +#define IMAGE_DLLCHARACTERISTICS_NO_BIND               0x0800
> +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER            0x2000
> +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
> +
> +#define IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT         0x0001
> +#define IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT 0x0040

The naming inconsistency (underscore or not after DLL) is somewhat
unhelpful. Do we maybe want to diverge from what Linux has here? Note
that e.g. the GNU binutils header has at least a comment there.

What I'm puzzled by is IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT
having the same value as IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE. Are
these meant to apply to the same field? Or do these values rather
relate to IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS? Some clarification
may be needed here, or the two entries may simply want omitting for
now.

Jan