[PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Oliver Upton]

Reality check: KVM's GIC/ITS emulation does not handle pending LPI state
to the letter of the architecture!

Although the GIC spec defines LPI pending state on a per-redistributor
basis, KVM's view of the LPIs in a VM are global by design. This is
intentional, as it is a massive simplification to the way KVM organizes
interrupts and deals with the interactions between the vCPUs and ITSes.

So, with that in mind, the KVM emulation of PENDBASE is completely wrong,
as unmapped INTIDs (i.e. not mapped in any ITS) are silently ignored,
even though the expecatation is that IRQs would be generated in real
hardware. Even more hilarious things can happen for the LPIs that are
actually mapped, as an update to global LPI state can pend an interrupt
on a different vCPU.

vgic_add_lpi() fetches pending state when creating the global
representation of an LPI. Both VM save/restore and guest interactions
with the ITS find their way here, so the pending state will get loaded
into KVM one way or another. That flow is slightly more correct, as it
consults the pending table of the targeted redistributor.

Anyway, it is clear that this code can never be correct and doesn't
serve a purpose. If bisection led you here you've probably got a *very*
funky guest, and should bring it up on the list...

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c     | 60 ------------------------------
 arch/arm64/kvm/vgic/vgic-mmio-v3.c |  8 +---
 arch/arm64/kvm/vgic/vgic.h         |  1 -
 3 files changed, 2 insertions(+), 67 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index e2764d0ffa9f..c114dde3739b 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -426,59 +426,6 @@ static u32 max_lpis_propbaser(u64 propbaser)
 	return 1U << min(nr_idbits, INTERRUPT_ID_BITS_ITS);
 }
 
-/*
- * Sync the pending table pending bit of LPIs targeting @vcpu
- * with our own data structures. This relies on the LPI being
- * mapped before.
- */
-static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
-{
-	gpa_t pendbase = GICR_PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
-	struct vgic_irq *irq;
-	int last_byte_offset = -1;
-	int ret = 0;
-	u32 *intids;
-	int nr_irqs, i;
-	unsigned long flags;
-	u8 pendmask;
-
-	nr_irqs = vgic_copy_lpi_list(vcpu->kvm, vcpu, &intids);
-	if (nr_irqs < 0)
-		return nr_irqs;
-
-	for (i = 0; i < nr_irqs; i++) {
-		int byte_offset, bit_nr;
-
-		byte_offset = intids[i] / BITS_PER_BYTE;
-		bit_nr = intids[i] % BITS_PER_BYTE;
-
-		/*
-		 * For contiguously allocated LPIs chances are we just read
-		 * this very same byte in the last iteration. Reuse that.
-		 */
-		if (byte_offset != last_byte_offset) {
-			ret = kvm_read_guest_lock(vcpu->kvm,
-						  pendbase + byte_offset,
-						  &pendmask, 1);
-			if (ret) {
-				kfree(intids);
-				return ret;
-			}
-			last_byte_offset = byte_offset;
-		}
-
-		irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
-		raw_spin_lock_irqsave(&irq->irq_lock, flags);
-		irq->pending_latch = pendmask & (1U << bit_nr);
-		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
-		vgic_put_irq(vcpu->kvm, irq);
-	}
-
-	kfree(intids);
-
-	return ret;
-}
-
 static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
 					      struct vgic_its *its,
 					      gpa_t addr, unsigned int len)
@@ -1857,13 +1804,6 @@ static struct vgic_register_region its_registers[] = {
 		VGIC_ACCESS_32bit),
 };
 
-/* This is called on setting the LPI enable bit in the redistributor. */
-void vgic_enable_lpis(struct kvm_vcpu *vcpu)
-{
-	if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ))
-		its_sync_lpi_pending_table(vcpu);
-}
-
 static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its,
 				   u64 addr)
 {
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index c15ee1df036a..b555b1b424d3 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -280,12 +280,8 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		vgic_its_invalidate_cache(vcpu->kvm);
 		atomic_set_release(&vgic_cpu->ctlr, 0);
 	} else {
-		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
-					      GICR_CTLR_ENABLE_LPIS);
-		if (ctlr != 0)
-			return;
-
-		vgic_enable_lpis(vcpu);
+		atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
+				       GICR_CTLR_ENABLE_LPIS);
 	}
 }
 
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 8d134569d0a1..b79c586343c0 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -250,7 +250,6 @@ void vgic_v3_vmcr_sync(struct kvm_vcpu *vcpu);
 
 bool vgic_has_its(struct kvm *kvm);
 int kvm_vgic_register_its_device(void);
-void vgic_enable_lpis(struct kvm_vcpu *vcpu);
 void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu);
 int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
 int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);

base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.44.0.rc1.240.g4c46232300-goog

Re: [PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Zenghui Yu]

Hi Oliver,

On 2024/2/28 8:01, Oliver Upton wrote:
> Reality check: KVM's GIC/ITS emulation does not handle pending LPI state
> to the letter of the architecture!
> 
> Although the GIC spec defines LPI pending state on a per-redistributor
> basis, KVM's view of the LPIs in a VM are global by design. This is
> intentional, as it is a massive simplification to the way KVM organizes
> interrupts and deals with the interactions between the vCPUs and ITSes.
> 
> So, with that in mind, the KVM emulation of PENDBASE is completely wrong,
> as unmapped INTIDs (i.e. not mapped in any ITS) are silently ignored,
> even though the expecatation is that IRQs would be generated in real
> hardware. Even more hilarious things can happen for the LPIs that are
> actually mapped, as an update to global LPI state can pend an interrupt
> on a different vCPU.

I don't fully understand this "hilarious" part. Could you please
elaborate a bit more on it?

> vgic_add_lpi() fetches pending state when creating the global
> representation of an LPI. Both VM save/restore and guest interactions
> with the ITS find their way here, so the pending state will get loaded
> into KVM one way or another. That flow is slightly more correct, as it
> consults the pending table of the targeted redistributor.

This looks reasonable. I remember I had looked several times at this
code path (vgic_enable_lpis()/its_sync_lpi_pending_table()) but hadn't
made any improvements or progress.

E.g., it purely does *nothing* on the guest restore path, as
redistributors are restored before ITS, so there is no LPI mapping at
all when we're at its_sync_lpi_pending_table().

It's just been a long time. I'll take another look and think about it.

> Anyway, it is clear that this code can never be correct and doesn't
> serve a purpose. If bisection led you here you've probably got a *very*
> funky guest, and should bring it up on the list...

So I've tested it with kvm-unit-tests and got failure with the
its-pending-migration case:

INFO: gicv3: its-pending-migration: Migration complete
INFO: gicv3: its-pending-migration: expected 128 LPIs on PE #30, 0 observed
FAIL: gicv3: its-pending-migration: 128 LPIs on both PE0 and PE1 after 
migration
SUMMARY: 1 tests, 1 unexpected failures

where the guest SW directly writes to the pending table when
GICR_CTLR.EnableLPIs == 0. I seriously doubt there is any use case like
that in real world. But not sure whether it is a funky behaviour from
the architectural perspective.

Thanks,
Zenghui

Re: [PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Marc Zyngier]

Hi Zenghui,

On Thu, 07 Mar 2024 09:00:42 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> So I've tested it with kvm-unit-tests and got failure with the
> its-pending-migration case:
> 
> INFO: gicv3: its-pending-migration: Migration complete
> INFO: gicv3: its-pending-migration: expected 128 LPIs on PE #30, 0 observed
> FAIL: gicv3: its-pending-migration: 128 LPIs on both PE0 and PE1 after
> migration
> SUMMARY: 1 tests, 1 unexpected failures
> 
> where the guest SW directly writes to the pending table when
> GICR_CTLR.EnableLPIs == 0. I seriously doubt there is any use case like
> that in real world. But not sure whether it is a funky behaviour from
> the architectural perspective.

Right, so this is *exactly* the thing I was worried about. A mapping
has been established, the interrupt wasn't pending, all good. Now, an
interrupt lands while GICR_CTLR.EnableLPIs == 0.

The spec says (4.7.3 "Effect of disabling interrupts")

  "When GICR_CTLR.EnableLPIs == 0, LPIs are never set pending."

which to me is a pretty damning indication that we shouldn't take
these bits into account.

Of course, there is a grey area, in the sense that when we restore an
LPI via the restore interface, we are actually consuming pending bits
before EnableLPIs is set to 1.

But I would tend to draw the line between an interrupt that is
restored as part of rebuilding the KVM state, and an interrupt that is
forcefully injected behind our back.

Thanks,

	M.

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

Re: [PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Zenghui Yu]

Hi Marc,

On 2024/3/7 18:09, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On Thu, 07 Mar 2024 09:00:42 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>
>> So I've tested it with kvm-unit-tests and got failure with the
>> its-pending-migration case:
>>
>> INFO: gicv3: its-pending-migration: Migration complete
>> INFO: gicv3: its-pending-migration: expected 128 LPIs on PE #30, 0 observed
>> FAIL: gicv3: its-pending-migration: 128 LPIs on both PE0 and PE1 after
>> migration
>> SUMMARY: 1 tests, 1 unexpected failures
>>
>> where the guest SW directly writes to the pending table when
>> GICR_CTLR.EnableLPIs == 0. I seriously doubt there is any use case like
>> that in real world. But not sure whether it is a funky behaviour from
>> the architectural perspective.
> 
> Right, so this is *exactly* the thing I was worried about. A mapping
> has been established, the interrupt wasn't pending, all good. Now, an
> interrupt lands while GICR_CTLR.EnableLPIs == 0.
> 
> The spec says (4.7.3 "Effect of disabling interrupts")
> 
>   "When GICR_CTLR.EnableLPIs == 0, LPIs are never set pending."
> 
> which to me is a pretty damning indication that we shouldn't take
> these bits into account.

Ah, thanks for pointing it out! And I believe this is the rationale of
Oliver's patch.

What confused me is that the spec also says (5.1.2 "LPI Pending tables")

| "For physical LPIs, when GICR_CTLR.EnableLPIs is changed to 1, the
|  Redistributor must read the pending status of the physical LPIs from
|  the physical LPI Pending table."

which implicitly indicates that the pending table can contain some
pending bits (=1) when EnableLPIs == 0, which would be loaded by RD and
make the relevant LPIs pending when EnableLPIs is written from 0 to 1.

I have no idea how to interpret these rules ;-) .

> Of course, there is a grey area, in the sense that when we restore an
> LPI via the restore interface, we are actually consuming pending bits
> before EnableLPIs is set to 1.

The restore path is clear to me now. VMM follows the "ITS Restore
Sequence" of arm-vgic-its.rst, KVM then restores pending status for all
LPIs via vgic_its_restore_ite()/vgic_v3_lpi_sync_pending_status().

Removing vgic_enable_lpis()/its_sync_lpi_pending_table() (in this patch)
makes it much clearer.

> But I would tend to draw the line between an interrupt that is
> restored as part of rebuilding the KVM state, and an interrupt that is
> forcefully injected behind our back.

+1!

Thanks,
Zenghui

Re: [PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Zenghui Yu]

On 2024/3/7 20:13, Zenghui Yu wrote:
>> Of course, there is a grey area, in the sense that when we restore an
>> LPI via the restore interface, we are actually consuming pending bits
>> before EnableLPIs is set to 1.
> 
> The restore path is clear to me now. VMM follows the "ITS Restore
> Sequence" of arm-vgic-its.rst, KVM then restores pending status for all
> LPIs via vgic_its_restore_ite()/vgic_v3_lpi_sync_pending_status().

And as ITSes are restored after redistributors, we actually consume
pending bits _after_ EnableLPIs is set to 1, right?

Re: [PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Marc Zyngier]

On Thu, 07 Mar 2024 12:13:43 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Hi Marc,
> 
> On 2024/3/7 18:09, Marc Zyngier wrote:
> > Hi Zenghui,
> > 
> > On Thu, 07 Mar 2024 09:00:42 +0000,
> > Zenghui Yu <yuzenghui@huawei.com> wrote:
> >> 
> >> So I've tested it with kvm-unit-tests and got failure with the
> >> its-pending-migration case:
> >> 
> >> INFO: gicv3: its-pending-migration: Migration complete
> >> INFO: gicv3: its-pending-migration: expected 128 LPIs on PE #30, 0 observed
> >> FAIL: gicv3: its-pending-migration: 128 LPIs on both PE0 and PE1 after
> >> migration
> >> SUMMARY: 1 tests, 1 unexpected failures
> >> 
> >> where the guest SW directly writes to the pending table when
> >> GICR_CTLR.EnableLPIs == 0. I seriously doubt there is any use case like
> >> that in real world. But not sure whether it is a funky behaviour from
> >> the architectural perspective.
> > 
> > Right, so this is *exactly* the thing I was worried about. A mapping
> > has been established, the interrupt wasn't pending, all good. Now, an
> > interrupt lands while GICR_CTLR.EnableLPIs == 0.
> > 
> > The spec says (4.7.3 "Effect of disabling interrupts")
> > 
> >   "When GICR_CTLR.EnableLPIs == 0, LPIs are never set pending."
> > 
> > which to me is a pretty damning indication that we shouldn't take
> > these bits into account.
> 
> Ah, thanks for pointing it out! And I believe this is the rationale of
> Oliver's patch.
> 
> What confused me is that the spec also says (5.1.2 "LPI Pending tables")
> 
> | "For physical LPIs, when GICR_CTLR.EnableLPIs is changed to 1, the
> |  Redistributor must read the pending status of the physical LPIs from
> |  the physical LPI Pending table."
> 
> which implicitly indicates that the pending table can contain some
> pending bits (=1) when EnableLPIs == 0, which would be loaded by RD and
> make the relevant LPIs pending when EnableLPIs is written from 0 to 1.
> 
> I have no idea how to interpret these rules ;-) .

