| Message ID | 1441395650-19663-4-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/04/2015 09:40 PM, Christoffer Dall wrote: > Currently vgic_process_maintenance() processes dealing with a completed > level-triggered interrupt directly, but we are soon going to reuse this > logic for level-triggered mapped interrupts with the HW bit set, so > move this logic into a separate static function. > > Probably the most scary part of this commit is convincing yourself that > the current flow is safe compared to the old one. In the following I > try to list the changes and why they are harmless: > > Move vgic_irq_clear_queued after kvm_notify_acked_irq: > Harmless because the effect of clearing the queued flag wrt. > kvm_set_irq is only that vgic_update_irq_pending does not set the > pending bit on the emulated CPU interface or in the pending_on_cpu > bitmask, well actually the notifier calls vgic_update_irq_pending with level ==0 so it does not reach the can_sample. but we set this in __kvm_vgic_sync_hwstate later on if the > level is stil high. still Reviewed-by: Eric Auger <eric.auger@linaro.org> Eric > > Move vgic_set_lr before kvm_notify_acked_irq: > Also, harmless because the LR are cpu-local operations and > kvm_notify_acked only affects the dist > > Move vgic_dist_irq_clear_soft_pend after kvm_notify_acked_irq: > Also harmless because it's just a bit which is cleared and altering > the line state does not affect this bit. > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > virt/kvm/arm/vgic.c | 88 ++++++++++++++++++++++++++++++----------------------- > 1 file changed, 50 insertions(+), 38 deletions(-) > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 6bd1c9b..fe0e5db 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1322,12 +1322,56 @@ epilog: > } > } > > +static int process_level_irq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr) > +{ > + int level_pending = 0; > + > + vlr.state = 0; > + vlr.hwirq = 0; > + vgic_set_lr(vcpu, lr, vlr); > + > + /* > + * If the IRQ was EOIed (called from vgic_process_maintenance) or it > + * went from active to non-active (called from vgic_sync_hwirq) it was > + * also ACKed and we we therefore assume we can clear the soft pending > + * state (should it had been set) for this interrupt. > + * > + * Note: if the IRQ soft pending state was set after the IRQ was > + * acked, it actually shouldn't be cleared, but we have no way of > + * knowing that unless we start trapping ACKs when the soft-pending > + * state is set. > + */ > + vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq); > + > + /* > + * Tell the gic to start sampling the line of this interrupt again. > + */ > + vgic_irq_clear_queued(vcpu, vlr.irq); > + > + /* Any additional pending interrupt? */ > + if (vgic_dist_irq_get_level(vcpu, vlr.irq)) { > + vgic_cpu_irq_set(vcpu, vlr.irq); > + level_pending = 1; > + } else { > + vgic_dist_irq_clear_pending(vcpu, vlr.irq); > + vgic_cpu_irq_clear(vcpu, vlr.irq); > + } > + > + /* > + * Despite being EOIed, the LR may not have > + * been marked as empty. > + */ > + vgic_sync_lr_elrsr(vcpu, lr, vlr); > + > + return level_pending; > +} > + > static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) > { > u32 status = vgic_get_interrupt_status(vcpu); > struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > - bool level_pending = false; > struct kvm *kvm = vcpu->kvm; > + int level_pending = 0; > > kvm_debug("STATUS = %08x\n", status); > > @@ -1342,54 +1386,22 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) > > for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) { > struct vgic_lr vlr = vgic_get_lr(vcpu, lr); > - WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq)); > > - spin_lock(&dist->lock); > - vgic_irq_clear_queued(vcpu, vlr.irq); > + WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq)); > WARN_ON(vlr.state & LR_STATE_MASK); > - vlr.state = 0; > - vgic_set_lr(vcpu, lr, vlr); > > - /* > - * If the IRQ was EOIed it was also ACKed and we we > - * therefore assume we can clear the soft pending > - * state (should it had been set) for this interrupt. > - * > - * Note: if the IRQ soft pending state was set after > - * the IRQ was acked, it actually shouldn't be > - * cleared, but we have no way of knowing that unless > - * we start trapping ACKs when the soft-pending state > - * is set. > - */ > - vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq); > > /* > * kvm_notify_acked_irq calls kvm_set_irq() > - * to reset the IRQ level. Need to release the > - * lock for kvm_set_irq to grab it. > + * to reset the IRQ level, which grabs the dist->lock > + * so we call this before taking the dist->lock. > */ > - spin_unlock(&dist->lock); > - > kvm_notify_acked_irq(kvm, 0, > vlr.irq - VGIC_NR_PRIVATE_IRQS); > - spin_lock(&dist->lock); > - > - /* Any additional pending interrupt? */ > - if (vgic_dist_irq_get_level(vcpu, vlr.irq)) { > - vgic_cpu_irq_set(vcpu, vlr.irq); > - level_pending = true; > - } else { > - vgic_dist_irq_clear_pending(vcpu, vlr.irq); > - vgic_cpu_irq_clear(vcpu, vlr.irq); > - } > > + spin_lock(&dist->lock); > + level_pending |= process_level_irq(vcpu, lr, vlr); > spin_unlock(&dist->lock); > - > - /* > - * Despite being EOIed, the LR may not have > - * been marked as empty. > - */ > - vgic_sync_lr_elrsr(vcpu, lr, vlr); > } > } > >
On Mon, Sep 07, 2015 at 05:32:35PM +0200, Eric Auger wrote: > > > On 09/04/2015 09:40 PM, Christoffer Dall wrote: > > Currently vgic_process_maintenance() processes dealing with a completed > > level-triggered interrupt directly, but we are soon going to reuse this > > logic for level-triggered mapped interrupts with the HW bit set, so > > move this logic into a separate static function. > > > > Probably the most scary part of this commit is convincing yourself that > > the current flow is safe compared to the old one. In the following I > > try to list the changes and why they are harmless: > > > > Move vgic_irq_clear_queued after kvm_notify_acked_irq: > > Harmless because the effect of clearing the queued flag wrt. > > kvm_set_irq is only that vgic_update_irq_pending does not set the > > pending bit on the emulated CPU interface or in the pending_on_cpu > > bitmask, > well actually the notifier calls vgic_update_irq_pending with level ==0 > so it does not reach the can_sample. > but we set this in __kvm_vgic_sync_hwstate later on if the can the notifier never go through a flow where it calls the function with level = 1 ? For example if the interrupt hit in between? In any case, it should still be functionally correct. Thanks for the RB. -Christoffer > > level is stil high. > still > > Reviewed-by: Eric Auger <eric.auger@linaro.org> > > Eric > > > > Move vgic_set_lr before kvm_notify_acked_irq: > > Also, harmless because the LR are cpu-local operations and > > kvm_notify_acked only affects the dist > > > > Move vgic_dist_irq_clear_soft_pend after kvm_notify_acked_irq: > > Also harmless because it's just a bit which is cleared and altering > > the line state does not affect this bit. > > > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > virt/kvm/arm/vgic.c | 88 ++++++++++++++++++++++++++++++----------------------- > > 1 file changed, 50 insertions(+), 38 deletions(-) > > > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > > index 6bd1c9b..fe0e5db 100644 > > --- a/virt/kvm/arm/vgic.c > > +++ b/virt/kvm/arm/vgic.c > > @@ -1322,12 +1322,56 @@ epilog: > > } > > } > > > > +static int process_level_irq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr) > > +{ > > + int level_pending = 0; > > + > > + vlr.state = 0; > > + vlr.hwirq = 0; > > + vgic_set_lr(vcpu, lr, vlr); > > + > > + /* > > + * If the IRQ was EOIed (called from vgic_process_maintenance) or it > > + * went from active to non-active (called from vgic_sync_hwirq) it was > > + * also ACKed and we we therefore assume we can clear the soft pending > > + * state (should it had been set) for this interrupt. > > + * > > + * Note: if the IRQ soft pending state was set after the IRQ was > > + * acked, it actually shouldn't be cleared, but we have no way of > > + * knowing that unless we start trapping ACKs when the soft-pending > > + * state is set. > > + */ > > + vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq); > > + > > + /* > > + * Tell the gic to start sampling the line of this interrupt again. > > + */ > > + vgic_irq_clear_queued(vcpu, vlr.irq); > > + > > + /* Any additional pending interrupt? */ > > + if (vgic_dist_irq_get_level(vcpu, vlr.irq)) { > > + vgic_cpu_irq_set(vcpu, vlr.irq); > > + level_pending = 1; > > + } else { > > + vgic_dist_irq_clear_pending(vcpu, vlr.irq); > > + vgic_cpu_irq_clear(vcpu, vlr.irq); > > + } > > + > > + /* > > + * Despite being EOIed, the LR may not have > > + * been marked as empty. > > + */ > > + vgic_sync_lr_elrsr(vcpu, lr, vlr); > > + > > + return level_pending; > > +} > > + > > static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) > > { > > u32 status = vgic_get_interrupt_status(vcpu); > > struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > > - bool level_pending = false; > > struct kvm *kvm = vcpu->kvm; > > + int level_pending = 0; > > > > kvm_debug("STATUS = %08x\n", status); > > > > @@ -1342,54 +1386,22 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) > > > > for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) { > > struct vgic_lr vlr = vgic_get_lr(vcpu, lr); > > - WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq)); > > > > - spin_lock(&dist->lock); > > - vgic_irq_clear_queued(vcpu, vlr.irq); > > + WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq)); > > WARN_ON(vlr.state & LR_STATE_MASK); > > - vlr.state = 0; > > - vgic_set_lr(vcpu, lr, vlr); > > > > - /* > > - * If the IRQ was EOIed it was also ACKed and we we > > - * therefore assume we can clear the soft pending > > - * state (should it had been set) for this interrupt. > > - * > > - * Note: if the IRQ soft pending state was set after > > - * the IRQ was acked, it actually shouldn't be > > - * cleared, but we have no way of knowing that unless > > - * we start trapping ACKs when the soft-pending state > > - * is set. > > - */ > > - vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq); > > > > /* > > * kvm_notify_acked_irq calls kvm_set_irq() > > - * to reset the IRQ level. Need to release the > > - * lock for kvm_set_irq to grab it. > > + * to reset the IRQ level, which grabs the dist->lock > > + * so we call this before taking the dist->lock. > > */ > > - spin_unlock(&dist->lock); > > - > > kvm_notify_acked_irq(kvm, 0, > > vlr.irq - VGIC_NR_PRIVATE_IRQS); > > - spin_lock(&dist->lock); > > - > > - /* Any additional pending interrupt? */ > > - if (vgic_dist_irq_get_level(vcpu, vlr.irq)) { > > - vgic_cpu_irq_set(vcpu, vlr.irq); > > - level_pending = true; > > - } else { > > - vgic_dist_irq_clear_pending(vcpu, vlr.irq); > > - vgic_cpu_irq_clear(vcpu, vlr.irq); > > - } > > > > + spin_lock(&dist->lock); > > + level_pending |= process_level_irq(vcpu, lr, vlr); > > spin_unlock(&dist->lock); > > - > > - /* > > - * Despite being EOIed, the LR may not have > > - * been marked as empty. > > - */ > > - vgic_sync_lr_elrsr(vcpu, lr, vlr); > > } > > } > > > > >
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 6bd1c9b..fe0e5db 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1322,12 +1322,56 @@ epilog: } } +static int process_level_irq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr) +{ + int level_pending = 0; + + vlr.state = 0; + vlr.hwirq = 0; + vgic_set_lr(vcpu, lr, vlr); + + /* + * If the IRQ was EOIed (called from vgic_process_maintenance) or it + * went from active to non-active (called from vgic_sync_hwirq) it was + * also ACKed and we we therefore assume we can clear the soft pending + * state (should it had been set) for this interrupt. + * + * Note: if the IRQ soft pending state was set after the IRQ was + * acked, it actually shouldn't be cleared, but we have no way of + * knowing that unless we start trapping ACKs when the soft-pending + * state is set. + */ + vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq); + + /* + * Tell the gic to start sampling the line of this interrupt again. + */ + vgic_irq_clear_queued(vcpu, vlr.irq); + + /* Any additional pending interrupt? */ + if (vgic_dist_irq_get_level(vcpu, vlr.irq)) { + vgic_cpu_irq_set(vcpu, vlr.irq); + level_pending = 1; + } else { + vgic_dist_irq_clear_pending(vcpu, vlr.irq); + vgic_cpu_irq_clear(vcpu, vlr.irq); + } + + /* + * Despite being EOIed, the LR may not have + * been marked as empty. + */ + vgic_sync_lr_elrsr(vcpu, lr, vlr); + + return level_pending; +} + static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) { u32 status = vgic_get_interrupt_status(vcpu); struct vgic_dist *dist = &vcpu->kvm->arch.vgic; - bool level_pending = false; struct kvm *kvm = vcpu->kvm; + int level_pending = 0; kvm_debug("STATUS = %08x\n", status); @@ -1342,54 +1386,22 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) { struct vgic_lr vlr = vgic_get_lr(vcpu, lr); - WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq)); - spin_lock(&dist->lock); - vgic_irq_clear_queued(vcpu, vlr.irq); + WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq)); WARN_ON(vlr.state & LR_STATE_MASK); - vlr.state = 0; - vgic_set_lr(vcpu, lr, vlr); - /* - * If the IRQ was EOIed it was also ACKed and we we - * therefore assume we can clear the soft pending - * state (should it had been set) for this interrupt. - * - * Note: if the IRQ soft pending state was set after - * the IRQ was acked, it actually shouldn't be - * cleared, but we have no way of knowing that unless - * we start trapping ACKs when the soft-pending state - * is set. - */ - vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq); /* * kvm_notify_acked_irq calls kvm_set_irq() - * to reset the IRQ level. Need to release the - * lock for kvm_set_irq to grab it. + * to reset the IRQ level, which grabs the dist->lock + * so we call this before taking the dist->lock. */ - spin_unlock(&dist->lock); - kvm_notify_acked_irq(kvm, 0, vlr.irq - VGIC_NR_PRIVATE_IRQS); - spin_lock(&dist->lock); - - /* Any additional pending interrupt? */ - if (vgic_dist_irq_get_level(vcpu, vlr.irq)) { - vgic_cpu_irq_set(vcpu, vlr.irq); - level_pending = true; - } else { - vgic_dist_irq_clear_pending(vcpu, vlr.irq); - vgic_cpu_irq_clear(vcpu, vlr.irq); - } + spin_lock(&dist->lock); + level_pending |= process_level_irq(vcpu, lr, vlr); spin_unlock(&dist->lock); - - /* - * Despite being EOIed, the LR may not have - * been marked as empty. - */ - vgic_sync_lr_elrsr(vcpu, lr, vlr); } }
Currently vgic_process_maintenance() processes dealing with a completed level-triggered interrupt directly, but we are soon going to reuse this logic for level-triggered mapped interrupts with the HW bit set, so move this logic into a separate static function. Probably the most scary part of this commit is convincing yourself that the current flow is safe compared to the old one. In the following I try to list the changes and why they are harmless: Move vgic_irq_clear_queued after kvm_notify_acked_irq: Harmless because the effect of clearing the queued flag wrt. kvm_set_irq is only that vgic_update_irq_pending does not set the pending bit on the emulated CPU interface or in the pending_on_cpu bitmask, but we set this in __kvm_vgic_sync_hwstate later on if the level is stil high. Move vgic_set_lr before kvm_notify_acked_irq: Also, harmless because the LR are cpu-local operations and kvm_notify_acked only affects the dist Move vgic_dist_irq_clear_soft_pend after kvm_notify_acked_irq: Also harmless because it's just a bit which is cleared and altering the line state does not affect this bit. Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- virt/kvm/arm/vgic.c | 88 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 50 insertions(+), 38 deletions(-)