| 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
| ||
|
Date: Wed, 23 Aug 2017 10:58:38 +0200
From: Auger Eric <eric.auger@...hat.com>
To: Christoffer Dall <cdall@...aro.org>
Cc: eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu,
alex.williamson@...hat.com, b.reynal@...tualopensystems.com,
pbonzini@...hat.com, marc.zyngier@....com,
christoffer.dall@...aro.org, drjones@...hat.com, wei@...hat.com
Subject: Re: [PATCH v2 6/8] KVM: arm/arm64: vgic: Implement forwarding setting
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@...hat.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
>
Powered by blists - more mailing lists