Ping for code review, please?
thanks
-- PMM
On Mon, 10 May 2021 at 16:00, Peter Maydell <peter.mayd...@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 <c...@etri.re.kr>
> Signed-off-by: Peter Maydell <peter.mayd...@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
>
