Re: [PATCH v6 15/24] KVM: arm64: vgic-its: Read config and pending bit in add_lpi()

[Christoffer Dall <christoffer.dall@linaro.org>]

On Thu, May 04, 2017 at 01:44:35PM +0200, Eric Auger wrote:
> When creating the lpi we now ask the redistributor what is the state
> of the LPI (priority, enabled, pending).
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v6: creation
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index f43ea30c..3ad615a 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -38,6 +38,8 @@
>  
>  static int vgic_its_set_abi(struct vgic_its *its, int rev);
>  static const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its);
> +static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> +			     struct kvm_vcpu *filter_vcpu);
>  
>  /*
>   * Creates a new (reference to a) struct vgic_irq for a given LPI.
> @@ -46,10 +48,12 @@ static const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its);
>   * If this is a "new" LPI, we allocate and initialize a new struct vgic_irq.
>   * This function returns a pointer to the _unlocked_ structure.
>   */
> -static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
> +static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
> +				     struct kvm_vcpu *vcpu)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct vgic_irq *irq = vgic_get_irq(kvm, NULL, intid), *oldirq;
> +	int ret;
>  
>  	/* In this case there is no put, since we keep the reference. */
>  	if (irq)
> @@ -66,6 +70,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
>  	irq->config = VGIC_CONFIG_EDGE;
>  	kref_init(&irq->refcount);
>  	irq->intid = intid;
> +	irq->target_vcpu = vcpu;
>  
>  	spin_lock(&dist->lpi_list_lock);
>  
> @@ -97,6 +102,19 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
>  out_unlock:
>  	spin_unlock(&dist->lpi_list_lock);
>  
> +	/*
> +	 * We "cache" the configuration table entries in out struct vgic_irq's.

s/out/our/ ?

> +	 * However we only have those structs for mapped IRQs, so we read in
> +	 * the respective config data from memory here upon mapping the LPI.
> +	 */
> +	ret = update_lpi_config(kvm, irq, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = vgic_v3_lpi_sync_pending_status(kvm, irq);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	return irq;
>  }
>  
> @@ -769,6 +787,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	u32 event_id = its_cmd_get_id(its_cmd);
>  	u32 coll_id = its_cmd_get_collection(its_cmd);
>  	struct its_ite *ite;
> +	struct kvm_vcpu *vcpu = NULL;
>  	struct its_device *device;
>  	struct its_collection *collection, *new_coll = NULL;
>  	int lpi_nr;
> @@ -814,7 +833,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	ite->collection = collection;
>  	ite->lpi = lpi_nr;
>  
> -	irq = vgic_add_lpi(kvm, lpi_nr);
> +	if (its_is_collection_mapped(collection))
> +		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
> +
> +	irq = vgic_add_lpi(kvm, lpi_nr, vcpu);

So, if we don't have the collection and therefore end up with irq->vcpu
== NULL, how do we ever read the pending bit from memory as the affinity
may later change?

Is this a problem with our idea of only looking at the pending bit on
vgic_add_lpi, or does it just mean that we should sample the pending bit
whenever we move LPIs around to VCPUs and if the bit is set, then also
set the pennding_latch (if not already set), or what should happen here?

I think in any case, we should have a comment here explaining that vcpu
configuration data gets picked up later?

Thanks,
-Christoffer

>  	if (IS_ERR(irq)) {
>  		if (new_coll)
>  			vgic_its_free_collection(its, coll_id);
> @@ -823,15 +845,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	}
>  	ite->irq = irq;
>  
> -	update_affinity_ite(kvm, ite);
> -
> -	/*
> -	 * We "cache" the configuration table entries in out struct vgic_irq's.
> -	 * However we only have those structs for mapped IRQs, so we read in
> -	 * the respective config data from memory here upon mapping the LPI.
> -	 */
> -	update_lpi_config(kvm, ite->irq, NULL);
> -
>  	return 0;
>  }
>  
> -- 
> 2.5.5
>