On Tue, Mar 21, 2017 at 10:57:49AM +0000, Marc Zyngier wrote:
> On 20/03/17 10:58, Christoffer Dall wrote:
> > From: Shih-Wei Li <shihwei@xxxxxxxxxxxxxxx>
> >
> > We do not need to flush vgic states in each world switch unless
> > there is pending IRQ queued to the vgic's ap list. We can thus reduce
> > the overhead by not grabbing the spinlock and not making the extra
> > function call to vgic_flush_lr_state.
> >
> > Signed-off-by: Shih-Wei Li <shihwei@xxxxxxxxxxxxxxx>
> > Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx>
> > ---
> > virt/kvm/arm/vgic/vgic.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index 2ac0def..1436c2e 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -637,12 +637,17 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
> > /* Sync back the hardware VGIC state into our emulation after a guest's run. */
> > void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> > {
> > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> > +
> > if (unlikely(!vgic_initialized(vcpu->kvm)))
> > return;
> >
> > vgic_process_maintenance_interrupt(vcpu);
> > vgic_fold_lr_state(vcpu);
> > vgic_prune_ap_list(vcpu);
> > +
> > + /* Make sure we can fast-path in flush_hwstate */
> > + vgic_cpu->used_lrs = 0;
> > }
> >
> > /* Flush our emulation state into the GIC hardware before entering the guest. */
> > @@ -651,6 +656,9 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
> > if (unlikely(!vgic_initialized(vcpu->kvm)))
> > return;
> >
> > + if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
> > + return;
> > +
>
> While I can see the READ_ONCE in list_empty(), it is not completely
> obvious that the race with another CPU injecting an interrupt is safe
> (if I get it correctly, it will have the same effect as if it was added
> right after the critical section below).
Yes, you either observe virtual interrupts or not, that's a benign race.
>
> Can we have a nice comment explaining this?
>
You sure can, I will add something.
> > spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> > vgic_flush_lr_state(vcpu);
> > spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >
>
Thanks,
-Christoffer
