| 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: Tue, 30 May 2017 14:50:28 +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 08/10] KVM: arm/arm64: vgic: Handle unshared mapped
interrupts
Hi Marc,
On 25/05/2017 21:14, Marc Zyngier wrote:
> On Wed, May 24 2017 at 10:13:21 pm BST, Eric Auger <eric.auger@...hat.com> wrote:
>> Virtual interrupts directly mapped to physical interrupts require
>> some special care. Their pending and active state must be observed
>> at distributor level and not in the list register.
>>
>> Also a level sensitive interrupt's level is not toggled down by any
>> maintenance IRQ handler as the EOI is not trapped.
>>
>> This patch adds an host_irq field in vgic_irq struct to easily
>> get the irqchip state of the host irq. We also handle the
>> physical IRQ case in vgic_validate_injection and add helpers to
>> get the line level and active state.
>>
>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>> ---
>> include/kvm/arm_vgic.h | 4 +++-
>> virt/kvm/arm/arch_timer.c | 3 ++-
>> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
>> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
>> 4 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index ef71858..695ebc7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -112,6 +112,7 @@ struct vgic_irq {
>> bool hw; /* Tied to HW IRQ */
>> struct kref refcount; /* Used for LPIs */
>> u32 hwintid; /* HW INTID number */
>> + unsigned int host_irq; /* linux irq corresponding to hwintid */
>> union {
>> u8 targets; /* GICv2 target VCPUs mask */
>> u32 mpidr; /* GICv3 target VCPU */
>> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> bool level);
>> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> bool level);
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> + u32 virt_irq, u32 phys_irq);
>> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 5976609..45f4779 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>> * Tell the VGIC that the virtual interrupt is tied to a
>> * physical interrupt. We do that once per VCPU.
>> */
>> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
>> + vtimer->irq.irq, phys_irq);
>> if (ret)
>> return ret;
>>
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 83b24d2..aa0618c 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -137,6 +137,28 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>> kfree(irq);
>> }
>>
>> +bool irq_line_level(struct vgic_irq *irq)
>> +{
>> + bool line_level = irq->line_level;
>> +
>> + if (unlikely(is_unshared_mapped(irq)))
>
> The "unshared" bit doesn't mean much to me. Do you want to say "an
> interrupt that belongs to a device only accessed by a single VM"?
Yes. This was the former naming. timer used shared HW irq and others
were dubberd unshared (https://lkml.org/lkml/2015/11/19/362).
>
> Given that this can only be an SPI, can we use something like
> "is_mapped_spi()" instead? I find it a lot more readable, but I'm open
> to alternative suggestions.
Yep!
Thanks
Eric
>
>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_PENDING,
>> + &line_level));
>> + return line_level;
>> +}
>> +
>> +bool irq_is_active(struct vgic_irq *irq)
>> +{
>> + bool is_active = irq->active;
>> +
>> + if (unlikely(is_unshared_mapped(irq)))
>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_ACTIVE,
>> + &is_active));
>> + return is_active;
>> +}
>> +
>> /**
>> * kvm_vgic_target_oracle - compute the target vcpu for an irq
>> *
>> @@ -153,7 +175,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
>> DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&irq->irq_lock));
>>
>> /* If the interrupt is active, it must stay on the current vcpu */
>> - if (irq->active)
>> + if (irq_is_active(irq))
>> return irq->vcpu ? : irq->target_vcpu;
>>
>> /*
>> @@ -195,14 +217,18 @@ static int vgic_irq_cmp(void *priv, struct list_head *a, struct list_head *b)
>> {
>> struct vgic_irq *irqa = container_of(a, struct vgic_irq, ap_list);
>> struct vgic_irq *irqb = container_of(b, struct vgic_irq, ap_list);
>> + bool activea, activeb;
>> bool penda, pendb;
>> int ret;
>>
>> spin_lock(&irqa->irq_lock);
>> spin_lock_nested(&irqb->irq_lock, SINGLE_DEPTH_NESTING);
>>
>> - if (irqa->active || irqb->active) {
>> - ret = (int)irqb->active - (int)irqa->active;
>> + activea = irq_is_active(irqa);
>> + activeb = irq_is_active(irqb);
>> +
>> + if (activea || activeb) {
>> + ret = (int)activeb - (int)activea;
>> goto out;
>> }
>>
>> @@ -234,13 +260,17 @@ static void vgic_sort_ap_list(struct kvm_vcpu *vcpu)
>>
>> /*
>> * Only valid injection if changing level for level-triggered IRQs or for a
>> - * rising edge.
>> + * rising edge. Injection of virtual interrupts associated to physical
>> + * interrupts always is valid.
>> */
>> static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>> {
>> switch (irq->config) {
>> case VGIC_CONFIG_LEVEL:
>> - return irq->line_level != level;
>> + if (unlikely(is_unshared_mapped(irq)))
>> + return true;
>> + else
>> + return irq->line_level != level;
>
> This would be more readable as:
>
> return (irq->line_level != level ||
> unlikely(is_unshared_mapped(irq)));
OK
Thanks
Eric
>
>> case VGIC_CONFIG_EDGE:
>> return level;
>> }
>> @@ -392,7 +422,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> return 0;
>> }
>>
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> + u32 virt_irq, u32 phys_irq)
>> {
>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
>>
>> @@ -402,6 +433,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>>
>> irq->hw = true;
>> irq->hwintid = phys_irq;
>> + irq->host_irq = host_irq;
>
> If you're now passing the Linux IRQ to the mapping function, you might
> as well move the code that extracts the host hwirq here as well.
>
>>
>> spin_unlock(&irq->irq_lock);
>> vgic_put_irq(vcpu->kvm, irq);
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index da83e4c..dc4972b 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -17,6 +17,7 @@
>> #define __KVM_ARM_VGIC_NEW_H__
>>
>> #include <linux/irqchip/arm-gic-common.h>
>> +#include <linux/interrupt.h>
>>
>> #define PRODUCT_ID_KVM 0x4b /* ASCII code K */
>> #define IMPLEMENTER_ARM 0x43b
>> @@ -96,14 +97,20 @@
>> /* we only support 64 kB translation table page size */
>> #define KVM_ITS_L1E_ADDR_MASK GENMASK_ULL(51, 16)
>>
>> +bool irq_line_level(struct vgic_irq *irq);
>> +bool irq_is_active(struct vgic_irq *irq);
>> +
>> static inline bool irq_is_pending(struct vgic_irq *irq)
>> {
>> if (irq->config == VGIC_CONFIG_EDGE)
>> return irq->pending_latch;
>> else
>> - return irq->pending_latch || irq->line_level;
>> + return irq->pending_latch || irq_line_level(irq);
>> }
>>
>> +#define is_unshared_mapped(i) \
>> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
>> +
>> /*
>> * This struct provides an intermediate representation of the fields contained
>> * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
>
> Thanks,
>
> M.
>
Powered by blists - more mailing lists