* [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices
@ 2015-07-08 17:56 Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 01/10] arm/arm64: KVM: Fix ordering of timer/GIC on guest entry Marc Zyngier
` (9 more replies)
0 siblings, 10 replies; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
>From day 1, our timer code has been using a terrible hack: whenever
the guest is scheduled with a timer interrupt pending (i.e. the HW
timer has expired), we restore the timer state with the MASK bit set,
in order to avoid the physical interrupt to fire again. And again. And
again...
This is absolutely silly, for at least two reasons:
- This relies on the device (the timer) having a mask bit that we can
play with. Not all devices are built like this.
- This expects some behaviour of the guest that only works because the
both the kernel timer code and the KVM counterpart have been written
by the same idiot (the idiot being me).
The One True Way is to set the GIC active bit when injecting the
interrupt, and to context-switch across the world switch. This is what
this series implements.
We introduce a relatively simple infrastructure enabling the mapping
of a virtual interrupt with its physical counterpart:
- Whenever an virtual interrupt is injected, we look it up in an
rbtree. If we have a match, the interrupt is injected with the HW
bit set in the LR, together with the physical interrupt.
- Across the world switch, we save/restore the active state for these
interrupts using the irqchip_state API.
- On guest EOI, the HW interrupt is automagically deactivated by the
GIC, allowing the interrupt to be resampled.
The timer code is slightly modified to set the active state at the
same time as the injection.
The last patch also allows non-shared devices to have their interrupt
deactivated the same way (in this case we do not context-switch the
active state). This is the first step in the long overdue direction of
the mythical IRQ forwarding thing...
This series is based on v4.2-rc1, and has been tested on Juno (GICv2)
and the FVP Base model (GICv3 host, both GICv2 and GICv3 guests). I'd
appreciate any form of testing, specially in the context of guest
migration (there is obviously some interesting stuff there...).
The code is otherwise available at
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/active-timer
* From v1:
- Rebased on top of current mainline
- Fixed non-shared handling of forwarded interrupts
- Fixed memory leaks on VM exit
- Used RCU lists instead of an RB tree
Marc Zyngier (10):
arm/arm64: KVM: Fix ordering of timer/GIC on guest entry
arm/arm64: KVM: Move vgic handling to a non-preemptible section
KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields
KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR
KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs
KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual
interrupts
KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest
KVM: arm/arm64: vgic: Add vgic_{get,set}_phys_irq_active
KVM: arm/arm64: timer: Allow the timer to control the active state
KVM: arm/arm64: vgic: Allow non-shared device HW interrupts
arch/arm/kvm/arm.c | 21 ++-
include/kvm/arm_arch_timer.h | 3 +
include/kvm/arm_vgic.h | 38 +++++-
include/linux/irqchip/arm-gic-v3.h | 3 +
include/linux/irqchip/arm-gic.h | 3 +-
virt/kvm/arm/arch_timer.c | 13 +-
virt/kvm/arm/vgic-v2.c | 16 ++-
virt/kvm/arm/vgic-v3.c | 21 ++-
virt/kvm/arm/vgic.c | 264 ++++++++++++++++++++++++++++++++++++-
9 files changed, 363 insertions(+), 19 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 01/10] arm/arm64: KVM: Fix ordering of timer/GIC on guest entry
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 02/10] arm/arm64: KVM: Move vgic handling to a non-preemptible section Marc Zyngier
` (8 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
As we now inject the timer interrupt when we're about to enter
the guest, it makes a lot more sense to make sure this happens
before the vgic code queues the pending interrupts.
Otherwise, we get the interrupt on the following exit, which is
not great for latency (and leads to all kind of bizarre issues
when using with active interrupts at the HW level).
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
---
arch/arm/kvm/arm.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bc738d2..d605180 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -528,8 +528,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
if (vcpu->arch.pause)
vcpu_pause(vcpu);
- kvm_vgic_flush_hwstate(vcpu);
kvm_timer_flush_hwstate(vcpu);
+ kvm_vgic_flush_hwstate(vcpu);
preempt_disable();
local_irq_disable();
@@ -545,8 +545,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
local_irq_enable();
preempt_enable();
- kvm_timer_sync_hwstate(vcpu);
kvm_vgic_sync_hwstate(vcpu);
+ kvm_timer_sync_hwstate(vcpu);
continue;
}
@@ -588,9 +588,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
trace_kvm_exit(kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
preempt_enable();
-
- kvm_timer_sync_hwstate(vcpu);
kvm_vgic_sync_hwstate(vcpu);
+ kvm_timer_sync_hwstate(vcpu);
ret = handle_exit(vcpu, run, ret);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 02/10] arm/arm64: KVM: Move vgic handling to a non-preemptible section
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 01/10] arm/arm64: KVM: Fix ordering of timer/GIC on guest entry Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields Marc Zyngier
` (7 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
As we're about to introduce some serious GIC-poking to the vgic code,
it is important to make sure that we're going to poke the part of
the GIC that belongs to the CPU we're about to run on (otherwise,
we'd end up with some unexpected interrupts firing)...
Introducing a non-preemptible section in kvm_arch_vcpu_ioctl_run
prevents the problem from occuring.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
---
arch/arm/kvm/arm.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index d605180..1583a34 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -528,10 +528,20 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
if (vcpu->arch.pause)
vcpu_pause(vcpu);
+ /*
+ * Disarming the background timer must be done in a
+ * preemptible context, as this call may sleep.
+ */
kvm_timer_flush_hwstate(vcpu);
- kvm_vgic_flush_hwstate(vcpu);
+ /*
+ * Preparing the interrupts to be injected also
+ * involves poking the GIC, which must be done in a
+ * non-preemptible context.
+ */
preempt_disable();
+ kvm_vgic_flush_hwstate(vcpu);
+
local_irq_disable();
/*
@@ -544,8 +554,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
local_irq_enable();
- preempt_enable();
kvm_vgic_sync_hwstate(vcpu);
+ preempt_enable();
kvm_timer_sync_hwstate(vcpu);
continue;
}
@@ -586,9 +596,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
*/
kvm_guest_exit();
trace_kvm_exit(kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
- preempt_enable();
kvm_vgic_sync_hwstate(vcpu);
+
+ preempt_enable();
+
kvm_timer_sync_hwstate(vcpu);
ret = handle_exit(vcpu, run, ret);
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 01/10] arm/arm64: KVM: Fix ordering of timer/GIC on guest entry Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 02/10] arm/arm64: KVM: Move vgic handling to a non-preemptible section Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR Marc Zyngier
` (6 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
As we're about to cram more information in the vgic_lr structure
(HW interrupt number and additional state information), we switch
to a layout similar to the HW's:
- use bitfields to save space (we don't need more than 10 bits
to represent the irq numbers)
- source CPU and HW interrupt can share the same field, as
a SGI doesn't have a physical line.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
---
include/kvm/arm_vgic.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 133ea00..4f9fa1d 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -95,11 +95,15 @@ enum vgic_type {
#define LR_STATE_ACTIVE (1 << 1)
#define LR_STATE_MASK (3 << 0)
#define LR_EOI_INT (1 << 2)
+#define LR_HW (1 << 3)
struct vgic_lr {
- u16 irq;
- u8 source;
- u8 state;
+ unsigned irq:10;
+ union {
+ unsigned hwirq:10;
+ unsigned source:8;
+ };
+ unsigned state:4;
};
struct vgic_vmcr {
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (2 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 19:50 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 05/10] KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs Marc Zyngier
` (5 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
Now that struct vgic_lr supports the LR_HW bit and carries a hwirq
field, we can encode that information into the list registers.
This patch provides implementations for both GICv2 and GICv3.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
include/linux/irqchip/arm-gic-v3.h | 3 +++
include/linux/irqchip/arm-gic.h | 3 ++-
virt/kvm/arm/vgic-v2.c | 16 +++++++++++++++-
virt/kvm/arm/vgic-v3.c | 21 ++++++++++++++++++---
4 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index ffbc034..cf637d6 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -268,9 +268,12 @@
#define ICH_LR_EOI (1UL << 41)
#define ICH_LR_GROUP (1UL << 60)
+#define ICH_LR_HW (1UL << 61)
#define ICH_LR_STATE (3UL << 62)
#define ICH_LR_PENDING_BIT (1UL << 62)
#define ICH_LR_ACTIVE_BIT (1UL << 63)
+#define ICH_LR_PHYS_ID_SHIFT 32
+#define ICH_LR_PHYS_ID_MASK (0x3ffUL << ICH_LR_PHYS_ID_SHIFT)
#define ICH_MISR_EOI (1 << 0)
#define ICH_MISR_U (1 << 1)
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 9de976b..ca88dad 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -71,11 +71,12 @@
#define GICH_LR_VIRTUALID (0x3ff << 0)
#define GICH_LR_PHYSID_CPUID_SHIFT (10)
-#define GICH_LR_PHYSID_CPUID (7 << GICH_LR_PHYSID_CPUID_SHIFT)
+#define GICH_LR_PHYSID_CPUID (0x3ff << GICH_LR_PHYSID_CPUID_SHIFT)
#define GICH_LR_STATE (3 << 28)
#define GICH_LR_PENDING_BIT (1 << 28)
#define GICH_LR_ACTIVE_BIT (1 << 29)
#define GICH_LR_EOI (1 << 19)
+#define GICH_LR_HW (1 << 31)
#define GICH_VMCR_CTRL_SHIFT 0
#define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT)
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index f9b9c7c..8d7b04d 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -48,6 +48,10 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
lr_desc.state |= LR_STATE_ACTIVE;
if (val & GICH_LR_EOI)
lr_desc.state |= LR_EOI_INT;
+ if (val & GICH_LR_HW) {
+ lr_desc.state |= LR_HW;
+ lr_desc.hwirq = (val & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT;
+ }
return lr_desc;
}
@@ -55,7 +59,9 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
struct vgic_lr lr_desc)
{
- u32 lr_val = (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT) | lr_desc.irq;
+ u32 lr_val;
+
+ lr_val = lr_desc.irq;
if (lr_desc.state & LR_STATE_PENDING)
lr_val |= GICH_LR_PENDING_BIT;
@@ -64,6 +70,14 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
if (lr_desc.state & LR_EOI_INT)
lr_val |= GICH_LR_EOI;
+ if (lr_desc.state & LR_HW) {
+ lr_val |= GICH_LR_HW;
+ lr_val |= (u32)lr_desc.hwirq << GICH_LR_PHYSID_CPUID_SHIFT;
+ }
+
+ if (lr_desc.irq < VGIC_NR_SGIS)
+ lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
+
vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
}
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index dff0602..afbf925 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -67,6 +67,10 @@ static struct vgic_lr vgic_v3_get_lr(const struct kvm_vcpu *vcpu, int lr)
lr_desc.state |= LR_STATE_ACTIVE;
if (val & ICH_LR_EOI)
lr_desc.state |= LR_EOI_INT;
+ if (val & ICH_LR_HW) {
+ lr_desc.state |= LR_HW;
+ lr_desc.hwirq = (val >> ICH_LR_PHYS_ID_SHIFT) & GENMASK(9, 0);
+ }
return lr_desc;
}
@@ -84,10 +88,17 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
* Eventually we want to make this configurable, so we may revisit
* this in the future.
*/
- if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+ switch (vcpu->kvm->arch.vgic.vgic_model) {
+ case KVM_DEV_TYPE_ARM_VGIC_V3:
lr_val |= ICH_LR_GROUP;
- else
- lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
+ break;
+ case KVM_DEV_TYPE_ARM_VGIC_V2:
+ if (lr_desc.irq < VGIC_NR_SGIS)
+ lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
+ break;
+ default:
+ BUG();
+ }
if (lr_desc.state & LR_STATE_PENDING)
lr_val |= ICH_LR_PENDING_BIT;
@@ -95,6 +106,10 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
lr_val |= ICH_LR_ACTIVE_BIT;
if (lr_desc.state & LR_EOI_INT)
lr_val |= ICH_LR_EOI;
+ if (lr_desc.state & LR_HW) {
+ lr_val |= ICH_LR_HW;
+ lr_val |= ((u64)lr_desc.hwirq) << ICH_LR_PHYS_ID_SHIFT;
+ }
vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 05/10] KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (3 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts Marc Zyngier
` (4 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
We only set the irq_queued flag for level interrupts, meaning
that "!vgic_irq_is_queued(vcpu, irq)" is a good enough predicate
for all interrupts.
This will allow us to inject edge HW interrupts, for which the
state ACTIVE+PENDING is not allowed.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
virt/kvm/arm/vgic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index bc40137..5bd1695 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -375,7 +375,7 @@ void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
static bool vgic_can_sample_irq(struct kvm_vcpu *vcpu, int irq)
{
- return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
+ return !vgic_irq_is_queued(vcpu, irq);
}
/**
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (4 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 05/10] KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 21:11 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest Marc Zyngier
` (3 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
In order to be able to feed physical interrupts to a guest, we need
to be able to establish the virtual-physical mapping between the two
worlds.
The mappings are kept in a set of RCU lists, indexed by virtual interrupts.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm/kvm/arm.c | 2 +
include/kvm/arm_vgic.h | 25 +++++++++
virt/kvm/arm/vgic.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 170 insertions(+), 1 deletion(-)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 1583a34..d5ce5cc 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -125,6 +125,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
if (ret)
goto out_free_stage2_pgd;
+ kvm_vgic_init(kvm);
kvm_timer_init(kvm);
/* Mark the initial VMID generation invalid */
@@ -249,6 +250,7 @@ out:
void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
{
+ kvm_vgic_vcpu_postcreate(vcpu);
}
void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4f9fa1d..32e57d2 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -159,6 +159,19 @@ struct vgic_io_device {
struct kvm_io_device dev;
};
+struct irq_phys_map {
+ u32 virt_irq;
+ u32 phys_irq;
+ u32 irq;
+ bool active;
+};
+
+struct irq_phys_map_entry {
+ struct list_head entry;
+ struct rcu_head rcu;
+ struct irq_phys_map map;
+};
+
struct vgic_dist {
spinlock_t lock;
bool in_kernel;
@@ -256,6 +269,10 @@ struct vgic_dist {
struct vgic_vm_ops vm_ops;
struct vgic_io_device dist_iodev;
struct vgic_io_device *redist_iodevs;
+
+ /* Virtual irq to hwirq mapping */
+ spinlock_t irq_phys_map_lock;
+ struct list_head irq_phys_map_list;
};
struct vgic_v2_cpu_if {
@@ -307,6 +324,9 @@ struct vgic_cpu {
struct vgic_v2_cpu_if vgic_v2;
struct vgic_v3_cpu_if vgic_v3;
};
+
+ /* Protected by the distributor's irq_phys_map_lock */
+ struct list_head irq_phys_map_list;
};
#define LR_EMPTY 0xff
@@ -321,8 +341,10 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
int kvm_vgic_hyp_init(void);
int kvm_vgic_map_resources(struct kvm *kvm);
int kvm_vgic_get_max_vcpus(void);
+void kvm_vgic_init(struct kvm *kvm);
int kvm_vgic_create(struct kvm *kvm, u32 type);
void kvm_vgic_destroy(struct kvm *kvm);
+void kvm_vgic_vcpu_postcreate(struct kvm_vcpu *vcpu);
void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
@@ -331,6 +353,9 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
+struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
+ int virt_irq, int irq);
+int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
#define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
#define vgic_initialized(k) (!!((k)->arch.vgic.nr_cpus))
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 5bd1695..3424329 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -24,6 +24,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
+#include <linux/rculist.h>
#include <linux/uaccess.h>
#include <asm/kvm_emulate.h>
@@ -82,6 +83,8 @@ 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 struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
+ int virt_irq);
static const struct vgic_ops *vgic_ops;
static const struct vgic_params *vgic;
@@ -1583,6 +1586,131 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
+ int virt_irq)
+{
+ if (virt_irq < VGIC_NR_PRIVATE_IRQS)
+ return &vcpu->arch.vgic_cpu.irq_phys_map_list;
+ else
+ return &vcpu->kvm->arch.vgic.irq_phys_map_list;
+}
+
+struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
+ int virt_irq, int irq)
+{
+ struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+ struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
+ struct irq_phys_map *map;
+ struct irq_phys_map_entry *entry;
+ struct irq_desc *desc;
+ struct irq_data *data;
+ int phys_irq;
+
+ desc = irq_to_desc(irq);
+ if (!desc) {
+ kvm_err("kvm_arch_timer: can't obtain interrupt descriptor\n");
+ return NULL;
+ }
+
+ data = irq_desc_get_irq_data(desc);
+ while (data->parent_data)
+ data = data->parent_data;
+
+ phys_irq = data->hwirq;
+
+ spin_lock(&dist->irq_phys_map_lock);
+
+ /* Try to match an existing mapping */
+ map = vgic_irq_map_search(vcpu, virt_irq);
+ if (map) {
+ /* Make sure this mapping matches */
+ if (map->phys_irq != phys_irq ||
+ map->irq != irq)
+ map = NULL;
+
+ goto out;
+ }
+
+ /* Create a new mapping */
+ entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
+ if (!entry)
+ goto out;
+
+ map = &entry->map;
+ map->virt_irq = virt_irq;
+ map->phys_irq = phys_irq;
+ map->irq = irq;
+
+ list_add_tail_rcu(&entry->entry, root);
+
+out:
+ spin_unlock(&dist->irq_phys_map_lock);
+ return map;
+}
+
+static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
+ int virt_irq)
+{
+ struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
+ struct irq_phys_map_entry *entry;
+ struct irq_phys_map *map;
+
+ rcu_read_lock();
+
+ list_for_each_entry_rcu(entry, root, entry) {
+ map = &entry->map;
+ if (map->virt_irq == virt_irq) {
+ rcu_read_unlock();
+ return map;
+ }
+ }
+
+ rcu_read_unlock();
+
+ return NULL;
+}
+
+static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
+{
+ struct irq_phys_map_entry *entry;
+
+ entry = container_of(rcu, struct irq_phys_map_entry, rcu);
+ kfree(entry);
+}
+
+int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
+{
+ struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+ struct irq_phys_map_entry *entry;
+
+ if (!map)
+ return -EINVAL;
+
+ entry = container_of(map, struct irq_phys_map_entry, map);
+
+ spin_lock(&dist->irq_phys_map_lock);
+ list_del_rcu(&entry->entry);
+ call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
+ spin_unlock(&dist->irq_phys_map_lock);
+
+ return 0;
+}
+
+static void vgic_destroy_irq_phys_map(struct kvm *kvm, struct list_head *root)
+{
+ struct vgic_dist *dist = &kvm->arch.vgic;
+ struct irq_phys_map_entry *entry;
+
+ spin_lock(&dist->irq_phys_map_lock);
+
+ list_for_each_entry(entry, root, entry) {
+ list_del_rcu(&entry->entry);
+ call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
+ }
+
+ spin_unlock(&dist->irq_phys_map_lock);
+}
+
void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
{
struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
@@ -1591,6 +1719,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
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;
@@ -1627,6 +1756,12 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
return 0;
}
+void kvm_vgic_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+ struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+ INIT_LIST_HEAD(&vgic_cpu->irq_phys_map_list);
+}
+
/**
* kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
*
@@ -1664,6 +1799,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
kfree(dist->irq_spi_target);
kfree(dist->irq_pending_on_cpu);
kfree(dist->irq_active_on_cpu);
+ vgic_destroy_irq_phys_map(kvm, &dist->irq_phys_map_list);
dist->irq_sgi_sources = NULL;
dist->irq_spi_cpu = NULL;
dist->irq_spi_target = NULL;
@@ -1787,6 +1923,13 @@ static int init_vgic_model(struct kvm *kvm, int type)
return 0;
}
+void kvm_vgic_init(struct kvm *kvm)
+{
+ spin_lock_init(&kvm->arch.vgic.lock);
+ spin_lock_init(&kvm->arch.vgic.irq_phys_map_lock);
+ INIT_LIST_HEAD(&kvm->arch.vgic.irq_phys_map_list);
+}
+
int kvm_vgic_create(struct kvm *kvm, u32 type)
{
int i, vcpu_lock_idx = -1, ret;
@@ -1832,7 +1975,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
if (ret)
goto out_unlock;
- spin_lock_init(&kvm->arch.vgic.lock);
kvm->arch.vgic.in_kernel = true;
kvm->arch.vgic.vgic_model = type;
kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (5 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 21:56 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get, set}_phys_irq_active Marc Zyngier
` (2 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
To allow a HW interrupt to be injected into a guest, we lookup the
guest virtual interrupt in the irq_phys_map rbtree, and if we have
a match, encode both interrupts in the LR.
We also mark the interrupt as "active" at the host distributor level.
On guest EOI on the virtual interrupt, the host interrupt will be
deactivated.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
virt/kvm/arm/vgic.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 69 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 3424329..f8582d7 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1118,6 +1118,26 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
if (!vgic_irq_is_edge(vcpu, irq))
vlr.state |= LR_EOI_INT;
+ if (vlr.irq >= VGIC_NR_SGIS) {
+ struct irq_phys_map *map;
+ map = vgic_irq_map_search(vcpu, irq);
+
+ if (map) {
+ int ret;
+
+ BUG_ON(!map->active);
+ vlr.hwirq = map->phys_irq;
+ vlr.state |= LR_HW;
+ vlr.state &= ~LR_EOI_INT;
+
+ ret = irq_set_irqchip_state(map->irq,
+ IRQCHIP_STATE_ACTIVE,
+ true);
+ vgic_irq_set_queued(vcpu, irq);
+ WARN_ON(ret);
+ }
+ }
+
vgic_set_lr(vcpu, lr_nr, vlr);
vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
}
@@ -1342,6 +1362,35 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
return level_pending;
}
+/* Return 1 if HW interrupt went from active to inactive, and 0 otherwise */
+static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
+{
+ struct irq_phys_map *map;
+ int ret;
+
+ if (!(vlr.state & LR_HW))
+ return 0;
+
+ map = vgic_irq_map_search(vcpu, vlr.irq);
+ BUG_ON(!map || !map->active);
+
+ ret = irq_get_irqchip_state(map->irq,
+ IRQCHIP_STATE_ACTIVE,
+ &map->active);
+
+ WARN_ON(ret);
+
+ if (map->active) {
+ ret = irq_set_irqchip_state(map->irq,
+ IRQCHIP_STATE_ACTIVE,
+ false);
+ WARN_ON(ret);
+ return 0;
+ }
+
+ return 1;
+}
+
/* Sync back the VGIC state after a guest run */
static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
{
@@ -1356,14 +1405,31 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *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) {
+ /* Deal with HW interrupts, and clear mappings for empty LRs */
+ for (lr = 0; lr < vgic->nr_lr; lr++) {
struct vgic_lr vlr;
- if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
+ if (!test_bit(lr, vgic_cpu->lr_used))
continue;
vlr = vgic_get_lr(vcpu, lr);
+ if (vgic_sync_hwirq(vcpu, vlr)) {
+ /*
+ * So this is a HW interrupt that the guest
+ * EOI-ed. Clean the LR state and allow the
+ * interrupt to be queued again.
+ */
+ vlr.state = 0;
+ vlr.hwirq = 0;
+ vgic_set_lr(vcpu, lr, vlr);
+ vgic_irq_clear_queued(vcpu, vlr.irq);
+ set_bit(lr, elrsr_ptr);
+ }
+
+ if (!test_bit(lr, elrsr_ptr))
+ continue;
+
+ clear_bit(lr, vgic_cpu->lr_used);
BUG_ON(vlr.irq >= dist->nr_irqs);
vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get, set}_phys_irq_active
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (6 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 22:15 ` [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get,set}_phys_irq_active Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 09/10] KVM: arm/arm64: timer: Allow the timer to control the active state Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts Marc Zyngier
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
In order to control the active state of an interrupt, introduce
a pair of accessors allowing the state to be set/queried.
This only affects the logical state, and the HW state will only be
applied at world-switch time.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
include/kvm/arm_vgic.h | 2 ++
virt/kvm/arm/vgic.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 32e57d2..9fd4023 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -356,6 +356,8 @@ int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
int virt_irq, int irq);
int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
+bool vgic_get_phys_irq_active(struct irq_phys_map *map);
+void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active);
#define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
#define vgic_initialized(k) (!!((k)->arch.vgic.nr_cpus))
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index f8582d7..39f9479 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1744,6 +1744,18 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
kfree(entry);
}
+bool vgic_get_phys_irq_active(struct irq_phys_map *map)
+{
+ BUG_ON(!map);
+ return map->active;
+}
+
+void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active)
+{
+ BUG_ON(!map);
+ map->active = active;
+}
+
int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
{
struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 09/10] KVM: arm/arm64: timer: Allow the timer to control the active state
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (7 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get, set}_phys_irq_active Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts Marc Zyngier
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
In order to remove the crude hack where we sneak the masked bit
into the timer's control register, make use of the phys_irq_map
API control the active state of the interrupt.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
include/kvm/arm_arch_timer.h | 3 +++
virt/kvm/arm/arch_timer.c | 13 +++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index e596675..9feebf1 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -52,6 +52,9 @@ struct arch_timer_cpu {
/* Timer IRQ */
const struct kvm_irq_level *irq;
+
+ /* VGIC mapping */
+ struct irq_phys_map *map;
};
int kvm_timer_hyp_init(void);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 98c95f2..b9fff78 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -64,7 +64,7 @@ static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu)
int ret;
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
- timer->cntv_ctl |= ARCH_TIMER_CTRL_IT_MASK;
+ vgic_set_phys_irq_active(timer->map, true);
ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
timer->irq->irq,
timer->irq->level);
@@ -117,7 +117,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
cycle_t cval, now;
if ((timer->cntv_ctl & ARCH_TIMER_CTRL_IT_MASK) ||
- !(timer->cntv_ctl & ARCH_TIMER_CTRL_ENABLE))
+ !(timer->cntv_ctl & ARCH_TIMER_CTRL_ENABLE) ||
+ vgic_get_phys_irq_active(timer->map))
return false;
cval = timer->cntv_cval;
@@ -196,6 +197,13 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
* vcpu timer irq number when the vcpu is reset.
*/
timer->irq = irq;
+
+ /*
+ * Tell the VGIC that the virtual interrupt is tied to a
+ * physical interrupt. We do that once per VCPU.
+ */
+ timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
+ WARN_ON(!timer->map);
}
void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
@@ -335,6 +343,7 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
timer_disarm(timer);
+ vgic_unmap_phys_irq(vcpu, timer->map);
}
void kvm_timer_enable(struct kvm *kvm)
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
` (8 preceding siblings ...)
2015-07-08 17:56 ` [PATCH v2 09/10] KVM: arm/arm64: timer: Allow the timer to control the active state Marc Zyngier
@ 2015-07-08 17:56 ` Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
9 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-08 17:56 UTC (permalink / raw
To: linux-arm-kernel
So far, the only use of the HW interrupt facility is the timer,
implying that the active state is context-switched for each vcpu,
as the device is is shared across all vcpus.
This does not work for a device that has been assigned to a VM,
as the guest is entierely in control of that device (the HW is
not shared). In that case, it makes sense to bypass the whole
active state switchint, and only track the deactivation of the
interrupt.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
include/kvm/arm_vgic.h | 5 ++--
virt/kvm/arm/arch_timer.c | 2 +-
virt/kvm/arm/vgic.c | 62 ++++++++++++++++++++++++++++++++++++-----------
3 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 9fd4023..31c987a 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -163,7 +163,8 @@ struct irq_phys_map {
u32 virt_irq;
u32 phys_irq;
u32 irq;
- bool active;
+ bool shared;
+ bool active; /* Only valid if shared */
};
struct irq_phys_map_entry {
@@ -354,7 +355,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
- int virt_irq, int irq);
+ int virt_irq, int irq, bool shared);
int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
bool vgic_get_phys_irq_active(struct irq_phys_map *map);
void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index b9fff78..9544d79 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -202,7 +202,7 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
* Tell the VGIC that the virtual interrupt is tied to a
* physical interrupt. We do that once per VCPU.
*/
- timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
+ timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq, true);
WARN_ON(!timer->map);
}
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 39f9479..3585bb0 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1123,18 +1123,21 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
map = vgic_irq_map_search(vcpu, irq);
if (map) {
- int ret;
-
- BUG_ON(!map->active);
vlr.hwirq = map->phys_irq;
vlr.state |= LR_HW;
vlr.state &= ~LR_EOI_INT;
- ret = irq_set_irqchip_state(map->irq,
- IRQCHIP_STATE_ACTIVE,
- true);
vgic_irq_set_queued(vcpu, irq);
- WARN_ON(ret);
+
+ if (map->shared) {
+ int ret;
+
+ BUG_ON(!map->active);
+ ret = irq_set_irqchip_state(map->irq,
+ IRQCHIP_STATE_ACTIVE,
+ true);
+ WARN_ON(ret);
+ }
}
}
@@ -1366,21 +1369,37 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
{
struct irq_phys_map *map;
+ bool active;
int ret;
if (!(vlr.state & LR_HW))
return 0;
map = vgic_irq_map_search(vcpu, vlr.irq);
- BUG_ON(!map || !map->active);
+ BUG_ON(!map);
+ BUG_ON(map->shared && !map->active);
ret = irq_get_irqchip_state(map->irq,
IRQCHIP_STATE_ACTIVE,
- &map->active);
+ &active);
WARN_ON(ret);
- if (map->active) {
+ /*
+ * For a non-shared interrupt, we have to catter for two
+ * possible deactivation conditions
+ *
+ * - the interrupt is now inactive
+ * - the interrupt is still active, but is flagged as not
+ * queued, indicating another interrupt has fired before we
+ * could observe the deactivate.
+ */
+ if (!map->shared)
+ return !active || !vgic_irq_is_queued(vcpu, vlr.irq);
+
+ map->active = active;
+
+ if (active) {
ret = irq_set_irqchip_state(map->irq,
IRQCHIP_STATE_ACTIVE,
false);
@@ -1523,6 +1542,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
int edge_triggered, level_triggered;
int enabled;
bool ret = true, can_inject = true;
+ struct irq_phys_map *map;
spin_lock(&dist->lock);
@@ -1569,6 +1589,18 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
goto out;
}
+ map = vgic_irq_map_search(vcpu, irq_num);
+ if (map && !map->shared) {
+ /*
+ * We are told to inject a HW irq, so we have to trust
+ * the caller that the previous one has been EOIed,
+ * and that a new one is now active. Clearing the
+ * queued state will have the effect of making it
+ * sample-able again.
+ */
+ vgic_irq_clear_queued(vcpu, irq_num);
+ }
+
if (!vgic_can_sample_irq(vcpu, irq_num)) {
/*
* Level interrupt in progress, will be picked up
@@ -1662,7 +1694,7 @@ static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
}
struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
- int virt_irq, int irq)
+ int virt_irq, int irq, bool shared)
{
struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
@@ -1691,7 +1723,8 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
if (map) {
/* Make sure this mapping matches */
if (map->phys_irq != phys_irq ||
- map->irq != irq)
+ map->irq != irq ||
+ map->shared != shared)
map = NULL;
goto out;
@@ -1706,6 +1739,7 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
map->virt_irq = virt_irq;
map->phys_irq = phys_irq;
map->irq = irq;
+ map->shared = shared;
list_add_tail_rcu(&entry->entry, root);
@@ -1746,13 +1780,13 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
bool vgic_get_phys_irq_active(struct irq_phys_map *map)
{
- BUG_ON(!map);
+ BUG_ON(!map || !map->shared);
return map->active;
}
void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active)
{
- BUG_ON(!map);
+ BUG_ON(!map || !map->shared);
map->active = active;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR
2015-07-08 17:56 ` [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR Marc Zyngier
@ 2015-07-17 19:50 ` Christoffer Dall
2015-07-21 16:38 ` Marc Zyngier
0 siblings, 1 reply; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 19:50 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:36PM +0100, Marc Zyngier wrote:
> Now that struct vgic_lr supports the LR_HW bit and carries a hwirq
> field, we can encode that information into the list registers.
>
> This patch provides implementations for both GICv2 and GICv3.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> include/linux/irqchip/arm-gic-v3.h | 3 +++
> include/linux/irqchip/arm-gic.h | 3 ++-
> virt/kvm/arm/vgic-v2.c | 16 +++++++++++++++-
> virt/kvm/arm/vgic-v3.c | 21 ++++++++++++++++++---
> 4 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index ffbc034..cf637d6 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -268,9 +268,12 @@
>
> #define ICH_LR_EOI (1UL << 41)
> #define ICH_LR_GROUP (1UL << 60)
> +#define ICH_LR_HW (1UL << 61)
> #define ICH_LR_STATE (3UL << 62)
> #define ICH_LR_PENDING_BIT (1UL << 62)
> #define ICH_LR_ACTIVE_BIT (1UL << 63)
> +#define ICH_LR_PHYS_ID_SHIFT 32
> +#define ICH_LR_PHYS_ID_MASK (0x3ffUL << ICH_LR_PHYS_ID_SHIFT)
>
> #define ICH_MISR_EOI (1 << 0)
> #define ICH_MISR_U (1 << 1)
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 9de976b..ca88dad 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -71,11 +71,12 @@
>
> #define GICH_LR_VIRTUALID (0x3ff << 0)
> #define GICH_LR_PHYSID_CPUID_SHIFT (10)
> -#define GICH_LR_PHYSID_CPUID (7 << GICH_LR_PHYSID_CPUID_SHIFT)
> +#define GICH_LR_PHYSID_CPUID (0x3ff << GICH_LR_PHYSID_CPUID_SHIFT)
> #define GICH_LR_STATE (3 << 28)
> #define GICH_LR_PENDING_BIT (1 << 28)
> #define GICH_LR_ACTIVE_BIT (1 << 29)
> #define GICH_LR_EOI (1 << 19)
> +#define GICH_LR_HW (1 << 31)
>
> #define GICH_VMCR_CTRL_SHIFT 0
> #define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT)
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index f9b9c7c..8d7b04d 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -48,6 +48,10 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
> lr_desc.state |= LR_STATE_ACTIVE;
> if (val & GICH_LR_EOI)
> lr_desc.state |= LR_EOI_INT;
> + if (val & GICH_LR_HW) {
> + lr_desc.state |= LR_HW;
> + lr_desc.hwirq = (val & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT;
> + }
>
> return lr_desc;
> }
> @@ -55,7 +59,9 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
> static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
> struct vgic_lr lr_desc)
> {
> - u32 lr_val = (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT) | lr_desc.irq;
> + u32 lr_val;
> +
> + lr_val = lr_desc.irq;
>
> if (lr_desc.state & LR_STATE_PENDING)
> lr_val |= GICH_LR_PENDING_BIT;
> @@ -64,6 +70,14 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
> if (lr_desc.state & LR_EOI_INT)
> lr_val |= GICH_LR_EOI;
>
> + if (lr_desc.state & LR_HW) {
> + lr_val |= GICH_LR_HW;
> + lr_val |= (u32)lr_desc.hwirq << GICH_LR_PHYSID_CPUID_SHIFT;
> + }
> +
> + if (lr_desc.irq < VGIC_NR_SGIS)
> + lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
> +
> vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
> }
>
> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> index dff0602..afbf925 100644
> --- a/virt/kvm/arm/vgic-v3.c
> +++ b/virt/kvm/arm/vgic-v3.c
> @@ -67,6 +67,10 @@ static struct vgic_lr vgic_v3_get_lr(const struct kvm_vcpu *vcpu, int lr)
> lr_desc.state |= LR_STATE_ACTIVE;
> if (val & ICH_LR_EOI)
> lr_desc.state |= LR_EOI_INT;
> + if (val & ICH_LR_HW) {
> + lr_desc.state |= LR_HW;
> + lr_desc.hwirq = (val >> ICH_LR_PHYS_ID_SHIFT) & GENMASK(9, 0);
> + }
>
> return lr_desc;
> }
> @@ -84,10 +88,17 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
> * Eventually we want to make this configurable, so we may revisit
> * this in the future.
> */
> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> + switch (vcpu->kvm->arch.vgic.vgic_model) {
> + case KVM_DEV_TYPE_ARM_VGIC_V3:
> lr_val |= ICH_LR_GROUP;
> - else
> - lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
> + break;
> + case KVM_DEV_TYPE_ARM_VGIC_V2:
> + if (lr_desc.irq < VGIC_NR_SGIS)
> + lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
I forget how this works: Why are we mixing GICH_LR_ stuff with ICH_LR_
in the same function? Aren't we always accessing these registers via
the system registers interface from the hypervisor control point of
view, regardless of which GIC version the guest sees?
I guess my question here is: Why should this not be
ICH_LR_PHYSID_CORE_SHIFT?
Thanks,
-Christoffer
> + break;
> + default:
> + BUG();
> + }
>
> if (lr_desc.state & LR_STATE_PENDING)
> lr_val |= ICH_LR_PENDING_BIT;
> @@ -95,6 +106,10 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
> lr_val |= ICH_LR_ACTIVE_BIT;
> if (lr_desc.state & LR_EOI_INT)
> lr_val |= ICH_LR_EOI;
> + if (lr_desc.state & LR_HW) {
> + lr_val |= ICH_LR_HW;
> + lr_val |= ((u64)lr_desc.hwirq) << ICH_LR_PHYS_ID_SHIFT;
> + }
>
> vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
> }
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts
2015-07-08 17:56 ` [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts Marc Zyngier
@ 2015-07-17 21:11 ` Christoffer Dall
2015-07-21 17:17 ` Marc Zyngier
0 siblings, 1 reply; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 21:11 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:38PM +0100, Marc Zyngier wrote:
> In order to be able to feed physical interrupts to a guest, we need
> to be able to establish the virtual-physical mapping between the two
> worlds.
>
> The mappings are kept in a set of RCU lists, indexed by virtual interrupts.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm/kvm/arm.c | 2 +
> include/kvm/arm_vgic.h | 25 +++++++++
> virt/kvm/arm/vgic.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 1583a34..d5ce5cc 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -125,6 +125,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> if (ret)
> goto out_free_stage2_pgd;
>
> + kvm_vgic_init(kvm);
> kvm_timer_init(kvm);
>
> /* Mark the initial VMID generation invalid */
> @@ -249,6 +250,7 @@ out:
>
> void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
> {
> + kvm_vgic_vcpu_postcreate(vcpu);
> }
>
> void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 4f9fa1d..32e57d2 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -159,6 +159,19 @@ struct vgic_io_device {
> struct kvm_io_device dev;
> };
>
> +struct irq_phys_map {
> + u32 virt_irq;
> + u32 phys_irq;
> + u32 irq;
> + bool active;
> +};
> +
> +struct irq_phys_map_entry {
> + struct list_head entry;
> + struct rcu_head rcu;
> + struct irq_phys_map map;
> +};
> +
> struct vgic_dist {
> spinlock_t lock;
> bool in_kernel;
> @@ -256,6 +269,10 @@ struct vgic_dist {
> struct vgic_vm_ops vm_ops;
> struct vgic_io_device dist_iodev;
> struct vgic_io_device *redist_iodevs;
> +
> + /* Virtual irq to hwirq mapping */
> + spinlock_t irq_phys_map_lock;
> + struct list_head irq_phys_map_list;
> };
>
> struct vgic_v2_cpu_if {
> @@ -307,6 +324,9 @@ struct vgic_cpu {
> struct vgic_v2_cpu_if vgic_v2;
> struct vgic_v3_cpu_if vgic_v3;
> };
> +
> + /* Protected by the distributor's irq_phys_map_lock */
> + struct list_head irq_phys_map_list;
> };
>
> #define LR_EMPTY 0xff
> @@ -321,8 +341,10 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
> int kvm_vgic_hyp_init(void);
> int kvm_vgic_map_resources(struct kvm *kvm);
> int kvm_vgic_get_max_vcpus(void);
> +void kvm_vgic_init(struct kvm *kvm);
> int kvm_vgic_create(struct kvm *kvm, u32 type);
> void kvm_vgic_destroy(struct kvm *kvm);
> +void kvm_vgic_vcpu_postcreate(struct kvm_vcpu *vcpu);
> void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
> void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
> void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
> @@ -331,6 +353,9 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
> +struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> + int virt_irq, int irq);
> +int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
should these functions not have a kvm_ prefix?
>
> #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
> #define vgic_initialized(k) (!!((k)->arch.vgic.nr_cpus))
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 5bd1695..3424329 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -24,6 +24,7 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> +#include <linux/rculist.h>
> #include <linux/uaccess.h>
>
> #include <asm/kvm_emulate.h>
> @@ -82,6 +83,8 @@ 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 struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
> + int virt_irq);
>
> static const struct vgic_ops *vgic_ops;
> static const struct vgic_params *vgic;
> @@ -1583,6 +1586,131 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
> + int virt_irq)
> +{
> + if (virt_irq < VGIC_NR_PRIVATE_IRQS)
> + return &vcpu->arch.vgic_cpu.irq_phys_map_list;
> + else
> + return &vcpu->kvm->arch.vgic.irq_phys_map_list;
> +}
> +
You know what I'm going to ask you for here, but let me help out with
the framwork:
/**
* vgic_map_phys_irq - map a virtual IRQ to a physical IRQ
* @vcpu: The VCPU pointer
* @virt_irq: The virtual irq number
* @irq: The Linux IRQ number
*
*
*/
> +struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> + int virt_irq, int irq)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
> + struct irq_phys_map *map;
> + struct irq_phys_map_entry *entry;
> + struct irq_desc *desc;
> + struct irq_data *data;
> + int phys_irq;
> +
> + desc = irq_to_desc(irq);
> + if (!desc) {
> + kvm_err("kvm_arch_timer: can't obtain interrupt descriptor\n");
arch_timer? this is vgic code, no?
> + return NULL;
> + }
> +
> + data = irq_desc_get_irq_data(desc);
> + while (data->parent_data)
> + data = data->parent_data;
> +
> + phys_irq = data->hwirq;
> +
> + spin_lock(&dist->irq_phys_map_lock);
> +
> + /* Try to match an existing mapping */
> + map = vgic_irq_map_search(vcpu, virt_irq);
> + if (map) {
> + /* Make sure this mapping matches */
> + if (map->phys_irq != phys_irq ||
> + map->irq != irq)
when would this happen? Is this something that should gracefully just
be accepted or is it a bug?
> + map = NULL;
> +
> + goto out;
> + }
> +
> + /* Create a new mapping */
> + entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
is GFP_ATOMIC really warranted here? The situatotion where you have an
existing map is extremely rare, I suppose, and is in fact an error, so
you could just pre-allocate and free on error.
> + if (!entry)
Here you seem to be returning a valid value on an error? Perhaps you
should return ERR_PTR(-ENOMEM) and generally use ERR_PTR/PTR_ERR here.
> + goto out;
> +
> + map = &entry->map;
> + map->virt_irq = virt_irq;
> + map->phys_irq = phys_irq;
> + map->irq = irq;
> +
> + list_add_tail_rcu(&entry->entry, root);
> +
> +out:
> + spin_unlock(&dist->irq_phys_map_lock);
> + return map;
> +}
> +
> +static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
> + int virt_irq)
> +{
> + struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
> + struct irq_phys_map_entry *entry;
> + struct irq_phys_map *map;
> +
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(entry, root, entry) {
> + map = &entry->map;
> + if (map->virt_irq == virt_irq) {
> + rcu_read_unlock();
> + return map;
> + }
> + }
> +
> + rcu_read_unlock();
> +
> + return NULL;
> +}
> +
> +static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
> +{
> + struct irq_phys_map_entry *entry;
> +
> + entry = container_of(rcu, struct irq_phys_map_entry, rcu);
> + kfree(entry);
> +}
> +
> +int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + struct irq_phys_map_entry *entry;
> +
> + if (!map)
> + return -EINVAL;
> +
> + entry = container_of(map, struct irq_phys_map_entry, map);
could this race with vgic_destroy_irq_phys_map, such that
vgic_destroy_irq_phys_map removes the entry from the list while we're
spinning on the lock, and then when we proceed we free the entry twice?
> +
> + spin_lock(&dist->irq_phys_map_lock);
> + list_del_rcu(&entry->entry);
> + call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
> + spin_unlock(&dist->irq_phys_map_lock);
> +
> + return 0;
> +}
> +
> +static void vgic_destroy_irq_phys_map(struct kvm *kvm, struct list_head *root)
> +{
> + struct vgic_dist *dist = &kvm->arch.vgic;
> + struct irq_phys_map_entry *entry;
> +
> + spin_lock(&dist->irq_phys_map_lock);
> +
> + list_for_each_entry(entry, root, entry) {
> + list_del_rcu(&entry->entry);
> + call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
> + }
> +
> + spin_unlock(&dist->irq_phys_map_lock);
> +}
> +
> void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
> {
> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> @@ -1591,6 +1719,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
> 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;
> @@ -1627,6 +1756,12 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
> return 0;
> }
>
> +void kvm_vgic_vcpu_postcreate(struct kvm_vcpu *vcpu)
> +{
> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> + INIT_LIST_HEAD(&vgic_cpu->irq_phys_map_list);
can you do this as part of vgic_init?
> +}
> +
> /**
> * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
> *
> @@ -1664,6 +1799,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
> kfree(dist->irq_spi_target);
> kfree(dist->irq_pending_on_cpu);
> kfree(dist->irq_active_on_cpu);
> + vgic_destroy_irq_phys_map(kvm, &dist->irq_phys_map_list);
> dist->irq_sgi_sources = NULL;
> dist->irq_spi_cpu = NULL;
> dist->irq_spi_target = NULL;
> @@ -1787,6 +1923,13 @@ static int init_vgic_model(struct kvm *kvm, int type)
> return 0;
> }
>
> +void kvm_vgic_init(struct kvm *kvm)
> +{
> + spin_lock_init(&kvm->arch.vgic.lock);
> + spin_lock_init(&kvm->arch.vgic.irq_phys_map_lock);
> + INIT_LIST_HEAD(&kvm->arch.vgic.irq_phys_map_list);
why can we not do this as part of kvm_vgic_create?
at least we need to think about naming here or document clearly what the
various init functions do; it is not clear what the difference between
vgic_init and kvm_vgic_init is...
> +}
> +
> int kvm_vgic_create(struct kvm *kvm, u32 type)
> {
> int i, vcpu_lock_idx = -1, ret;
> @@ -1832,7 +1975,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
> if (ret)
> goto out_unlock;
>
> - spin_lock_init(&kvm->arch.vgic.lock);
> kvm->arch.vgic.in_kernel = true;
> kvm->arch.vgic.vgic_model = type;
> kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest
2015-07-08 17:56 ` [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest Marc Zyngier
@ 2015-07-17 21:56 ` Christoffer Dall
2015-07-21 17:21 ` Marc Zyngier
0 siblings, 1 reply; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 21:56 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:39PM +0100, Marc Zyngier wrote:
> To allow a HW interrupt to be injected into a guest, we lookup the
> guest virtual interrupt in the irq_phys_map rbtree, and if we have
s/rbtree/list/
> a match, encode both interrupts in the LR.
>
> We also mark the interrupt as "active" at the host distributor level.
>
> On guest EOI on the virtual interrupt, the host interrupt will be
> deactivated.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> virt/kvm/arm/vgic.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 3424329..f8582d7 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1118,6 +1118,26 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
> if (!vgic_irq_is_edge(vcpu, irq))
> vlr.state |= LR_EOI_INT;
>
> + if (vlr.irq >= VGIC_NR_SGIS) {
> + struct irq_phys_map *map;
> + map = vgic_irq_map_search(vcpu, irq);
> +
> + if (map) {
> + int ret;
> +
> + BUG_ON(!map->active);
> + vlr.hwirq = map->phys_irq;
> + vlr.state |= LR_HW;
> + vlr.state &= ~LR_EOI_INT;
> +
> + ret = irq_set_irqchip_state(map->irq,
> + IRQCHIP_STATE_ACTIVE,
> + true);
So if we have a mapping, and the virtual interrupt is being injected,
then we must set the state to active in the physical world, because
otherwise the guest will just exit before processing the virtual
interrupt, yes?
I think this deserves a comment here.
> + vgic_irq_set_queued(vcpu, irq);
And we set it queued only to avoid sampling it again and setting
PENDING+ACTIVE, because the hardware doesn't support setting that state
when the HW bit is set, yes?
I think a comment here is warranted too.
> + WARN_ON(ret);
> + }
> + }
> +
> vgic_set_lr(vcpu, lr_nr, vlr);
> vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
> }
> @@ -1342,6 +1362,35 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> return level_pending;
> }
>
> +/* Return 1 if HW interrupt went from active to inactive, and 0 otherwise */
... also clear the active state on the physical distributor so that
shared devices can be used in a different context. (did I get this
right?)
> +static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
> +{
> + struct irq_phys_map *map;
> + int ret;
> +
> + if (!(vlr.state & LR_HW))
> + return 0;
> +
> + map = vgic_irq_map_search(vcpu, vlr.irq);
> + BUG_ON(!map || !map->active);
> +
> + ret = irq_get_irqchip_state(map->irq,
> + IRQCHIP_STATE_ACTIVE,
> + &map->active);
> +
> + WARN_ON(ret);
> +
> + if (map->active) {
> + ret = irq_set_irqchip_state(map->irq,
> + IRQCHIP_STATE_ACTIVE,
> + false);
> + WARN_ON(ret);
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> /* Sync back the VGIC state after a guest run */
> static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> {
> @@ -1356,14 +1405,31 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *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) {
> + /* Deal with HW interrupts, and clear mappings for empty LRs */
> + for (lr = 0; lr < vgic->nr_lr; lr++) {
> struct vgic_lr vlr;
>
> - if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
> + if (!test_bit(lr, vgic_cpu->lr_used))
> continue;
>
> vlr = vgic_get_lr(vcpu, lr);
> + if (vgic_sync_hwirq(vcpu, vlr)) {
> + /*
> + * So this is a HW interrupt that the guest
> + * EOI-ed. Clean the LR state and allow the
> + * interrupt to be queued again.
s/queued/sampled/ ?
> + */
> + vlr.state = 0;
> + vlr.hwirq = 0;
> + vgic_set_lr(vcpu, lr, vlr);
> + vgic_irq_clear_queued(vcpu, vlr.irq);
> + set_bit(lr, elrsr_ptr);
> + }
> +
> + if (!test_bit(lr, elrsr_ptr))
> + continue;
> +
> + clear_bit(lr, vgic_cpu->lr_used);
>
> BUG_ON(vlr.irq >= dist->nr_irqs);
> vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
> --
> 2.1.4
>
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts
2015-07-08 17:56 ` [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts Marc Zyngier
@ 2015-07-17 22:15 ` Christoffer Dall
2015-07-21 18:01 ` Marc Zyngier
0 siblings, 1 reply; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 22:15 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:42PM +0100, Marc Zyngier wrote:
> So far, the only use of the HW interrupt facility is the timer,
> implying that the active state is context-switched for each vcpu,
> as the device is is shared across all vcpus.
>
> This does not work for a device that has been assigned to a VM,
> as the guest is entierely in control of that device (the HW is
> not shared). In that case, it makes sense to bypass the whole
> active state switchint, and only track the deactivation of the
switching
> interrupt.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> include/kvm/arm_vgic.h | 5 ++--
> virt/kvm/arm/arch_timer.c | 2 +-
> virt/kvm/arm/vgic.c | 62 ++++++++++++++++++++++++++++++++++++-----------
> 3 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 9fd4023..31c987a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -163,7 +163,8 @@ struct irq_phys_map {
> u32 virt_irq;
> u32 phys_irq;
> u32 irq;
> - bool active;
> + bool shared;
> + bool active; /* Only valid if shared */
> };
>
> struct irq_phys_map_entry {
> @@ -354,7 +355,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
> struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> - int virt_irq, int irq);
> + int virt_irq, int irq, bool shared);
> int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> bool vgic_get_phys_irq_active(struct irq_phys_map *map);
> void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active);
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index b9fff78..9544d79 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -202,7 +202,7 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> * Tell the VGIC that the virtual interrupt is tied to a
> * physical interrupt. We do that once per VCPU.
> */
> - timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
> + timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq, true);
> WARN_ON(!timer->map);
> }
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 39f9479..3585bb0 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1123,18 +1123,21 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
> map = vgic_irq_map_search(vcpu, irq);
>
> if (map) {
> - int ret;
> -
> - BUG_ON(!map->active);
> vlr.hwirq = map->phys_irq;
> vlr.state |= LR_HW;
> vlr.state &= ~LR_EOI_INT;
>
> - ret = irq_set_irqchip_state(map->irq,
> - IRQCHIP_STATE_ACTIVE,
> - true);
> vgic_irq_set_queued(vcpu, irq);
> - WARN_ON(ret);
> +
> + if (map->shared) {
> + int ret;
> +
> + BUG_ON(!map->active);
> + ret = irq_set_irqchip_state(map->irq,
> + IRQCHIP_STATE_ACTIVE,
> + true);
> + WARN_ON(ret);
do we have any other example of a shared device or is this really simply
because the timer hardware is fscking strangely tied to the gic and is a
total one-off?
In the latter case, would it be cleaner to drop this notion of a shared
device entirely and put all this logic in the arch timer code instead?
> + }
> }
> }
>
> @@ -1366,21 +1369,37 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
> {
> struct irq_phys_map *map;
> + bool active;
> int ret;
>
> if (!(vlr.state & LR_HW))
> return 0;
>
> map = vgic_irq_map_search(vcpu, vlr.irq);
> - BUG_ON(!map || !map->active);
> + BUG_ON(!map);
> + BUG_ON(map->shared && !map->active);
>
> ret = irq_get_irqchip_state(map->irq,
> IRQCHIP_STATE_ACTIVE,
> - &map->active);
> + &active);
>
> WARN_ON(ret);
>
> - if (map->active) {
> + /*
> + * For a non-shared interrupt, we have to catter for two
s/catter/cater/ ?
> + * possible deactivation conditions
conditions:
> + *
> + * - the interrupt is now inactive
> + * - the interrupt is still active, but is flagged as not
> + * queued, indicating another interrupt has fired before we
> + * could observe the deactivate.
are these physical or virtual interrupts we are talking about? I assume
virtual, but it would be good to be specific. It's not like we're going
to remember any of this in a little bit.
> + */
> + if (!map->shared)
> + return !active || !vgic_irq_is_queued(vcpu, vlr.irq);
if the second part of the disjunction returns true, doesn't this mean
we can potentially write the active+pending state in the LR for a HW
interrupt, which is not allowed?
> +
> + map->active = active;
> +
> + if (active) {
should you be doing this for a non-shared interrupt?
> ret = irq_set_irqchip_state(map->irq,
> IRQCHIP_STATE_ACTIVE,
> false);
> @@ -1523,6 +1542,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> int edge_triggered, level_triggered;
> int enabled;
> bool ret = true, can_inject = true;
> + struct irq_phys_map *map;
>
> spin_lock(&dist->lock);
>
> @@ -1569,6 +1589,18 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> goto out;
> }
>
> + map = vgic_irq_map_search(vcpu, irq_num);
> + if (map && !map->shared) {
> + /*
> + * We are told to inject a HW irq, so we have to trust
> + * the caller that the previous one has been EOIed,
> + * and that a new one is now active. Clearing the
> + * queued state will have the effect of making it
> + * sample-able again.
> + */
> + vgic_irq_clear_queued(vcpu, irq_num);
see my question above about active+pending
> + }
> +
> if (!vgic_can_sample_irq(vcpu, irq_num)) {
> /*
> * Level interrupt in progress, will be picked up
> @@ -1662,7 +1694,7 @@ static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
> }
>
> struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> - int virt_irq, int irq)
> + int virt_irq, int irq, bool shared)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
> @@ -1691,7 +1723,8 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> if (map) {
> /* Make sure this mapping matches */
> if (map->phys_irq != phys_irq ||
> - map->irq != irq)
> + map->irq != irq ||
> + map->shared != shared)
this really feels like a BUG() - if we have an existing mapping for the
same virtual IRQ, but it's shared in one case and not shared in the
other? Shouldn we even allow the caller to get this far?
> map = NULL;
>
> goto out;
> @@ -1706,6 +1739,7 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> map->virt_irq = virt_irq;
> map->phys_irq = phys_irq;
> map->irq = irq;
> + map->shared = shared;
>
> list_add_tail_rcu(&entry->entry, root);
>
> @@ -1746,13 +1780,13 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
>
> bool vgic_get_phys_irq_active(struct irq_phys_map *map)
> {
> - BUG_ON(!map);
> + BUG_ON(!map || !map->shared);
> return map->active;
> }
>
> void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active)
> {
> - BUG_ON(!map);
> + BUG_ON(!map || !map->shared);
> map->active = active;
> }
>
> --
> 2.1.4
>
Do we really need this patch right now as part of this series or should
this rather go with Eric's forwarding series and we can focus on getting
rid of the timer hack for now?
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 02/10] arm/arm64: KVM: Move vgic handling to a non-preemptible section
2015-07-08 17:56 ` [PATCH v2 02/10] arm/arm64: KVM: Move vgic handling to a non-preemptible section Marc Zyngier
@ 2015-07-17 22:15 ` Christoffer Dall
0 siblings, 0 replies; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 22:15 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:34PM +0100, Marc Zyngier wrote:
> As we're about to introduce some serious GIC-poking to the vgic code,
> it is important to make sure that we're going to poke the part of
> the GIC that belongs to the CPU we're about to run on (otherwise,
> we'd end up with some unexpected interrupts firing)...
>
> Introducing a non-preemptible section in kvm_arch_vcpu_ioctl_run
> prevents the problem from occuring.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields
2015-07-08 17:56 ` [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields Marc Zyngier
@ 2015-07-17 22:15 ` Christoffer Dall
2015-07-21 18:02 ` Marc Zyngier
0 siblings, 1 reply; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 22:15 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:35PM +0100, Marc Zyngier wrote:
> As we're about to cram more information in the vgic_lr structure
> (HW interrupt number and additional state information), we switch
> to a layout similar to the HW's:
>
> - use bitfields to save space (we don't need more than 10 bits
> to represent the irq numbers)
> - source CPU and HW interrupt can share the same field, as
> a SGI doesn't have a physical line.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
> include/kvm/arm_vgic.h | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 133ea00..4f9fa1d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -95,11 +95,15 @@ enum vgic_type {
> #define LR_STATE_ACTIVE (1 << 1)
> #define LR_STATE_MASK (3 << 0)
> #define LR_EOI_INT (1 << 2)
> +#define LR_HW (1 << 3)
>
> struct vgic_lr {
> - u16 irq;
> - u8 source;
> - u8 state;
> + unsigned irq:10;
> + union {
> + unsigned hwirq:10;
> + unsigned source:8;
why 8? why not 3?
> + };
> + unsigned state:4;
> };
>
> struct vgic_vmcr {
> --
> 2.1.4
>
otherwise:
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 05/10] KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs
2015-07-08 17:56 ` [PATCH v2 05/10] KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs Marc Zyngier
@ 2015-07-17 22:15 ` Christoffer Dall
0 siblings, 0 replies; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 22:15 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:37PM +0100, Marc Zyngier wrote:
> We only set the irq_queued flag for level interrupts, meaning
> that "!vgic_irq_is_queued(vcpu, irq)" is a good enough predicate
> for all interrupts.
>
> This will allow us to inject edge HW interrupts, for which the
> state ACTIVE+PENDING is not allowed.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> virt/kvm/arm/vgic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index bc40137..5bd1695 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -375,7 +375,7 @@ void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
>
> static bool vgic_can_sample_irq(struct kvm_vcpu *vcpu, int irq)
> {
> - return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
> + return !vgic_irq_is_queued(vcpu, irq);
> }
>
> /**
> --
> 2.1.4
>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get,set}_phys_irq_active
2015-07-08 17:56 ` [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get, set}_phys_irq_active Marc Zyngier
@ 2015-07-17 22:15 ` Christoffer Dall
0 siblings, 0 replies; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 22:15 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:40PM +0100, Marc Zyngier wrote:
> In order to control the active state of an interrupt, introduce
> a pair of accessors allowing the state to be set/queried.
>
> This only affects the logical state, and the HW state will only be
> applied at world-switch time.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 09/10] KVM: arm/arm64: timer: Allow the timer to control the active state
2015-07-08 17:56 ` [PATCH v2 09/10] KVM: arm/arm64: timer: Allow the timer to control the active state Marc Zyngier
@ 2015-07-17 22:15 ` Christoffer Dall
0 siblings, 0 replies; 27+ messages in thread
From: Christoffer Dall @ 2015-07-17 22:15 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 08, 2015 at 06:56:41PM +0100, Marc Zyngier wrote:
> In order to remove the crude hack where we sneak the masked bit
> into the timer's control register, make use of the phys_irq_map
> API control the active state of the interrupt.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR
2015-07-17 19:50 ` Christoffer Dall
@ 2015-07-21 16:38 ` Marc Zyngier
2015-08-04 12:14 ` Christoffer Dall
0 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-21 16:38 UTC (permalink / raw
To: linux-arm-kernel
On 17/07/15 20:50, Christoffer Dall wrote:
> On Wed, Jul 08, 2015 at 06:56:36PM +0100, Marc Zyngier wrote:
>> Now that struct vgic_lr supports the LR_HW bit and carries a hwirq
>> field, we can encode that information into the list registers.
>>
>> This patch provides implementations for both GICv2 and GICv3.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> include/linux/irqchip/arm-gic-v3.h | 3 +++
>> include/linux/irqchip/arm-gic.h | 3 ++-
>> virt/kvm/arm/vgic-v2.c | 16 +++++++++++++++-
>> virt/kvm/arm/vgic-v3.c | 21 ++++++++++++++++++---
>> 4 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index ffbc034..cf637d6 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -268,9 +268,12 @@
>>
>> #define ICH_LR_EOI (1UL << 41)
>> #define ICH_LR_GROUP (1UL << 60)
>> +#define ICH_LR_HW (1UL << 61)
>> #define ICH_LR_STATE (3UL << 62)
>> #define ICH_LR_PENDING_BIT (1UL << 62)
>> #define ICH_LR_ACTIVE_BIT (1UL << 63)
>> +#define ICH_LR_PHYS_ID_SHIFT 32
>> +#define ICH_LR_PHYS_ID_MASK (0x3ffUL << ICH_LR_PHYS_ID_SHIFT)
>>
>> #define ICH_MISR_EOI (1 << 0)
>> #define ICH_MISR_U (1 << 1)
>> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
>> index 9de976b..ca88dad 100644
>> --- a/include/linux/irqchip/arm-gic.h
>> +++ b/include/linux/irqchip/arm-gic.h
>> @@ -71,11 +71,12 @@
>>
>> #define GICH_LR_VIRTUALID (0x3ff << 0)
>> #define GICH_LR_PHYSID_CPUID_SHIFT (10)
>> -#define GICH_LR_PHYSID_CPUID (7 << GICH_LR_PHYSID_CPUID_SHIFT)
>> +#define GICH_LR_PHYSID_CPUID (0x3ff << GICH_LR_PHYSID_CPUID_SHIFT)
>> #define GICH_LR_STATE (3 << 28)
>> #define GICH_LR_PENDING_BIT (1 << 28)
>> #define GICH_LR_ACTIVE_BIT (1 << 29)
>> #define GICH_LR_EOI (1 << 19)
>> +#define GICH_LR_HW (1 << 31)
>>
>> #define GICH_VMCR_CTRL_SHIFT 0
>> #define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT)
>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>> index f9b9c7c..8d7b04d 100644
>> --- a/virt/kvm/arm/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic-v2.c
>> @@ -48,6 +48,10 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
>> lr_desc.state |= LR_STATE_ACTIVE;
>> if (val & GICH_LR_EOI)
>> lr_desc.state |= LR_EOI_INT;
>> + if (val & GICH_LR_HW) {
>> + lr_desc.state |= LR_HW;
>> + lr_desc.hwirq = (val & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT;
>> + }
>>
>> return lr_desc;
>> }
>> @@ -55,7 +59,9 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
>> static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
>> struct vgic_lr lr_desc)
>> {
>> - u32 lr_val = (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT) | lr_desc.irq;
>> + u32 lr_val;
>> +
>> + lr_val = lr_desc.irq;
>>
>> if (lr_desc.state & LR_STATE_PENDING)
>> lr_val |= GICH_LR_PENDING_BIT;
>> @@ -64,6 +70,14 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
>> if (lr_desc.state & LR_EOI_INT)
>> lr_val |= GICH_LR_EOI;
>>
>> + if (lr_desc.state & LR_HW) {
>> + lr_val |= GICH_LR_HW;
>> + lr_val |= (u32)lr_desc.hwirq << GICH_LR_PHYSID_CPUID_SHIFT;
>> + }
>> +
>> + if (lr_desc.irq < VGIC_NR_SGIS)
>> + lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
>> +
>> vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
>> }
>>
>> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
>> index dff0602..afbf925 100644
>> --- a/virt/kvm/arm/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic-v3.c
>> @@ -67,6 +67,10 @@ static struct vgic_lr vgic_v3_get_lr(const struct kvm_vcpu *vcpu, int lr)
>> lr_desc.state |= LR_STATE_ACTIVE;
>> if (val & ICH_LR_EOI)
>> lr_desc.state |= LR_EOI_INT;
>> + if (val & ICH_LR_HW) {
>> + lr_desc.state |= LR_HW;
>> + lr_desc.hwirq = (val >> ICH_LR_PHYS_ID_SHIFT) & GENMASK(9, 0);
>> + }
>>
>> return lr_desc;
>> }
>> @@ -84,10 +88,17 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
>> * Eventually we want to make this configurable, so we may revisit
>> * this in the future.
>> */
>> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
>> + switch (vcpu->kvm->arch.vgic.vgic_model) {
>> + case KVM_DEV_TYPE_ARM_VGIC_V3:
>> lr_val |= ICH_LR_GROUP;
>> - else
>> - lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
>> + break;
>> + case KVM_DEV_TYPE_ARM_VGIC_V2:
>> + if (lr_desc.irq < VGIC_NR_SGIS)
>> + lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
>
> I forget how this works: Why are we mixing GICH_LR_ stuff with ICH_LR_
> in the same function? Aren't we always accessing these registers via
> the system registers interface from the hypervisor control point of
> view, regardless of which GIC version the guest sees?
That's a bit odd indeed, but there's a small trick here:
- GICv2 has the CPUID as part of the PHYSID field.
- GICv3 "native" has no CPUID
- GICv3, when used with a GICv2 guest, puts CPUID as part of the the
bottom 32bit of the ICH_LRn_EL2 register, which happens to be the VIRTID.
And that VIRTID, when used with a GICv2 guest, actually contains most of
the GICH_LR fields. Crucially, CPUID is located at VIRTID[12:10], which
happens to be GICH_LRn[12:10], exactly where it is on a GICv2.
Confusing? You bet. Oh, and that subtle difference seems to have been
removed from the architecture spec. Time to file a bug report.
> I guess my question here is: Why should this not be
> ICH_LR_PHYSID_CORE_SHIFT?
No, because as explained above, the CPUID doesn't belong to PHYSID...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts
2015-07-17 21:11 ` Christoffer Dall
@ 2015-07-21 17:17 ` Marc Zyngier
0 siblings, 0 replies; 27+ messages in thread
From: Marc Zyngier @ 2015-07-21 17:17 UTC (permalink / raw
To: linux-arm-kernel
On 17/07/15 22:11, Christoffer Dall wrote:
> On Wed, Jul 08, 2015 at 06:56:38PM +0100, Marc Zyngier wrote:
>> In order to be able to feed physical interrupts to a guest, we need
>> to be able to establish the virtual-physical mapping between the two
>> worlds.
>>
>> The mappings are kept in a set of RCU lists, indexed by virtual interrupts.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> arch/arm/kvm/arm.c | 2 +
>> include/kvm/arm_vgic.h | 25 +++++++++
>> virt/kvm/arm/vgic.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 170 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 1583a34..d5ce5cc 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -125,6 +125,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>> if (ret)
>> goto out_free_stage2_pgd;
>>
>> + kvm_vgic_init(kvm);
>> kvm_timer_init(kvm);
>>
>> /* Mark the initial VMID generation invalid */
>> @@ -249,6 +250,7 @@ out:
>>
>> void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>> {
>> + kvm_vgic_vcpu_postcreate(vcpu);
>> }
>>
>> void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 4f9fa1d..32e57d2 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -159,6 +159,19 @@ struct vgic_io_device {
>> struct kvm_io_device dev;
>> };
>>
>> +struct irq_phys_map {
>> + u32 virt_irq;
>> + u32 phys_irq;
>> + u32 irq;
>> + bool active;
>> +};
>> +
>> +struct irq_phys_map_entry {
>> + struct list_head entry;
>> + struct rcu_head rcu;
>> + struct irq_phys_map map;
>> +};
>> +
>> struct vgic_dist {
>> spinlock_t lock;
>> bool in_kernel;
>> @@ -256,6 +269,10 @@ struct vgic_dist {
>> struct vgic_vm_ops vm_ops;
>> struct vgic_io_device dist_iodev;
>> struct vgic_io_device *redist_iodevs;
>> +
>> + /* Virtual irq to hwirq mapping */
>> + spinlock_t irq_phys_map_lock;
>> + struct list_head irq_phys_map_list;
>> };
>>
>> struct vgic_v2_cpu_if {
>> @@ -307,6 +324,9 @@ struct vgic_cpu {
>> struct vgic_v2_cpu_if vgic_v2;
>> struct vgic_v3_cpu_if vgic_v3;
>> };
>> +
>> + /* Protected by the distributor's irq_phys_map_lock */
>> + struct list_head irq_phys_map_list;
>> };
>>
>> #define LR_EMPTY 0xff
>> @@ -321,8 +341,10 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
>> int kvm_vgic_hyp_init(void);
>> int kvm_vgic_map_resources(struct kvm *kvm);
>> int kvm_vgic_get_max_vcpus(void);
>> +void kvm_vgic_init(struct kvm *kvm);
>> int kvm_vgic_create(struct kvm *kvm, u32 type);
>> void kvm_vgic_destroy(struct kvm *kvm);
>> +void kvm_vgic_vcpu_postcreate(struct kvm_vcpu *vcpu);
>> void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
>> void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>> void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>> @@ -331,6 +353,9 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>> void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
>> +struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> + int virt_irq, int irq);
>> +int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
>
> should these functions not have a kvm_ prefix?
Guess that'd be a good idea - VFIO will probably have to use them somehow.
>> #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>> #define vgic_initialized(k) (!!((k)->arch.vgic.nr_cpus))
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 5bd1695..3424329 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -24,6 +24,7 @@
>> #include <linux/of.h>
>> #include <linux/of_address.h>
>> #include <linux/of_irq.h>
>> +#include <linux/rculist.h>
>> #include <linux/uaccess.h>
>>
>> #include <asm/kvm_emulate.h>
>> @@ -82,6 +83,8 @@ 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 struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
>> + int virt_irq);
>>
>> static const struct vgic_ops *vgic_ops;
>> static const struct vgic_params *vgic;
>> @@ -1583,6 +1586,131 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>>
>> +static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
>> + int virt_irq)
>> +{
>> + if (virt_irq < VGIC_NR_PRIVATE_IRQS)
>> + return &vcpu->arch.vgic_cpu.irq_phys_map_list;
>> + else
>> + return &vcpu->kvm->arch.vgic.irq_phys_map_list;
>> +}
>> +
>
> You know what I'm going to ask you for here, but let me help out with
> the framwork:
>
> /**
> * vgic_map_phys_irq - map a virtual IRQ to a physical IRQ
> * @vcpu: The VCPU pointer
> * @virt_irq: The virtual irq number
> * @irq: The Linux IRQ number
> *
> *
> */
You're making it to easy! ;-)
>> +struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> + int virt_irq, int irq)
>> +{
>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> + struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
>> + struct irq_phys_map *map;
>> + struct irq_phys_map_entry *entry;
>> + struct irq_desc *desc;
>> + struct irq_data *data;
>> + int phys_irq;
>> +
>> + desc = irq_to_desc(irq);
>> + if (!desc) {
>> + kvm_err("kvm_arch_timer: can't obtain interrupt descriptor\n");
>
> arch_timer? this is vgic code, no?
-ECUTnPASTE, I'll fix that.
>> + return NULL;
>> + }
>> +
>> + data = irq_desc_get_irq_data(desc);
>> + while (data->parent_data)
>> + data = data->parent_data;
>> +
>> + phys_irq = data->hwirq;
>> +
>> + spin_lock(&dist->irq_phys_map_lock);
>> +
>> + /* Try to match an existing mapping */
>> + map = vgic_irq_map_search(vcpu, virt_irq);
>> + if (map) {
>> + /* Make sure this mapping matches */
>> + if (map->phys_irq != phys_irq ||
>> + map->irq != irq)
>
> when would this happen? Is this something that should gracefully just
> be accepted or is it a bug?
This is definitely a bug, hence the NULL value being returned. There is
already an existing mapping for this virtual interrupt, and the caller
should handle the problem.
>> + map = NULL;
>> +
>> + goto out;
>> + }
>> +
>> + /* Create a new mapping */
>> + entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
>
> is GFP_ATOMIC really warranted here? The situatotion where you have an
> existing map is extremely rare, I suppose, and is in fact an error, so
> you could just pre-allocate and free on error.
Good point.
>> + if (!entry)
>
> Here you seem to be returning a valid value on an error? Perhaps you
> should return ERR_PTR(-ENOMEM) and generally use ERR_PTR/PTR_ERR here.
Indeed, that'd be nicer.
>> + goto out;
>> +
>> + map = &entry->map;
>> + map->virt_irq = virt_irq;
>> + map->phys_irq = phys_irq;
>> + map->irq = irq;
>> +
>> + list_add_tail_rcu(&entry->entry, root);
>> +
>> +out:
>> + spin_unlock(&dist->irq_phys_map_lock);
>> + return map;
>> +}
>> +
>> +static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
>> + int virt_irq)
>> +{
>> + struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
>> + struct irq_phys_map_entry *entry;
>> + struct irq_phys_map *map;
>> +
>> + rcu_read_lock();
>> +
>> + list_for_each_entry_rcu(entry, root, entry) {
>> + map = &entry->map;
>> + if (map->virt_irq == virt_irq) {
>> + rcu_read_unlock();
>> + return map;
>> + }
>> + }
>> +
>> + rcu_read_unlock();
>> +
>> + return NULL;
>> +}
>> +
>> +static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
>> +{
>> + struct irq_phys_map_entry *entry;
>> +
>> + entry = container_of(rcu, struct irq_phys_map_entry, rcu);
>> + kfree(entry);
>> +}
>> +
>> +int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
>> +{
>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> + struct irq_phys_map_entry *entry;
>> +
>> + if (!map)
>> + return -EINVAL;
>> +
>> + entry = container_of(map, struct irq_phys_map_entry, map);
>
> could this race with vgic_destroy_irq_phys_map, such that
> vgic_destroy_irq_phys_map removes the entry from the list while we're
> spinning on the lock, and then when we proceed we free the entry twice?
Hmmm. That's nasty. I guess that the only way around this is to parse
the list, looking for that particular entry. If destroy already
happened, we won't find it, catastrophy avoided.
>> +
>> + spin_lock(&dist->irq_phys_map_lock);
>> + list_del_rcu(&entry->entry);
>> + call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
>> + spin_unlock(&dist->irq_phys_map_lock);
>> +
>> + return 0;
>> +}
>> +
>> +static void vgic_destroy_irq_phys_map(struct kvm *kvm, struct list_head *root)
>> +{
>> + struct vgic_dist *dist = &kvm->arch.vgic;
>> + struct irq_phys_map_entry *entry;
>> +
>> + spin_lock(&dist->irq_phys_map_lock);
>> +
>> + list_for_each_entry(entry, root, entry) {
>> + list_del_rcu(&entry->entry);
>> + call_rcu(&entry->rcu, vgic_free_phys_irq_map_rcu);
>> + }
>> +
>> + spin_unlock(&dist->irq_phys_map_lock);
>> +}
>> +
>> void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
>> {
>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> @@ -1591,6 +1719,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
>> 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;
>> @@ -1627,6 +1756,12 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
>> return 0;
>> }
>>
>> +void kvm_vgic_vcpu_postcreate(struct kvm_vcpu *vcpu)
>> +{
>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> + INIT_LIST_HEAD(&vgic_cpu->irq_phys_map_list);
>
> can you do this as part of vgic_init?
No, there is a horrible race with vgic_init which can be lazy (at least
with GICv2). In the interval, the timer code will try and register its
interrupt mapping. Pain will follow.
>> +}
>> +
>> /**
>> * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
>> *
>> @@ -1664,6 +1799,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
>> kfree(dist->irq_spi_target);
>> kfree(dist->irq_pending_on_cpu);
>> kfree(dist->irq_active_on_cpu);
>> + vgic_destroy_irq_phys_map(kvm, &dist->irq_phys_map_list);
>> dist->irq_sgi_sources = NULL;
>> dist->irq_spi_cpu = NULL;
>> dist->irq_spi_target = NULL;
>> @@ -1787,6 +1923,13 @@ static int init_vgic_model(struct kvm *kvm, int type)
>> return 0;
>> }
>>
>> +void kvm_vgic_init(struct kvm *kvm)
>> +{
>> + spin_lock_init(&kvm->arch.vgic.lock);
>> + spin_lock_init(&kvm->arch.vgic.irq_phys_map_lock);
>> + INIT_LIST_HEAD(&kvm->arch.vgic.irq_phys_map_list);
>
> why can we not do this as part of kvm_vgic_create?
For the same reason as above. The spinlock must be initialized early
enough for the timer (or any other subsystem) code to call the map
function and register its interrupts.
> at least we need to think about naming here or document clearly what the
> various init functions do; it is not clear what the difference between
> vgic_init and kvm_vgic_init is...
Agreed, this is a bit of a mess. I'll try to come up with a list of init
functions, their expected execution order, and what is guaranteed at
which stage.
>> +}
>> +
>> int kvm_vgic_create(struct kvm *kvm, u32 type)
>> {
>> int i, vcpu_lock_idx = -1, ret;
>> @@ -1832,7 +1975,6 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>> if (ret)
>> goto out_unlock;
>>
>> - spin_lock_init(&kvm->arch.vgic.lock);
>> kvm->arch.vgic.in_kernel = true;
>> kvm->arch.vgic.vgic_model = type;
>> kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>> --
>> 2.1.4
>>
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest
2015-07-17 21:56 ` Christoffer Dall
@ 2015-07-21 17:21 ` Marc Zyngier
0 siblings, 0 replies; 27+ messages in thread
From: Marc Zyngier @ 2015-07-21 17:21 UTC (permalink / raw
To: linux-arm-kernel
On 17/07/15 22:56, Christoffer Dall wrote:
> On Wed, Jul 08, 2015 at 06:56:39PM +0100, Marc Zyngier wrote:
>> To allow a HW interrupt to be injected into a guest, we lookup the
>> guest virtual interrupt in the irq_phys_map rbtree, and if we have
>
> s/rbtree/list/
>
>> a match, encode both interrupts in the LR.
>>
>> We also mark the interrupt as "active" at the host distributor level.
>>
>> On guest EOI on the virtual interrupt, the host interrupt will be
>> deactivated.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> virt/kvm/arm/vgic.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 69 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 3424329..f8582d7 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1118,6 +1118,26 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
>> if (!vgic_irq_is_edge(vcpu, irq))
>> vlr.state |= LR_EOI_INT;
>>
>> + if (vlr.irq >= VGIC_NR_SGIS) {
>> + struct irq_phys_map *map;
>> + map = vgic_irq_map_search(vcpu, irq);
>> +
>> + if (map) {
>> + int ret;
>> +
>> + BUG_ON(!map->active);
>> + vlr.hwirq = map->phys_irq;
>> + vlr.state |= LR_HW;
>> + vlr.state &= ~LR_EOI_INT;
>> +
>> + ret = irq_set_irqchip_state(map->irq,
>> + IRQCHIP_STATE_ACTIVE,
>> + true);
>
> So if we have a mapping, and the virtual interrupt is being injected,
> then we must set the state to active in the physical world, because
> otherwise the guest will just exit before processing the virtual
> interrupt, yes?
Exactly. This is what will prevent the interrupt from firing on the host
(we going back to the state the GIC would be in if we were handling this
interrupt on the host).
> I think this deserves a comment here.
Sure.
>> + vgic_irq_set_queued(vcpu, irq);
>
> And we set it queued only to avoid sampling it again and setting
> PENDING+ACTIVE, because the hardware doesn't support setting that state
> when the HW bit is set, yes?
Indeed.
> I think a comment here is warranted too.
Fair enough.
>> + WARN_ON(ret);
>> + }
>> + }
>> +
>> vgic_set_lr(vcpu, lr_nr, vlr);
>> vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
>> }
>> @@ -1342,6 +1362,35 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>> return level_pending;
>> }
>>
>> +/* Return 1 if HW interrupt went from active to inactive, and 0 otherwise */
>
> ... also clear the active state on the physical distributor so that
> shared devices can be used in a different context. (did I get this
> right?)
You're getting good at that! ;-)
>> +static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
>> +{
>> + struct irq_phys_map *map;
>> + int ret;
>> +
>> + if (!(vlr.state & LR_HW))
>> + return 0;
>> +
>> + map = vgic_irq_map_search(vcpu, vlr.irq);
>> + BUG_ON(!map || !map->active);
>> +
>> + ret = irq_get_irqchip_state(map->irq,
>> + IRQCHIP_STATE_ACTIVE,
>> + &map->active);
>> +
>> + WARN_ON(ret);
>> +
>> + if (map->active) {
>> + ret = irq_set_irqchip_state(map->irq,
>> + IRQCHIP_STATE_ACTIVE,
>> + false);
>> + WARN_ON(ret);
>> + return 0;
>> + }
>> +
>> + return 1;
>> +}
>> +
>> /* Sync back the VGIC state after a guest run */
>> static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>> {
>> @@ -1356,14 +1405,31 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *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) {
>> + /* Deal with HW interrupts, and clear mappings for empty LRs */
>> + for (lr = 0; lr < vgic->nr_lr; lr++) {
>> struct vgic_lr vlr;
>>
>> - if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
>> + if (!test_bit(lr, vgic_cpu->lr_used))
>> continue;
>>
>> vlr = vgic_get_lr(vcpu, lr);
>> + if (vgic_sync_hwirq(vcpu, vlr)) {
>> + /*
>> + * So this is a HW interrupt that the guest
>> + * EOI-ed. Clean the LR state and allow the
>> + * interrupt to be queued again.
>
> s/queued/sampled/ ?
Indeed.
>> + */
>> + vlr.state = 0;
>> + vlr.hwirq = 0;
>> + vgic_set_lr(vcpu, lr, vlr);
>> + vgic_irq_clear_queued(vcpu, vlr.irq);
>> + set_bit(lr, elrsr_ptr);
>> + }
>> +
>> + if (!test_bit(lr, elrsr_ptr))
>> + continue;
>> +
>> + clear_bit(lr, vgic_cpu->lr_used);
>>
>> BUG_ON(vlr.irq >= dist->nr_irqs);
>> vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
>> --
>> 2.1.4
>>
> Thanks,
> -Christoffer
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts
2015-07-17 22:15 ` Christoffer Dall
@ 2015-07-21 18:01 ` Marc Zyngier
2015-08-04 12:26 ` Christoffer Dall
0 siblings, 1 reply; 27+ messages in thread
From: Marc Zyngier @ 2015-07-21 18:01 UTC (permalink / raw
To: linux-arm-kernel
On 17/07/15 23:15, Christoffer Dall wrote:
> On Wed, Jul 08, 2015 at 06:56:42PM +0100, Marc Zyngier wrote:
>> So far, the only use of the HW interrupt facility is the timer,
>> implying that the active state is context-switched for each vcpu,
>> as the device is is shared across all vcpus.
>>
>> This does not work for a device that has been assigned to a VM,
>> as the guest is entierely in control of that device (the HW is
>> not shared). In that case, it makes sense to bypass the whole
>> active state switchint, and only track the deactivation of the
>
> switching
>
>> interrupt.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> include/kvm/arm_vgic.h | 5 ++--
>> virt/kvm/arm/arch_timer.c | 2 +-
>> virt/kvm/arm/vgic.c | 62 ++++++++++++++++++++++++++++++++++++-----------
>> 3 files changed, 52 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 9fd4023..31c987a 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -163,7 +163,8 @@ struct irq_phys_map {
>> u32 virt_irq;
>> u32 phys_irq;
>> u32 irq;
>> - bool active;
>> + bool shared;
>> + bool active; /* Only valid if shared */
>> };
>>
>> struct irq_phys_map_entry {
>> @@ -354,7 +355,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
>> struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> - int virt_irq, int irq);
>> + int virt_irq, int irq, bool shared);
>> int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
>> bool vgic_get_phys_irq_active(struct irq_phys_map *map);
>> void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active);
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index b9fff78..9544d79 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -202,7 +202,7 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>> * Tell the VGIC that the virtual interrupt is tied to a
>> * physical interrupt. We do that once per VCPU.
>> */
>> - timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
>> + timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq, true);
>> WARN_ON(!timer->map);
>> }
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 39f9479..3585bb0 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1123,18 +1123,21 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
>> map = vgic_irq_map_search(vcpu, irq);
>>
>> if (map) {
>> - int ret;
>> -
>> - BUG_ON(!map->active);
>> vlr.hwirq = map->phys_irq;
>> vlr.state |= LR_HW;
>> vlr.state &= ~LR_EOI_INT;
>>
>> - ret = irq_set_irqchip_state(map->irq,
>> - IRQCHIP_STATE_ACTIVE,
>> - true);
>> vgic_irq_set_queued(vcpu, irq);
>> - WARN_ON(ret);
>> +
>> + if (map->shared) {
>> + int ret;
>> +
>> + BUG_ON(!map->active);
>> + ret = irq_set_irqchip_state(map->irq,
>> + IRQCHIP_STATE_ACTIVE,
>> + true);
>> + WARN_ON(ret);
>
> do we have any other example of a shared device or is this really simply
> because the timer hardware is fscking strangely tied to the gic and is a
> total one-off?
I don't think of it as a one-off. PMUs could very well be in the same
category.
> In the latter case, would it be cleaner to drop this notion of a shared
> device entirely and put all this logic in the arch timer code instead?
My initial implementation did exactly that (hence the cut-n-paste crap
you spotted in patch #6). It wasn't bad, but it did put a lot of GIC
knowledge inside the timer code, and a few callbacks too many between
the two subsystems.
>> + }
>> }
>> }
>>
>> @@ -1366,21 +1369,37 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>> static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
>> {
>> struct irq_phys_map *map;
>> + bool active;
>> int ret;
>>
>> if (!(vlr.state & LR_HW))
>> return 0;
>>
>> map = vgic_irq_map_search(vcpu, vlr.irq);
>> - BUG_ON(!map || !map->active);
>> + BUG_ON(!map);
>> + BUG_ON(map->shared && !map->active);
>>
>> ret = irq_get_irqchip_state(map->irq,
>> IRQCHIP_STATE_ACTIVE,
>> - &map->active);
>> + &active);
>>
>> WARN_ON(ret);
>>
>> - if (map->active) {
>> + /*
>> + * For a non-shared interrupt, we have to catter for two
>
> s/catter/cater/ ?
Yup.
>> + * possible deactivation conditions
>
> conditions:
>
>> + *
>> + * - the interrupt is now inactive
>> + * - the interrupt is still active, but is flagged as not
>> + * queued, indicating another interrupt has fired before we
>> + * could observe the deactivate.
>
> are these physical or virtual interrupts we are talking about? I assume
> virtual, but it would be good to be specific. It's not like we're going
> to remember any of this in a little bit.
There is a massive ambiguity here. It should read:
- the physical interrupt is now inactive (EOIed from the guest)
- the physical interrupt is still active, but its virtual counterpart is
flagged as "not queued", indicating another interrupt has fired between
the EOI and the guest exit.
>> + */
>> + if (!map->shared)
>> + return !active || !vgic_irq_is_queued(vcpu, vlr.irq);
>
> if the second part of the disjunction returns true, doesn't this mean
> we can potentially write the active+pending state in the LR for a HW
> interrupt, which is not allowed?
I don't think so. If the physical interrupt has fired, it is because it
has been EOIed (hence deactivated) first (the active state would
otherwise prevent it from firing).
>> +
>> + map->active = active;
>> +
>> + if (active) {
>
> should you be doing this for a non-shared interrupt?
No. A non-shared interrupt is only dealt with by a single VM, so there
is no need to clear its state. That would actually completely confuse
both the device and the VM if you did, because you could now inject a
new interrupt while the previous one is still in progress. Mayhem follows...
>> ret = irq_set_irqchip_state(map->irq,
>> IRQCHIP_STATE_ACTIVE,
>> false);
>> @@ -1523,6 +1542,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>> int edge_triggered, level_triggered;
>> int enabled;
>> bool ret = true, can_inject = true;
>> + struct irq_phys_map *map;
>>
>> spin_lock(&dist->lock);
>>
>> @@ -1569,6 +1589,18 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>> goto out;
>> }
>>
>> + map = vgic_irq_map_search(vcpu, irq_num);
>> + if (map && !map->shared) {
>> + /*
>> + * We are told to inject a HW irq, so we have to trust
>> + * the caller that the previous one has been EOIed,
>> + * and that a new one is now active. Clearing the
>> + * queued state will have the effect of making it
>> + * sample-able again.
>> + */
>> + vgic_irq_clear_queued(vcpu, irq_num);
>
> see my question above about active+pending
I hope I convinced you above. I realize there is one thing missing
though. Userspace shouldn't be allowed to inject a mapped interrupt.
Ever. I'll fix that.
>> + }
>> +
>> if (!vgic_can_sample_irq(vcpu, irq_num)) {
>> /*
>> * Level interrupt in progress, will be picked up
>> @@ -1662,7 +1694,7 @@ static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
>> }
>>
>> struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> - int virt_irq, int irq)
>> + int virt_irq, int irq, bool shared)
>> {
>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
>> @@ -1691,7 +1723,8 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> if (map) {
>> /* Make sure this mapping matches */
>> if (map->phys_irq != phys_irq ||
>> - map->irq != irq)
>> + map->irq != irq ||
>> + map->shared != shared)
>
> this really feels like a BUG() - if we have an existing mapping for the
> same virtual IRQ, but it's shared in one case and not shared in the
> other? Shouldn we even allow the caller to get this far?
How early do you want to detect it? All we can do is tell the caller to
bugger off by returning an error (we return NULL at the moment, meaning
that the caller doesn't get a mapping at all).
>> map = NULL;
>>
>> goto out;
>> @@ -1706,6 +1739,7 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>> map->virt_irq = virt_irq;
>> map->phys_irq = phys_irq;
>> map->irq = irq;
>> + map->shared = shared;
>>
>> list_add_tail_rcu(&entry->entry, root);
>>
>> @@ -1746,13 +1780,13 @@ static void vgic_free_phys_irq_map_rcu(struct rcu_head *rcu)
>>
>> bool vgic_get_phys_irq_active(struct irq_phys_map *map)
>> {
>> - BUG_ON(!map);
>> + BUG_ON(!map || !map->shared);
>> return map->active;
>> }
>>
>> void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active)
>> {
>> - BUG_ON(!map);
>> + BUG_ON(!map || !map->shared);
>> map->active = active;
>> }
>>
>> --
>> 2.1.4
>>
>
> Do we really need this patch right now as part of this series or should
> this rather go with Eric's forwarding series and we can focus on getting
> rid of the timer hack for now?
It should definitely be part of Eric's. I kept it with the rest of this
series in order to keep a relative level of sanity, but I'd be happy for
Eric to take custody of it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields
2015-07-17 22:15 ` Christoffer Dall
@ 2015-07-21 18:02 ` Marc Zyngier
0 siblings, 0 replies; 27+ messages in thread
From: Marc Zyngier @ 2015-07-21 18:02 UTC (permalink / raw
To: linux-arm-kernel
On 17/07/15 23:15, Christoffer Dall wrote:
> On Wed, Jul 08, 2015 at 06:56:35PM +0100, Marc Zyngier wrote:
>> As we're about to cram more information in the vgic_lr structure
>> (HW interrupt number and additional state information), we switch
>> to a layout similar to the HW's:
>>
>> - use bitfields to save space (we don't need more than 10 bits
>> to represent the irq numbers)
>> - source CPU and HW interrupt can share the same field, as
>> a SGI doesn't have a physical line.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>> ---
>> include/kvm/arm_vgic.h | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 133ea00..4f9fa1d 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -95,11 +95,15 @@ enum vgic_type {
>> #define LR_STATE_ACTIVE (1 << 1)
>> #define LR_STATE_MASK (3 << 0)
>> #define LR_EOI_INT (1 << 2)
>> +#define LR_HW (1 << 3)
>>
>> struct vgic_lr {
>> - u16 irq;
>> - u8 source;
>> - u8 state;
>> + unsigned irq:10;
>> + union {
>> + unsigned hwirq:10;
>> + unsigned source:8;
>
> why 8? why not 3?
Because I can't count :-).
>> + };
>> + unsigned state:4;
>> };
>>
>> struct vgic_vmcr {
>> --
>> 2.1.4
>>
>
> otherwise:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR
2015-07-21 16:38 ` Marc Zyngier
@ 2015-08-04 12:14 ` Christoffer Dall
0 siblings, 0 replies; 27+ messages in thread
From: Christoffer Dall @ 2015-08-04 12:14 UTC (permalink / raw
To: linux-arm-kernel
On Tue, Jul 21, 2015 at 05:38:52PM +0100, Marc Zyngier wrote:
> On 17/07/15 20:50, Christoffer Dall wrote:
> > On Wed, Jul 08, 2015 at 06:56:36PM +0100, Marc Zyngier wrote:
> >> Now that struct vgic_lr supports the LR_HW bit and carries a hwirq
> >> field, we can encode that information into the list registers.
> >>
> >> This patch provides implementations for both GICv2 and GICv3.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >> include/linux/irqchip/arm-gic-v3.h | 3 +++
> >> include/linux/irqchip/arm-gic.h | 3 ++-
> >> virt/kvm/arm/vgic-v2.c | 16 +++++++++++++++-
> >> virt/kvm/arm/vgic-v3.c | 21 ++++++++++++++++++---
> >> 4 files changed, 38 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> >> index ffbc034..cf637d6 100644
> >> --- a/include/linux/irqchip/arm-gic-v3.h
> >> +++ b/include/linux/irqchip/arm-gic-v3.h
> >> @@ -268,9 +268,12 @@
> >>
> >> #define ICH_LR_EOI (1UL << 41)
> >> #define ICH_LR_GROUP (1UL << 60)
> >> +#define ICH_LR_HW (1UL << 61)
> >> #define ICH_LR_STATE (3UL << 62)
> >> #define ICH_LR_PENDING_BIT (1UL << 62)
> >> #define ICH_LR_ACTIVE_BIT (1UL << 63)
> >> +#define ICH_LR_PHYS_ID_SHIFT 32
> >> +#define ICH_LR_PHYS_ID_MASK (0x3ffUL << ICH_LR_PHYS_ID_SHIFT)
> >>
> >> #define ICH_MISR_EOI (1 << 0)
> >> #define ICH_MISR_U (1 << 1)
> >> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> >> index 9de976b..ca88dad 100644
> >> --- a/include/linux/irqchip/arm-gic.h
> >> +++ b/include/linux/irqchip/arm-gic.h
> >> @@ -71,11 +71,12 @@
> >>
> >> #define GICH_LR_VIRTUALID (0x3ff << 0)
> >> #define GICH_LR_PHYSID_CPUID_SHIFT (10)
> >> -#define GICH_LR_PHYSID_CPUID (7 << GICH_LR_PHYSID_CPUID_SHIFT)
> >> +#define GICH_LR_PHYSID_CPUID (0x3ff << GICH_LR_PHYSID_CPUID_SHIFT)
> >> #define GICH_LR_STATE (3 << 28)
> >> #define GICH_LR_PENDING_BIT (1 << 28)
> >> #define GICH_LR_ACTIVE_BIT (1 << 29)
> >> #define GICH_LR_EOI (1 << 19)
> >> +#define GICH_LR_HW (1 << 31)
> >>
> >> #define GICH_VMCR_CTRL_SHIFT 0
> >> #define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT)
> >> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> >> index f9b9c7c..8d7b04d 100644
> >> --- a/virt/kvm/arm/vgic-v2.c
> >> +++ b/virt/kvm/arm/vgic-v2.c
> >> @@ -48,6 +48,10 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
> >> lr_desc.state |= LR_STATE_ACTIVE;
> >> if (val & GICH_LR_EOI)
> >> lr_desc.state |= LR_EOI_INT;
> >> + if (val & GICH_LR_HW) {
> >> + lr_desc.state |= LR_HW;
> >> + lr_desc.hwirq = (val & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT;
> >> + }
> >>
> >> return lr_desc;
> >> }
> >> @@ -55,7 +59,9 @@ static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr)
> >> static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
> >> struct vgic_lr lr_desc)
> >> {
> >> - u32 lr_val = (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT) | lr_desc.irq;
> >> + u32 lr_val;
> >> +
> >> + lr_val = lr_desc.irq;
> >>
> >> if (lr_desc.state & LR_STATE_PENDING)
> >> lr_val |= GICH_LR_PENDING_BIT;
> >> @@ -64,6 +70,14 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
> >> if (lr_desc.state & LR_EOI_INT)
> >> lr_val |= GICH_LR_EOI;
> >>
> >> + if (lr_desc.state & LR_HW) {
> >> + lr_val |= GICH_LR_HW;
> >> + lr_val |= (u32)lr_desc.hwirq << GICH_LR_PHYSID_CPUID_SHIFT;
> >> + }
> >> +
> >> + if (lr_desc.irq < VGIC_NR_SGIS)
> >> + lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
> >> +
> >> vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
> >> }
> >>
> >> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> >> index dff0602..afbf925 100644
> >> --- a/virt/kvm/arm/vgic-v3.c
> >> +++ b/virt/kvm/arm/vgic-v3.c
> >> @@ -67,6 +67,10 @@ static struct vgic_lr vgic_v3_get_lr(const struct kvm_vcpu *vcpu, int lr)
> >> lr_desc.state |= LR_STATE_ACTIVE;
> >> if (val & ICH_LR_EOI)
> >> lr_desc.state |= LR_EOI_INT;
> >> + if (val & ICH_LR_HW) {
> >> + lr_desc.state |= LR_HW;
> >> + lr_desc.hwirq = (val >> ICH_LR_PHYS_ID_SHIFT) & GENMASK(9, 0);
> >> + }
> >>
> >> return lr_desc;
> >> }
> >> @@ -84,10 +88,17 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
> >> * Eventually we want to make this configurable, so we may revisit
> >> * this in the future.
> >> */
> >> - if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >> + switch (vcpu->kvm->arch.vgic.vgic_model) {
> >> + case KVM_DEV_TYPE_ARM_VGIC_V3:
> >> lr_val |= ICH_LR_GROUP;
> >> - else
> >> - lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
> >> + break;
> >> + case KVM_DEV_TYPE_ARM_VGIC_V2:
> >> + if (lr_desc.irq < VGIC_NR_SGIS)
> >> + lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
> >
> > I forget how this works: Why are we mixing GICH_LR_ stuff with ICH_LR_
> > in the same function? Aren't we always accessing these registers via
> > the system registers interface from the hypervisor control point of
> > view, regardless of which GIC version the guest sees?
>
> That's a bit odd indeed, but there's a small trick here:
>
> - GICv2 has the CPUID as part of the PHYSID field.
> - GICv3 "native" has no CPUID
> - GICv3, when used with a GICv2 guest, puts CPUID as part of the the
> bottom 32bit of the ICH_LRn_EL2 register, which happens to be the VIRTID.
>
> And that VIRTID, when used with a GICv2 guest, actually contains most of
> the GICH_LR fields. Crucially, CPUID is located at VIRTID[12:10], which
> happens to be GICH_LRn[12:10], exactly where it is on a GICv2.
>
> Confusing? You bet. Oh, and that subtle difference seems to have been
> removed from the architecture spec. Time to file a bug report.
>
> > I guess my question here is: Why should this not be
> > ICH_LR_PHYSID_CORE_SHIFT?
>
> No, because as explained above, the CPUID doesn't belong to PHYSID...
>
I get it. Thanks for explanation, you can point me here when I'm going
to ask you again in a year or so.
-Christoffer
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts
2015-07-21 18:01 ` Marc Zyngier
@ 2015-08-04 12:26 ` Christoffer Dall
0 siblings, 0 replies; 27+ messages in thread
From: Christoffer Dall @ 2015-08-04 12:26 UTC (permalink / raw
To: linux-arm-kernel
On Tue, Jul 21, 2015 at 07:01:13PM +0100, Marc Zyngier wrote:
> On 17/07/15 23:15, Christoffer Dall wrote:
> > On Wed, Jul 08, 2015 at 06:56:42PM +0100, Marc Zyngier wrote:
> >> So far, the only use of the HW interrupt facility is the timer,
> >> implying that the active state is context-switched for each vcpu,
> >> as the device is is shared across all vcpus.
> >>
> >> This does not work for a device that has been assigned to a VM,
> >> as the guest is entierely in control of that device (the HW is
> >> not shared). In that case, it makes sense to bypass the whole
> >> active state switchint, and only track the deactivation of the
> >
> > switching
> >
> >> interrupt.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >> include/kvm/arm_vgic.h | 5 ++--
> >> virt/kvm/arm/arch_timer.c | 2 +-
> >> virt/kvm/arm/vgic.c | 62 ++++++++++++++++++++++++++++++++++++-----------
> >> 3 files changed, 52 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> index 9fd4023..31c987a 100644
> >> --- a/include/kvm/arm_vgic.h
> >> +++ b/include/kvm/arm_vgic.h
> >> @@ -163,7 +163,8 @@ struct irq_phys_map {
> >> u32 virt_irq;
> >> u32 phys_irq;
> >> u32 irq;
> >> - bool active;
> >> + bool shared;
> >> + bool active; /* Only valid if shared */
> >> };
> >>
> >> struct irq_phys_map_entry {
> >> @@ -354,7 +355,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> >> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> >> int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
> >> struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> >> - int virt_irq, int irq);
> >> + int virt_irq, int irq, bool shared);
> >> int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> >> bool vgic_get_phys_irq_active(struct irq_phys_map *map);
> >> void vgic_set_phys_irq_active(struct irq_phys_map *map, bool active);
> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >> index b9fff78..9544d79 100644
> >> --- a/virt/kvm/arm/arch_timer.c
> >> +++ b/virt/kvm/arm/arch_timer.c
> >> @@ -202,7 +202,7 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >> * Tell the VGIC that the virtual interrupt is tied to a
> >> * physical interrupt. We do that once per VCPU.
> >> */
> >> - timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
> >> + timer->map = vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq, true);
> >> WARN_ON(!timer->map);
> >> }
> >>
> >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >> index 39f9479..3585bb0 100644
> >> --- a/virt/kvm/arm/vgic.c
> >> +++ b/virt/kvm/arm/vgic.c
> >> @@ -1123,18 +1123,21 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
> >> map = vgic_irq_map_search(vcpu, irq);
> >>
> >> if (map) {
> >> - int ret;
> >> -
> >> - BUG_ON(!map->active);
> >> vlr.hwirq = map->phys_irq;
> >> vlr.state |= LR_HW;
> >> vlr.state &= ~LR_EOI_INT;
> >>
> >> - ret = irq_set_irqchip_state(map->irq,
> >> - IRQCHIP_STATE_ACTIVE,
> >> - true);
> >> vgic_irq_set_queued(vcpu, irq);
> >> - WARN_ON(ret);
> >> +
> >> + if (map->shared) {
> >> + int ret;
> >> +
> >> + BUG_ON(!map->active);
> >> + ret = irq_set_irqchip_state(map->irq,
> >> + IRQCHIP_STATE_ACTIVE,
> >> + true);
> >> + WARN_ON(ret);
> >
> > do we have any other example of a shared device or is this really simply
> > because the timer hardware is fscking strangely tied to the gic and is a
> > total one-off?
>
> I don't think of it as a one-off. PMUs could very well be in the same
> category.
>
> > In the latter case, would it be cleaner to drop this notion of a shared
> > device entirely and put all this logic in the arch timer code instead?
>
> My initial implementation did exactly that (hence the cut-n-paste crap
> you spotted in patch #6). It wasn't bad, but it did put a lot of GIC
> knowledge inside the timer code, and a few callbacks too many between
> the two subsystems.
>
> >> + }
> >> }
> >> }
> >>
> >> @@ -1366,21 +1369,37 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> >> static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
> >> {
> >> struct irq_phys_map *map;
> >> + bool active;
> >> int ret;
> >>
> >> if (!(vlr.state & LR_HW))
> >> return 0;
> >>
> >> map = vgic_irq_map_search(vcpu, vlr.irq);
> >> - BUG_ON(!map || !map->active);
> >> + BUG_ON(!map);
> >> + BUG_ON(map->shared && !map->active);
> >>
> >> ret = irq_get_irqchip_state(map->irq,
> >> IRQCHIP_STATE_ACTIVE,
> >> - &map->active);
> >> + &active);
> >>
> >> WARN_ON(ret);
> >>
> >> - if (map->active) {
> >> + /*
> >> + * For a non-shared interrupt, we have to catter for two
> >
> > s/catter/cater/ ?
>
> Yup.
>
> >> + * possible deactivation conditions
> >
> > conditions:
> >
> >> + *
> >> + * - the interrupt is now inactive
> >> + * - the interrupt is still active, but is flagged as not
> >> + * queued, indicating another interrupt has fired before we
> >> + * could observe the deactivate.
> >
> > are these physical or virtual interrupts we are talking about? I assume
> > virtual, but it would be good to be specific. It's not like we're going
> > to remember any of this in a little bit.
>
> There is a massive ambiguity here. It should read:
> - the physical interrupt is now inactive (EOIed from the guest)
> - the physical interrupt is still active, but its virtual counterpart is
> flagged as "not queued", indicating another interrupt has fired between
> the EOI and the guest exit.
>
> >> + */
> >> + if (!map->shared)
> >> + return !active || !vgic_irq_is_queued(vcpu, vlr.irq);
> >
> > if the second part of the disjunction returns true, doesn't this mean
> > we can potentially write the active+pending state in the LR for a HW
> > interrupt, which is not allowed?
>
> I don't think so. If the physical interrupt has fired, it is because it
> has been EOIed (hence deactivated) first (the active state would
> otherwise prevent it from firing).
>
> >> +
> >> + map->active = active;
> >> +
> >> + if (active) {
> >
> > should you be doing this for a non-shared interrupt?
>
> No. A non-shared interrupt is only dealt with by a single VM, so there
> is no need to clear its state. That would actually completely confuse
> both the device and the VM if you did, because you could now inject a
> new interrupt while the previous one is still in progress. Mayhem follows...
>
> >> ret = irq_set_irqchip_state(map->irq,
> >> IRQCHIP_STATE_ACTIVE,
> >> false);
> >> @@ -1523,6 +1542,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> >> int edge_triggered, level_triggered;
> >> int enabled;
> >> bool ret = true, can_inject = true;
> >> + struct irq_phys_map *map;
> >>
> >> spin_lock(&dist->lock);
> >>
> >> @@ -1569,6 +1589,18 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> >> goto out;
> >> }
> >>
> >> + map = vgic_irq_map_search(vcpu, irq_num);
> >> + if (map && !map->shared) {
> >> + /*
> >> + * We are told to inject a HW irq, so we have to trust
> >> + * the caller that the previous one has been EOIed,
> >> + * and that a new one is now active. Clearing the
> >> + * queued state will have the effect of making it
> >> + * sample-able again.
> >> + */
> >> + vgic_irq_clear_queued(vcpu, irq_num);
> >
> > see my question above about active+pending
>
> I hope I convinced you above. I realize there is one thing missing
> though. Userspace shouldn't be allowed to inject a mapped interrupt.
> Ever. I'll fix that.
>
I think I get it; I think the confusion comes from the fact that all
this depends on the non-shared forwareded interrupts must be configured
with separate deactivate and priority drop (correct?), and since this
support is not in nor is a requirement for this series, it's kind of
hard to understand.
I noticed now that you had in the cover letter that this patch wasn't
intended for merging, but I didn't before...
> >> + }
> >> +
> >> if (!vgic_can_sample_irq(vcpu, irq_num)) {
> >> /*
> >> * Level interrupt in progress, will be picked up
> >> @@ -1662,7 +1694,7 @@ static struct list_head *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
> >> }
> >>
> >> struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> >> - int virt_irq, int irq)
> >> + int virt_irq, int irq, bool shared)
> >> {
> >> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >> struct list_head *root = vgic_get_irq_phys_map(vcpu, virt_irq);
> >> @@ -1691,7 +1723,8 @@ struct irq_phys_map *vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> >> if (map) {
> >> /* Make sure this mapping matches */
> >> if (map->phys_irq != phys_irq ||
> >> - map->irq != irq)
> >> + map->irq != irq ||
> >> + map->shared != shared)
> >
> > this really feels like a BUG() - if we have an existing mapping for the
> > same virtual IRQ, but it's shared in one case and not shared in the
> > other? Shouldn we even allow the caller to get this far?
>
> How early do you want to detect it? All we can do is tell the caller to
> bugger off by returning an error (we return NULL at the moment, meaning
> that the caller doesn't get a mapping at all).
>
fair enough, I hope the caller then has a BUG_ON(mapping == NULL).
-Christoffer
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-08-04 12:26 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-08 17:56 [PATCH v2 00/10] arm/arm64: KVM: Active interrupt state switching for shared devices Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 01/10] arm/arm64: KVM: Fix ordering of timer/GIC on guest entry Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 02/10] arm/arm64: KVM: Move vgic handling to a non-preemptible section Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 03/10] KVM: arm/arm64: vgic: Convert struct vgic_lr to use bitfields Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-21 18:02 ` Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 04/10] KVM: arm/arm64: vgic: Allow HW irq to be encoded in LR Marc Zyngier
2015-07-17 19:50 ` Christoffer Dall
2015-07-21 16:38 ` Marc Zyngier
2015-08-04 12:14 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 05/10] KVM: arm/arm64: vgic: Relax vgic_can_sample_irq for edge IRQs Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 06/10] KVM: arm/arm64: vgic: Allow dynamic mapping of physical/virtual interrupts Marc Zyngier
2015-07-17 21:11 ` Christoffer Dall
2015-07-21 17:17 ` Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 07/10] KVM: arm/arm64: vgic: Allow HW interrupts to be queued to a guest Marc Zyngier
2015-07-17 21:56 ` Christoffer Dall
2015-07-21 17:21 ` Marc Zyngier
2015-07-08 17:56 ` [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get, set}_phys_irq_active Marc Zyngier
2015-07-17 22:15 ` [PATCH v2 08/10] KVM: arm/arm64: vgic: Add vgic_{get,set}_phys_irq_active Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 09/10] KVM: arm/arm64: timer: Allow the timer to control the active state Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-08 17:56 ` [PATCH v2 10/10] KVM: arm/arm64: vgic: Allow non-shared device HW interrupts Marc Zyngier
2015-07-17 22:15 ` Christoffer Dall
2015-07-21 18:01 ` Marc Zyngier
2015-08-04 12:26 ` Christoffer Dall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).