Re: [PATCH v4 6/9] KVM: arm64: vgic: Implement SW-driven deactivation

Hi Marc,

On 6/22/21 5:12 PM, Marc Zyngier wrote:

On Thu, 17 Jun 2021 15:58:31 +0100,
Alexandru Elisei [off-list ref] wrote:

quoted

Hi Marc,

On 6/1/21 11:40 AM, Marc Zyngier wrote:

quoted

In order to deal with these systems that do not offer HW-based
deactivation of interrupts, let implement a SW-based approach:

Nitpick, but shouldn't that be "let's"?

"Let it be...". ;-) Yup.

quoted

quoted

- When the irq is queued into a LR, treat it as a pure virtual
  interrupt and set the EOI flag in the LR.

- When the interrupt state is read back from the LR, force a
  deactivation when the state is invalid (neither active nor
  pending)

Interrupts requiring such treatment get the VGIC_SW_RESAMPLE flag.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-v2.c | 19 +++++++++++++++----
 arch/arm64/kvm/vgic/vgic-v3.c | 19 +++++++++++++++----
 include/kvm/arm_vgic.h        | 10 ++++++++++
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 11934c2af2f4..2c580204f1dc 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c

@@ -108,11 +108,22 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 		 * If this causes us to lower the level, we have to also clear
 		 * the physical active state, since we will otherwise never be
 		 * told when the interrupt becomes asserted again.
+		 *
+		 * Another case is when the interrupt requires a helping hand
+		 * on deactivation (no HW deactivation, for example).
 		 */
-		if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
-			irq->line_level = vgic_get_phys_line_level(irq);
+		if (vgic_irq_is_mapped_level(irq)) {
+			bool resample = false;
+
+			if (val & GICH_LR_PENDING_BIT) {
+				irq->line_level = vgic_get_phys_line_level(irq);
+				resample = !irq->line_level;
+			} else if (vgic_irq_needs_resampling(irq) &&
+				   !(irq->active || irq->pending_latch)) {

I'm having a hard time figuring out when and why a level sensitive
can have pending_latch = true.

I looked kvm_vgic_inject_irq(), and that function sets pending_latch
only for edge triggered interrupts (it sets line_level for level
sensitive ones). But irq_is_pending() looks at **both**
pending_latch and line_level for level sensitive interrupts.

Yes, and that's what an implementation requires.

quoted

The only place that I've found that sets pending_latch regardless of
the  interrupt type  is in  vgic_mmio_write_spending() (called  on a
trapped  write  to   GICD_ISENABLER).

Are you sure? It really should be GICD_ISPENDR. I'll assume that this
is what you mean below.

Yes, that's what I meant, sorry for the confusion.

quoted

vgic_v2_populate_lr()  clears
pending_latch  only for  edge triggered  interrupts, so  that leaves
vgic_v2_fold_lr_state()  as  the   only  function  pending_latch  is
cleared for level sensitive interrupts,  when the interrupt has been
handled by the guest.  Are we doing all of this  to emulate the fact
that level sensitive interrupts (either purely virtual or hw mapped)
made pending by a write  to GICD_ISENABLER remain pending until they
are handled by the guest?

Yes, or cleared by a write to GICD_ICPENDR. You really need to think
of the input into the GIC as some sort of OR gate combining both the
line level and the PEND register. With a latch for edge interrupts.

Have a look at Figure 4-10 ("Logic of the pending status of a
level-sensitive interrupt") in the GICv2 arch spec (ARM IHI 0048B.b)
to see what I actually mean.

quoted

If that is the case, then I think this is what the code is doing:

- There's no functional change when the irqchip has HW deactivation

- For level sensitive, hw mapped interrupts made pending by a write
to GICD_ISENABLER and not yet handled by the guest (pending_latch ==
true) we don't clear the pending state of the interrupt.

- For level sensitive, hw mapped interrupts we clear the pending
state in the GIC and the device will assert the interrupt again if
it's still pending at the device 1level. I have a question about
this. Why don't we sample the interrupt state by calling
vgic_get_phys_line_level()? Because that would be slower than the
alternative that you are proposing here?

Yes. It is *much* faster to read the timer status register (for
example) than going via an MMIO access to read the (re)distributor
that will return the same value.

Thank you for the explanation, much appreciated. The patch looks to me like it's
doing the right thing.

Thanks,

Alex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel