[PATCH 0/2] arm/arm64: KVM: Fix arthictected timer issues

[PATCH 0/2] arm/arm64: KVM: Fix arthictected timer issues

[Christoffer Dall]

These two patches fix two separate issues with the architected timer and
the corresponding interrupt injection to VMs on KVM/ARM.

The first patch fixes an issue introduced with the active timer state
switching series recently merged for v4.3, which could cause a guest to
loop without progress if another VCPU is run on the same physical CPU
and preempts the original VCPU while the guest is running the ISR for
the timer interrupt.

The second patch resets the architected timer's control register to zero
on system reset, ensuring that interrupts are not injected when a system
resets.  This fixes a long-standing issue with UEFI, where soft reset
initiated from within UEFI prevented the system from booting again.

Christoffer Dall (2):
  arm/arm64: KVM: vgic: Move active state handling to flush_hwstate
  arm/arm64: KVM: arch timer: Reset CNTV_CTL to 0

 virt/kvm/arm/arch_timer.c |  8 ++++++++
 virt/kvm/arm/vgic.c       | 42 ++++++++++++++++++++++++++----------------
 2 files changed, 34 insertions(+), 16 deletions(-)

-- 
2.1.2.330.g565301e.dirty

[PATCH 1/2] arm/arm64: KVM: vgic: Move active state handling to flush_hwstate

[Christoffer Dall]

We currently set the physical active state only when we *inject* a new
pending virtual interrupt, but this is actually not correct, because we
could have been preempted and run something else on the system that
resets the active state to clear.  This causes us to run the VM with the
timer set to fire, but without setting the physical active state.

The solution is to always check the LR configurations, and we if have a
mapped interrupt in the LR in either the pending or active state
(virtual), then set the physical active state.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 9eb489a..6bd1c9b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1144,26 +1144,11 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
 		struct irq_phys_map *map;
 		map = vgic_irq_map_search(vcpu, irq);
 
-		/*
-		 * If we have a mapping, and the virtual interrupt is
-		 * being injected, then we must set the state to
-		 * active in the physical world. Otherwise the
-		 * physical interrupt will fire and the guest will
-		 * exit before processing the virtual interrupt.
-		 */
 		if (map) {
-			int ret;
-
-			BUG_ON(!map->active);
 			vlr.hwirq = map->phys_irq;
 			vlr.state |= LR_HW;
 			vlr.state &= ~LR_EOI_INT;
 
-			ret = irq_set_irqchip_state(map->irq,
-						    IRQCHIP_STATE_ACTIVE,
-						    true);
-			WARN_ON(ret);
-
 			/*
 			 * Make sure we're not going to sample this
 			 * again, as a HW-backed interrupt cannot be
@@ -1255,7 +1240,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	unsigned long *pa_percpu, *pa_shared;
-	int i, vcpu_id;
+	int i, vcpu_id, lr, ret;
 	int overflow = 0;
 	int nr_shared = vgic_nr_shared_irqs(dist);
 
@@ -1310,6 +1295,31 @@ epilog:
 		 */
 		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
 	}
+
+	for (lr = 0; lr < vgic->nr_lr; lr++) {
+		struct vgic_lr vlr;
+
+		if (!test_bit(lr, vgic_cpu->lr_used))
+			continue;
+
+		vlr = vgic_get_lr(vcpu, lr);
+
+		/*
+		 * If we have a mapping, and the virtual interrupt is
+		 * presented to the guest (as pending or active), then we must
+		 * set the state to active in the physical world. See
+		 * Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt.
+		 */
+		if (vlr.state & LR_HW) {
+			struct irq_phys_map *map;
+			map = vgic_irq_map_search(vcpu, vlr.irq);
+
+			ret = irq_set_irqchip_state(map->irq,
+						    IRQCHIP_STATE_ACTIVE,
+						    true);
+			WARN_ON(ret);
+		}
+	}
 }
 
 static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
