* [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic
@ 2021-05-10 15:00 Peter Maydell
2021-05-20 13:22 ` Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Peter Maydell @ 2021-05-10 15:00 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Chan Kim
In icc_eoir_write() we assume that we can identify the group of the
IRQ being completed based purely on which register is being written
to and the current CPU state, and that "CPU state matches group
indicated by register" is the only necessary access check.
This isn't correct: if the CPU is not in Secure state then EOIR1 will
only complete Group 1 NS IRQs, but if the CPU is in EL3 it can
complete both Group 1 S and Group 1 NS IRQs. (The pseudocode
ICC_EOIR1_EL1 makes this clear.) We were also missing the logic to
prevent EOIR0 writes completing G0 IRQs when they should not.
Rearrange the logic to first identify the group of the current
highest priority interrupt and then look at whether we should
complete it or ignore the access based on which register was accessed
and the state of the CPU. The resulting behavioural change is:
* EL3 can now complete G1NS interrupts
* G0 interrupt completion is now ignored if the GIC
and the CPU have the security extension enabled and
the CPU is not secure
Reported-by: Chan Kim <ckim@etri.re.kr>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/arm_gicv3_cpuif.c | 48 ++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 43ef1d7a840..81f94c7f4ad 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1307,27 +1307,16 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
GICv3CPUState *cs = icc_cs_from_env(env);
int irq = value & 0xffffff;
int grp;
+ bool is_eoir0 = ri->crm == 8;
- if (icv_access(env, ri->crm == 8 ? HCR_FMO : HCR_IMO)) {
+ if (icv_access(env, is_eoir0 ? HCR_FMO : HCR_IMO)) {
icv_eoir_write(env, ri, value);
return;
}
- trace_gicv3_icc_eoir_write(ri->crm == 8 ? 0 : 1,
+ trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1,
gicv3_redist_affid(cs), value);
- if (ri->crm == 8) {
- /* EOIR0 */
- grp = GICV3_G0;
- } else {
- /* EOIR1 */
- if (arm_is_secure(env)) {
- grp = GICV3_G1;
- } else {
- grp = GICV3_G1NS;
- }
- }
-
if (irq >= cs->gic->num_irq) {
/* This handles two cases:
* 1. If software writes the ID of a spurious interrupt [ie 1020-1023]
@@ -1340,8 +1329,35 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
return;
}
- if (icc_highest_active_group(cs) != grp) {
- return;
+ grp = icc_highest_active_group(cs);
+ switch (grp) {
+ case GICV3_G0:
+ if (!is_eoir0) {
+ return;
+ }
+ if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS)
+ && arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
+ return;
+ }
+ break;
+ case GICV3_G1:
+ if (is_eoir0) {
+ return;
+ }
+ if (!arm_is_secure(env)) {
+ return;
+ }
+ break;
+ case GICV3_G1NS:
+ if (is_eoir0) {
+ return;
+ }
+ if (!arm_is_el3_or_mon(env) && arm_is_secure(env)) {
+ return;
+ }
+ break;
+ default:
+ g_assert_not_reached();
}
icc_drop_prio(cs, grp);
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic
2021-05-10 15:00 [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic Peter Maydell
@ 2021-05-20 13:22 ` Peter Maydell
2021-05-24 15:48 ` Alex Bennée
2021-05-24 16:10 ` Richard Henderson
2 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2021-05-20 13:22 UTC (permalink / raw)
To: qemu-arm, QEMU Developers; +Cc: Chan Kim
Ping for code review, please?
thanks
-- PMM
On Mon, 10 May 2021 at 16:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In icc_eoir_write() we assume that we can identify the group of the
> IRQ being completed based purely on which register is being written
> to and the current CPU state, and that "CPU state matches group
> indicated by register" is the only necessary access check.
>
> This isn't correct: if the CPU is not in Secure state then EOIR1 will
> only complete Group 1 NS IRQs, but if the CPU is in EL3 it can
> complete both Group 1 S and Group 1 NS IRQs. (The pseudocode
> ICC_EOIR1_EL1 makes this clear.) We were also missing the logic to
> prevent EOIR0 writes completing G0 IRQs when they should not.
>
> Rearrange the logic to first identify the group of the current
> highest priority interrupt and then look at whether we should
> complete it or ignore the access based on which register was accessed
> and the state of the CPU. The resulting behavioural change is:
> * EL3 can now complete G1NS interrupts
> * G0 interrupt completion is now ignored if the GIC
> and the CPU have the security extension enabled and
> the CPU is not secure
>
> Reported-by: Chan Kim <ckim@etri.re.kr>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 48 ++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 43ef1d7a840..81f94c7f4ad 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1307,27 +1307,16 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
> GICv3CPUState *cs = icc_cs_from_env(env);
> int irq = value & 0xffffff;
> int grp;
> + bool is_eoir0 = ri->crm == 8;
>
> - if (icv_access(env, ri->crm == 8 ? HCR_FMO : HCR_IMO)) {
> + if (icv_access(env, is_eoir0 ? HCR_FMO : HCR_IMO)) {
> icv_eoir_write(env, ri, value);
> return;
> }
>
> - trace_gicv3_icc_eoir_write(ri->crm == 8 ? 0 : 1,
> + trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1,
> gicv3_redist_affid(cs), value);
>
> - if (ri->crm == 8) {
> - /* EOIR0 */
> - grp = GICV3_G0;
> - } else {
> - /* EOIR1 */
> - if (arm_is_secure(env)) {
> - grp = GICV3_G1;
> - } else {
> - grp = GICV3_G1NS;
> - }
> - }
> -
> if (irq >= cs->gic->num_irq) {
> /* This handles two cases:
> * 1. If software writes the ID of a spurious interrupt [ie 1020-1023]
> @@ -1340,8 +1329,35 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
> return;
> }
>
> - if (icc_highest_active_group(cs) != grp) {
> - return;
> + grp = icc_highest_active_group(cs);
> + switch (grp) {
> + case GICV3_G0:
> + if (!is_eoir0) {
> + return;
> + }
> + if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS)
> + && arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
> + return;
> + }
> + break;
> + case GICV3_G1:
> + if (is_eoir0) {
> + return;
> + }
> + if (!arm_is_secure(env)) {
> + return;
> + }
> + break;
> + case GICV3_G1NS:
> + if (is_eoir0) {
> + return;
> + }
> + if (!arm_is_el3_or_mon(env) && arm_is_secure(env)) {
> + return;
> + }
> + break;
> + default:
> + g_assert_not_reached();
> }
>
> icc_drop_prio(cs, grp);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic
2021-05-10 15:00 [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic Peter Maydell
2021-05-20 13:22 ` Peter Maydell
@ 2021-05-24 15:48 ` Alex Bennée
2021-05-24 16:19 ` Peter Maydell
2021-05-24 16:10 ` Richard Henderson
2 siblings, 1 reply; 5+ messages in thread
From: Alex Bennée @ 2021-05-24 15:48 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, qemu-devel, Chan Kim
Peter Maydell <peter.maydell@linaro.org> writes:
> In icc_eoir_write() we assume that we can identify the group of the
> IRQ being completed based purely on which register is being written
> to and the current CPU state, and that "CPU state matches group
> indicated by register" is the only necessary access check.
>
> This isn't correct: if the CPU is not in Secure state then EOIR1 will
> only complete Group 1 NS IRQs, but if the CPU is in EL3 it can
> complete both Group 1 S and Group 1 NS IRQs. (The pseudocode
> ICC_EOIR1_EL1 makes this clear.) We were also missing the logic to
> prevent EOIR0 writes completing G0 IRQs when they should not.
>
> Rearrange the logic to first identify the group of the current
> highest priority interrupt and then look at whether we should
> complete it or ignore the access based on which register was accessed
> and the state of the CPU. The resulting behavioural change is:
> * EL3 can now complete G1NS interrupts
> * G0 interrupt completion is now ignored if the GIC
> and the CPU have the security extension enabled and
> the CPU is not secure
>
> Reported-by: Chan Kim <ckim@etri.re.kr>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 48 ++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 43ef1d7a840..81f94c7f4ad 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1307,27 +1307,16 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
> GICv3CPUState *cs = icc_cs_from_env(env);
> int irq = value & 0xffffff;
Un-related but the docs do say that:
This field has either 16 or 24 bits implemented. The number of
implemented bits can be found in ICC_CTLR_EL1.IDbits and
ICC_CTLR_EL3.IDbits. If only 16 bits are implemented, bits [23:16] of
this register are RES0
> int grp;
> + bool is_eoir0 = ri->crm == 8;
>
> - if (icv_access(env, ri->crm == 8 ? HCR_FMO : HCR_IMO)) {
> + if (icv_access(env, is_eoir0 ? HCR_FMO : HCR_IMO)) {
> icv_eoir_write(env, ri, value);
> return;
> }
>
> - trace_gicv3_icc_eoir_write(ri->crm == 8 ? 0 : 1,
> + trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1,
> gicv3_redist_affid(cs), value);
>
> - if (ri->crm == 8) {
> - /* EOIR0 */
> - grp = GICV3_G0;
> - } else {
> - /* EOIR1 */
> - if (arm_is_secure(env)) {
> - grp = GICV3_G1;
> - } else {
> - grp = GICV3_G1NS;
> - }
> - }
> -
> if (irq >= cs->gic->num_irq) {
> /* This handles two cases:
> * 1. If software writes the ID of a spurious interrupt [ie 1020-1023]
> @@ -1340,8 +1329,35 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
> return;
> }
>
> - if (icc_highest_active_group(cs) != grp) {
> - return;
> + grp = icc_highest_active_group(cs);
> + switch (grp) {
> + case GICV3_G0:
> + if (!is_eoir0) {
> + return;
> + }
> + if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS)
> + && arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
> + return;
> + }
> + break;
> + case GICV3_G1:
> + if (is_eoir0) {
> + return;
> + }
> + if (!arm_is_secure(env)) {
> + return;
> + }
> + break;
> + case GICV3_G1NS:
> + if (is_eoir0) {
> + return;
> + }
> + if (!arm_is_el3_or_mon(env) && arm_is_secure(env)) {
> + return;
> + }
> + break;
> + default:
> + g_assert_not_reached();
> }
From my reading of the spec it looks OK to me:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic
2021-05-24 15:48 ` Alex Bennée
@ 2021-05-24 16:19 ` Peter Maydell
0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2021-05-24 16:19 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-arm, QEMU Developers, Chan Kim
On Mon, 24 May 2021 at 17:09, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > In icc_eoir_write() we assume that we can identify the group of the
> > IRQ being completed based purely on which register is being written
> > to and the current CPU state, and that "CPU state matches group
> > indicated by register" is the only necessary access check.
> >
> > This isn't correct: if the CPU is not in Secure state then EOIR1 will
> > only complete Group 1 NS IRQs, but if the CPU is in EL3 it can
> > complete both Group 1 S and Group 1 NS IRQs. (The pseudocode
> > ICC_EOIR1_EL1 makes this clear.) We were also missing the logic to
> > prevent EOIR0 writes completing G0 IRQs when they should not.
> >
> > Rearrange the logic to first identify the group of the current
> > highest priority interrupt and then look at whether we should
> > complete it or ignore the access based on which register was accessed
> > and the state of the CPU. The resulting behavioural change is:
> > * EL3 can now complete G1NS interrupts
> > * G0 interrupt completion is now ignored if the GIC
> > and the CPU have the security extension enabled and
> > the CPU is not secure
> >
> > Reported-by: Chan Kim <ckim@etri.re.kr>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > hw/intc/arm_gicv3_cpuif.c | 48 ++++++++++++++++++++++++++-------------
> > 1 file changed, 32 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> > index 43ef1d7a840..81f94c7f4ad 100644
> > --- a/hw/intc/arm_gicv3_cpuif.c
> > +++ b/hw/intc/arm_gicv3_cpuif.c
> > @@ -1307,27 +1307,16 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > GICv3CPUState *cs = icc_cs_from_env(env);
> > int irq = value & 0xffffff;
>
> Un-related but the docs do say that:
>
> This field has either 16 or 24 bits implemented. The number of
> implemented bits can be found in ICC_CTLR_EL1.IDbits and
> ICC_CTLR_EL3.IDbits. If only 16 bits are implemented, bits [23:16] of
> this register are RES0
Yeah, this is one of those implementation-choice areas: you can
implement either 16 or 24 bits. QEMU chooses 24 bits and
correspondingly we set the ICC_CTLR_ELx.IDbits field to 1
in icc_reset().
> From my reading of the spec it looks OK to me:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic
2021-05-10 15:00 [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic Peter Maydell
2021-05-20 13:22 ` Peter Maydell
2021-05-24 15:48 ` Alex Bennée
@ 2021-05-24 16:10 ` Richard Henderson
2 siblings, 0 replies; 5+ messages in thread
From: Richard Henderson @ 2021-05-24 16:10 UTC (permalink / raw)
To: peter.maydell; +Cc: qemu-arm, ckim, qemu-devel
> In icc_eoir_write() we assume that we can identify the group of the
> IRQ being completed based purely on which register is being written
> to and the current CPU state, and that "CPU state matches group
> indicated by register" is the only necessary access check.
>
> This isn't correct: if the CPU is not in Secure state then EOIR1 will
> only complete Group 1 NS IRQs, but if the CPU is in EL3 it can
> complete both Group 1 S and Group 1 NS IRQs. (The pseudocode
> ICC_EOIR1_EL1 makes this clear.) We were also missing the logic to
> prevent EOIR0 writes completing G0 IRQs when they should not.
>
> Rearrange the logic to first identify the group of the current
> highest priority interrupt and then look at whether we should
> complete it or ignore the access based on which register was accessed
> and the state of the CPU. The resulting behavioural change is:
> * EL3 can now complete G1NS interrupts
> * G0 interrupt completion is now ignored if the GIC
> and the CPU have the security extension enabled and
> the CPU is not secure
>
> Reported-by: Chan Kim <ckim@etri.re.kr>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 48 ++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 16 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-24 16:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-10 15:00 [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic Peter Maydell
2021-05-20 13:22 ` Peter Maydell
2021-05-24 15:48 ` Alex Bennée
2021-05-24 16:19 ` Peter Maydell
2021-05-24 16:10 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).