Hi Andre,
On 22/03/2017 15:39, Andre Przywara wrote:
> Hi Eric,
>
> On 06/03/17 11:34, Eric Auger wrote:
>> Save and restore the pending tables.
>>
>> Pending table restore obviously requires the pendbaser to be
>> already set.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>>
>> v1 -> v2:
>> - do not care about the 1st KB which should be zeroed according to
>> the spec.
>> ---
>> virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 69 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 27ebabd..24824be 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>> */
>> static int vgic_its_flush_pending_tables(struct vgic_its *its)
>
> So as suspected before, I think passing the "its" pointer here is wrong.
> In fact you don't use that ITS except for getting the kvm pointer.
> Architecturally the pending tables are per redistributor, so you should
> pass a struct vcpu pointer.
> So the cleanest way would be to have a FLUSH/RESTORE_PENDING_TABLE ioctl
> on the *redistributor* kvm_device, which iterates through the list here
> and just dumps the pending bits for LPIs targeting that VCPU (skipping
> over others).
> Alternatively it would be enough to pass just a struct kvm pointer here
> and keep dumping all LPIs, but call this function only once per VM (not
> for each ITS). That sounds a bit dodgy from the architectural point of
> view, though.
>
>> {
>> - return -ENXIO;
>> + struct kvm *kvm = its->dev->kvm;
>> + struct vgic_dist *dist = &kvm->arch.vgic;
>> + struct vgic_irq *irq;
>> + int ret;
>> +
>> + /**
>> + * we do not take the dist->lpi_list_lock since we have a garantee
>> + * the LPI list is not touched while the its lock is held
>> + */
>
> As mentioned before I think this has to be reworked to take the lock,
> copy the table, drop the lock again and then iterate over the (private)
> copy to handle each LPI.
> Or something completely different.
> But IIRC the lpi_list_lock is taken completely independent of any ITS
> emulation code in vgic.c.
I aligned the code with other user access:
take the kvm lock and take all vcpu locks to make sure the vcpu are not
running
>
>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>> + struct kvm_vcpu *vcpu;
>> + gpa_t pendbase, ptr;
>> + bool stored;
>> + u8 val;
>> +
>> + vcpu = irq->target_vcpu;
>> + if (!vcpu)
>> + return -EINVAL;
>
> Isn't target_vcpu == NULL a valid use case? So continue; instead of return?
yes correct.
>
>> +
>> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
>> +
>> + ptr = pendbase + (irq->intid / BITS_PER_BYTE);
>> +
>> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1);
>> + if (ret)
>> + return ret;
>> +
>> + stored = val & (irq->intid % BITS_PER_BYTE);
>> + if (stored == irq->pending_latch)
>> + continue;
>> +
>> + if (irq->pending_latch)
>> + val |= 1 << (irq->intid % BITS_PER_BYTE);
>> + else
>> + val &= ~(1 << (irq->intid % BITS_PER_BYTE));
>> +
>> + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> }
>>
>> /**
>> @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its)
>> */
>> static int vgic_its_restore_pending_tables(struct vgic_its *its)
>
> Could deserve the comment that it doesn't actually scan the table for
> set bits, but only checks the mapped LPIs (and thus should come last in
> the restore process).
> Also the same comment as above about using the "its" pointer applies here.
done
>
>> {
>> - return -ENXIO;
>> + struct vgic_irq *irq;
>> + struct kvm *kvm = its->dev->kvm;
>> + struct vgic_dist *dist = &kvm->arch.vgic;
>> + int ret;
>> +
>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>> + struct kvm_vcpu *vcpu;
>> + gpa_t pendbase, ptr;
>> + u8 val;
>> +
>> + vcpu = irq->target_vcpu;
>> + if (!vcpu)
>> + return -EINVAL;
>> +
>> + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ))
>> + return 0;
>
> I believe this bit is only set once by software to communicate that
> *initially* (upon enabling LPIs in the redistributor) the pending table
> is clear. It is never cleared by the redistributor, so you can't rely on
> it here.
you're right
thanks
Eric
>
> Cheers,
> Andre.
>
>> +
>> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
>> +
>> + ptr = pendbase + (irq->intid / BITS_PER_BYTE);
>> +
>> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1);
>> + if (ret)
>> + return ret;
>> + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE));
>> + }
>> + return 0;
>> }
>>
>> static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev,
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
