Hi Andre,
On 07/10/2015 04:21 PM, Andre Przywara wrote:
> Currently we track which IRQ has been mapped to which VGIC list
> register and also have to synchronize both. We used to do this
> to hold some extra state (for instance the active bit).
> It turns out that this extra state in the LRs is no longer needed and
> this extra tracking causes some pain later.
> Remove the tracking feature (lr_map and lr_used) and get rid of
> quite some code on the way.
> On a guest exit we pick up all still pending IRQs from the LRs and put
> them back in the distributor. We don't care about active-only IRQs,
> so we keep them in the LRs. They will be retired either by our
> vgic_process_maintenance() routine or by the GIC hardware in case of
> edge triggered interrupts.
> In places where we scan LRs we now use our shadow copy of the ELRSR
> register directly.
> This code change means we lose the "piggy-back" optimization, which
> would re-use an active-only LR to inject the pending state on top of
> it. Tracing with various workloads shows that this actually occurred
> very rarely, the ballpark figure is about once every 10,000 exits
> in a disk I/O heavy workload. Also the list registers don't seem to
> as scarce as assumed, with all 4 LRs on the popular implementations
> used less than once every 100,000 exits.
>
> This has been briefly tested on Midway, Juno and the model (the latter
> both with GICv2 and GICv3 guests).
>
> Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
> ---
> include/kvm/arm_vgic.h | 6 ---
> virt/kvm/arm/vgic-v2.c | 1 +
> virt/kvm/arm/vgic-v3.c | 1 +
> virt/kvm/arm/vgic.c | 143
> ++++++++++++++++++++++---------------------------
> 4 files changed, 66 insertions(+), 85 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 133ea00..2ccfa9a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -279,9 +279,6 @@ struct vgic_v3_cpu_if {
> };
>
> struct vgic_cpu {
> - /* per IRQ to LR mapping */
> - u8 *vgic_irq_lr_map;
> -
> /* Pending/active/both interrupts on this VCPU */
> DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS);
> DECLARE_BITMAP( active_percpu, VGIC_NR_PRIVATE_IRQS);
> @@ -292,9 +289,6 @@ struct vgic_cpu {
> unsigned long *active_shared;
> unsigned long *pend_act_shared;
>
> - /* Bitmap of used/free list registers */
> - DECLARE_BITMAP( lr_used, VGIC_V2_MAX_LRS);
> -
> /* Number of list registers on this CPU */
> int nr_lr;
>
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index f9b9c7c..f723710 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -144,6 +144,7 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
> * anyway.
> */
> vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = 0;
> + vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr = ~0;
>
> /* Get the show on the road... */
> vcpu->arch.vgic_cpu.vgic_v2.vgic_hcr = GICH_HCR_EN;
> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> index dff0602..21e5d28 100644
> --- a/virt/kvm/arm/vgic-v3.c
> +++ b/virt/kvm/arm/vgic-v3.c
> @@ -178,6 +178,7 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
> * anyway.
> */
> vgic_v3->vgic_vmcr = 0;
> + vgic_v3->vgic_elrsr = ~0;
>
> /*
> * If we are emulating a GICv3, we do it in an non-GICv2-compatible
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index bc40137..394622c 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -79,7 +79,6 @@
> #include "vgic.h"
>
> static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
> -static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
> static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
> static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr, struct vgic_lr
> lr_desc);
>
> @@ -647,6 +646,17 @@ bool vgic_handle_cfg_reg(u32 *reg, struct kvm_exit_mmio
> *mmio,
> return false;
> }
>
> +static void vgic_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
> + struct vgic_lr vlr)
> +{
> + vgic_ops->sync_lr_elrsr(vcpu, lr, vlr);
> +}
why not renaming this into vgic_set_elrsr. This would be homogeneous
with other virtual interface control register setters?
> +
> +static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
> +{
> + return vgic_ops->get_elrsr(vcpu);
> +}
If I am not wrong, each time you manipulate the elrsr you handle the
bitmap. why not directly returning an unsigned long * then (elrsr_ptr)?
> +
> /**
> * vgic_unqueue_irqs - move pending/active IRQs from LRs to the distributor
> * @vgic_cpu: Pointer to the vgic_cpu struct holding the LRs
> @@ -658,9 +668,11 @@ bool vgic_handle_cfg_reg(u32 *reg, struct kvm_exit_mmio
> *mmio,
> void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
> {
> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> + u64 elrsr = vgic_get_elrsr(vcpu);
> + unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
> int i;
>
> - for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
> + for_each_clear_bit(i, elrsr_ptr, vgic_cpu->nr_lr) {
> struct vgic_lr lr = vgic_get_lr(vcpu, i);
>
> /*
> @@ -703,7 +715,7 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
> * Mark the LR as free for other use.
> */
> BUG_ON(lr.state & LR_STATE_MASK);
> - vgic_retire_lr(i, lr.irq, vcpu);
> + vgic_sync_lr_elrsr(vcpu, i, lr);
> vgic_irq_clear_queued(vcpu, lr.irq);
>
> /* Finally update the VGIC state. */
> @@ -1011,17 +1023,6 @@ static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
> vgic_ops->set_lr(vcpu, lr, vlr);
> }
>
> -static void vgic_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
> - struct vgic_lr vlr)
> -{
> - vgic_ops->sync_lr_elrsr(vcpu, lr, vlr);
> -}
> -
> -static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
> -{
> - return vgic_ops->get_elrsr(vcpu);
> -}
> -
> static inline u64 vgic_get_eisr(struct kvm_vcpu *vcpu)
> {
> return vgic_ops->get_eisr(vcpu);
> @@ -1062,18 +1063,6 @@ static inline void vgic_enable(struct kvm_vcpu *vcpu)
> vgic_ops->enable(vcpu);
> }
>
> -static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
> -{
> - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> - struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
> -
> - vlr.state = 0;
> - vgic_set_lr(vcpu, lr_nr, vlr);
> - clear_bit(lr_nr, vgic_cpu->lr_used);
> - vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
> - vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
> -}
> -
> /*
> * An interrupt may have been disabled after being made pending on the
> * CPU interface (the classic case is a timer running while we're
> @@ -1085,23 +1074,32 @@ static void vgic_retire_lr(int lr_nr, int irq, struct
> kvm_vcpu *vcpu)
> */
> static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
> {
> - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> + u64 elrsr = vgic_get_elrsr(vcpu);
> + unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
if you agree with above modif I would simply rename elrsr_ptr into elrsr.
> int lr;
> + struct vgic_lr vlr;
why moving this declaration here. I think this can remain in the block.
>
> - for_each_set_bit(lr, vgic_cpu->lr_used, vgic->nr_lr) {
> - struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
> + for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
> + vlr = vgic_get_lr(vcpu, lr);
>
> if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
> - vgic_retire_lr(lr, vlr.irq, vcpu);
> - if (vgic_irq_is_queued(vcpu, vlr.irq))
> - vgic_irq_clear_queued(vcpu, vlr.irq);
> + vlr.state = 0;
> + vgic_set_lr(vcpu, lr, vlr);
> + vgic_sync_lr_elrsr(vcpu, lr, vlr);
vgic_set_elrsr?
the "if (vgic_irq_is_queued(vcpu, vlr.irq))" check disappeared. This
change might be introduced separately.
> + vgic_irq_clear_queued(vcpu, vlr.irq);
> }
> }
> }
>
> static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
> - int lr_nr, struct vgic_lr vlr)
I don't think this change of proto is compatible with Marc's "KVM:
arm/arm64: vgic: Allow HW interrupts to be queued to a guest".
I think we need to keep former signature.
> + int lr_nr, int sgi_source_id)
> {
> + struct vgic_lr vlr;
> +
> + vlr.state = 0;
> + vlr.irq = irq;
> + vlr.source = sgi_source_id;
> +
> if (vgic_irq_is_active(vcpu, irq)) {
> vlr.state |= LR_STATE_ACTIVE;
> kvm_debug("Set active, clear distributor: 0x%x\n", vlr.state);
> @@ -1126,9 +1124,9 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu,
> int irq,
> */
> bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
> {
> - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> - struct vgic_lr vlr;
> + u64 elrsr = vgic_get_elrsr(vcpu);
> + unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
> int lr;
>
> /* Sanitize the input... */
> @@ -1138,42 +1136,20 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8
> sgi_source_id, int irq)
>
> kvm_debug("Queue IRQ%d\n", irq);
>
> - lr = vgic_cpu->vgic_irq_lr_map[irq];
> -
> - /* Do we have an active interrupt for the same CPUID? */
> - if (lr != LR_EMPTY) {
> - vlr = vgic_get_lr(vcpu, lr);
> - if (vlr.source == sgi_source_id) {
> - kvm_debug("LR%d piggyback for IRQ%d\n", lr, vlr.irq);
> - BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
> - vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
> - return true;
> - }
> - }
> + lr = find_first_bit(elrsr_ptr, vgic->nr_lr);
>
> - /* Try to use another LR for this interrupt */
> - lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used,
> - vgic->nr_lr);
> if (lr >= vgic->nr_lr)
> return false;
>
> kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
> - vgic_cpu->vgic_irq_lr_map[irq] = lr;
> - set_bit(lr, vgic_cpu->lr_used);
>
> - vlr.irq = irq;
> - vlr.source = sgi_source_id;
> - vlr.state = 0;
> - vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
> + vgic_queue_irq_to_lr(vcpu, irq, lr, sgi_source_id);
>
> return true;
> }
>
> static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
> {
> - if (!vgic_can_sample_irq(vcpu, irq))
> - return true; /* level interrupt, already queued */
> -
I think that change needs to be introduced in a separate patch as the
other one mentioned above and justified since it affects the state machine.
Best Regards
Eric
> if (vgic_queue_irq(vcpu, 0, irq)) {
> if (vgic_irq_is_edge(vcpu, irq)) {
> vgic_dist_irq_clear_pending(vcpu, irq);
> @@ -1346,29 +1322,44 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu
> *vcpu)
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> u64 elrsr;
> unsigned long *elrsr_ptr;
> - int lr, pending;
> - bool level_pending;
> + struct vgic_lr vlr;
> + int lr_nr;
> + bool pending;
> +
> + pending = vgic_process_maintenance(vcpu);
>
> - level_pending = vgic_process_maintenance(vcpu);
> elrsr = vgic_get_elrsr(vcpu);
> elrsr_ptr = u64_to_bitmask(&elrsr);
>
> - /* Clear mappings for empty LRs */
> - for_each_set_bit(lr, elrsr_ptr, vgic->nr_lr) {
> - struct vgic_lr vlr;
> + for_each_clear_bit(lr_nr, elrsr_ptr, vgic_cpu->nr_lr) {
> + vlr = vgic_get_lr(vcpu, lr_nr);
> +
> + BUG_ON(!(vlr.state & LR_STATE_MASK));
> + pending = true;
>
> - if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
> + /* Reestablish SGI source for pending and active SGIs */
> + if (vlr.irq < VGIC_NR_SGIS)
> + add_sgi_source(vcpu, vlr.irq, vlr.source);
> +
> + /*
> + * If the LR holds a pure active (10) interrupt then keep it
> + * in the LR without mirroring this status in the emulation.
> + */
> + if (vlr.state == LR_STATE_ACTIVE)
> continue;
>
> - vlr = vgic_get_lr(vcpu, lr);
> + if (vlr.state & LR_STATE_PENDING)
> + vgic_dist_irq_set_pending(vcpu, vlr.irq);
>
> - BUG_ON(vlr.irq >= dist->nr_irqs);
> - vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
> + /* Mark this LR as empty now. */
> + vlr.state = 0;
> + vgic_set_lr(vcpu, lr_nr, vlr);
> + vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
> }
> + vgic_update_state(vcpu->kvm);
>
> - /* Check if we still have something up our sleeve... */
> - pending = find_first_zero_bit(elrsr_ptr, vgic->nr_lr);
> - if (level_pending || pending < vgic->nr_lr)
> + /* vgic_update_state would not cover only-active IRQs */
> + if (pending)
> set_bit(vcpu->vcpu_id, dist->irq_pending_on_cpu);
> }
>
> @@ -1590,11 +1581,9 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
> kfree(vgic_cpu->pending_shared);
> kfree(vgic_cpu->active_shared);
> kfree(vgic_cpu->pend_act_shared);
> - kfree(vgic_cpu->vgic_irq_lr_map);
> vgic_cpu->pending_shared = NULL;
> vgic_cpu->active_shared = NULL;
> vgic_cpu->pend_act_shared = NULL;
> - vgic_cpu->vgic_irq_lr_map = NULL;
> }
>
> static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
> @@ -1605,18 +1594,14 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu,
> int nr_irqs)
> vgic_cpu->pending_shared = kzalloc(sz, GFP_KERNEL);
> vgic_cpu->active_shared = kzalloc(sz, GFP_KERNEL);
> vgic_cpu->pend_act_shared = kzalloc(sz, GFP_KERNEL);
> - vgic_cpu->vgic_irq_lr_map = kmalloc(nr_irqs, GFP_KERNEL);
>
> if (!vgic_cpu->pending_shared
> || !vgic_cpu->active_shared
> - || !vgic_cpu->pend_act_shared
> - || !vgic_cpu->vgic_irq_lr_map) {
> + || !vgic_cpu->pend_act_shared) {
> kvm_vgic_vcpu_destroy(vcpu);
> return -ENOMEM;
> }
>
> - memset(vgic_cpu->vgic_irq_lr_map, LR_EMPTY, nr_irqs);
> -
> /*
> * Store the number of LRs per vcpu, so we don't have to go
> * all the way to the distributor structure to find out. Only
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
