Re: [PATCH] KVM: arm/arm64: Remove struct vgic_irq pending field

[Christoffer Dall <christoffer.dall@linaro.org>]

On Tue, Jan 24, 2017 at 10:01:07AM +0000, Andre Przywara wrote:
> Hi Christoffer,
> 
> On 23/01/17 18:34, Christoffer Dall wrote:
> > On Mon, Jan 23, 2017 at 05:57:05PM +0000, Andre Przywara wrote:
> >> Hi Christoffer,
> >>
> >> On 23/01/17 13:39, Christoffer Dall wrote:
> >>> One of the goals behind the VGIC redesign was to get rid of cached or
> >>> intermediate state in the data structures, but we decided to allow
> >>> ourselves to precompute the pending value of an IRQ based on the line
> >>> level and pending latch state.  However, this has now become difficult
> >>> to base proper GICv3 save/restore on, because there is a potential to
> >>> modify the pending state without knowing if an interrupt is edge or
> >>> level configured.
> >>>
> >>> See the following post and related message for more background:
> >>> https://lists.cs.columbia.edu/pipermail/kvmarm/2017-January/023195.html
> >>>
> >>> This commit gets rid of the precomputed pending field in favor of a
> >>> function that calculates the value when needed, irq_is_pending().
> >>>
> >>> The soft_pending field is renamed to pending_latch to represent that
> >>> this latch is the equivalent hardware latch which gets manipulated by
> >>> the input signal for edge-triggered interrupts and when writing to the
> >>> SPENDR/CPENDR registers.
> >>>
> >>> After this commit save/restore code should be able to simply restore the
> >>> pending_latch state, line_level state, and config state in any order and
> >>> get the desired result.
> >>
> >> In general I like this very much and am wondering why we didn't come up
> >> with this earlier. But I guess we were so subscribed to our "cached
> >> pending" bit.
> >>
> >> So I checked several cases, tested some logic equations and drew the
> >> state diagrams for the "before" and "after" case, but I couldn't find a
> >> reason why this wouldn't work.
> >>
> >> I also think you can get rid of the irq_set_pending_latch() wrapper and
> >> assign the value directly, as I don't see any reason to hide the fact
> >> that it's indeed a simple assignment and nothing magic behind this
> >> function. Also it would make the diff a bit easier to read.
> > 
> > Agreed.  Once we have agreement about if we should cleanup clearing the
> > latch state, I'll respin with that and add you and Marc's RB tags with
> > the changed version.
> > 
> >>
> >> But apart from that:
> >>
> >> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> >>
> >>
> >> I also gave this a quick test on the Juno and the Midway with both
> >> kvmtool and QEMU and they survived some basic testing.
> >>
> >> I need to check a GICv3 machine tomorrow, but I don't expect any
> >> differences here.
> >>
> > 
> > Thanks.
> > 
> > I tested this, including GICv2 migration on Mustang, and was going to do
> > GICv3 testing and 32-bit testin tomorrow, but I'm quite happy to hear
> > you gave it a spin on a 32-bit platform already.
> 
> OK, so a GICv3 machine survived over night testing, resulting in some 10
> millions of interrupts of every kind (LPIs, edge SPIs, some 100,000
> level SPIs (UART only), SGIs, PPIs).
> Same on a Juno, no LPIs there of course, but also millions of interrupts
> working fine over night.
> 
> So:
> 
> Tested-by: Andre Przywara <andre.przywara@arm.com>

Thanks, I'll respin this with the trivial changes and add yours and
Marc's tested and reviewed by tags.

> 
> So this was just Linux as a guest, which - at least last time I checked
> - doesn't use GIC_DIST_SET_PENDING or GIC_DIST_CLEAR_PENDING registers
> too much (if at all under normal load).
> I think we should add some tests to kvm-unit-tests to actually trigger
> this code explicitly.
> 

Yeah, we probably should.  But I'm not going to for the purposes of
merging this patch.

Thanks,
-Christoffer