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 12:50:22 +0100	[thread overview]
Message-ID: <20211019115022.GB941@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20211019112954.GG13251@willie-the-truck>

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.

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

Thanks,
Mark.

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