Re: [PATCH] elf: Set p_align to the common page size if possible

[Florian Weimer <fweimer@redhat.com>]

* H. J. Lu:

> On Mon, Dec 20, 2021 at 11:25 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Mon, Dec 20, 2021 at 10:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> * H. J. Lu:
>> >>
>> >> > We need to set larger p_align > sh_addralign for huge page executables.
>> >> > My current algorithm to decide if p_align should be used as the maximum
>> >> > page size for objcopy is
>> >> >
>> >> > static bool
>> >> > elf_is_p_align_valid (bfd *abfd)
>> >> > {
>> >> >   unsigned int i;
>> >> >   Elf_Internal_Phdr *segment;
>> >> >   unsigned int num_segments;
>> >> >   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>> >> >   bfd_size_type maxpagesize = bed->maxpagesize;
>> >> >   bfd_size_type commonpagesize = bed->commonpagesize;
>> >> >
>> >> >   if (commonpagesize == maxpagesize)
>> >> >     return true;
>> >> >
>> >> >   /* When the common page size != the maximum page size, p_align may
>> >> >      be set to the common page size while segments are aligned to
>> >> >      the maximum page size.  In this case, the input p_align will be
>> >> >      ignored and the maximum page size will be used to align the output
>> >> >      segments.  */
>> >> >   segment = elf_tdata (abfd)->phdr;
>> >> >   num_segments = elf_elfheader (abfd)->e_phnum;
>> >> >   for (i = 0; i < num_segments; i++, segment++)
>> >> >     if (segment->p_type == PT_LOAD
>> >> >         && (segment->p_align != commonpagesize
>> >> >             || vma_page_aligned_bias (segment->p_vaddr,
>> >> >                                       segment->p_offset,
>> >> >                                       maxpagesize) != 0))
>> >> >       return true;
>> >> >
>> >> >   return false;
>> >> > }
>> >> >
>> >> > It should cover all cases.
>> >>
>> >> Will this switch from 64K to 4K on POWER and AArch64?  I think this will
>> >
>> > It will set p_align to ELF_COMMONPAGESIZE.  It glibc doesn't support
>> > p_align == ELF_COMMONPAGESIZE, the ELF_COMMONPAGESIZE
>> > is wrong.
>>
>> Hmm.
>>
>> bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
>> bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
>> bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
>> bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
>> bfd/elf32-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
>> bfd/elf64-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
>> bfd/elfnn-aarch64.c:#define ELF_COMMONPAGESIZE          0x1000
>>
>> This looks problematic.
>>
>> We have a sed patch in binutils.spec to change ELF_COMMONPAGESIZE:
>>
>> # On ppc64 and aarch64, we might use 64KiB pages
>> sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*ppc.c
>> sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*aarch64.c
>> sed -i -e '/common_pagesize/s/4 /64 /' gold/powerpc.cc
>> sed -i -e '/pagesize/s/0x1000,/0x10000,/' gold/aarch64.cc
>
> This should be fixed in binutils upstream.

I've brought this up internally.  To me it looks like binutils is fine,
we tried to workaround the strict p_align stricting in glibc (bug 28688)
for some reason.  This was fifteen years ago, so we'll see if we can
still figure out why we did it this way.

Thanks,
Florian