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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
