diff mbox

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

Message ID 1441376679-8341-2-git-send-email-christoffer.dall@linaro.org
State Superseded
Headers show

Commit Message

Christoffer Dall Sept. 4, 2015, 2:24 p.m. UTC
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(-)

Comments

Auger Eric Sept. 7, 2015, 2:44 p.m. UTC | #1
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)
>
Auger Eric Sept. 7, 2015, 3:46 p.m. UTC | #2
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)
>>
>
Marc Zyngier Sept. 7, 2015, 3:54 p.m. UTC | #3
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.
diff mbox

Patch

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)