Re: [RFC PATCH v3 04/21] target/arm: Implement ALLINT MSR (immediate)

2024-02-26 Thread Richard Henderson 

On 2/25/24 16:22, Jinjie Ruan wrote:



On 2024/2/24 3:03, Richard Henderson wrote:

On 2/23/24 00:32, Jinjie Ruan via wrote:

Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
to unwind.

Signed-off-by: Jinjie Ruan 
---
v3:
- Remove EL0 check in allint_check().
- Add TALLINT check for EL1 in allint_check().
- Remove unnecessarily arm_rebuild_hflags() in msr_i_allint helper.
---
   target/arm/tcg/a64.decode  |  1 +
   target/arm/tcg/helper-a64.c    | 24 
   target/arm/tcg/helper-a64.h    |  1 +
   target/arm/tcg/translate-a64.c | 10 ++
   4 files changed, 36 insertions(+)

diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
index 8a20dce3c8..3588080024 100644
--- a/target/arm/tcg/a64.decode
+++ b/target/arm/tcg/a64.decode
@@ -207,6 +207,7 @@ MSR_i_DIT   1101 0101  0 011 0100  010
1 @msr_i
   MSR_i_TCO   1101 0101  0 011 0100  100 1 @msr_i
   MSR_i_DAIFSET   1101 0101  0 011 0100  110 1 @msr_i
   MSR_i_DAIFCLEAR 1101 0101  0 011 0100  111 1 @msr_i
+MSR_i_ALLINT    1101 0101  0 001 0100  000 1 @msr_i


Decode is incorrect either here, or in trans_MSR_i_ALLINT, because CRm
!= '000x' is UNDEFINED.

MSR_i_ALLINT    1101 0101  0 001 0100 000 imm:1 000 1

is perhaps the clearest implementation.


