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

[Marc Zyngier]

On 02/03/15 08:50, Alex BennÃe wrote:
> 
> 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?

In general, I prefer smaller patches (keeps the few brain cells left
from overheating), so if these changes make sense on their own, please
post them as a separate patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
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/