Re: Re: [PATCH v3 1/3] KVM: arm/arm64: vgic: Make vgic_irq->irq_lock a raw_spinlock
On Fri, Feb 01, 2019 at 03:30:58PM +, Julien Grall wrote:
> Hi Julien,
>
> On 07/01/2019 15:06, Julien Thierry wrote:
> > vgic_irq->irq_lock must always be taken with interrupts disabled as
> > it is used in interrupt context.
>
> I am a bit confused with the reason here. The code mention that ap_list_lock
> could be taken from the timer interrupt handler interrupt. I assume it
> speaks about the handler kvm_arch_timer_handler. Looking at the
> configuration of the interrupt, the flag IRQF_NO_THREAD is not set, so the
> interrupt should be threaded when CONFIG_PREEMPT_FULL is set. If my
> understanding is correct, this means the interrupt thread would sleep if it
> takes the spinlock.
>
> Did I miss anything? Do you have an exact path where the vGIC is actually
> called from an interrupt context?
The part you're missing is that percpu interrupts are not force
threaded:
static int irq_setup_forced_threading(struct irqaction *new)
{
if (!force_irqthreads)
return 0;
if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
return 0;
/* ...*/
}
Julia
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 1/3] KVM: arm/arm64: vgic: Make vgic_irq->irq_lock a raw_spinlock
Hi Julia,
On 01/02/2019 17:36, Julia Cartwright wrote:
On Fri, Feb 01, 2019 at 03:30:58PM +, Julien Grall wrote:
Hi Julien,
On 07/01/2019 15:06, Julien Thierry wrote:
vgic_irq->irq_lock must always be taken with interrupts disabled as
it is used in interrupt context.
I am a bit confused with the reason here. The code mention that ap_list_lock
could be taken from the timer interrupt handler interrupt. I assume it
speaks about the handler kvm_arch_timer_handler. Looking at the
configuration of the interrupt, the flag IRQF_NO_THREAD is not set, so the
interrupt should be threaded when CONFIG_PREEMPT_FULL is set. If my
understanding is correct, this means the interrupt thread would sleep if it
takes the spinlock.
Did I miss anything? Do you have an exact path where the vGIC is actually
called from an interrupt context?
The part you're missing is that percpu interrupts are not force
threaded:
static int irq_setup_forced_threading(struct irqaction *new)
{
if (!force_irqthreads)
return 0;
if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
return 0;
/* ...*/
}
Thank you for the pointer! I think it would be worth mentioning in the commit
message that per-cpu interrupts are not threaded.
Best regards,
--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 1/3] KVM: arm/arm64: vgic: Make vgic_irq->irq_lock a raw_spinlock
Hi Julien,
On 07/01/2019 15:06, Julien Thierry wrote:
vgic_irq->irq_lock must always be taken with interrupts disabled as
it is used in interrupt context.
I am a bit confused with the reason here. The code mention that ap_list_lock
could be taken from the timer interrupt handler interrupt. I assume it speaks
about the handler kvm_arch_timer_handler. Looking at the configuration of the
interrupt, the flag IRQF_NO_THREAD is not set, so the interrupt should be
threaded when CONFIG_PREEMPT_FULL is set. If my understanding is correct, this
means the interrupt thread would sleep if it takes the spinlock.
Did I miss anything? Do you have an exact path where the vGIC is actually called
from an interrupt context?
However, those functions can be called from section with hardirq disabled (see
kvm_vgic_sync_hwstate). So I can see a reason to use raw_spin_lock here and the
rest of the series.
Cheers,
For configurations such as PREEMPT_RT_FULL, this means that it should
be a raw_spinlock since RT spinlocks are interruptible.
Signed-off-by: Julien Thierry
Acked-by: Christoffer Dall
Cc: Christoffer Dall
Cc: Marc Zyngier
---
include/kvm/arm_vgic.h | 2 +-
virt/kvm/arm/vgic/vgic-debug.c | 4 +--
virt/kvm/arm/vgic/vgic-init.c| 4 +--
virt/kvm/arm/vgic/vgic-its.c | 14
virt/kvm/arm/vgic/vgic-mmio-v2.c | 14
virt/kvm/arm/vgic/vgic-mmio-v3.c | 12 +++
virt/kvm/arm/vgic/vgic-mmio.c| 34 +--
virt/kvm/arm/vgic/vgic-v2.c | 4 +--
virt/kvm/arm/vgic/vgic-v3.c | 8 ++---
virt/kvm/arm/vgic/vgic.c | 71
10 files changed, 83 insertions(+), 84 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4f31f96..b542605 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -100,7 +100,7 @@ enum vgic_irq_config {
};
struct vgic_irq {
- spinlock_t irq_lock;/* Protects the content of the struct */
+ raw_spinlock_t irq_lock;/* Protects the content of the struct */
struct list_head lpi_list; /* Used to link all LPIs together */
struct list_head ap_list;
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index 07aa900..1f62f2b 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -251,9 +251,9 @@ static int vgic_debug_show(struct seq_file *s, void *v)
return 0;
}
- spin_lock_irqsave(>irq_lock, flags);
+ raw_spin_lock_irqsave(>irq_lock, flags);
print_irq_state(s, irq, vcpu);
- spin_unlock_irqrestore(>irq_lock, flags);
+ raw_spin_unlock_irqrestore(>irq_lock, flags);
vgic_put_irq(kvm, irq);
return 0;
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index c0c0b88..1128e97 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -171,7 +171,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int
nr_spis)
irq->intid = i + VGIC_NR_PRIVATE_IRQS;
INIT_LIST_HEAD(>ap_list);
- spin_lock_init(>irq_lock);
+ raw_spin_lock_init(>irq_lock);
irq->vcpu = NULL;
irq->target_vcpu = vcpu0;
kref_init(>refcount);
@@ -216,7 +216,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
struct vgic_irq *irq = _cpu->private_irqs[i];
INIT_LIST_HEAD(>ap_list);
- spin_lock_init(>irq_lock);
+ raw_spin_lock_init(>irq_lock);
irq->intid = i;
irq->vcpu = NULL;
irq->target_vcpu = vcpu;
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index eb2a390..911ba61 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -65,7 +65,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32
intid,
INIT_LIST_HEAD(>lpi_list);
INIT_LIST_HEAD(>ap_list);
- spin_lock_init(>irq_lock);
+ raw_spin_lock_init(>irq_lock);
irq->config = VGIC_CONFIG_EDGE;
kref_init(>refcount);
@@ -287,7 +287,7 @@ static int update_lpi_config(struct kvm *kvm, struct
vgic_irq *irq,
if (ret)
return ret;
- spin_lock_irqsave(>irq_lock, flags);
+ raw_spin_lock_irqsave(>irq_lock, flags);
if (!filter_vcpu || filter_vcpu == irq->target_vcpu) {
irq->priority = LPI_PROP_PRIORITY(prop);
@@ -299,7 +299,7 @@ static int update_lpi_config(struct kvm *kvm, struct
vgic_irq *irq,
}
}
- spin_unlock_irqrestore(>irq_lock, flags);
+ raw_spin_unlock_irqrestore(>irq_lock, flags);
if (irq->hw)
return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
@@ -352,9 +352,9 @@ static int update_affinity(struct vgic_irq *irq, struct
kvm_vcpu *vcpu)
int ret = 0;
unsigned long flags;
-
[PATCH v3 1/3] KVM: arm/arm64: vgic: Make vgic_irq->irq_lock a raw_spinlock
vgic_irq->irq_lock must always be taken with interrupts disabled as
it is used in interrupt context.
For configurations such as PREEMPT_RT_FULL, this means that it should
be a raw_spinlock since RT spinlocks are interruptible.
Signed-off-by: Julien Thierry
Acked-by: Christoffer Dall
Cc: Christoffer Dall
Cc: Marc Zyngier
---
include/kvm/arm_vgic.h | 2 +-
virt/kvm/arm/vgic/vgic-debug.c | 4 +--
virt/kvm/arm/vgic/vgic-init.c| 4 +--
virt/kvm/arm/vgic/vgic-its.c | 14
virt/kvm/arm/vgic/vgic-mmio-v2.c | 14
virt/kvm/arm/vgic/vgic-mmio-v3.c | 12 +++
virt/kvm/arm/vgic/vgic-mmio.c| 34 +--
virt/kvm/arm/vgic/vgic-v2.c | 4 +--
virt/kvm/arm/vgic/vgic-v3.c | 8 ++---
virt/kvm/arm/vgic/vgic.c | 71
10 files changed, 83 insertions(+), 84 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4f31f96..b542605 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -100,7 +100,7 @@ enum vgic_irq_config {
};
struct vgic_irq {
- spinlock_t irq_lock;/* Protects the content of the struct */
+ raw_spinlock_t irq_lock;/* Protects the content of the struct */
struct list_head lpi_list; /* Used to link all LPIs together */
struct list_head ap_list;
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index 07aa900..1f62f2b 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -251,9 +251,9 @@ static int vgic_debug_show(struct seq_file *s, void *v)
return 0;
}
- spin_lock_irqsave(>irq_lock, flags);
+ raw_spin_lock_irqsave(>irq_lock, flags);
print_irq_state(s, irq, vcpu);
- spin_unlock_irqrestore(>irq_lock, flags);
+ raw_spin_unlock_irqrestore(>irq_lock, flags);
vgic_put_irq(kvm, irq);
return 0;
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index c0c0b88..1128e97 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -171,7 +171,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int
nr_spis)
irq->intid = i + VGIC_NR_PRIVATE_IRQS;
INIT_LIST_HEAD(>ap_list);
- spin_lock_init(>irq_lock);
+ raw_spin_lock_init(>irq_lock);
irq->vcpu = NULL;
irq->target_vcpu = vcpu0;
kref_init(>refcount);
@@ -216,7 +216,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
struct vgic_irq *irq = _cpu->private_irqs[i];
INIT_LIST_HEAD(>ap_list);
- spin_lock_init(>irq_lock);
+ raw_spin_lock_init(>irq_lock);
irq->intid = i;
irq->vcpu = NULL;
irq->target_vcpu = vcpu;
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index eb2a390..911ba61 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -65,7 +65,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32
intid,
INIT_LIST_HEAD(>lpi_list);
INIT_LIST_HEAD(>ap_list);
- spin_lock_init(>irq_lock);
+ raw_spin_lock_init(>irq_lock);
irq->config = VGIC_CONFIG_EDGE;
kref_init(>refcount);
@@ -287,7 +287,7 @@ static int update_lpi_config(struct kvm *kvm, struct
vgic_irq *irq,
if (ret)
return ret;
- spin_lock_irqsave(>irq_lock, flags);
+ raw_spin_lock_irqsave(>irq_lock, flags);
if (!filter_vcpu || filter_vcpu == irq->target_vcpu) {
irq->priority = LPI_PROP_PRIORITY(prop);
@@ -299,7 +299,7 @@ static int update_lpi_config(struct kvm *kvm, struct
vgic_irq *irq,
}
}
- spin_unlock_irqrestore(>irq_lock, flags);
+ raw_spin_unlock_irqrestore(>irq_lock, flags);
if (irq->hw)
return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
@@ -352,9 +352,9 @@ static int update_affinity(struct vgic_irq *irq, struct
kvm_vcpu *vcpu)
int ret = 0;
unsigned long flags;
- spin_lock_irqsave(>irq_lock, flags);
+ raw_spin_lock_irqsave(>irq_lock, flags);
irq->target_vcpu = vcpu;
- spin_unlock_irqrestore(>irq_lock, flags);
+ raw_spin_unlock_irqrestore(>irq_lock, flags);
if (irq->hw) {
struct its_vlpi_map map;
@@ -455,7 +455,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
}
irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
- spin_lock_irqsave(>irq_lock, flags);
+ raw_spin_lock_irqsave(>irq_lock, flags);
irq->pending_latch = pendmask & (1U << bit_nr);
vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
vgic_put_irq(vcpu->kvm, irq);
@@ -612,7 +612,7 @@ static int vgic_its_trigger_msi(struct