-- 
2.1.2.330.g565301e.dirty

[PATCH 1/2] arm/arm64: KVM: vgic: Move active state handling to flush_hwstate

[Eric Auger]

Hi,
On 09/04/2015 04:24 PM, Christoffer Dall wrote:
> We currently set the physical active state only when we *inject* a new
> pending virtual interrupt, but this is actually not correct, because we
> could have been preempted and run something else on the system that
> resets the active state to clear.  This causes us to run the VM with the
> timer set to fire, but without setting the physical active state.
> 
> The solution is to always check the LR configurations, and we if have a
> mapped interrupt in the LR in either the pending or active state
> (virtual), then set the physical active state.
> 
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 42 ++++++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 9eb489a..6bd1c9b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1144,26 +1144,11 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
>  		struct irq_phys_map *map;
>  		map = vgic_irq_map_search(vcpu, irq);
>  
> -		/*
> -		 * If we have a mapping, and the virtual interrupt is
> -		 * being injected, then we must set the state to
> -		 * active in the physical world. Otherwise the
> -		 * physical interrupt will fire and the guest will
> -		 * exit before processing the virtual interrupt.
> -		 */
>  		if (map) {
> -			int ret;
> -
> -			BUG_ON(!map->active);
I have a question about this map->active. I did not find any user of
kvm_vgic_set_phys_irq_active anymore. The flag is directly updated in
vgic_sync_hwirq through the irq_get_irqchip_state query. In the same
function, before there is a "BUG_ON(!map || !map->active);"

Can't we have this BUG_ON firing?


>  			vlr.hwirq = map->phys_irq;
>  			vlr.state |= LR_HW;
>  			vlr.state &= ~LR_EOI_INT;
>  
> -			ret = irq_set_irqchip_state(map->irq,
> -						    IRQCHIP_STATE_ACTIVE,
> -						    true);
> -			WARN_ON(ret);
> -
>  			/*
>  			 * Make sure we're not going to sample this
>  			 * again, as a HW-backed interrupt cannot be
> @@ -1255,7 +1240,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	unsigned long *pa_percpu, *pa_shared;
> -	int i, vcpu_id;
> +	int i, vcpu_id, lr, ret;
>  	int overflow = 0;
>  	int nr_shared = vgic_nr_shared_irqs(dist);
>  
> @@ -1310,6 +1295,31 @@ epilog:
>  		 */
>  		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
>  	}
> +
> +	for (lr = 0; lr < vgic->nr_lr; lr++) {
> +		struct vgic_lr vlr;
> +
> +		if (!test_bit(lr, vgic_cpu->lr_used))
> +			continue;
> +
> +		vlr = vgic_get_lr(vcpu, lr);
> +
> +		/*
> +		 * If we have a mapping, and the virtual interrupt is
> +		 * presented to the guest (as pending or active), then we must
> +		 * set the state to active in the physical world. See
> +		 * Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt.
if upstreamed in 4.3 whereas the other series is not there,
vgic-mapped-irqs.txt won't be available.
> +		 */
> +		if (vlr.state & LR_HW) {
> +			struct irq_phys_map *map;
> +			map = vgic_irq_map_search(vcpu, vlr.irq);
> +
> +			ret = irq_set_irqchip_state(map->irq,
> +						    IRQCHIP_STATE_ACTIVE,
> +						    true);
I understand the need for manually setting the phys dist state in case
of timer however for non shared IRQs, GIC does the job directly. But I
guess it does not harm.

Eric
> +			WARN_ON(ret);
> +		}
> +	}
>  }
>  
>  static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> 

[PATCH 1/2] arm/arm64: KVM: vgic: Move active state handling to flush_hwstate

[Eric Auger]

