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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Mon, Jul 29, 2024 at 01:42:46PM +0200, Jan Beulich wrote:
> On 23.07.2024 20:22, Milan Djokic wrote:
> > From: Nikola Jelic <nikola.jelic@rt-rk.com>
> > 
> > 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@rt-rk.com>
> > Signed-off-by: Milan Djokic <milan.djokic@rt-rk.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> 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.

Indeed it doesn't look great, but IMO leaving it consistent with Linux
is okay as it ease updating and porting/comparing other code if needed.

> 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.

One has _EX_ infix and the other doesn't so IMO together with visual
separation it's clear they apply to a different field.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]