On Tue, Mar 21, 2017 at 01:26:52PM +0000, Marc Zyngier wrote:
> On 21/03/17 12:26, Christoffer Dall wrote:
> > On Tue, Mar 21, 2017 at 11:36:19AM +0000, Marc Zyngier wrote:
> >> On 20/03/17 10:58, Christoffer Dall wrote:
> >>> From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >>>
> >>> Since we always read back the LRs that we wrote to the guest and the
> >>> MISR and EISR registers simply provide a summary of the configuration of
> >>> the bits in the LRs, there is really no need to read back those status
> >>> registers and process them. We can might as well just signal the
> >>> notifyfd when folding the LR state and save some cycles in the process.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >>> ---
> >>> virt/kvm/arm/vgic/vgic-v2.c | 55 ++++++++++-----------------------------------
> >>> virt/kvm/arm/vgic/vgic-v3.c | 47 +++++++++++---------------------------
> >>> virt/kvm/arm/vgic/vgic.c | 8 +++----
> >>> virt/kvm/arm/vgic/vgic.h | 4 ++--
> >>> 4 files changed, 31 insertions(+), 83 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> >>> index dfe6e5e..0172754 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-v2.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> >>> @@ -22,20 +22,6 @@
> >>>
> >>> #include "vgic.h"
> >>>
> >>> -/*
> >>> - * Call this function to convert a u64 value to an unsigned long * bitmask
> >>> - * in a way that works on both 32-bit and 64-bit LE and BE platforms.
> >>> - *
> >>> - * Warning: Calling this function may modify *val.
> >>> - */
> >>> -static unsigned long *u64_to_bitmask(u64 *val)
> >>> -{
> >>> -#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 32
> >>> - *val = (*val >> 32) | (*val << 32);
> >>> -#endif
> >>> - return (unsigned long *)val;
> >>> -}
> >>> -
> >>
> >> Getting rid of u64_to_bitmask makes me feel slightly better...
> >>
> >>> static inline void vgic_v2_write_lr(int lr, u32 val)
> >>> {
> >>> void __iomem *base = kvm_vgic_global_state.vctrl_base;
> >>> @@ -51,38 +37,10 @@ void vgic_v2_init_lrs(void)
> >>> vgic_v2_write_lr(i, 0);
> >>> }
> >>>
> >>> -void vgic_v2_process_maintenance(struct kvm_vcpu *vcpu)
> >>> +void vgic_v2_clear_uie(struct kvm_vcpu *vcpu)
> >>> {
> >>> struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2;
> >>> -
> >>> - if (cpuif->vgic_misr & GICH_MISR_EOI) {
> >>> - u64 eisr = cpuif->vgic_eisr;
> >>> - unsigned long *eisr_bmap = u64_to_bitmask(&eisr);
> >>> - int lr;
> >>> -
> >>> - for_each_set_bit(lr, eisr_bmap, kvm_vgic_global_state.nr_lr) {
> >>> - u32 intid = cpuif->vgic_lr[lr] & GICH_LR_VIRTUALID;
> >>> -
> >>> - WARN_ON(cpuif->vgic_lr[lr] & GICH_LR_STATE);
> >>> -
> >>> - /* Only SPIs require notification */
> >>> - if (vgic_valid_spi(vcpu->kvm, intid))
> >>> - kvm_notify_acked_irq(vcpu->kvm, 0,
> >>> - intid - VGIC_NR_PRIVATE_IRQS);
> >>> - }
> >>> - }
> >>> -
> >>> - /* check and disable underflow maintenance IRQ */
> >>> cpuif->vgic_hcr &= ~GICH_HCR_UIE;
> >>> -
> >>> - /*
> >>> - * In the next iterations of the vcpu loop, if we sync the
> >>> - * vgic state after flushing it, but before entering the guest
> >>> - * (this happens for pending signals and vmid rollovers), then
> >>> - * make sure we don't pick up any old maintenance interrupts
> >>> - * here.
> >>> - */
> >>> - cpuif->vgic_eisr = 0;
> >>> }
> >>>
> >>> void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
> >>> @@ -92,6 +50,12 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
> >>> cpuif->vgic_hcr |= GICH_HCR_UIE;
> >>> }
> >>>
> >>> +static bool lr_signals_eoi_mi(u32 lr_val)
> >>> +{
> >>> + return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI) &&
> >>> + !(lr_val & GICH_LR_HW);
> >>> +}
> >>> +
> >>> /*
> >>> * transfer the content of the LRs back into the corresponding ap_list:
> >>> * - active bit is transferred as is
> >>> @@ -109,6 +73,11 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> >>> u32 intid = val & GICH_LR_VIRTUALID;
> >>> struct vgic_irq *irq;
> >>>
> >>> + /* Notify fds when the guest EOI'ed a level-triggered SPI */
> >>> + if (lr_signals_eoi_mi(val) && vgic_valid_spi(vcpu->kvm, intid))
> >>> + kvm_notify_acked_irq(vcpu->kvm, 0,
> >>> + intid - VGIC_NR_PRIVATE_IRQS);
> >>> +
> >>> irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
> >>>
> >>> spin_lock(&irq->irq_lock);
> >>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> >>> index 3d7796c..7880c5c 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-v3.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> >>> @@ -21,42 +21,9 @@
> >>>
> >>> #include "vgic.h"
> >>>
> >>> -void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
> >>> +void vgic_v3_clear_uie(struct kvm_vcpu *vcpu)
> >>> {
> >>> struct vgic_v3_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v3;
> >>> - u32 model = vcpu->kvm->arch.vgic.vgic_model;
> >>> -
> >>> - if (cpuif->vgic_misr & ICH_MISR_EOI) {
> >>> - unsigned long eisr_bmap = cpuif->vgic_eisr;
> >>> - int lr;
> >>> -
> >>> - for_each_set_bit(lr, &eisr_bmap, kvm_vgic_global_state.nr_lr) {
> >>> - u32 intid;
> >>> - u64 val = cpuif->vgic_lr[lr];
> >>> -
> >>> - if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >>> - intid = val & ICH_LR_VIRTUAL_ID_MASK;
> >>> - else
> >>> - intid = val & GICH_LR_VIRTUALID;
> >>> -
> >>> - WARN_ON(cpuif->vgic_lr[lr] & ICH_LR_STATE);
> >>> -
> >>> - /* Only SPIs require notification */
> >>> - if (vgic_valid_spi(vcpu->kvm, intid))
> >>> - kvm_notify_acked_irq(vcpu->kvm, 0,
> >>> - intid - VGIC_NR_PRIVATE_IRQS);
> >>> - }
> >>> -
> >>> - /*
> >>> - * In the next iterations of the vcpu loop, if we sync
> >>> - * the vgic state after flushing it, but before
> >>> - * entering the guest (this happens for pending
> >>> - * signals and vmid rollovers), then make sure we
> >>> - * don't pick up any old maintenance interrupts here.
> >>> - */
> >>> - cpuif->vgic_eisr = 0;
> >>> - }
> >>> -
> >>> cpuif->vgic_hcr &= ~ICH_HCR_UIE;
> >>> }
> >>>
> >>> @@ -67,6 +34,12 @@ void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
> >>> cpuif->vgic_hcr |= ICH_HCR_UIE;
> >>> }
> >>>
> >>> +static bool lr_signals_eoi_mi(u64 lr_val)
> >>> +{
> >>> + return !(lr_val & ICH_LR_STATE) && (lr_val & ICH_LR_EOI) &&
> >>> + !(lr_val & ICH_LR_HW);
> >>> +}
> >>> +
> >>> void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> >>> {
> >>> struct vgic_v3_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v3;
> >>> @@ -82,6 +55,12 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> >>> intid = val & ICH_LR_VIRTUAL_ID_MASK;
> >>> else
> >>> intid = val & GICH_LR_VIRTUALID;
> >>> +
> >>> + /* Notify fds when the guest EOI'ed a level-triggered IRQ */
> >>> + if (lr_signals_eoi_mi(val) && vgic_valid_spi(vcpu->kvm, intid))
> >>> + kvm_notify_acked_irq(vcpu->kvm, 0,
> >>> + intid - VGIC_NR_PRIVATE_IRQS);
> >>> +
> >>> irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
> >>> if (!irq) /* An LPI could have been unmapped. */
> >>> continue;
> >>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> >>> index 1436c2e..f5b3c25 100644
> >>> --- a/virt/kvm/arm/vgic/vgic.c
> >>> +++ b/virt/kvm/arm/vgic/vgic.c
> >>> @@ -527,12 +527,12 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
> >>> spin_unlock(&vgic_cpu->ap_list_lock);
> >>> }
> >>>
> >>> -static inline void vgic_process_maintenance_interrupt(struct kvm_vcpu *vcpu)
> >>> +static inline void vgic_clear_uie(struct kvm_vcpu *vcpu)
> >>> {
> >>> if (kvm_vgic_global_state.type == VGIC_V2)
> >>> - vgic_v2_process_maintenance(vcpu);
> >>> + vgic_v2_clear_uie(vcpu);
> >>> else
> >>> - vgic_v3_process_maintenance(vcpu);
> >>> + vgic_v3_clear_uie(vcpu);
> >>> }
> >>>
> >>> static inline void vgic_fold_lr_state(struct kvm_vcpu *vcpu)
> >>> @@ -642,7 +642,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> >>> if (unlikely(!vgic_initialized(vcpu->kvm)))
> >>> return;
> >>>
> >>> - vgic_process_maintenance_interrupt(vcpu);
> >>> + vgic_clear_uie(vcpu);
> >>> vgic_fold_lr_state(vcpu);
> >>
> >> nit: If you moved vgic_*_clear_uie() into vgic_*_fold_lr_state, you
> >> could get rid of this call, extra prototypes, and save yourself the
> >> extra check. Not a big deal though.
> >>
> >
> > I thought about this, but the problem is that strictly speaking, you
> > don't need to consume any LRs to set the underflow, you just need enough
> > in the AP list, and it's theoretically possible that you have nr_lrs+1
> > interrupts in the AP list which have all been migrated away.
>
> But isn't that a problem we already have, independently of reworking the
> above?
>
Sort of, but it doesn't really hurt anyone, because we currently always
clear the underflow bit unconditionally, so worst case, you'll take an
additional exit.
After I introduce the conditionals on the sync side, you could loop
forever between KVM/VM.
> > So, it would require something like this including a rework in
> > flush_lr_state to work with the following patch which only calls
> > vgic_fold_lr_state when there are in fact anything live in LRs:
> >
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index e5cf930..4b1feef 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -601,10 +601,8 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
> >
> > DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&vgic_cpu->ap_list_lock));
> >
> > - if (compute_ap_list_depth(vcpu) > kvm_vgic_global_state.nr_lr) {
> > - vgic_set_underflow(vcpu);
> > + if (compute_ap_list_depth(vcpu) > kvm_vgic_global_state.nr_lr)
> > vgic_sort_ap_list(vcpu);
> > - }
> >
> > list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
> > spin_lock(&irq->irq_lock);
> > @@ -623,8 +621,12 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
> > next:
> > spin_unlock(&irq->irq_lock);
> >
> > - if (count == kvm_vgic_global_state.nr_lr)
> > + if (count == kvm_vgic_global_state.nr_lr) {
> > + if (!list_is_last(&irq->ap_list,
> > + &vgic_cpu->ap_list_head))
> > + vgic_set_underflow(vcpu);
> > break;
> > + }
> > }
> >
> > vcpu->arch.vgic_cpu.used_lrs = count;
> >
> > What do you think?
>
> I think this deserves a patch of its own. Also, how about renaming
> clear_uie to clear_underflow?
>
Fair enough, I can add this.
Thanks,
-Christoffer
