Re: [PATCH] arm64: Add support for relocating the kernel with RELR relocations

[Nick Desaulniers <ndesaulniers@google.com>]

On Tue, Jul 9, 2019 at 3:04 PM Peter Collingbourne <pcc@google.com> wrote:
>
> On Mon, Jul 8, 2019 at 11:02 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Fri, Jul 5, 2019 at 1:03 AM 'Peter Collingbourne' via Clang Built
> > Linux <clang-built-linux@googlegroups.com> wrote:
> > > +config RELR
> > > +       bool "Use RELR relocation packing"
> > > +       depends on RELOCATABLE && $(ld-option,--pack-dyn-relocs=relr)
> >
> > Oh, ld-option in Kconfig? +Masahiro
> >
> > > +       help
> > > +         Store the kernel's dynamic relocations in the RELR relocation packing
> > > +         format. Requires a compatible linker (currently only LLD supports
> > > +         this feature), as well as compatible NM and OBJCOPY utilities
> > > +         (llvm-nm and llvm-objcopy are compatible).
> >
> > So sounds like `make LD=ld.lld NM=llvm-nm OBJCOPY=llvm-objcopy` will
> > be needed to test.  The ld-option check above doesn't seem strong
> > enough, but maybe it's not easy to feature test NM or OBJCOPY?
>
> Right. Ideally we want to test the property that the tool accepts an
> input file with a .relr.dyn section, and this isn't easy without
> actually creating such a file. We could test that the tools are
> actually the LLVM versions (e.g. by testing the output of $TOOL
> --version), but I'm not sure if we want to exclude the possibility
> that GNU or other toolchains will add support for this section in the
> future.

eh, I strongly dislike version checks due to their brittleness.
https://lkml.org/lkml/2019/6/25/1253
Maybe a script like `scripts/cc-can-link.sh` ?

> > >         add     x9, x9, x11                     // __va(.rela)
> > >         add     x10, x9, x10                    // __va(.rela) + sizeof(.rela)
> > >
> > >  0:     cmp     x9, x10
> > >         b.hs    1f
> > > -       ldp     x11, x12, [x9], #24
> > > -       ldr     x13, [x9, #-8]
> > > -       cmp     w12, #R_AARCH64_RELATIVE
> > > +       ldp     x12, x13, [x9], #24
> > > +       ldr     x14, [x9, #-8]
> > > +       cmp     w13, #R_AARCH64_RELATIVE
> >
> > Can you help me understand this renaming?
> > x11 -> x12
> > x13 -> x14
> > x12 -> x13
> > but they all get clobbered before use in your added ifdef hunk?
>
> I use the value of x11 before it is clobbered in the instruction with
> the comment "// __va(.relr)".
>
> > >         b.ne    0b
> > > -       add     x13, x13, x23                   // relocate
> > > -       str     x13, [x11, x23]
> > > +       add     x14, x14, x23                   // relocate
> > > +       str     x14, [x12, x23]
> > >         b       0b
> > > -1:     ret
> > > +
> > > +1:
> > > +#ifdef CONFIG_RELR
> > > +       ldr     w9, =__relr_offset              // offset to reloc table
> > > +       ldr     w10, =__relr_size               // size of reloc table
> >
> > Were these modified since first loaded in the above hunk?  I see the
> > offsets applied below, but I don't spot any loops back up to `1:` (but
> > could be missing it).  It also doesn't look like x11 or x10 are
> > modified below (or above), so this looks like we're rematerializing
> > values that already exist in those registers, IIUC?  Maybe I'm missing
> > some side effect of one of the instructions?
>
> These instructions refer to __relr_{offset,size} which are different
> from __rela_{offset,size} loaded above. They are only loaded once per
> function call; the main loop is between labels 2 and 7 below.

oh, sorry, yes I missed __relr_ vs __rela__.  Thanks for the clarification.

>
> You might have missed the implicit increment of x9 by 8 in the "ldr
> x11, [x9], #8" instruction below (see also similar instructions
> above), which is how we move to the next relocation table entry in the
> main loop.

Yep, I forgot about the pre vs post increment syntax:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.kui0100a/armasm_cihgjhed.htm
https://azeria-labs.com/memory-instructions-load-and-store-part-4/

> > > +6:     add     x13, x13, #(8 * 63)             // move to next bitmap's address
> >
> > Sorry, what's this constant `#(8 * 63)`?
>
> It is the number of bytes covered by a bitmap entry. 8 is the word
> size, and 63 is the number of significant bits in a bitmap entry.

Might be good to add that as a comment inline w/ the code?

Pulling down the patch to test.
-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel