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

[Marc Zyngier <maz@kernel.org>]

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.