[PATCH v3 19/19] KVM: arm64: ITS: Pending table save/restore

[Auger Eric]

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 at redhat.com>
>>
>> ---
>>
>> 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>