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

[Milan Đokić <milandjokic1995@xxxxxxxxx>]

Hello Jan,

On Mon, Jul 29, 2024 at 1:42 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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:
>
Are we supposed to CC someone additionally for approval?

> > +#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.
>
These values relate to different fields.
IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE value is related to COFF
optional header, while IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT
is related to debug section directory ( when directory type is
IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS as you assumed). Sorry for the
late response on this one, I was off in the past weeks.

BR,
MIlan