(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

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.