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

["H.J. Lu" <hjl.tools@gmail.com>]

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.

> So I guess we paper over the problem.  But we reject to run-time load
> binaries without this patch due to bug 28688.
>
> I think we need to fix glibc bug 28688 and drop our downstream
> workaround from our binutils builds.  We tried to fix it in the wrong
> place, it seems.
>
> This lack of cross-distro interoperability is embarrassing. 8-(
>
> Thanks,
> Florian
>


-- 
H.J.