linux-kernel - Re: [PATCH 09/10] KVM: arm/arm64: vgic: Implement forwarding setting

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 linux-cve-announce 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]
Message-ID: <9192256c-157b-2112-b8ef-e7815ad14e7d@redhat.com>
Date:   Tue, 30 May 2017 14:54:35 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu,
        alex.williamson@...hat.com, pbonzini@...hat.com,
        christoffer.dall@...aro.org, drjones@...hat.com, wei@...hat.com
Subject: Re: [PATCH 09/10] KVM: arm/arm64: vgic: Implement forwarding setting

Hi,

On 25/05/2017 21:19, Marc Zyngier wrote:
> On Wed, May 24 2017 at 10:13:22 pm BST, Eric Auger <eric.auger@...hat.com> wrote:
>> Implements kvm_vgic_[set|unset]_forwarding.
>>
>> Handle low-level VGIC programming and consistent irqchip
>> programming.
>>
>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>>
>> ---
>> ---
>>  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 <linux/kvm.h>
>>  #include <linux/kvm_host.h>
>>  #include <linux/list_sort.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>>  
>>  #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.
>