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

[Alex BennÃe]

Marc Zyngier <marc.zyngier@xxxxxxx> writes:

> 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.

Done, new series re-sent.

>
> 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/