On Thu, Aug 14 2014 at 1:18:44 pm BST, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Hi Christoffer,
>
> On Thu, Jul 10 2014 at 3:39:52 pm BST, Christoffer Dall
> <christoffer.dall@xxxxxxxxxx> wrote:
>> We have a special bitmap on the distributor struct to keep track of when
>> level-triggered interrupts are queued on the list registers. This was
>> named irq_active, which is confusing, because the active state of an
>> interrupt as per the GIC spec is a different thing, not specifically
>> related to edge-triggered/level-triggered configurations but rather
>> indicates an interrupt which has been ack'ed but not yet eoi'ed.
>>
>> Rename the bitmap and the corresponding accessor functions to irq_queued
>> to clarify what this is actually used for.
>>
>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>
> So to illustrate what I was going on about the first time you summitted
> this patch, have a look at my below take on this. It is basically yours,
> but just with the bitmap named "irq_can_sample", which is exactly what
> this bitmap is about.
>
> What do you think?
>
> Thanks,
>
> M.
>
> From b6f864841878a5509e7d03581a224e270b25dd02 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@xxxxxxx>
> Date: Thu, 14 Aug 2014 13:14:34 +0100
> Subject: [PATCH] KVM: arm: vgic: rename irq_active to irq_can sample
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> include/kvm/arm_vgic.h | 4 ++--
> virt/kvm/arm/vgic.c | 35 +++++++++++++++++++----------------
> 2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 35b0c12..385d771 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -143,8 +143,8 @@ struct vgic_dist {
> /* Interrupt 'pin' level */
> struct vgic_bitmap irq_state;
>
> - /* Level-triggered interrupt in progress */
> - struct vgic_bitmap irq_active;
> + /* Can we sample the pending bit to inject an interrupt? */
> + struct vgic_bitmap irq_can_sample;
>
> /* Interrupt priority. Not used yet. */
> struct vgic_bytemap irq_priority;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 73eba79..1dedf03 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -60,12 +60,12 @@
> * the 'line' again. This is achieved as such:
> *
> * - When a level interrupt is moved onto a vcpu, the corresponding
> - * bit in irq_active is set. As long as this bit is set, the line
> + * bit in irq_can_sample is cleared. As long as this bit is 0, the line
> * will be ignored for further interrupts. The interrupt is injected
> * into the vcpu with the GICH_LR_EOI bit set (generate a
> * maintenance interrupt on EOI).
> * - When the interrupt is EOIed, the maintenance interrupt fires,
> - * and clears the corresponding bit in irq_active. This allow the
> + * and sets the corresponding bit in irq_can_sample. This allow the
> * interrupt line to be sampled again.
> */
>
> @@ -196,25 +196,25 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
> return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
> }
>
> -static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
> +static int vgic_irq_can_sample(struct kvm_vcpu *vcpu, int irq)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>
> - return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
> + return vgic_bitmap_get_irq_val(&dist->irq_can_sample, vcpu->vcpu_id, irq);
> }
>
> -static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_irq_allow_sample(struct kvm_vcpu *vcpu, int irq)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>
> - vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
> + vgic_bitmap_set_irq_val(&dist->irq_can_sample, vcpu->vcpu_id, irq, 1);
> }
>
> -static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_irq_forbid_sample(struct kvm_vcpu *vcpu, int irq)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>
> - vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
> + vgic_bitmap_set_irq_val(&dist->irq_can_sample, vcpu->vcpu_id, irq, 0);
> }
>
> static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
> @@ -1079,8 +1079,8 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
>
> if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
> vgic_retire_lr(lr, vlr.irq, vcpu);
> - if (vgic_irq_is_active(vcpu, vlr.irq))
> - vgic_irq_clear_active(vcpu, vlr.irq);
> + if (!vgic_irq_can_sample(vcpu, vlr.irq))
> + vgic_irq_allow_sample(vcpu, vlr.irq);
> }
> }
> }
> @@ -1170,7 +1170,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>
> static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
> {
> - if (vgic_irq_is_active(vcpu, irq))
> + if (!vgic_irq_can_sample(vcpu, irq))
> return true; /* level interrupt, already queued */
>
> if (vgic_queue_irq(vcpu, 0, irq)) {
> @@ -1178,7 +1178,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
> vgic_dist_irq_clear(vcpu, irq);
> vgic_cpu_irq_clear(vcpu, irq);
> } else {
> - vgic_irq_set_active(vcpu, irq);
> + vgic_irq_forbid_sample(vcpu, irq);
> }
>
> return true;
> @@ -1252,8 +1252,8 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>
> if (status & INT_STATUS_EOI) {
> /*
> - * Some level interrupts have been EOIed. Clear their
> - * active bit.
> + * Some level interrupts have been EOIed. Allow them
> + * to be resampled.
> */
> u64 eisr = vgic_get_eisr(vcpu);
> unsigned long *eisr_ptr = (unsigned long *)&eisr;
> @@ -1262,7 +1262,7 @@ 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);
>
> - vgic_irq_clear_active(vcpu, vlr.irq);
> + vgic_irq_allow_sample(vcpu, vlr.irq);
> WARN_ON(vlr.state & LR_STATE_MASK);
> vlr.state = 0;
> vgic_set_lr(vcpu, lr, vlr);
> @@ -1429,7 +1429,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
> goto out;
> }
>
> - if (is_level && vgic_irq_is_active(vcpu, irq_num)) {
> + if (is_level && !vgic_irq_can_sample(vcpu, irq_num)) {
> /*
> * Level interrupt in progress, will be picked up
> * when EOId.
> @@ -1506,6 +1506,9 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> if (i < VGIC_NR_PRIVATE_IRQS)
> vgic_bitmap_set_irq_val(&dist->irq_cfg,
> vcpu->vcpu_id, i, VGIC_CFG_EDGE);
> + /* Let vcpu0 also allow sampling of SPIs */
> + if (i < VGIC_NR_PRIVATE_IRQS || vcpu->vcpu_id == 0)
> + vgic_irq_allow_sample(vcpu, i);
>
> vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY;
> }
> --
> 2.0.0
The astute reader will have noticed that this change allows for further
cleanups, such as this:
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 1dedf03..327588d 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1395,15 +1395,12 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
{
struct vgic_dist *dist = &kvm->arch.vgic;
struct kvm_vcpu *vcpu;
- int is_edge, is_level;
int enabled;
bool ret = true;
spin_lock(&dist->lock);
vcpu = kvm_get_vcpu(kvm, cpuid);
- is_edge = vgic_irq_is_edge(vcpu, irq_num);
- is_level = !is_edge;
if (!vgic_validate_injection(vcpu, irq_num, level)) {
ret = false;
@@ -1429,7 +1426,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
goto out;
}
- if (is_level && !vgic_irq_can_sample(vcpu, irq_num)) {
+ if (!vgic_irq_can_sample(vcpu, irq_num)) {
/*
* Level interrupt in progress, will be picked up
* when EOId.
M.
--
Jazz is not dead. It just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
