[PATCH for-6.2? 0/2] arm_gicv3: Fix handling of LPIs in list registers

[PATCH for-6.2? 0/2] arm_gicv3: Fix handling of LPIs in list registers

[Peter Maydell]

(Marc: cc'd you on this one in case you're still using QEMU
to test KVM stuff with, in which case you might have run into
the bug this is fixing.)

It is valid for an OS to put virtual interrupt ID values into the
list registers ICH_LR<n> which are greater than 1023.  This
corresponds to (for example) KVM running as an L1 guest inside
emulated QEMU and using the in-kernel emulated ITS to give a (nested)
L2 guest an ITS.  LPIs are delivered by the L1 kernel to the L2 guest
via the list registers in the same way as non-LPI interrupts.
    
QEMU's code for handling writes to ICV_IARn (which happen when the L2
guest acknowledges an interrupt) and to ICV_EOIRn (which happen at
the end of the interrupt) did not consider LPIs, so it would
incorrectly treat interrupt IDs above 1023 as invalid, with the
effect that a read to ICV_IARn would return the correct interrupt ID
number but not actually mark the interrupt active or set the CPU
priority accordingly, and a write to ICV_EOIRn would do nothing.

This bug doesn't seem to have any visible effect on Linux L2 guests
most of the time, because the two bugs cancel each other out: we
neither mark the interrupt active nor deactivate it.  However it does
mean that the L2 vCPU priority while the LPI handler is running will
not be correct, so the interrupt handler could be unexpectedly
interrupted by a different interrupt.  (I haven't observed this; I
found the ICV_IARn bug by code inspection, and then the ICV_EOIRn bug
by figuring out why fixing ICV_IARn broke L2 guests :-))
    
This isn't a regression -- we've behaved like this since the GICv3
support for virtualization was first implemented. I'm tempted to
put it into 6.2 anyway, though.

Patch 1 abstracts out the test we were using already elsewhere
in the code into its own function, and patch 2 uses it to fix
the EOIR and IAR behaviour.

Based-on: 20211124202005.989935-1-peter.maydell@linaro.org
("[PATCH v2] hw/intc/arm_gicv3: Update cached state after LPI state changes")

Peter Maydell (2):
  hw/intc/arm_gicv3: Add new gicv3_intid_is_special() function
  hw/intc/arm_gicv3: fix handling of LPIs in list registers

 hw/intc/gicv3_internal.h  | 13 +++++++++++++
 hw/intc/arm_gicv3_cpuif.c |  9 ++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH for-6.2? 1/2] hw/intc/arm_gicv3: Add new gicv3_intid_is_special() function

[Peter Maydell]

The GICv3/v4 pseudocode has a function IsSpecial() which returns true
if passed a "special" interrupt ID number (anything between 1020 and
1023 inclusive).  We open-code this condition in a couple of places,
so abstract it out into a new function gicv3_intid_is_special().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/gicv3_internal.h  | 13 +++++++++++++
 hw/intc/arm_gicv3_cpuif.c |  4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 70f34ee4955..b9c37453b04 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -411,6 +411,19 @@ FIELD(MAPC, RDBASE, 16, 32)
 
 /* Functions internal to the emulated GICv3 */
 