Yeah, even after all this time, the GICv3 spec never fails to
entertain.  My take on this is that it is there to support the
different ways to build a GICv3:

- either RDs are completely independent of the ITSs, and only deal
  with the forwarding of enabled interrupts. In this case, setting
  EnableLPIs to 1 would naturally lead to interrupt being forwarded to
  the CPU. New bits are not allowed to be made pending while it is set
  to 0 though, which matches 4.7.3.

- or RDs and ITSs are pretty monolithic, and the GIC infrastructure as
  a whole maintain the pending state as a result of a translation, not
  as a set of bits in the pending tables. In this case, EnableLPIs
  gates the translation process and there are no new pending bits
  being produced when it is set to 0. We can easily satisfy 5.1.2 by
  not having created new pending bits.

So the two rules are more or less correct. They just are saying that
it is IMPDEF what happens in this case, which is pretty damming for an
interrupt controller architecture. I feel dirty just having to think
of these things!

Thanks,

	M.

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

Re: [PATCH] KVM: arm64: vgic-v3: Don't load pending state when enabling LPIs on RD

[Zenghui Yu]

On 2024/3/7 21:50, Marc Zyngier wrote:
> On Thu, 07 Mar 2024 12:13:43 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>
>> Hi Marc,
>>
>> On 2024/3/7 18:09, Marc Zyngier wrote:
>>> Hi Zenghui,
>>>
>>> On Thu, 07 Mar 2024 09:00:42 +0000,
>>> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>>>
>>>> So I've tested it with kvm-unit-tests and got failure with the
>>>> its-pending-migration case:
>>>>
>>>> INFO: gicv3: its-pending-migration: Migration complete
>>>> INFO: gicv3: its-pending-migration: expected 128 LPIs on PE #30, 0 observed
>>>> FAIL: gicv3: its-pending-migration: 128 LPIs on both PE0 and PE1 after
>>>> migration
>>>> SUMMARY: 1 tests, 1 unexpected failures
>>>>
>>>> where the guest SW directly writes to the pending table when
>>>> GICR_CTLR.EnableLPIs == 0. I seriously doubt there is any use case like
>>>> that in real world. But not sure whether it is a funky behaviour from
>>>> the architectural perspective.
>>>
>>> Right, so this is *exactly* the thing I was worried about. A mapping
>>> has been established, the interrupt wasn't pending, all good. Now, an
>>> interrupt lands while GICR_CTLR.EnableLPIs == 0.
>>>
>>> The spec says (4.7.3 "Effect of disabling interrupts")
>>>
>>>   "When GICR_CTLR.EnableLPIs == 0, LPIs are never set pending."
>>>
>>> which to me is a pretty damning indication that we shouldn't take
>>> these bits into account.
>>
>> Ah, thanks for pointing it out! And I believe this is the rationale of
>> Oliver's patch.
>>
>> What confused me is that the spec also says (5.1.2 "LPI Pending tables")
>>
>> | "For physical LPIs, when GICR_CTLR.EnableLPIs is changed to 1, the
>> |  Redistributor must read the pending status of the physical LPIs from
>> |  the physical LPI Pending table."
>>
>> which implicitly indicates that the pending table can contain some
>> pending bits (=1) when EnableLPIs == 0, which would be loaded by RD and
>> make the relevant LPIs pending when EnableLPIs is written from 0 to 1.
>>
>> I have no idea how to interpret these rules ;-) .
> 
> Yeah, even after all this time, the GICv3 spec never fails to
> entertain.  My take on this is that it is there to support the
> different ways to build a GICv3:
> 
> - either RDs are completely independent of the ITSs, and only deal
>   with the forwarding of enabled interrupts. In this case, setting
>   EnableLPIs to 1 would naturally lead to interrupt being forwarded to
>   the CPU. New bits are not allowed to be made pending while it is set
>   to 0 though, which matches 4.7.3.
> 
> - or RDs and ITSs are pretty monolithic, and the GIC infrastructure as
>   a whole maintain the pending state as a result of a translation, not
>   as a set of bits in the pending tables. In this case, EnableLPIs
>   gates the translation process and there are no new pending bits
>   being produced when it is set to 0. We can easily satisfy 5.1.2 by
>   not having created new pending bits.
> 
> So the two rules are more or less correct. They just are saying that
> it is IMPDEF what happens in this case, which is pretty damming for an
> interrupt controller architecture. I feel dirty just having to think
> of these things!

Me too.. Hey, thanks for sharing your thoughts!

Zenghui