* [PATCH v6 0/8] Handle forwarded level-triggered interrupts
@ 2017-12-04 20:04 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:04 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
This series is an alternative approach to Eric Auger's direct EOI setup
patches [1] in terms of the KVM VGIC support.
The idea is to maintain existing semantics for the VGIC for mapped
level-triggered IRQs and also support the timer using mapped IRQs with
the same VGIC support as VFIO interrupts.
Based on v4.15-rc.
Also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git level-mapped-v6
Changes since v5:
- Rebased on v4.15-rc1
- Changed comment on preemption code as suggested by Andre
- Fixed white space and confusing conditionals as suggested by Drew
Changes since v4:
- Rebased on the timer optimization series merged in the v4.15 merge
window, which caused a fair amount of modifications to patch 3.
- Added a static key to disable the sync operations when no VMs are
using userspace irqchips to further optimize the performance
- Fixed extra semicolon in vgic-mmio.c
- Added commentary as requested during review
- Dropped what was patch 4, because it was merged as part of GICv4
support.
- Factored out the VGIC input level function change as separate patch
(helps bisect and debugging), before providing a function for the
timer.
Changes since v3:
- Added a number of patches and moved patches around a bit.
- Check for uaccesses in the mmio handler functions
- Fixed bugs in the mmio handler functions
Changes since v2:
- Removed patch 5 from v2 and integrating the changes in what's now
patch 5 to make it easier to reuse code when adding VFIO integration.
- Changed the virtual distributor MMIO handling to use the
pending_latch and more closely match the semantics of SPENDR and
CPENDR for both level and edge mapped interrupts.
Changes since v1:
- Added necessary changes to the timer (Patch 1)
- Added handling of guest MMIO accesses to the virtual distributor
(Patch 4)
- Addressed Marc's comments from the initial RFC (mostly renames)
Thanks,
-Christoffer
Christoffer Dall (8):
KVM: arm/arm64: Remove redundant preemptible checks
KVM: arm/arm64: Factor out functionality to get vgic mmio
requester_vcpu
KVM: arm/arm64: Don't cache the timer IRQ level
KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
KVM: arm/arm64: Support a vgic interrupt line level sample function
KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
KVM: arm/arm64: Provide a get_input_level for the arch timer
KVM: arm/arm64: Avoid work when userspace iqchips are not used
include/kvm/arm_arch_timer.h | 2 +
include/kvm/arm_vgic.h | 13 ++++-
virt/kvm/arm/arch_timer.c | 105 +++++++++++++++++++++-----------------
virt/kvm/arm/arm.c | 2 -
virt/kvm/arm/vgic/vgic-mmio.c | 115 ++++++++++++++++++++++++++++++++++--------
virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++
virt/kvm/arm/vgic/vgic.c | 42 +++++++++++++--
virt/kvm/arm/vgic/vgic.h | 8 +++
9 files changed, 270 insertions(+), 75 deletions(-)
--
2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 1/8] KVM: arm/arm64: Remove redundant preemptible checks
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:04 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:04 UTC (permalink / raw)
To: kvmarm; +Cc: kvm, Marc Zyngier, Andre Przywara, linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The __this_cpu_read() and __this_cpu_write() functions already implement
checks for the required preemption levels when using
CONFIG_DEBUG_PREEMPT which gives you nice error messages and such.
Therefore there is no need to explicitly check this using a BUG_ON() in
the code (which we don't do for other uses of per cpu variables either).
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index a6524ff27de4..859ff7e3a1eb 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -71,7 +71,6 @@ static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
{
- BUG_ON(preemptible());
__this_cpu_write(kvm_arm_running_vcpu, vcpu);
}
@@ -81,7 +80,6 @@ static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
*/
struct kvm_vcpu *kvm_arm_get_running_vcpu(void)
{
- BUG_ON(preemptible());
return __this_cpu_read(kvm_arm_running_vcpu);
}
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 1/8] KVM: arm/arm64: Remove redundant preemptible checks
@ 2017-12-04 20:04 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:04 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The __this_cpu_read() and __this_cpu_write() functions already implement
checks for the required preemption levels when using
CONFIG_DEBUG_PREEMPT which gives you nice error messages and such.
Therefore there is no need to explicitly check this using a BUG_ON() in
the code (which we don't do for other uses of per cpu variables either).
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index a6524ff27de4..859ff7e3a1eb 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -71,7 +71,6 @@ static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
{
- BUG_ON(preemptible());
__this_cpu_write(kvm_arm_running_vcpu, vcpu);
}
@@ -81,7 +80,6 @@ static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
*/
struct kvm_vcpu *kvm_arm_get_running_vcpu(void)
{
- BUG_ON(preemptible());
return __this_cpu_read(kvm_arm_running_vcpu);
}
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v6 2/8] KVM: arm/arm64: Factor out functionality to get vgic mmio requester_vcpu
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm
Cc: linux-arm-kernel, kvm, Marc Zyngier, Andre Przywara, Eric Auger,
Christoffer Dall
From: Christoffer Dall <christoffer.dall@linaro.org>
We are about to distinguish between userspace accesses and mmio traps
for a number of the mmio handlers. When the requester vcpu is NULL, it
mens we are handling a userspace acccess.
Factor out the functionality to get the request vcpu into its own
function, mostly so we have a common place to document the semantics of
the return value.
Also take the chance to move the functionality outside of holding a
spinlock and instead explicitly disable and enable preemption. This
supports PREEMPT_RT kernels as well.
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index deb51ee16a3d..747b0a3b4784 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
return value;
}
+/*
+ * This function will return the VCPU that performed the MMIO access and
+ * trapped from twithin the VM, and will return NULL if this is a userspace
+ * access.
+ *
+ * We can disable preemption locally around accessing the per-CPU variable,
+ * and use the resolved vcpu pointer after enabling preemption again, because
+ * even if the current thread is migrated to another CPU, reading the per-CPU
+ * value later will give us the same value as we update the per-CPU variable
+ * in the preempt notifier handlers.
+ */
+static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
+{
+ struct kvm_vcpu *vcpu;
+
+ preempt_disable();
+ vcpu = kvm_arm_get_running_vcpu();
+ preempt_enable();
+ return vcpu;
+}
+
void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
bool new_active_state)
{
- struct kvm_vcpu *requester_vcpu;
unsigned long flags;
- spin_lock_irqsave(&irq->irq_lock, flags);
+ struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
- /*
- * The vcpu parameter here can mean multiple things depending on how
- * this function is called; when handling a trap from the kernel it
- * depends on the GIC version, and these functions are also called as
- * part of save/restore from userspace.
- *
- * Therefore, we have to figure out the requester in a reliable way.
- *
- * When accessing VGIC state from user space, the requester_vcpu is
- * NULL, which is fine, because we guarantee that no VCPUs are running
- * when accessing VGIC state from user space so irq->vcpu->cpu is
- * always -1.
- */
- requester_vcpu = kvm_arm_get_running_vcpu();
+ spin_lock_irqsave(&irq->irq_lock, flags);
/*
* If this virtual IRQ was written into a list register, we
@@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
* vgic_change_active_prepare) and still has to sync back this IRQ,
* so we release and re-acquire the spin_lock to let the other thread
* sync back the IRQ.
+ *
+ * When accessing VGIC state from user space, requester_vcpu is
+ * NULL, which is fine, because we guarantee that no VCPUs are running
+ * when accessing VGIC state from user space so irq->vcpu->cpu is
+ * always -1.
*/
while (irq->vcpu && /* IRQ may have state in an LR somewhere */
irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 2/8] KVM: arm/arm64: Factor out functionality to get vgic mmio requester_vcpu
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
We are about to distinguish between userspace accesses and mmio traps
for a number of the mmio handlers. When the requester vcpu is NULL, it
mens we are handling a userspace acccess.
Factor out the functionality to get the request vcpu into its own
function, mostly so we have a common place to document the semantics of
the return value.
Also take the chance to move the functionality outside of holding a
spinlock and instead explicitly disable and enable preemption. This
supports PREEMPT_RT kernels as well.
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index deb51ee16a3d..747b0a3b4784 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
return value;
}
+/*
+ * This function will return the VCPU that performed the MMIO access and
+ * trapped from twithin the VM, and will return NULL if this is a userspace
+ * access.
+ *
+ * We can disable preemption locally around accessing the per-CPU variable,
+ * and use the resolved vcpu pointer after enabling preemption again, because
+ * even if the current thread is migrated to another CPU, reading the per-CPU
+ * value later will give us the same value as we update the per-CPU variable
+ * in the preempt notifier handlers.
+ */
+static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
+{
+ struct kvm_vcpu *vcpu;
+
+ preempt_disable();
+ vcpu = kvm_arm_get_running_vcpu();
+ preempt_enable();
+ return vcpu;
+}
+
void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
bool new_active_state)
{
- struct kvm_vcpu *requester_vcpu;
unsigned long flags;
- spin_lock_irqsave(&irq->irq_lock, flags);
+ struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
- /*
- * The vcpu parameter here can mean multiple things depending on how
- * this function is called; when handling a trap from the kernel it
- * depends on the GIC version, and these functions are also called as
- * part of save/restore from userspace.
- *
- * Therefore, we have to figure out the requester in a reliable way.
- *
- * When accessing VGIC state from user space, the requester_vcpu is
- * NULL, which is fine, because we guarantee that no VCPUs are running
- * when accessing VGIC state from user space so irq->vcpu->cpu is
- * always -1.
- */
- requester_vcpu = kvm_arm_get_running_vcpu();
+ spin_lock_irqsave(&irq->irq_lock, flags);
/*
* If this virtual IRQ was written into a list register, we
@@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
* vgic_change_active_prepare) and still has to sync back this IRQ,
* so we release and re-acquire the spin_lock to let the other thread
* sync back the IRQ.
+ *
+ * When accessing VGIC state from user space, requester_vcpu is
+ * NULL, which is fine, because we guarantee that no VCPUs are running
+ * when accessing VGIC state from user space so irq->vcpu->cpu is
+ * always -1.
*/
while (irq->vcpu && /* IRQ may have state in an LR somewhere */
irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v6 2/8] KVM: arm/arm64: Factor out functionality to get vgic mmio requester_vcpu
2017-12-04 20:05 ` Christoffer Dall
@ 2017-12-05 13:46 ` Yury Norov
-1 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 13:46 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvm, Marc Zyngier, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Dec 04, 2017 at 09:05:00PM +0100, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> We are about to distinguish between userspace accesses and mmio traps
> for a number of the mmio handlers. When the requester vcpu is NULL, it
> mens we are handling a userspace acccess.
Typo: means?
> Factor out the functionality to get the request vcpu into its own
> function, mostly so we have a common place to document the semantics of
> the return value.
>
> Also take the chance to move the functionality outside of holding a
> spinlock and instead explicitly disable and enable preemption. This
> supports PREEMPT_RT kernels as well.
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index deb51ee16a3d..747b0a3b4784 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +/*
> + * This function will return the VCPU that performed the MMIO access and
> + * trapped from twithin the VM, and will return NULL if this is a userspace
Typo: from within?
> + * access.
> + *
> + * We can disable preemption locally around accessing the per-CPU variable,
> + * and use the resolved vcpu pointer after enabling preemption again, because
> + * even if the current thread is migrated to another CPU, reading the per-CPU
> + * value later will give us the same value as we update the per-CPU variable
> + * in the preempt notifier handlers.
> + */
> +static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> +{
> + struct kvm_vcpu *vcpu;
> +
> + preempt_disable();
> + vcpu = kvm_arm_get_running_vcpu();
> + preempt_enable();
> + return vcpu;
> +}
> +
> void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> @@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> bool new_active_state)
> {
> - struct kvm_vcpu *requester_vcpu;
> unsigned long flags;
> - spin_lock_irqsave(&irq->irq_lock, flags);
> + struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
>
> - /*
> - * The vcpu parameter here can mean multiple things depending on how
> - * this function is called; when handling a trap from the kernel it
> - * depends on the GIC version, and these functions are also called as
> - * part of save/restore from userspace.
> - *
> - * Therefore, we have to figure out the requester in a reliable way.
> - *
> - * When accessing VGIC state from user space, the requester_vcpu is
> - * NULL, which is fine, because we guarantee that no VCPUs are running
> - * when accessing VGIC state from user space so irq->vcpu->cpu is
> - * always -1.
> - */
> - requester_vcpu = kvm_arm_get_running_vcpu();
> + spin_lock_irqsave(&irq->irq_lock, flags);
>
> /*
> * If this virtual IRQ was written into a list register, we
> @@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> * vgic_change_active_prepare) and still has to sync back this IRQ,
> * so we release and re-acquire the spin_lock to let the other thread
> * sync back the IRQ.
> + *
> + * When accessing VGIC state from user space, requester_vcpu is
> + * NULL, which is fine, because we guarantee that no VCPUs are running
> + * when accessing VGIC state from user space so irq->vcpu->cpu is
> + * always -1.
> */
> while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */
> --
> 2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 2/8] KVM: arm/arm64: Factor out functionality to get vgic mmio requester_vcpu
@ 2017-12-05 13:46 ` Yury Norov
0 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 13:46 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 04, 2017 at 09:05:00PM +0100, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> We are about to distinguish between userspace accesses and mmio traps
> for a number of the mmio handlers. When the requester vcpu is NULL, it
> mens we are handling a userspace acccess.
Typo: means?
> Factor out the functionality to get the request vcpu into its own
> function, mostly so we have a common place to document the semantics of
> the return value.
>
> Also take the chance to move the functionality outside of holding a
> spinlock and instead explicitly disable and enable preemption. This
> supports PREEMPT_RT kernels as well.
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index deb51ee16a3d..747b0a3b4784 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +/*
> + * This function will return the VCPU that performed the MMIO access and
> + * trapped from twithin the VM, and will return NULL if this is a userspace
Typo: from within?
> + * access.
> + *
> + * We can disable preemption locally around accessing the per-CPU variable,
> + * and use the resolved vcpu pointer after enabling preemption again, because
> + * even if the current thread is migrated to another CPU, reading the per-CPU
> + * value later will give us the same value as we update the per-CPU variable
> + * in the preempt notifier handlers.
> + */
> +static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> +{
> + struct kvm_vcpu *vcpu;
> +
> + preempt_disable();
> + vcpu = kvm_arm_get_running_vcpu();
> + preempt_enable();
> + return vcpu;
> +}
> +
> void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> @@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> bool new_active_state)
> {
> - struct kvm_vcpu *requester_vcpu;
> unsigned long flags;
> - spin_lock_irqsave(&irq->irq_lock, flags);
> + struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
>
> - /*
> - * The vcpu parameter here can mean multiple things depending on how
> - * this function is called; when handling a trap from the kernel it
> - * depends on the GIC version, and these functions are also called as
> - * part of save/restore from userspace.
> - *
> - * Therefore, we have to figure out the requester in a reliable way.
> - *
> - * When accessing VGIC state from user space, the requester_vcpu is
> - * NULL, which is fine, because we guarantee that no VCPUs are running
> - * when accessing VGIC state from user space so irq->vcpu->cpu is
> - * always -1.
> - */
> - requester_vcpu = kvm_arm_get_running_vcpu();
> + spin_lock_irqsave(&irq->irq_lock, flags);
>
> /*
> * If this virtual IRQ was written into a list register, we
> @@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> * vgic_change_active_prepare) and still has to sync back this IRQ,
> * so we release and re-acquire the spin_lock to let the other thread
> * sync back the IRQ.
> + *
> + * When accessing VGIC state from user space, requester_vcpu is
> + * NULL, which is fine, because we guarantee that no VCPUs are running
> + * when accessing VGIC state from user space so irq->vcpu->cpu is
> + * always -1.
> */
> while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */
> --
> 2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v6 2/8] KVM: arm/arm64: Factor out functionality to get vgic mmio requester_vcpu
2017-12-05 13:46 ` Yury Norov
@ 2017-12-06 10:54 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-06 10:54 UTC (permalink / raw)
To: Yury Norov
Cc: Christoffer Dall, kvm, Marc Zyngier, Andre Przywara, kvmarm,
linux-arm-kernel
On Tue, Dec 05, 2017 at 04:46:08PM +0300, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:00PM +0100, Christoffer Dall wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > We are about to distinguish between userspace accesses and mmio traps
> > for a number of the mmio handlers. When the requester vcpu is NULL, it
> > mens we are handling a userspace acccess.
>
> Typo: means?
>
yes
> > Factor out the functionality to get the request vcpu into its own
> > function, mostly so we have a common place to document the semantics of
> > the return value.
> >
> > Also take the chance to move the functionality outside of holding a
> > spinlock and instead explicitly disable and enable preemption. This
> > supports PREEMPT_RT kernels as well.
> >
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
> > 1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index deb51ee16a3d..747b0a3b4784 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> > return value;
> > }
> >
> > +/*
> > + * This function will return the VCPU that performed the MMIO access and
> > + * trapped from twithin the VM, and will return NULL if this is a userspace
>
> Typo: from within?
>
yes
> > + * access.
> > + *
> > + * We can disable preemption locally around accessing the per-CPU variable,
> > + * and use the resolved vcpu pointer after enabling preemption again, because
> > + * even if the current thread is migrated to another CPU, reading the per-CPU
> > + * value later will give us the same value as we update the per-CPU variable
> > + * in the preempt notifier handlers.
> > + */
> > +static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> > +{
> > + struct kvm_vcpu *vcpu;
> > +
> > + preempt_disable();
> > + vcpu = kvm_arm_get_running_vcpu();
> > + preempt_enable();
> > + return vcpu;
> > +}
> > +
> > void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len,
> > unsigned long val)
> > @@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> > static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > bool new_active_state)
> > {
> > - struct kvm_vcpu *requester_vcpu;
> > unsigned long flags;
> > - spin_lock_irqsave(&irq->irq_lock, flags);
> > + struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
> >
> > - /*
> > - * The vcpu parameter here can mean multiple things depending on how
> > - * this function is called; when handling a trap from the kernel it
> > - * depends on the GIC version, and these functions are also called as
> > - * part of save/restore from userspace.
> > - *
> > - * Therefore, we have to figure out the requester in a reliable way.
> > - *
> > - * When accessing VGIC state from user space, the requester_vcpu is
> > - * NULL, which is fine, because we guarantee that no VCPUs are running
> > - * when accessing VGIC state from user space so irq->vcpu->cpu is
> > - * always -1.
> > - */
> > - requester_vcpu = kvm_arm_get_running_vcpu();
> > + spin_lock_irqsave(&irq->irq_lock, flags);
> >
> > /*
> > * If this virtual IRQ was written into a list register, we
> > @@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > * vgic_change_active_prepare) and still has to sync back this IRQ,
> > * so we release and re-acquire the spin_lock to let the other thread
> > * sync back the IRQ.
> > + *
> > + * When accessing VGIC state from user space, requester_vcpu is
> > + * NULL, which is fine, because we guarantee that no VCPUs are running
> > + * when accessing VGIC state from user space so irq->vcpu->cpu is
> > + * always -1.
> > */
> > while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> > irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */
> > --
> > 2.14.2
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 2/8] KVM: arm/arm64: Factor out functionality to get vgic mmio requester_vcpu
@ 2017-12-06 10:54 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-06 10:54 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 05, 2017 at 04:46:08PM +0300, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:00PM +0100, Christoffer Dall wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > We are about to distinguish between userspace accesses and mmio traps
> > for a number of the mmio handlers. When the requester vcpu is NULL, it
> > mens we are handling a userspace acccess.
>
> Typo: means?
>
yes
> > Factor out the functionality to get the request vcpu into its own
> > function, mostly so we have a common place to document the semantics of
> > the return value.
> >
> > Also take the chance to move the functionality outside of holding a
> > spinlock and instead explicitly disable and enable preemption. This
> > supports PREEMPT_RT kernels as well.
> >
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
> > 1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index deb51ee16a3d..747b0a3b4784 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> > return value;
> > }
> >
> > +/*
> > + * This function will return the VCPU that performed the MMIO access and
> > + * trapped from twithin the VM, and will return NULL if this is a userspace
>
> Typo: from within?
>
yes
> > + * access.
> > + *
> > + * We can disable preemption locally around accessing the per-CPU variable,
> > + * and use the resolved vcpu pointer after enabling preemption again, because
> > + * even if the current thread is migrated to another CPU, reading the per-CPU
> > + * value later will give us the same value as we update the per-CPU variable
> > + * in the preempt notifier handlers.
> > + */
> > +static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> > +{
> > + struct kvm_vcpu *vcpu;
> > +
> > + preempt_disable();
> > + vcpu = kvm_arm_get_running_vcpu();
> > + preempt_enable();
> > + return vcpu;
> > +}
> > +
> > void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len,
> > unsigned long val)
> > @@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> > static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > bool new_active_state)
> > {
> > - struct kvm_vcpu *requester_vcpu;
> > unsigned long flags;
> > - spin_lock_irqsave(&irq->irq_lock, flags);
> > + struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
> >
> > - /*
> > - * The vcpu parameter here can mean multiple things depending on how
> > - * this function is called; when handling a trap from the kernel it
> > - * depends on the GIC version, and these functions are also called as
> > - * part of save/restore from userspace.
> > - *
> > - * Therefore, we have to figure out the requester in a reliable way.
> > - *
> > - * When accessing VGIC state from user space, the requester_vcpu is
> > - * NULL, which is fine, because we guarantee that no VCPUs are running
> > - * when accessing VGIC state from user space so irq->vcpu->cpu is
> > - * always -1.
> > - */
> > - requester_vcpu = kvm_arm_get_running_vcpu();
> > + spin_lock_irqsave(&irq->irq_lock, flags);
> >
> > /*
> > * If this virtual IRQ was written into a list register, we
> > @@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > * vgic_change_active_prepare) and still has to sync back this IRQ,
> > * so we release and re-acquire the spin_lock to let the other thread
> > * sync back the IRQ.
> > + *
> > + * When accessing VGIC state from user space, requester_vcpu is
> > + * NULL, which is fine, because we guarantee that no VCPUs are running
> > + * when accessing VGIC state from user space so irq->vcpu->cpu is
> > + * always -1.
> > */
> > while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> > irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */
> > --
> > 2.14.2
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v6 3/8] KVM: arm/arm64: Don't cache the timer IRQ level
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm
Cc: linux-arm-kernel, kvm, Marc Zyngier, Andre Przywara, Eric Auger,
Christoffer Dall
From: Christoffer Dall <christoffer.dall@linaro.org>
The timer was modeled after a strict idea of modelling an interrupt line
level in software, meaning that only transitions in the level needed to
be reported to the VGIC. This works well for the timer, because the
arch timer code is in complete control of the device and can track the
transitions of the line.
However, as we are about to support using the HW bit in the VGIC not
just for the timer, but also for VFIO which cannot track transitions of
the interrupt line, we have to decide on an interface for level
triggered mapped interrupts to the GIC, which both the timer and VFIO
can use.
VFIO only sees an asserting transition of the physical interrupt line,
and tells the VGIC when that happens. That means that part of the
interrupt flow is offloaded to the hardware.
To use the same interface for VFIO devices and the timer, we therefore
have to change the timer (we cannot change VFIO because it doesn't know
the details of the device it is assigning to a VM).
Luckily, changing the timer is simple, we just need to stop 'caching'
the line level, but instead let the VGIC know the state of the timer
every time there is a potential change in the line level, and when the
line level should be asserted from the timer ISR. The VGIC can ignore
extra notifications using its validate mechanism.
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arch_timer.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 4151250ce8da..dd5aca05c500 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -99,11 +99,9 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
}
vtimer = vcpu_vtimer(vcpu);
- if (!vtimer->irq.level) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- if (kvm_timer_irq_can_fire(vtimer))
- kvm_timer_update_irq(vcpu, true, vtimer);
- }
+ vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
+ if (kvm_timer_irq_can_fire(vtimer))
+ kvm_timer_update_irq(vcpu, true, vtimer);
if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
kvm_vtimer_update_mask_user(vcpu);
@@ -324,12 +322,20 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+ bool level;
if (unlikely(!timer->enabled))
return;
- if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
- kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
+ /*
+ * The vtimer virtual interrupt is a 'mapped' interrupt, meaning part
+ * of its lifecycle is offloaded to the hardware, and we therefore may
+ * not have lowered the irq.level value before having to signal a new
+ * interrupt, but have to signal an interrupt every time the level is
+ * asserted.
+ */
+ level = kvm_timer_should_fire(vtimer);
+ kvm_timer_update_irq(vcpu, level, vtimer);
if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 3/8] KVM: arm/arm64: Don't cache the timer IRQ level
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The timer was modeled after a strict idea of modelling an interrupt line
level in software, meaning that only transitions in the level needed to
be reported to the VGIC. This works well for the timer, because the
arch timer code is in complete control of the device and can track the
transitions of the line.
However, as we are about to support using the HW bit in the VGIC not
just for the timer, but also for VFIO which cannot track transitions of
the interrupt line, we have to decide on an interface for level
triggered mapped interrupts to the GIC, which both the timer and VFIO
can use.
VFIO only sees an asserting transition of the physical interrupt line,
and tells the VGIC when that happens. That means that part of the
interrupt flow is offloaded to the hardware.
To use the same interface for VFIO devices and the timer, we therefore
have to change the timer (we cannot change VFIO because it doesn't know
the details of the device it is assigning to a VM).
Luckily, changing the timer is simple, we just need to stop 'caching'
the line level, but instead let the VGIC know the state of the timer
every time there is a potential change in the line level, and when the
line level should be asserted from the timer ISR. The VGIC can ignore
extra notifications using its validate mechanism.
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arch_timer.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 4151250ce8da..dd5aca05c500 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -99,11 +99,9 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
}
vtimer = vcpu_vtimer(vcpu);
- if (!vtimer->irq.level) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- if (kvm_timer_irq_can_fire(vtimer))
- kvm_timer_update_irq(vcpu, true, vtimer);
- }
+ vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
+ if (kvm_timer_irq_can_fire(vtimer))
+ kvm_timer_update_irq(vcpu, true, vtimer);
if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
kvm_vtimer_update_mask_user(vcpu);
@@ -324,12 +322,20 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+ bool level;
if (unlikely(!timer->enabled))
return;
- if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
- kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
+ /*
+ * The vtimer virtual interrupt is a 'mapped' interrupt, meaning part
+ * of its lifecycle is offloaded to the hardware, and we therefore may
+ * not have lowered the irq.level value before having to signal a new
+ * interrupt, but have to signal an interrupt every time the level is
+ * asserted.
+ */
+ level = kvm_timer_should_fire(vtimer);
+ kvm_timer_update_irq(vcpu, level, vtimer);
if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v6 4/8] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm; +Cc: kvm, Marc Zyngier, Andre Przywara, linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
Level-triggered mapped IRQs are special because we only observe rising
edges as input to the VGIC, and we don't set the EOI flag and therefore
are not told when the level goes down, so that we can re-queue a new
interrupt when the level goes up.
One way to solve this problem is to side-step the logic of the VGIC and
special case the validation in the injection path, but it has the
unfortunate drawback of having to peak into the physical GIC state
whenever we want to know if the interrupt is pending on the virtual
distributor.
Instead, we can maintain the current semantics of a level triggered
interrupt by sort of treating it as an edge-triggered interrupt,
following from the fact that we only observe an asserting edge. This
requires us to be a bit careful when populating the LRs and when folding
the state back in though:
* We lower the line level when populating the LR, so that when
subsequently observing an asserting edge, the VGIC will do the right
thing.
* If the guest never acked the interrupt while running (for example if
it had masked interrupts at the CPU level while running), we have
to preserve the pending state of the LR and move it back to the
line_level field of the struct irq when folding LR state.
If the guest never acked the interrupt while running, but changed the
device state and lowered the line (again with interrupts masked) then
we need to observe this change in the line_level.
Both of the above situations are solved by sampling the physical line
and set the line level when folding the LR back.
* Finally, if the guest never acked the interrupt while running and
sampling the line reveals that the device state has changed and the
line has been lowered, we must clear the physical active state, since
we will otherwise never be told when the interrupt becomes asserted
again.
This has the added benefit of making the timer optimization patches
(https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026343.html) a
bit simpler, because the timer code doesn't have to clear the active
state on the sync anymore. It also potentially improves the performance
of the timer implementation because the GIC knows the state or the LR
and only needs to clear the
active state when the pending bit in the LR is still set, where the
timer has to always clear it when returning from running the guest with
an injected timer interrupt.
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.c | 23 +++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 7 +++++++
4 files changed, 88 insertions(+)
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 80897102da26..c32d7b93ffd1 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -105,6 +105,26 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
irq->pending_latch = false;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only
+ * observe rising edges as input to the VGIC.
+ *
+ * If the guest never acked the interrupt we have to sample
+ * the physical line and set the line level, because the
+ * device state could have changed or we simply need to
+ * process the still pending interrupt later.
+ *
+ * If this causes us to lower the level, we have to also clear
+ * the physical active state, since we will otherwise never be
+ * told when the interrupt becomes asserted again.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
+ irq->line_level = vgic_get_phys_line_level(irq);
+
+ if (!irq->line_level)
+ vgic_irq_set_phys_active(irq, false);
+ }
+
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
}
@@ -162,6 +182,15 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
val |= GICH_LR_EOI;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only observe
+ * rising edges as input to the VGIC. We therefore lower the line
+ * level here, so that we can take new virtual IRQs. See
+ * vgic_v2_fold_lr_state for more info.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT))
+ irq->line_level = false;
+
/* The GICv2 LR only holds five bits of priority. */
val |= (irq->priority >> 3) << GICH_LR_PRIORITY_SHIFT;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 2f05f732d3fd..a14423a0d383 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -96,6 +96,26 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
irq->pending_latch = false;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only
+ * observe rising edges as input to the VGIC.
+ *
+ * If the guest never acked the interrupt we have to sample
+ * the physical line and set the line level, because the
+ * device state could have changed or we simply need to
+ * process the still pending interrupt later.
+ *
+ * If this causes us to lower the level, we have to also clear
+ * the physical active state, since we will otherwise never be
+ * told when the interrupt becomes asserted again.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) {
+ irq->line_level = vgic_get_phys_line_level(irq);
+
+ if (!irq->line_level)
+ vgic_irq_set_phys_active(irq, false);
+ }
+
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
}
@@ -145,6 +165,15 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
val |= ICH_LR_EOI;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only observe
+ * rising edges as input to the VGIC. We therefore lower the line
+ * level here, so that we can take new virtual IRQs. See
+ * vgic_v3_fold_lr_state for more info.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
+ irq->line_level = false;
+
/*
* We currently only support Group1 interrupts, which is a
* known defect. This needs to be addressed at some point.
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index b168a328a9e0..607cbbc27a1c 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -144,6 +144,29 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
kfree(irq);
}
+/* Get the input level of a mapped IRQ directly from the physical GIC */
+bool vgic_get_phys_line_level(struct vgic_irq *irq)
+{
+ bool line_level;
+
+ BUG_ON(!irq->hw);
+
+ WARN_ON(irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ &line_level));
+ return line_level;
+}
+
+/* Set/Clear the physical active state */
+void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active)
+{
+
+ BUG_ON(!irq->hw);
+ WARN_ON(irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_ACTIVE,
+ active));
+}
+
/**
* kvm_vgic_target_oracle - compute the target vcpu for an irq
*
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index efbcf8f96f9c..d0787983a357 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -104,6 +104,11 @@ static inline bool irq_is_pending(struct vgic_irq *irq)
return irq->pending_latch || irq->line_level;
}
+static inline bool vgic_irq_is_mapped_level(struct vgic_irq *irq)
+{
+ return irq->config == VGIC_CONFIG_LEVEL && irq->hw;
+}
+
/*
* This struct provides an intermediate representation of the fields contained
* in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
@@ -140,6 +145,8 @@ vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
u32 intid);
void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
+bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
unsigned long flags);
void vgic_kick_vcpus(struct kvm *kvm);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 4/8] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
Level-triggered mapped IRQs are special because we only observe rising
edges as input to the VGIC, and we don't set the EOI flag and therefore
are not told when the level goes down, so that we can re-queue a new
interrupt when the level goes up.
One way to solve this problem is to side-step the logic of the VGIC and
special case the validation in the injection path, but it has the
unfortunate drawback of having to peak into the physical GIC state
whenever we want to know if the interrupt is pending on the virtual
distributor.
Instead, we can maintain the current semantics of a level triggered
interrupt by sort of treating it as an edge-triggered interrupt,
following from the fact that we only observe an asserting edge. This
requires us to be a bit careful when populating the LRs and when folding
the state back in though:
* We lower the line level when populating the LR, so that when
subsequently observing an asserting edge, the VGIC will do the right
thing.
* If the guest never acked the interrupt while running (for example if
it had masked interrupts at the CPU level while running), we have
to preserve the pending state of the LR and move it back to the
line_level field of the struct irq when folding LR state.
If the guest never acked the interrupt while running, but changed the
device state and lowered the line (again with interrupts masked) then
we need to observe this change in the line_level.
Both of the above situations are solved by sampling the physical line
and set the line level when folding the LR back.
* Finally, if the guest never acked the interrupt while running and
sampling the line reveals that the device state has changed and the
line has been lowered, we must clear the physical active state, since
we will otherwise never be told when the interrupt becomes asserted
again.
This has the added benefit of making the timer optimization patches
(https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026343.html) a
bit simpler, because the timer code doesn't have to clear the active
state on the sync anymore. It also potentially improves the performance
of the timer implementation because the GIC knows the state or the LR
and only needs to clear the
active state when the pending bit in the LR is still set, where the
timer has to always clear it when returning from running the guest with
an injected timer interrupt.
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.c | 23 +++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 7 +++++++
4 files changed, 88 insertions(+)
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 80897102da26..c32d7b93ffd1 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -105,6 +105,26 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
irq->pending_latch = false;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only
+ * observe rising edges as input to the VGIC.
+ *
+ * If the guest never acked the interrupt we have to sample
+ * the physical line and set the line level, because the
+ * device state could have changed or we simply need to
+ * process the still pending interrupt later.
+ *
+ * If this causes us to lower the level, we have to also clear
+ * the physical active state, since we will otherwise never be
+ * told when the interrupt becomes asserted again.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
+ irq->line_level = vgic_get_phys_line_level(irq);
+
+ if (!irq->line_level)
+ vgic_irq_set_phys_active(irq, false);
+ }
+
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
}
@@ -162,6 +182,15 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
val |= GICH_LR_EOI;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only observe
+ * rising edges as input to the VGIC. We therefore lower the line
+ * level here, so that we can take new virtual IRQs. See
+ * vgic_v2_fold_lr_state for more info.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT))
+ irq->line_level = false;
+
/* The GICv2 LR only holds five bits of priority. */
val |= (irq->priority >> 3) << GICH_LR_PRIORITY_SHIFT;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 2f05f732d3fd..a14423a0d383 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -96,6 +96,26 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
irq->pending_latch = false;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only
+ * observe rising edges as input to the VGIC.
+ *
+ * If the guest never acked the interrupt we have to sample
+ * the physical line and set the line level, because the
+ * device state could have changed or we simply need to
+ * process the still pending interrupt later.
+ *
+ * If this causes us to lower the level, we have to also clear
+ * the physical active state, since we will otherwise never be
+ * told when the interrupt becomes asserted again.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) {
+ irq->line_level = vgic_get_phys_line_level(irq);
+
+ if (!irq->line_level)
+ vgic_irq_set_phys_active(irq, false);
+ }
+
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
}
@@ -145,6 +165,15 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
val |= ICH_LR_EOI;
}
+ /*
+ * Level-triggered mapped IRQs are special because we only observe
+ * rising edges as input to the VGIC. We therefore lower the line
+ * level here, so that we can take new virtual IRQs. See
+ * vgic_v3_fold_lr_state for more info.
+ */
+ if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
+ irq->line_level = false;
+
/*
* We currently only support Group1 interrupts, which is a
* known defect. This needs to be addressed at some point.
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index b168a328a9e0..607cbbc27a1c 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -144,6 +144,29 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
kfree(irq);
}
+/* Get the input level of a mapped IRQ directly from the physical GIC */
+bool vgic_get_phys_line_level(struct vgic_irq *irq)
+{
+ bool line_level;
+
+ BUG_ON(!irq->hw);
+
+ WARN_ON(irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ &line_level));
+ return line_level;
+}
+
+/* Set/Clear the physical active state */
+void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active)
+{
+
+ BUG_ON(!irq->hw);
+ WARN_ON(irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_ACTIVE,
+ active));
+}
+
/**
* kvm_vgic_target_oracle - compute the target vcpu for an irq
*
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index efbcf8f96f9c..d0787983a357 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -104,6 +104,11 @@ static inline bool irq_is_pending(struct vgic_irq *irq)
return irq->pending_latch || irq->line_level;
}
+static inline bool vgic_irq_is_mapped_level(struct vgic_irq *irq)
+{
+ return irq->config == VGIC_CONFIG_LEVEL && irq->hw;
+}
+
/*
* This struct provides an intermediate representation of the fields contained
* in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
@@ -140,6 +145,8 @@ vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
u32 intid);
void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
+bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
unsigned long flags);
void vgic_kick_vcpus(struct kvm *kvm);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v6 5/8] KVM: arm/arm64: Support a vgic interrupt line level sample function
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm; +Cc: kvm, Marc Zyngier, Andre Przywara, linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The GIC sometimes need to sample the physical line of a mapped
interrupt. As we know this to be notoriously slow, provide a callback
function for devices (such as the timer) which can do this much faster
than talking to the distributor, for example by comparing a few
in-memory values. Fall back to the good old method of poking the
physical GIC if no callback is provided.
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
include/kvm/arm_vgic.h | 13 ++++++++++++-
virt/kvm/arm/arch_timer.c | 3 ++-
virt/kvm/arm/vgic/vgic.c | 12 +++++++++---
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 8c896540a72c..cdbd142ca7f2 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -130,6 +130,17 @@ struct vgic_irq {
u8 priority;
enum vgic_irq_config config; /* Level or edge */
+ /*
+ * Callback function pointer to in-kernel devices that can tell us the
+ * state of the input level of mapped level-triggered IRQ faster than
+ * peaking into the physical GIC.
+ *
+ * Always called in non-preemptible section and the functions can use
+ * kvm_arm_get_running_vcpu() to get the vcpu pointer for private
+ * IRQs.
+ */
+ bool (*get_input_level)(int vintid);
+
void *owner; /* Opaque pointer to reserve an interrupt
for in-kernel devices. */
};
@@ -331,7 +342,7 @@ void kvm_vgic_init_cpu_hardware(void);
int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
bool level, void *owner);
int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
- u32 vintid);
+ u32 vintid, bool (*get_input_level)(int vindid));
int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index dd5aca05c500..e78ba5e20f74 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -840,7 +840,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return -EINVAL;
}
- ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
+ ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
+ NULL);
if (ret)
return ret;
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 607cbbc27a1c..eadabb249d2a 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -151,6 +151,9 @@ bool vgic_get_phys_line_level(struct vgic_irq *irq)
BUG_ON(!irq->hw);
+ if (irq->get_input_level)
+ return irq->get_input_level(irq->intid);
+
WARN_ON(irq_get_irqchip_state(irq->host_irq,
IRQCHIP_STATE_PENDING,
&line_level));
@@ -436,7 +439,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
/* @irq->irq_lock must be held */
static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
- unsigned int host_irq)
+ unsigned int host_irq,
+ bool (*get_input_level)(int vindid))
{
struct irq_desc *desc;
struct irq_data *data;
@@ -456,6 +460,7 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
irq->hw = true;
irq->host_irq = host_irq;
irq->hwintid = data->hwirq;
+ irq->get_input_level = get_input_level;
return 0;
}
@@ -464,10 +469,11 @@ static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
{
irq->hw = false;
irq->hwintid = 0;
+ irq->get_input_level = NULL;
}
int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
- u32 vintid)
+ u32 vintid, bool (*get_input_level)(int vindid))
{
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
unsigned long flags;
@@ -476,7 +482,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
BUG_ON(!irq);
spin_lock_irqsave(&irq->irq_lock, flags);
- ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
+ ret = kvm_vgic_map_irq(vcpu, irq, host_irq, get_input_level);
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 5/8] KVM: arm/arm64: Support a vgic interrupt line level sample function
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The GIC sometimes need to sample the physical line of a mapped
interrupt. As we know this to be notoriously slow, provide a callback
function for devices (such as the timer) which can do this much faster
than talking to the distributor, for example by comparing a few
in-memory values. Fall back to the good old method of poking the
physical GIC if no callback is provided.
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
include/kvm/arm_vgic.h | 13 ++++++++++++-
virt/kvm/arm/arch_timer.c | 3 ++-
virt/kvm/arm/vgic/vgic.c | 12 +++++++++---
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 8c896540a72c..cdbd142ca7f2 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -130,6 +130,17 @@ struct vgic_irq {
u8 priority;
enum vgic_irq_config config; /* Level or edge */
+ /*
+ * Callback function pointer to in-kernel devices that can tell us the
+ * state of the input level of mapped level-triggered IRQ faster than
+ * peaking into the physical GIC.
+ *
+ * Always called in non-preemptible section and the functions can use
+ * kvm_arm_get_running_vcpu() to get the vcpu pointer for private
+ * IRQs.
+ */
+ bool (*get_input_level)(int vintid);
+
void *owner; /* Opaque pointer to reserve an interrupt
for in-kernel devices. */
};
@@ -331,7 +342,7 @@ void kvm_vgic_init_cpu_hardware(void);
int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
bool level, void *owner);
int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
- u32 vintid);
+ u32 vintid, bool (*get_input_level)(int vindid));
int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index dd5aca05c500..e78ba5e20f74 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -840,7 +840,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return -EINVAL;
}
- ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
+ ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
+ NULL);
if (ret)
return ret;
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 607cbbc27a1c..eadabb249d2a 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -151,6 +151,9 @@ bool vgic_get_phys_line_level(struct vgic_irq *irq)
BUG_ON(!irq->hw);
+ if (irq->get_input_level)
+ return irq->get_input_level(irq->intid);
+
WARN_ON(irq_get_irqchip_state(irq->host_irq,
IRQCHIP_STATE_PENDING,
&line_level));
@@ -436,7 +439,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
/* @irq->irq_lock must be held */
static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
- unsigned int host_irq)
+ unsigned int host_irq,
+ bool (*get_input_level)(int vindid))
{
struct irq_desc *desc;
struct irq_data *data;
@@ -456,6 +460,7 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
irq->hw = true;
irq->host_irq = host_irq;
irq->hwintid = data->hwirq;
+ irq->get_input_level = get_input_level;
return 0;
}
@@ -464,10 +469,11 @@ static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
{
irq->hw = false;
irq->hwintid = 0;
+ irq->get_input_level = NULL;
}
int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
- u32 vintid)
+ u32 vintid, bool (*get_input_level)(int vindid))
{
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
unsigned long flags;
@@ -476,7 +482,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
BUG_ON(!irq);
spin_lock_irqsave(&irq->irq_lock, flags);
- ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
+ ret = kvm_vgic_map_irq(vcpu, irq, host_irq, get_input_level);
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm; +Cc: kvm, Marc Zyngier, Andre Przywara, linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
For mapped IRQs (with the HW bit set in the LR) we have to follow some
rules of the architecture. One of these rules is that VM must not be
allowed to deactivate a virtual interrupt with the HW bit set unless the
physical interrupt is also active.
This works fine when injecting mapped interrupts, because we leave it up
to the injector to either set EOImode==1 or manually set the active
state of the physical interrupt.
However, the guest can set virtual interrupt to be pending or active by
writing to the virtual distributor, which could lead to deactivating a
virtual interrupt with the HW bit set without the physical interrupt
being active.
We could set the physical interrupt to active whenever we are about to
enter the VM with a HW interrupt either pending or active, but that
would be really slow, especially on GICv2. So we take the long way
around and do the hard work when needed, which is expected to be
extremely rare.
When the VM sets the pending state for a HW interrupt on the virtual
distributor we set the active state on the physical distributor, because
the virtual interrupt can become active and then the guest can
deactivate it.
When the VM clears the pending state we also clear it on the physical
side, because the injector might otherwise raise the interrupt. We also
clear the physical active state when the virtual interrupt is not
active, since otherwise a SPEND/CPEND sequence from the guest would
prevent signaling of future interrupts.
Changing the state of mapped interrupts from userspace is not supported,
and it's expected that userspace unmaps devices from VFIO before
attempting to set the interrupt state, because the interrupt state is
driven by hardware.
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic.c | 7 +++++
virt/kvm/arm/vgic/vgic.h | 1 +
3 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 747b0a3b4784..8d173d99a7a4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -16,6 +16,7 @@
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <kvm/iodev.h>
+#include <kvm/arm_arch_timer.h>
#include <kvm/arm_vgic.h>
#include "vgic.h"
@@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
return vcpu;
}
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+ bool is_uaccess)
+{
+ if (is_uaccess)
+ return;
+
+ irq->pending_latch = true;
+ vgic_irq_set_phys_active(irq, true);
+}
+
void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
{
+ bool is_uaccess = !vgic_get_mmio_requester_vcpu();
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
int i;
unsigned long flags;
@@ -155,17 +168,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
spin_lock_irqsave(&irq->irq_lock, flags);
- irq->pending_latch = true;
-
+ if (irq->hw)
+ vgic_hw_irq_spending(vcpu, irq, is_uaccess);
+ else
+ irq->pending_latch = true;
vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
vgic_put_irq(vcpu->kvm, irq);
}
}
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+ bool is_uaccess)
+{
+ if (is_uaccess)
+ return;
+
+ irq->pending_latch = false;
+
+ /*
+ * We don't want the guest to effectively mask the physical
+ * interrupt by doing a write to SPENDR followed by a write to
+ * CPENDR for HW interrupts, so we clear the active state on
+ * the physical side if the virtual interrupt is not active.
+ * This may lead to taking an additional interrupt on the
+ * host, but that should not be a problem as the worst that
+ * can happen is an additional vgic injection. We also clear
+ * the pending state to maintain proper semantics for edge HW
+ * interrupts.
+ */
+ vgic_irq_set_phys_pending(irq, false);
+ if (!irq->active)
+ vgic_irq_set_phys_active(irq, false);
+}
+
void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
{
+ bool is_uaccess = !vgic_get_mmio_requester_vcpu();
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
int i;
unsigned long flags;
@@ -175,7 +216,10 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
spin_lock_irqsave(&irq->irq_lock, flags);
- irq->pending_latch = false;
+ if (irq->hw)
+ vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
+ else
+ irq->pending_latch = false;
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
@@ -202,8 +246,19 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
return value;
}
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+ bool active, bool is_uaccess)
+{
+ if (!is_uaccess)
+ irq->active = active;;
+
+ if (!is_uaccess)
+ vgic_irq_set_phys_active(irq, active);
+}
+
static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
- bool new_active_state)
+ bool active)
{
unsigned long flags;
struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
@@ -231,8 +286,12 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
irq->vcpu->cpu != -1) /* VCPU thread is running */
cond_resched_lock(&irq->irq_lock);
- irq->active = new_active_state;
- if (new_active_state)
+ if (irq->hw)
+ vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
+ else
+ irq->active = active;
+
+ if (irq->active)
vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
else
spin_unlock_irqrestore(&irq->irq_lock, flags);
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index eadabb249d2a..f4c92fae9cd3 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -144,6 +144,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
kfree(irq);
}
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
+{
+ WARN_ON(irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ pending));
+}
+
/* Get the input level of a mapped IRQ directly from the physical GIC */
bool vgic_get_phys_line_level(struct vgic_irq *irq)
{
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index d0787983a357..12c37b89f7a3 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
u32 intid);
void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
unsigned long flags);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
For mapped IRQs (with the HW bit set in the LR) we have to follow some
rules of the architecture. One of these rules is that VM must not be
allowed to deactivate a virtual interrupt with the HW bit set unless the
physical interrupt is also active.
This works fine when injecting mapped interrupts, because we leave it up
to the injector to either set EOImode==1 or manually set the active
state of the physical interrupt.
However, the guest can set virtual interrupt to be pending or active by
writing to the virtual distributor, which could lead to deactivating a
virtual interrupt with the HW bit set without the physical interrupt
being active.
We could set the physical interrupt to active whenever we are about to
enter the VM with a HW interrupt either pending or active, but that
would be really slow, especially on GICv2. So we take the long way
around and do the hard work when needed, which is expected to be
extremely rare.
When the VM sets the pending state for a HW interrupt on the virtual
distributor we set the active state on the physical distributor, because
the virtual interrupt can become active and then the guest can
deactivate it.
When the VM clears the pending state we also clear it on the physical
side, because the injector might otherwise raise the interrupt. We also
clear the physical active state when the virtual interrupt is not
active, since otherwise a SPEND/CPEND sequence from the guest would
prevent signaling of future interrupts.
Changing the state of mapped interrupts from userspace is not supported,
and it's expected that userspace unmaps devices from VFIO before
attempting to set the interrupt state, because the interrupt state is
driven by hardware.
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic.c | 7 +++++
virt/kvm/arm/vgic/vgic.h | 1 +
3 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 747b0a3b4784..8d173d99a7a4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -16,6 +16,7 @@
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <kvm/iodev.h>
+#include <kvm/arm_arch_timer.h>
#include <kvm/arm_vgic.h>
#include "vgic.h"
@@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
return vcpu;
}
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+ bool is_uaccess)
+{
+ if (is_uaccess)
+ return;
+
+ irq->pending_latch = true;
+ vgic_irq_set_phys_active(irq, true);
+}
+
void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
{
+ bool is_uaccess = !vgic_get_mmio_requester_vcpu();
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
int i;
unsigned long flags;
@@ -155,17 +168,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
spin_lock_irqsave(&irq->irq_lock, flags);
- irq->pending_latch = true;
-
+ if (irq->hw)
+ vgic_hw_irq_spending(vcpu, irq, is_uaccess);
+ else
+ irq->pending_latch = true;
vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
vgic_put_irq(vcpu->kvm, irq);
}
}
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+ bool is_uaccess)
+{
+ if (is_uaccess)
+ return;
+
+ irq->pending_latch = false;
+
+ /*
+ * We don't want the guest to effectively mask the physical
+ * interrupt by doing a write to SPENDR followed by a write to
+ * CPENDR for HW interrupts, so we clear the active state on
+ * the physical side if the virtual interrupt is not active.
+ * This may lead to taking an additional interrupt on the
+ * host, but that should not be a problem as the worst that
+ * can happen is an additional vgic injection. We also clear
+ * the pending state to maintain proper semantics for edge HW
+ * interrupts.
+ */
+ vgic_irq_set_phys_pending(irq, false);
+ if (!irq->active)
+ vgic_irq_set_phys_active(irq, false);
+}
+
void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
{
+ bool is_uaccess = !vgic_get_mmio_requester_vcpu();
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
int i;
unsigned long flags;
@@ -175,7 +216,10 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
spin_lock_irqsave(&irq->irq_lock, flags);
- irq->pending_latch = false;
+ if (irq->hw)
+ vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
+ else
+ irq->pending_latch = false;
spin_unlock_irqrestore(&irq->irq_lock, flags);
vgic_put_irq(vcpu->kvm, irq);
@@ -202,8 +246,19 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
return value;
}
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+ bool active, bool is_uaccess)
+{
+ if (!is_uaccess)
+ irq->active = active;;
+
+ if (!is_uaccess)
+ vgic_irq_set_phys_active(irq, active);
+}
+
static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
- bool new_active_state)
+ bool active)
{
unsigned long flags;
struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
@@ -231,8 +286,12 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
irq->vcpu->cpu != -1) /* VCPU thread is running */
cond_resched_lock(&irq->irq_lock);
- irq->active = new_active_state;
- if (new_active_state)
+ if (irq->hw)
+ vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
+ else
+ irq->active = active;
+
+ if (irq->active)
vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
else
spin_unlock_irqrestore(&irq->irq_lock, flags);
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index eadabb249d2a..f4c92fae9cd3 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -144,6 +144,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
kfree(irq);
}
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
+{
+ WARN_ON(irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ pending));
+}
+
/* Get the input level of a mapped IRQ directly from the physical GIC */
bool vgic_get_phys_line_level(struct vgic_irq *irq)
{
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index d0787983a357..12c37b89f7a3 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
u32 intid);
void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
unsigned long flags);
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
2017-12-04 20:05 ` Christoffer Dall
@ 2017-12-05 12:43 ` Andrew Jones
-1 siblings, 0 replies; 40+ messages in thread
From: Andrew Jones @ 2017-12-05 12:43 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvmarm, linux-arm-kernel, kvm, Marc Zyngier, Andre Przywara,
Eric Auger, Christoffer Dall
On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool active, bool is_uaccess)
> +{
> + if (!is_uaccess)
> + irq->active = active;;
> +
> + if (!is_uaccess)
> + vgic_irq_set_phys_active(irq, active);
> +}
> +
Missed one.
drew
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
@ 2017-12-05 12:43 ` Andrew Jones
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Jones @ 2017-12-05 12:43 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool active, bool is_uaccess)
> +{
> + if (!is_uaccess)
> + irq->active = active;;
> +
> + if (!is_uaccess)
> + vgic_irq_set_phys_active(irq, active);
> +}
> +
Missed one.
drew
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
2017-12-04 20:05 ` Christoffer Dall
@ 2017-12-05 15:03 ` Yury Norov
-1 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 15:03 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvmarm, linux-arm-kernel, kvm, Marc Zyngier, Andre Przywara,
Eric Auger, Christoffer Dall
On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> For mapped IRQs (with the HW bit set in the LR) we have to follow some
> rules of the architecture. One of these rules is that VM must not be
> allowed to deactivate a virtual interrupt with the HW bit set unless the
> physical interrupt is also active.
>
> This works fine when injecting mapped interrupts, because we leave it up
> to the injector to either set EOImode==1 or manually set the active
> state of the physical interrupt.
>
> However, the guest can set virtual interrupt to be pending or active by
> writing to the virtual distributor, which could lead to deactivating a
> virtual interrupt with the HW bit set without the physical interrupt
> being active.
>
> We could set the physical interrupt to active whenever we are about to
> enter the VM with a HW interrupt either pending or active, but that
> would be really slow, especially on GICv2. So we take the long way
> around and do the hard work when needed, which is expected to be
> extremely rare.
>
> When the VM sets the pending state for a HW interrupt on the virtual
> distributor we set the active state on the physical distributor, because
> the virtual interrupt can become active and then the guest can
> deactivate it.
>
> When the VM clears the pending state we also clear it on the physical
> side, because the injector might otherwise raise the interrupt. We also
> clear the physical active state when the virtual interrupt is not
> active, since otherwise a SPEND/CPEND sequence from the guest would
> prevent signaling of future interrupts.
>
> Changing the state of mapped interrupts from userspace is not supported,
> and it's expected that userspace unmaps devices from VFIO before
> attempting to set the interrupt state, because the interrupt state is
> driven by hardware.
>
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic.c | 7 +++++
> virt/kvm/arm/vgic/vgic.h | 1 +
> 3 files changed, 73 insertions(+), 6 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 747b0a3b4784..8d173d99a7a4 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -16,6 +16,7 @@
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> #include <kvm/iodev.h>
> +#include <kvm/arm_arch_timer.h>
> #include <kvm/arm_vgic.h>
>
> #include "vgic.h"
> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> return vcpu;
> }
>
> +/* Must be called with irq->irq_lock held */
Instead of enforcing this rule in comment, you can enforce it in code:
BUG_ON(!spin_is_locked(irq->irq_lock))
> +static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool is_uaccess)
> +{
> + if (is_uaccess)
> + return;
> +
> + irq->pending_latch = true;
> + vgic_irq_set_phys_active(irq, true);
> +}
> +
> void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> + bool is_uaccess = !vgic_get_mmio_requester_vcpu();
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> int i;
> unsigned long flags;
> @@ -155,17 +168,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> spin_lock_irqsave(&irq->irq_lock, flags);
> - irq->pending_latch = true;
> -
> + if (irq->hw)
> + vgic_hw_irq_spending(vcpu, irq, is_uaccess);
> + else
> + irq->pending_latch = true;
> vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> vgic_put_irq(vcpu->kvm, irq);
> }
> }
>
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool is_uaccess)
> +{
> + if (is_uaccess)
> + return;
> +
> + irq->pending_latch = false;
> +
> + /*
> + * We don't want the guest to effectively mask the physical
> + * interrupt by doing a write to SPENDR followed by a write to
> + * CPENDR for HW interrupts, so we clear the active state on
> + * the physical side if the virtual interrupt is not active.
> + * This may lead to taking an additional interrupt on the
> + * host, but that should not be a problem as the worst that
> + * can happen is an additional vgic injection. We also clear
> + * the pending state to maintain proper semantics for edge HW
> + * interrupts.
> + */
> + vgic_irq_set_phys_pending(irq, false);
> + if (!irq->active)
> + vgic_irq_set_phys_active(irq, false);
> +}
> +
> void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> + bool is_uaccess = !vgic_get_mmio_requester_vcpu();
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> int i;
> unsigned long flags;
> @@ -175,7 +216,10 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>
> spin_lock_irqsave(&irq->irq_lock, flags);
>
> - irq->pending_latch = false;
> + if (irq->hw)
> + vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
> + else
> + irq->pending_latch = false;
>
> spin_unlock_irqrestore(&irq->irq_lock, flags);
> vgic_put_irq(vcpu->kvm, irq);
> @@ -202,8 +246,19 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool active, bool is_uaccess)
> +{
> + if (!is_uaccess)
> + irq->active = active;;
Double semicolon.
> +
> + if (!is_uaccess)
> + vgic_irq_set_phys_active(irq, active);
> +}
Why not like this?
if (is_uaccess)
return;
irq->active = active;
vgic_irq_set_phys_active(irq, active);
> +
> static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> - bool new_active_state)
> + bool active)
> {
> unsigned long flags;
> struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
> @@ -231,8 +286,12 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> irq->vcpu->cpu != -1) /* VCPU thread is running */
> cond_resched_lock(&irq->irq_lock);
>
> - irq->active = new_active_state;
> - if (new_active_state)
> + if (irq->hw)
> + vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
> + else
> + irq->active = active;
> +
> + if (irq->active)
> vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> else
> spin_unlock_irqrestore(&irq->irq_lock, flags);
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index eadabb249d2a..f4c92fae9cd3 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -144,6 +144,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
> kfree(irq);
> }
>
> +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
> +{
> + WARN_ON(irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + pending));
> +}
> +
> /* Get the input level of a mapped IRQ directly from the physical GIC */
> bool vgic_get_phys_line_level(struct vgic_irq *irq)
> {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index d0787983a357..12c37b89f7a3 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
> u32 intid);
> void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
> bool vgic_get_phys_line_level(struct vgic_irq *irq);
> +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
> void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
> bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> unsigned long flags);
> --
> 2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
@ 2017-12-05 15:03 ` Yury Norov
0 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 15:03 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> For mapped IRQs (with the HW bit set in the LR) we have to follow some
> rules of the architecture. One of these rules is that VM must not be
> allowed to deactivate a virtual interrupt with the HW bit set unless the
> physical interrupt is also active.
>
> This works fine when injecting mapped interrupts, because we leave it up
> to the injector to either set EOImode==1 or manually set the active
> state of the physical interrupt.
>
> However, the guest can set virtual interrupt to be pending or active by
> writing to the virtual distributor, which could lead to deactivating a
> virtual interrupt with the HW bit set without the physical interrupt
> being active.
>
> We could set the physical interrupt to active whenever we are about to
> enter the VM with a HW interrupt either pending or active, but that
> would be really slow, especially on GICv2. So we take the long way
> around and do the hard work when needed, which is expected to be
> extremely rare.
>
> When the VM sets the pending state for a HW interrupt on the virtual
> distributor we set the active state on the physical distributor, because
> the virtual interrupt can become active and then the guest can
> deactivate it.
>
> When the VM clears the pending state we also clear it on the physical
> side, because the injector might otherwise raise the interrupt. We also
> clear the physical active state when the virtual interrupt is not
> active, since otherwise a SPEND/CPEND sequence from the guest would
> prevent signaling of future interrupts.
>
> Changing the state of mapped interrupts from userspace is not supported,
> and it's expected that userspace unmaps devices from VFIO before
> attempting to set the interrupt state, because the interrupt state is
> driven by hardware.
>
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic.c | 7 +++++
> virt/kvm/arm/vgic/vgic.h | 1 +
> 3 files changed, 73 insertions(+), 6 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 747b0a3b4784..8d173d99a7a4 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -16,6 +16,7 @@
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> #include <kvm/iodev.h>
> +#include <kvm/arm_arch_timer.h>
> #include <kvm/arm_vgic.h>
>
> #include "vgic.h"
> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> return vcpu;
> }
>
> +/* Must be called with irq->irq_lock held */
Instead of enforcing this rule in comment, you can enforce it in code:
BUG_ON(!spin_is_locked(irq->irq_lock))
> +static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool is_uaccess)
> +{
> + if (is_uaccess)
> + return;
> +
> + irq->pending_latch = true;
> + vgic_irq_set_phys_active(irq, true);
> +}
> +
> void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> + bool is_uaccess = !vgic_get_mmio_requester_vcpu();
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> int i;
> unsigned long flags;
> @@ -155,17 +168,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> spin_lock_irqsave(&irq->irq_lock, flags);
> - irq->pending_latch = true;
> -
> + if (irq->hw)
> + vgic_hw_irq_spending(vcpu, irq, is_uaccess);
> + else
> + irq->pending_latch = true;
> vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> vgic_put_irq(vcpu->kvm, irq);
> }
> }
>
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool is_uaccess)
> +{
> + if (is_uaccess)
> + return;
> +
> + irq->pending_latch = false;
> +
> + /*
> + * We don't want the guest to effectively mask the physical
> + * interrupt by doing a write to SPENDR followed by a write to
> + * CPENDR for HW interrupts, so we clear the active state on
> + * the physical side if the virtual interrupt is not active.
> + * This may lead to taking an additional interrupt on the
> + * host, but that should not be a problem as the worst that
> + * can happen is an additional vgic injection. We also clear
> + * the pending state to maintain proper semantics for edge HW
> + * interrupts.
> + */
> + vgic_irq_set_phys_pending(irq, false);
> + if (!irq->active)
> + vgic_irq_set_phys_active(irq, false);
> +}
> +
> void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> + bool is_uaccess = !vgic_get_mmio_requester_vcpu();
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> int i;
> unsigned long flags;
> @@ -175,7 +216,10 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>
> spin_lock_irqsave(&irq->irq_lock, flags);
>
> - irq->pending_latch = false;
> + if (irq->hw)
> + vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
> + else
> + irq->pending_latch = false;
>
> spin_unlock_irqrestore(&irq->irq_lock, flags);
> vgic_put_irq(vcpu->kvm, irq);
> @@ -202,8 +246,19 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> + bool active, bool is_uaccess)
> +{
> + if (!is_uaccess)
> + irq->active = active;;
Double semicolon.
> +
> + if (!is_uaccess)
> + vgic_irq_set_phys_active(irq, active);
> +}
Why not like this?
if (is_uaccess)
return;
irq->active = active;
vgic_irq_set_phys_active(irq, active);
> +
> static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> - bool new_active_state)
> + bool active)
> {
> unsigned long flags;
> struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
> @@ -231,8 +286,12 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> irq->vcpu->cpu != -1) /* VCPU thread is running */
> cond_resched_lock(&irq->irq_lock);
>
> - irq->active = new_active_state;
> - if (new_active_state)
> + if (irq->hw)
> + vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
> + else
> + irq->active = active;
> +
> + if (irq->active)
> vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> else
> spin_unlock_irqrestore(&irq->irq_lock, flags);
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index eadabb249d2a..f4c92fae9cd3 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -144,6 +144,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
> kfree(irq);
> }
>
> +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
> +{
> + WARN_ON(irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + pending));
> +}
> +
> /* Get the input level of a mapped IRQ directly from the physical GIC */
> bool vgic_get_phys_line_level(struct vgic_irq *irq)
> {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index d0787983a357..12c37b89f7a3 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
> u32 intid);
> void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
> bool vgic_get_phys_line_level(struct vgic_irq *irq);
> +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
> void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
> bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> unsigned long flags);
> --
> 2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
2017-12-05 15:03 ` Yury Norov
@ 2017-12-05 16:47 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2017-12-05 16:47 UTC (permalink / raw)
To: Yury Norov, Christoffer Dall
Cc: kvm, Andre Przywara, linux-arm-kernel, kvmarm
On 05/12/17 15:03, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>
>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>> rules of the architecture. One of these rules is that VM must not be
>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>> physical interrupt is also active.
>>
>> This works fine when injecting mapped interrupts, because we leave it up
>> to the injector to either set EOImode==1 or manually set the active
>> state of the physical interrupt.
>>
>> However, the guest can set virtual interrupt to be pending or active by
>> writing to the virtual distributor, which could lead to deactivating a
>> virtual interrupt with the HW bit set without the physical interrupt
>> being active.
>>
>> We could set the physical interrupt to active whenever we are about to
>> enter the VM with a HW interrupt either pending or active, but that
>> would be really slow, especially on GICv2. So we take the long way
>> around and do the hard work when needed, which is expected to be
>> extremely rare.
>>
>> When the VM sets the pending state for a HW interrupt on the virtual
>> distributor we set the active state on the physical distributor, because
>> the virtual interrupt can become active and then the guest can
>> deactivate it.
>>
>> When the VM clears the pending state we also clear it on the physical
>> side, because the injector might otherwise raise the interrupt. We also
>> clear the physical active state when the virtual interrupt is not
>> active, since otherwise a SPEND/CPEND sequence from the guest would
>> prevent signaling of future interrupts.
>>
>> Changing the state of mapped interrupts from userspace is not supported,
>> and it's expected that userspace unmaps devices from VFIO before
>> attempting to set the interrupt state, because the interrupt state is
>> driven by hardware.
>>
>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
>> virt/kvm/arm/vgic/vgic.c | 7 +++++
>> virt/kvm/arm/vgic/vgic.h | 1 +
>> 3 files changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 747b0a3b4784..8d173d99a7a4 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -16,6 +16,7 @@
>> #include <linux/kvm.h>
>> #include <linux/kvm_host.h>
>> #include <kvm/iodev.h>
>> +#include <kvm/arm_arch_timer.h>
>> #include <kvm/arm_vgic.h>
>>
>> #include "vgic.h"
>> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
>> return vcpu;
>> }
>>
>> +/* Must be called with irq->irq_lock held */
>
> Instead of enforcing this rule in comment, you can enforce it in code:
>
> BUG_ON(!spin_is_locked(irq->irq_lock))
Are we going to litter the kernel with such assertions? I don't think
that's a valuable thing to do.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
@ 2017-12-05 16:47 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2017-12-05 16:47 UTC (permalink / raw)
To: linux-arm-kernel
On 05/12/17 15:03, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>
>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>> rules of the architecture. One of these rules is that VM must not be
>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>> physical interrupt is also active.
>>
>> This works fine when injecting mapped interrupts, because we leave it up
>> to the injector to either set EOImode==1 or manually set the active
>> state of the physical interrupt.
>>
>> However, the guest can set virtual interrupt to be pending or active by
>> writing to the virtual distributor, which could lead to deactivating a
>> virtual interrupt with the HW bit set without the physical interrupt
>> being active.
>>
>> We could set the physical interrupt to active whenever we are about to
>> enter the VM with a HW interrupt either pending or active, but that
>> would be really slow, especially on GICv2. So we take the long way
>> around and do the hard work when needed, which is expected to be
>> extremely rare.
>>
>> When the VM sets the pending state for a HW interrupt on the virtual
>> distributor we set the active state on the physical distributor, because
>> the virtual interrupt can become active and then the guest can
>> deactivate it.
>>
>> When the VM clears the pending state we also clear it on the physical
>> side, because the injector might otherwise raise the interrupt. We also
>> clear the physical active state when the virtual interrupt is not
>> active, since otherwise a SPEND/CPEND sequence from the guest would
>> prevent signaling of future interrupts.
>>
>> Changing the state of mapped interrupts from userspace is not supported,
>> and it's expected that userspace unmaps devices from VFIO before
>> attempting to set the interrupt state, because the interrupt state is
>> driven by hardware.
>>
>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
>> virt/kvm/arm/vgic/vgic.c | 7 +++++
>> virt/kvm/arm/vgic/vgic.h | 1 +
>> 3 files changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 747b0a3b4784..8d173d99a7a4 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -16,6 +16,7 @@
>> #include <linux/kvm.h>
>> #include <linux/kvm_host.h>
>> #include <kvm/iodev.h>
>> +#include <kvm/arm_arch_timer.h>
>> #include <kvm/arm_vgic.h>
>>
>> #include "vgic.h"
>> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
>> return vcpu;
>> }
>>
>> +/* Must be called with irq->irq_lock held */
>
> Instead of enforcing this rule in comment, you can enforce it in code:
>
> BUG_ON(!spin_is_locked(irq->irq_lock))
Are we going to litter the kernel with such assertions? I don't think
that's a valuable thing to do.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
2017-12-05 16:47 ` Marc Zyngier
@ 2017-12-05 22:39 ` Yury Norov
-1 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 22:39 UTC (permalink / raw)
To: Marc Zyngier
Cc: Christoffer Dall, kvmarm, linux-arm-kernel, kvm, Andre Przywara,
Eric Auger, Christoffer Dall
On Tue, Dec 05, 2017 at 04:47:46PM +0000, Marc Zyngier wrote:
> On 05/12/17 15:03, Yury Norov wrote:
> > On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
> >> From: Christoffer Dall <christoffer.dall@linaro.org>
> >>
> >> For mapped IRQs (with the HW bit set in the LR) we have to follow some
> >> rules of the architecture. One of these rules is that VM must not be
> >> allowed to deactivate a virtual interrupt with the HW bit set unless the
> >> physical interrupt is also active.
> >>
> >> This works fine when injecting mapped interrupts, because we leave it up
> >> to the injector to either set EOImode==1 or manually set the active
> >> state of the physical interrupt.
> >>
> >> However, the guest can set virtual interrupt to be pending or active by
> >> writing to the virtual distributor, which could lead to deactivating a
> >> virtual interrupt with the HW bit set without the physical interrupt
> >> being active.
> >>
> >> We could set the physical interrupt to active whenever we are about to
> >> enter the VM with a HW interrupt either pending or active, but that
> >> would be really slow, especially on GICv2. So we take the long way
> >> around and do the hard work when needed, which is expected to be
> >> extremely rare.
> >>
> >> When the VM sets the pending state for a HW interrupt on the virtual
> >> distributor we set the active state on the physical distributor, because
> >> the virtual interrupt can become active and then the guest can
> >> deactivate it.
> >>
> >> When the VM clears the pending state we also clear it on the physical
> >> side, because the injector might otherwise raise the interrupt. We also
> >> clear the physical active state when the virtual interrupt is not
> >> active, since otherwise a SPEND/CPEND sequence from the guest would
> >> prevent signaling of future interrupts.
> >>
> >> Changing the state of mapped interrupts from userspace is not supported,
> >> and it's expected that userspace unmaps devices from VFIO before
> >> attempting to set the interrupt state, because the interrupt state is
> >> driven by hardware.
> >>
> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> ---
> >> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
> >> virt/kvm/arm/vgic/vgic.c | 7 +++++
> >> virt/kvm/arm/vgic/vgic.h | 1 +
> >> 3 files changed, 73 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> >> index 747b0a3b4784..8d173d99a7a4 100644
> >> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> >> @@ -16,6 +16,7 @@
> >> #include <linux/kvm.h>
> >> #include <linux/kvm_host.h>
> >> #include <kvm/iodev.h>
> >> +#include <kvm/arm_arch_timer.h>
> >> #include <kvm/arm_vgic.h>
> >>
> >> #include "vgic.h"
> >> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> >> return vcpu;
> >> }
> >>
> >> +/* Must be called with irq->irq_lock held */
> >
> > Instead of enforcing this rule in comment, you can enforce it in code:
> >
> > BUG_ON(!spin_is_locked(irq->irq_lock))
>
> Are we going to litter the kernel with such assertions? I don't think
> that's a valuable thing to do.
That's what I agree - BUG-ifiyng is somewhat debugging technique and
should be avoided in release code. But as I can see, in kvm code BUG()s
are widely used:
$ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l
155
So I tuned my littering detector. :)
In this patchset new BUG()s are added in patches 4 and 6. In patch 6
BUG() has meaning of TODO:
+ if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+ timer = vcpu_vtimer(vcpu);
+ else
+ BUG(); /* We only map the vtimer so far */
+
Which is far from original purpose of BUG().
If you think that BUG() is not OK in this case (and I agree with it),
I think they should be also removed from patches 4 and 6. 6 - for sure.
Yury
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
@ 2017-12-05 22:39 ` Yury Norov
0 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 22:39 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 05, 2017 at 04:47:46PM +0000, Marc Zyngier wrote:
> On 05/12/17 15:03, Yury Norov wrote:
> > On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
> >> From: Christoffer Dall <christoffer.dall@linaro.org>
> >>
> >> For mapped IRQs (with the HW bit set in the LR) we have to follow some
> >> rules of the architecture. One of these rules is that VM must not be
> >> allowed to deactivate a virtual interrupt with the HW bit set unless the
> >> physical interrupt is also active.
> >>
> >> This works fine when injecting mapped interrupts, because we leave it up
> >> to the injector to either set EOImode==1 or manually set the active
> >> state of the physical interrupt.
> >>
> >> However, the guest can set virtual interrupt to be pending or active by
> >> writing to the virtual distributor, which could lead to deactivating a
> >> virtual interrupt with the HW bit set without the physical interrupt
> >> being active.
> >>
> >> We could set the physical interrupt to active whenever we are about to
> >> enter the VM with a HW interrupt either pending or active, but that
> >> would be really slow, especially on GICv2. So we take the long way
> >> around and do the hard work when needed, which is expected to be
> >> extremely rare.
> >>
> >> When the VM sets the pending state for a HW interrupt on the virtual
> >> distributor we set the active state on the physical distributor, because
> >> the virtual interrupt can become active and then the guest can
> >> deactivate it.
> >>
> >> When the VM clears the pending state we also clear it on the physical
> >> side, because the injector might otherwise raise the interrupt. We also
> >> clear the physical active state when the virtual interrupt is not
> >> active, since otherwise a SPEND/CPEND sequence from the guest would
> >> prevent signaling of future interrupts.
> >>
> >> Changing the state of mapped interrupts from userspace is not supported,
> >> and it's expected that userspace unmaps devices from VFIO before
> >> attempting to set the interrupt state, because the interrupt state is
> >> driven by hardware.
> >>
> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> ---
> >> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
> >> virt/kvm/arm/vgic/vgic.c | 7 +++++
> >> virt/kvm/arm/vgic/vgic.h | 1 +
> >> 3 files changed, 73 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> >> index 747b0a3b4784..8d173d99a7a4 100644
> >> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> >> @@ -16,6 +16,7 @@
> >> #include <linux/kvm.h>
> >> #include <linux/kvm_host.h>
> >> #include <kvm/iodev.h>
> >> +#include <kvm/arm_arch_timer.h>
> >> #include <kvm/arm_vgic.h>
> >>
> >> #include "vgic.h"
> >> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> >> return vcpu;
> >> }
> >>
> >> +/* Must be called with irq->irq_lock held */
> >
> > Instead of enforcing this rule in comment, you can enforce it in code:
> >
> > BUG_ON(!spin_is_locked(irq->irq_lock))
>
> Are we going to litter the kernel with such assertions? I don't think
> that's a valuable thing to do.
That's what I agree - BUG-ifiyng is somewhat debugging technique and
should be avoided in release code. But as I can see, in kvm code BUG()s
are widely used:
$ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l
155
So I tuned my littering detector. :)
In this patchset new BUG()s are added in patches 4 and 6. In patch 6
BUG() has meaning of TODO:
+ if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+ timer = vcpu_vtimer(vcpu);
+ else
+ BUG(); /* We only map the vtimer so far */
+
Which is far from original purpose of BUG().
If you think that BUG() is not OK in this case (and I agree with it),
I think they should be also removed from patches 4 and 6. 6 - for sure.
Yury
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
2017-12-05 22:39 ` Yury Norov
@ 2017-12-06 8:56 ` Marc Zyngier
-1 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2017-12-06 8:56 UTC (permalink / raw)
To: Yury Norov
Cc: Christoffer Dall, kvm, Andre Przywara, kvmarm, linux-arm-kernel
On 05/12/17 22:39, Yury Norov wrote:
> On Tue, Dec 05, 2017 at 04:47:46PM +0000, Marc Zyngier wrote:
>> On 05/12/17 15:03, Yury Norov wrote:
>>> On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
>>>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>>>
>>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>>>> rules of the architecture. One of these rules is that VM must not be
>>>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>>>> physical interrupt is also active.
>>>>
>>>> This works fine when injecting mapped interrupts, because we leave it up
>>>> to the injector to either set EOImode==1 or manually set the active
>>>> state of the physical interrupt.
>>>>
>>>> However, the guest can set virtual interrupt to be pending or active by
>>>> writing to the virtual distributor, which could lead to deactivating a
>>>> virtual interrupt with the HW bit set without the physical interrupt
>>>> being active.
>>>>
>>>> We could set the physical interrupt to active whenever we are about to
>>>> enter the VM with a HW interrupt either pending or active, but that
>>>> would be really slow, especially on GICv2. So we take the long way
>>>> around and do the hard work when needed, which is expected to be
>>>> extremely rare.
>>>>
>>>> When the VM sets the pending state for a HW interrupt on the virtual
>>>> distributor we set the active state on the physical distributor, because
>>>> the virtual interrupt can become active and then the guest can
>>>> deactivate it.
>>>>
>>>> When the VM clears the pending state we also clear it on the physical
>>>> side, because the injector might otherwise raise the interrupt. We also
>>>> clear the physical active state when the virtual interrupt is not
>>>> active, since otherwise a SPEND/CPEND sequence from the guest would
>>>> prevent signaling of future interrupts.
>>>>
>>>> Changing the state of mapped interrupts from userspace is not supported,
>>>> and it's expected that userspace unmaps devices from VFIO before
>>>> attempting to set the interrupt state, because the interrupt state is
>>>> driven by hardware.
>>>>
>>>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>> ---
>>>> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
>>>> virt/kvm/arm/vgic/vgic.c | 7 +++++
>>>> virt/kvm/arm/vgic/vgic.h | 1 +
>>>> 3 files changed, 73 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>>> index 747b0a3b4784..8d173d99a7a4 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>>> @@ -16,6 +16,7 @@
>>>> #include <linux/kvm.h>
>>>> #include <linux/kvm_host.h>
>>>> #include <kvm/iodev.h>
>>>> +#include <kvm/arm_arch_timer.h>
>>>> #include <kvm/arm_vgic.h>
>>>>
>>>> #include "vgic.h"
>>>> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
>>>> return vcpu;
>>>> }
>>>>
>>>> +/* Must be called with irq->irq_lock held */
>>>
>>> Instead of enforcing this rule in comment, you can enforce it in code:
>>>
>>> BUG_ON(!spin_is_locked(irq->irq_lock))
>>
>> Are we going to litter the kernel with such assertions? I don't think
>> that's a valuable thing to do.
>
> That's what I agree - BUG-ifiyng is somewhat debugging technique and
> should be avoided in release code. But as I can see, in kvm code BUG()s
> are widely used:
> $ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l
> 155
>
> So I tuned my littering detector. :)
>
> In this patchset new BUG()s are added in patches 4 and 6. In patch 6
> BUG() has meaning of TODO:
>
> + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> + timer = vcpu_vtimer(vcpu);
> + else
> + BUG(); /* We only map the vtimer so far */
> +
>
> Which is far from original purpose of BUG().
>
> If you think that BUG() is not OK in this case (and I agree with it),
> I think they should be also removed from patches 4 and 6. 6 - for sure.
There is a small, but important difference here. Your earlier suggestion
had the implication that we should check for the irq->irq_lock on all
code paths, as there is no point in only checking it in a single
function. That assertion must be true in a lot of places in the vgic
code, including places that are on the fast path.
This would turn the code into a pretty horrible mess, and bring very
little to the table (unless you consider slowing down your system to be
a feature). Also, we have better ways of detecting locking issues, such
as lockdep.
On the other hand, the BUG you quote above check for a condition that
only makes sense in this particular function.
So I'd suggest that instead of blindly counting the number of assertion,
you look at whether that makes sense at that particular point. Yes, we
probably have too many BUG_ONs, and we usually weed them after gaining
some confidence that the code is sane. For new code, a BUG_ON is a very
reassuring point that we don't hit anything unexpected.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
@ 2017-12-06 8:56 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2017-12-06 8:56 UTC (permalink / raw)
To: linux-arm-kernel
On 05/12/17 22:39, Yury Norov wrote:
> On Tue, Dec 05, 2017 at 04:47:46PM +0000, Marc Zyngier wrote:
>> On 05/12/17 15:03, Yury Norov wrote:
>>> On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote:
>>>> From: Christoffer Dall <christoffer.dall@linaro.org>
>>>>
>>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>>>> rules of the architecture. One of these rules is that VM must not be
>>>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>>>> physical interrupt is also active.
>>>>
>>>> This works fine when injecting mapped interrupts, because we leave it up
>>>> to the injector to either set EOImode==1 or manually set the active
>>>> state of the physical interrupt.
>>>>
>>>> However, the guest can set virtual interrupt to be pending or active by
>>>> writing to the virtual distributor, which could lead to deactivating a
>>>> virtual interrupt with the HW bit set without the physical interrupt
>>>> being active.
>>>>
>>>> We could set the physical interrupt to active whenever we are about to
>>>> enter the VM with a HW interrupt either pending or active, but that
>>>> would be really slow, especially on GICv2. So we take the long way
>>>> around and do the hard work when needed, which is expected to be
>>>> extremely rare.
>>>>
>>>> When the VM sets the pending state for a HW interrupt on the virtual
>>>> distributor we set the active state on the physical distributor, because
>>>> the virtual interrupt can become active and then the guest can
>>>> deactivate it.
>>>>
>>>> When the VM clears the pending state we also clear it on the physical
>>>> side, because the injector might otherwise raise the interrupt. We also
>>>> clear the physical active state when the virtual interrupt is not
>>>> active, since otherwise a SPEND/CPEND sequence from the guest would
>>>> prevent signaling of future interrupts.
>>>>
>>>> Changing the state of mapped interrupts from userspace is not supported,
>>>> and it's expected that userspace unmaps devices from VFIO before
>>>> attempting to set the interrupt state, because the interrupt state is
>>>> driven by hardware.
>>>>
>>>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>> ---
>>>> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
>>>> virt/kvm/arm/vgic/vgic.c | 7 +++++
>>>> virt/kvm/arm/vgic/vgic.h | 1 +
>>>> 3 files changed, 73 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>>> index 747b0a3b4784..8d173d99a7a4 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>>> @@ -16,6 +16,7 @@
>>>> #include <linux/kvm.h>
>>>> #include <linux/kvm_host.h>
>>>> #include <kvm/iodev.h>
>>>> +#include <kvm/arm_arch_timer.h>
>>>> #include <kvm/arm_vgic.h>
>>>>
>>>> #include "vgic.h"
>>>> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
>>>> return vcpu;
>>>> }
>>>>
>>>> +/* Must be called with irq->irq_lock held */
>>>
>>> Instead of enforcing this rule in comment, you can enforce it in code:
>>>
>>> BUG_ON(!spin_is_locked(irq->irq_lock))
>>
>> Are we going to litter the kernel with such assertions? I don't think
>> that's a valuable thing to do.
>
> That's what I agree - BUG-ifiyng is somewhat debugging technique and
> should be avoided in release code. But as I can see, in kvm code BUG()s
> are widely used:
> $ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l
> 155
>
> So I tuned my littering detector. :)
>
> In this patchset new BUG()s are added in patches 4 and 6. In patch 6
> BUG() has meaning of TODO:
>
> + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> + timer = vcpu_vtimer(vcpu);
> + else
> + BUG(); /* We only map the vtimer so far */
> +
>
> Which is far from original purpose of BUG().
>
> If you think that BUG() is not OK in this case (and I agree with it),
> I think they should be also removed from patches 4 and 6. 6 - for sure.
There is a small, but important difference here. Your earlier suggestion
had the implication that we should check for the irq->irq_lock on all
code paths, as there is no point in only checking it in a single
function. That assertion must be true in a lot of places in the vgic
code, including places that are on the fast path.
This would turn the code into a pretty horrible mess, and bring very
little to the table (unless you consider slowing down your system to be
a feature). Also, we have better ways of detecting locking issues, such
as lockdep.
On the other hand, the BUG you quote above check for a condition that
only makes sense in this particular function.
So I'd suggest that instead of blindly counting the number of assertion,
you look at whether that makes sense at that particular point. Yes, we
probably have too many BUG_ONs, and we usually weed them after gaining
some confidence that the code is sane. For new code, a BUG_ON is a very
reassuring point that we don't hit anything unexpected.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm; +Cc: kvm, Marc Zyngier, Andre Przywara, linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The VGIC can now support the life-cycle of mapped level-triggered
interrupts, and we no longer have to read back the timer state on every
exit from the VM if we had an asserted timer interrupt signal, because
the VGIC already knows if we hit the unlikely case where the guest
disables the timer without ACKing the virtual timer interrupt.
This means we rework a bit of the code to factor out the functionality
to snapshot the timer state from vtimer_save_state(), and we can reuse
this functionality in the sync path when we have an irqchip in
userspace, and also to support our implementation of the
get_input_level() function for the timer.
This change also means that we can no longer rely on the timer's view of
the interrupt line to set the active state, because we no longer
maintain this state for mapped interrupts when exiting from the guest.
Instead, we only set the active state if the virtual interrupt is
active, and otherwise we simply let the timer fire again and raise the
virtual interrupt from the ISR.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
include/kvm/arm_arch_timer.h | 2 ++
virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
2 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 01ee473517e2..f57f795d704c 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
void kvm_timer_init_vhe(void);
+bool kvm_arch_timer_get_input_level(int vintid);
+
#define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
#define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index e78ba5e20f74..82d4963f63b8 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -343,6 +343,12 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
phys_timer_emulate(vcpu);
}
+static void __timer_snapshot_state(struct arch_timer_context *timer)
+{
+ timer->cnt_ctl = read_sysreg_el0(cntv_ctl);
+ timer->cnt_cval = read_sysreg_el0(cntv_cval);
+}
+
static void vtimer_save_state(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -354,10 +360,8 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
if (!vtimer->loaded)
goto out;
- if (timer->enabled) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- vtimer->cnt_cval = read_sysreg_el0(cntv_cval);
- }
+ if (timer->enabled)
+ __timer_snapshot_state(vtimer);
/* Disable the virtual timer */
write_sysreg_el0(0, cntv_ctl);
@@ -454,8 +458,7 @@ static void kvm_timer_vcpu_load_vgic(struct kvm_vcpu *vcpu)
bool phys_active;
int ret;
- phys_active = vtimer->irq.level ||
- kvm_vgic_map_is_active(vcpu, vtimer->irq.irq);
+ phys_active = kvm_vgic_map_is_active(vcpu, vtimer->irq.irq);
ret = irq_set_irqchip_state(host_vtimer_irq,
IRQCHIP_STATE_ACTIVE,
@@ -541,27 +544,19 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
set_cntvoff(0);
}
-static void unmask_vtimer_irq(struct kvm_vcpu *vcpu)
+/*
+ * With a userspace irqchip we have to check if the guest de-asserted the
+ * timer and if so, unmask the timer irq signal on the host interrupt
+ * controller to ensure that we see future timer signals.
+ */
+static void unmask_vtimer_irq_user(struct kvm_vcpu *vcpu)
{
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
if (unlikely(!irqchip_in_kernel(vcpu->kvm))) {
- kvm_vtimer_update_mask_user(vcpu);
- return;
- }
-
- /*
- * If the guest disabled the timer without acking the interrupt, then
- * we must make sure the physical and virtual active states are in
- * sync by deactivating the physical interrupt, because otherwise we
- * wouldn't see the next timer interrupt in the host.
- */
- if (!kvm_vgic_map_is_active(vcpu, vtimer->irq.irq)) {
- int ret;
- ret = irq_set_irqchip_state(host_vtimer_irq,
- IRQCHIP_STATE_ACTIVE,
- false);
- WARN_ON(ret);
+ __timer_snapshot_state(vtimer);
+ if (!kvm_timer_should_fire(vtimer))
+ kvm_vtimer_update_mask_user(vcpu);
}
}
@@ -574,21 +569,7 @@ static void unmask_vtimer_irq(struct kvm_vcpu *vcpu)
*/
void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
{
- struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
-
- /*
- * If we entered the guest with the vtimer output asserted we have to
- * check if the guest has modified the timer so that we should lower
- * the line at this point.
- */
- if (vtimer->irq.level) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- vtimer->cnt_cval = read_sysreg_el0(cntv_cval);
- if (!kvm_timer_should_fire(vtimer)) {
- kvm_timer_update_irq(vcpu, false, vtimer);
- unmask_vtimer_irq(vcpu);
- }
- }
+ unmask_vtimer_irq_user(vcpu);
}
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -819,6 +800,22 @@ static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
return true;
}
+bool kvm_arch_timer_get_input_level(int vintid)
+{
+ struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+ struct arch_timer_context *timer;
+
+ if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+ timer = vcpu_vtimer(vcpu);
+ else
+ BUG(); /* We only map the vtimer so far */
+
+ if (timer->loaded)
+ __timer_snapshot_state(timer);
+
+ return kvm_timer_should_fire(timer);
+}
+
int kvm_timer_enable(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
}
ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
- NULL);
+ kvm_arch_timer_get_input_level);
if (ret)
return ret;
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
The VGIC can now support the life-cycle of mapped level-triggered
interrupts, and we no longer have to read back the timer state on every
exit from the VM if we had an asserted timer interrupt signal, because
the VGIC already knows if we hit the unlikely case where the guest
disables the timer without ACKing the virtual timer interrupt.
This means we rework a bit of the code to factor out the functionality
to snapshot the timer state from vtimer_save_state(), and we can reuse
this functionality in the sync path when we have an irqchip in
userspace, and also to support our implementation of the
get_input_level() function for the timer.
This change also means that we can no longer rely on the timer's view of
the interrupt line to set the active state, because we no longer
maintain this state for mapped interrupts when exiting from the guest.
Instead, we only set the active state if the virtual interrupt is
active, and otherwise we simply let the timer fire again and raise the
virtual interrupt from the ISR.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
include/kvm/arm_arch_timer.h | 2 ++
virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
2 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 01ee473517e2..f57f795d704c 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
void kvm_timer_init_vhe(void);
+bool kvm_arch_timer_get_input_level(int vintid);
+
#define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
#define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index e78ba5e20f74..82d4963f63b8 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -343,6 +343,12 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
phys_timer_emulate(vcpu);
}
+static void __timer_snapshot_state(struct arch_timer_context *timer)
+{
+ timer->cnt_ctl = read_sysreg_el0(cntv_ctl);
+ timer->cnt_cval = read_sysreg_el0(cntv_cval);
+}
+
static void vtimer_save_state(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -354,10 +360,8 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
if (!vtimer->loaded)
goto out;
- if (timer->enabled) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- vtimer->cnt_cval = read_sysreg_el0(cntv_cval);
- }
+ if (timer->enabled)
+ __timer_snapshot_state(vtimer);
/* Disable the virtual timer */
write_sysreg_el0(0, cntv_ctl);
@@ -454,8 +458,7 @@ static void kvm_timer_vcpu_load_vgic(struct kvm_vcpu *vcpu)
bool phys_active;
int ret;
- phys_active = vtimer->irq.level ||
- kvm_vgic_map_is_active(vcpu, vtimer->irq.irq);
+ phys_active = kvm_vgic_map_is_active(vcpu, vtimer->irq.irq);
ret = irq_set_irqchip_state(host_vtimer_irq,
IRQCHIP_STATE_ACTIVE,
@@ -541,27 +544,19 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
set_cntvoff(0);
}
-static void unmask_vtimer_irq(struct kvm_vcpu *vcpu)
+/*
+ * With a userspace irqchip we have to check if the guest de-asserted the
+ * timer and if so, unmask the timer irq signal on the host interrupt
+ * controller to ensure that we see future timer signals.
+ */
+static void unmask_vtimer_irq_user(struct kvm_vcpu *vcpu)
{
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
if (unlikely(!irqchip_in_kernel(vcpu->kvm))) {
- kvm_vtimer_update_mask_user(vcpu);
- return;
- }
-
- /*
- * If the guest disabled the timer without acking the interrupt, then
- * we must make sure the physical and virtual active states are in
- * sync by deactivating the physical interrupt, because otherwise we
- * wouldn't see the next timer interrupt in the host.
- */
- if (!kvm_vgic_map_is_active(vcpu, vtimer->irq.irq)) {
- int ret;
- ret = irq_set_irqchip_state(host_vtimer_irq,
- IRQCHIP_STATE_ACTIVE,
- false);
- WARN_ON(ret);
+ __timer_snapshot_state(vtimer);
+ if (!kvm_timer_should_fire(vtimer))
+ kvm_vtimer_update_mask_user(vcpu);
}
}
@@ -574,21 +569,7 @@ static void unmask_vtimer_irq(struct kvm_vcpu *vcpu)
*/
void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
{
- struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
-
- /*
- * If we entered the guest with the vtimer output asserted we have to
- * check if the guest has modified the timer so that we should lower
- * the line at this point.
- */
- if (vtimer->irq.level) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- vtimer->cnt_cval = read_sysreg_el0(cntv_cval);
- if (!kvm_timer_should_fire(vtimer)) {
- kvm_timer_update_irq(vcpu, false, vtimer);
- unmask_vtimer_irq(vcpu);
- }
- }
+ unmask_vtimer_irq_user(vcpu);
}
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -819,6 +800,22 @@ static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
return true;
}
+bool kvm_arch_timer_get_input_level(int vintid)
+{
+ struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+ struct arch_timer_context *timer;
+
+ if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+ timer = vcpu_vtimer(vcpu);
+ else
+ BUG(); /* We only map the vtimer so far */
+
+ if (timer->loaded)
+ __timer_snapshot_state(timer);
+
+ return kvm_timer_should_fire(timer);
+}
+
int kvm_timer_enable(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
}
ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
- NULL);
+ kvm_arch_timer_get_input_level);
if (ret)
return ret;
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
2017-12-04 20:05 ` Christoffer Dall
@ 2017-12-05 15:24 ` Yury Norov
-1 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 15:24 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvm, Marc Zyngier, Andre Przywara, kvmarm, linux-arm-kernel
On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> The VGIC can now support the life-cycle of mapped level-triggered
> interrupts, and we no longer have to read back the timer state on every
> exit from the VM if we had an asserted timer interrupt signal, because
> the VGIC already knows if we hit the unlikely case where the guest
> disables the timer without ACKing the virtual timer interrupt.
>
> This means we rework a bit of the code to factor out the functionality
> to snapshot the timer state from vtimer_save_state(), and we can reuse
> this functionality in the sync path when we have an irqchip in
> userspace, and also to support our implementation of the
> get_input_level() function for the timer.
>
> This change also means that we can no longer rely on the timer's view of
> the interrupt line to set the active state, because we no longer
> maintain this state for mapped interrupts when exiting from the guest.
> Instead, we only set the active state if the virtual interrupt is
> active, and otherwise we simply let the timer fire again and raise the
> virtual interrupt from the ISR.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> include/kvm/arm_arch_timer.h | 2 ++
> virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> 2 files changed, 38 insertions(+), 39 deletions(-)
>
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 01ee473517e2..f57f795d704c 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>
> void kvm_timer_init_vhe(void);
>
> +bool kvm_arch_timer_get_input_level(int vintid);
> +
> #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
>
[...]
> +bool kvm_arch_timer_get_input_level(int vintid)
> +{
> + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> + struct arch_timer_context *timer;
> +
> + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> + timer = vcpu_vtimer(vcpu);
> + else
> + BUG(); /* We only map the vtimer so far */
> +
> + if (timer->loaded)
> + __timer_snapshot_state(timer);
> +
> + return kvm_timer_should_fire(timer);
> +}
I think it worth to reword to highlight your intention about BUG,
and save 1 call to vcpu_vtimer()
bool kvm_arch_timer_get_input_level(int vintid)
{
struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
struct arch_timer_context *timer = vcpu_vtimer(vcpu);
/* We only map the vtimer so far */
BUG_ON(vintid != timer->irq.irq)
if (timer->loaded)
__timer_snapshot_state(timer);
return kvm_timer_should_fire(timer);
}
> int kvm_timer_enable(struct kvm_vcpu *vcpu)
> {
> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> @@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> }
>
> ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
> - NULL);
> + kvm_arch_timer_get_input_level);
> if (ret)
> return ret;
>
> --
> 2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
@ 2017-12-05 15:24 ` Yury Norov
0 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-05 15:24 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> The VGIC can now support the life-cycle of mapped level-triggered
> interrupts, and we no longer have to read back the timer state on every
> exit from the VM if we had an asserted timer interrupt signal, because
> the VGIC already knows if we hit the unlikely case where the guest
> disables the timer without ACKing the virtual timer interrupt.
>
> This means we rework a bit of the code to factor out the functionality
> to snapshot the timer state from vtimer_save_state(), and we can reuse
> this functionality in the sync path when we have an irqchip in
> userspace, and also to support our implementation of the
> get_input_level() function for the timer.
>
> This change also means that we can no longer rely on the timer's view of
> the interrupt line to set the active state, because we no longer
> maintain this state for mapped interrupts when exiting from the guest.
> Instead, we only set the active state if the virtual interrupt is
> active, and otherwise we simply let the timer fire again and raise the
> virtual interrupt from the ISR.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> include/kvm/arm_arch_timer.h | 2 ++
> virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> 2 files changed, 38 insertions(+), 39 deletions(-)
>
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 01ee473517e2..f57f795d704c 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>
> void kvm_timer_init_vhe(void);
>
> +bool kvm_arch_timer_get_input_level(int vintid);
> +
> #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
>
[...]
> +bool kvm_arch_timer_get_input_level(int vintid)
> +{
> + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> + struct arch_timer_context *timer;
> +
> + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> + timer = vcpu_vtimer(vcpu);
> + else
> + BUG(); /* We only map the vtimer so far */
> +
> + if (timer->loaded)
> + __timer_snapshot_state(timer);
> +
> + return kvm_timer_should_fire(timer);
> +}
I think it worth to reword to highlight your intention about BUG,
and save 1 call to vcpu_vtimer()
bool kvm_arch_timer_get_input_level(int vintid)
{
struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
struct arch_timer_context *timer = vcpu_vtimer(vcpu);
/* We only map the vtimer so far */
BUG_ON(vintid != timer->irq.irq)
if (timer->loaded)
__timer_snapshot_state(timer);
return kvm_timer_should_fire(timer);
}
> int kvm_timer_enable(struct kvm_vcpu *vcpu)
> {
> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> @@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> }
>
> ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
> - NULL);
> + kvm_arch_timer_get_input_level);
> if (ret)
> return ret;
>
> --
> 2.14.2
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
2017-12-05 15:24 ` Yury Norov
@ 2017-12-06 10:59 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-06 10:59 UTC (permalink / raw)
To: Yury Norov
Cc: Christoffer Dall, kvm, Marc Zyngier, Andre Przywara, kvmarm,
linux-arm-kernel
On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > The VGIC can now support the life-cycle of mapped level-triggered
> > interrupts, and we no longer have to read back the timer state on every
> > exit from the VM if we had an asserted timer interrupt signal, because
> > the VGIC already knows if we hit the unlikely case where the guest
> > disables the timer without ACKing the virtual timer interrupt.
> >
> > This means we rework a bit of the code to factor out the functionality
> > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > this functionality in the sync path when we have an irqchip in
> > userspace, and also to support our implementation of the
> > get_input_level() function for the timer.
> >
> > This change also means that we can no longer rely on the timer's view of
> > the interrupt line to set the active state, because we no longer
> > maintain this state for mapped interrupts when exiting from the guest.
> > Instead, we only set the active state if the virtual interrupt is
> > active, and otherwise we simply let the timer fire again and raise the
> > virtual interrupt from the ISR.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > include/kvm/arm_arch_timer.h | 2 ++
> > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > 2 files changed, 38 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > index 01ee473517e2..f57f795d704c 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
> > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> >
> > void kvm_timer_init_vhe(void);
> >
> > +bool kvm_arch_timer_get_input_level(int vintid);
> > +
> > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> >
>
> [...]
>
> > +bool kvm_arch_timer_get_input_level(int vintid)
> > +{
> > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > + struct arch_timer_context *timer;
> > +
> > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > + timer = vcpu_vtimer(vcpu);
> > + else
> > + BUG(); /* We only map the vtimer so far */
> > +
> > + if (timer->loaded)
> > + __timer_snapshot_state(timer);
> > +
> > + return kvm_timer_should_fire(timer);
> > +}
>
> I think it worth to reword to highlight your intention about BUG,
> and save 1 call to vcpu_vtimer()
>
> bool kvm_arch_timer_get_input_level(int vintid)
> {
> struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> struct arch_timer_context *timer = vcpu_vtimer(vcpu);
>
> /* We only map the vtimer so far */
> BUG_ON(vintid != timer->irq.irq)
>
> if (timer->loaded)
> __timer_snapshot_state(timer);
>
> return kvm_timer_should_fire(timer);
> }
>
Besides the incredible bikesheding nature of your comments, I disagree.
The current code suggests where to add functionality once we move to
using the physical timer hardware to drive an EL1 physical timer on VHE
systems, and is purposely written this way.
I'm sure we have real bugs and real issues in the code, perhaps you
could spend your energy looking for those, and if you cannot find any,
then provide a reviewed-by instead of these pointless cosmetic
adjustments.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
@ 2017-12-06 10:59 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-06 10:59 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > The VGIC can now support the life-cycle of mapped level-triggered
> > interrupts, and we no longer have to read back the timer state on every
> > exit from the VM if we had an asserted timer interrupt signal, because
> > the VGIC already knows if we hit the unlikely case where the guest
> > disables the timer without ACKing the virtual timer interrupt.
> >
> > This means we rework a bit of the code to factor out the functionality
> > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > this functionality in the sync path when we have an irqchip in
> > userspace, and also to support our implementation of the
> > get_input_level() function for the timer.
> >
> > This change also means that we can no longer rely on the timer's view of
> > the interrupt line to set the active state, because we no longer
> > maintain this state for mapped interrupts when exiting from the guest.
> > Instead, we only set the active state if the virtual interrupt is
> > active, and otherwise we simply let the timer fire again and raise the
> > virtual interrupt from the ISR.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > include/kvm/arm_arch_timer.h | 2 ++
> > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > 2 files changed, 38 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > index 01ee473517e2..f57f795d704c 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
> > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> >
> > void kvm_timer_init_vhe(void);
> >
> > +bool kvm_arch_timer_get_input_level(int vintid);
> > +
> > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> >
>
> [...]
>
> > +bool kvm_arch_timer_get_input_level(int vintid)
> > +{
> > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > + struct arch_timer_context *timer;
> > +
> > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > + timer = vcpu_vtimer(vcpu);
> > + else
> > + BUG(); /* We only map the vtimer so far */
> > +
> > + if (timer->loaded)
> > + __timer_snapshot_state(timer);
> > +
> > + return kvm_timer_should_fire(timer);
> > +}
>
> I think it worth to reword to highlight your intention about BUG,
> and save 1 call to vcpu_vtimer()
>
> bool kvm_arch_timer_get_input_level(int vintid)
> {
> struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> struct arch_timer_context *timer = vcpu_vtimer(vcpu);
>
> /* We only map the vtimer so far */
> BUG_ON(vintid != timer->irq.irq)
>
> if (timer->loaded)
> __timer_snapshot_state(timer);
>
> return kvm_timer_should_fire(timer);
> }
>
Besides the incredible bikesheding nature of your comments, I disagree.
The current code suggests where to add functionality once we move to
using the physical timer hardware to drive an EL1 physical timer on VHE
systems, and is purposely written this way.
I'm sure we have real bugs and real issues in the code, perhaps you
could spend your energy looking for those, and if you cannot find any,
then provide a reviewed-by instead of these pointless cosmetic
adjustments.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
2017-12-06 10:59 ` Christoffer Dall
@ 2017-12-06 14:17 ` Yury Norov
-1 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-06 14:17 UTC (permalink / raw)
To: Christoffer Dall
Cc: Christoffer Dall, kvmarm, linux-arm-kernel, kvm, Marc Zyngier,
Andre Przywara, Eric Auger
On Wed, Dec 06, 2017 at 11:59:04AM +0100, Christoffer Dall wrote:
> On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > > From: Christoffer Dall <christoffer.dall@linaro.org>
> > >
> > > The VGIC can now support the life-cycle of mapped level-triggered
> > > interrupts, and we no longer have to read back the timer state on every
> > > exit from the VM if we had an asserted timer interrupt signal, because
> > > the VGIC already knows if we hit the unlikely case where the guest
> > > disables the timer without ACKing the virtual timer interrupt.
> > >
> > > This means we rework a bit of the code to factor out the functionality
> > > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > > this functionality in the sync path when we have an irqchip in
> > > userspace, and also to support our implementation of the
> > > get_input_level() function for the timer.
> > >
> > > This change also means that we can no longer rely on the timer's view of
> > > the interrupt line to set the active state, because we no longer
> > > maintain this state for mapped interrupts when exiting from the guest.
> > > Instead, we only set the active state if the virtual interrupt is
> > > active, and otherwise we simply let the timer fire again and raise the
> > > virtual interrupt from the ISR.
> > >
> > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > > ---
> > > include/kvm/arm_arch_timer.h | 2 ++
> > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > > 2 files changed, 38 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > > index 01ee473517e2..f57f795d704c 100644
> > > --- a/include/kvm/arm_arch_timer.h
> > > +++ b/include/kvm/arm_arch_timer.h
> > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> > >
> > > void kvm_timer_init_vhe(void);
> > >
> > > +bool kvm_arch_timer_get_input_level(int vintid);
> > > +
> > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> > >
> >
> > [...]
> >
> > > +bool kvm_arch_timer_get_input_level(int vintid)
> > > +{
> > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > > + struct arch_timer_context *timer;
> > > +
> > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > > + timer = vcpu_vtimer(vcpu);
> > > + else
> > > + BUG() We only map the vtimer so far */
> > > +
> > > + if (timer->loaded)
> > > + __timer_snapshot_state(timer);
> > > +
> > > + return kvm_timer_should_fire(timer);
> > > +}
> >
> > I think it worth to reword to highlight your intention about BUG,
> > and save 1 call to vcpu_vtimer()
> >
> > bool kvm_arch_timer_get_input_level(int vintid)
> > {
> > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > struct arch_timer_context *timer = vcpu_vtimer(vcpu);
> >
> > /* We only map the vtimer so far */
> > BUG_ON(vintid != timer->irq.irq)
> >
> > if (timer->loaded)
> > __timer_snapshot_state(timer);
> >
> > return kvm_timer_should_fire(timer);
> > }
> >
>
> Besides the incredible bikesheding nature of your comments, I disagree.
> The current code suggests where to add functionality once we move to
> using the physical timer hardware to drive an EL1 physical timer on VHE
> systems, and is purposely written this way.
>
> I'm sure we have real bugs and real issues in the code, perhaps you
> could spend your energy looking for those, and if you cannot find any,
> then provide a reviewed-by instead of these pointless cosmetic
> adjustments.
OK. I understood. Let me elaborate.
0. As you say in patch 0, This series is based on 4.15-rc, so I decided that
the code above is assumed to be release version. You may change it in
future, or may not, but the code will exist as is in mainline kernel for
some time, right?
1. BUG() is needed to kill system, and it really kills it. This is wrong
to kill system in else-branch of some minor helper due to unimplemented
feature. You should use pr_err() or WARN_ON() instead. The best - return
reasonable error and do everything possible on upper levels to keep system
alive. What's really wrong in my comment is that I didn't suggest you switch
to WARN_ON().
Did you follow this thread?
https://lkml.org/lkml/2016/10/4/1
2. Calling the same function with the same argument twice in code path is
also an issue. Besides the nasty feeling of that code, it might be dangerous.
The most obvious scenario is when it returns different values due to change
of internal state. I realize that vcpu_vtimer() is just a pointer dereference.
But it may bite you painfully as well. I know it for sure because it bitten
me once. Consider racing scenario in this patch:
https://patchwork.kernel.org/patch/9974327/
It may never become the problem, or may become one day. But fix is
clear and obvious - why not taking it?
3. I will be really happy to provide tested-by and reviewed-by. But
for doing that I need to actually test and review. It would be extremely
helpful if you share your testing modules and scripts. I have access
to several Cavium machines and can do before/after measurements. Right
now I have few tests, but kvm is very complex system, and I'm not sure I
measure what I'm actually going to. Guidance from KVM experts is what
really lacks.
Thanks,
Yury
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
@ 2017-12-06 14:17 ` Yury Norov
0 siblings, 0 replies; 40+ messages in thread
From: Yury Norov @ 2017-12-06 14:17 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 06, 2017 at 11:59:04AM +0100, Christoffer Dall wrote:
> On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > > From: Christoffer Dall <christoffer.dall@linaro.org>
> > >
> > > The VGIC can now support the life-cycle of mapped level-triggered
> > > interrupts, and we no longer have to read back the timer state on every
> > > exit from the VM if we had an asserted timer interrupt signal, because
> > > the VGIC already knows if we hit the unlikely case where the guest
> > > disables the timer without ACKing the virtual timer interrupt.
> > >
> > > This means we rework a bit of the code to factor out the functionality
> > > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > > this functionality in the sync path when we have an irqchip in
> > > userspace, and also to support our implementation of the
> > > get_input_level() function for the timer.
> > >
> > > This change also means that we can no longer rely on the timer's view of
> > > the interrupt line to set the active state, because we no longer
> > > maintain this state for mapped interrupts when exiting from the guest.
> > > Instead, we only set the active state if the virtual interrupt is
> > > active, and otherwise we simply let the timer fire again and raise the
> > > virtual interrupt from the ISR.
> > >
> > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > > ---
> > > include/kvm/arm_arch_timer.h | 2 ++
> > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > > 2 files changed, 38 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > > index 01ee473517e2..f57f795d704c 100644
> > > --- a/include/kvm/arm_arch_timer.h
> > > +++ b/include/kvm/arm_arch_timer.h
> > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> > >
> > > void kvm_timer_init_vhe(void);
> > >
> > > +bool kvm_arch_timer_get_input_level(int vintid);
> > > +
> > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> > >
> >
> > [...]
> >
> > > +bool kvm_arch_timer_get_input_level(int vintid)
> > > +{
> > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > > + struct arch_timer_context *timer;
> > > +
> > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > > + timer = vcpu_vtimer(vcpu);
> > > + else
> > > + BUG() We only map the vtimer so far */
> > > +
> > > + if (timer->loaded)
> > > + __timer_snapshot_state(timer);
> > > +
> > > + return kvm_timer_should_fire(timer);
> > > +}
> >
> > I think it worth to reword to highlight your intention about BUG,
> > and save 1 call to vcpu_vtimer()
> >
> > bool kvm_arch_timer_get_input_level(int vintid)
> > {
> > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > struct arch_timer_context *timer = vcpu_vtimer(vcpu);
> >
> > /* We only map the vtimer so far */
> > BUG_ON(vintid != timer->irq.irq)
> >
> > if (timer->loaded)
> > __timer_snapshot_state(timer);
> >
> > return kvm_timer_should_fire(timer);
> > }
> >
>
> Besides the incredible bikesheding nature of your comments, I disagree.
> The current code suggests where to add functionality once we move to
> using the physical timer hardware to drive an EL1 physical timer on VHE
> systems, and is purposely written this way.
>
> I'm sure we have real bugs and real issues in the code, perhaps you
> could spend your energy looking for those, and if you cannot find any,
> then provide a reviewed-by instead of these pointless cosmetic
> adjustments.
OK. I understood. Let me elaborate.
0. As you say in patch 0, This series is based on 4.15-rc, so I decided that
the code above is assumed to be release version. You may change it in
future, or may not, but the code will exist as is in mainline kernel for
some time, right?
1. BUG() is needed to kill system, and it really kills it. This is wrong
to kill system in else-branch of some minor helper due to unimplemented
feature. You should use pr_err() or WARN_ON() instead. The best - return
reasonable error and do everything possible on upper levels to keep system
alive. What's really wrong in my comment is that I didn't suggest you switch
to WARN_ON().
Did you follow this thread?
https://lkml.org/lkml/2016/10/4/1
2. Calling the same function with the same argument twice in code path is
also an issue. Besides the nasty feeling of that code, it might be dangerous.
The most obvious scenario is when it returns different values due to change
of internal state. I realize that vcpu_vtimer() is just a pointer dereference.
But it may bite you painfully as well. I know it for sure because it bitten
me once. Consider racing scenario in this patch:
https://patchwork.kernel.org/patch/9974327/
It may never become the problem, or may become one day. But fix is
clear and obvious - why not taking it?
3. I will be really happy to provide tested-by and reviewed-by. But
for doing that I need to actually test and review. It would be extremely
helpful if you share your testing modules and scripts. I have access
to several Cavium machines and can do before/after measurements. Right
now I have few tests, but kvm is very complex system, and I'm not sure I
measure what I'm actually going to. Guidance from KVM experts is what
really lacks.
Thanks,
Yury
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
2017-12-06 14:17 ` Yury Norov
@ 2017-12-06 16:38 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-06 16:38 UTC (permalink / raw)
To: Yury Norov
Cc: Christoffer Dall, kvmarm, linux-arm-kernel, kvm, Marc Zyngier,
Andre Przywara, Eric Auger
On Wed, Dec 06, 2017 at 05:17:28PM +0300, Yury Norov wrote:
> On Wed, Dec 06, 2017 at 11:59:04AM +0100, Christoffer Dall wrote:
> > On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> > > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > > > From: Christoffer Dall <christoffer.dall@linaro.org>
> > > >
> > > > The VGIC can now support the life-cycle of mapped level-triggered
> > > > interrupts, and we no longer have to read back the timer state on every
> > > > exit from the VM if we had an asserted timer interrupt signal, because
> > > > the VGIC already knows if we hit the unlikely case where the guest
> > > > disables the timer without ACKing the virtual timer interrupt.
> > > >
> > > > This means we rework a bit of the code to factor out the functionality
> > > > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > > > this functionality in the sync path when we have an irqchip in
> > > > userspace, and also to support our implementation of the
> > > > get_input_level() function for the timer.
> > > >
> > > > This change also means that we can no longer rely on the timer's view of
> > > > the interrupt line to set the active state, because we no longer
> > > > maintain this state for mapped interrupts when exiting from the guest.
> > > > Instead, we only set the active state if the virtual interrupt is
> > > > active, and otherwise we simply let the timer fire again and raise the
> > > > virtual interrupt from the ISR.
> > > >
> > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > > > ---
> > > > include/kvm/arm_arch_timer.h | 2 ++
> > > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > > > 2 files changed, 38 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > > > index 01ee473517e2..f57f795d704c 100644
> > > > --- a/include/kvm/arm_arch_timer.h
> > > > +++ b/include/kvm/arm_arch_timer.h
> > > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> > > >
> > > > void kvm_timer_init_vhe(void);
> > > >
> > > > +bool kvm_arch_timer_get_input_level(int vintid);
> > > > +
> > > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> > > >
> > >
> > > [...]
> > >
> > > > +bool kvm_arch_timer_get_input_level(int vintid)
> > > > +{
> > > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > > > + struct arch_timer_context *timer;
> > > > +
> > > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > > > + timer = vcpu_vtimer(vcpu);
> > > > + else
> > > > + BUG() We only map the vtimer so far */
> > > > +
> > > > + if (timer->loaded)
> > > > + __timer_snapshot_state(timer);
> > > > +
> > > > + return kvm_timer_should_fire(timer);
> > > > +}
> > >
> > > I think it worth to reword to highlight your intention about BUG,
> > > and save 1 call to vcpu_vtimer()
> > >
> > > bool kvm_arch_timer_get_input_level(int vintid)
> > > {
> > > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > > struct arch_timer_context *timer = vcpu_vtimer(vcpu);
> > >
> > > /* We only map the vtimer so far */
> > > BUG_ON(vintid != timer->irq.irq)
> > >
> > > if (timer->loaded)
> > > __timer_snapshot_state(timer);
> > >
> > > return kvm_timer_should_fire(timer);
> > > }
> > >
> >
> > Besides the incredible bikesheding nature of your comments, I disagree.
> > The current code suggests where to add functionality once we move to
> > using the physical timer hardware to drive an EL1 physical timer on VHE
> > systems, and is purposely written this way.
> >
> > I'm sure we have real bugs and real issues in the code, perhaps you
> > could spend your energy looking for those, and if you cannot find any,
> > then provide a reviewed-by instead of these pointless cosmetic
> > adjustments.
>
> OK. I understood. Let me elaborate.
>
> 0. As you say in patch 0, This series is based on 4.15-rc, so I decided that
> the code above is assumed to be release version. You may change it in
> future, or may not, but the code will exist as is in mainline kernel for
> some time, right?
When the code is properly reviewed and nobody raises relevant
objections, the maintainers of the subsystem can merge the code.
>
> 1. BUG() is needed to kill system, and it really kills it. This is wrong
> to kill system in else-branch of some minor helper due to unimplemented
> feature. You should use pr_err() or WARN_ON() instead. The best - return
> reasonable error and do everything possible on upper levels to keep system
> alive. What's really wrong in my comment is that I didn't suggest you switch
> to WARN_ON().
>
You're missing a key point. If the system can reasonably recover from a
failure or some condition, it's definitely preferred to use a print or a
warning.
However, if an internal function is called with an unsupported value,
when managing the operation of hardware, the most sane thing to do is to
panic, because the system is in an enitrely unsupported state.
>
> 2. Calling the same function with the same argument twice in code path is
> also an issue. Besides the nasty feeling of that code, it might be dangerous.
> The most obvious scenario is when it returns different values due to change
> of internal state. I realize that vcpu_vtimer() is just a pointer dereference.
> But it may bite you painfully as well. I know it for sure because it bitten
> me once. Consider racing scenario in this patch:
> https://patchwork.kernel.org/patch/9974327/
Seriously, what make you think I need a programming lesson from you?
You are taking a specific lesson and generalizing it, and then applying
it to a piece of code you admit to not understanding. This is not
helpful.
I really don't care about your nasty feelings at this point, and
claiming that calling the same function twice is in general a problem is
a ridiculous statement.
>
> It may never become the problem, or may become one day. But fix is
> clear and obvious - why not taking it?
I think I've answered this already, but that's because this function
will eventually become:
bool kvm_arch_timer_get_input_level(int vintid)
{
struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
struct arch_timer_context *timer;
if (vintid == vcpu_vtimer(vcpu)->irq.irq)
timer = vcpu_vtimer(vcpu);
else
timer = vcpu_ptimer(vcpu);
if (timer->loaded)
__timer_snapshot_state(timer);
return kvm_timer_should_fire(timer);
}
>
> 3. I will be really happy to provide tested-by and reviewed-by. But
> for doing that I need to actually test and review. It would be extremely
> helpful if you share your testing modules and scripts. I have access
> to several Cavium machines and can do before/after measurements. Right
> now I have few tests, but kvm is very complex system, and I'm not sure I
> measure what I'm actually going to. Guidance from KVM experts is what
> really lacks.
>
If you don't understand what KVM is and how to use it, I'm afraid your
input is not very valuable.
Until you change your attitude and make some attempt to understand how
we work, and try to actually understand the code you comment on, I'll be
inclined to ignore future posting from you.
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v6 7/8] KVM: arm/arm64: Provide a get_input_level for the arch timer
@ 2017-12-06 16:38 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-06 16:38 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 06, 2017 at 05:17:28PM +0300, Yury Norov wrote:
> On Wed, Dec 06, 2017 at 11:59:04AM +0100, Christoffer Dall wrote:
> > On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> > > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > > > From: Christoffer Dall <christoffer.dall@linaro.org>
> > > >
> > > > The VGIC can now support the life-cycle of mapped level-triggered
> > > > interrupts, and we no longer have to read back the timer state on every
> > > > exit from the VM if we had an asserted timer interrupt signal, because
> > > > the VGIC already knows if we hit the unlikely case where the guest
> > > > disables the timer without ACKing the virtual timer interrupt.
> > > >
> > > > This means we rework a bit of the code to factor out the functionality
> > > > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > > > this functionality in the sync path when we have an irqchip in
> > > > userspace, and also to support our implementation of the
> > > > get_input_level() function for the timer.
> > > >
> > > > This change also means that we can no longer rely on the timer's view of
> > > > the interrupt line to set the active state, because we no longer
> > > > maintain this state for mapped interrupts when exiting from the guest.
> > > > Instead, we only set the active state if the virtual interrupt is
> > > > active, and otherwise we simply let the timer fire again and raise the
> > > > virtual interrupt from the ISR.
> > > >
> > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > > > ---
> > > > include/kvm/arm_arch_timer.h | 2 ++
> > > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > > > 2 files changed, 38 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > > > index 01ee473517e2..f57f795d704c 100644
> > > > --- a/include/kvm/arm_arch_timer.h
> > > > +++ b/include/kvm/arm_arch_timer.h
> > > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> > > >
> > > > void kvm_timer_init_vhe(void);
> > > >
> > > > +bool kvm_arch_timer_get_input_level(int vintid);
> > > > +
> > > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> > > >
> > >
> > > [...]
> > >
> > > > +bool kvm_arch_timer_get_input_level(int vintid)
> > > > +{
> > > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > > > + struct arch_timer_context *timer;
> > > > +
> > > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > > > + timer = vcpu_vtimer(vcpu);
> > > > + else
> > > > + BUG() We only map the vtimer so far */
> > > > +
> > > > + if (timer->loaded)
> > > > + __timer_snapshot_state(timer);
> > > > +
> > > > + return kvm_timer_should_fire(timer);
> > > > +}
> > >
> > > I think it worth to reword to highlight your intention about BUG,
> > > and save 1 call to vcpu_vtimer()
> > >
> > > bool kvm_arch_timer_get_input_level(int vintid)
> > > {
> > > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > > struct arch_timer_context *timer = vcpu_vtimer(vcpu);
> > >
> > > /* We only map the vtimer so far */
> > > BUG_ON(vintid != timer->irq.irq)
> > >
> > > if (timer->loaded)
> > > __timer_snapshot_state(timer);
> > >
> > > return kvm_timer_should_fire(timer);
> > > }
> > >
> >
> > Besides the incredible bikesheding nature of your comments, I disagree.
> > The current code suggests where to add functionality once we move to
> > using the physical timer hardware to drive an EL1 physical timer on VHE
> > systems, and is purposely written this way.
> >
> > I'm sure we have real bugs and real issues in the code, perhaps you
> > could spend your energy looking for those, and if you cannot find any,
> > then provide a reviewed-by instead of these pointless cosmetic
> > adjustments.
>
> OK. I understood. Let me elaborate.
>
> 0. As you say in patch 0, This series is based on 4.15-rc, so I decided that
> the code above is assumed to be release version. You may change it in
> future, or may not, but the code will exist as is in mainline kernel for
> some time, right?
When the code is properly reviewed and nobody raises relevant
objections, the maintainers of the subsystem can merge the code.
>
> 1. BUG() is needed to kill system, and it really kills it. This is wrong
> to kill system in else-branch of some minor helper due to unimplemented
> feature. You should use pr_err() or WARN_ON() instead. The best - return
> reasonable error and do everything possible on upper levels to keep system
> alive. What's really wrong in my comment is that I didn't suggest you switch
> to WARN_ON().
>
You're missing a key point. If the system can reasonably recover from a
failure or some condition, it's definitely preferred to use a print or a
warning.
However, if an internal function is called with an unsupported value,
when managing the operation of hardware, the most sane thing to do is to
panic, because the system is in an enitrely unsupported state.
>
> 2. Calling the same function with the same argument twice in code path is
> also an issue. Besides the nasty feeling of that code, it might be dangerous.
> The most obvious scenario is when it returns different values due to change
> of internal state. I realize that vcpu_vtimer() is just a pointer dereference.
> But it may bite you painfully as well. I know it for sure because it bitten
> me once. Consider racing scenario in this patch:
> https://patchwork.kernel.org/patch/9974327/
Seriously, what make you think I need a programming lesson from you?
You are taking a specific lesson and generalizing it, and then applying
it to a piece of code you admit to not understanding. This is not
helpful.
I really don't care about your nasty feelings at this point, and
claiming that calling the same function twice is in general a problem is
a ridiculous statement.
>
> It may never become the problem, or may become one day. But fix is
> clear and obvious - why not taking it?
I think I've answered this already, but that's because this function
will eventually become:
bool kvm_arch_timer_get_input_level(int vintid)
{
struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
struct arch_timer_context *timer;
if (vintid == vcpu_vtimer(vcpu)->irq.irq)
timer = vcpu_vtimer(vcpu);
else
timer = vcpu_ptimer(vcpu);
if (timer->loaded)
__timer_snapshot_state(timer);
return kvm_timer_should_fire(timer);
}
>
> 3. I will be really happy to provide tested-by and reviewed-by. But
> for doing that I need to actually test and review. It would be extremely
> helpful if you share your testing modules and scripts. I have access
> to several Cavium machines and can do before/after measurements. Right
> now I have few tests, but kvm is very complex system, and I'm not sure I
> measure what I'm actually going to. Guidance from KVM experts is what
> really lacks.
>
If you don't understand what KVM is and how to use it, I'm afraid your
input is not very valuable.
Until you change your attitude and make some attempt to understand how
we work, and try to actually understand the code you comment on, I'll be
inclined to ignore future posting from you.
-Christoffer
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v6 8/8] KVM: arm/arm64: Avoid work when userspace iqchips are not used
2017-12-04 20:04 ` Christoffer Dall
@ 2017-12-04 20:05 ` Christoffer Dall
-1 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: kvmarm; +Cc: kvm, Marc Zyngier, Andre Przywara, linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
We currently check if the VM has a userspace irqchip on every exit from
the VCPU, and if so, we do some work to ensure correct timer behavior.
This is unfortunate, as we could avoid doing any work entirely, if we
didn't have to support irqchip in userspace.
Realizing the userspace irqchip on ARM is mostly a developer or hobby
feature, and is unlikely to be used in servers or other scenarios where
performance is a priority, we can use a refcounted static key to only
check the irqchip configuration when we have at least one VM that uses
an irqchip in userspace.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arch_timer.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 82d4963f63b8..df21451e7654 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -51,6 +51,8 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
struct arch_timer_context *timer_ctx);
static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
+static DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
+
u64 kvm_phys_timer_read(void)
{
return timecounter->cc->read(timecounter->cc);
@@ -569,7 +571,8 @@ static void unmask_vtimer_irq_user(struct kvm_vcpu *vcpu)
*/
void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
{
- unmask_vtimer_irq_user(vcpu);
+ if (static_branch_unlikely(&userspace_irqchip_in_use))
+ unmask_vtimer_irq_user(vcpu);
}
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -774,6 +777,8 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
soft_timer_cancel(&timer->bg_timer, &timer->expired);
soft_timer_cancel(&timer->phys_timer, NULL);
kvm_vgic_unmap_phys_irq(vcpu, vtimer->irq.irq);
+ if (timer->enabled && !irqchip_in_kernel(vcpu->kvm))
+ static_branch_dec(&userspace_irqchip_in_use);
}
static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
@@ -826,8 +831,10 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return 0;
/* Without a VGIC we do not map virtual IRQs to physical IRQs */
- if (!irqchip_in_kernel(vcpu->kvm))
+ if (!irqchip_in_kernel(vcpu->kvm)) {
+ static_branch_inc(&userspace_irqchip_in_use);
goto no_vgic;
+ }
if (!vgic_initialized(vcpu->kvm))
return -ENODEV;
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v6 8/8] KVM: arm/arm64: Avoid work when userspace iqchips are not used
@ 2017-12-04 20:05 ` Christoffer Dall
0 siblings, 0 replies; 40+ messages in thread
From: Christoffer Dall @ 2017-12-04 20:05 UTC (permalink / raw)
To: linux-arm-kernel
From: Christoffer Dall <christoffer.dall@linaro.org>
We currently check if the VM has a userspace irqchip on every exit from
the VCPU, and if so, we do some work to ensure correct timer behavior.
This is unfortunate, as we could avoid doing any work entirely, if we
didn't have to support irqchip in userspace.
Realizing the userspace irqchip on ARM is mostly a developer or hobby
feature, and is unlikely to be used in servers or other scenarios where
performance is a priority, we can use a refcounted static key to only
check the irqchip configuration when we have at least one VM that uses
an irqchip in userspace.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arch_timer.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 82d4963f63b8..df21451e7654 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -51,6 +51,8 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
struct arch_timer_context *timer_ctx);
static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
+static DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
+
u64 kvm_phys_timer_read(void)
{
return timecounter->cc->read(timecounter->cc);
@@ -569,7 +571,8 @@ static void unmask_vtimer_irq_user(struct kvm_vcpu *vcpu)
*/
void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
{
- unmask_vtimer_irq_user(vcpu);
+ if (static_branch_unlikely(&userspace_irqchip_in_use))
+ unmask_vtimer_irq_user(vcpu);
}
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -774,6 +777,8 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
soft_timer_cancel(&timer->bg_timer, &timer->expired);
soft_timer_cancel(&timer->phys_timer, NULL);
kvm_vgic_unmap_phys_irq(vcpu, vtimer->irq.irq);
+ if (timer->enabled && !irqchip_in_kernel(vcpu->kvm))
+ static_branch_dec(&userspace_irqchip_in_use);
}
static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
@@ -826,8 +831,10 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return 0;
/* Without a VGIC we do not map virtual IRQs to physical IRQs */
- if (!irqchip_in_kernel(vcpu->kvm))
+ if (!irqchip_in_kernel(vcpu->kvm)) {
+ static_branch_inc(&userspace_irqchip_in_use);
goto no_vgic;
+ }
if (!vgic_initialized(vcpu->kvm))
return -ENODEV;
--
2.14.2
^ permalink raw reply related [flat|nested] 40+ messages in thread