| Message ID | 20200205110541.37811-2-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,RFC,v2,1/2] target/arm: Allow to inject SError interrupt | expand |
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); > } >
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
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
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~
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
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
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
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); }
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(-)