From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751635AbdE3Myx (ORCPT ); Tue, 30 May 2017 08:54:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38244 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750952AbdE3Myu (ORCPT ); Tue, 30 May 2017 08:54:50 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8D53D13A5D Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=eric.auger@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 8D53D13A5D Subject: Re: [PATCH 09/10] KVM: arm/arm64: vgic: Implement forwarding setting To: Marc Zyngier References: <1495656803-28011-1-git-send-email-eric.auger@redhat.com> <1495656803-28011-10-git-send-email-eric.auger@redhat.com> <877f14lp76.fsf@arm.com> Cc: eric.auger.pro@gmail.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, alex.williamson@redhat.com, pbonzini@redhat.com, christoffer.dall@linaro.org, drjones@redhat.com, wei@redhat.com From: Auger Eric Message-ID: <9192256c-157b-2112-b8ef-e7815ad14e7d@redhat.com> Date: Tue, 30 May 2017 14:54:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <877f14lp76.fsf@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 30 May 2017 12:54:44 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 25/05/2017 21:19, Marc Zyngier wrote: > On Wed, May 24 2017 at 10:13:22 pm BST, Eric Auger wrote: >> Implements kvm_vgic_[set|unset]_forwarding. >> >> Handle low-level VGIC programming and consistent irqchip >> programming. >> >> Signed-off-by: Eric Auger >> >> --- >> --- >> include/kvm/arm_vgic.h | 5 +++ >> virt/kvm/arm/vgic/vgic.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 110 insertions(+) >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h >> index 695ebc7..7ddac8a 100644 >> --- a/include/kvm/arm_vgic.h >> +++ b/include/kvm/arm_vgic.h >> @@ -343,4 +343,9 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi); >> */ >> int kvm_vgic_setup_default_irq_routing(struct kvm *kvm); >> >> +int kvm_vgic_set_forwarding(struct kvm *kvm, unsigned int irq, >> + unsigned int virt_irq); >> +void kvm_vgic_unset_forwarding(struct kvm *kvm, unsigned int irq, >> + unsigned int virt_irq); > > nit: the name of the variables do not match that of the function > definition, and are much clearer there. > >> + >> #endif /* __KVM_ARM_VGIC_H */ >> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c >> index aa0618c..c2add8d 100644 >> --- a/virt/kvm/arm/vgic/vgic.c >> +++ b/virt/kvm/arm/vgic/vgic.c >> @@ -17,6 +17,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "vgic.h" >> >> @@ -771,3 +773,106 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq) >> return map_is_active; >> } >> >> +/** >> + * kvm_vgic_set_forwarding - Set IRQ forwarding >> + * >> + * @kvm: kvm handle >> + * @host_irq: the host linux IRQ >> + * @vintid: the virtual INTID >> + * >> + * This function must be called when the IRQ is not active: >> + * ie. not active at GIC level and not currently under injection >> + * into the guest using the unforwarded mode. The physical IRQ must >> + * be disabled and all vCPUs must have been exited and prevented >> + * from being re-entered. >> + */ >> +int kvm_vgic_set_forwarding(struct kvm *kvm, unsigned int host_irq, >> + unsigned int vintid) >> +{ >> + struct kvm_vcpu *vcpu; >> + struct vgic_irq *irq; >> + struct irq_desc *desc; >> + struct irq_data *data; >> + unsigned int pintid; >> + int ret = 0; >> + >> + >> + kvm_debug("%s host linux irq=%d vintid=%d\n", >> + __func__, host_irq, vintid); >> + >> + if (!vgic_valid_spi(kvm, vintid)) >> + return 0; >> + >> + /* find the INTID corresponding to @host_irq */ >> + desc = irq_to_desc(host_irq); >> + if (!desc) { >> + kvm_err("%s: no interrupt descriptor\n", __func__); >> + return -EINVAL; >> + } >> + >> + data = irq_desc_get_irq_data(desc); >> + while (data->parent_data) >> + data = data->parent_data; >> + >> + pintid = data->hwirq; >> + >> + irq = vgic_get_irq(kvm, NULL, vintid); >> + >> + spin_lock(&irq->irq_lock); >> + >> + vcpu = irq->target_vcpu; >> + >> + if (!vcpu) { >> + ret = -EAGAIN; >> + goto unlock; >> + } >> + >> + irq_set_vcpu_affinity(host_irq, vcpu); >> + >> + irq->hw = true; >> + irq->hwintid = pintid; >> + irq->host_irq = host_irq; > > This feels like a duplication of kvm_vgic_map_phys_irq(), specially if > you move the pintid discovery there. Can we somehow unify them? Sure. At the beginning it was just a matter of irq_lock I did not want to release. I was somehow embarrassed by the vcpu param of irq_set_vcpu_affinity. Shall we really test target_vcpu. The actual value is unused for SPI so shouldn't we simply use something != NULL. > >> + >> +unlock: >> + spin_unlock(&irq->irq_lock); >> + vgic_put_irq(kvm, irq); >> + return ret; >> +} >> + >> +/** >> + * kvm_vgic_unset_forwarding - Unset IRQ forwarding >> + * >> + * @kvm: KVM handle >> + * @host_irq: the host Linux IRQ number >> + * @vintid: virtual INTID >> + * >> + * This function must be called when the host irq is disabled and >> + * all vCPUs have been exited and prevented from being re-entered. >> + */ >> +void kvm_vgic_unset_forwarding(struct kvm *kvm, >> + unsigned int host_irq, >> + unsigned int vintid) >> +{ >> + struct vgic_irq *irq; >> + bool active; >> + >> + kvm_debug("%s host_irq=%d virt_irq=%d\n", __func__, host_irq, vintid); >> + >> + irq_get_irqchip_state(host_irq, IRQCHIP_STATE_ACTIVE, &active); >> + >> + irq = vgic_get_irq(kvm, NULL, vintid); >> + spin_lock(&irq->irq_lock); >> + >> + if (!is_unshared_mapped(irq)) >> + goto unlock; >> + >> + if (active) >> + irq_set_irqchip_state(host_irq, IRQCHIP_STATE_ACTIVE, false); >> + >> + irq->hw = false; >> + irq_set_vcpu_affinity(host_irq, NULL); >> + >> +unlock: >> + spin_unlock(&irq->irq_lock); > > Same here. OK Thanks Eric > >> +} >> + > > > Thanks, > > M. >