On Thu, Jul 10 2014 at 3:39:54 pm BST, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> Writes to GICD_ISPENDRn and GICD_ICPENDRn are currently not handled
> correctly for level-triggered interrupts. The spec states that for
> level-triggered interrupts, writes to the GICD_ISPENDRn activate the
> output of a flip-flop which is in turn or'ed with the actual input
> interrupt signal. Correspondingly, writes to GICD_ICPENDRn simply
> deactivates the output of that flip-flop, but does not (of course) affect
> the external input signal. Reads from GICC_IAR will also deactivate the
> flip-flop output.
>
> This requires us to track the state of the level-input separately from
> the state in the flip-flop. We therefore introduce two new variables on
> the distributor struct to track these two states. Astute readers may
> notice that this is introducing more state than required (because an OR
> of the two states gives you the pending state), but the remaining vgic
> code uses the pending bitmap for optimized operations to figure out, at
> the end of the day, if an interrupt is pending or not on the distributor
> side. Refactoring the code to consider the two state variables all the
> places where we currently access the precomputed pending value, did not
> look pretty.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> ---
> include/kvm/arm_vgic.h | 16 ++++++-
> virt/kvm/arm/vgic.c | 123 ++++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 127 insertions(+), 12 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 7d8e61f..f074539 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -140,9 +140,23 @@ struct vgic_dist {
> /* Interrupt enabled (one bit per IRQ) */
> struct vgic_bitmap irq_enabled;
>
> - /* Interrupt state is pending on the distributor */
> + /* Level-triggered interrupt external input is asserted */
> + struct vgic_bitmap irq_level;
> +
> + /*
> + * Interrupt state is pending on the distributor
> + */
> struct vgic_bitmap irq_pending;
>
> + /*
> + * Tracks writes to GICD_ISPENDRn and GICD_ICPENDRn for level-triggered
> + * interrupts. Essentially holds the state of the flip-flop in
> + * Figure 4-10 on page 4-101 in ARM IHI 0048B.b.
> + * Once set, it is only cleared for level-triggered interrupts on
> + * guest ACKs (when we queue it) or writes to GICD_ICPENDRn.
> + */
> + struct vgic_bitmap irq_soft_pend;
> +
> /* Level-triggered interrupt queued on VCPU interface */
> struct vgic_bitmap irq_queued;
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 1b85f42..7b0ab7f 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -67,6 +67,11 @@
> * - When the interrupt is EOIed, the maintenance interrupt fires,
> * and clears the corresponding bit in irq_queued. This allow the
> * interrupt line to be sampled again.
> + * - Note that level-triggered interrupts can also be set to pending from
> + * writes to GICD_ISPENDRn and lowering the external input line does not
> + * cause the interrupt to become inactive in such a situation.
> + * Conversely, writes to GICD_ICPENDRn do not cause the interrupt to become
> + * inactive as long as the external input line is held high.
> */
>
> #define VGIC_ADDR_UNDEF (-1)
> @@ -217,6 +222,41 @@ static void vgic_irq_clear_queued(struct kvm_vcpu *vcpu, int irq)
> vgic_bitmap_set_irq_val(&dist->irq_queued, vcpu->vcpu_id, irq, 0);
> }
>
> +static int vgic_dist_irq_get_level(struct kvm_vcpu *vcpu, int irq)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + return vgic_bitmap_get_irq_val(&dist->irq_level, vcpu->vcpu_id, irq);
> +}
> +
> +static void vgic_dist_irq_set_level(struct kvm_vcpu *vcpu, int irq)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + vgic_bitmap_set_irq_val(&dist->irq_level, vcpu->vcpu_id, irq, 1);
> +}
> +
> +static void vgic_dist_irq_clear_level(struct kvm_vcpu *vcpu, int irq)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + vgic_bitmap_set_irq_val(&dist->irq_level, vcpu->vcpu_id, irq, 0);
> +}
> +
> +static int vgic_dist_irq_soft_pend(struct kvm_vcpu *vcpu, int irq)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + return vgic_bitmap_get_irq_val(&dist->irq_soft_pend, vcpu->vcpu_id, irq);
> +}
> +
> +static void vgic_dist_irq_clear_soft_pend(struct kvm_vcpu *vcpu, int irq)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + vgic_bitmap_set_irq_val(&dist->irq_soft_pend, vcpu->vcpu_id, irq, 0);
> +}
> +
> static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> @@ -409,11 +449,26 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
> struct kvm_exit_mmio *mmio,
> phys_addr_t offset)
> {
> - u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
> - vcpu->vcpu_id, offset);
> + u32 *reg;
> + u32 level_mask;
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + reg = vgic_bitmap_get_reg(&dist->irq_cfg, vcpu->vcpu_id, offset);
> + level_mask = (~(*reg));
> +
> + /* Mark both level and edge triggered irqs as pending */
> + reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
> vgic_reg_access(mmio, reg, offset,
> ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
> +
> if (mmio->is_write) {
> + /* Set the soft-pending flag only for level-triggered irqs */
> + reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
> + vcpu->vcpu_id, offset);
> + vgic_reg_access(mmio, reg, offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
> + *reg &= level_mask;
I'm a bit confused here. You're doing the MMIO access, and then updating
the register again?
> +
> vgic_update_state(vcpu->kvm);
> return true;
> }
> @@ -425,11 +480,27 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
> struct kvm_exit_mmio *mmio,
> phys_addr_t offset)
> {
> - u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
> - vcpu->vcpu_id, offset);
> + u32 *level_active;
> + u32 *reg;
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> + reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
> vgic_reg_access(mmio, reg, offset,
> ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> if (mmio->is_write) {
> + /* Re-set level triggered level-active interrupts */
> + level_active = vgic_bitmap_get_reg(&dist->irq_level,
> + vcpu->vcpu_id, offset);
> + reg = vgic_bitmap_get_reg(&dist->irq_pending,
> + vcpu->vcpu_id, offset);
> + *reg |= *level_active;
So here we're updating the main pending bitmap with what comes from the
wires...
> + /* Clear soft-pending flags */
> + reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
> + vcpu->vcpu_id, offset);
> + vgic_reg_access(mmio, reg, offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> +
... and then clearing the soft bits only. But we're supposed to have
"pending = wire | soft" at any time, aren't we? Here, I read "pending =
wire", and the soft bits not having influence on pending.
I'm probably missing something obvious...
> vgic_update_state(vcpu->kvm);
> return true;
> }
> @@ -1263,17 +1334,36 @@ 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));
>
> vgic_irq_clear_queued(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);
> +
> /* Any additional pending interrupt? */
> - if (vgic_dist_irq_is_pending(vcpu, vlr.irq)) {
> + if (vgic_dist_irq_get_level(vcpu, vlr.irq)) {
> + /*
> + * XXX: vgic_cpu_irq_set not always be true in
> + * this case?
> + */
> 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);
> }
>
> @@ -1379,17 +1469,19 @@ static void vgic_kick_vcpus(struct kvm *kvm)
> static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
> {
> int edge_triggered = vgic_irq_is_edge(vcpu, irq);
> - int state = vgic_dist_irq_is_pending(vcpu, irq);
>
> /*
> * Only inject an interrupt if:
> * - edge triggered and we have a rising edge
> * - level triggered and we change level
> */
> - if (edge_triggered)
> + if (edge_triggered) {
> + int state = vgic_dist_irq_is_pending(vcpu, irq);
> return level > state;
> - else
> + } else {
> + int state = vgic_dist_irq_get_level(vcpu, irq);
> return level != state;
> + }
> }
>
> static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> @@ -1419,10 +1511,19 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>
> kvm_debug("Inject IRQ%d level %d CPU%d\n", irq_num, level, cpuid);
>
> - if (level)
> + if (level) {
> + if (level_triggered)
> + vgic_dist_irq_set_level(vcpu, irq_num);
> vgic_dist_irq_set_pending(vcpu, irq_num);
> - else
> - vgic_dist_irq_clear_pending(vcpu, irq_num);
> + } else {
> + if (level_triggered) {
> + vgic_dist_irq_clear_level(vcpu, irq_num);
> + if (!vgic_dist_irq_soft_pend(vcpu, irq_num))
> + vgic_dist_irq_clear_pending(vcpu, irq_num);
> + } else {
> + vgic_dist_irq_clear_pending(vcpu, irq_num);
> + }
> + }
>
> enabled = vgic_irq_is_enabled(vcpu, irq_num);
Overall, I find the change a bit confusing. I understand the need to
maintain two different bitmaps for wire and soft, but I don't really get
the logic that you've introduced here... Maybe I should try more coffee
or something? ;-)
M.
--
Jazz is not dead. It just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
