Re: [PATCH v4 09/11] KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is cleared

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger wrote:
> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> guest RAM are not valid anymore. The device, collection
> and LPI lists stored in the in-kernel ITS represent the same
> information in some form of cache. So let's void the cache.
> 
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> 
> ---
> 
> v2 -> v3:
> - add a comment and clear cache in if block
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index f3f0026f..084239c 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1471,8 +1471,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  				      unsigned long val)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> -	u64 entry_size, device_type;
> +	u64 entry_size;
>  	u64 reg, *regptr, clearbits = 0;
> +	int type;
>  
>  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
>  	if (its->enabled)
> @@ -1482,12 +1483,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	case 0:
>  		regptr = &its->baser_device_table;
>  		entry_size = abi->dte_esz;
> -		device_type = GITS_BASER_TYPE_DEVICE;
> +		type = GITS_BASER_TYPE_DEVICE;
>  		break;
>  	case 1:
>  		regptr = &its->baser_coll_table;
>  		entry_size = abi->cte_esz;
> -		device_type = GITS_BASER_TYPE_COLLECTION;
> +		type = GITS_BASER_TYPE_COLLECTION;
>  		clearbits = GITS_BASER_INDIRECT;
>  		break;
>  	default:
> @@ -1499,10 +1500,24 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	reg &= ~clearbits;
>  
>  	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> -	reg |= device_type << GITS_BASER_TYPE_SHIFT;
> +	reg |= (u64)type << GITS_BASER_TYPE_SHIFT;
>  	reg = vgic_sanitise_its_baser(reg);
>  
>  	*regptr = reg;
> +
> +	/* Table no longer valid: clear cached data */
> +	if (!(reg & GITS_BASER_VALID)) {
> +		switch (type) {
> +		case GITS_BASER_TYPE_DEVICE:
> +			vgic_its_free_device_list(kvm, its);
> +			break;
> +		case GITS_BASER_TYPE_COLLECTION:
> +			vgic_its_free_collection_list(kvm, its);
> +			break;
> +		default:
> +			break;
> +		}
> +	}

So we do this after setting the *regptr, which makes we worried about
races.

How are guest writes to these registers synchronized with, for example
trying to save the tables.  Perhaps we don't care because userspace
should have stopped the VM before trying to save the ITS state?



>  }
>  
>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
> -- 
> 2.5.5
> 

Otherwise looks good to me.
-Christoffer