On 14/12/12 14:57, Christoffer Dall wrote:
> On Fri, Dec 14, 2012 at 5:20 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>> On 13/12/12 21:52, Christoffer Dall wrote:
>>>
>>>
>>> On Thursday, December 6, 2012, Marc Zyngier wrote:
>>>
>>> Shuffle things around to make handling of level interrupts less
>>> messy:
>>> - encapsulate irq_active in accessors
>>> - introduce vgic_queue_{sgi,hwirq} functions
>>> - vgic_is_edge now takes a vcpu instrad of a distributor
>>>
>>>
>>> instead
>>>
>>>
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx <javascript:;>>
>>> ---
>>> arch/arm/kvm/vgic.c | 172
>>> ++++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 100 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
>>> index 11a4416..d666df7 100644
>>> --- a/arch/arm/kvm/vgic.c
>>> +++ b/arch/arm/kvm/vgic.c
>>> @@ -154,9 +154,11 @@ static u32 *vgic_bytemap_get_reg(struct
>>> vgic_bytemap *x, int cpuid, u32 offset)
>>> #define VGIC_CFG_LEVEL 0
>>> #define VGIC_CFG_EDGE 1
>>>
>>> -static bool vgic_irq_is_edge(struct vgic_dist *dist, int irq)
>>> +static bool vgic_irq_is_edge(struct kvm_vcpu *vcpu, int irq)
>>> {
>>> - return vgic_bitmap_get_irq_val(&dist->irq_cfg, 0, irq) ==
>>> VGIC_CFG_EDGE;
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> +
>>> + return vgic_bitmap_get_irq_val(&dist->irq_cfg,
>>> vcpu->vcpu_id, irq) == VGIC_CFG_EDGE;
>>> }
>>>
>>> static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
>>> @@ -166,6 +168,31 @@ static int vgic_irq_is_enabled(struct kvm_vcpu
>>> *vcpu, int irq)
>>> return vgic_bitmap_get_irq_val(&dist->irq_enabled,
>>> 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_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, 1);
>>> + atomic_inc(&vcpu->arch.vgic_cpu.irq_active_count);
>>> + smp_mb__after_atomic_inc();
>>> +}
>>> +
>>> +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);
>>> + atomic_dec(&vcpu->arch.vgic_cpu.irq_active_count);
>>> + smp_mb__after_atomic_dec();
>>>
>>>
>>> uf, could we have a comment explaining these memory barriers?
>>>
>>> Specifically, what is the change after the decrement, which must be
>>> perceived after the decrement from another cpu?
>>
>> There is a long thread about this:
>> https://patchwork.kernel.org/patch/1724141/
>
> I know, but you can't expect future readers of this code to go and
> find that thread to understand that memory barrier. I feel like we
> should have a comment with every use of memory barriers explaining why
> it's there, unless it's braindead obvious, which it never is.
>
>>
>> and a subsequent patch removes completely the feature.
>>
>
> funny ordering of the patches then
Basically the ordering of the review. I'll repost a series with this bit
removed.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
