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

[Andre Przywara]

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 at 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 at arm.com>

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.

Cheers,
Andre.