Re: [PATCH 10/13] arm64: extable: add `type` and `data` fields

From: Mark Rutland <mark.rutland@arm.com>
To: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org, alexandru.elisei@arm.com,
	andrii@kernel.org, ardb@kernel.org, ast@kernel.org,
	broonie@kernel.org, catalin.marinas@arm.com,
	daniel@iogearbox.net, dvyukov@google.com, james.morse@arm.com,
	jean-philippe@linaro.org, jpoimboe@redhat.com, maz@kernel.org,
	peterz@infradead.org, robin.murphy@arm.com,
	suzuki.poulose@arm.com
Subject: Re: [PATCH 10/13] arm64: extable: add `type` and `data` fields
Date: Tue, 19 Oct 2021 14:01:34 +0100	[thread overview]
Message-ID: <20211019130134.GC941@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20211019120505.GI13251@willie-the-truck>

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:
> > On Tue, Oct 19, 2021 at 12:29:55PM +0100, Will Deacon wrote:
> > > On Wed, Oct 13, 2021 at 12:00:56PM +0100, Mark Rutland wrote:
> > > > -#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?

[...]

> > > > +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