[PATCH, AARCH64] support s2_<op1>_<Cn>_<Cm>_<op2> sys reg

[PATCH, AARCH64] support s2_<op1>_<Cn>_<Cm>_<op2> sys reg

[Zhenqiang Chen]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

Hi,

Current parse_sys_reg only supports s3_<op1>_<Cn>_<Cm>_<op2>. But
according to the document,

  integer sys_op0 = 2 + UInt(o0)

o0 is bit-19.

So sys_op0 can be 2 or 3.

This patch changes GAS to to add s2_<op1>_<Cn>_<Cm>_<op2> support.
OK for trunk?

Thanks!
-Zhenqiang

ChangeLog
2013-11-12  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * config/tc-aarch64.c (parse_sys_reg): Support S2_<op1>_<Cn>_<Cm>_<op2>.

[-- Attachment #2: op0.patch --]
[-- Type: text/x-patch, Size: 1717 bytes --]

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 14ffdad..e992e7c 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -3270,7 +3270,7 @@ parse_barrier (char **str)
    Returns the encoding for the option, or PARSE_FAIL.
 
    If IMPLE_DEFINED_P is non-zero, the function will also try to parse the
-   implementation defined system register name S3_<op1>_<Cn>_<Cm>_<op2>.  */
+   implementation defined system register name S<op0>_<op1>_<Cn>_<Cm>_<op2>.  */
 
 static int
 parse_sys_reg (char **str, struct hash_control *sys_regs, int imple_defined_p)
@@ -3295,7 +3295,7 @@ parse_sys_reg (char **str, struct hash_control *sys_regs, int imple_defined_p)
 	return PARSE_FAIL;
       else
 	{
-	  /* Parse S3_<op1>_<Cn>_<Cm>_<op2>, the implementation defined
+	  /* Parse S<op0>_<op1>_<Cn>_<Cm>_<op2>, the implementation defined
 	     registers.  */
 	  unsigned int op0, op1, cn, cm, op2;
 	  if (sscanf (buf, "s%u_%u_c%u_c%u_%u", &op0, &op1, &cn, &cm, &op2) != 5)
@@ -3303,11 +3303,11 @@ parse_sys_reg (char **str, struct hash_control *sys_regs, int imple_defined_p)
 	  /* The architecture specifies the encoding space for implementation
 	     defined registers as:
 	     op0  op1  CRn   CRm   op2
-	     11   xxx  1x11  xxxx  xxx
+	     1x   xxx  1x11  xxxx  xxx
 	     For convenience GAS accepts a wider encoding space, as follows:
 	     op0  op1  CRn   CRm   op2
-	     11   xxx  xxxx  xxxx  xxx  */
-	  if (op0 != 3 || op1 > 7 || cn > 15 || cm > 15 || op2 > 7)
+	     1x   xxx  xxxx  xxxx  xxx  */
+	  if (op0 != 2 && op0 != 3 || op1 > 7 || cn > 15 || cm > 15 || op2 > 7)
 	    return PARSE_FAIL;
 	  value = (op0 << 14) | (op1 << 11) | (cn << 7) | (cm << 3) | op2;
 	}

Re: [PATCH, AARCH64] support s2_<op1>_<Cn>_<Cm>_<op2> sys reg

[Yufeng Zhang]

Hi Zhenqiang,

On 11/12/13 06:12, Zhenqiang Chen wrote:
> Hi,
>
> Current parse_sys_reg only supports s3_<op1>_<Cn>_<Cm>_<op2>. But
> according to the document,
>
>    integer sys_op0 = 2 + UInt(o0)
>
> o0 is bit-19.
>
> So sys_op0 can be 2 or 3.
>
> This patch changes GAS to to add s2_<op1>_<Cn>_<Cm>_<op2>  support.
> OK for trunk?

For MSR/MRS instruction, it is correct that sys_op0 can be 2 or 3. 
However, it doesn't necessarily mean that the encoding space of 
sys_op0==2 contains reserved area for implementation defined system 
registers.

The ARMARMv8 mentions in section C4.2.6 "Op0==0b11, Moves to and from 
non-debug System registers and special-purpose registers" that:

"The instructions that move data to and from non-debug system registers 
are encoded with Op0==0b11, except that some of this encoding space is 
reserved for IMPLEMENTATION DEFINED functionality."

The document however doesn't mention about the implementation defined 
functionality in section C4.2.5 "Op0==0b10, Moves to and from debug, 
qtrace, and Execution environment System registers".

Is there any named system register in the op0==0b10 encoding space that 
you would like to add support for, by extending the syntax?

Also if we go head with the patch, please also add some tests to 
gas/testsuite/gas/aarch64/sysreg.[d|s].


Thanks,
Yufeng

Re: [PATCH, AARCH64] support s2_<op1>_<Cn>_<Cm>_<op2> sys reg

[Zhenqiang Chen]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

On 13 November 2013 07:09, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote:
> Hi Zhenqiang,
>
>
> On 11/12/13 06:12, Zhenqiang Chen wrote:
>>
>> Hi,
>>
>> Current parse_sys_reg only supports s3_<op1>_<Cn>_<Cm>_<op2>. But
>> according to the document,
>>
>>    integer sys_op0 = 2 + UInt(o0)
>>
>> o0 is bit-19.
>>
>> So sys_op0 can be 2 or 3.
>>
>> This patch changes GAS to to add s2_<op1>_<Cn>_<Cm>_<op2>  support.
>> OK for trunk?
>
>
> For MSR/MRS instruction, it is correct that sys_op0 can be 2 or 3. However,
> it doesn't necessarily mean that the encoding space of sys_op0==2 contains
> reserved area for implementation defined system registers.
>
> The ARMARMv8 mentions in section C4.2.6 "Op0==0b11, Moves to and from
> non-debug System registers and special-purpose registers" that:
>
> "The instructions that move data to and from non-debug system registers are
> encoded with Op0==0b11, except that some of this encoding space is reserved
> for IMPLEMENTATION DEFINED functionality."
>
> The document however doesn't mention about the implementation defined
> functionality in section C4.2.5 "Op0==0b10, Moves to and from debug, qtrace,
> and Execution environment System registers".

C4.2.5
...
This section includes **only** the System register access encodings
for which both:
* Op0 is 0b10.
* The value of Op1 is one of {0, 3, 4}.

So this section only covers "Debug" registers, not "Trace" and
"Execution environment" registers.

Section C5.6.129 defines the full encode.

> Is there any named system register in the op0==0b10 encoding space that you
> would like to add support for, by extending the syntax?

I do not know. Maybe customers can define some registers and map them
to s2_<op1>_<Cn>_<Cm>_<op2>.

> Also if we go head with the patch, please also add some tests to
> gas/testsuite/gas/aarch64/sysreg.[d|s].

Thanks. Patch is updated to include tests.

-Zhenqiang

[-- Attachment #2: op0.patch --]
[-- Type: text/x-patch, Size: 2489 bytes --]

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 14ffdad..e992e7c 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -3270,7 +3270,7 @@ parse_barrier (char **str)
    Returns the encoding for the option, or PARSE_FAIL.
 
    If IMPLE_DEFINED_P is non-zero, the function will also try to parse the
-   implementation defined system register name S3_<op1>_<Cn>_<Cm>_<op2>.  */
+   implementation defined system register name S<op0>_<op1>_<Cn>_<Cm>_<op2>.  */
 
 static int
 parse_sys_reg (char **str, struct hash_control *sys_regs, int imple_defined_p)
@@ -3295,7 +3295,7 @@ parse_sys_reg (char **str, struct hash_control *sys_regs, int imple_defined_p)
 	return PARSE_FAIL;
       else
 	{
-	  /* Parse S3_<op1>_<Cn>_<Cm>_<op2>, the implementation defined
+	  /* Parse S<op0>_<op1>_<Cn>_<Cm>_<op2>, the implementation defined
 	     registers.  */
 	  unsigned int op0, op1, cn, cm, op2;
 	  if (sscanf (buf, "s%u_%u_c%u_c%u_%u", &op0, &op1, &cn, &cm, &op2) != 5)
@@ -3303,11 +3303,11 @@ parse_sys_reg (char **str, struct hash_control *sys_regs, int imple_defined_p)
 	  /* The architecture specifies the encoding space for implementation
 	     defined registers as:
 	     op0  op1  CRn   CRm   op2
-	     11   xxx  1x11  xxxx  xxx
+	     1x   xxx  1x11  xxxx  xxx
 	     For convenience GAS accepts a wider encoding space, as follows:
 	     op0  op1  CRn   CRm   op2
-	     11   xxx  xxxx  xxxx  xxx  */
-	  if (op0 != 3 || op1 > 7 || cn > 15 || cm > 15 || op2 > 7)
+	     1x   xxx  xxxx  xxxx  xxx  */
+	  if (op0 != 2 && op0 != 3 || op1 > 7 || cn > 15 || cm > 15 || op2 > 7)
 	    return PARSE_FAIL;
 	  value = (op0 << 14) | (op1 << 11) | (cn << 7) | (cm << 3) | op2;
 	}
diff --git a/gas/testsuite/gas/aarch64/sysreg.d b/gas/testsuite/gas/aarch64/sysreg.d
index c7cf00e..7795b4d 100644
--- a/gas/testsuite/gas/aarch64/sysreg.d
+++ b/gas/testsuite/gas/aarch64/sysreg.d
@@ -26,3 +26,5 @@ Disassembly of section \.text:
   48:	d538cc00 	mrs	x0, s3_0_c12_c12_0
   4c:	d5384600 	mrs	x0, s3_0_c4_c6_0
   50:	d5184600 	msr	s3_0_c4_c6_0, x0
+  54:	d5310300 	mrs	x0, s2_1_c0_c3_0
+  58:	d5110300 	msr	s2_1_c0_c3_0, x0
diff --git a/gas/testsuite/gas/aarch64/sysreg.s b/gas/testsuite/gas/aarch64/sysreg.s
index 3287594..b7e5ff6 100644
--- a/gas/testsuite/gas/aarch64/sysreg.s
+++ b/gas/testsuite/gas/aarch64/sysreg.s
@@ -26,3 +26,6 @@
 	mrs x0, s3_0_c12_c12_0
 	mrs x0, s3_0_c4_c6_0
 	msr s3_0_c4_c6_0, x0
+
+	mrs x0, s2_1_c0_c3_0
+	msr s2_1_c0_c3_0, x0

Re: [PATCH, AARCH64] support s2_<op1>_<Cn>_<Cm>_<op2> sys reg

[Yufeng Zhang]

I have committed the patch to the master and binutils-2_24-branch 
branches.  See http://sourceware.org/ml/binutils/2013-11/msg00164.html 
for more info.

Yufeng

On 11/13/13 05:23, Zhenqiang Chen wrote:
> On 13 November 2013 07:09, Yufeng Zhang<Yufeng.Zhang@arm.com>  wrote:
>> Hi Zhenqiang,
>>
>>
>> On 11/12/13 06:12, Zhenqiang Chen wrote:
>>>
>>> Hi,
>>>
>>> Current parse_sys_reg only supports s3_<op1>_<Cn>_<Cm>_<op2>. But
>>> according to the document,
>>>
>>>     integer sys_op0 = 2 + UInt(o0)
>>>
>>> o0 is bit-19.
>>>
>>> So sys_op0 can be 2 or 3.
>>>
>>> This patch changes GAS to to add s2_<op1>_<Cn>_<Cm>_<op2>   support.
>>> OK for trunk?
>>
>>
>> For MSR/MRS instruction, it is correct that sys_op0 can be 2 or 3. However,
>> it doesn't necessarily mean that the encoding space of sys_op0==2 contains
>> reserved area for implementation defined system registers.
>>
>> The ARMARMv8 mentions in section C4.2.6 "Op0==0b11, Moves to and from
>> non-debug System registers and special-purpose registers" that:
>>
>> "The instructions that move data to and from non-debug system registers are
>> encoded with Op0==0b11, except that some of this encoding space is reserved
>> for IMPLEMENTATION DEFINED functionality."
>>
>> The document however doesn't mention about the implementation defined
>> functionality in section C4.2.5 "Op0==0b10, Moves to and from debug, qtrace,
>> and Execution environment System registers".
>
> C4.2.5
> ...
> This section includes **only** the System register access encodings
> for which both:
> * Op0 is 0b10.
> * The value of Op1 is one of {0, 3, 4}.
>
> So this section only covers "Debug" registers, not "Trace" and
> "Execution environment" registers.
>
> Section C5.6.129 defines the full encode.
>
>> Is there any named system register in the op0==0b10 encoding space that you
>> would like to add support for, by extending the syntax?
>
> I do not know. Maybe customers can define some registers and map them
> to s2_<op1>_<Cn>_<Cm>_<op2>.
>
>> Also if we go head with the patch, please also add some tests to
>> gas/testsuite/gas/aarch64/sysreg.[d|s].
>
> Thanks. Patch is updated to include tests.
>
> -Zhenqiang