Re: [Xen-devel] [RFC PATCH 26/49] ARM: new VGIC: Implement vgic_vcpu_pending_irq

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 26/02/18 15:55, Julien Grall wrote:
> Hi,
> 
> On 02/26/2018 03:29 PM, Andre Przywara wrote:
>> On 13/02/18 16:35, Julien Grall wrote:
>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>>>> index f4f2a04a60..9e7fb1edcb 100644
>>>> --- a/xen/arch/arm/vgic/vgic.c
>>>> +++ b/xen/arch/arm/vgic/vgic.c
>>>> @@ -646,6 +646,38 @@ void gic_inject(void)
>>>>        vgic_restore_state(current);
>>>>    }
>>>>    +static int vgic_vcpu_pending_irq(struct vcpu *vcpu)
>>>> +{
>>>> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>> +    struct vgic_irq *irq;
>>>> +    bool pending = false;
>>>> +    unsigned long flags;
>>>> +
>>>> +    if ( !vcpu->domain->arch.vgic.enabled )
>>>> +        return false;
>>>> +
>>>> +    spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
>>>> +
>>>> +    list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list)
>>>> +    {
>>>> +        spin_lock(&irq->irq_lock);
>>>> +        pending = irq_is_pending(irq) && irq->enabled;
>>>> +        spin_unlock(&irq->irq_lock);
>>>> +
>>>> +        if ( pending )
>>>> +            break;
>>>> +    }
>>>> +
>>>> +    spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
>>>> +
>>>> +    return pending;
>>>> +}
>>>> +
>>>> +int gic_events_need_delivery(void)
>>>
>>> You probably want to rename that function or just expose
>>> vgic_vcpu_pending_irq().
>>
>> Rename to what? I need both functions: vgic_vcpu_pending_irq() is also
>> called by vgic_kick_vcpus() (later in the series).
>> And gic_events_need_delivery(void) is the interface that the arch code
>> expects. Shall I rename this there? To what?
> 
> Let me start with it is a bit odd to have a function name 'gic_*' in the
> virtual GIC code. So at least renaming to vgic_events_need_delivery
> would be an improvement.
> 
> Regarding the interface itself, it is ARM specific and not set in stone.
> It would not be too bad to use vgic_vcpu_pending_irq(current). Is there
> any reason for not doing that?

Not really, but I am a bit reluctant to change too much original Xen
code, don't want to step on anyone's toes ;-)

But if that's fine with you, I am OK with the renaming - though it adds
yet another patch ;-)

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel