DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1181AC057FAF
Subject: Re: [PATCH v2 6/8] KVM: arm/arm64: vgic: Implement forwarding setting
To: Christoffer Dall <cdall@linaro.org>
References: <1497531160-29162-1-git-send-email-eric.auger@redhat.com>
 <1497531160-29162-7-git-send-email-eric.auger@redhat.com>
 <20170721131346.GH16350@cbox>
Cc: eric.auger.pro@gmail.com, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
        alex.williamson@redhat.com, b.reynal@virtualopensystems.com,
        pbonzini@redhat.com, marc.zyngier@arm.com, christoffer.dall@linaro.org,
        drjones@redhat.com, wei@redhat.com
From: Auger Eric <eric.auger@redhat.com>
Message-ID: <04c0ed5f-40b3-b0bf-cdf3-8bccbe2430d0@redhat.com>
Date: Wed, 23 Aug 2017 10:58:38 +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: <20170721131346.GH16350@cbox>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5099
Lines: 174

Hi Christoffer,

On 21/07/2017 15:13, Christoffer Dall wrote:
> On Thu, Jun 15, 2017 at 02:52:38PM +0200, Eric Auger wrote:
>> Implements kvm_vgic_[set|unset]_forwarding.
>>
>> Handle low-level VGIC programming and consistent irqchip
>> programming.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - change the parameter names used in the declaration
>> - use kvm_vgic_map/unmap_phys_irq and handle their returned value
>> ---
>>  include/kvm/arm_vgic.h   |  5 +++
>>  virt/kvm/arm/vgic/vgic.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index cceb31d..5064a57 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 host_irq,
>> +			    unsigned int vintid);
>> +void kvm_vgic_unset_forwarding(struct kvm *kvm, unsigned int host_irq,
>> +			       unsigned int vintid);
>> +
>>  #endif /* __KVM_ARM_VGIC_H */
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 2e35ac7..9ee3e77 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -781,3 +781,91 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid)
>>  	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;
>> +	int ret;
>> +
>> +	kvm_debug("%s host_irq=%d vintid=%d\n", __func__, host_irq, vintid);
> 
> do you need to check if the vgic is initialized etc. here?
yes
> 
>> +
>> +	if (!vgic_valid_spi(kvm, vintid))
>> +		return -EINVAL;
>> +
>> +	irq = vgic_get_irq(kvm, NULL, vintid);
>> +	spin_lock(&irq->irq_lock);
>> +
>> +	if (irq->hw) {
>> +		ret = -EINVAL;
> 
> is this because it's already forwarded?  How about EBUSY or EEXIST
> instead then?
OK
> 
>> +		goto unlock;
>> +	}
>> +	vcpu = irq->target_vcpu;
>> +	if (!vcpu) {
>> +		ret = -EAGAIN;
> 
> what is this case exactly?
This was discussed previously with Marc
(https://patchwork.kernel.org/patch/9746841/). In GICv3 case the vcpu
parameter is not used in irq_set_vcpu_affinity. What this function does
is tell the GIC not to DIR the physical IRQ.

So in my case I just need a non NULL vcpu passed as parameter of
irq_set_vcpu_affinity. kvm_vgic_map_irq is not using it because we are
handling SPIs. But in GICv4 the actual target vpcu will be needed so I
decided to use this latter and return an error in case it is not known.
> 
>> +		goto unlock;
>> +	}
>> +	
>> +	ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
>> +	if (!ret)
>> +		irq_set_vcpu_affinity(host_irq, vcpu);
> 
> so this is essentially map + set_vcpu_affinity.  Why do we want the GIC
> to do this in one go as opposed to leaving it up to the caller?
The VGIC code already use some genirq functions like
irq_set/get_irqchip_state. Using the irq->lock prevents the 2 actions
from being raced with an kvm_vgic_unset_forwarding(). Both the GIC and
VGIC programming must be consistent.

Thanks

Eric
> 
>> +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 vintid=%d\n", __func__, host_irq, vintid);
> 
> do you need to check if the vgic is initialized etc. here?
> 
>> +
>> +	if (!vgic_valid_spi(kvm, vintid))
>> +		return;
>> +
>> +	irq = vgic_get_irq(kvm, NULL, vintid);
>> +	spin_lock(&irq->irq_lock);
>> +
>> +	if (!irq->hw)
>> +		goto unlock;
>> +
>> +	WARN_ON(irq_get_irqchip_state(host_irq, IRQCHIP_STATE_ACTIVE, &active));
>> +
>> +	if (active)
>> +		irq_set_irqchip_state(host_irq, IRQCHIP_STATE_ACTIVE, false);
>> +
>> +	kvm_vgic_unmap_irq(irq);
>> +	irq_set_vcpu_affinity(host_irq, NULL);
>> +
>> +unlock:
>> +	spin_unlock(&irq->irq_lock);
>> +	vgic_put_irq(kvm, irq);
>> +}
>> +
>> -- 
>> 2.5.5
>>
> 
> Thanks,
> -Christoffer
>