On Tue, Oct 19, 2021 at 01:05:06PM +0100, Will Deacon wrote:

On Tue, Oct 19, 2021 at 12:50:22PM +0100, Mark Rutland wrote:

quoted

On Tue, Oct 19, 2021 at 12:29:55PM +0100, Will Deacon wrote:

quoted

On Wed, Oct 13, 2021 at 12:00:56PM +0100, Mark Rutland wrote:

quoted

-#define __ASM_EXTABLE_RAW(insn, fixup)		\
-	.pushsection	__ex_table, "a";	\
-	.align		3;			\
-	.long		((insn) - .);		\
-	.long		((fixup) - .);		\
+#define __ASM_EXTABLE_RAW(insn, fixup, type, data)	\
+	.pushsection	__ex_table, "a";		\
+	.align		2;				\
+	.long		((insn) - .);			\
+	.long		((fixup) - .);			\
+	.short		(type);				\
+	.short		(data);				\

Why are you reducing the alignment here?

That's because the size of each entry is now 12 bytes, and 
`.align 3` aligns to 8 bytes, which would leave a gap between entries.
We only require the fields are naturally aligned, so `.align 2` is
sufficient, and doesn't waste space.

I'll update the commit message to call that out.

I think the part which is confusing me is that I would expect the alignment
here to match the alignment of the corresponding C type, but the old value
of '3' doesn't seem to do that, so is this patch fixing an earlier bug?

Without your patches in the picture, we're using a '.align 3' in
_asm_extable, but with:

struct exception_table_entry
{
	int insn, fixup;
};

I suppose it works out because that over-alignment doesn't result in any
additional padding, but I think we could reduce the current alignment
without any of these other changes, no?

Yes, we could reduce that first, but no, it's not a bug -- there's no
functional issue today.

For context, today the `__ex_table` section as a whole and the
`__start___ex_table` symbol also got 8 byte alignment, since in
ARch/arm64/kernel/vmlinux.lds.S we have:

| #define RO_EXCEPTION_TABLE_ALIGN        8

... and so in include/asm-generic/vmlinux.lds.h when the exception table
gets output with:

| EXCEPTION_TABLE(RO_EXCEPTION_TABLE_ALIGN)

| #define EXCEPTION_TABLE(align)                                          \
|         . = ALIGN(align);                                               \
|         __ex_table : AT(ADDR(__ex_table) - LOAD_OFFSET) {               \
|                 __start___ex_table = .;                                 \
|                 KEEP(*(__ex_table))                                     \
|                 __stop___ex_table = .;                                  \
|         }

If you want, I can split out a preparatory patch which drops the
alignment to the minimum necessary, both in the asm and for
RO_EXCEPTION_TABLE_ALIGN?

[...]

quoted

quoted

quoted

+static void arm64_sort_relative_table(char *extab_image, int image_size)
+{
+	int i = 0;
+
+	while (i < image_size) {
+		uint32_t *loc = (uint32_t *)(extab_image + i);
+
+		w(r(loc) + i, loc);
+		w(r(loc + 1) + i + 4, loc + 1);
+		/* Don't touch the fixup type or data */
+
+		i += sizeof(uint32_t) * 3;
+	}
+
+	qsort(extab_image, image_size / 12, 12, compare_relative_table);
+
+	i = 0;
+	while (i < image_size) {
+		uint32_t *loc = (uint32_t *)(extab_image + i);
+
+		w(r(loc) - i, loc);
+		w(r(loc + 1) - (i + 4), loc + 1);
+		/* Don't touch the fixup type or data */
+
+		i += sizeof(uint32_t) * 3;
+	}
+}

This is very nearly a direct copy of x86_sort_relative_table() (magic
numbers and all). It would be nice to tidy that up, but I couldn't
immediately see a good way to do it :(

Beware that's true in linux-next, but not mainline, as that changes in
commit:

  46d28947d9876fc0 ("x86/extable: Rework the exception table mechanics")

A patch to unify the two is trivial, but will cause a cross-tree
dependency, so I'd suggest having this separate for now and sending a
unification patch come -rc1.

I can note something to that effect in the commit message, if that
helps?

Yeah, I suppose. It's not worth tripping over the x86 changes, but we
should try to remember to come back and unify things.

Sure; works for me.

Thanks,
Mark.

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