[v4,26/40] target/arm: Update define_one_arm_cp_reg_with_opaque for VHE

Message ID 20191203022937.1474-27-richard.henderson@linaro.org
State Superseded
Headers show
Series
  • target/arm: Implement ARMv8.1-VHE
Related show

Commit Message

Richard Henderson Dec. 3, 2019, 2:29 a.m.
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(-)

-- 
2.17.1

Comments

Alex Bennée Dec. 4, 2019, 6:58 p.m. | #1
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
Richard Henderson Dec. 4, 2019, 7:47 p.m. | #2
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~
Alex Bennée Dec. 4, 2019, 10:38 p.m. | #3
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
Richard Henderson Dec. 5, 2019, 3:09 p.m. | #4
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~
Peter Maydell Dec. 6, 2019, 3:53 p.m. | #5
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

Patch

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;