+/**
+ * gicv3_intid_is_special:
+ * @intid: interrupt ID
+ *
+ * Return true if @intid is a special interrupt ID (1020 to
+ * 1023 inclusive). This corresponds to the GIC spec pseudocode
+ * IsSpecial() function.
+ */
+static inline bool gicv3_intid_is_special(int intid)
+{
+    return intid >= INTID_SECURE && intid <= INTID_SPURIOUS;
+}
+
 /**
  * gicv3_redist_update:
  * @cs: GICv3CPUState for this redistributor
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 3fe5de8ad7d..7fbc36ff41b 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -997,7 +997,7 @@ static uint64_t icc_iar0_read(CPUARMState *env, const ARMCPRegInfo *ri)
         intid = icc_hppir0_value(cs, env);
     }
 
-    if (!(intid >= INTID_SECURE && intid <= INTID_SPURIOUS)) {
+    if (!gicv3_intid_is_special(intid)) {
         icc_activate_irq(cs, intid);
     }
 
@@ -1020,7 +1020,7 @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
         intid = icc_hppir1_value(cs, env);
     }
 
-    if (!(intid >= INTID_SECURE && intid <= INTID_SPURIOUS)) {
+    if (!gicv3_intid_is_special(intid)) {
         icc_activate_irq(cs, intid);
     }
 
-- 
2.25.1

Re: [PATCH for-6.2? 1/2] hw/intc/arm_gicv3: Add new gicv3_intid_is_special() function

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The GICv3/v4 pseudocode has a function IsSpecial() which returns true
> if passed a "special" interrupt ID number (anything between 1020 and
> 1023 inclusive).  We open-code this condition in a couple of places,
> so abstract it out into a new function gicv3_intid_is_special().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH for-6.2? 2/2] hw/intc/arm_gicv3: fix handling of LPIs in list registers

[Peter Maydell]

It is valid for an OS to put virtual interrupt ID values into the
list registers ICH_LR<n> which are greater than 1023.  This
corresponds to (for example) KVM using the in-kernel emulated ITS to
give a (nested) guest an ITS.  LPIs are delivered by the L1 kernel to
the L2 guest via the list registers in the same way as non-LPI
interrupts.

QEMU's code for handling writes to ICV_IARn (which happen when the L2
guest acknowledges an interrupt) and to ICV_EOIRn (which happen at
the end of the interrupt) did not consider LPIs, so it would
incorrectly treat interrupt IDs above 1023 as invalid.  Fix this by
using the correct condition, which is gicv3_intid_is_special().

Note that the condition in icv_dir_write() is correct -- LPIs
are not valid there and so we want to ignore both "special" ID
values and LPIs.

(In the pseudocode this logic is in:
 - VirtualReadIAR0(), VirtualReadIAR1(), which call IsSpecial()
 - VirtualWriteEOIR0(), VirtualWriteEOIR1(), which call
     VirtualIdentifierValid(data, TRUE) meaning "LPIs OK"
 - VirtualWriteDIR(), which calls VirtualIdentifierValid(data, FALSE)
     meaning "LPIs not OK")

This bug doesn't seem to have any visible effect on Linux L2 guests
most of the time, because the two bugs cancel each other out: we
neither mark the interrupt active nor deactivate it.  However it does
mean that the L2 vCPU priority while the LPI handler is running will
not be correct, so the interrupt handler could be unexpectedly
interrupted by a different interrupt.

(NB: this has nothing to do with using QEMU's emulated ITS.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Not sure whether to put this into 6.2 -- I haven't ever seen
any actual misbehaviour, I found the bug by code inspection;
and we've behaved this way since the GICv3 support for
virtualization was first implemented, so it's not a regression.
---
 hw/intc/arm_gicv3_cpuif.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 7fbc36ff41b..7fba9314508 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -653,7 +653,7 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
 
         if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
             intid = ich_lr_vintid(lr);
-            if (intid < INTID_SECURE) {
+            if (!gicv3_intid_is_special(intid)) {
                 icv_activate_irq(cs, idx, grp);
             } else {
                 /* Interrupt goes from Pending to Invalid */
@@ -1265,8 +1265,7 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
     trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1,
                                gicv3_redist_affid(cs), value);
 
-    if (irq >= GICV3_MAXIRQ) {
-        /* Also catches special interrupt numbers and LPIs */
+    if (gicv3_intid_is_special(irq)) {
         return;
     }
 
-- 
2.25.1

Re: [PATCH for-6.2? 0/2] arm_gicv3: Fix handling of LPIs in list registers

[Marc Zyngier]

Hi Peter,

On Fri, 26 Nov 2021 16:39:13 +0000,
Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> (Marc: cc'd you on this one in case you're still using QEMU
> to test KVM stuff with, in which case you might have run into
> the bug this is fixing.)

Amusingly enough, I have recently fixed [1] a very similar issue with
the ICV_*_EL1 emulation that KVM uses when dealing with sub-par HW
(ThunderX and M1).

When writing this a very long while ago, I modelled it so that LPIs
wouldn't have an Active state, similar to bare metal. As it turns out,
the pseudocode actually treats LPIs almost like any other interrupt,
and is quite happy to carry an active bit that eventually gets exposed
to the hypervisor.

I don't think this ever caused any issue, but I'd be pretty happy to
see the QEMU implementation fixed.

For the whole series:

Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/kvm/hyp/vgic-v3-sr.c?id=9d449c71bd8f74282e84213c8f0b8328293ab0a7

-- 
Without deviation from the norm, progress is not possible.