* [PATCH v3 1/3] KVM: arm/arm64: Optimize away redundant LR tracking
2015-10-26 9:00 [PATCH v3 0/3] KVM: arm/arm64: Clean up some obsolete code Pavel Fedin
@ 2015-10-26 9:00 ` Pavel Fedin
2015-10-26 9:00 ` [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr() Pavel Fedin
2015-10-26 9:00 ` [PATCH v3 3/3] KVM: arm/arm64: Refactor vgic_retire_lr() Pavel Fedin
2 siblings, 0 replies; 8+ messages in thread
From: Pavel Fedin @ 2015-10-26 9:00 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara, Christoffer Dall
Currently we use vgic_irq_lr_map in order to track which LRs hold which
IRQs, and lr_used bitmap in order to track which LRs are used or free.
vgic_irq_lr_map is actually used only for piggy-back optimization, and
can be easily replaced by iteration over lr_used. This is good because in
future, when LPI support is introduced, number of IRQs will grow up to at
least 16384, while numbers from 1024 to 8192 are never going to be used.
This would be a huge memory waste.
In its turn, lr_used is also completely redundant since
ae705930fca6322600690df9dc1c7d0516145a93 ("arm/arm64: KVM: Keep elrsr/aisr
in sync with software model"), because together with lr_used we also update
elrsr. This allows to easily replace lr_used with elrsr, inverting all
conditions (because in elrsr '1' means 'free').
Signed-off-by: Pavel Fedin <p.fedin@samsung.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 | 53 ++++++++++++++------------------------------------
4 files changed, 17 insertions(+), 44 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 8065801..3936bf8 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -295,9 +295,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);
@@ -308,9 +305,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 8d7b04d..c0f5d7f 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -158,6 +158,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 7dd5d62..92003cb 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -193,6 +193,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 d4669eb..265a410 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -108,6 +108,7 @@ 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);
+static u64 vgic_get_elrsr(struct kvm_vcpu *vcpu);
static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
int virt_irq);
static int compute_pending_for_cpu(struct kvm_vcpu *vcpu);
@@ -691,9 +692,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);
/*
@@ -1098,7 +1101,6 @@ static inline void vgic_enable(struct kvm_vcpu *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);
/*
@@ -1112,8 +1114,6 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
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);
}
@@ -1128,10 +1128,11 @@ 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);
int lr;
- for_each_set_bit(lr, vgic_cpu->lr_used, vgic->nr_lr) {
+ for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
@@ -1188,8 +1189,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;
+ u64 elrsr = vgic_get_elrsr(vcpu);
+ unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
struct vgic_lr vlr;
int lr;
@@ -1200,28 +1202,22 @@ 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) {
+ for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
vlr = vgic_get_lr(vcpu, lr);
- if (vlr.source == sgi_source_id) {
+ if (vlr.irq == irq && 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;
}
}
/* Try to use another LR for this interrupt */
- lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used,
- vgic->nr_lr);
+ lr = find_first_bit(elrsr_ptr, 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;
@@ -1456,7 +1452,6 @@ static bool vgic_sync_hwirq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
/* Sync back the VGIC state after a guest run */
static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
{
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
u64 elrsr;
unsigned long *elrsr_ptr;
@@ -1469,22 +1464,10 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
/* Deal with HW interrupts, and clear mappings for empty LRs */
for (lr = 0; lr < vgic->nr_lr; lr++) {
- struct vgic_lr vlr;
-
- if (!test_bit(lr, vgic_cpu->lr_used))
- continue;
-
- vlr = vgic_get_lr(vcpu, lr);
- if (vgic_sync_hwirq(vcpu, lr, vlr))
- level_pending = true;
-
- if (!test_bit(lr, elrsr_ptr))
- continue;
-
- clear_bit(lr, vgic_cpu->lr_used);
+ struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
+ level_pending |= vgic_sync_hwirq(vcpu, lr, vlr);
BUG_ON(vlr.irq >= dist->nr_irqs);
- vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
}
/* Check if we still have something up our sleeve... */
@@ -1912,12 +1895,10 @@ 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_destroy_irq_phys_map(vcpu->kvm, &vgic_cpu->irq_phys_map_list);
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)
@@ -1928,18 +1909,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
--
2.4.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()
2015-10-26 9:00 [PATCH v3 0/3] KVM: arm/arm64: Clean up some obsolete code Pavel Fedin
2015-10-26 9:00 ` [PATCH v3 1/3] KVM: arm/arm64: Optimize away redundant LR tracking Pavel Fedin
@ 2015-10-26 9:00 ` Pavel Fedin
2015-10-26 15:22 ` Christoffer Dall
2015-10-26 9:00 ` [PATCH v3 3/3] KVM: arm/arm64: Refactor vgic_retire_lr() Pavel Fedin
2 siblings, 1 reply; 8+ messages in thread
From: Pavel Fedin @ 2015-10-26 9:00 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara
Now we see that vgic_set_lr() and vgic_sync_lr_elrsr() are always used
together. Merge them into one function, saving from second vgic_ops
dereferencing every time.
Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
in vgic_unqueue_irqs(), because all these things are now done by the
following vgic_retire_lr().
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
include/kvm/arm_vgic.h | 1 -
virt/kvm/arm/vgic-v2.c | 5 -----
virt/kvm/arm/vgic-v3.c | 5 -----
virt/kvm/arm/vgic.c | 33 ++++-----------------------------
4 files changed, 4 insertions(+), 40 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 3936bf8..f62addc 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -112,7 +112,6 @@ struct vgic_vmcr {
struct vgic_ops {
struct vgic_lr (*get_lr)(const struct kvm_vcpu *, int);
void (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
- void (*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
u64 (*get_elrsr)(const struct kvm_vcpu *vcpu);
u64 (*get_eisr)(const struct kvm_vcpu *vcpu);
void (*clear_eisr)(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index c0f5d7f..ff02f08 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -79,11 +79,7 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
-}
-static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
- struct vgic_lr lr_desc)
-{
if (!(lr_desc.state & LR_STATE_MASK))
vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
else
@@ -167,7 +163,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
static const struct vgic_ops vgic_v2_ops = {
.get_lr = vgic_v2_get_lr,
.set_lr = vgic_v2_set_lr,
- .sync_lr_elrsr = vgic_v2_sync_lr_elrsr,
.get_elrsr = vgic_v2_get_elrsr,
.get_eisr = vgic_v2_get_eisr,
.clear_eisr = vgic_v2_clear_eisr,
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 92003cb..487d635 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -112,11 +112,7 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
}
vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
-}
-static void vgic_v3_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
- struct vgic_lr lr_desc)
-{
if (!(lr_desc.state & LR_STATE_MASK))
vcpu->arch.vgic_cpu.vgic_v3.vgic_elrsr |= (1U << lr);
else
@@ -212,7 +208,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
static const struct vgic_ops vgic_v3_ops = {
.get_lr = vgic_v3_get_lr,
.set_lr = vgic_v3_set_lr,
- .sync_lr_elrsr = vgic_v3_sync_lr_elrsr,
.get_elrsr = vgic_v3_get_elrsr,
.get_eisr = vgic_v3_get_eisr,
.clear_eisr = vgic_v3_clear_eisr,
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 265a410..43f2564 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -717,28 +717,13 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
* interrupt then move the active state to the
* distributor tracking bit.
*/
- if (lr.state & LR_STATE_ACTIVE) {
+ if (lr.state & LR_STATE_ACTIVE)
vgic_irq_set_active(vcpu, lr.irq);
- lr.state &= ~LR_STATE_ACTIVE;
- }
/*
* Reestablish the pending state on the distributor and the
- * CPU interface. It may have already been pending, but that
- * is fine, then we are only setting a few bits that were
- * already set.
- */
- if (lr.state & LR_STATE_PENDING) {
- vgic_dist_irq_set_pending(vcpu, lr.irq);
- lr.state &= ~LR_STATE_PENDING;
- }
-
- vgic_set_lr(vcpu, i, lr);
-
- /*
- * Mark the LR as free for other use.
+ * CPU interface and mark the LR as free for other use.
*/
- BUG_ON(lr.state & LR_STATE_MASK);
vgic_retire_lr(i, lr.irq, vcpu);
vgic_irq_clear_queued(vcpu, lr.irq);
@@ -1048,12 +1033,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);
@@ -1114,7 +1093,6 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
vlr.state = 0;
vgic_set_lr(vcpu, lr_nr, vlr);
- vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
}
/*
@@ -1179,7 +1157,6 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
}
vgic_set_lr(vcpu, lr_nr, vlr);
- vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
}
/*
@@ -1357,8 +1334,6 @@ static int process_queued_irq(struct kvm_vcpu *vcpu,
vlr.hwirq = 0;
vgic_set_lr(vcpu, lr, vlr);
- vgic_sync_lr_elrsr(vcpu, lr, vlr);
-
return pending;
}
@@ -1459,8 +1434,6 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
bool level_pending;
level_pending = vgic_process_maintenance(vcpu);
- elrsr = vgic_get_elrsr(vcpu);
- elrsr_ptr = u64_to_bitmask(&elrsr);
/* Deal with HW interrupts, and clear mappings for empty LRs */
for (lr = 0; lr < vgic->nr_lr; lr++) {
@@ -1471,6 +1444,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
}
/* Check if we still have something up our sleeve... */
+ elrsr = vgic_get_elrsr(vcpu);
+ elrsr_ptr = u64_to_bitmask(&elrsr);
pending = find_first_zero_bit(elrsr_ptr, vgic->nr_lr);
if (level_pending || pending < vgic->nr_lr)
set_bit(vcpu->vcpu_id, dist->irq_pending_on_cpu);
--
2.4.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()
2015-10-26 9:00 ` [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr() Pavel Fedin
@ 2015-10-26 15:22 ` Christoffer Dall
2015-10-26 15:49 ` Pavel Fedin
0 siblings, 1 reply; 8+ messages in thread
From: Christoffer Dall @ 2015-10-26 15:22 UTC (permalink / raw)
To: Pavel Fedin; +Cc: kvmarm, kvm, Marc Zyngier, Andre Przywara
On Mon, Oct 26, 2015 at 12:00:29PM +0300, Pavel Fedin wrote:
> Now we see that vgic_set_lr() and vgic_sync_lr_elrsr() are always used
> together. Merge them into one function, saving from second vgic_ops
> dereferencing every time.
>
> Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
> in vgic_unqueue_irqs(), because all these things are now done by the
> following vgic_retire_lr().
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
> include/kvm/arm_vgic.h | 1 -
> virt/kvm/arm/vgic-v2.c | 5 -----
> virt/kvm/arm/vgic-v3.c | 5 -----
> virt/kvm/arm/vgic.c | 33 ++++-----------------------------
> 4 files changed, 4 insertions(+), 40 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 3936bf8..f62addc 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -112,7 +112,6 @@ struct vgic_vmcr {
> struct vgic_ops {
> struct vgic_lr (*get_lr)(const struct kvm_vcpu *, int);
> void (*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
> - void (*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
> u64 (*get_elrsr)(const struct kvm_vcpu *vcpu);
> u64 (*get_eisr)(const struct kvm_vcpu *vcpu);
> void (*clear_eisr)(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index c0f5d7f..ff02f08 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -79,11 +79,7 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
> lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
>
> vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
> -}
>
> -static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
> - struct vgic_lr lr_desc)
> -{
> if (!(lr_desc.state & LR_STATE_MASK))
> vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
> else
> @@ -167,7 +163,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
> static const struct vgic_ops vgic_v2_ops = {
> .get_lr = vgic_v2_get_lr,
> .set_lr = vgic_v2_set_lr,
> - .sync_lr_elrsr = vgic_v2_sync_lr_elrsr,
> .get_elrsr = vgic_v2_get_elrsr,
> .get_eisr = vgic_v2_get_eisr,
> .clear_eisr = vgic_v2_clear_eisr,
> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> index 92003cb..487d635 100644
> --- a/virt/kvm/arm/vgic-v3.c
> +++ b/virt/kvm/arm/vgic-v3.c
> @@ -112,11 +112,7 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
> }
>
> vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
> -}
>
> -static void vgic_v3_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
> - struct vgic_lr lr_desc)
> -{
> if (!(lr_desc.state & LR_STATE_MASK))
> vcpu->arch.vgic_cpu.vgic_v3.vgic_elrsr |= (1U << lr);
> else
> @@ -212,7 +208,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
> static const struct vgic_ops vgic_v3_ops = {
> .get_lr = vgic_v3_get_lr,
> .set_lr = vgic_v3_set_lr,
> - .sync_lr_elrsr = vgic_v3_sync_lr_elrsr,
> .get_elrsr = vgic_v3_get_elrsr,
> .get_eisr = vgic_v3_get_eisr,
> .clear_eisr = vgic_v3_clear_eisr,
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 265a410..43f2564 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -717,28 +717,13 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
> * interrupt then move the active state to the
> * distributor tracking bit.
> */
> - if (lr.state & LR_STATE_ACTIVE) {
> + if (lr.state & LR_STATE_ACTIVE)
> vgic_irq_set_active(vcpu, lr.irq);
> - lr.state &= ~LR_STATE_ACTIVE;
> - }
>
> /*
> * Reestablish the pending state on the distributor and the
> - * CPU interface. It may have already been pending, but that
> - * is fine, then we are only setting a few bits that were
> - * already set.
> - */
> - if (lr.state & LR_STATE_PENDING) {
> - vgic_dist_irq_set_pending(vcpu, lr.irq);
this looks wrong: You should still be setting the pending state on the
distributor, perhaps this is an ordering issue with the last patch?
-Christoffer
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()
2015-10-26 15:22 ` Christoffer Dall
@ 2015-10-26 15:49 ` Pavel Fedin
2015-10-26 19:42 ` Christoffer Dall
0 siblings, 1 reply; 8+ messages in thread
From: Pavel Fedin @ 2015-10-26 15:49 UTC (permalink / raw)
To: 'Christoffer Dall'
Cc: kvmarm, kvm, 'Marc Zyngier', 'Andre Przywara'
Hello!
> > /*
> > * Reestablish the pending state on the distributor and the
> > - * CPU interface. It may have already been pending, but that
> > - * is fine, then we are only setting a few bits that were
> > - * already set.
> > - */
> > - if (lr.state & LR_STATE_PENDING) {
> > - vgic_dist_irq_set_pending(vcpu, lr.irq);
>
> this looks wrong: You should still be setting the pending state on the
> distributor, perhaps this is an ordering issue with the last patch?
No, explained in the commit msg:
--- cut ---
Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
in vgic_unqueue_irqs(), because all these things are now done by the
following vgic_retire_lr().
--- cut ---
The last patch touches vgic_irq_clear_queued(). LR_STATE_PENDING check was included in vgic_retire_lr() by
cff9211eb1a1f58ce7f5a2d596b617928fd4be0e ("arm/arm64: KVM: Fix arch timer behavior for disabled interrupts "), so i simply removed
duplicated code.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()
2015-10-26 15:49 ` Pavel Fedin
@ 2015-10-26 19:42 ` Christoffer Dall
2015-10-27 6:56 ` Pavel Fedin
0 siblings, 1 reply; 8+ messages in thread
From: Christoffer Dall @ 2015-10-26 19:42 UTC (permalink / raw)
To: Pavel Fedin; +Cc: kvmarm, kvm, 'Marc Zyngier', 'Andre Przywara'
On Mon, Oct 26, 2015 at 06:49:51PM +0300, Pavel Fedin wrote:
> Hello!
>
> > > /*
> > > * Reestablish the pending state on the distributor and the
> > > - * CPU interface. It may have already been pending, but that
> > > - * is fine, then we are only setting a few bits that were
> > > - * already set.
> > > - */
> > > - if (lr.state & LR_STATE_PENDING) {
> > > - vgic_dist_irq_set_pending(vcpu, lr.irq);
> >
> > this looks wrong: You should still be setting the pending state on the
> > distributor, perhaps this is an ordering issue with the last patch?
>
> No, explained in the commit msg:
> --- cut ---
> Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
> in vgic_unqueue_irqs(), because all these things are now done by the
> following vgic_retire_lr().
> --- cut ---
This does not explain the question I'm raising.
After applying this patch, and before applying your next patch,
unqueueing an IRQ will not restore the pending state on the
distributor, but just throw that piece of state away, which breaks
bisectability and makes it impossible to understand the logic by looking
at this commit in isolation.
Please maintain at least current working semantics on a patch by patch
basis unless it's absolutely impossible to do so.
That's what I meant with an ordering issue with your last patch to which
you so elegantly reply 'No'.
Please rework this.
-Christoffer
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()
2015-10-26 19:42 ` Christoffer Dall
@ 2015-10-27 6:56 ` Pavel Fedin
0 siblings, 0 replies; 8+ messages in thread
From: Pavel Fedin @ 2015-10-27 6:56 UTC (permalink / raw)
To: 'Christoffer Dall'
Cc: kvmarm, kvm, 'Marc Zyngier', 'Andre Przywara'
Hello!
> > --- cut ---
> > Additionally, remove unnecessary vgic_set_lr() and LR_STATE_PENDING check
> > in vgic_unqueue_irqs(), because all these things are now done by the
> > following vgic_retire_lr().
> > --- cut ---
>
> This does not explain the question I'm raising.
>
> After applying this patch, and before applying your next patch,
> unqueueing an IRQ will not restore the pending state on the
> distributor, but just throw that piece of state away
It will restore the state and not throw it away.
I guess i'm just not clear enough and you misunderstand me. This check in vgic_unqueue_irqs() is redundant from the beginning.
Let's look at current vgic_retire_lr():
https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/tree/virt/kvm/arm/vgic.c?h=next#n1099
It already does LR_STATE_PENDING check and pushback by itself, since cff9211eb1a1f58ce7f5a2d596b617928fd4be0e (it's your commit,
BTW), so that this check:
https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/tree/virt/kvm/arm/vgic.c?h=next#n728
is already redundant. So actually this is a separate change, and perhaps it's my fault to squash it in.
> which breaks bisectability and makes it impossible to understand the logic by looking
> at this commit in isolation.
Will this be understood better if i make this particular refactor a separate commit, with better explanations?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 3/3] KVM: arm/arm64: Refactor vgic_retire_lr()
2015-10-26 9:00 [PATCH v3 0/3] KVM: arm/arm64: Clean up some obsolete code Pavel Fedin
2015-10-26 9:00 ` [PATCH v3 1/3] KVM: arm/arm64: Optimize away redundant LR tracking Pavel Fedin
2015-10-26 9:00 ` [PATCH v3 2/3] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr() Pavel Fedin
@ 2015-10-26 9:00 ` Pavel Fedin
2 siblings, 0 replies; 8+ messages in thread
From: Pavel Fedin @ 2015-10-26 9:00 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara, Christoffer Dall
1. Remove unnecessary 'irq' argument, because irq number can be retrieved
from the LR.
2. vgic_retire_lr() is always accompanied by vgic_irq_clear_queued(). Since
it already does more than just clearing the LR, move
vgic_irq_clear_queued() inside of it.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
virt/kvm/arm/vgic.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 43f2564..fe451d4 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -105,7 +105,7 @@
#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 void vgic_retire_lr(int lr_nr, 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);
static u64 vgic_get_elrsr(struct kvm_vcpu *vcpu);
@@ -724,8 +724,7 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
* Reestablish the pending state on the distributor and the
* CPU interface and mark the LR as free for other use.
*/
- vgic_retire_lr(i, lr.irq, vcpu);
- vgic_irq_clear_queued(vcpu, lr.irq);
+ vgic_retire_lr(i, vcpu);
/* Finally update the VGIC state. */
vgic_update_state(vcpu->kvm);
@@ -1078,16 +1077,18 @@ 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)
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
{
struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
+ vgic_irq_clear_queued(vcpu, vlr.irq);
+
/*
* We must transfer the pending state back to the distributor before
* retiring the LR, otherwise we may loose edge-triggered interrupts.
*/
if (vlr.state & LR_STATE_PENDING) {
- vgic_dist_irq_set_pending(vcpu, irq);
+ vgic_dist_irq_set_pending(vcpu, vlr.irq);
vlr.hwirq = 0;
}
@@ -1113,11 +1114,8 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
struct vgic_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);
- }
+ if (!vgic_irq_is_enabled(vcpu, vlr.irq))
+ vgic_retire_lr(lr, vcpu);
}
}
--
2.4.4
^ permalink raw reply related [flat|nested] 8+ messages in thread