On Fri, Dec 14, 2012 at 10:01 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> 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.
>
that's ok, I just wanted to make sure that we were in fact doing what
we intend to do.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