On 09/07/2015 04:44 PM, Eric Auger wrote:
> Hi,
> On 09/04/2015 04:24 PM, Christoffer Dall wrote:
>> We currently set the physical active state only when we *inject* a new
>> pending virtual interrupt, but this is actually not correct, because we
>> could have been preempted and run something else on the system that
>> resets the active state to clear.  This causes us to run the VM with the
>> timer set to fire, but without setting the physical active state.
>>
>> The solution is to always check the LR configurations, and we if have a
>> mapped interrupt in the LR in either the pending or active state
>> (virtual), then set the physical active state.
>>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  virt/kvm/arm/vgic.c | 42 ++++++++++++++++++++++++++----------------
>>  1 file changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 9eb489a..6bd1c9b 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1144,26 +1144,11 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
>>  		struct irq_phys_map *map;
>>  		map = vgic_irq_map_search(vcpu, irq);
>>  
>> -		/*
>> -		 * If we have a mapping, and the virtual interrupt is
>> -		 * being injected, then we must set the state to
>> -		 * active in the physical world. Otherwise the
>> -		 * physical interrupt will fire and the guest will
>> -		 * exit before processing the virtual interrupt.
>> -		 */
>>  		if (map) {
>> -			int ret;
>> -
>> -			BUG_ON(!map->active);
> I have a question about this map->active. I did not find any user of
> kvm_vgic_set_phys_irq_active anymore. The flag is directly updated in
> vgic_sync_hwirq through the irq_get_irqchip_state query. In the same
> function, before there is a "BUG_ON(!map || !map->active);"
> 
> Can't we have this BUG_ON firing?
hum ignore that comment. Didn't see the call to
kvm_vgic_set_phys_irq_active in arch_timer.c

Eric
> 
> 
>>  			vlr.hwirq = map->phys_irq;
>>  			vlr.state |= LR_HW;
>>  			vlr.state &= ~LR_EOI_INT;
>>  
>> -			ret = irq_set_irqchip_state(map->irq,
>> -						    IRQCHIP_STATE_ACTIVE,
>> -						    true);
>> -			WARN_ON(ret);
>> -
>>  			/*
>>  			 * Make sure we're not going to sample this
>>  			 * again, as a HW-backed interrupt cannot be
>> @@ -1255,7 +1240,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>  	unsigned long *pa_percpu, *pa_shared;
>> -	int i, vcpu_id;
>> +	int i, vcpu_id, lr, ret;
>>  	int overflow = 0;
>>  	int nr_shared = vgic_nr_shared_irqs(dist);
>>  
>> @@ -1310,6 +1295,31 @@ epilog:
>>  		 */
>>  		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
>>  	}
>> +
>> +	for (lr = 0; lr < vgic->nr_lr; lr++) {
>> +		struct vgic_lr vlr;
>> +
>> +		if (!test_bit(lr, vgic_cpu->lr_used))
>> +			continue;
>> +
>> +		vlr = vgic_get_lr(vcpu, lr);
>> +
>> +		/*
>> +		 * If we have a mapping, and the virtual interrupt is
>> +		 * presented to the guest (as pending or active), then we must
>> +		 * set the state to active in the physical world. See
>> +		 * Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt.
> if upstreamed in 4.3 whereas the other series is not there,
> vgic-mapped-irqs.txt won't be available.
>> +		 */
>> +		if (vlr.state & LR_HW) {
>> +			struct irq_phys_map *map;
>> +			map = vgic_irq_map_search(vcpu, vlr.irq);
>> +
>> +			ret = irq_set_irqchip_state(map->irq,
>> +						    IRQCHIP_STATE_ACTIVE,
>> +						    true);
> I understand the need for manually setting the phys dist state in case
> of timer however for non shared IRQs, GIC does the job directly. But I
> guess it does not harm.
> 
> Eric
>> +			WARN_ON(ret);
>> +		}
>> +	}
>>  }
>>  
>>  static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>>
> 

[PATCH 1/2] arm/arm64: KVM: vgic: Move active state handling to flush_hwstate

[Marc Zyngier]

