Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Auger Eric]

Hi Zenghui,

On 10/29/19 2:31 PM, Zenghui Yu wrote:
> Hi Eric,
> 
> On 2019/10/29 20:49, Auger Eric wrote:
>> On 10/29/19 1:27 PM, Zenghui Yu wrote:
>>> okay, the remaining question is that in vgic_v3_save_pending_tables():
>>>
>>> ÂÂÂÂÂstored = val & (1U << bit_nr);
>>> ÂÂÂÂÂif (stored == irq->pending_latch)
>>> ÂÂÂÂÂÂÂÂ continue;
>>>
>>> ÂÂÂÂÂif (irq->pending_latch)
>>> ÂÂÂÂÂÂÂÂ val |= 1 << bit_nr;
>>> ÂÂÂÂÂelse
>>> ÂÂÂÂÂÂÂÂ val &= ~(1 << bit_nr);
>>>
>>> Do we really have a scenario where irq->pending_latch==false and
>>> stored==true (corresponds to the above "else") and then we clear
>>> pending status of this LPI in guest memory?
>>> I can not think out one now.
>>
>> if you save, restore and save again. On the 1st save the LPI may be
>> pending, it gets stored. On the second save the LPI may be not pending
>> anymore?
> 
> I assume you mean the "restore" by vgic_its_restore_ite().

yes that's what I meant

> 
> While restoring a LPI, we will sync the pending status from guest
> pending table (into the software pending_latch), and clear the
> corresponding bit in guest memory.
> See vgic_v3_lpi_sync_pending_status().
> 
> So on the second save, the LPI can be not pending, the guest pending
> table will also indicate not pending.

You're right; I did not remember vgic_v3_lpi_sync_pending_status (called
from vgic_its_restore_ite/vgic_add_lpi) "cleared the consumed data"
(44de9d683847  KVM: arm64: vgic-v3: vgic_v3_lpi_sync_pending_status).

So effectively after restore the pending table is zeroed and the above
code should be rewrittable in a more simple manner, ie. just update the
byte in case the pending_latch is set.

Nethertheless your patch indeed fixes an actual bug independently on
this cleanup, ie. the written byte may be incorrect if LPIs belonging to
this byte target different RDIST.

Thanks

Eric
> 
> 
> Thanks,
> Zenghui
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel