From 9ba0807ab47d7d86e28e8573e0741d6c7fd6d81b Mon Sep 17 00:00:00 2001 From: Giacomo Travaglini Date: Thu, 23 Jul 2020 13:19:49 +0100 Subject: [PATCH] dev-arm: Fix _CTL_EL.ISTATUS when masking the irq According to the ArmArm: "When the value of the ENABLE bit is 1, ISTATUS indicates whether the timer condition is met. ISTATUS takes no account of the value of the IMASK bit. If the value of ISTATUS is 1 and the value of IMASK is 0 then the timer interrupt is asserted." Since ISTATUS is simply flagging that timer conditions are met, an interrupt mask (via the _CTL_EL.IMASK) shouldn't reset the field to 0. Clearing the ISTATUS bit leads to the following problem as an example: 1) virtual timer (EL1) issuing a physical interrupt to the GIC 2) hypervisor handling the physical interrupt; setting the CNTV_CTL_EL0.IMASK to 1 before issuing the virtual interrupt to the VM 3) The VM receives the virtual interrupt but it gets confused since CNTV_CTL_EL0.ISTATUS is 0 (due to point 2) What happens when we disable the timer? "When the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN." So we are allowed to not clear the ISTATUS bit if the timer gets disabled Change-Id: I8eb32459a3ef6829c1910cf63815e102e2705566 Signed-off-by: Giacomo Travaglini Reviewed-by: Andreas Sandberg Reviewed-by: Adrian Herrera Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/31775 Reviewed-by: Hsuan Hsu Tested-by: kokoro --- src/dev/arm/generic_timer.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/dev/arm/generic_timer.cc b/src/dev/arm/generic_timer.cc index 7bb2defef..a620eec6c 100644 --- a/src/dev/arm/generic_timer.cc +++ b/src/dev/arm/generic_timer.cc @@ -281,11 +281,14 @@ ArchTimer::updateCounter() if (value() >= _counterLimit) { counterLimitReached(); } else { - if (_control.istatus) { + // Clear the interurpt when timers conditions are not met + if (_interrupt->active()) { DPRINTF(Timer, "Clearing interrupt\n"); _interrupt->clear(); - _control.istatus = 0; } + + _control.istatus = 0; + if (scheduleEvents()) { _parent.schedule(_counterLimitReachedEvent, whenValue(_counterLimit)); @@ -320,10 +323,16 @@ ArchTimer::setControl(uint32_t val) // Timer masked or disabled else if ((!old_ctl.imask && new_ctl.imask) || (old_ctl.enable && !new_ctl.enable)) { - if (_control.istatus) { + + if (_interrupt->active()) { DPRINTF(Timer, "Clearing interrupt\n"); + // We are clearing the interrupt but we are not + // setting istatus to 0 as we are doing + // in the updateCounter. + // istatus signals that Timer conditions are met. + // It shouldn't depend on masking. + // if enable is zero. istatus is unknown. _interrupt->clear(); - _control.istatus = 0; } } } -- 2.30.2