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

[Marc Zyngier]

On 30/05/17 13:54, Auger Eric wrote:
> 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@xxxxxxxxxx> wrote:
>>> Implements kvm_vgic_[set|unset]_forwarding.
>>>
>>> Handle low-level VGIC programming and consistent irqchip
>>> programming.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>>
>>> ---
>>> ---
>>>  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.

I guess that for the time being, this would be good enough. But GICv4
requires some actual tracking of the affinity, so we may have to bite
the bullet already, and decide that the interrupt is always affine to a
vcpu.

Does this have any userspace visible impact?

Thanks,

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