[RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Gavin Shan]

This allows to inject SError interrupt, which will be used on receiving
QMP/HMP "nmi" command in next patch.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/cpu.c    | 11 +++++++++++
 target/arm/cpu.h    | 12 +++++++++---
 target/arm/helper.c |  4 ++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index f86e71a260..24fd77437b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -461,6 +461,17 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         }
     }
 
+    if (interrupt_request & CPU_INTERRUPT_SERROR) {
+        excp_idx = EXCP_SERROR;
+        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
+        if (arm_excp_unmasked(cs, excp_idx, target_el)) {
+            cs->exception_index = excp_idx;
+            env->exception.target_el = target_el;
+            cc->do_interrupt(cs);
+            ret = true;
+        }
+    }
+
     return ret;
 }
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 608fcbd0b7..0bbc46ff6d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -49,6 +49,7 @@
 #define EXCP_LAZYFP         20   /* v7M fault during lazy FP stacking */
 #define EXCP_LSERR          21   /* v8M LSERR SecureFault */
 #define EXCP_UNALIGNED      22   /* v7M UNALIGNED UsageFault */
+#define EXCP_SERROR         23   /* SError Interrupt */
 /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
 
 #define ARMV7M_EXCP_RESET   1
@@ -79,9 +80,10 @@ enum {
 };
 
 /* ARM-specific interrupt pending bits.  */
-#define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
-#define CPU_INTERRUPT_VIRQ  CPU_INTERRUPT_TGT_EXT_2
-#define CPU_INTERRUPT_VFIQ  CPU_INTERRUPT_TGT_EXT_3
+#define CPU_INTERRUPT_FIQ    CPU_INTERRUPT_TGT_EXT_1
+#define CPU_INTERRUPT_VIRQ   CPU_INTERRUPT_TGT_EXT_2
+#define CPU_INTERRUPT_VFIQ   CPU_INTERRUPT_TGT_EXT_3
+#define CPU_INTERRUPT_SERROR CPU_INTERRUPT_TGT_EXT_4
 
 /* The usual mapping for an AArch64 system register to its AArch32
  * counterpart is for the 32 bit world to have access to the lower
@@ -2731,6 +2733,10 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
         pstate_unmasked = !(env->daif & PSTATE_I);
         break;
 
+    case EXCP_SERROR:
+       pstate_unmasked = !(env->daif & PSTATE_A);
+       break;
+
     case EXCP_VFIQ:
         if (secure || !(hcr_el2 & HCR_FMO) || (hcr_el2 & HCR_TGE)) {
             /* VFIQs are only taken when hypervized and non-secure.  */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 19a57a17da..622a1d8796 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7980,6 +7980,7 @@ void arm_log_exception(int idx)
             [EXCP_LAZYFP] = "v7M exception during lazy FP stacking",
             [EXCP_LSERR] = "v8M LSERR UsageFault",
             [EXCP_UNALIGNED] = "v7M UNALIGNED UsageFault",
+            [EXCP_SERROR] = "SError Interrupt",
         };
 
         if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
@@ -8566,6 +8567,9 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     case EXCP_VFIQ:
         addr += 0x100;
         break;
+    case EXCP_SERROR:
+        addr += 0x180;
+        break;
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
-- 
2.23.0

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Gavin Shan]

On 2/5/20 10:05 PM, Gavin Shan wrote:
> This allows to inject SError interrupt, which will be used on receiving
> QMP/HMP "nmi" command in next patch.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   target/arm/cpu.c    | 11 +++++++++++
>   target/arm/cpu.h    | 12 +++++++++---
>   target/arm/helper.c |  4 ++++
>   3 files changed, 24 insertions(+), 3 deletions(-)
> 

Hi Peter, could you please take a look when you get a chance? I'm not sure
the implementation is good enough to inject SError. If there are somebody
else who can help to review, please let me know so that I can copy her/him
either.

