On Thu, 20 Feb 2020 at 18:28, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > On Thu, Feb 20, 2020 at 12:06:47PM +0100, Ard Biesheuvel wrote: > > Replace bare numbers in the PE/COFF header structure with symbolic > > constants so they become self documenting. > > > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > > --- > > arch/x86/boot/header.S | 60 ++++++++++---------- > > 1 file changed, 31 insertions(+), 29 deletions(-) > > > > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S > > index 44aeb63ca6ae..9110b58aa2ec 100644 > > --- a/arch/x86/boot/header.S > > +++ b/arch/x86/boot/header.S > > @@ -210,7 +200,10 @@ section_table: > > .long 0 # PointerToLineNumbers > > .word 0 # NumberOfRelocations > > .word 0 # NumberOfLineNumbers > > - .long 0x60500020 # Characteristics (section flags) > > + .long IMAGE_SCN_CNT_CODE | \ > > + IMAGE_SCN_MEM_READ | \ > > + IMAGE_SCN_MEM_EXECUTE | \ > > + IMAGE_SCN_ALIGN_16BYTES # Characteristics > > The IMAGE_SCN_ALIGN bits are marked as "valid only for object files". > Can they be removed? > Yes, they probably should. I just didn't want to do it in the same patch. > > > > # > > # The EFI application loader requires a relocation section > > @@ -228,7 +221,10 @@ section_table: > > .long 0 # PointerToLineNumbers > > .word 0 # NumberOfRelocations > > .word 0 # NumberOfLineNumbers > > - .long 0x42100040 # Characteristics (section flags) > > + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \ > > + IMAGE_SCN_MEM_READ | \ > > + IMAGE_SCN_MEM_DISCARDABLE | \ > > + IMAGE_SCN_ALIGN_1BYTES # Characteristics > > > > #ifdef CONFIG_EFI_MIXED > > # > > @@ -244,7 +240,10 @@ section_table: > > .long 0 # PointerToLineNumbers > > .word 0 # NumberOfRelocations > > .word 0 # NumberOfLineNumbers > > - .long 0x42100040 # Characteristics (section flags) > > + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \ > > + IMAGE_SCN_MEM_READ | \ > > + IMAGE_SCN_MEM_DISCARDABLE | \ > > + IMAGE_SCN_ALIGN_1BYTES # Characteristics > > #endif > > > > # > > @@ -263,7 +262,10 @@ section_table: > > .long 0 # PointerToLineNumbers > > .word 0 # NumberOfRelocations > > .word 0 # NumberOfLineNumbers > > - .long 0x60500020 # Characteristics (section flags) > > + .long IMAGE_SCN_CNT_CODE | \ > > + IMAGE_SCN_MEM_READ | \ > > + IMAGE_SCN_MEM_EXECUTE | \ > > + IMAGE_SCN_ALIGN_16BYTES # Characteristics > > The .text section contains data as well. Shouldn't it be marked > IMAGE_SCN_MEM_WRITE also? > Another good point. For ARM and arm64, I actually changed this into split R-X and RW- sections, in case the loader decides to honour these attributes (which it should imo) So yes, we should either add IMAGE_SCN_MEM_WRITE, or add a .data section (although we probably don't have the space for that). Another thing I wondered was whether we really need the .reloc section. We don't have one on ARM, and it works fine with the existing EDK2 loader. Peter: any idea whether the issue with .reloc you pointed out to me the other day still exists in EDK2 today?
