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 16:30, Julien Grall wrote:
> 
> 
> On 02/26/2018 04:25 PM, Andre Przywara wrote:
>> 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?

The two interfaces used for that purpose are different in the two VGICs:
- The old VGIC only works on the current VCPU, since it peeks into the
GICH_ register to learn the priority (regardless of whether this is
really needed or useful).
- The new VGIC can use this function for any VCPU, and we need this
functionality later on (when we iterate over all VCPUs).
So we can't use a function hardwiring "current", that would break
vgic_kick_vcpus() in the new VGIC. And we can't pass a VCPU parameter,
that would not work for the old VGIC.
So I believe having this small wrapper here is the easiest solution.
I will add a patch to rename this function to vgic_pending_irq(),
though, so this one here looks like:
int vgic_pending_irq(void)
{
    return vgic_vcpu_pending_irq(current);
}

We can clean this up when the old VGIC gets removed.

Cheers,
Andre.

>> Not really, but I am a bit reluctant to change too much original Xen
>> code, don't want to step on anyone's toes ;-)
> 
> The original code is going to get kill at some point. So better use name
> that makes sense in the new context. It is quite similar to the
> gic_inject change.
> 
>>
>> But if that's fine with you, I am OK with the renaming - though it adds
>> yet another patch ;-)
> 
> The end goal is a better world, so the number of patches does not matter
> here :).
> 
> If you put them at the beginning, we can merge them right away.
> 
> Cheers,
> 

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