On 29/09/15 15:49, Christoffer Dall wrote:
> We mark edge-triggered interrupts with the HW bit set as queued to
> prevent the VGIC code from injecting LRs with both the Active and
> Pending bits set at the same time while also setting the HW bit,
> because the hardware does not support this.
>
> However, this means that we must also clear the queued flag when we sync
> back a LR where the state on the physical distributor went from active
> to inactive because the guest deactivated the interrupt. At this point
> we must also check if the interrupt is pending on the distributor, and
> tell the VGIC to queue it again if it is.
>
> Since these actions on the sync path are extremely close to those for
> level-triggered interrupts, rename process_level_irq to
> process_queued_irq, allowing it to cater for both cases.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> ---
> virt/kvm/arm/vgic.c | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 53548f1..f3e76e5 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1322,13 +1322,10 @@ epilog:
> }
> }
>
> -static int process_level_irq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
> +static int process_queued_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);
> + int pending = 0;
As I mentioned in my reply to 3/8 already: shouldn't this be "bool"?
>
> /*
> * If the IRQ was EOIed (called from vgic_process_maintenance) or it
> @@ -1344,26 +1341,35 @@ static int process_level_irq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
> vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq);
>
> /*
> - * Tell the gic to start sampling the line of this interrupt again.
> + * Tell the gic to start sampling 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;
> + if (vgic_irq_is_edge(vcpu, vlr.irq)) {
> + BUG_ON(!(vlr.state & LR_HW));
Is that really needed here? I don't see how this function would fail if
called on a non-mapped IRQ. Also the two current callers would always
fulfil this requirement:
- vgic_process_maintenance() already has a WARN_ON(vgic_irq_is_edge)
- vgic_sync_irq() returns early if it's not a mapped IRQ
Removing this would also allow to pass "int irq" instead of "struct
vgic_lr vlr".
Just an idea, though and not a show-stopper.
Other than that it looks good to me.
Cheers,
Andre.
> + pending = vgic_dist_irq_is_pending(vcpu, vlr.irq);
> } else {
> - vgic_dist_irq_clear_pending(vcpu, vlr.irq);
> - vgic_cpu_irq_clear(vcpu, vlr.irq);
> + if (vgic_dist_irq_get_level(vcpu, vlr.irq)) {
> + vgic_cpu_irq_set(vcpu, vlr.irq);
> + 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.
> */
> + vlr.state = 0;
> + vlr.hwirq = 0;
> + vgic_set_lr(vcpu, lr, vlr);
> +
> vgic_sync_lr_elrsr(vcpu, lr, vlr);
>
> - return level_pending;
> + return pending;
> }
>
> static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> @@ -1400,7 +1406,7 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> vlr.irq - VGIC_NR_PRIVATE_IRQS);
>
> spin_lock(&dist->lock);
> - level_pending |= process_level_irq(vcpu, lr, vlr);
> + level_pending |= process_queued_irq(vcpu, lr, vlr);
> spin_unlock(&dist->lock);
> }
> }
> @@ -1422,7 +1428,7 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> /*
> * Save the physical active state, and reset it to inactive.
> *
> - * Return true if there's a pending level triggered interrupt line to queue.
> + * Return true if there's a pending forwarded interrupt to queue.
> */
> static bool vgic_sync_hwirq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
> {
> @@ -1458,10 +1464,8 @@ static bool vgic_sync_hwirq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
> return false;
> }
>
> - /* Mapped edge-triggered interrupts not yet supported. */
> - WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq));
> spin_lock(&dist->lock);
> - level_pending = process_level_irq(vcpu, lr, vlr);
> + level_pending = process_queued_irq(vcpu, lr, vlr);
> spin_unlock(&dist->lock);
> return level_pending;
> }
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
