On Tue, Aug 04, 2015 at 06:08:53PM +0100, Marc Zyngier wrote: > On 04/08/15 15:32, Christoffer Dall wrote: > > On Fri, Jul 24, 2015 at 04:55:09PM +0100, Marc Zyngier wrote: > >> So far, the only use of the HW interrupt facility is the timer, > >> implying that the active state is context-switched for each vcpu, > >> as the device is is shared across all vcpus. > >> > >> This does not work for a device that has been assigned to a VM, > >> as the guest is entierely in control of that device (the HW is > >> not shared). In that case, it makes sense to bypass the whole > >> active state switching, and only track the deactivation of the > >> interrupt. > >> > >> Signed-off-by: Marc Zyngier <[email protected]> > >> --- > >> include/kvm/arm_vgic.h | 6 ++-- > >> virt/kvm/arm/arch_timer.c | 3 +- > >> virt/kvm/arm/vgic.c | 73 > >> +++++++++++++++++++++++++++++++++++++---------- > >> 3 files changed, 64 insertions(+), 18 deletions(-) > >> > >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h > >> index f6bfd79..6f0a4e1 100644 > >> --- a/include/kvm/arm_vgic.h > >> +++ b/include/kvm/arm_vgic.h > >> @@ -164,7 +164,8 @@ struct irq_phys_map { > >> u32 phys_irq; > >> u32 irq; > >> bool deleted; > >> - bool active; > >> + bool shared; > >> + bool active; /* Only valid if shared */ > >> }; > >> > >> struct irq_phys_map_entry { > >> @@ -357,7 +358,8 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 > >> reg); > >> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); > >> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu); > >> struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, > >> - int virt_irq, int irq); > >> + int virt_irq, int irq, > >> + bool shared); > >> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map > >> *map); > >> bool kvm_vgic_get_phys_irq_active(struct irq_phys_map *map); > >> void kvm_vgic_set_phys_irq_active(struct irq_phys_map *map, bool active); > >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > >> index 76e38d2..db21d8f 100644 > >> --- a/virt/kvm/arm/arch_timer.c > >> +++ b/virt/kvm/arm/arch_timer.c > >> @@ -203,7 +203,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu, > >> * Tell the VGIC that the virtual interrupt is tied to a > >> * physical interrupt. We do that once per VCPU. > >> */ > >> - map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq); > >> + map = kvm_vgic_map_phys_irq(vcpu, irq->irq, > >> + host_vtimer_irq, true); > >> if (WARN_ON(IS_ERR(map))) > >> return PTR_ERR(map); > >> > >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > >> index e40ef70..5e6b816 100644 > >> --- a/virt/kvm/arm/vgic.c > >> +++ b/virt/kvm/arm/vgic.c > >> @@ -1128,19 +1128,25 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu > >> *vcpu, int irq, > >> * active in the physical world. Otherwise the > >> * physical interrupt will fire and the guest will > >> * exit before processing the virtual interrupt. > >> + * > >> + * This is of course only valid for a shared > >> + * interrupt. A non shared interrupt should already be > >> + * active. > >> */ > >> if (map) { > >> - int ret; > >> - > >> - BUG_ON(!map->active); > >> vlr.hwirq = map->phys_irq; > >> vlr.state |= LR_HW; > >> vlr.state &= ~LR_EOI_INT; > >> > >> - ret = irq_set_irqchip_state(map->irq, > >> - IRQCHIP_STATE_ACTIVE, > >> - true); > >> - WARN_ON(ret); > >> + if (map->shared) { > >> + int ret; > >> + > >> + BUG_ON(!map->active); > >> + ret = irq_set_irqchip_state(map->irq, > >> + > >> IRQCHIP_STATE_ACTIVE, > >> + true); > >> + WARN_ON(ret); > >> + } > >> > >> /* > >> * Make sure we're not going to sample this > >> @@ -1383,21 +1389,41 @@ static bool vgic_process_maintenance(struct > >> kvm_vcpu *vcpu) > >> static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr) > >> { > >> struct irq_phys_map *map; > >> + bool active; > >> int ret; > >> > >> if (!(vlr.state & LR_HW)) > >> return 0; > >> > >> map = vgic_irq_map_search(vcpu, vlr.irq); > >> - BUG_ON(!map || !map->active); > >> + BUG_ON(!map); > >> + BUG_ON(map->shared && !map->active); > >> > >> ret = irq_get_irqchip_state(map->irq, > >> IRQCHIP_STATE_ACTIVE, > >> - &map->active); > >> + &active); > >> > >> WARN_ON(ret); > >> > >> - if (map->active) { > >> + /* > >> + * For a non-shared interrupt, we have to cater for two > >> + * possible deactivation conditions: > >> + * > >> + * - the physical interrupt is now inactive (EOIed from the > >> + * guest) > > > > nit: whitespace funkyness > > > >> + * - the physical interrupt is still active, but its virtual > >> + * counterpart is flagged as "not queued", indicating another > >> + * interrupt has fired between the EOI and the guest exit. > >> + * > >> + * Also, we are not reactivating a non-shared interrupt > > > > what does reactivating mean? did you mean deactivate? > > Deactivate indeed. > > >> + * ourselves. This is always left to the guest. > > > > In which case, add ", because the device is solely owned by the guest." > > Sure. > > >> + */ > >> + if (!map->shared) > >> + return !active || !vgic_irq_is_queued(vcpu, vlr.irq); > > > > do you really need the second part of the disjunction? > > > > The effect seems to be that we clear the queued flag once again, and > > clear the LR. If we don't do this, won't we simply pick up the pending > > flag next time we're about to run this VCPU and piggy-back on the > > existing LR. Perhaps this is a weird flow though? > > Crucially, we free the LR in this case (the set_bit on elrsr_ptr). If we > don't do this, we're indeed going to schedule the vcpu (it has something > to process), but we never allow piggy-backing on level interrupts. We'd > need some special hack to handle this. > > I definitely feel more comfortable reporting that the interrupt has been > deactivated (which is the case), and let the normal flow pick up the > next injected interrupt. > Yes, you're right, it's much better this way.
I assume you'll still wait with this stuff until the priority drop / EOI stuff is in, so I'll do a formal review again once all the dependencies are there. But this looks good to me. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
