For ARMv8.1, op1 == 5 is reserved for EL2 aliases of
EL1 and EL0 registers.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 023b8963cf..1812588fa1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
             mask = PL0_RW;
             break;
         case 4:
+        case 5:
             /* min_EL EL2 */
             mask = PL2_RW;
             break;
-        case 5:
-            /* unallocated encoding, so not possible */
-            assert(false);
-            break;
         case 6:
             /* min_EL EL3 */
             mask = PL3_RW;
-- 
2.17.1

On Tue, 3 Dec 2019 at 02:30, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For ARMv8.1, op1 == 5 is reserved for EL2 aliases of
> EL1 and EL0 registers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 023b8963cf..1812588fa1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>              mask = PL0_RW;
>              break;
>          case 4:
> +        case 5:
>              /* min_EL EL2 */
>              mask = PL2_RW;
>              break;
> -        case 5:
> -            /* unallocated encoding, so not possible */
> -            assert(false);
> -            break;
>          case 6:
>              /* min_EL EL3 */
>              mask = PL3_RW;
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Richard Henderson <richard.henderson@linaro.org> writes:

> For ARMv8.1, op1 == 5 is reserved for EL2 aliases of
> EL1 and EL0 registers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 023b8963cf..1812588fa1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>              mask = PL0_RW;
>              break;
>          case 4:
> +        case 5:
>              /* min_EL EL2 */
>              mask = PL2_RW;
>              break;
> -        case 5:
> -            /* unallocated encoding, so not possible */
> -            assert(false);
> -            break;

This change is fine - I don't think we should have asserted here anyway.
But don't we generate an unallocated exception if the CPU is v8.0?


>          case 6:
>              /* min_EL EL3 */
>              mask = PL3_RW;


-- 
Alex Bennée

On 12/4/19 10:58 AM, Alex Bennée wrote:
>> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>>              mask = PL0_RW;
>>              break;
>>          case 4:
>> +        case 5:
>>              /* min_EL EL2 */
>>              mask = PL2_RW;
>>              break;
>> -        case 5:
>> -            /* unallocated encoding, so not possible */
>> -            assert(false);
>> -            break;
> 
> This change is fine - I don't think we should have asserted here anyway.
> But don't we generate an unallocated exception if the CPU is v8.0?

This change is only for validation of the system registers themselves.  It has
nothing to do with the usage of system registers from the actual guest.


r~

Richard Henderson <richard.henderson@linaro.org> writes:

> On 12/4/19 10:58 AM, Alex Bennée wrote:
>>> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>>>              mask = PL0_RW;
>>>              break;
>>>          case 4:
>>> +        case 5:
>>>              /* min_EL EL2 */
>>>              mask = PL2_RW;
>>>              break;
>>> -        case 5:
>>> -            /* unallocated encoding, so not possible */
>>> -            assert(false);
>>> -            break;
>> 
>> This change is fine - I don't think we should have asserted here anyway.
>> But don't we generate an unallocated exception if the CPU is v8.0?
>
> This change is only for validation of the system registers themselves.  It has
> nothing to do with the usage of system registers from the actual guest.

So what is the mechanism that feeds back to the translator?
access_check_cp_reg only seems to care about XSCALE. I guess
cp_access_ok would trip if you weren't at EL2 but what if you are a v8.0
at EL2?

-- 
Alex Bennée

On 12/4/19 2:38 PM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 12/4/19 10:58 AM, Alex Bennée wrote:
>>>> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>>>>              mask = PL0_RW;
>>>>              break;
>>>>          case 4:
>>>> +        case 5:
>>>>              /* min_EL EL2 */
>>>>              mask = PL2_RW;
>>>>              break;
>>>> -        case 5:
>>>> -            /* unallocated encoding, so not possible */
>>>> -            assert(false);
>>>> -            break;
>>>
>>> This change is fine - I don't think we should have asserted here anyway.
>>> But don't we generate an unallocated exception if the CPU is v8.0?
>>
>> This change is only for validation of the system registers themselves.  It has
>> nothing to do with the usage of system registers from the actual guest.
> 
> So what is the mechanism that feeds back to the translator?

The existence of a structure in the hash table.

> access_check_cp_reg only seems to care about XSCALE. I guess
> cp_access_ok would trip if you weren't at EL2 but what if you are a v8.0
> at EL2?

This is sanity-checking the structure as it goes into the hash table.  The
version check happens when creating the structure -- we don't create registers
that exist only for v8+ if the cpu is a v7.


r~