Re: [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

From: Julien Grall <julien.grall@linaro.org>
To: Stefano Stabellini <sstabellini@kernel.org>,
	Andre Przywara <andre.przywara@arm.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()
Date: Thu, 26 Oct 2017 09:22:30 +0100	[thread overview]
Message-ID: <73012469-f5b2-b665-bd7d-116f32408606@linaro.org> (raw)
In-Reply-To: <alpine.DEB.2.10.1710251714370.574@sstabellini-ThinkPad-X260>

Hi,

On 10/26/2017 01:19 AM, Stefano Stabellini wrote:
> On Thu, 19 Oct 2017, Andre Przywara wrote:
>> gic_remove_irq_from_queues() was not only misnamed, it also has the wrong
>> abstraction, as it should not live in gic.c.
>> Move it into vgic.c and vgic.h, where it belongs to, and rename it on
>> the way.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Yes, gic_remove_irq_from_queues could be in the vgic.
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> One comment about cosmetics below.
> 
> 
>> ---
>>   xen/arch/arm/gic.c         |  9 ---------
>>   xen/arch/arm/vgic-v3-its.c |  4 ++--
>>   xen/arch/arm/vgic.c        | 11 ++++++++++-
>>   xen/include/asm-arm/gic.h  |  1 -
>>   xen/include/asm-arm/vgic.h |  1 +
>>   5 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 75b2e0e0ca..ef041354ea 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -411,15 +411,6 @@ void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
>>       list_del_init(&p->lr_queue);
>>   }
>>   
>> -void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
>> -{
>> -    ASSERT(spin_is_locked(&v->arch.vgic.lock));
>> -
>> -    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
>> -    list_del_init(&p->inflight);
>> -    gic_remove_from_lr_pending(v, p);
>> -}
>> -
>>   void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
>>   {
>>       struct pending_irq *n = irq_to_pending(v, virtual_irq);
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 72a5c70656..d8fa44258d 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -381,7 +381,7 @@ static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr)
>>        * have no active state, we don't need to care about this here.
>>        */
>>       if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>> -        gic_remove_irq_from_queues(vcpu, p);
>> +        vgic_remove_irq_from_queues(vcpu, p);
>>   
>>       spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
>>       ret = 0;
>> @@ -619,7 +619,7 @@ static int its_discard_event(struct virt_its *its,
>>       }
>>   
>>       /* Cleanup the pending_irq and disconnect it from the LPI. */
>> -    gic_remove_irq_from_queues(vcpu, p);
>> +    vgic_remove_irq_from_queues(vcpu, p);
>>       vgic_init_pending_irq(p, INVALID_LPI);
>>   
>>       spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 451a306a98..cd50b90d67 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -281,7 +281,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>>       /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
>>       if ( !list_empty(&p->lr_queue) )
>>       {
>> -        gic_remove_irq_from_queues(old, p);
>> +        vgic_remove_irq_from_queues(old, p);
>>           irq_set_affinity(p->desc, cpumask_of(new->processor));
>>           spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>>           vgic_vcpu_inject_irq(new, irq);
>> @@ -510,6 +510,15 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>>       spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>   }
>>   
>> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
>> +{
>> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
>> +
>> +    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
>> +    list_del_init(&p->inflight);
>> +    gic_remove_from_lr_pending(v, p);
>> +}
>> +
>>   void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>>   {
>>       uint8_t priority;
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 2f248301ce..030c1d86a7 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -243,7 +243,6 @@ extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
>>           unsigned int priority);
>>   extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
>>   extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p);
>> -extern void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
>>   
>>   /* Accept an interrupt from the GIC and dispatch its handler */
>>   extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index e489d0bf21..8d0ff65708 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -204,6 +204,7 @@ extern int vcpu_vgic_init(struct vcpu *v);
>>   extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
>>   extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
>>   extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
>> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> 
> cosmetic: you might as well add an extern

Or just dropped extern from the others. The keyword is pointless.

> 
> 
>>   extern void vgic_clear_pending_irqs(struct vcpu *v);
>>   extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
>>   extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel