Hi, On 07/11/2017 15:42, Marc Zyngier wrote: > Hi Eric, > > On 07/11/17 13:06, Auger Eric wrote: >> Hi Marc, >> >> On 27/10/2017 16:28, Marc Zyngier wrote: >>> Let's use the irq bypass mechanism introduced for platform device >>> interrupts >> nit: I would remove "introduced for platform device interrupts" >> as this is not upstream yet. x86 posted interrupts also use it. >> >>> >> and establish our LPI->VLPI mapping. >>> >>> Reviewed-by: Christoffer Dall <[email protected]> >>> Signed-off-by: Marc Zyngier <[email protected]> >>> --- >>> include/kvm/arm_vgic.h | 8 ++++ >>> virt/kvm/arm/arm.c | 6 ++- >>> virt/kvm/arm/vgic/vgic-v4.c | 108 >>> ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 120 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h >>> index 7eeb6c2a2f9c..2f750c770bf2 100644 >>> --- a/include/kvm/arm_vgic.h >>> +++ b/include/kvm/arm_vgic.h >>> @@ -373,4 +373,12 @@ int kvm_vgic_setup_default_irq_routing(struct kvm >>> *kvm); >>> >>> int kvm_vgic_set_owner(struct kvm_vcpu *vcpu, unsigned int intid, void >>> *owner); >>> >>> +struct kvm_kernel_irq_routing_entry; >>> + >>> +int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int irq, >>> + struct kvm_kernel_irq_routing_entry *irq_entry); >>> + >>> +int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int irq, >>> + struct kvm_kernel_irq_routing_entry >>> *irq_entry); >>> + >>> #endif /* __KVM_ARM_VGIC_H */ >>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>> index 5d5218ecd547..8388c1cc23f6 100644 >>> --- a/virt/kvm/arm/arm.c >>> +++ b/virt/kvm/arm/arm.c >>> @@ -1462,7 +1462,8 @@ int kvm_arch_irq_bypass_add_producer(struct >>> irq_bypass_consumer *cons, >>> struct kvm_kernel_irqfd *irqfd = >>> container_of(cons, struct kvm_kernel_irqfd, consumer); >>> >>> - return 0; >>> + return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq, >>> + &irqfd->irq_entry); >>> } >>> void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons, >>> struct irq_bypass_producer *prod) >>> @@ -1470,7 +1471,8 @@ void kvm_arch_irq_bypass_del_producer(struct >>> irq_bypass_consumer *cons, >>> struct kvm_kernel_irqfd *irqfd = >>> container_of(cons, struct kvm_kernel_irqfd, consumer); >>> >>> - return; >>> + kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq, >>> + &irqfd->irq_entry); >>> } >>> >>> void kvm_arch_irq_bypass_stop(struct irq_bypass_consumer *cons) >>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c >>> index c794f0cef09e..01a2889b7b7c 100644 >>> --- a/virt/kvm/arm/vgic/vgic-v4.c >>> +++ b/virt/kvm/arm/vgic/vgic-v4.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/interrupt.h> >>> #include <linux/irqdomain.h> >>> #include <linux/kvm_host.h> >>> +#include <linux/irqchip/arm-gic-v3.h> >>> >>> #include "vgic.h" >>> >>> @@ -81,3 +82,110 @@ void vgic_v4_teardown(struct kvm *kvm) >>> its_vm->nr_vpes = 0; >>> its_vm->vpes = NULL; >>> } >>> + >>> +static struct vgic_its *vgic_get_its(struct kvm *kvm, >>> + struct kvm_kernel_irq_routing_entry >>> *irq_entry) >>> +{ >>> + struct kvm_msi msi = (struct kvm_msi) { >>> + .address_lo = irq_entry->msi.address_lo, >>> + .address_hi = irq_entry->msi.address_hi, >>> + .data = irq_entry->msi.data, >>> + .flags = irq_entry->msi.flags, >>> + .devid = irq_entry->msi.devid, >>> + }; >>> + >>> + /* >>> + * Get a reference on the LPI. If NULL, this is not a valid >>> + * translation for any of our vITSs. >>> + */ >> I don't understand the relevance of the above comment. > > Hmmm. The first part looks like an outdated leftover, as the ITS is not > refcounted, and we don't deal with LPIs here. > >>> + return vgic_msi_to_its(kvm, &msi); >>> +} >>> + >>> +int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq, >> @virq is the host linux irq. virq naming is a bit confusing to me. > > There is plenty of irq-related code that uses this naming. In this > context, we tend to use irq for the vgic view, hwirq for the irqchip > view. How would you call this one? OK > >>> + struct kvm_kernel_irq_routing_entry *irq_entry) >>> +{ >>> + struct vgic_its *its; >>> + struct vgic_irq *irq; >>> + struct its_vlpi_map map; >>> + int ret; >>> + >> Don't you need to check that the linux irq (virq) is an LPI? You may >> encounter some VFIO "producers" for irq that are not LPIs, typically if >> we eventually upstream SPI forwarding. > > That's indeed a concern. The issue is that we don't really have a way to > check this, other than following the irq_data pointers and check that > the hwirq is within a certain range. And even that doesn't guarantee > anything. OK. But somehow this means the userspace can setup forwarding between an SPI and a vLPI, right? > >>> + if (!vgic_supports_direct_msis(kvm)) >>> + return 0; >>> + >>> + /* >>> + * Get the ITS, and escape early on error (not a valid >>> + * doorbell for any of our vITSs). >>> + */ >>> + its = vgic_get_its(kvm, irq_entry); >>> + if (IS_ERR(its)) >>> + return 0; >>> + >>> + mutex_lock(&its->its_lock); >> wouldn't it be safer to take the its_lock before the get_its()? and even >> before the vgic_supports_direct_msis, in an unlikely case where the its >> would disappear? > > How do you get a lock on the ITS before getting a pointer to it? Oh forget it Also, > aren't we in the context of a vcpu here (we trap to userspace, come back > via a VFIO ioctl, and arrive here)? Could the ITS actually vanish from > under our feet here? The only way to do it would be to destroy the VM. correct, sorry for the noise. > > >>> + >>> + /* Perform then actual DevID/EventID -> LPI translation. */ >>> + ret = vgic_its_resolve_lpi(kvm, its, irq_entry->msi.devid, >>> + irq_entry->msi.data, &irq); >>> + if (ret) >>> + goto out; >>> + >>> + /* >>> + * Emit the mapping request. If it fails, the ITS probably >>> + * isn't v4 compatible, so let's silently bail out. Holding >>> + * the ITS lock should ensure that nothing can modify the >>> + * target vcpu. >>> + */ >>> + map = (struct its_vlpi_map) { >>> + .vm = &kvm->arch.vgic.its_vm, >>> + .vpe = >>> &irq->target_vcpu->arch.vgic_cpu.vgic_v3.its_vpe, >>> + .vintid = irq->intid, >>> + .properties = ((irq->priority & 0xfc) | >>> + (irq->enabled ? LPI_PROP_ENABLED : 0) | >>> + LPI_PROP_GROUP1), >> there is an inconsistency between the comment in irqchip/arm-gic-v4.h >> and this setting: >> >> * @properties: Priority and enable bits (as written in the prop table) > > Which inconsistency? I was confused by LPI_PROP_GROUP1 which was not documented in the comment. But looking more carefully in the spec it corresponds to [1] = RES1. > >> Also maybe we could use LPI_PROP_PRIORITY macro instead of 0xfc? > > We could. But given that irq->priority is assigned with > LPI_PROP_PRIORITY(), we could drop the 0xfc altogether. > >>> + .db_enabled = true, >>> + }; >>> + >>> + ret = its_map_vlpi(virq, &map); >> Looking at its_map_lpi implementation, assuming irq_set_vcpu_affinity() >> fails, will you get a change to turn IRQ_DISABLE_LAZY again. > > Good point. I'll fix this. > >>> + if (ret) >>> + goto out; >>> + >>> + irq->hw = true; >>> + irq->host_irq = virq; >> Shouldn't we theoretically hold the irq lock here? > > We hold the ITS lock. The only way to DISCARD the irq would be to issue > another command, which is not possible until we actually release the > lock. Or am I missing something? No you don't. I was rather anticipating other (future) callers, for instance other bypass functions which could mess with those fields.
Thanks Eric > > Thanks, > > M. >