On 07/09/15 15:44, Eric Auger wrote:
> Hi,
> On 09/04/2015 04:24 PM, Christoffer Dall wrote:
>> We currently set the physical active state only when we *inject* a new
>> pending virtual interrupt, but this is actually not correct, because we
>> could have been preempted and run something else on the system that
>> resets the active state to clear.  This causes us to run the VM with the
>> timer set to fire, but without setting the physical active state.
>>
>> The solution is to always check the LR configurations, and we if have a
>> mapped interrupt in the LR in either the pending or active state
>> (virtual), then set the physical active state.
>>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  virt/kvm/arm/vgic.c | 42 ++++++++++++++++++++++++++----------------
>>  1 file changed, 26 insertions(+), 16 deletions(-)
>>
[...]
>> +
>> +	for (lr = 0; lr < vgic->nr_lr; lr++) {
>> +		struct vgic_lr vlr;
>> +
>> +		if (!test_bit(lr, vgic_cpu->lr_used))
>> +			continue;
>> +
>> +		vlr = vgic_get_lr(vcpu, lr);
>> +
>> +		/*
>> +		 * If we have a mapping, and the virtual interrupt is
>> +		 * presented to the guest (as pending or active), then we must
>> +		 * set the state to active in the physical world. See
>> +		 * Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt.
> if upstreamed in 4.3 whereas the other series is not there,
> vgic-mapped-irqs.txt won't be available.

Good point, I'll update the queued patch.

>> +		 */
>> +		if (vlr.state & LR_HW) {
>> +			struct irq_phys_map *map;
>> +			map = vgic_irq_map_search(vcpu, vlr.irq);
>> +
>> +			ret = irq_set_irqchip_state(map->irq,
>> +						    IRQCHIP_STATE_ACTIVE,
>> +						    true);
> I understand the need for manually setting the phys dist state in case
> of timer however for non shared IRQs, GIC does the job directly. But I
> guess it does not harm.

For non-shared interrupts, I'd expect an additional test on map->shared
to avoid hitting the distributor once more.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] arm/arm64: KVM: arch timer: Reset CNTV_CTL to 0

[Christoffer Dall]

Provide a better quality of implementation and be architecture compliant
on ARMv7 for the architected timer by resetting the CNTV_CTL to 0 on
reset of the timer, and call kvm_timer_update_state(vcpu) at the same
time, ensuring the timer output is not asserted after, for example, a
PSCI system reset.

This change alone fixes the UEFI reset issue reported by Laszlo back in
February.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Drew Jones <drjones@redhat.com>
Cc: Wei Huang <wei@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 76e38d2..48c6e1a 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -200,6 +200,14 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 	timer->irq = irq;
 
 	/*
+	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
+	 * and to 0 for ARMv7.  We provide an implementation that always
+	 * resets the timer to be disabled and unmasked and is compliant with
+	 * the ARMv7 architecture.
+	 */
+	timer->cntv_ctl = 0;
+
+	/*
 	 * Tell the VGIC that the virtual interrupt is tied to a
 	 * physical interrupt. We do that once per VCPU.
 	 */
-- 
2.1.2.330.g565301e.dirty

[PATCH 2/2] arm/arm64: KVM: arch timer: Reset CNTV_CTL to 0

[Christoffer Dall]

On Fri, Sep 04, 2015 at 04:24:39PM +0200, Christoffer Dall wrote:
> Provide a better quality of implementation and be architecture compliant
> on ARMv7 for the architected timer by resetting the CNTV_CTL to 0 on
> reset of the timer, and call kvm_timer_update_state(vcpu) at the same
> time, ensuring the timer output is not asserted after, for example, a
> PSCI system reset.

forgot to remove the bit about kvm_timer_update_state(vcpu) which is no
longer valid when these patches are merged before the rework series.

Marc, if you're otherwise happy with this patch, can you fix this up at
commit time?

Thanks,
-Christoffer

> 
> This change alone fixes the UEFI reset issue reported by Laszlo back in
> February.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Drew Jones <drjones@redhat.com>
> Cc: Wei Huang <wei@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/arch_timer.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 76e38d2..48c6e1a 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -200,6 +200,14 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  	timer->irq = irq;
>  
>  	/*
> +	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
> +	 * and to 0 for ARMv7.  We provide an implementation that always
> +	 * resets the timer to be disabled and unmasked and is compliant with
> +	 * the ARMv7 architecture.
> +	 */
> +	timer->cntv_ctl = 0;
> +
> +	/*
>  	 * Tell the VGIC that the virtual interrupt is tied to a
>  	 * physical interrupt. We do that once per VCPU.
>  	 */
> -- 
> 2.1.2.330.g565301e.dirty
> 

[PATCH 2/2] arm/arm64: KVM: arch timer: Reset CNTV_CTL to 0

[Marc Zyngier]

On 04/09/15 15:47, Christoffer Dall wrote:
> On Fri, Sep 04, 2015 at 04:24:39PM +0200, Christoffer Dall wrote:
>> Provide a better quality of implementation and be architecture compliant
>> on ARMv7 for the architected timer by resetting the CNTV_CTL to 0 on
>> reset of the timer, and call kvm_timer_update_state(vcpu) at the same
>> time, ensuring the timer output is not asserted after, for example, a
>> PSCI system reset.
> 
> forgot to remove the bit about kvm_timer_update_state(vcpu) which is no
> longer valid when these patches are merged before the rework series.
> 
> Marc, if you're otherwise happy with this patch, can you fix this up at
> commit time?

No problem, I can fix the commit message on applying the patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 0/2] arm/arm64: KVM: Fix arthictected timer issues

