Re: [PATCH 3/3] arm/arm64: KVM: Fix disabled distributor operation

[Eric Auger <eric.auger@linaro.org>]

Hi Christoffer,
On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> We currently do a single update of the vgic state when the distrbutor
distributor
> enable/disable control register is accessed and then bypass updating the
> state for as long as the distributor remains disabled.
> 
> This is incorrect, because updating the state does not consider the
> distributor enable bit, and this you can end up in a situation where an
> interrupt is marked as pending on the CPU interface, but not pending on
> the distributor, which is an impossible state to be in, and triggers a
> warning.  Consider for example the following sequence of events:
> 
> 1. An interrupt is marked as pending on the distributor
>    - the interrupt is also forwarded to the CPU interface
> 2. The guest turns off the distributor (it's about to do a reboot)
>    - we stop updating the CPU interface state from now on
> 3. The guest disables the pending interrupt
>    - we remove the pending state from the distributor, but don't touch
>      the CPU interface, see point 2.
> 
> Since the distributor disable bit really means that no interrupts should
> be forwarded to the CPU interface, we modify the code to keep updating
> the internal VGIC state, but always set the CPU interface pending bits
> to zero when the distributor is disabled.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 58b1256..66c6616 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1012,6 +1012,12 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>  	pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
>  	pend_shared = vcpu->arch.vgic_cpu.pending_shared;
>  
> +	if (!dist->enabled) {
> +		bitmap_zero(pend_percpu, VGIC_NR_PRIVATE_IRQS);
> +		bitmap_zero(pend_shared, nr_shared);
> +		return 0;
> +	}
> +
>  	pending = vgic_bitmap_get_cpu_map(&dist->irq_pending, vcpu_id);
>  	enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
>  	bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
> @@ -1039,11 +1045,6 @@ void vgic_update_state(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	int c;
>  
> -	if (!dist->enabled) {
> -		set_bit(0, dist->irq_pending_on_cpu);
> -		return;
I am confused. Don't you want to clear the whole bitmap?

Shouldn't we also handle interrupts programmed in the LR. Spec says any
ack should return a spurious ID. Is it what is going to happen with the
current implementation?

Eric
> -	}
> -
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>  		if (compute_pending_for_cpu(vcpu))
>  			set_bit(c, dist->irq_pending_on_cpu);
>