On Fri, Jun 15, 2018 at 11:58:10AM +0100, James Hogan wrote:
> On Thu, Jun 14, 2018 at 04:52:10PM -0700, Paul Burton wrote:
> > +#define __RSEQ_ASM_DEFINE_TABLE(version, flags,start_ip,
> > \
>
> Nit: technically all these \'s are on 81st column...
True... I'll replace the runs of tabs with a single space.
> > +#define __RSEQ_ASM_DEFINE_ABORT(table_label, label, teardown,
> > \
> > + abort_label, version, flags,
> > \
> > + start_ip, post_commit_offset, abort_ip)
> > \
> > + ".balign 32\n\t"
> > \
>
> ARM doesn't do this for DEFINE_ABORT. Is it intentional that we do for
> MIPS?
We need to align this structure at least in the MIPS64 case because the
.dword directive seems to lead to extra padding if we don't, and that
messes up the offsets of the fields.
For example here's an extract from basic_percpu_ops_test built for
MIPS64r6el without the .balign directive, starting from the
RSEQ_ASM_STORE_RSEQ_CS macro in rseq_cmpeqv_storev():
120001298: df848068ld a0,-32664(gp)
12000129c: fc640008sd a0,8(v1)
1200012a0: 8c640004lw a0,4(v1)
1200012a4: 14820011bne a0,v0,1200012ec <.L4^B1>
1200012a8: nop
1200012ac: dca4ld a0,0(a1)
1200012b0: 14870015bne a0,a3,120001308 <.L5>
1200012b4: nop
1200012b8: fca6sd a2,0(a1)
1200012bc: 100db 1200012f4 <.L5^B1>
1200012c0: nop
1200012c4: nop
1200012c8: nop
1200012cc: nop
1200012d0: 200012a0bovczero,zero,120005d54
<__FRAME_END__+0x3e28>
1200012d4: 00010x1
1200012d8: 001c0x1c
1200012dc: nop
1200012e0: 200012ecbovczero,zero,120005e94
<__FRAME_END__+0x3f68>
1200012e4: 00010x1
1200012e8: 530530530x53053053
...
120012118: 200012c4bovczero,zero,120016c2c <_end+0x49bc>
12001211c: 00010x1
And _gp, which the gp register contains the address of:
$ nm tools/testing/selftests/rseq/basic_percpu_ops_test | grep gp
00012001a0b0 d _gp
The ld instruction @120001298 is what the "dla $4, 3f" expanded to, so
it's loading the address of the table which we're going to write to
rseq_cs. It loads from gp-32664, ie. 0x12001a0b0-32664, ie. 0x120012118,
so the table address loaded is 0x1200012c4.
If we take that as the start of the struct rseq_cs then we get:
1200012c4: __u32 version = 0x0
1200012c8: __u32 flags = 0x0
1200012cc: __u64 start_ip = 200012a0
Where start_ip is both not naturally aligned, so might be slow to access
or involve T&E, and more importantly doesn't have the intended value.
What happened is that gas inserted 4 bytes of padding at 1200012cc to
naturally align the .dword directive for start_ip, and that throws us
off.
Using the .balign directive avoids this, and I went with 32 bytes
because that's what struct rseq_cs is declared with in linux/rseq.h.
The ARM code has probably gotten away with it because it's 32 bit only,
so isn't emitting any 64 bit values (though if it did I don't know what
an ARM64 assembler would do with regards to alignment & padding anyway).
> Otherwise this whole series looks reasonable to me, so feel free to add
> my rb on the whole series if you do apply youself:
>
> Reviewed-by: James Hogan
Thanks James :)
Paul