Re: [PATCH v5 09/13] KVM: arm64: connect LPIs to the VGIC emulation

[Marc Zyngier <marc.zyngier@arm.com>]

On 03/06/16 15:02, Andre Przywara wrote:
> LPIs are dynamically created (mapped) at guest runtime and their
> actual numbers can be quite high, but is mostly assigned using a very
> sparse allocation scheme. So arrays are not an ideal data structure
> to hold the information. We use our equivalent of the "Interrupt
> Translation Table Entry" (ITTE) to hold the vgic_irq struct for a
> virtual LPI embedded in in the ITTE.
> Connect the VGIC core code via an accessor function to help it get the
> struct vgic_irq for a certain LPI.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.c     |  2 +-
>  virt/kvm/arm/vgic/vgic.h     |  6 ++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 3ec12ef..4f248ef 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -68,11 +68,29 @@ struct its_collection {
>  struct its_itte {
>  	struct list_head itte_list;
>  
> +	struct vgic_irq irq;
>  	struct its_collection *collection;
>  	u32 lpi;
>  	u32 event_id;
>  };
>  
> +/* To be used as an iterator this macro misses the enclosing parentheses */
> +#define for_each_lpi(dev, itte, its) \
> +	list_for_each_entry(dev, &(its)->device_list, dev_list) \
> +		list_for_each_entry(itte, &(dev)->itt, itte_list)

Well, this is not really "for each LPI". This is "for each LPI that can
be generated by this ITS". Are you sure that you can always do this on a
per-ITS basis? In other words, while this work for a direct translation,
it doesn't work for a reverse one. Do we have any such case?

> +
> +static struct its_itte *find_itte_by_lpi(struct vgic_its *its, int lpi)
> +{
> +	struct its_device *device;
> +	struct its_itte *itte;
> +
> +	for_each_lpi(device, itte, its) {
> +		if (itte->lpi == lpi)
> +			return itte;
> +	}
> +	return NULL;
> +}
> +
>  #define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL)
>  
>  #define ITS_FRAME(addr) ((addr) & ~(SZ_64K - 1))
> @@ -158,6 +176,22 @@ static unsigned long vgic_mmio_read_its_idregs(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +struct vgic_irq *vgic_its_get_lpi(struct kvm *kvm, u32 intid)
> +{
> +	struct vgic_its *its;
> +	struct its_itte *itte;
> +
> +	list_for_each_entry(its, &kvm->arch.vits_list, its_list) {
> +		itte = find_itte_by_lpi(its, intid);
> +		if (!itte)
> +			continue;
> +
> +		return &itte->irq;

Or rather
		if (itte)
			return &itte->irq;

This function implements the case I was worried about above. It would be
worth mentioning that this *only* works because of 6.1.1 in the
architecture spec (an LPI can only be generated by a single EID/DID pair).

What doesn't really work here is that you are allowed to program this
EID/DID->LPI translation on several ITSs (think of a device moving its
doorbell from one ITS to another), which means that you cannot store the
vgic_irq in the ITE. Instead, this must be a pointer to IRQ, and the
interrupt as part of a separate list.

> +	}
> +
> +	return NULL;
> +}
> +
>  static void its_free_itte(struct its_itte *itte)
>  {
>  	list_del(&itte->itte_list);
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 69b61ab..6812ff1 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -58,7 +58,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  
>  	/* LPIs are not yet covered */
>  	if (intid >= VGIC_MIN_LPI)
> -		return NULL;
> +		return vgic_its_get_lpi(kvm, intid);
>  
>  	WARN(1, "Looking up struct vgic_irq for reserved INTID");
>  	return NULL;
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 66578d2..6fecd70 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -78,6 +78,7 @@ bool vgic_has_its(struct kvm *kvm);
>  int vits_init(struct kvm *kvm, struct vgic_its *its);
>  void vits_destroy(struct kvm *kvm, struct vgic_its *its);
>  int kvm_vgic_register_its_device(void);
> +struct vgic_irq *vgic_its_get_lpi(struct kvm *kvm, u32 intid);
>  #else
>  static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
>  {
> @@ -148,6 +149,11 @@ static int kvm_vgic_register_its_device(void)
>  {
>  	return -ENODEV;
>  }
> +
> +static inline struct vgic_irq *vgic_its_get_lpi(struct kvm *kvm, u32 intid)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  int kvm_register_vgic_device(unsigned long type);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...