Thanks,
Gavin

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index f86e71a260..24fd77437b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -461,6 +461,17 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>           }
>       }
>   
> +    if (interrupt_request & CPU_INTERRUPT_SERROR) {
> +        excp_idx = EXCP_SERROR;
> +        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
> +        if (arm_excp_unmasked(cs, excp_idx, target_el)) {
> +            cs->exception_index = excp_idx;
> +            env->exception.target_el = target_el;
> +            cc->do_interrupt(cs);
> +            ret = true;
> +        }
> +    }
> +
>       return ret;
>   }
>   
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 608fcbd0b7..0bbc46ff6d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -49,6 +49,7 @@
>   #define EXCP_LAZYFP         20   /* v7M fault during lazy FP stacking */
>   #define EXCP_LSERR          21   /* v8M LSERR SecureFault */
>   #define EXCP_UNALIGNED      22   /* v7M UNALIGNED UsageFault */
> +#define EXCP_SERROR         23   /* SError Interrupt */
>   /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
>   
>   #define ARMV7M_EXCP_RESET   1
> @@ -79,9 +80,10 @@ enum {
>   };
>   
>   /* ARM-specific interrupt pending bits.  */
> -#define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
> -#define CPU_INTERRUPT_VIRQ  CPU_INTERRUPT_TGT_EXT_2
> -#define CPU_INTERRUPT_VFIQ  CPU_INTERRUPT_TGT_EXT_3
> +#define CPU_INTERRUPT_FIQ    CPU_INTERRUPT_TGT_EXT_1
> +#define CPU_INTERRUPT_VIRQ   CPU_INTERRUPT_TGT_EXT_2
> +#define CPU_INTERRUPT_VFIQ   CPU_INTERRUPT_TGT_EXT_3
> +#define CPU_INTERRUPT_SERROR CPU_INTERRUPT_TGT_EXT_4
>   
>   /* The usual mapping for an AArch64 system register to its AArch32
>    * counterpart is for the 32 bit world to have access to the lower
> @@ -2731,6 +2733,10 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>           pstate_unmasked = !(env->daif & PSTATE_I);
>           break;
>   
> +    case EXCP_SERROR:
> +       pstate_unmasked = !(env->daif & PSTATE_A);
> +       break;
> +
>       case EXCP_VFIQ:
>           if (secure || !(hcr_el2 & HCR_FMO) || (hcr_el2 & HCR_TGE)) {
>               /* VFIQs are only taken when hypervized and non-secure.  */
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 19a57a17da..622a1d8796 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7980,6 +7980,7 @@ void arm_log_exception(int idx)
>               [EXCP_LAZYFP] = "v7M exception during lazy FP stacking",
>               [EXCP_LSERR] = "v8M LSERR UsageFault",
>               [EXCP_UNALIGNED] = "v7M UNALIGNED UsageFault",
> +            [EXCP_SERROR] = "SError Interrupt",
>           };
>   
>           if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
> @@ -8566,6 +8567,9 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>       case EXCP_VFIQ:
>           addr += 0x100;
>           break;
> +    case EXCP_SERROR:
> +        addr += 0x180;
> +        break;
>       default:
>           cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>       }
> 

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Peter Maydell]

On Wed, 12 Feb 2020 at 06:39, Gavin Shan <gshan@redhat.com> wrote:
>
> On 2/5/20 10:05 PM, Gavin Shan wrote:
> > This allows to inject SError interrupt, which will be used on receiving
> > QMP/HMP "nmi" command in next patch.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   target/arm/cpu.c    | 11 +++++++++++
> >   target/arm/cpu.h    | 12 +++++++++---
> >   target/arm/helper.c |  4 ++++
> >   3 files changed, 24 insertions(+), 3 deletions(-)
> >
>
> Hi Peter, could you please take a look when you get a chance? I'm not sure
> the implementation is good enough to inject SError. If there are somebody
> else who can help to review, please let me know so that I can copy her/him
> either.

Yeah, this is on my list to look at; Richard Henderson also could
have a look at it. From a quick scan I suspect you may be missing
handling for AArch32.

