Re: [PATCH, BINUTILS, AARCH64, 5/8] Add Tag getting instruction in Memory Tagging Extension

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 08/11/2018 10:34, Sudakshina Das wrote:
> On 02/11/18 16:09, Sudakshina Das wrote:
>> Hi Richard
>>
>> On 30/10/18 10:15, Richard Earnshaw (lists) wrote:
>>> On 09/10/2018 18:25, Sudakshina Das wrote:
>>>> Hi
>>>>
>>>> This patch is part of the patch series to add support for ARMv8.5-A
>>>> Memory Tagging Extensions.
>>>> (https://developer.arm.com/products/architecture/cpu-architecture/a-profile/exploration-tools) 
>>>>
>>>>
>>>> Memory Tagging Extension (MTE) is an optional extension to
>>>> ARMv8.5-A and is enabled using the +memtag command line option.
>>>>
>>>> This patch add support to the Tag Getting instruction from
>>>> MTE:
>>>> - LDG <Xt>, [<Xn|SP>, #<simm>]
>>>>
>>>> where
>>>> <Xt> : Is the 64-bit destination GPR.
>>>> <Xn|SP> : Is the 64-bit first source GPR or Stack pointer.
>>>> <simm> : Is the optional signed immediate offset, a multiple of 16
>>>> in the range of -4096 and 4080, defaulting to 0.
>>>>
>>>> Testing done: Builds and reg tests all pass on aarch64-none-linux-gnu.
>>>> Added tests.
>>>>
>>>> Is this ok for trunk?
>>>>
>>>> Thanks
>>>> Sudi
>>>>
>>>> *** opcodes/ChangeLog ***
>>>>
>>>> 2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>
>>>>
>>>>      * aarch64-tbl.h (QL_LDG): New.
>>>>      (aarch64_opcode_table): Add ldg.
>>>>      * aarch64-asm-2.c: Regenarated.
>>>>      * aarch64-dis-2.c: Regenerated.
>>>>      * aarch64-opc-2.c: Regenerated.
>>>>
>>>> *** gas/ChangeLog ***
>>>>
>>>> 2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>
>>>>
>>>>      * testsuite/gas/aarch64/armv8_5-a-mte.s: Add tests for ldg.
>>>>      * testsuite/gas/aarch64/armv8_5-a-mte.d: Likewise.
>>>>
>>>>
>>>
>>> Same mte/memtag issue as earlier patches.
>>>
>>> What's the rationale behind this set of test instructions?
>>>
>>>
>>> +.*:    d96001d5     ldg    x21, \[x14\]
>>> +.*:    d960a1d5     ldg    x21, \[x14, #160\]
>>> +.*:    d961e1d5     ldg    x21, \[x14, #480\]
>>> +.*:    d962d1d5     ldg    x21, \[x14, #720\]
>>> +.*:    d965a1d5     ldg    x21, \[x14, #1440\]
>>> +.*:    d96c81d5     ldg    x21, \[x14, #3200\]
>>> +.*:    d96f51d5     ldg    x21, \[x14, #3920\]
>>> +.*:    d97fb30c     ldg    x12, \[x24, #-80\]
>>> +.*:    d97ec30c     ldg    x12, \[x24, #-320\]
>>> +.*:    d97dd30c     ldg    x12, \[x24, #-560\]
>>> +.*:    d97d330c     ldg    x12, \[x24, #-720\]
>>> +.*:    d97a530c     ldg    x12, \[x24, #-1456\]
>>> +.*:    d973730c     ldg    x12, \[x24, #-3216\]
>>> +.*:    d970330c     ldg    x12, \[x24, #-4048\]
>>> +.*:    d96ff3e1     ldg    x1, \[sp, #4080\]
>>> +.*:    d97003eb     ldg    x11, \[sp, #-4096\]
>>>
>>> It doesn't seem particularly scientific to me.
>>>
>>> Generally gas tests need to:
>>>
>>> - test the base encoding of the instruction (try to find (a minimal set
>>> of) examples where all the operand fields are not set)
>>> - test each operand independently with a minimal set of tests that
>>> ensure the bits are encoded in the correct places.  eg if you have a
>>> contiguous 5-bit field, try to set all the bits in it: if x31/sp, that
>>> is sufficient.  Fields that have multiple insertion points (ie are split
>>> across the instruction need a bit more care).
>>> - a couple of random values, just to cover some other cases - try to
>>> avoid combinations that look too similar to other tests in the suite.
>>>
>>> - test that invalid operands are rejected (don't permit xzr/sp if the
>>> instruction should not allow it, reject invalid immediates - don't
>>> forget invalid cases like not-a-multiple-of as well as too large/small).
>>>
>>> So tests here should probably be something like:
>>>
>>> ldg    x0, [x0]    // base pattern
>>> ldg    x21, [x0]    // sets top and bottom bits of load register
>>> ldg    x0, [sp]    // top-and-bottom of address
>>> ldg    x0, [x0, #-4096] // minimum negative offset
>>> ldg    x0, [x0, #4080] // maximum positive offset
>>> ldg    x8, [x10, #720] // random tests...
>>>
>>> ldg    xzr, [x0]    // error, xzr not valid
>>> ldg    x0, [x0, #3]    // error, invalid offset
>>> ...
>>>
>>> If you have access to another disassembler, then results should be cross
>>> checked with that.  Otherwise each test value needs to be manually
>>> verified for accuracy (otherwise the test doesn't tell us anything).
>>>
>>
>> As answered before on patch 2/8
>> Thanks you for test case suggestions. I have also updated the tests and
>> added a test for the illegal tests. The illegal testing has also
>> allowed me to fish out a couple of bugs.
>>
>> I have changed the general format of the positive tests to make it
>> better to read. All macros are moved on top and every instruction has
>> the following testing pattern:
>> 1) Base test with x0 and #0 depending on the instruction
>> 2) Macro call to add some random tests of different combinations.
>> 3) Special casing some SP register and immediate corner cases depending
>> on the instruction.
>>
>>> R.
>>>
>> *** opcodes/ChangeLog ***
>>
>> 2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * aarch64-tbl.h (QL_LDG): New.
>>      (aarch64_opcode_table): Add ldg.
>>      * aarch64-asm-2.c: Regenerated.
>>      * aarch64-dis-2.c: Regenerated.
>>      * aarch64-opc-2.c: Regenerated.
>>
>> *** gas/ChangeLog ***
>>
>> 2018-xx-xx  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * testsuite/gas/aarch64/armv8_5-a-memtag.s: Add tests for ldg.
>>      * testsuite/gas/aarch64/armv8_5-a-memtag.d: Likewise.
>>      * testsuite/gas/aarch64/illegal-memtag.s: Likewise.
>>      * testsuite/gas/aarch64/illegal-memtag.l: Likewise.
>>
>> Thanks
>> Sudi
>>
> New patch. Changelog still applies.
> 

OK.

R.

> Sudi
> 
>>
> 
> 
> 
> rb10009.patch
> 
> diff --git a/gas/testsuite/gas/aarch64/armv8_5-a-memtag.d b/gas/testsuite/gas/aarch64/armv8_5-a-memtag.d
> index 41406464bb84731e6bc65aa65f008efec6def905..363bbe2eb3b1da373737c7d6efce9eba3b6bb6a1 100644
> --- a/gas/testsuite/gas/aarch64/armv8_5-a-memtag.d
> +++ b/gas/testsuite/gas/aarch64/armv8_5-a-memtag.d
> @@ -113,3 +113,11 @@ Disassembly of section \.text:
>  .*:	69207c00 	stgp	x0, xzr, \[x0, #-1024\]
>  .*:	699f83e0 	stgp	x0, x0, \[sp, #1008\]!
>  .*:	68a003e0 	stgp	x0, x0, \[sp\], #-1024
> +.*:	d9600000 	ldg	x0, \[x0\]
> +.*:	d960001b 	ldg	x27, \[x0\]
> +.*:	d9600360 	ldg	x0, \[x27\]
> +.*:	d960037b 	ldg	x27, \[x27\]
> +.*:	d96003e0 	ldg	x0, \[sp\]
> +.*:	d960001f 	ldg	xzr, \[x0\]
> +.*:	d96ff000 	ldg	x0, \[x0, #4080\]
> +.*:	d9700000 	ldg	x0, \[x0, #-4096\]
> diff --git a/gas/testsuite/gas/aarch64/armv8_5-a-memtag.s b/gas/testsuite/gas/aarch64/armv8_5-a-memtag.s
> index 729dae6f49eb74c0e11c4ff0ed02f4ab22e5bcdf..62c9436d78fea63a8ac45126c5f85416a1487822 100644
> --- a/gas/testsuite/gas/aarch64/armv8_5-a-memtag.s
> +++ b/gas/testsuite/gas/aarch64/armv8_5-a-memtag.s
> @@ -89,3 +89,12 @@ func:
>  	stgp x0, xzr, [x0, #-1024]
>  	stgp x0, x0, [sp, #1008]!
>  	stgp x0, x0, [sp], #-1024
> +
> +	ldg x0, [x0, #0]
> +	ldg x27, [x0, #0]
> +	ldg x0, [x27, #0]
> +	ldg x27, [x27, #0]
> +	ldg x0, [sp, #0]
> +	ldg xzr, [x0, #0]
> +	ldg x0, [x0, #4080]
> +	ldg x0, [x0, #-4096]
> diff --git a/gas/testsuite/gas/aarch64/illegal-memtag.l b/gas/testsuite/gas/aarch64/illegal-memtag.l
> index dd568ccd4eea69d329343cc66ef1d2868968b6c5..dfdf00aba1ea3e21c29ca602de30336b3f00859b 100644
> --- a/gas/testsuite/gas/aarch64/illegal-memtag.l
> +++ b/gas/testsuite/gas/aarch64/illegal-memtag.l
> @@ -7,6 +7,8 @@
>  [^:]*:[0-9]+: Error: immediate value must be a multiple of 16 at operand 1 -- `stg \[x1,#15\]'
>  [^:]*:[0-9]+: Error: immediate offset out of range -4096 to 4080 at operand 1 -- `stzg \[x1,#-4097]!'
>  [^:]*:[0-9]+: Error: immediate offset out of range -4096 to 4080 at operand 1 -- `st2g \[x1],#4096'
> +[^:]*:[0-9]+: Error: immediate value must be a multiple of 16 at operand 2 -- `ldg x1,\[x2,#33\]'
> +[^:]*:[0-9]+: Error: immediate offset out of range -4096 to 4080 at operand 2 -- `ldg x1,\[x2,#4112\]'
>  [^:]*:[0-9]+: Error: immediate offset out of range -1024 to 1008 at operand 3 -- `stgp x1,x2,\[x3,#1009\]'
>  [^:]*:[0-9]+: Error: immediate value must be a multiple of 16 at operand 3 -- `stgp x1,x2,\[x3,#33\]'
>  [^:]*:[0-9]+: Error: immediate offset out of range -1024 to 1008 at operand 3 -- `stgp x1,x2,\[x3,#-1025\]'
> @@ -33,3 +35,5 @@
>  [^:]*:[0-9]+: Error: operand 1 must be an integer register -- `stgp sp,x2,\[x3\]'
>  [^:]*:[0-9]+: Error: operand 2 must be an integer register -- `stgp x1,sp,\[x3\]'
>  [^:]*:[0-9]+: Error: 64-bit integer or SP register expected at operand 3 -- `stgp x0,x0,\[xzr\]'
> +[^:]*:[0-9]+: Error: operand 1 must be an integer register -- `ldg sp,\[x0,#16\]'
> +[^:]*:[0-9]+: Error: 64-bit integer or SP register expected at operand 2 -- `ldg x0,\[xzr,#16\]'
> diff --git a/gas/testsuite/gas/aarch64/illegal-memtag.s b/gas/testsuite/gas/aarch64/illegal-memtag.s
> index 2a66366ebdec78cba3b4d453eaa6681dd78fc1e0..35d1b12870bee105ede52d86b392afc68c8190d0 100644
> --- a/gas/testsuite/gas/aarch64/illegal-memtag.s
> +++ b/gas/testsuite/gas/aarch64/illegal-memtag.s
> @@ -8,10 +8,12 @@ func:
>  	addg x1, x2, #0x3f0, #0x10
>  	subg x1, x2, #0x3f0, -4
>  
> -	# STG/STZG/ST2G : Fail imm
> +	# STG/STZG/ST2G/LDG : Fail imm
>  	stg [x1, #15]
>  	stzg [x1, #-4097]!
>  	st2g [x1], #4096
> +	ldg x1, [x2, #33]
> +	ldg x1, [x2, #4112]
>  
>  	# STGP : Fail imm
>  	stgp x1, x2, [x3, #1009]
> @@ -42,3 +44,5 @@ func:
>  	stgp sp, x2, [x3]
>  	stgp x1, sp, [x3]
>  	stgp x0, x0, [xzr]
> +	ldg sp, [x0, #16]
> +	ldg x0, [xzr, #16]
> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
> index e70d318d934621b6ef0609033d2257ac439d0ca3..1118433a5a03bcaf143dd136582af6661ff1071b 100644
> --- a/opcodes/aarch64-tbl.h
> +++ b/opcodes/aarch64-tbl.h
> @@ -1218,6 +1218,12 @@
>    QLF2(NIL, S_D),		\
>  }
>  
> +/* e.g. LDG <Xt>, [<Xn|SP>{, #<simm>}].  */
> +#define QL_LDG			\
> +{				\
> +  QLF2(X, imm_tag),		\
> +}
> +
>  /* e.g. LDPSW <Xt1>, <Xt2>, [<Xn|SP>{, #<imm>}].  */
>  #define QL_LDST_PAIR_X32	\
>  {				\
> @@ -3288,6 +3294,7 @@ struct aarch64_opcode aarch64_opcode_table[] =
>    CORE_INSN ("ldur", 0xb8400000, 0xbfe00c00, ldst_unscaled, OP_LDUR, OP2 (Rt, ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>    CORE_INSN ("ldursw", 0xb8800000, 0xffe00c00, ldst_unscaled, OP_LDURSW, OP2 (Rt, ADDR_SIMM9), QL_LDST_X32, 0),
>    CORE_INSN ("prfum", 0xf8800000, 0xffe00c00, ldst_unscaled, OP_PRFUM, OP2 (PRFOP, ADDR_SIMM9), QL_LDST_PRFM, 0),
> +  MEMTAG_INSN ("ldg",  0xd9600000, 0xffe00c00, ldst_unscaled, OP2 (Rt, ADDR_SIMM13), QL_LDG, 0),
>    /* Load/store register (scaled signed immediate).  */
>    V8_3_INSN ("ldraa", 0xf8200400, 0xffa00400, ldst_imm10, OP2 (Rt, ADDR_SIMM10), QL_X1NIL, 0),
>    V8_3_INSN ("ldrab", 0xf8a00400, 0xffa00400, ldst_imm10, OP2 (Rt, ADDR_SIMM10), QL_X1NIL, 0),
>