On Wed, Aug 30, 2017 at 03:46:12PM +0100, Marc Zyngier wrote:
> On 28/08/17 19:18, Christoffer Dall wrote:
> > On Mon, Jul 31, 2017 at 06:26:24PM +0100, Marc Zyngier wrote:
> >> The current implementation of MOVALL doesn't allow us to call
> >> into the core ITS code as we hold a number of spinlocks.
> >>
> >> Let's try a method used in other parts of the code, were we copy
> >> the intids of the candicate interrupts, and then do whatever
> >> we need to do with them outside of the critical section.
> >>
> >> This allows us to move the interrupts one by one, at the expense
> >> of a bit of CPU time. Who cares? MOVALL is such a stupid command
> >> anyway...
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >> ---
> >> virt/kvm/arm/vgic/vgic-its.c | 27 ++++++++++++++++++++-------
> >> 1 file changed, 20 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 2c065c970ba0..65cc77fde609 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -1147,11 +1147,12 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its,
> >> static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
> >> u64 *its_cmd)
> >> {
> >> - struct vgic_dist *dist = &kvm->arch.vgic;
> >> u32 target1_addr = its_cmd_get_target_addr(its_cmd);
> >> u32 target2_addr = its_cmd_mask_field(its_cmd, 3, 16, 32);
> >> struct kvm_vcpu *vcpu1, *vcpu2;
> >> struct vgic_irq *irq;
> >> + u32 *intids;
> >> + int irq_count, i;
> >>
> >> if (target1_addr >= atomic_read(&kvm->online_vcpus) ||
> >> target2_addr >= atomic_read(&kvm->online_vcpus))
> >> @@ -1163,19 +1164,31 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
> >> vcpu1 = kvm_get_vcpu(kvm, target1_addr);
> >> vcpu2 = kvm_get_vcpu(kvm, target2_addr);
> >>
> >> - spin_lock(&dist->lpi_list_lock);
> >> + irq_count = vgic_copy_lpi_list(vcpu1, &intids);
> >> + if (irq_count < 0)
> >> + return irq_count;
> >>
> >> - list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> >> - spin_lock(&irq->irq_lock);
> >> + for (i = 0; i < irq_count; i++) {
> >> + irq = vgic_get_irq(kvm, NULL, intids[i]);
> >> + if (!irq)
> >> + continue;
> >
> > Getting irq == NULL means that we've removed this LPI since
> > vgic_copy_lpi_list, right? Can this really happen while we hold the its
> > mutex?
>
> A disappearing LPI can only be the result of a DISCARD, which cannot
> happen, as we indeed hold the ITS lock.
>
> > Also, we don't check this in its_sync_lpi_pending_table which would
> > indicate that we either have a bug there or are being overly careful
> > here (or should change the continue to BUG).
>
> Let's aim for consistency. I'll drop this test.
>
> >
> >
> >>
> >> if (irq->target_vcpu == vcpu1)
> >> irq->target_vcpu = vcpu2;
> >>
> >> - spin_unlock(&irq->irq_lock);
> >
> > Is it safe to modify target_vcpu without holding the irq_lock?
>
> Unintentional regression. I'll fix that. But I wonder if there is an
> actual point in testing testing the target_vcpu here. Since we hold the
> ITS lock, we're damn sure that the affinity can't be changed, right?
>
Ah, yes, because you filtered the list on the source VCPU already you
should be able to let go of this check.
Thanks,
-Christoffer
