Hi Eric, On 23/07/2019 16:10, Auger Eric wrote: > Hi Marc, > > On 6/11/19 7:03 PM, Marc Zyngier wrote: >> When performing an MSI injection, let's first check if the translation >> is already in the cache. If so, let's inject it quickly without >> going through the whole translation process. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >> --- >> virt/kvm/arm/vgic/vgic-its.c | 36 ++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic/vgic.h | 1 + >> 2 files changed, 37 insertions(+) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 62932458476a..83d80ec33473 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -577,6 +577,20 @@ static struct vgic_irq *__vgic_its_check_cache(struct vgic_dist *dist, >> return irq; >> } >> >> +static struct vgic_irq *vgic_its_check_cache(struct kvm *kvm, phys_addr_t db, >> + u32 devid, u32 eventid) >> +{ >> + struct vgic_dist *dist = &kvm->arch.vgic; >> + struct vgic_irq *irq; >> + unsigned long flags; >> + >> + raw_spin_lock_irqsave(&dist->lpi_list_lock, flags); >> + irq = __vgic_its_check_cache(dist, db, devid, eventid); >> + raw_spin_unlock_irqrestore(&dist->lpi_list_lock, flags); >> + >> + return irq; >> +} >> + >> static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its, >> u32 devid, u32 eventid, >> struct vgic_irq *irq) >> @@ -736,6 +750,25 @@ static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its, >> return 0; >> } >> >> +int vgic_its_inject_cached_translation(struct kvm *kvm, struct kvm_msi *msi) >> +{ >> + struct vgic_irq *irq; >> + unsigned long flags; >> + phys_addr_t db; >> + >> + db = (u64)msi->address_hi << 32 | msi->address_lo; >> + irq = vgic_its_check_cache(kvm, db, msi->devid, msi->data); > > I think we miss a check of its->enabled. This is currently done in > vgic_its_resolve_lpi() but now likely to be bypassed. But why would a translation be cached if the ITS is disabled? It should never haver been there the first place (vgic_its_resolve_lpi does check for the ITS being enabled, as you pointed out). Which makes me think that we miss an invalidate on an ITS being disabled: diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 2633b0e88981..5f2ad74ad834 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1719,6 +1719,8 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its, goto out; its->enabled = !!(val & GITS_CTLR_ENABLE); + if (!its->enabled) + vgic_its_invalidate_cache(kvm); /* * Try to process any pending commands. This function bails out early What do you think? M. -- Jazz is not dead. It just smells funny...