| Message ID | 1560975087-25632-4-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/link: Fixes and improvements to Xen's linking | expand |
On 6/19/19 9:11 PM, Andrew Cooper wrote: > .data.read_mostly only needs separating from adjacent data by one cache line > to be effective, and placing it adjacent to .data.page_aligned fulfills this > requirement. > > For ARM, .ex_table appears to be a vestigial remnant. Nothing in the > resulting build ever inspects or acts on the contents of the table. The arm32 > build does however have some assembly routines which fill .ex_table. > > Drop all of ARM's .ex_table infrastructure, to reduce the size of the compiled > binary, and avoid giving the illusion of exception handling working. I am not in favor of this change. assembler.h is meant to be a verbatim copy of the Linux counterpart. [...] > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 2b44e5d..3dc5117 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -94,27 +94,13 @@ SECTIONS > _erodata = .; /* End of read-only data */ > > .data : { /* Data */ > + *(.data.read_mostly) Before, .data.read_mostly was SMP_CACHE_BYTES aligned. Now, it seems there are no alignment. This may end up to have _erodata and .data.read_mostly to be part of the same page. As Arm enforce read-only, you may crash on access to .data.read_mostly. So I think you have . = ALIGN(PAGE_SIZE) *(.data.read_mostly) .align(SMP_CACHE_BYTES). Cheers,
On 19/06/2019 22:28, Julien Grall wrote: > On 6/19/19 9:11 PM, Andrew Cooper wrote: >> .data.read_mostly only needs separating from adjacent data by one >> cache line >> to be effective, and placing it adjacent to .data.page_aligned >> fulfills this >> requirement. >> >> For ARM, .ex_table appears to be a vestigial remnant. Nothing in the >> resulting build ever inspects or acts on the contents of the table. >> The arm32 >> build does however have some assembly routines which fill .ex_table. >> >> Drop all of ARM's .ex_table infrastructure, to reduce the size of the >> compiled >> binary, and avoid giving the illusion of exception handling working. > > I am not in favor of this change. assembler.h is meant to be a > verbatim copy of the Linux counterpart. What alternative do you propose then, because having infrastructure that gives the illusion of exception recovery working is a far worse option than deviating from Linux. > > [...] > >> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >> index 2b44e5d..3dc5117 100644 >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -94,27 +94,13 @@ SECTIONS >> _erodata = .; /* End of read-only data */ >> .data : { /* Data */ >> + *(.data.read_mostly) > > Before, .data.read_mostly was SMP_CACHE_BYTES aligned. Now, it seems > there are no alignment. > > This may end up to have _erodata and .data.read_mostly to be part of > the same page. As Arm enforce read-only, you may crash on access to > .data.read_mostly. > > So I think you have > . = ALIGN(PAGE_SIZE) > *(.data.read_mostly) > .align(SMP_CACHE_BYTES). These both need to be PAGE_SIZE, or .data.page_aligned can end up having problems. ~Andrew
Hi Andrew, On 6/19/19 10:48 PM, Andrew Cooper wrote: > On 19/06/2019 22:28, Julien Grall wrote: >> On 6/19/19 9:11 PM, Andrew Cooper wrote: >>> .data.read_mostly only needs separating from adjacent data by one >>> cache line >>> to be effective, and placing it adjacent to .data.page_aligned >>> fulfills this >>> requirement. >>> >>> For ARM, .ex_table appears to be a vestigial remnant. Nothing in the >>> resulting build ever inspects or acts on the contents of the table. >>> The arm32 >>> build does however have some assembly routines which fill .ex_table. >>> >>> Drop all of ARM's .ex_table infrastructure, to reduce the size of the >>> compiled >>> binary, and avoid giving the illusion of exception handling working. >> >> I am not in favor of this change. assembler.h is meant to be a >> verbatim copy of the Linux counterpart. > > What alternative do you propose then, because having infrastructure that > gives the illusion of exception recovery working is a far worse option > than deviating from Linux. I learnt the hard way before that trying to adapt a Linux file to Xen makes very difficult to keep track what's going on. So my preference here is to just disable the section if they exists. >> >> [...] >> >>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>> index 2b44e5d..3dc5117 100644 >>> --- a/xen/arch/arm/xen.lds.S >>> +++ b/xen/arch/arm/xen.lds.S >>> @@ -94,27 +94,13 @@ SECTIONS >>> _erodata = .; /* End of read-only data */ >>> .data : { /* Data */ >>> + *(.data.read_mostly) >> >> Before, .data.read_mostly was SMP_CACHE_BYTES aligned. Now, it seems >> there are no alignment. >> >> This may end up to have _erodata and .data.read_mostly to be part of >> the same page. As Arm enforce read-only, you may crash on access to >> .data.read_mostly. >> >> So I think you have >> . = ALIGN(PAGE_SIZE) >> *(.data.read_mostly) >> .align(SMP_CACHE_BYTES). > > These both need to be PAGE_SIZE, or .data.page_aligned can end up having > problems. Good point, I missed the .data.page_aligned. Cheers,
>>> On 19.06.19 at 22:11, <andrew.cooper3@citrix.com> wrote: > .data.read_mostly only needs separating from adjacent data by one cache line > to be effective, and placing it adjacent to .data.page_aligned fulfills this > requirement. > > For ARM, .ex_table appears to be a vestigial remnant. Nothing in the > resulting build ever inspects or acts on the contents of the table. The arm32 > build does however have some assembly routines which fill .ex_table. > > Drop all of ARM's .ex_table infrastructure, to reduce the size of the compiled > binary, and avoid giving the illusion of exception handling working. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Without you undoing part of 01fe4da624 ("x86: force suitable alignment in sources rather than in linker script"), i.e. with the ALIGN(PAGE_SIZE) dropped, x86 part Acked-by: Jan Beulich <jbeulich@suse.com> Yet FTR - I don't think there was anything wrong with it having its own section in the final binary. Of course there's also nothing wrong with it getting folded into .data, in particular with it sitting ahead of .data.page_aligned anyway. Jan
diff --git a/xen/arch/arm/arm32/lib/assembler.h b/xen/arch/arm/arm32/lib/assembler.h index 6de2638..42eaabb 100644 --- a/xen/arch/arm/arm32/lib/assembler.h +++ b/xen/arch/arm/arm32/lib/assembler.h @@ -160,13 +160,6 @@ restore_irqs_notrace \oldcpsr .endm -#define USER(x...) \ -9999: x; \ - .pushsection __ex_table,"a"; \ - .align 3; \ - .long 9999b,9001f; \ - .popsection - #ifdef CONFIG_SMP #define ALT_SMP(instr...) \ 9998: instr @@ -247,7 +240,7 @@ #ifdef CONFIG_THUMB2_KERNEL .macro usraccoff, instr, reg, ptr, inc, off, cond, abort, t=T() -9999: + .if \inc == 1 \instr\cond\()b\()\t\().w \reg, [\ptr, #\off] .elseif \inc == 4 @@ -256,10 +249,6 @@ .error "Unsupported inc macro argument" .endif - .pushsection __ex_table,"a" - .align 3 - .long 9999b, \abort - .popsection .endm .macro usracc, instr, reg, ptr, inc, cond, rept, abort @@ -288,7 +277,7 @@ .macro usracc, instr, reg, ptr, inc, cond, rept, abort, t=T() .rept \rept -9999: + .if \inc == 1 \instr\cond\()b\()\t \reg, [\ptr], #\inc .elseif \inc == 4 @@ -297,10 +286,6 @@ .error "Unsupported inc macro argument" .endif - .pushsection __ex_table,"a" - .align 3 - .long 9999b, \abort - .popsection .endr .endm diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 2b44e5d..3dc5117 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -94,27 +94,13 @@ SECTIONS _erodata = .; /* End of read-only data */ .data : { /* Data */ + *(.data.read_mostly) . = ALIGN(PAGE_SIZE); *(.data.page_aligned) *(.data) *(.data.*) } :text - . = ALIGN(SMP_CACHE_BYTES); - .data.read_mostly : { - /* Exception table */ - __start___ex_table = .; - *(.ex_table) - __stop___ex_table = .; - - /* Pre-exception table */ - __start___pre_ex_table = .; - *(.ex_table.pre) - __stop___pre_ex_table = .; - - *(.data.read_mostly) - } :text - . = ALIGN(8); .arch.info : { _splatform = .; diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 9fa6c99..ef11949 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -264,12 +264,9 @@ SECTIONS __2M_init_end = .; __2M_rwdata_start = .; /* Start of 2M superpages, mapped RW. */ - . = ALIGN(SMP_CACHE_BYTES); - DECL_SECTION(.data.read_mostly) { - *(.data.read_mostly) - } :text - DECL_SECTION(.data) { + *(.data.read_mostly) + . = ALIGN(PAGE_SIZE); *(.data.page_aligned) *(.data) *(.data.*)
.data.read_mostly only needs separating from adjacent data by one cache line to be effective, and placing it adjacent to .data.page_aligned fulfills this requirement. For ARM, .ex_table appears to be a vestigial remnant. Nothing in the resulting build ever inspects or acts on the contents of the table. The arm32 build does however have some assembly routines which fill .ex_table. Drop all of ARM's .ex_table infrastructure, to reduce the size of the compiled binary, and avoid giving the illusion of exception handling working. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> Only compile tested on ARM. --- xen/arch/arm/arm32/lib/assembler.h | 19 ++----------------- xen/arch/arm/xen.lds.S | 16 +--------------- xen/arch/x86/xen.lds.S | 7 ++----- 3 files changed, 5 insertions(+), 37 deletions(-)