thanks
-- PMM

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Gavin Shan]

On 2/12/20 10:34 PM, Peter Maydell wrote:
> On Wed, 12 Feb 2020 at 06:39, Gavin Shan <gshan@redhat.com> wrote:
>> On 2/5/20 10:05 PM, Gavin Shan wrote:
>>> This allows to inject SError interrupt, which will be used on receiving
>>> QMP/HMP "nmi" command in next patch.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>    target/arm/cpu.c    | 11 +++++++++++
>>>    target/arm/cpu.h    | 12 +++++++++---
>>>    target/arm/helper.c |  4 ++++
>>>    3 files changed, 24 insertions(+), 3 deletions(-)
>>>
>>
>> Hi Peter, could you please take a look when you get a chance? I'm not sure
>> the implementation is good enough to inject SError. If there are somebody
>> else who can help to review, please let me know so that I can copy her/him
>> either.
> 
> Yeah, this is on my list to look at; Richard Henderson also could
> have a look at it. From a quick scan I suspect you may be missing
> handling for AArch32.
> 

[Thanks for copying Richard Henderson]

Yes, the functionality is only supported on aarch64 currently by intention
because the next patch enables it on "max" and "host" CPU models and both
of them are running in aarch64 mode.

https://patchwork.kernel.org/patch/11366119/

If you really want to get this supported for aarch32 either, I can do
it. However, it seems there is a long list of aarch32 CPU models, defined
in target/arm/cpu.c::arm_cpus. so which CPU models you prefer to see with
this supported? I think we might choose one or two popular CPU models if
you agree.

Thanks,
Gavin

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Richard Henderson]

On 2/12/20 7:49 PM, Gavin Shan wrote:
> On 2/12/20 10:34 PM, Peter Maydell wrote:
>> Yeah, this is on my list to look at; Richard Henderson also could
>> have a look at it. From a quick scan I suspect you may be missing
>> handling for AArch32.
>>
> 
> [Thanks for copying Richard Henderson]
> 
> Yes, the functionality is only supported on aarch64 currently by intention
> because the next patch enables it on "max" and "host" CPU models and both
> of them are running in aarch64 mode.

We shouldn't leave the aarch32 exception entry paths unimplemented though.  C.f.

AArch32.TakePhysicalSErrorException()
AArch32.TakeVirtualSErrorException()

It really shouldn't be more than a couple of lines, just like
arm_cpu_do_interrupt_aarch64.  Remember both arm_cpu_do_interrupt_aarch32 and
arm_cpu_do_interrupt_aarch32_hyp.

> However, it seems there is a long list of aarch32 CPU models, defined
> in target/arm/cpu.c::arm_cpus. so which CPU models you prefer to see with
> this supported? I think we might choose one or two popular CPU models if
> you agree.

Even qemu-system-aarch64 -cpu max can exercise this path when EL1 is running in
aarch32 mode.  Admittedly it would be easier if we had the rest of the plumbing
so that -cpu max,aarch64=off worked.

FWIW, the rest of the patch looks good.


r~

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Peter Maydell]

On Thu, 13 Feb 2020 at 03:49, Gavin Shan <gshan@redhat.com> wrote:
> On 2/12/20 10:34 PM, Peter Maydell wrote:
> > Yeah, this is on my list to look at; Richard Henderson also could
> > have a look at it. From a quick scan I suspect you may be missing
> > handling for AArch32.

> Yes, the functionality is only supported on aarch64 currently by intention
> because the next patch enables it on "max" and "host" CPU models and both
> of them are running in aarch64 mode.
>
> https://patchwork.kernel.org/patch/11366119/
>
> If you really want to get this supported for aarch32 either, I can do
> it. However, it seems there is a long list of aarch32 CPU models, defined
> in target/arm/cpu.c::arm_cpus. so which CPU models you prefer to see with
> this supported? I think we might choose one or two popular CPU models if
> you agree.

