Re: [PATCH 3/4] arm/arm64: KVM: Fix migration race in the arch timer

[Alex BennÃe]

Marc Zyngier <marc.zyngier@xxxxxxx> writes:

> On Wed, 25 Feb 2015 15:36:21 +0000
> Alex BennÃe <alex.bennee@xxxxxxxxxx> wrote:
>
> Alex, Christoffer,
>
<snip>
>
> So the first half of the patch looks perfectly OK to me...
>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index af6a521..3b4ded2 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -263,6 +263,13 @@ static int vgic_irq_is_queued(struct kvm_vcpu
>> *vcpu, int irq) return vgic_bitmap_get_irq_val(&dist->irq_queued,
>> vcpu->vcpu_id, irq); }
>>  
>> +static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
>> +{
>> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> +
>> +	return vgic_bitmap_get_irq_val(&dist->irq_active,
>> vcpu->vcpu_id, irq); +}
>> +
>>  static void vgic_irq_set_queued(struct kvm_vcpu *vcpu, int irq)
>>  {
>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> @@ -285,6 +292,13 @@ static void vgic_irq_set_active(struct kvm_vcpu
>> *vcpu, int irq) vgic_bitmap_set_irq_val(&dist->irq_active,
>> vcpu->vcpu_id, irq, 1); }
>>  
>> +static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
>> +{
>> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> +
>> +	vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id,
>> irq, 0); +}
>> +
>>  static int vgic_dist_irq_get_level(struct kvm_vcpu *vcpu, int irq)
>>  {
>>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> @@ -634,16 +648,12 @@ bool vgic_handle_cfg_reg(u32 *reg, struct
>> kvm_exit_mmio *mmio, }
>>  
>>  /**
>> - * vgic_unqueue_irqs - move pending IRQs from LRs to the distributor
>> + * vgic_unqueue_irqs - move pending/active IRQs from LRs to the
>> distributor
>>   * @vgic_cpu: Pointer to the vgic_cpu struct holding the LRs
>>   *
>> - * Move any pending IRQs that have already been assigned to LRs back
>> to the
>> + * Move any IRQs that have already been assigned to LRs back to the
>>   * emulated distributor state so that the complete emulated state
>> can be read
>>   * from the main emulation structures without investigating the LRs.
>> - *
>> - * Note that IRQs in the active state in the LRs get their pending
>> state moved
>> - * to the distributor but the active state stays in the LRs, because
>> we don't
>> - * track the active state on the distributor side.
>>   */
>>  void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>>  {
>> @@ -919,7 +929,7 @@ static int compute_pending_for_cpu(struct
>> kvm_vcpu *vcpu) 
>>  /*
>>   * Update the interrupt state and determine which CPUs have pending
>> - * interrupts. Must be called with distributor lock held.
>> + * or active interrupts. Must be called with distributor lock held.
>>   */
>>  void vgic_update_state(struct kvm *kvm)
>>  {
>> @@ -1036,6 +1046,25 @@ static void vgic_retire_disabled_irqs(struct
>> kvm_vcpu *vcpu) }
>>  }
>>  
>> +static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
>> +				 int lr_nr, struct vgic_lr vlr)
>> +{
>> +	if (vgic_irq_is_active(vcpu, irq)) {
>> +		vlr.state |= LR_STATE_ACTIVE;
>> +		kvm_debug("Set active, clear distributor: 0x%x\n",
>> vlr.state);
>> +		vgic_irq_clear_active(vcpu, irq);
>> +		vgic_update_state(vcpu->kvm);
>> +	} else if (vgic_dist_irq_is_pending(vcpu, irq)) {
>> +		vlr.state |= LR_STATE_PENDING;
>> +		kvm_debug("Set pending: 0x%x\n", vlr.state);
>> +	}
>> +
>> +	if (!vgic_irq_is_edge(vcpu, irq))
>> +		vlr.state |= LR_EOI_INT;
>> +
>> +	vgic_set_lr(vcpu, lr_nr, vlr);
>> +}
>> +
>>  /*
>>   * Queue an interrupt to a CPU virtual interface. Return true on
>> success,
>>   * or false if it wasn't possible to queue it.
>> @@ -1063,8 +1092,7 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8
>> sgi_source_id, int irq) if (vlr.source == sgi_source_id) {
>>  			kvm_debug("LR%d piggyback for IRQ%d\n", lr,
>> vlr.irq); BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
>> -			vlr.state |= LR_STATE_PENDING;
>> -			vgic_set_lr(vcpu, lr, vlr);
>> +			vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
>>  			return true;
>>  		}
>>  	}
>> @@ -1081,11 +1109,8 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8
>> sgi_source_id, int irq) 
>>  	vlr.irq = irq;
>>  	vlr.source = sgi_source_id;
>> -	vlr.state = LR_STATE_PENDING;
>> -	if (!vgic_irq_is_edge(vcpu, irq))
>> -		vlr.state |= LR_EOI_INT;
>> -
>> -	vgic_set_lr(vcpu, lr, vlr);
>> +	vlr.state = 0;
>> +	vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
>>  
>>  	return true;
>>  }
>
>
> ... but this whole vgic rework seems rather out of place, and I can't
> really see its connection with the timer. Isn't it logically part of the
> previous patch?

Probably - I was going to re-factor that code with the original patch
but it was on the todo list once we had it working. Christoffer than
cleaned it up when he fixed the race hence it being here.

Would you like it as a separate patch (between 2 and 3) or just rolled
into the previous patch?

>
> Thanks,
>
> 	M.

-- 
Alex BennÃe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/