[Marc Zyngier]

Hi Christoffer,

On 04/09/15 15:24, Christoffer Dall wrote:
> These two patches fix two separate issues with the architected timer and
> the corresponding interrupt injection to VMs on KVM/ARM.
> 
> The first patch fixes an issue introduced with the active timer state
> switching series recently merged for v4.3, which could cause a guest to
> loop without progress if another VCPU is run on the same physical CPU
> and preempts the original VCPU while the guest is running the ISR for
> the timer interrupt.
> 
> The second patch resets the architected timer's control register to zero
> on system reset, ensuring that interrupts are not injected when a system
> resets.  This fixes a long-standing issue with UEFI, where soft reset
> initiated from within UEFI prevented the system from booting again.

Thanks for respinning these patches. I've queued them in our -next
queue. I'll send this to Paolo some time next week, once they get some
hammering.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 0/2] arm/arm64: KVM: Fix arthictected timer issues

[Christoffer Dall]

On Fri, Sep 04, 2015 at 04:35:20PM +0100, Marc Zyngier wrote:
> Hi Christoffer,
> 
> On 04/09/15 15:24, Christoffer Dall wrote:
> > These two patches fix two separate issues with the architected timer and
> > the corresponding interrupt injection to VMs on KVM/ARM.
> > 
> > The first patch fixes an issue introduced with the active timer state
> > switching series recently merged for v4.3, which could cause a guest to
> > loop without progress if another VCPU is run on the same physical CPU
> > and preempts the original VCPU while the guest is running the ISR for
> > the timer interrupt.
> > 
> > The second patch resets the architected timer's control register to zero
> > on system reset, ensuring that interrupts are not injected when a system
> > resets.  This fixes a long-standing issue with UEFI, where soft reset
> > initiated from within UEFI prevented the system from booting again.
> 
> Thanks for respinning these patches. I've queued them in our -next
> queue. I'll send this to Paolo some time next week, once they get some
> hammering.
> 
Awesome, fyi I left these patches running in a loop with reboots and
running hackbench on 2 simultaneous VMs with 4 vcpus each on X-gene for
100 iterations, and left the guest and host run cyclictest for ~20
minutes without issues.

-Christoffer