+static void allint_check(CPUARMState *env, uint32_t op,
+   uint32_t imm, uintptr_t ra)
+{
+    /* ALLINT update to PSTATE. */
+    if (arm_current_el(env) == 1 && arm_is_el2_enabled(env) &&
+    (arm_hcrx_el2_eff(env) & HCRX_TALLINT)) {
+    raise_exception_ra(env, EXCP_UDEF,
+   syn_aa64_sysregtrap(0, extract32(op, 0, 3),
+   extract32(op, 3, 3), 4,
+   imm, 0x1f, 0),
+   exception_target_el(env), ra);
+    }
+}
+
+void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
+{
+    allint_check(env, 0x8, imm, GETPC());


As previously noted, the check for MSR_i only applies to imm==1, not 0.


Sorry! The hardware manual I looked at didn't say this.


In DDI0487J.a, C6.2.229 MSR (immediate), it is present in the pseudocode

  when PSTATEField_ALLINT
if (PSTATE.EL == EL1 && IsHCRXEL2Enabled()
&& HCRX_EL2.TALLINT == '1' && CRm<0> == '1') then
  AArch64.SystemAccessTrap(EL2, 0x18);
PSTATE.ALLINT = CRm<0>;

In D19.2.49 HCRX_EL2, it is present as text for the description of TALLINT.


r~

Re: [RFC PATCH v3 04/21] target/arm: Implement ALLINT MSR (immediate)

2024-02-25 Thread Jinjie Ruan via 



On 2024/2/24 3:03, Richard Henderson wrote:
> On 2/23/24 00:32, Jinjie Ruan via wrote:
>> Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
>> to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
>> to unwind.
>>
>> Signed-off-by: Jinjie Ruan 
>> ---
>> v3:
>> - Remove EL0 check in allint_check().
>> - Add TALLINT check for EL1 in allint_check().
>> - Remove unnecessarily arm_rebuild_hflags() in msr_i_allint helper.
>> ---
>>   target/arm/tcg/a64.decode  |  1 +
>>   target/arm/tcg/helper-a64.c    | 24 
>>   target/arm/tcg/helper-a64.h    |  1 +
>>   target/arm/tcg/translate-a64.c | 10 ++
>>   4 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
>> index 8a20dce3c8..3588080024 100644
>> --- a/target/arm/tcg/a64.decode
>> +++ b/target/arm/tcg/a64.decode
>> @@ -207,6 +207,7 @@ MSR_i_DIT   1101 0101  0 011 0100  010
>> 1 @msr_i
>>   MSR_i_TCO   1101 0101  0 011 0100  100 1 @msr_i
>>   MSR_i_DAIFSET   1101 0101  0 011 0100  110 1 @msr_i
>>   MSR_i_DAIFCLEAR 1101 0101  0 011 0100  111 1 @msr_i
>> +MSR_i_ALLINT    1101 0101  0 001 0100  000 1 @msr_i
> 
> Decode is incorrect either here, or in trans_MSR_i_ALLINT, because CRm
> != '000x' is UNDEFINED.
> 
> MSR_i_ALLINT    1101 0101  0 001 0100 000 imm:1 000 1
> 
> is perhaps the clearest implementation.
> 
>> +static void allint_check(CPUARMState *env, uint32_t op,
>> +   uint32_t imm, uintptr_t ra)
>> +{
>> +    /* ALLINT update to PSTATE. */
>> +    if (arm_current_el(env) == 1 && arm_is_el2_enabled(env) &&
>> +    (arm_hcrx_el2_eff(env) & HCRX_TALLINT)) {
>> +    raise_exception_ra(env, EXCP_UDEF,
>> +   syn_aa64_sysregtrap(0, extract32(op, 0, 3),
>> +   extract32(op, 3, 3), 4,
>> +   imm, 0x1f, 0),
>> +   exception_target_el(env), ra);
>> +    }
>> +}
>> +
>> +void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
>> +{
>> +    allint_check(env, 0x8, imm, GETPC());
> 
> As previously noted, the check for MSR_i only applies to imm==1, not 0.

Sorry! The hardware manual I looked at didn't say this.

> 
> As previously noted, with ALLINT in env->pstate, you can implement this
> completely inline for EL[23], or EL1 with imm==0.
> 
> No point in passing in "op" and extracting, because you know exactly
> what the value should be for all MSR ALLINT.
> 
> 
> r~

Re: [RFC PATCH v3 04/21] target/arm: Implement ALLINT MSR (immediate)

2024-02-23 Thread Richard Henderson 

On 2/23/24 00:32, Jinjie Ruan via wrote:

Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
to unwind.

Signed-off-by: Jinjie Ruan 
---
v3:
- Remove EL0 check in allint_check().
- Add TALLINT check for EL1 in allint_check().
- Remove unnecessarily arm_rebuild_hflags() in msr_i_allint helper.
---
  target/arm/tcg/a64.decode  |  1 +
  target/arm/tcg/helper-a64.c| 24 
  target/arm/tcg/helper-a64.h|  1 +
  target/arm/tcg/translate-a64.c | 10 ++
  4 files changed, 36 insertions(+)

diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
index 8a20dce3c8..3588080024 100644
--- a/target/arm/tcg/a64.decode
+++ b/target/arm/tcg/a64.decode
@@ -207,6 +207,7 @@ MSR_i_DIT   1101 0101  0 011 0100  010 1 
@msr_i
  MSR_i_TCO   1101 0101  0 011 0100  100 1 @msr_i
  MSR_i_DAIFSET   1101 0101  0 011 0100  110 1 @msr_i
  MSR_i_DAIFCLEAR 1101 0101  0 011 0100  111 1 @msr_i
+MSR_i_ALLINT1101 0101  0 001 0100  000 1 @msr_i


Decode is incorrect either here, or in trans_MSR_i_ALLINT, because CRm != 
'000x' is UNDEFINED.

MSR_i_ALLINT1101 0101  0 001 0100 000 imm:1 000 1

is perhaps the clearest implementation.


+static void allint_check(CPUARMState *env, uint32_t op,
+   uint32_t imm, uintptr_t ra)
+{
+/* ALLINT update to PSTATE. */
+if (arm_current_el(env) == 1 && arm_is_el2_enabled(env) &&
+(arm_hcrx_el2_eff(env) & HCRX_TALLINT)) {
+raise_exception_ra(env, EXCP_UDEF,
+   syn_aa64_sysregtrap(0, extract32(op, 0, 3),
+   extract32(op, 3, 3), 4,
+   imm, 0x1f, 0),
+   exception_target_el(env), ra);
+}
+}
+
+void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
+{
+allint_check(env, 0x8, imm, GETPC());


As previously noted, the check for MSR_i only applies to imm==1, not 0.

As previously noted, with ALLINT in env->pstate, you can implement this completely inline 
for EL[23], or EL1 with imm==0.


No point in passing in "op" and extracting, because you know exactly what the value should 
be for all MSR ALLINT.



r~