I don't think you should need to care about the CPU models.
We should implement SError (aka "asynchronous external
abort" in ARMv7 and earlier) generically for all CPUs. The
SError/async abort should be triggered by a qemu_irq line
inbound to the CPU (similar to FIQ and IRQ); the board can
choose to wire that up to something, or not, as it likes.

thanks
-- PMM

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Gavin Shan]

On 2/13/20 4:39 PM, Richard Henderson wrote:
> On 2/12/20 7:49 PM, Gavin Shan wrote:
>> On 2/12/20 10:34 PM, Peter Maydell wrote:
>>> Yeah, this is on my list to look at; Richard Henderson also could
>>> have a look at it. From a quick scan I suspect you may be missing
>>> handling for AArch32.
>>>
>>
>> [Thanks for copying Richard Henderson]
>>
>> Yes, the functionality is only supported on aarch64 currently by intention
>> because the next patch enables it on "max" and "host" CPU models and both
>> of them are running in aarch64 mode.
> 
> We shouldn't leave the aarch32 exception entry paths unimplemented though.  C.f.
> 
> AArch32.TakePhysicalSErrorException()
> AArch32.TakeVirtualSErrorException()
> 
> It really shouldn't be more than a couple of lines, just like
> arm_cpu_do_interrupt_aarch64.  Remember both arm_cpu_do_interrupt_aarch32 and
> arm_cpu_do_interrupt_aarch32_hyp.
> 

Thanks for the details. The SError injection for aarch32 will be included in v3.

>> However, it seems there is a long list of aarch32 CPU models, defined
>> in target/arm/cpu.c::arm_cpus. so which CPU models you prefer to see with
>> this supported? I think we might choose one or two popular CPU models if
>> you agree.
> 
> Even qemu-system-aarch64 -cpu max can exercise this path when EL1 is running in
> aarch32 mode.  Admittedly it would be easier if we had the rest of the plumbing
> so that -cpu max,aarch64=off worked.
> 
> FWIW, the rest of the patch looks good.
> 

I think "-cpu max,aarch64=off" is only valid when KVM is enabled? If that's true,
the ioctl(cpu, KVM_SET_VCPU_EVENTS, &events) already worked for aarch32 or aarch64
guest if I'm correct enough. But yes, I need to test it because I never tested this
series on aarch32 guest :)

Thanks,
Gavin

Re: [RESEND RFC PATCH v2 1/2] target/arm: Allow to inject SError interrupt

[Gavin Shan]

On 2/13/20 9:31 PM, Peter Maydell wrote:
> On Thu, 13 Feb 2020 at 03:49, Gavin Shan <gshan@redhat.com> wrote:
>> On 2/12/20 10:34 PM, Peter Maydell wrote:
>>> Yeah, this is on my list to look at; Richard Henderson also could
>>> have a look at it. From a quick scan I suspect you may be missing
>>> handling for AArch32.
> 
>> Yes, the functionality is only supported on aarch64 currently by intention
>> because the next patch enables it on "max" and "host" CPU models and both
>> of them are running in aarch64 mode.
>>
>> https://patchwork.kernel.org/patch/11366119/
>>
>> If you really want to get this supported for aarch32 either, I can do
>> it. However, it seems there is a long list of aarch32 CPU models, defined
>> in target/arm/cpu.c::arm_cpus. so which CPU models you prefer to see with
>> this supported? I think we might choose one or two popular CPU models if
>> you agree.
> 
> I don't think you should need to care about the CPU models.
> We should implement SError (aka "asynchronous external
> abort" in ARMv7 and earlier) generically for all CPUs. The
> SError/async abort should be triggered by a qemu_irq line
> inbound to the CPU (similar to FIQ and IRQ); the board can
> choose to wire that up to something, or not, as it likes.
> 

Yes, what I need to care about is board with this. It means
I will get this supported on "virt" only. Thanks for the comments
and the changes will be included in v3.

Thanks,
Gavin