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

[Will Deacon <will@kernel.org>]

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:
> > > Subsequent patches will add specialized handlers for fixups, in addition
> > > to the simple PC fixup and BPF handlers we have today. In preparation,
> > > this patch adds a new `type` field to struct exception_table_entry, and
> > > uses this to distinguish the fixup and BPF cases. A `data` field is also
> > > added so that subsequent patches can associate data specific to each
> > > exception site (e.g. register numbers).
> > > 
> > > Handlers are named ex_handler_*() for consistency, following the exmaple
> > > of x86. At the same time, get_ex_fixup() is split out into a helper so
> > > that it can be used by other ex_handler_*() functions ins subsequent
> > > patches.
> > > 
> > > This patch will increase the size of the exception tables, which will be
> > > remedied by subsequent patches removing redundant fixup code. There
> > > should be no functional change as a result of this patch.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: Andrii Nakryiko <andrii@kernel.org>
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/asm-extable.h | 32 ++++++++++++++++++++------------
> > >  arch/arm64/include/asm/extable.h     | 19 +++++++++++++++----
> > >  arch/arm64/mm/extable.c              | 29 +++++++++++++++++++++++++----
> > >  arch/arm64/net/bpf_jit_comp.c        |  7 +++++--
> > >  scripts/sorttable.c                  | 30 ++++++++++++++++++++++++++++++
> > >  5 files changed, 95 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h
> > > index 986b4c0d4792..5ee748edaef1 100644
> > > --- a/arch/arm64/include/asm/asm-extable.h
> > > +++ b/arch/arm64/include/asm/asm-extable.h
> > > @@ -2,13 +2,19 @@
> > >  #ifndef __ASM_ASM_EXTABLE_H
> > >  #define __ASM_ASM_EXTABLE_H
> > >  
> > > +#define EX_TYPE_NONE			0
> > > +#define EX_TYPE_FIXUP			1
> > > +#define EX_TYPE_BPF			2
> > > +
> > >  #ifdef __ASSEMBLY__
> > >  
> > > -#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?

> > > diff --git a/scripts/sorttable.c b/scripts/sorttable.c
> > > index 6ee4fa882919..ee95bb47a50d 100644
> > > --- a/scripts/sorttable.c
> > > +++ b/scripts/sorttable.c
> > > @@ -231,6 +231,34 @@ static void sort_relative_table(char *extab_image, int image_size)
> > >  	}
> > >  }
> > >  
> > > +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.

Will

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