Re: [PATCH v3 41/59] KVM: arm/arm64: GICv4: Wire mapping/unmapping of VLPIs in VFIO irq bypass

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

On 26/08/17 20:48, Christoffer Dall wrote:
> On Mon, Jul 31, 2017 at 06:26:19PM +0100, Marc Zyngier wrote:
>> Let's use the irq bypass mechanism introduced for platform device
>> interrupts to intercept the virtual PCIe endpoint configuration
>> and establish our LPI->VLPI mapping.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  include/kvm/arm_vgic.h      |   8 ++++
>>  virt/kvm/arm/arm.c          |  27 ++++++++----
>>  virt/kvm/arm/vgic/vgic-v4.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 130 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 359eeffe9857..050f78d4fb42 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -367,4 +367,12 @@ int kvm_vgic_set_forwarding(struct kvm *kvm, unsigned int host_irq,
>>  void kvm_vgic_unset_forwarding(struct kvm *kvm, unsigned int host_irq,
>>  			       unsigned int vintid);
>>  
>> +struct kvm_kernel_irq_routing_entry;
>> +
>> +int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int irq,
>> +			       struct kvm_kernel_irq_routing_entry *irq_entry);
>> +
>> +int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int irq,
>> +				 struct kvm_kernel_irq_routing_entry *irq_entry);
>> +
>>  #endif /* __KVM_ARM_VGIC_H */
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index ebab6c29e3be..6803ea27c47d 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -1457,11 +1457,16 @@ int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
>>  	struct kvm_kernel_irqfd *irqfd =
>>  		container_of(cons, struct kvm_kernel_irqfd, consumer);
>>  
>> -	if (prod->type != IRQ_BYPASS_VFIO_PLATFORM)
>> +	switch (prod->type) {
>> +	case IRQ_BYPASS_VFIO_PLATFORM:
>> +		return kvm_vgic_set_forwarding(irqfd->kvm, prod->irq,
>> +					       irqfd->gsi + VGIC_NR_PRIVATE_IRQS);
>> +	case IRQ_BYPASS_VFIO_PCI_MSI:
>> +		return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq,
>> +						  &irqfd->irq_entry);
>> +	default:
>>  		return 0;
>> -
>> -	return kvm_vgic_set_forwarding(irqfd->kvm, prod->irq,
>> -				       irqfd->gsi + VGIC_NR_PRIVATE_IRQS);
>> +	}
>>  }
>>  void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>>  				      struct irq_bypass_producer *prod)
>> @@ -1469,11 +1474,17 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>>  	struct kvm_kernel_irqfd *irqfd =
>>  		container_of(cons, struct kvm_kernel_irqfd, consumer);
>>  
>> -	if (prod->type != IRQ_BYPASS_VFIO_PLATFORM)
>> -		return;
>> +	switch (prod->type) {
>> +	case IRQ_BYPASS_VFIO_PLATFORM:
>> +		kvm_vgic_unset_forwarding(irqfd->kvm, prod->irq,
>> +					  irqfd->gsi + VGIC_NR_PRIVATE_IRQS);
>> +		break;
>>  
>> -	kvm_vgic_unset_forwarding(irqfd->kvm, prod->irq,
>> -				  irqfd->gsi + VGIC_NR_PRIVATE_IRQS);
>> +	case IRQ_BYPASS_VFIO_PCI_MSI:
>> +		kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq,
>> +					     &irqfd->irq_entry);
>> +		break;
>> +	}
>>  }
>>  
>>  void kvm_arch_irq_bypass_stop(struct irq_bypass_consumer *cons)
>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
>> index 207e1fda0dcd..338c86c5159f 100644
>> --- a/virt/kvm/arm/vgic/vgic-v4.c
>> +++ b/virt/kvm/arm/vgic/vgic-v4.c
>> @@ -72,3 +72,106 @@ void vgic_v4_teardown(struct kvm *kvm)
>>  	its_vm->nr_vpes = 0;
>>  	its_vm->vpes = NULL;
>>  }
>> +
>> +static struct vgic_its *vgic_get_its(struct kvm *kvm,
>> +				     struct kvm_kernel_irq_routing_entry *irq_entry)
>> +{
>> +	struct kvm_msi msi  = (struct kvm_msi) {
>> +		.address_lo	= irq_entry->msi.address_lo,
>> +		.address_hi	= irq_entry->msi.address_hi,
>> +		.data		= irq_entry->msi.data,
>> +		.flags		= irq_entry->msi.flags,
>> +		.devid		= irq_entry->msi.devid,
>> +	};
>> +
>> +	/*
>> +	 * Get a reference on the LPI. If NULL, this is not a valid
>> +	 * translation for any of our vITSs.
>> +	 */
>> +	return vgic_msi_to_its(kvm, &msi);
>> +}
>> +
>> +int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>> +			       struct kvm_kernel_irq_routing_entry *irq_entry)
>> +{
>> +	struct vgic_its *its;
>> +	struct vgic_irq *irq;
>> +	struct its_vlpi_map map;
>> +	int ret;
>> +
>> +	if (!vgic_is_v4_capable(kvm))
>> +		return 0;
>> +
>> +	/*
>> +	 * Get the ITS, and escape early on error (not a valid
>> +	 * doorbell for any of our vITSs).
>> +	 */
>> +	its = vgic_get_its(kvm, irq_entry);
>> +	if (IS_ERR(its))
>> +		return 0;
>> +
>> +	mutex_lock(&its->its_lock);
>> +
>> +	/* Perform then actual DevID/EventID -> LPI translation. */
>> +	ret = vgic_its_resolve_lpi(kvm, its, irq_entry->msi.devid,
>> +				   irq_entry->msi.data, &irq);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/*
>> +	 * Emit the mapping request. If it fails, the ITS probably
>> +	 * isn't v4 compatible, so let's silently bail out. Holding
>> +	 * the ITS lock should ensure that nothing can modify the
>> +	 * target vcpu.
>> +	 */
>> +	map = (struct its_vlpi_map) {
>> +		.vm		= &kvm->arch.vgic.its_vm,
>> +		.vintid		= irq->intid,
>> +		.db_enabled	= true,
>> +		.vpe_idx	= irq->target_vcpu->vcpu_id,

This is just wrong. We cannot assume that the vcpu_id has anything to do
with the vpe_idx. It happens to be the same thing now, but the two things
should be clearly disconnected.

I suggest the following (untested):

diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
index cf5d6e2de6b8..0146e004401a 100644
--- a/virt/kvm/arm/vgic/vgic-v4.c
+++ b/virt/kvm/arm/vgic/vgic-v4.c
@@ -251,13 +251,27 @@ static void dump_routing(int virq, struct kvm_kernel_irq_routing_entry *irq_entr
 
 }
 
+static int vgic_v4_vcpu_to_index(struct its_vm *its_vm, struct kvm_vcpu *vcpu)
+{
+	int i;
+
+	for (i = 0; i < its_vm->nr_vpes; i++) {
+		struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
+
+		if (its_vm->vpes[i] == vpe)
+			return i;
+	}
+
+	return -ENODEV;
+}
+
 int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 			       struct kvm_kernel_irq_routing_entry *irq_entry)
 {
 	struct vgic_its *its;
 	struct vgic_irq *irq;
 	struct its_vlpi_map map;
-	int ret;
+	int ret, idx;
 
 	dump_routing(virq, irq_entry, true);
 
@@ -280,6 +294,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 	if (ret)
 		goto out;
 
+	idx = vgic_v4_vcpu_to_index(&kvm->arch.vgic.its_vm,
+				    irq->target_vcpu);
+	if (idx < 0)
+		goto out;
+
 	/*
 	 * Emit the mapping request. If it fails, the ITS probably
 	 * isn't v4 compatible, so let's silently bail out. Holding
@@ -290,7 +309,7 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 		.vm		= &kvm->arch.vgic.its_vm,
 		.vintid		= irq->intid,
 		.db_enabled	= true,
-		.vpe_idx	= irq->target_vcpu->vcpu_id,
+		.vpe_idx	= idx,
 	};
 
 	if (its_map_vlpi(virq, &map))

Another option would be to just pass the pointer to the vpe instead,
and let the irq code to figure out the index, which can easily be
derived the doorbells (vpe->vpe_db_lpi - vpe->its_vm->db_lpi_base).

I'll work something out for the next version.

Thanks,

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