lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 30 Aug 2017 11:28:08 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Christoffer Dall <cdall@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        kvmarm@...ts.cs.columbia.edu, kvm@...r.kernel.org,
        Christoffer Dall <christoffer.dall@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Eric Auger <eric.auger@...hat.com>,
        Shanker Donthineni <shankerd@...eaurora.org>,
        Mark Rutland <mark.rutland@....com>,
        Shameerali Kolothum Thodi 
        <shameerali.kolothum.thodi@...wei.com>
Subject: Re: [PATCH v3 41/59] KVM: arm/arm64: GICv4: Wire mapping/unmapping of
 VLPIs in VFIO irq bypass

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@....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...

Powered by blists - more mailing lists