| Message ID | 20240221130823.677762-8-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI | expand |
On 2/21/24 03:08, Jinjie Ruan via wrote: > The NMI exception state include whether the interrupt with super priority > is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish it. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > target/arm/cpu.h | 2 ++ > target/arm/helper.c | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 5257343bcb..051e589e19 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -603,6 +603,8 @@ typedef struct CPUArchState { > /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ > uint32_t irq_line_state; > > + bool nmi_is_irq; Why would you need to add this to CPUARMState? This has the appearance of requiring only a local variable. But it is hard to tell since you do not set it within this patch at all. r~
On 2/21/24 10:10, Richard Henderson wrote: > On 2/21/24 03:08, Jinjie Ruan via wrote: >> The NMI exception state include whether the interrupt with super priority >> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish it. >> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> target/arm/cpu.h | 2 ++ >> target/arm/helper.c | 9 +++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >> index 5257343bcb..051e589e19 100644 >> --- a/target/arm/cpu.h >> +++ b/target/arm/cpu.h >> @@ -603,6 +603,8 @@ typedef struct CPUArchState { >> /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ >> uint32_t irq_line_state; >> + bool nmi_is_irq; > > Why would you need to add this to CPUARMState? > This has the appearance of requiring only a local variable. > But it is hard to tell since you do not set it within this patch at all. According to Arm GIC section 4.6.3 Interrupt superpriority, NMI is always IRQ, never FIQ, so this is never required. r~
On 2024/2/22 5:25, Richard Henderson wrote: > On 2/21/24 10:10, Richard Henderson wrote: >> On 2/21/24 03:08, Jinjie Ruan via wrote: >>> The NMI exception state include whether the interrupt with super >>> priority >>> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish >>> it. >>> >>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>> --- >>> target/arm/cpu.h | 2 ++ >>> target/arm/helper.c | 9 +++++++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >>> index 5257343bcb..051e589e19 100644 >>> --- a/target/arm/cpu.h >>> +++ b/target/arm/cpu.h >>> @@ -603,6 +603,8 @@ typedef struct CPUArchState { >>> /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ >>> uint32_t irq_line_state; >>> + bool nmi_is_irq; >> >> Why would you need to add this to CPUARMState? >> This has the appearance of requiring only a local variable. >> But it is hard to tell since you do not set it within this patch at all. > > According to Arm GIC section 4.6.3 Interrupt superpriority, NMI is > always IRQ, never FIQ, so this is never required. There is a bit of ambiguity here. The processor manual says that both irq and fiq can have superpriority attributes, but the gic manual only says that the IRQ has superpriority attributes. > > > r~ >
On 2/22/24 01:52, Jinjie Ruan wrote: > > > On 2024/2/22 5:25, Richard Henderson wrote: >> On 2/21/24 10:10, Richard Henderson wrote: >>> On 2/21/24 03:08, Jinjie Ruan via wrote: >>>> The NMI exception state include whether the interrupt with super >>>> priority >>>> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish >>>> it. >>>> >>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>>> --- >>>> target/arm/cpu.h | 2 ++ >>>> target/arm/helper.c | 9 +++++++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >>>> index 5257343bcb..051e589e19 100644 >>>> --- a/target/arm/cpu.h >>>> +++ b/target/arm/cpu.h >>>> @@ -603,6 +603,8 @@ typedef struct CPUArchState { >>>> /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ >>>> uint32_t irq_line_state; >>>> + bool nmi_is_irq; >>> >>> Why would you need to add this to CPUARMState? >>> This has the appearance of requiring only a local variable. >>> But it is hard to tell since you do not set it within this patch at all. >> >> According to Arm GIC section 4.6.3 Interrupt superpriority, NMI is >> always IRQ, never FIQ, so this is never required. > > There is a bit of ambiguity here. The processor manual says that both > irq and fiq can have superpriority attributes, but the gic manual only > says that the IRQ has superpriority attributes. Yes, it is possible to inject a superpriority FIQ via HCRX_EL2. But you don't need an extra variable to handle this. r~
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 5257343bcb..051e589e19 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -603,6 +603,8 @@ typedef struct CPUArchState { /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ uint32_t irq_line_state; + bool nmi_is_irq; + /* Thumb-2 EE state. */ uint32_t teecr; uint32_t teehbr; diff --git a/target/arm/helper.c b/target/arm/helper.c index bd7858e02e..0bd7a87e51 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -10575,6 +10575,15 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ); hcr = hcr_el2 & HCR_FMO; break; + case EXCP_NMI: + if (env->nmi_is_irq) { + scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ); + hcr = hcr_el2 & HCR_IMO; + } else { + scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ); + hcr = hcr_el2 & HCR_FMO; + } + break; default: scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA); hcr = hcr_el2 & HCR_AMO;
The NMI exception state include whether the interrupt with super priority is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish it. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- target/arm/cpu.h | 2 ++ target/arm/helper.c | 9 +++++++++ 2 files changed, 11 insertions(+)