[PATCH v2 3/3] KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

[Andre Przywara]

Hi,

On 09/08/16 12:16, Christoffer Dall wrote:
> There are two problems with the current implementation of the MMIO
> handlers for the propbaser and pendbaser:
> 
> First, the write to the value itself is not guaranteed to be an atomic
> 64-bit write so two concurrent writes to the structure field could be
> intermixed.
> 
> Second, because we do a read-modify-update operation without any
> synchronization, if we have two 32-bit accesses to separate parts of the
> register, we can loose one of them.
> 
> By using the atomic cmpxchg64 we should cover both issues above.

This is really a neat solution. The actual implementation of cmpxchg64
is really hard to chase in the code, but given that the prototype is
(ptr, old, new) and it complies with the usual compare-exchange
semantics this looks good to me.

> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>

Reviewed-by: Andre Przywara <andre.przywara at arm.com>

Cheers,
Andre.

> ---
> 
> Notes:
>     Changes since v1:
>      - Use atomic cmpxchg64 instead of taking a lock
> 
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ff668e0..a50d5ba 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -306,16 +306,18 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	u64 propbaser = dist->propbaser;
> +	u64 old_propbaser, propbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
>  	if (vgic_cpu->lpis_enabled)
>  		return;
>  
> -	propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
> -	propbaser = vgic_sanitise_propbaser(propbaser);
> -
> -	dist->propbaser = propbaser;
> +	do {
> +		old_propbaser = dist->propbaser;
> +		propbaser = old_propbaser;
> +		propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
> +		propbaser = vgic_sanitise_propbaser(propbaser);
> +	} while (cmpxchg64(&dist->propbaser, old_propbaser, propbaser));
>  }
>  
>  static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu,
> @@ -331,16 +333,18 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>  				     unsigned long val)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	u64 pendbaser = vgic_cpu->pendbaser;
> +	u64 old_pendbaser, pendbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
>  	if (vgic_cpu->lpis_enabled)
>  		return;
>  
> -	pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
> -	pendbaser = vgic_sanitise_pendbaser(pendbaser);
> -
> -	vgic_cpu->pendbaser = pendbaser;
> +	do {
> +		old_pendbaser = vgic_cpu->pendbaser;
> +		pendbaser = old_pendbaser;
> +		pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
> +		pendbaser = vgic_sanitise_pendbaser(pendbaser);
> +	} while (cmpxchg64(&vgic_cpu->pendbaser, old_pendbaser, pendbaser));
>  }
>  
>  /*
>